Do not optimize out SwitchInt before borrowck, or if Zmir-preserve-ub

Author: compiler-errors

compiler/rustc_interface/src/tests.rs

@@ -817,8 +817,8 @@ fn test_unstable_options_tracking_hash() {
     tracked!(min_function_alignment, Some(Align::EIGHT));
     tracked!(mir_emit_retag, true);
     tracked!(mir_enable_passes, vec![("DestProp".to_string(), false)]);
-    tracked!(mir_keep_place_mention, true);
     tracked!(mir_opt_level, Some(4));
+    tracked!(mir_preserve_ub, true);
     tracked!(move_size_limit, Some(4096));
     tracked!(mutable_noalias, false);
     tracked!(next_solver, NextSolverConfig { coherence: true, globally: true });

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_place_mention.rs

@@ -8,7 +8,7 @@ pub(super) struct RemovePlaceMention;
 
 impl<'tcx> crate::MirPass<'tcx> for RemovePlaceMention {
     fn is_enabled(&self, sess: &rustc_session::Session) -> bool {
-        !sess.opts.unstable_opts.mir_keep_place_mention
+        !sess.opts.unstable_opts.mir_preserve_ub
     }
 
     fn run_pass(&self, _: TyCtxt<'tcx>, body: &mut Body<'tcx>) {

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

@@ -26,6 +26,13 @@
 //! Here the block (`{ return; }`) has the return type `char`, rather than `()`, but the MIR we
 //! naively generate still contains the `_a = ()` write in the unreachable block "after" the
 //! return.
+//!
+//! **WARNING**: This is one of the few optimizations that runs on built and analysis MIR, and
+//! so its effects may affect the type-checking, borrow-checking, and other analysis of MIR.
+//! We must be extremely careful to only apply optimizations that preserve UB and all
+//! non-determinism, since changes here can affect which programs compile in an insta-stable way.
+//! The normal logic that a program with UB can be changed to do anything does not apply to
+//! pre-"runtime" MIR!
 
 use rustc_index::{Idx, IndexSlice, IndexVec};
 use rustc_middle::mir::visit::{MutVisitor, MutatingUseContext, PlaceContext, Visitor};
@@ -66,8 +73,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 +86,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 +97,13 @@ impl<'tcx> crate::MirPass<'tcx> for SimplifyCfg {
 }
 
 struct CfgSimplifier<'a, 'tcx> {
+    preserve_switch_reads: bool,
     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
@@ -110,9 +118,12 @@ impl<'a, 'tcx> CfgSimplifier<'a, 'tcx> {
             }
         }
 
+        // Preserve `SwitchInt` reads on built and analysis MIR, or if `-Zmir-preserve-ub`.
+        let preserve_switch_reads = matches!(body.phase, MirPhase::Built | MirPhase::Analysis(_))
+            || tcx.sess.opts.unstable_opts.mir_preserve_ub;
         let basic_blocks = body.basic_blocks_mut();
 
-        CfgSimplifier { basic_blocks, pred_count }
+        CfgSimplifier { preserve_switch_reads, basic_blocks, pred_count }
     }
 
     fn simplify(mut self) {
@@ -253,9 +264,15 @@ 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 before borrowck or when
+        // `-Zmir-preserve-ub` is set.
+        if self.preserve_switch_reads {
+            return false;
+        }
+
+        let TerminatorKind::SwitchInt { .. } = terminator.kind else {
+            return false;
         };
 
         let first_succ = {

compiler/rustc_session/src/options.rs

@@ -2319,12 +2319,12 @@ options! {
     mir_include_spans: MirIncludeSpans = (MirIncludeSpans::default(), parse_mir_include_spans, [UNTRACKED],
         "include extra comments in mir pretty printing, like line numbers and statement indices, \
          details about types, etc. (boolean for all passes, 'nll' to enable in NLL MIR only, default: 'nll')"),
-    mir_keep_place_mention: bool = (false, parse_bool, [TRACKED],
-        "keep place mention MIR statements, interpreted e.g., by miri; implies -Zmir-opt-level=0 \
-        (default: no)"),
     #[rustc_lint_opt_deny_field_access("use `Session::mir_opt_level` instead of this field")]
     mir_opt_level: Option<usize> = (None, parse_opt_number, [TRACKED],
         "MIR optimization level (0-4; default: 1 in non optimized builds and 2 in optimized builds)"),
+    mir_preserve_ub: bool = (false, parse_bool, [TRACKED],
+        "keep place mention statements and reads in trivial SwitchInt terminators, which are interpreted \
+        e.g., by miri; implies -Zmir-opt-level=0 (default: no)"),
     mir_strip_debuginfo: MirStripDebugInfo = (MirStripDebugInfo::None, parse_mir_strip_debuginfo, [TRACKED],
         "Whether to remove some of the MIR debug info from methods.  Default: None"),
     move_size_limit: Option<usize> = (None, parse_opt_number, [TRACKED],

src/tools/miri/src/lib.rs

@@ -169,7 +169,7 @@ pub const MIRI_DEFAULT_ARGS: &[&str] = &[
     "-Zalways-encode-mir",
     "-Zextra-const-ub-checks",
     "-Zmir-emit-retag",
-    "-Zmir-keep-place-mention",
+    "-Zmir-preserve-ub",
     "-Zmir-opt-level=0",
     "-Zmir-enable-passes=-CheckAlignment,-CheckNull",
     // Deduplicating diagnostics means we miss events when tracking what happens during an

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

@@ -0,0 +1,14 @@
+// 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>.
+
+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
+}