Skip to content

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Mar 14, 2017

fixes #151

@solson
Copy link
Contributor

solson commented Mar 14, 2017

Hmm, but I think we are supposed to always mask the shift width...

Compare the Debug and Release mode MIR and LLVM IR here: https://is.gd/LglRp7

Debug mode uses CheckedShl and an assert MIR statement on the overflow result, while Release uses a plain Shl. Both do a shift width mask in the LLVM IR, but only Debug mode can panic (from the assert).

So I think we need to leave the code for Shl alone and change something for CheckedShl?

@solson
Copy link
Contributor

solson commented Mar 14, 2017

Oh, I guess this code handles both Shl and CheckedShl. Maybe we just need to adjust the cases where the overflow result is true to make sure the assert is triggered.

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 14, 2017

In debug mode, the mir shows a concrete assert. In the release mode, we still forward and the masking is done by the forwarded to function. The masking was entirely redundant

@solson
Copy link
Contributor

solson commented Mar 14, 2017

Ohhh. In release mode we still use the overflowing_shl intrinsic function and that does the masking?

@solson
Copy link
Contributor

solson commented Mar 14, 2017

Huh, TIL the masking for overflowing_shl is done in std code, not via an intrinsic: https://doc.rust-lang.org/stable/src/core/num/mod.rs.html#1092-1094

@solson solson merged commit a82924b into rust-lang:master Mar 14, 2017
@oli-obk oli-obk deleted the intrinsics branch March 17, 2017 09:34
erickt pushed a commit to erickt/miri that referenced this pull request Feb 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bitshift overflow doesn't panic
2 participants