Auto merge of #139042 - compiler-errors:do-not-optimize-switchint, r=<try>

Do not remove trivial `SwitchInt` with mir-opt-level=0

When mir-opt-level=0, do not optimize out `SwitchInt` terminators that all have the same terminator since it may remove a read which affects miri's ability to detect UB on that operand.

cc `@RalfJung`

Fixes rust-lang/miri#4237

This affects some tests... I guess I could mark them as `mir-opt-level=1`? Not sure.

Author: bors

compiler/rustc_mir_transform/src/early_otherwise_branch.rs

@@ -224,7 +224,7 @@ impl<'tcx> crate::MirPass<'tcx> for EarlyOtherwiseBranch {
         // Since this optimization adds new basic blocks and invalidates others,
         // clean up the cfg to make it nicer for other passes
         if should_cleanup {
-            simplify_cfg(body);
+            simplify_cfg(tcx, body);
         }
     }
 

compiler/rustc_mir_transform/src/inline.rs

@@ -63,7 +63,7 @@ impl<'tcx> crate::MirPass<'tcx> for Inline {
         let _guard = span.enter();
         if inline::<NormalInliner<'tcx>>(tcx, body) {
             debug!("running simplify cfg on {:?}", body.source);
-            simplify_cfg(body);
+            simplify_cfg(tcx, body);
             deref_finder(tcx, body);
         }
     }
@@ -99,7 +99,7 @@ impl<'tcx> crate::MirPass<'tcx> for ForceInline {
         let _guard = span.enter();
         if inline::<ForceInliner<'tcx>>(tcx, body) {
             debug!("running simplify cfg on {:?}", body.source);
-            simplify_cfg(body);
+            simplify_cfg(tcx, body);
             deref_finder(tcx, body);
         }
     }

compiler/rustc_mir_transform/src/match_branches.rs

@@ -45,7 +45,7 @@ impl<'tcx> crate::MirPass<'tcx> for MatchBranchSimplification {
         }
 
         if should_cleanup {
-            simplify_cfg(body);
+            simplify_cfg(tcx, body);
         }
     }
 

compiler/rustc_mir_transform/src/remove_unneeded_drops.rs

@@ -35,7 +35,7 @@ impl<'tcx> crate::MirPass<'tcx> for RemoveUnneededDrops {
         // if we applied optimizations, we potentially have some cfg to cleanup to
         // make it easier for further passes
         if should_simplify {
-            simplify_cfg(body);
+            simplify_cfg(tcx, body);
         }
     }
 

compiler/rustc_mir_transform/src/simplify.rs

@@ -66,8 +66,8 @@ impl SimplifyCfg {
     }
 }
 
-pub(super) fn simplify_cfg(body: &mut Body<'_>) {
-    CfgSimplifier::new(body).simplify();
+pub(super) fn simplify_cfg<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
+    CfgSimplifier::new(tcx, body).simplify();
     remove_dead_blocks(body);
 
     // FIXME: Should probably be moved into some kind of pass manager
@@ -79,9 +79,9 @@ impl<'tcx> crate::MirPass<'tcx> for SimplifyCfg {
         self.name()
     }
 
-    fn run_pass(&self, _: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
+    fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
         debug!("SimplifyCfg({:?}) - simplifying {:?}", self.name(), body.source);
-        simplify_cfg(body);
+        simplify_cfg(tcx, body);
     }
 
     fn is_required(&self) -> bool {
@@ -90,12 +90,13 @@ impl<'tcx> crate::MirPass<'tcx> for SimplifyCfg {
 }
 
 struct CfgSimplifier<'a, 'tcx> {
+    mir_opt_level: usize,
     basic_blocks: &'a mut IndexSlice<BasicBlock, BasicBlockData<'tcx>>,
     pred_count: IndexVec<BasicBlock, u32>,
 }
 
 impl<'a, 'tcx> CfgSimplifier<'a, 'tcx> {
-    fn new(body: &'a mut Body<'tcx>) -> Self {
+    fn new(tcx: TyCtxt<'tcx>, body: &'a mut Body<'tcx>) -> Self {
         let mut pred_count = IndexVec::from_elem(0u32, &body.basic_blocks);
 
         // we can't use mir.predecessors() here because that counts
@@ -112,7 +113,7 @@ impl<'a, 'tcx> CfgSimplifier<'a, 'tcx> {
 
         let basic_blocks = body.basic_blocks_mut();
 
-        CfgSimplifier { basic_blocks, pred_count }
+        CfgSimplifier { mir_opt_level: tcx.sess.mir_opt_level(), basic_blocks, pred_count }
     }
 
     fn simplify(mut self) {
@@ -253,9 +254,14 @@ impl<'a, 'tcx> CfgSimplifier<'a, 'tcx> {
 
     // turn a branch with all successors identical to a goto
     fn simplify_branch(&mut self, terminator: &mut Terminator<'tcx>) -> bool {
-        match terminator.kind {
-            TerminatorKind::SwitchInt { .. } => {}
-            _ => return false,
+        // Removing a `SwitchInt` terminator may remove reads that result in UB,
+        // so we must not apply this optimization when mir-opt-level = 0.
+        if self.mir_opt_level == 0 {
+            return false;
+        }
+
+        let TerminatorKind::SwitchInt { .. } = terminator.kind else {
+            return false;
         };
 
         let first_succ = {

src/tools/miri/tests/fail/read_from_trivial_switch.rs

@@ -0,0 +1,8 @@
+use std::mem::MaybeUninit;
+
+fn main() {
+    let uninit: MaybeUninit<i32> = MaybeUninit::uninit();
+    let bad_ref: &i32 = unsafe { uninit.assume_init_ref() };
+    let &(0 | _) = bad_ref;
+    //~^ ERROR: Undefined Behavior: using uninitialized data, but this operation requires initialized memory
+}

src/tools/miri/tests/fail/read_from_trivial_switch.stderr

@@ -0,0 +1,15 @@
+error: Undefined Behavior: using uninitialized data, but this operation requires initialized memory
+  --> tests/fail/read_from_trivial_switch.rs:LL:CC
+   |
+LL |     let &(0 | _) = bad_ref;
+   |         ^^^^^^^^ using uninitialized data, but this operation requires initialized memory
+   |
+   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
+   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
+   = note: BACKTRACE:
+   = note: inside `main` at tests/fail/read_from_trivial_switch.rs:LL:CC
+
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
+
+error: aborting due to 1 previous error
+

tests/mir-opt/early_otherwise_branch_unwind.poll.EarlyOtherwiseBranch.diff

@@ -12,6 +12,7 @@
       let mut _7: bool;
       let mut _8: bool;
       let mut _9: isize;
+      let mut _10: isize;
       scope 1 {
           debug _trailers => _5;
       }
@@ -99,12 +100,13 @@
       }
 
       bb15: {
-          goto -> bb14;
+          _9 = discriminant(((((_1 as Ready).0: std::result::Result<std::option::Option<std::vec::Vec<u8>>, u8>) as Ok).0: std::option::Option<std::vec::Vec<u8>>));
+          switchInt(move _9) -> [1: bb14, otherwise: bb14];
       }
 
       bb16: {
-          _9 = discriminant(((_1 as Ready).0: std::result::Result<std::option::Option<std::vec::Vec<u8>>, u8>));
-          switchInt(move _9) -> [0: bb13, otherwise: bb12];
+          _10 = discriminant(((_1 as Ready).0: std::result::Result<std::option::Option<std::vec::Vec<u8>>, u8>));
+          switchInt(move _10) -> [0: bb13, otherwise: bb12];
       }
 
       bb17: {
@@ -116,7 +118,7 @@
       }
 
       bb19 (cleanup): {
-          goto -> bb9;
+          switchInt(copy _2) -> [1: bb9, otherwise: bb9];
       }
 
       bb20 (cleanup): {

tests/mir-opt/early_otherwise_branch_unwind.unwind.EarlyOtherwiseBranch.diff

@@ -11,6 +11,7 @@
       let mut _6: bool;
       let mut _7: bool;
       let mut _8: isize;
+      let mut _9: isize;
       scope 1 {
           debug _v => _5;
       }
@@ -92,12 +93,13 @@
       }
 
       bb15: {
-          goto -> bb14;
+          _8 = discriminant(((((_1 as Some).0: std::option::Option<std::option::Option<T>>) as Some).0: std::option::Option<T>));
+          switchInt(move _8) -> [1: bb14, otherwise: bb14];
       }
 
       bb16: {
-          _8 = discriminant(((_1 as Some).0: std::option::Option<std::option::Option<T>>));
-          switchInt(move _8) -> [1: bb13, otherwise: bb12];
+          _9 = discriminant(((_1 as Some).0: std::option::Option<std::option::Option<T>>));
+          switchInt(move _9) -> [1: bb13, otherwise: bb12];
       }
 
       bb17: {
@@ -109,7 +111,7 @@
       }
 
       bb19 (cleanup): {
-          goto -> bb9;
+          switchInt(copy _2) -> [1: bb9, otherwise: bb9];
       }
 
       bb20 (cleanup): {

tests/mir-opt/gvn_copy_aggregate.remove_storage_dead.GVN.diff

@@ -39,6 +39,10 @@
 +         _2 = copy _5;
 +         nop;
           _7 = discriminant(_3);
+          switchInt(move _7) -> bb2;
+      }
+  
+      bb2: {
 -         StorageDead(_3);
 +         nop;
           StorageLive(_6);

tests/mir-opt/read_from_trivial_switch.main.SimplifyCfg-initial.diff

@@ -0,0 +1,83 @@
+- // MIR for `main` before SimplifyCfg-initial
++ // MIR for `main` after SimplifyCfg-initial
+  
+  | User Type Annotations
+  | 0: user_ty: Canonical { value: Ty(std::mem::MaybeUninit<i32>), max_universe: U0, variables: [] }, span: $DIR/read_from_trivial_switch.rs:7:17: 7:33, inferred_ty: std::mem::MaybeUninit<i32>
+  | 1: user_ty: Canonical { value: TypeOf(DefId(2:2155 ~ core[aaca]::mem::maybe_uninit::{impl#2}::uninit), UserArgs { args: [^0], user_self_ty: Some(UserSelfTy { impl_def_id: DefId(2:2152 ~ core[aaca]::mem::maybe_uninit::{impl#2}), self_ty: std::mem::MaybeUninit<^1> }) }), max_universe: U0, variables: [CanonicalVarInfo { kind: Ty(General(U0)) }, CanonicalVarInfo { kind: Ty(General(U0)) }] }, span: $DIR/read_from_trivial_switch.rs:7:36: 7:55, inferred_ty: fn() -> std::mem::MaybeUninit<i32> {std::mem::MaybeUninit::<i32>::uninit}
+  | 2: user_ty: Canonical { value: Ty(std::mem::MaybeUninit<i32>), max_universe: U0, variables: [] }, span: $DIR/read_from_trivial_switch.rs:7:17: 7:33, inferred_ty: std::mem::MaybeUninit<i32>
+  | 3: user_ty: Canonical { value: Ty(&i32), max_universe: U0, variables: [CanonicalVarInfo { kind: Region(U0) }] }, span: $DIR/read_from_trivial_switch.rs:8:18: 8:22, inferred_ty: &i32
+  | 4: user_ty: Canonical { value: Ty(&i32), max_universe: U0, variables: [CanonicalVarInfo { kind: Region(U0) }] }, span: $DIR/read_from_trivial_switch.rs:8:18: 8:22, inferred_ty: &i32
+  |
+  fn main() -> () {
+      let mut _0: ();
+      let _1: std::mem::MaybeUninit<i32> as UserTypeProjection { base: UserType(0), projs: [] };
+      let _3: &i32;
+      let mut _4: &std::mem::MaybeUninit<i32>;
+      scope 1 {
+          debug uninit => _1;
+          let _2: &i32 as UserTypeProjection { base: UserType(3), projs: [] };
+          scope 2 {
+              debug bad_ref => _2;
+              scope 3 {
+              }
+          }
+      }
+  
+      bb0: {
+          StorageLive(_1);
+-         _1 = MaybeUninit::<i32>::uninit() -> [return: bb1, unwind: bb8];
++         _1 = MaybeUninit::<i32>::uninit() -> [return: bb1, unwind: bb4];
+      }
+  
+      bb1: {
+          FakeRead(ForLet(None), _1);
+          AscribeUserType(_1, o, UserTypeProjection { base: UserType(2), projs: [] });
+          StorageLive(_2);
+          StorageLive(_3);
+          StorageLive(_4);
+          _4 = &_1;
+-         _3 = MaybeUninit::<i32>::assume_init_ref(move _4) -> [return: bb2, unwind: bb8];
++         _3 = MaybeUninit::<i32>::assume_init_ref(move _4) -> [return: bb2, unwind: bb4];
+      }
+  
+      bb2: {
+          _2 = &(*_3);
+          StorageDead(_4);
+          FakeRead(ForLet(None), _2);
+          AscribeUserType(_2, o, UserTypeProjection { base: UserType(4), projs: [] });
+          StorageDead(_3);
+          PlaceMention(_2);
+-         switchInt(copy (*_2)) -> [0: bb4, otherwise: bb3];
++         switchInt(copy (*_2)) -> [0: bb3, otherwise: bb3];
+      }
+  
+      bb3: {
+-         goto -> bb7;
+-     }
+- 
+-     bb4: {
+-         goto -> bb7;
+-     }
+- 
+-     bb5: {
+-         goto -> bb3;
+-     }
+- 
+-     bb6: {
+-         FakeRead(ForMatchedPlace(None), _2);
+-         unreachable;
+-     }
+- 
+-     bb7: {
+          _0 = const ();
+          StorageDead(_2);
+          StorageDead(_1);
+          return;
+      }
+  
+-     bb8 (cleanup): {
++     bb4 (cleanup): {
+          resume;
+      }
+  }
+  

tests/mir-opt/read_from_trivial_switch.rs

@@ -0,0 +1,11 @@
+//@ test-mir-pass: SimplifyCfg-initial
+
+use std::mem::MaybeUninit;
+
+// EMIT_MIR read_from_trivial_switch.main.SimplifyCfg-initial.diff
+fn main() {
+    let uninit: MaybeUninit<i32> = MaybeUninit::uninit();
+    let bad_ref: &i32 = unsafe { uninit.assume_init_ref() };
+    // CHECK: switchInt
+    let &(0 | _) = bad_ref;
+}

tests/mir-opt/simplify_locals_removes_unused_discriminant_reads.map.SimplifyLocals-before-const-prop.diff

@@ -10,6 +10,7 @@
 -     let mut _5: bool;
 -     let mut _6: isize;
 -     let mut _7: isize;
++     let mut _5: isize;
       scope 1 {
           debug x => _3;
       }
@@ -33,17 +34,23 @@
           _0 = Option::<Box<()>>::Some(move _4);
           StorageDead(_4);
           StorageDead(_3);
-          goto -> bb4;
+          goto -> bb5;
       }
 
       bb3: {
           _0 = Option::<Box<()>>::None;
-          goto -> bb4;
+          goto -> bb5;
       }
 
       bb4: {
--         _6 = discriminant(_1);
           return;
       }
+  
+      bb5: {
+-         _6 = discriminant(_1);
+-         switchInt(move _6) -> [1: bb4, otherwise: bb4];
++         _5 = discriminant(_1);
++         switchInt(move _5) -> [1: bb4, otherwise: bb4];
+      }
   }