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.

compiler/rustc_const_eval/src/transform/validate.rs

@@ -74,7 +74,6 @@ fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
             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 @@ fn visit_local(&mut self, local: Local, _context: PlaceContext, location: Locati
 
     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 @@ fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) {
                     self.fail(location, format!("explicit `{kind:?}` is forbidden"));
                 }
             }
-            StatementKind::StorageLive(_)
+            StatementKind::Assign(..)
+            | StatementKind::StorageLive(_)
             | StatementKind::StorageDead(_)
             | StatementKind::Intrinsic(_)
             | StatementKind::Coverage(_)
@@ -404,10 +390,7 @@ fn visit_terminator(&mut self, terminator: &Terminator<'tcx>, location: Location
                 }
 
                 // 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 @@ fn visit_terminator(&mut self, terminator: &Terminator<'tcx>, location: Location
                         ),
                     );
                 }
-                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 @@ fn visit_terminator(&mut self, terminator: &Terminator<'tcx>, location: Location
                         }
                     }
                 }
-
-                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 @@ fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) {
                         )
                     }
                 }
-                // 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 @@ fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
             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::*;
@@ -31,6 +32,7 @@ pub fn lint_body<'tcx>(tcx: TyCtxt<'tcx>, body: &Body<'tcx>, when: String) {
         always_live_locals,
         maybe_storage_live,
         maybe_storage_dead,
+        places: Default::default(),
     };
     for (bb, data) in traversal::reachable(body) {
         lint.visit_basic_block_data(bb, data);
@@ -45,6 +47,7 @@ struct Lint<'a, 'tcx> {
     always_live_locals: &'a BitSet<Local>,
     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> {
@@ -75,10 +78,22 @@ fn visit_local(&mut self, local: Local, context: PlaceContext, location: Locatio
     }
 
     fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) {
-        match statement.kind {
+        match &statement.kind {
+            StatementKind::Assign(box (dest, rvalue)) => {
+                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::StorageLive(local) => {
                 self.maybe_storage_live.seek_before_primary_effect(location);
-                if self.maybe_storage_live.get().contains(local) {
+                if self.maybe_storage_live.get().contains(*local) {
                     self.fail(
                         location,
                         format!("StorageLive({local:?}) which already has storage here"),
@@ -92,7 +107,7 @@ fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) {
     }
 
     fn visit_terminator(&mut self, terminator: &Terminator<'tcx>, location: Location) {
-        match terminator.kind {
+        match &terminator.kind {
             TerminatorKind::Return => {
                 if self.is_fn_like {
                     self.maybe_storage_live.seek_after_primary_effect(location);
@@ -108,6 +123,28 @@ fn visit_terminator(&mut self, terminator: &Terminator<'tcx>, location: Location
                     }
                 }
             }
+            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,
+                        ),
+                    );
+                }
+            }
             _ => {}
         }
 

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 }