Skip to content

Commit

Permalink
Rollup merge of rust-lang#102045 - RalfJung:const-prop-regression-fix…
Browse files Browse the repository at this point in the history 
…, r=oli-obk

fix ConstProp handling of written_only_inside_own_block_locals

Fixes a regression introduced by rust-lang#100239, which adds an early return and thus skips some code in `visit_terminator` that must be run for soundness.

Fixes rust-lang#101973
  • Loading branch information
@Dylan-DPC
Dylan-DPC authored Sep 21, 2022
2 parents 4b7c596 + 7373788 commit 7d7f555
Show file tree
Hide file tree
Showing 3 changed files with 140 additions and 20 deletions.
40 changes: 20 additions & 20 deletions compiler/rustc_mir_transform/src/const_prop.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1066,32 +1066,32 @@ impl<'tcx> MutVisitor<'tcx> for ConstPropagator<'_, 'tcx> {
let source_info = terminator.source_info;
self.source_info = Some(source_info);
self.super_terminator(terminator, location);
// Do NOT early return in this function, it does some crucial fixup of the state at the end!
match &mut terminator.kind {
TerminatorKind::Assert { expected, ref mut cond, .. } => {
if let Some(ref value) = self.eval_operand(&cond) {
trace!("assertion on {:?} should be {:?}", value, expected);
let expected = Scalar::from_bool(*expected);
let Ok(value_const) = self.ecx.read_scalar(&value) else {
// FIXME should be used use_ecx rather than a local match... but we have
// quite a few of these read_scalar/read_immediate that need fixing.
return
};
if expected != value_const {
// Poison all places this operand references so that further code
// doesn't use the invalid value
match cond {
Operand::Move(ref place) | Operand::Copy(ref place) => {
Self::remove_const(&mut self.ecx, place.local);
// FIXME should be used use_ecx rather than a local match... but we have
// quite a few of these read_scalar/read_immediate that need fixing.
if let Ok(value_const) = self.ecx.read_scalar(&value) {
if expected != value_const {
// Poison all places this operand references so that further code
// doesn't use the invalid value
match cond {
Operand::Move(ref place) | Operand::Copy(ref place) => {
Self::remove_const(&mut self.ecx, place.local);
}
Operand::Constant(_) => {}
}
} else {
if self.should_const_prop(value) {
*cond = self.operand_from_scalar(
value_const,
self.tcx.types.bool,
source_info.span,
);
}
Operand::Constant(_) => {}
}
} else {
if self.should_const_prop(value) {
*cond = self.operand_from_scalar(
value_const,
self.tcx.types.bool,
source_info.span,
);
}
}
}
Expand Down
20 changes: 20 additions & 0 deletions src/test/mir-opt/issue-101973.rs
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// compile-flags: -O -C debug-assertions=on
// This needs inlining followed by ConstProp to reproduce, so we cannot use "unit-test".

#[inline]
pub fn imm8(x: u32) -> u32 {
let mut out = 0u32;
out |= (x >> 0) & 0xff;
out
}

// EMIT_MIR issue_101973.inner.ConstProp.diff
#[inline(never)]
pub fn inner(fields: u32) -> i64 {
imm8(fields).rotate_right(((fields >> 8) & 0xf) << 1) as i32 as i64
}

fn main() {
let val = inner(0xe32cf20f);
assert_eq!(val as u64, 0xfffffffff0000000);
}
100 changes: 100 additions & 0 deletions src/test/mir-opt/issue_101973.inner.ConstProp.diff
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
- // MIR for `inner` before ConstProp
+ // MIR for `inner` after ConstProp

fn inner(_1: u32) -> i64 {
debug fields => _1; // in scope 0 at $DIR/issue-101973.rs:+0:14: +0:20
let mut _0: i64; // return place in scope 0 at $DIR/issue-101973.rs:+0:30: +0:33
let mut _2: i32; // in scope 0 at $DIR/issue-101973.rs:+1:5: +1:65
let mut _3: u32; // in scope 0 at $DIR/issue-101973.rs:+1:5: +1:58
let mut _4: u32; // in scope 0 at $DIR/issue-101973.rs:+1:5: +1:17
let mut _5: u32; // in scope 0 at $DIR/issue-101973.rs:+1:10: +1:16
let mut _6: u32; // in scope 0 at $DIR/issue-101973.rs:+1:31: +1:57
let mut _7: u32; // in scope 0 at $DIR/issue-101973.rs:+1:31: +1:52
let mut _8: u32; // in scope 0 at $DIR/issue-101973.rs:+1:32: +1:45
let mut _9: u32; // in scope 0 at $DIR/issue-101973.rs:+1:33: +1:39
let mut _10: (u32, bool); // in scope 0 at $DIR/issue-101973.rs:+1:32: +1:45
let mut _11: (u32, bool); // in scope 0 at $DIR/issue-101973.rs:+1:31: +1:57
scope 1 (inlined imm8) { // at $DIR/issue-101973.rs:14:5: 14:17
debug x => _5; // in scope 1 at $DIR/issue-101973.rs:5:13: 5:14
let mut _12: u32; // in scope 1 at $DIR/issue-101973.rs:7:12: 7:27
let mut _13: u32; // in scope 1 at $DIR/issue-101973.rs:7:12: 7:20
let mut _14: u32; // in scope 1 at $DIR/issue-101973.rs:7:13: 7:14
let mut _15: (u32, bool); // in scope 1 at $DIR/issue-101973.rs:7:12: 7:20
scope 2 {
debug out => _4; // in scope 2 at $DIR/issue-101973.rs:6:9: 6:16
}
}
scope 3 (inlined core::num::<impl u32>::rotate_right) { // at $DIR/issue-101973.rs:14:5: 14:58
debug self => _4; // in scope 3 at $SRC_DIR/core/src/num/uint_macros.rs:LL:COL
debug n => _6; // in scope 3 at $SRC_DIR/core/src/num/uint_macros.rs:LL:COL
let mut _16: u32; // in scope 3 at $SRC_DIR/core/src/num/uint_macros.rs:LL:COL
let mut _17: u32; // in scope 3 at $SRC_DIR/core/src/num/uint_macros.rs:LL:COL
}

bb0: {
StorageLive(_2); // scope 0 at $DIR/issue-101973.rs:+1:5: +1:65
StorageLive(_3); // scope 0 at $DIR/issue-101973.rs:+1:5: +1:58
StorageLive(_4); // scope 0 at $DIR/issue-101973.rs:+1:5: +1:17
StorageLive(_5); // scope 0 at $DIR/issue-101973.rs:+1:10: +1:16
_5 = _1; // scope 0 at $DIR/issue-101973.rs:+1:10: +1:16
_4 = const 0_u32; // scope 1 at $DIR/issue-101973.rs:6:19: 6:23
StorageLive(_12); // scope 2 at $DIR/issue-101973.rs:7:12: 7:27
StorageLive(_13); // scope 2 at $DIR/issue-101973.rs:7:12: 7:20
StorageLive(_14); // scope 2 at $DIR/issue-101973.rs:7:13: 7:14
_14 = _5; // scope 2 at $DIR/issue-101973.rs:7:13: 7:14
_15 = CheckedShr(_14, const 0_i32); // scope 2 at $DIR/issue-101973.rs:7:12: 7:20
assert(!move (_15.1: bool), "attempt to shift right by `{}`, which would overflow", const 0_i32) -> bb3; // scope 2 at $DIR/issue-101973.rs:7:12: 7:20
}

bb1: {
_8 = move (_10.0: u32); // scope 0 at $DIR/issue-101973.rs:+1:32: +1:45
StorageDead(_9); // scope 0 at $DIR/issue-101973.rs:+1:44: +1:45
_7 = BitAnd(move _8, const 15_u32); // scope 0 at $DIR/issue-101973.rs:+1:31: +1:52
StorageDead(_8); // scope 0 at $DIR/issue-101973.rs:+1:51: +1:52
_11 = CheckedShl(_7, const 1_i32); // scope 0 at $DIR/issue-101973.rs:+1:31: +1:57
assert(!move (_11.1: bool), "attempt to shift left by `{}`, which would overflow", const 1_i32) -> bb2; // scope 0 at $DIR/issue-101973.rs:+1:31: +1:57
}

bb2: {
_6 = move (_11.0: u32); // scope 0 at $DIR/issue-101973.rs:+1:31: +1:57
StorageDead(_7); // scope 0 at $DIR/issue-101973.rs:+1:56: +1:57
StorageLive(_16); // scope 3 at $SRC_DIR/core/src/num/uint_macros.rs:LL:COL
_16 = _4; // scope 3 at $SRC_DIR/core/src/num/uint_macros.rs:LL:COL
StorageLive(_17); // scope 3 at $SRC_DIR/core/src/num/uint_macros.rs:LL:COL
_17 = _6; // scope 3 at $SRC_DIR/core/src/num/uint_macros.rs:LL:COL
_3 = rotate_right::<u32>(move _16, move _17) -> bb4; // scope 3 at $SRC_DIR/core/src/num/uint_macros.rs:LL:COL
// mir::Constant
// + span: $SRC_DIR/core/src/num/uint_macros.rs:LL:COL
// + literal: Const { ty: extern "rust-intrinsic" fn(u32, u32) -> u32 {rotate_right::<u32>}, val: Value(<ZST>) }
}

bb3: {
_13 = move (_15.0: u32); // scope 2 at $DIR/issue-101973.rs:7:12: 7:20
StorageDead(_14); // scope 2 at $DIR/issue-101973.rs:7:19: 7:20
_12 = BitAnd(move _13, const 255_u32); // scope 2 at $DIR/issue-101973.rs:7:12: 7:27
StorageDead(_13); // scope 2 at $DIR/issue-101973.rs:7:26: 7:27
_4 = BitOr(_4, move _12); // scope 2 at $DIR/issue-101973.rs:7:5: 7:27
StorageDead(_12); // scope 2 at $DIR/issue-101973.rs:7:26: 7:27
StorageDead(_5); // scope 0 at $DIR/issue-101973.rs:+1:16: +1:17
StorageLive(_6); // scope 0 at $DIR/issue-101973.rs:+1:31: +1:57
StorageLive(_7); // scope 0 at $DIR/issue-101973.rs:+1:31: +1:52
StorageLive(_8); // scope 0 at $DIR/issue-101973.rs:+1:32: +1:45
StorageLive(_9); // scope 0 at $DIR/issue-101973.rs:+1:33: +1:39
_9 = _1; // scope 0 at $DIR/issue-101973.rs:+1:33: +1:39
_10 = CheckedShr(_9, const 8_i32); // scope 0 at $DIR/issue-101973.rs:+1:32: +1:45
assert(!move (_10.1: bool), "attempt to shift right by `{}`, which would overflow", const 8_i32) -> bb1; // scope 0 at $DIR/issue-101973.rs:+1:32: +1:45
}

bb4: {
StorageDead(_17); // scope 3 at $SRC_DIR/core/src/num/uint_macros.rs:LL:COL
StorageDead(_16); // scope 3 at $SRC_DIR/core/src/num/uint_macros.rs:LL:COL
StorageDead(_6); // scope 0 at $DIR/issue-101973.rs:+1:57: +1:58
StorageDead(_4); // scope 0 at $DIR/issue-101973.rs:+1:57: +1:58
_2 = move _3 as i32 (Misc); // scope 0 at $DIR/issue-101973.rs:+1:5: +1:65
StorageDead(_3); // scope 0 at $DIR/issue-101973.rs:+1:64: +1:65
_0 = move _2 as i64 (Misc); // scope 0 at $DIR/issue-101973.rs:+1:5: +1:72
StorageDead(_2); // scope 0 at $DIR/issue-101973.rs:+1:71: +1:72
return; // scope 0 at $DIR/issue-101973.rs:+2:2: +2:2
}
}

0 comments on commit 7d7f555

Please sign in to comment.