-
Notifications
You must be signed in to change notification settings - Fork 13.3k
LLVM addition optimizations default to wrapping_add instead of saturating_add #114888
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
You should be able to use alive2 to verify if a desired transformation is correct. |
Not sure what the bug is here. You are pointing to a part of the assembly output. What's up with that? In a bug report, please always clearly state the behavior you see and the behavior you would have expected instead. Are you saying LLVM should have optimized this code more? |
Why would the compiler assume overflow can't happen? |
Right, good point. If you add |
Because a language can make anything UB and by definition something being UB means the compiler can assume it won't happen. @RalfJung my question is if overflow is UB. Seems implicitly answered by the fact that it's on this page: https://doc.rust-lang.org/reference/behavior-not-considered-unsafe.html#integer-overflow. But there's no explicit discussion of UB. If it's not, it would be nice to have an explanation for why that was chosen: seems weird to crash on debug mode on overflow but then say the compiler can't optimize on that assumption? |
That page lists two possible behaviors that the implementation can choose, and neither of them is UB:
I think that's explicit enough. We don't want to imply that things are allowed to be UB unless there's a clause specifically saying they aren't. It's not discussed further because addition is safe, so allowing it to cause UB is not on the table. |
Rational for that decision (or a link to such documentation) would be helpful. Either way, this answers my question. |
Rationale is that addition is safe so UB was never an option. Making addition unsafe is so obviously undesirable that I don't think it needs much explanation?
We have unchecked_add operations that are UB on overflow (https://doc.rust-lang.org/std/primitive.u32.html#method.unchecked_add). That is totally in line with Rust's philosophy: provide the dangerous option unsafely as an opt-in for when it is really needed. I think we should consider stabilizing these.
|
Sure, that makes sense. I guess I'm a little confused about why these are in a gray zone: the compiler can't assume ops won't overflow, but it also won't check that for you. So if you overflow an allocation capacity for example, you'll almost certainly end up with UB in release mode which means you need to use checked_add and panic on overflow. So neither the programmer nor the compiler may assume arithmetic won't overflow. I assume the rationale is that inserting checks into release mode is just too slow? |
Rust guarantees no UB even in release mode. Every access to the allocation is bounds-checked after all.
Yes. It's not so much the actual checks (is my understanding) but the fact that they break up control flow, which makes it much harder for the compiler to move code around. rust-lang/rfcs#2635 contains some interesting discussion regarding a proposal to enable overflow checking in release mode at least for some operations.
It is definitely a bug to have overflowing addition. So any Rust verification or sanitization tool can flag overflows. However, given how easy it is to accidentally overflow, UB is just too harsh of a penalty for getting this wrong, and hence we don't have the compiler assume no-overflow. |
Thank you! These are the details that were missing. |
I tried this code:
Using
*count = count.saturating_add(1);
instead removes the panic.cc @RalfJung to confirm that assuming overflow cannot happen is a valid optimization.
The text was updated successfully, but these errors were encountered: