overflowing-checking for rhs of shift operators #23536
Conversation
|
r? @Aatch (rust_highfive has picked a reviewer for you, use r? to override) |
| { | ||
| let to_llty = |r| bcx.ccx().tn().type_to_string(Type::from_ref(r)); | ||
| { | ||
| let given_llty = type_of::type_of(bcx.ccx(), rhs_t); |
There was a problem hiding this comment.
ah, this whole sub-block is debugging code that is no longer needed; I will remove it.
|
One of the tests in this PR uncovered #23551, so don't attempt to merge this until that has a fix (or until I revise the test to sidestep that bug). |
This includes a slight refactoring of the `cast_shift_rhs` and related functions in `trans::base`, so that I can call them from much later in the compiler's control flow (so that we can clearly dilineate where automatic conversions of the RHS occur, versus where we check it). The rhs-checking and fallback-masking is generalized to 8- and 16-bit values, and the fallback-masking is turned on unconditionally. Fix rust-lang#10183. Is this a [breaking-change]? I would argue it is not; it only adds a strict definition to what was previously undefined behavior; however, there might be code that was e.g. assuming that `1_i8 << 17` yields 0. (This happens in certain contexts and at certain optimization levels.)
Note the tests have been revised to match new semantics for 8- and 16-bit values.
pnkfelix
force-pushed
the
arith-oflo-shifts
branch
from
1beaee8
to
61ff823
Compare
|
Okay, after discovering that #23551 was a duplicate of #10183 and reading the discussion there further, I have decided to go with @glaebhoerl's advice and ensure that we do not encounter undefined behavior due to the shift's RHS. This PR has now been updated accordingly, and is ready for review/merge. |
|
r+ -- This looks good to me. I had two questions though so not informing bors just yet (though no action is needed or requested necessarily). |
|
@nikomatsakis constant evaluation is not yet addressed. I don't know whether we have a reliable way to force the debug-assertions off, though that sounds like a good thing to have if we do not already have it. I will investigate. (Still, we probably can land this and leave those as to-do items. In particular, we need to deal with const-evaluation for every operation; not just shifts, and that still remains to be done.) |
|
⌛ Testing commit 61ff823 with merge c9798cb... |
|
💔 Test failed - auto-mac-64-opt |
|
odd, thought I had done a full test ... or no, maybe I only did |
|
(the bug I'm referring to in the commit is #20937 ) |
…atsakis overflow-checking for rhs of shift operators Subtask of rust-lang#22020 ([RFC 560](https://github.com/rust-lang/rfcs/blob/master/text/0560-integer-overflow.md))
|
⌛ Testing commit 5e47c66 with merge 1c9b5a6... |
|
💔 Test failed - auto-mac-64-nopt-t |
|
💔 Test failed - auto-win-32-nopt-t |
hmm, I guess I need to pass Update: Oh, it also needs a 32-bit build. Hmm. |
|
@bors r=nikomatsakis bb9d210 |
1 similar comment
|
@bors r=nikomatsakis bb9d210 |
|
🙀 |
|
🙀 |
pnkfelix
force-pushed
the
arith-oflo-shifts
branch
from
7623b75
to
bb9d210
Compare
|
@bors r=nikomatsakis bb9d210 |
|
@bors p=1 |
|
⌛ Testing commit bb9d210 with merge 9c638f8... |
|
⛄ The build was interrupted to prioritize another pull request. |
|
⌛ Testing commit bb9d210 with merge b93456c... |
|
⛄ The build was interrupted to prioritize another pull request. |
|
⌛ Testing commit bb9d210 with merge 28a0b25... |
overflow-checking for rhs of shift operators Subtask of #22020 ([RFC 560](https://github.com/rust-lang/rfcs/blob/master/text/0560-integer-overflow.md))
overflow-checking for rhs of shift operators Subtask of rust-lang#22020 ([RFC 560](https://github.com/rust-lang/rfcs/blob/master/text/0560-integer-overflow.md))
|
💔 Test failed - auto-win-32-nopt-t |
|
well, here's hoping the rollup suceeds where this latest merge attempt failed... |
|
@bors retry |
|
⚡ Previous build results for auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt are reusable. Rebuilding only auto-win-32-nopt-t... |
overflow-checking for rhs of shift operators
Subtask of #22020 (RFC 560)