Rollup merge of #119577 - tmiasko:lint, r=oli-obk

Migrate memory overlap check from validator to lint

The check attempts to identify potential undefined behaviour, rather
than whether MIR is well-formed. It belongs in the lint not validator.

Follow up to changes from #119077.

compiler/rustc_const_eval/src/transform/validate.rs

@@ -74,7 +74,6 @@ impl<'tcx> MirPass<'tcx> for Validator {
             mir_phase,
             unwind_edge_count: 0,
             reachable_blocks: traversal::reachable_as_bitset(body),
-            place_cache: FxHashSet::default(),
             value_cache: FxHashSet::default(),
             can_unwind,
         };
@@ -106,7 +105,6 @@ struct CfgChecker<'a, 'tcx> {
     mir_phase: MirPhase,
     unwind_edge_count: usize,
     reachable_blocks: BitSet<BasicBlock>,
-    place_cache: FxHashSet<PlaceRef<'tcx>>,
     value_cache: FxHashSet<u128>,
     // If `false`, then the MIR must not contain `UnwindAction::Continue` or
     // `TerminatorKind::Resume`.
@@ -294,19 +292,6 @@ impl<'a, 'tcx> Visitor<'tcx> for CfgChecker<'a, 'tcx> {
 
     fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) {
         match &statement.kind {
-            StatementKind::Assign(box (dest, rvalue)) => {
-                // FIXME(JakobDegen): Check this for all rvalues, not just this one.
-                if let Rvalue::Use(Operand::Copy(src) | Operand::Move(src)) = rvalue {
-                    // The sides of an assignment must not alias. Currently this just checks whether
-                    // the places are identical.
-                    if dest == src {
-                        self.fail(
-                            location,
-                            "encountered `Assign` statement with overlapping memory",
-                        );
-                    }
-                }
-            }
             StatementKind::AscribeUserType(..) => {
                 if self.mir_phase >= MirPhase::Runtime(RuntimePhase::Initial) {
                     self.fail(
@@ -341,7 +326,8 @@ impl<'a, 'tcx> Visitor<'tcx> for CfgChecker<'a, 'tcx> {
                     self.fail(location, format!("explicit `{kind:?}` is forbidden"));
                 }
             }
-            StatementKind::StorageLive(_)
+            StatementKind::Assign(..)
+            | StatementKind::StorageLive(_)
             | StatementKind::StorageDead(_)
             | StatementKind::Intrinsic(_)
             | StatementKind::Coverage(_)
@@ -404,10 +390,7 @@ impl<'a, 'tcx> Visitor<'tcx> for CfgChecker<'a, 'tcx> {
                 }
 
                 // The call destination place and Operand::Move place used as an argument might be
-                // passed by a reference to the callee. Consequently they must be non-overlapping
+                // passed by a reference to the callee. Consequently they cannot be packed.
-                // and cannot be packed. Currently this simply checks for duplicate places.
-                self.place_cache.clear();
-                self.place_cache.insert(destination.as_ref());
                 if is_within_packed(self.tcx, &self.body.local_decls, *destination).is_some() {
                     // This is bad! The callee will expect the memory to be aligned.
                     self.fail(
@@ -418,10 +401,8 @@ impl<'a, 'tcx> Visitor<'tcx> for CfgChecker<'a, 'tcx> {
                         ),
                     );
                 }
-                let mut has_duplicates = false;
                 for arg in args {
                     if let Operand::Move(place) = arg {
-                        has_duplicates |= !self.place_cache.insert(place.as_ref());
                         if is_within_packed(self.tcx, &self.body.local_decls, *place).is_some() {
                             // This is bad! The callee will expect the memory to be aligned.
                             self.fail(
@@ -434,16 +415,6 @@ impl<'a, 'tcx> Visitor<'tcx> for CfgChecker<'a, 'tcx> {
                         }
                     }
                 }
-
-                if has_duplicates {
-                    self.fail(
-                        location,
-                        format!(
-                            "encountered overlapping memory in `Move` arguments to `Call` terminator: {:?}",
-                            terminator.kind,
-                        ),
-                    );
-                }
             }
             TerminatorKind::Assert { target, unwind, .. } => {
                 self.check_edge(location, *target, EdgeKind::Normal);
@@ -1112,17 +1083,6 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
                         )
                     }
                 }
-                // FIXME(JakobDegen): Check this for all rvalues, not just this one.
-                if let Rvalue::Use(Operand::Copy(src) | Operand::Move(src)) = rvalue {
-                    // The sides of an assignment must not alias. Currently this just checks whether
-                    // the places are identical.
-                    if dest == src {
-                        self.fail(
-                            location,
-                            "encountered `Assign` statement with overlapping memory",
-                        );
-                    }
-                }
             }
             StatementKind::AscribeUserType(..) => {
                 if self.mir_phase >= MirPhase::Runtime(RuntimePhase::Initial) {

compiler/rustc_mir_transform/src/dest_prop.rs

@@ -133,7 +133,6 @@
 
 use std::collections::hash_map::{Entry, OccupiedEntry};
 
-use crate::simplify::remove_dead_blocks;
 use crate::MirPass;
 use rustc_data_structures::fx::FxHashMap;
 use rustc_index::bit_set::BitSet;
@@ -241,12 +240,6 @@ impl<'tcx> MirPass<'tcx> for DestinationPropagation {
             apply_merges(body, tcx, &merges, &merged_locals);
         }
 
-        if round_count != 0 {
-            // Merging can introduce overlap between moved arguments and/or call destination in an
-            // unreachable code, which validator considers to be ill-formed.
-            remove_dead_blocks(body);
-        }
-
         trace!(round_count);
     }
 }

compiler/rustc_mir_transform/src/lint.rs

@@ -1,6 +1,7 @@
 //! This pass statically detects code which has undefined behaviour or is likely to be erroneous.
 //! It can be used to locate problems in MIR building or optimizations. It assumes that all code
 //! can be executed, so it has false positives.
+use rustc_data_structures::fx::FxHashSet;
 use rustc_index::bit_set::BitSet;
 use rustc_middle::mir::visit::{PlaceContext, Visitor};
 use rustc_middle::mir::*;
@@ -11,7 +12,6 @@ use rustc_mir_dataflow::{Analysis, ResultsCursor};
 use std::borrow::Cow;
 
 pub fn lint_body<'tcx>(tcx: TyCtxt<'tcx>, body: &Body<'tcx>, when: String) {
-    let reachable_blocks = traversal::reachable_as_bitset(body);
     let always_live_locals = &always_storage_live_locals(body);
 
     let maybe_storage_live = MaybeStorageLive::new(Cow::Borrowed(always_live_locals))
@@ -24,17 +24,19 @@ pub fn lint_body<'tcx>(tcx: TyCtxt<'tcx>, body: &Body<'tcx>, when: String) {
         .iterate_to_fixpoint()
         .into_results_cursor(body);
 
-    Lint {
+    let mut lint = Lint {
         tcx,
         when,
         body,
         is_fn_like: tcx.def_kind(body.source.def_id()).is_fn_like(),
         always_live_locals,
-        reachable_blocks,
         maybe_storage_live,
         maybe_storage_dead,
+        places: Default::default(),
+    };
+    for (bb, data) in traversal::reachable(body) {
+        lint.visit_basic_block_data(bb, data);
     }
-    .visit_body(body);
 }
 
 struct Lint<'a, 'tcx> {
@@ -43,9 +45,9 @@ struct Lint<'a, 'tcx> {
     body: &'a Body<'tcx>,
     is_fn_like: bool,
     always_live_locals: &'a BitSet<Local>,
-    reachable_blocks: BitSet<BasicBlock>,
     maybe_storage_live: ResultsCursor<'a, 'tcx, MaybeStorageLive<'a>>,
     maybe_storage_dead: ResultsCursor<'a, 'tcx, MaybeStorageDead<'a>>,
+    places: FxHashSet<PlaceRef<'tcx>>,
 }
 
 impl<'a, 'tcx> Lint<'a, 'tcx> {
@@ -67,7 +69,7 @@ impl<'a, 'tcx> Lint<'a, 'tcx> {
 
 impl<'a, 'tcx> Visitor<'tcx> for Lint<'a, 'tcx> {
     fn visit_local(&mut self, local: Local, context: PlaceContext, location: Location) {
-        if self.reachable_blocks.contains(location.block) && context.is_use() {
+        if context.is_use() {
             self.maybe_storage_dead.seek_after_primary_effect(location);
             if self.maybe_storage_dead.get().contains(local) {
                 self.fail(location, format!("use of local {local:?}, which has no storage here"));
@@ -76,18 +78,28 @@ impl<'a, 'tcx> Visitor<'tcx> for Lint<'a, 'tcx> {
     }
 
     fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) {
-        match statement.kind {
+        match &statement.kind {
-            StatementKind::StorageLive(local) => {
+            StatementKind::Assign(box (dest, rvalue)) => {
-                if self.reachable_blocks.contains(location.block) {
+                if let Rvalue::Use(Operand::Copy(src) | Operand::Move(src)) = rvalue {
-                    self.maybe_storage_live.seek_before_primary_effect(location);
+                    // The sides of an assignment must not alias. Currently this just checks whether
-                    if self.maybe_storage_live.get().contains(local) {
+                    // the places are identical.
+                    if dest == src {
                         self.fail(
                             location,
-                            format!("StorageLive({local:?}) which already has storage here"),
+                            "encountered `Assign` statement with overlapping memory",
                         );
                     }
                 }
             }
+            StatementKind::StorageLive(local) => {
+                self.maybe_storage_live.seek_before_primary_effect(location);
+                if self.maybe_storage_live.get().contains(*local) {
+                    self.fail(
+                        location,
+                        format!("StorageLive({local:?}) which already has storage here"),
+                    );
+                }
+            }
             _ => {}
         }
 
@@ -95,9 +107,9 @@ impl<'a, 'tcx> Visitor<'tcx> for Lint<'a, 'tcx> {
     }
 
     fn visit_terminator(&mut self, terminator: &Terminator<'tcx>, location: Location) {
-        match terminator.kind {
+        match &terminator.kind {
             TerminatorKind::Return => {
-                if self.is_fn_like && self.reachable_blocks.contains(location.block) {
+                if self.is_fn_like {
                     self.maybe_storage_live.seek_after_primary_effect(location);
                     for local in self.maybe_storage_live.get().iter() {
                         if !self.always_live_locals.contains(local) {
@@ -111,6 +123,28 @@ impl<'a, 'tcx> Visitor<'tcx> for Lint<'a, 'tcx> {
                     }
                 }
             }
+            TerminatorKind::Call { args, destination, .. } => {
+                // The call destination place and Operand::Move place used as an argument might be
+                // passed by a reference to the callee. Consequently they must be non-overlapping.
+                // Currently this simply checks for duplicate places.
+                self.places.clear();
+                self.places.insert(destination.as_ref());
+                let mut has_duplicates = false;
+                for arg in args {
+                    if let Operand::Move(place) = arg {
+                        has_duplicates |= !self.places.insert(place.as_ref());
+                    }
+                }
+                if has_duplicates {
+                    self.fail(
+                        location,
+                        format!(
+                            "encountered overlapping memory in `Move` arguments to `Call` terminator: {:?}",
+                            terminator.kind,
+                        ),
+                    );
+                }
+            }
             _ => {}
         }
 

compiler/rustc_mir_transform/src/pass_manager.rs

@@ -109,14 +109,15 @@ fn run_passes_inner<'tcx>(
     phase_change: Option<MirPhase>,
     validate_each: bool,
 ) {
-    let lint = tcx.sess.opts.unstable_opts.lint_mir & !body.should_skip();
-    let validate = validate_each & tcx.sess.opts.unstable_opts.validate_mir & !body.should_skip();
     let overridden_passes = &tcx.sess.opts.unstable_opts.mir_enable_passes;
     trace!(?overridden_passes);
 
     let prof_arg = tcx.sess.prof.enabled().then(|| format!("{:?}", body.source.def_id()));
 
     if !body.should_skip() {
+        let validate = validate_each & tcx.sess.opts.unstable_opts.validate_mir;
+        let lint = tcx.sess.opts.unstable_opts.lint_mir;
+
         for pass in passes {
             let name = pass.name();
 
@@ -162,7 +163,12 @@ fn run_passes_inner<'tcx>(
         body.pass_count = 0;
 
         dump_mir_for_phase_change(tcx, body);
-        if validate || new_phase == MirPhase::Runtime(RuntimePhase::Optimized) {
+
+        let validate =
+            (validate_each & tcx.sess.opts.unstable_opts.validate_mir & !body.should_skip())
+                || new_phase == MirPhase::Runtime(RuntimePhase::Optimized);
+        let lint = tcx.sess.opts.unstable_opts.lint_mir & !body.should_skip();
+        if validate {
             validate_body(tcx, body, format!("after phase change to {}", new_phase.name()));
         }
         if lint {

tests/mir-opt/copy-prop/custom_move_arg.f.CopyProp.panic-unwind.diff

@@ -8,14 +8,14 @@
   
       bb0: {
 -         _2 = _1;
--         _0 = opaque::<NotCopy>(move _1) -> [return: bb1, unwind continue];
+-         _0 = opaque::<NotCopy>(move _1) -> [return: bb1, unwind unreachable];
-+         _0 = opaque::<NotCopy>(_1) -> [return: bb1, unwind continue];
++         _0 = opaque::<NotCopy>(_1) -> [return: bb1, unwind unreachable];
       }
   
       bb1: {
 -         _3 = move _2;
--         _0 = opaque::<NotCopy>(_3) -> [return: bb2, unwind continue];
+-         _0 = opaque::<NotCopy>(_3) -> [return: bb2, unwind unreachable];
-+         _0 = opaque::<NotCopy>(_1) -> [return: bb2, unwind continue];
++         _0 = opaque::<NotCopy>(_1) -> [return: bb2, unwind unreachable];
       }
   
       bb2: {

tests/mir-opt/copy-prop/custom_move_arg.rs

@@ -10,15 +10,15 @@ use core::intrinsics::mir::*;
 struct NotCopy(bool);
 
 // EMIT_MIR custom_move_arg.f.CopyProp.diff
-#[custom_mir(dialect = "analysis", phase = "post-cleanup")]
+#[custom_mir(dialect = "runtime")]
 fn f(_1: NotCopy) {
     mir!({
         let _2 = _1;
-        Call(RET = opaque(Move(_1)), ReturnTo(bb1), UnwindContinue())
+        Call(RET = opaque(Move(_1)), ReturnTo(bb1), UnwindUnreachable())
     }
     bb1 = {
         let _3 = Move(_2);
-        Call(RET = opaque(_3), ReturnTo(bb2), UnwindContinue())
+        Call(RET = opaque(_3), ReturnTo(bb2), UnwindUnreachable())
     }
     bb2 = {
         Return()

tests/mir-opt/copy-prop/move_projection.f.CopyProp.panic-unwind.diff

@@ -9,13 +9,13 @@
       bb0: {
 -         _2 = _1;
 -         _3 = move (_2.0: u8);
--         _0 = opaque::<Foo>(move _1) -> [return: bb1, unwind continue];
+-         _0 = opaque::<Foo>(move _1) -> [return: bb1, unwind unreachable];
 +         _3 = (_1.0: u8);
-+         _0 = opaque::<Foo>(_1) -> [return: bb1, unwind continue];
++         _0 = opaque::<Foo>(_1) -> [return: bb1, unwind unreachable];
       }
   
       bb1: {
-          _0 = opaque::<u8>(move _3) -> [return: bb2, unwind continue];
+          _0 = opaque::<u8>(move _3) -> [return: bb2, unwind unreachable];
       }
   
       bb2: {

tests/mir-opt/copy-prop/move_projection.rs

@@ -11,17 +11,17 @@ fn opaque(_: impl Sized) -> bool { true }
 
 struct Foo(u8);
 
-#[custom_mir(dialect = "analysis", phase = "post-cleanup")]
+#[custom_mir(dialect = "runtime")]
 fn f(a: Foo) -> bool {
     mir!(
         {
             let b = a;
             // This is a move out of a copy, so must become a copy of `a.0`.
             let c = Move(b.0);
-            Call(RET = opaque(Move(a)), ReturnTo(bb1), UnwindContinue())
+            Call(RET = opaque(Move(a)), ReturnTo(bb1), UnwindUnreachable())
         }
         bb1 = {
-            Call(RET = opaque(Move(c)), ReturnTo(ret), UnwindContinue())
+            Call(RET = opaque(Move(c)), ReturnTo(ret), UnwindUnreachable())
         }
         ret = {
             Return()

tests/mir-opt/dest-prop/unreachable.f.DestinationPropagation.panic-abort.diff

@@ -1,82 +0,0 @@
-- // MIR for `f` before DestinationPropagation
-+ // MIR for `f` after DestinationPropagation
-  
-  fn f(_1: T) -> () {
-      debug a => _1;
-      let mut _0: ();
-      let _2: T;
-      let mut _3: bool;
-      let _4: ();
-      let mut _5: T;
-      let mut _6: T;
-      let _7: ();
-      let mut _8: T;
-      let mut _9: T;
-      scope 1 {
--         debug b => _2;
-+         debug b => _1;
-      }
-  
-      bb0: {
--         StorageLive(_2);
--         _2 = _1;
-+         nop;
-+         nop;
-          StorageLive(_3);
-          _3 = const false;
--         goto -> bb3;
-+         goto -> bb1;
-      }
-  
-      bb1: {
--         StorageLive(_4);
--         StorageLive(_5);
--         _5 = _1;
--         StorageLive(_6);
--         _6 = _1;
--         _4 = g::<T>(_1, _1) -> [return: bb2, unwind unreachable];
--     }
-- 
--     bb2: {
--         StorageDead(_6);
--         StorageDead(_5);
--         StorageDead(_4);
--         _0 = const ();
--         goto -> bb5;
--     }
-- 
--     bb3: {
-          StorageLive(_7);
--         StorageLive(_8);
--         _8 = _1;
--         StorageLive(_9);
--         _9 = _1;
--         _7 = g::<T>(_1, _1) -> [return: bb4, unwind unreachable];
-+         nop;
-+         nop;
-+         nop;
-+         nop;
-+         _7 = g::<T>(_1, _1) -> [return: bb2, unwind unreachable];
-      }
-  
--     bb4: {
--         StorageDead(_9);
--         StorageDead(_8);
-+     bb2: {
-+         nop;
-+         nop;
-          StorageDead(_7);
-          _0 = const ();
--         goto -> bb5;
-+         goto -> bb3;
-      }
-  
--     bb5: {
-+     bb3: {
-          StorageDead(_3);
--         StorageDead(_2);
-+         nop;
-          return;
-      }
-  }
-  

tests/mir-opt/dest-prop/unreachable.f.DestinationPropagation.panic-unwind.diff

@@ -1,82 +0,0 @@
-- // MIR for `f` before DestinationPropagation
-+ // MIR for `f` after DestinationPropagation
-  
-  fn f(_1: T) -> () {
-      debug a => _1;
-      let mut _0: ();
-      let _2: T;
-      let mut _3: bool;
-      let _4: ();
-      let mut _5: T;
-      let mut _6: T;
-      let _7: ();
-      let mut _8: T;
-      let mut _9: T;
-      scope 1 {
--         debug b => _2;
-+         debug b => _1;
-      }
-  
-      bb0: {
--         StorageLive(_2);
--         _2 = _1;
-+         nop;
-+         nop;
-          StorageLive(_3);
-          _3 = const false;
--         goto -> bb3;
-+         goto -> bb1;
-      }
-  
-      bb1: {
--         StorageLive(_4);
--         StorageLive(_5);
--         _5 = _1;
--         StorageLive(_6);
--         _6 = _1;
--         _4 = g::<T>(_1, _1) -> [return: bb2, unwind continue];
--     }
-- 
--     bb2: {
--         StorageDead(_6);
--         StorageDead(_5);
--         StorageDead(_4);
--         _0 = const ();
--         goto -> bb5;
--     }
-- 
--     bb3: {
-          StorageLive(_7);
--         StorageLive(_8);
--         _8 = _1;
--         StorageLive(_9);
--         _9 = _1;
--         _7 = g::<T>(_1, _1) -> [return: bb4, unwind continue];
-+         nop;
-+         nop;
-+         nop;
-+         nop;
-+         _7 = g::<T>(_1, _1) -> [return: bb2, unwind continue];
-      }
-  
--     bb4: {
--         StorageDead(_9);
--         StorageDead(_8);
-+     bb2: {
-+         nop;
-+         nop;
-          StorageDead(_7);
-          _0 = const ();
--         goto -> bb5;
-+         goto -> bb3;
-      }
-  
--     bb5: {
-+     bb3: {
-          StorageDead(_3);
--         StorageDead(_2);
-+         nop;
-          return;
-      }
-  }
-  

tests/mir-opt/dest-prop/unreachable.rs

@@ -1,20 +0,0 @@
-// skip-filecheck
-// EMIT_MIR_FOR_EACH_PANIC_STRATEGY
-// Check that unreachable code is removed after the destination propagation.
-// Regression test for issue #105428.
-//
-// compile-flags: --crate-type=lib -Zmir-opt-level=0
-// compile-flags: -Zmir-enable-passes=+GVN,+SimplifyConstCondition-after-const-prop,+DestinationPropagation
-
-// EMIT_MIR unreachable.f.DestinationPropagation.diff
-pub fn f<T: Copy>(a: T) {
-    let b = a;
-    if false {
-        g(a, b);
-    } else {
-        g(b, b);
-    }
-}
-
-#[inline(never)]
-pub fn g<T: Copy>(_: T, _: T) {}

tests/ui/mir/lint/assignment-overlap.rs

@@ -0,0 +1,19 @@
+// compile-flags: --crate-type=lib -Zlint-mir -Ztreat-err-as-bug
+// build-fail
+// failure-status: 101
+// dont-check-compiler-stderr
+// error-pattern: encountered `Assign` statement with overlapping memory
+#![feature(custom_mir, core_intrinsics)]
+extern crate core;
+use core::intrinsics::mir::*;
+
+#[custom_mir(dialect = "runtime", phase = "optimized")]
+pub fn main() {
+    mir!(
+        let a: [u8; 1024];
+        {
+            a = a;
+            Return()
+        }
+    )
+}

tests/ui/mir/lint/call-overlap.rs

@@ -0,0 +1,23 @@
+// compile-flags: -Zlint-mir -Ztreat-err-as-bug
+// build-fail
+// failure-status: 101
+// dont-check-compiler-stderr
+// error-pattern: encountered overlapping memory in `Move` arguments to `Call`
+#![feature(custom_mir, core_intrinsics)]
+extern crate core;
+use core::intrinsics::mir::*;
+
+#[custom_mir(dialect = "runtime", phase = "optimized")]
+pub fn main() {
+    mir!(
+        let a: [u8; 1024];
+        {
+            Call(a = f(Move(a)), ReturnTo(bb1), UnwindUnreachable())
+        }
+        bb1 = {
+            Return()
+        }
+    )
+}
+
+pub fn f<T: Copy>(a: T) -> T { a }