Rollup merge of #139042 - compiler-errors:do-not-optimize-switchint, r=saethlin
Do not remove trivial `SwitchInt` in analysis MIR This PR ensures that we don't prematurely remove trivial `SwitchInt` terminators which affects both the borrow-checking and runtime semantics (i.e. UB) of the code. Previously the `SimplifyCfg` optimization was removing `SwitchInt` terminators when they was "trivial", i.e. when all arms branched to the same basic block, even if that `SwitchInt` terminator had the side-effect of reading an operand which (for example) may not be initialized or may point to an invalid place in memory. This behavior is unlike all other optimizations, which are only applied after "analysis" (i.e. borrow-checking) is finished, and which Miri disables to make sure the compiler doesn't silently remove UB. Fixing this code "breaks" (i.e. unmasks) code that used to borrow-check but no longer does, like: ```rust fn foo() { let x; let (0 | _) = x; } ``` This match expression should perform a read because `_` does not shadow the `0` literal pattern, and the compiler should have to read the match scrutinee to compare it to 0. I've checked that this behavior does not actually manifest in practice via a crater run which came back clean: https://github.com/rust-lang/rust/pull/139042#issuecomment-2767436367 As a side-note, it may be tempting to suggest that this is actually a good thing or that we should preserve this behavior. If we wanted to make this work (i.e. trivially optimize out reads from matches that are redundant like `0 | _`), then we should be enabling this behavior *after* fixing this. However, I think it's kinda unprincipled, and for example other variations of the code don't even work today, e.g.: ```rust fn foo() { let x; let (0.. | _) = x; } ```
This commit is contained in:
commit
5d2375f789
18 changed files with 197 additions and 51 deletions
|
@ -24,43 +24,47 @@ fn match_tuple(_1: (u32, bool, Option<i32>, u32)) -> u32 {
|
|||
|
||||
bb1: {
|
||||
_0 = const 0_u32;
|
||||
goto -> bb10;
|
||||
goto -> bb11;
|
||||
}
|
||||
|
||||
bb2: {
|
||||
_2 = discriminant((_1.2: std::option::Option<i32>));
|
||||
switchInt(move _2) -> [0: bb4, 1: bb3, otherwise: bb1];
|
||||
switchInt(copy (_1.1: bool)) -> [0: bb3, otherwise: bb3];
|
||||
}
|
||||
|
||||
bb3: {
|
||||
switchInt(copy (((_1.2: std::option::Option<i32>) as Some).0: i32)) -> [1: bb4, 8: bb4, otherwise: bb1];
|
||||
_2 = discriminant((_1.2: std::option::Option<i32>));
|
||||
switchInt(move _2) -> [0: bb5, 1: bb4, otherwise: bb1];
|
||||
}
|
||||
|
||||
bb4: {
|
||||
_5 = Le(const 6_u32, copy (_1.3: u32));
|
||||
switchInt(move _5) -> [0: bb5, otherwise: bb7];
|
||||
switchInt(copy (((_1.2: std::option::Option<i32>) as Some).0: i32)) -> [1: bb5, 8: bb5, otherwise: bb1];
|
||||
}
|
||||
|
||||
bb5: {
|
||||
_3 = Le(const 13_u32, copy (_1.3: u32));
|
||||
switchInt(move _3) -> [0: bb1, otherwise: bb6];
|
||||
_5 = Le(const 6_u32, copy (_1.3: u32));
|
||||
switchInt(move _5) -> [0: bb6, otherwise: bb8];
|
||||
}
|
||||
|
||||
bb6: {
|
||||
_4 = Le(copy (_1.3: u32), const 16_u32);
|
||||
switchInt(move _4) -> [0: bb1, otherwise: bb8];
|
||||
_3 = Le(const 13_u32, copy (_1.3: u32));
|
||||
switchInt(move _3) -> [0: bb1, otherwise: bb7];
|
||||
}
|
||||
|
||||
bb7: {
|
||||
_6 = Le(copy (_1.3: u32), const 9_u32);
|
||||
switchInt(move _6) -> [0: bb5, otherwise: bb8];
|
||||
_4 = Le(copy (_1.3: u32), const 16_u32);
|
||||
switchInt(move _4) -> [0: bb1, otherwise: bb9];
|
||||
}
|
||||
|
||||
bb8: {
|
||||
falseEdge -> [real: bb9, imaginary: bb1];
|
||||
_6 = Le(copy (_1.3: u32), const 9_u32);
|
||||
switchInt(move _6) -> [0: bb6, otherwise: bb9];
|
||||
}
|
||||
|
||||
bb9: {
|
||||
falseEdge -> [real: bb10, imaginary: bb1];
|
||||
}
|
||||
|
||||
bb10: {
|
||||
StorageLive(_7);
|
||||
_7 = copy (_1.0: u32);
|
||||
StorageLive(_8);
|
||||
|
@ -74,10 +78,10 @@ fn match_tuple(_1: (u32, bool, Option<i32>, u32)) -> u32 {
|
|||
StorageDead(_9);
|
||||
StorageDead(_8);
|
||||
StorageDead(_7);
|
||||
goto -> bb10;
|
||||
goto -> bb11;
|
||||
}
|
||||
|
||||
bb10: {
|
||||
bb11: {
|
||||
return;
|
||||
}
|
||||
}
|
||||
|
|
|
@ -2,7 +2,7 @@
|
|||
// and don't remove it as a dead store.
|
||||
//
|
||||
//@ test-mir-pass: DeadStoreElimination-initial
|
||||
//@ compile-flags: -Zmir-keep-place-mention
|
||||
//@ compile-flags: -Zmir-preserve-ub
|
||||
|
||||
// EMIT_MIR place_mention.main.DeadStoreElimination-initial.diff
|
||||
fn main() {
|
||||
|
|
|
@ -14,7 +14,7 @@ fn single_switchint() -> () {
|
|||
}
|
||||
|
||||
bb1: {
|
||||
switchInt(copy (_2.0: i32)) -> [3: bb8, 4: bb8, otherwise: bb7];
|
||||
switchInt(copy (_2.0: i32)) -> [3: bb9, 4: bb9, otherwise: bb8];
|
||||
}
|
||||
|
||||
bb2: {
|
||||
|
@ -22,7 +22,7 @@ fn single_switchint() -> () {
|
|||
}
|
||||
|
||||
bb3: {
|
||||
falseEdge -> [real: bb12, imaginary: bb4];
|
||||
falseEdge -> [real: bb14, imaginary: bb4];
|
||||
}
|
||||
|
||||
bb4: {
|
||||
|
@ -30,43 +30,51 @@ fn single_switchint() -> () {
|
|||
}
|
||||
|
||||
bb5: {
|
||||
falseEdge -> [real: bb11, imaginary: bb6];
|
||||
falseEdge -> [real: bb13, imaginary: bb6];
|
||||
}
|
||||
|
||||
bb6: {
|
||||
falseEdge -> [real: bb10, imaginary: bb1];
|
||||
switchInt(copy (_2.1: bool)) -> [0: bb7, otherwise: bb7];
|
||||
}
|
||||
|
||||
bb7: {
|
||||
_1 = const 5_i32;
|
||||
goto -> bb13;
|
||||
falseEdge -> [real: bb12, imaginary: bb1];
|
||||
}
|
||||
|
||||
bb8: {
|
||||
falseEdge -> [real: bb9, imaginary: bb7];
|
||||
_1 = const 5_i32;
|
||||
goto -> bb15;
|
||||
}
|
||||
|
||||
bb9: {
|
||||
_1 = const 4_i32;
|
||||
goto -> bb13;
|
||||
switchInt(copy (_2.1: bool)) -> [0: bb10, otherwise: bb10];
|
||||
}
|
||||
|
||||
bb10: {
|
||||
_1 = const 3_i32;
|
||||
goto -> bb13;
|
||||
falseEdge -> [real: bb11, imaginary: bb8];
|
||||
}
|
||||
|
||||
bb11: {
|
||||
_1 = const 2_i32;
|
||||
goto -> bb13;
|
||||
_1 = const 4_i32;
|
||||
goto -> bb15;
|
||||
}
|
||||
|
||||
bb12: {
|
||||
_1 = const 1_i32;
|
||||
goto -> bb13;
|
||||
_1 = const 3_i32;
|
||||
goto -> bb15;
|
||||
}
|
||||
|
||||
bb13: {
|
||||
_1 = const 2_i32;
|
||||
goto -> bb15;
|
||||
}
|
||||
|
||||
bb14: {
|
||||
_1 = const 1_i32;
|
||||
goto -> bb15;
|
||||
}
|
||||
|
||||
bb15: {
|
||||
StorageDead(_2);
|
||||
StorageDead(_1);
|
||||
_0 = const ();
|
||||
|
|
|
@ -0,0 +1,49 @@
|
|||
- // MIR for `main` before SimplifyCfg-initial
|
||||
+ // MIR for `main` after SimplifyCfg-initial
|
||||
|
||||
fn main() -> () {
|
||||
let mut _0: ();
|
||||
let _1: &i32;
|
||||
let _2: i32;
|
||||
scope 1 {
|
||||
debug ref_ => _1;
|
||||
scope 2 {
|
||||
}
|
||||
}
|
||||
|
||||
bb0: {
|
||||
StorageLive(_1);
|
||||
StorageLive(_2);
|
||||
_2 = const 1_i32;
|
||||
_1 = &_2;
|
||||
FakeRead(ForLet(None), _1);
|
||||
PlaceMention(_1);
|
||||
- switchInt(copy (*_1)) -> [0: bb2, otherwise: bb1];
|
||||
+ switchInt(copy (*_1)) -> [0: bb1, otherwise: bb1];
|
||||
}
|
||||
|
||||
bb1: {
|
||||
- goto -> bb5;
|
||||
- }
|
||||
-
|
||||
- bb2: {
|
||||
- goto -> bb5;
|
||||
- }
|
||||
-
|
||||
- bb3: {
|
||||
- goto -> bb1;
|
||||
- }
|
||||
-
|
||||
- bb4: {
|
||||
- FakeRead(ForMatchedPlace(None), _1);
|
||||
- unreachable;
|
||||
- }
|
||||
-
|
||||
- bb5: {
|
||||
_0 = const ();
|
||||
StorageDead(_2);
|
||||
StorageDead(_1);
|
||||
return;
|
||||
}
|
||||
}
|
||||
|
15
tests/mir-opt/read_from_trivial_switch.rs
Normal file
15
tests/mir-opt/read_from_trivial_switch.rs
Normal file
|
@ -0,0 +1,15 @@
|
|||
// Ensure that we don't optimize out `SwitchInt` reads even if that terminator
|
||||
// branches to the same basic block on every target, since the operand may have
|
||||
// side-effects that affect analysis of the MIR.
|
||||
//
|
||||
// See <https://github.com/rust-lang/miri/issues/4237>.
|
||||
|
||||
//@ test-mir-pass: SimplifyCfg-initial
|
||||
//@ compile-flags: -Zmir-preserve-ub
|
||||
|
||||
// EMIT_MIR read_from_trivial_switch.main.SimplifyCfg-initial.diff
|
||||
fn main() {
|
||||
let ref_ = &1i32;
|
||||
// CHECK: switchInt
|
||||
let &(0 | _) = ref_;
|
||||
}
|
Loading…
Add table
Add a link
Reference in a new issue