Bug in my code from compiler optimization [video]
youtube.com
youtube.com
He wants code:
If x then return y
With y an unsafe expression, and x always true if y can be evaluated without invoking undefined behaviour.He wrote it as:
return x.then_some(y)
This always calculates y, but throws it away if x is false.By calculating y, he accidentaly gives a proof to the compiler that x must be true. The proof is of course invalid, but as y is unsafe, the compiler trusts him nevertheless. So the 'if' gets optimized away.
Sounds like a great learning experience imo.
If n!=0 return 1/n
Is well behaved: Division by zero wont happen. z=1/n
If n!=0 return z
Is not well behaved. It will crash when n=0. What the compiler does is take the existence of z as a proof that n can't be 0, and optimize the if away: z=1/n
return z
Just like division by 0, undefined behaviour (UB) is not allowed. The difference is: UB might crash or give a nonsensical result. You can't tell beforehand. Different versions of a compiler might make different choices of crash vs nonsense. Until recently, the author calculated nonsense, and threw it away, but that was accidental. It might have crashed too.C had a lot of UB that gets thrown away, until some minor shift in optimizations changes it to a crasher. Compare it to an insanely sharp knife: Powerfull, but the slightest error might take off your fingers, it's very hard to know if something is or is not an error, and errors mostly dont cut if your fingers, until surprise surprise they do.
Rust is much better: only unsafe code behaves like C's crazy knife. Only when you use unsafe, the rust guarantees go out of the window, because that's what the programmer explicitly asked.
Instead, compilers seemingly try to make insights into the intent or true meaning of your code, often inaccurately, and change it using ad-hoc unintuitive rules as justification (those being the rules of undefined behavior, and what assumptions the authors are and aren't allowed to make about your code). I know they aren't actually reading your intent; it's just the result of many, many passes that each make seemingly reasonable assumptions, which together may add up to a set of unreasonable ones.
> because that's what the programmer explicitly asked.
No, the programmer didn't explicitly ask for their check to be deleted, even with "unsafe" written. There is no request written anywhere. At most, you can say they implied they wanted it by invoking undefined behavior. Probably, though, they just made a mistake, or assumed that behavior was defined when it wasn't (e.g. they may have thought division corresponds to a hardware division instruction, modulo optimizations like right shifting for division by 2, constant folding division by 1, etc.)
https://gist.github.com/rygorous/e0f055bfb74e3d5f0af20690759...
This is a weird complaint to make here, where the compiler optimization isn't introducing a bug. If the check were left in place, the code would have exactly the same problem that it does without the check.
The optimization is exactly what you hope for: something that makes the code faster without affecting its behavior in any way. Why is that bad?
> By calculating y, he accidentally gives a proof to the compiler that x must be true.
But the actual example is written to hide this problem. It should make clear that "y" is actually an expression that depends on x, something like "return x.then_some(x.field)". With that example, it would be immediately clear why evaluating x.field before checking x was a bug.
Edit: it turns out there’s also a .then() that does exactly that.
No, the compiler can't make that assumption for an if statement. That's because the condition of an if statement is always evaluated first, so the following expression(s) are only conditionally evaluated. There's nothing the compiler can do to reason "backwards" in such a case.
then_some effectively flips the order around - the consequent of the "if" statement is evaluated first, and then the condition is evaluated. This reversal of the order makes all the difference.
For a more concrete example, consider the following snippets:
if (ptr != nullptr) {
return *ptr;
} else {
return nullptr;
}
This is (probably) safe. The condition is evaluated first, and only if the condition is true is *ptr evaluated. The buggy code, on the other hand, was more like (ptr != nullptr).then_some(*ptr)
In this case, the argument *ptr is evaluated first, before the condition. This is sort of equivalent to: result = *ptr
if (ptr != nullptr) {
return result;
} else {
return nullptr;
}
And that is definitely buggy, as it unconditionally dereferences ptr.There isn't really a compiler bug here; it's just language semantics coupled with unsafe.
I am not sure about Rust, but gcc will definitely elide an if check if it detects pointer dereference inside the if block. The reasoning was that if a pointer is being dereferenced, it must be non null. Because if it is null that would be undefined behavior and compiler is allowed to do whatever wants in that case anyway. So
if (ptr != NULL) {
val = *ptr
// do something with val
}
becomes // null check removed
val = *ptr
// do something with val
So no, the condition is not guaranteed to be evaluated first, or even evaluated at all. I would be surprised if Rust compiler is not doing these sorts of optimisations.The compiler will only optimize the if away if you already potentially invoked UB. Like this:
val = *ptr; // May be *NULL
if (*ptr != NULL) ...Only if you dereferenced the pointer before the if-statement.
As others have stated, this is almost certainly incorrect.
> The reasoning was that if a pointer is being dereferenced, it must be non null.
The key here is "if a pointer is being dereferenced". In that case the pointer is only dereferenced if the condition evaluates to true and does nothing otherwise. The optimizer would be incorrect to turn that into an unconditional dereference.
¹(but there has to be a crate for this)
unsafe in rust is a blank cheque for the compiler. Doing anything complicated in it is almost guaranteed to have UB somewhere. You decide to disable safety.
If possible, write the safe but tedious code. Then measure. Only when you're sure unsafe is worth it should you take out the big cannon. Write a km of comments proving this code needs and deserves unsafe. Writing the tedious code is less time consuming than debugging the mess.
I wish the video author documented why he choose unsafe here. Was it measured and worth it in an older version of rust? Or a folly of youth (which is fine, as long as you learn from it, been there done that paid the time debugging my stupidity)
num-traits' FromPrimitive https://docs.rs/num-traits/latest/num_traits/cast/trait.From...
https://doc.rust-lang.org/beta/std/primitive.bool.html#metho...
> Arguments passed to then_some are eagerly evaluated; if you are passing the result of a function call, it is recommended to use then, which is lazily evaluated.
[1] https://rust-lang.github.io/rust-clippy/master/index.html#/o...