Promote shifts across blocks in reconstruct_fallible_operations#1328
Promote shifts across blocks in reconstruct_fallible_operations#1328MavenRain wants to merge 2 commits into
Conversation
The shift in `out[i] = x >> y` with an overloaded `IndexMut` ends up in the block the `index_mut` call returns to, so `reconstruct_fallible_operations` does not find it, leaving the shift as `wrap.>>` behind a leftover overflow check. See issue AeneasVerif#1041. Signed-off-by: Onyeka Obi <softwareengineerasaservant@isurvivable.cv>
Rustc evaluates the operands of a shift (and emits its overflow check) before computing the destination place. When that computation involves a function call, e.g. `out[i] = x >> y` through an overloaded `IndexMut`, the call terminator separates the check from the shift, so the per-block pattern match could not find the shift and the operation stayed `wrap.>>`. Record such checks during the per-block pass and resolve them afterwards by following the goto/call chain, traversing only single-predecessor blocks so the shift cannot be reached without passing the check, and promote the first still-wrapping shift by the checked amount. Give up conservatively if the checked operand could be modified along the way. The check statements are deliberately kept: removing them would move the failure point past the interposed call's side effects (and past its potential divergence), and keeping them makes the resolution robust even if two shifts by the same constant amount were to cross-match, since promoting a checked shift never changes behavior. Fixes AeneasVerif#1041 Signed-off-by: Onyeka Obi <softwareengineerasaservant@isurvivable.cv>
|
Hi! Thank you for your contribution. Would you mind re-writing your PR description yourself? It is currently decent but still full of unnecessary details that LLMs love to add. Then, instead of leaving the check, I would like to remove the check and do the arithmetic computation early instead (and store it in a fresh variable). That's always correct since the only side-effects are the panic and the memory accesses, all of which are already done by the overflow check. Also your solution is doing too much work to be correct compared to what we need: we don't need to follow the graph structure since these asserts are only ever emitted by the compiler: let's simply look for all the uses the given local in the body. Also I'd like to handle the shift checks in a single pass: iterate over the whole body looking for shifts that use the operand in question, and replace them all. To know where to insert the original computation, record the |
Fixes #1041.
Root cause
The issue suggested the promotion pattern fails to match shifts assigned through a dereferenced pointer, but the pattern never inspects the assignment destination at all (
make_binop_overflow_panicvisits everyRvalue). The actual mechanism: rustc evaluates the shift operands, and emits the overflow check, before computing the destination place. When that computation involves a function call, as with the overloadedIndexMutin the MWE, the call is a block terminator in ULLBC, so the deferred shift lands in the block the call returns to:remove_dynamic_checkspattern-matches within a single statement sequence, never finds the shift, and conservatively leaves the wrapping shift, which is what Aeneas then extracts.The fix
The two shift arms now record a
PendingShiftCheck(block, checked amount operand) when the shift is not in the same block. After the per-block sweep,resolve_pending_shift_checkfollows theGoto/Callchain out of the block, traversing only single-predecessor blocks so the shift cannot be reached without passing the check, and promotes the first still-wrapping shift by the checked amount (reusingmake_binop_overflow_panic). The walk gives up if the checked operand is not a constant or a plain local, or if the call destination or any statement scanned before the shift could touch that local. Pending checks are only recorded forOverflowasserts, so the bounds checks that share the second pattern never trigger a cross-block walk.Unlike the per-block path, the check statements are deliberately NOT removed here, for two reasons:
index_mutthat loops forever on some input would turn a must-panic execution into a non-terminating one).o8[{ o64[0] = x >> N; 0 }] = w >> N;withW64/W8wrappers): both shifts get promoted and both checks stay.The leftover assert is redundant next to the promoted shift, but harmless to consumers. If you would rather have the assert removed for output cleanliness, that requires deciding the two points above; happy to iterate.
Testing
issue-1041-shift-through-index-mut.rswith the issue's MWE plus a variable shift amount (exercises both the cast and no-cast check shapes, and the local-operand tracking). Both now producepanic.>>/panic.<<.charontest suite passes; no other test output changed, so the blast radius is exactly the issue's case.cargo clippyclean,cargo fmtclean.Drafted with AI assistance; the analysis, code, and tests were reviewed and run by me.