GVN: invalidate moved-from droppable locals on Operand::Move

compiler/rustc_mir_transform/src/gvn.rs

@@ -528,6 +528,25 @@ impl<'body, 'a, 'tcx> VnState<'body, 'a, 'tcx> {
         self.rev_locals[value].push(local);
     }
 
+    /// Drop everything GVN knows about `local`, so that later code cannot unify another
+    /// value-equivalent expression with it via `try_as_local` / `try_as_place`. This must
+    /// be called whenever the storage backing `local` is invalidated — in particular,
+    /// whenever `local` is moved out of, because `Operand::Move(p)` leaves `p` in an
+    /// unspecified state (the callee may mutate it, it may be partially deinitialised,
+    /// and it may no longer be dropped by this function).
+    ///
+    /// Before this was introduced, GVN happily rewrote a later aggregate whose components
+    /// happened to have the same VnIndex into `Operand::Copy(earlier_local)`, and the
+    /// subsequent `StorageRemover` pass downgraded `Operand::Move(earlier_local)` uses
+    /// to `Operand::Copy(earlier_local)`, turning a single `move` into a double-read of
+    /// freed/mutated memory. See <https://github.com/rust-lang/rust/issues/155241>.
+    #[instrument(level = "trace", skip(self))]
+    fn invalidate_local(&mut self, local: Local) {
+        if let Some(vn) = self.locals[local].take() {
+            self.rev_locals[vn].retain(|l| *l != local);
+        }
+    }
+
     fn insert_bool(&mut self, flag: bool) -> VnIndex {
         // Booleans are deterministic.
         let value = Const::from_bool(self.tcx, flag);
@@ -1014,8 +1033,32 @@ impl<'body, 'a, 'tcx> VnState<'body, 'a, 'tcx> {
         let value = match *operand {
             Operand::RuntimeChecks(c) => self.insert(self.tcx.types.bool, Value::RuntimeChecks(c)),
             Operand::Constant(ref constant) => self.insert_constant(constant.const_),
-            Operand::Copy(ref mut place) | Operand::Move(ref mut place) => {
-                self.simplify_place_value(place, location)?
+            Operand::Copy(ref mut place) => self.simplify_place_value(place, location)?,
+            Operand::Move(ref mut place) => {
+                // Compute the VnIndex first (using the same machinery as `Copy`, since for the
+                // purpose of *this* read `Move` and `Copy` yield the same value). Then, if the
+                // moved-from place actually has destructors to run, drop everything we know
+                // about the backing local. After that program point the storage behind
+                // `place.local` is in an unspecified state — continuing to treat `place.local`
+                // as holding the read value would let GVN unify a later rvalue with it and
+                // rewrite the later rvalue into `Operand::Copy(place.local)`, which (once
+                // `StorageRemover` rewrites the original `Move` to `Copy` too) turns a single
+                // move into a double read of freed / mutated memory.
+                // See <https://github.com/rust-lang/rust/issues/155241>.
+                //
+                // For types with no drop glue (`!needs_drop`), `Move` and `Copy` are
+                // observationally equivalent in the post-borrowck MIR that GVN runs on: the
+                // bit pattern at the source is preserved and there is no destructor that would
+                // free / overwrite it on the move path, so unifying later reads with
+                // `place.local` is sound. We keep the existing GVN behaviour in that case to
+                // avoid pessimising e.g. moves of `&mut T` and other non-`Copy` but
+                // non-droppable values.
+                let value = self.simplify_place_value(place, location)?;
+                let place_ty = place.ty(self.local_decls, self.tcx).ty;
+                if place_ty.needs_drop(self.tcx, self.typing_env()) {
+                    self.invalidate_local(place.local);
+                }
+                value
             }
         };
         if let Some(const_) = self.try_as_constant(value) {

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

@@ -12,6 +12,9 @@
       let mut _7: bool;
       let mut _8: bool;
       let mut _9: isize;
+      let mut _10: isize;
+      let mut _11: isize;
+      let mut _12: isize;
       scope 1 {
           debug _trailers => _5;
       }
@@ -25,7 +28,7 @@
           _7 = const true;
           _8 = const true;
           _4 = discriminant(_1);
-          switchInt(copy _4) -> [0: bb2, 1: bb4, otherwise: bb1];
+          switchInt(move _4) -> [0: bb2, 1: bb4, otherwise: bb1];
       }
 
       bb1: {
@@ -34,12 +37,12 @@
 
       bb2: {
           _3 = discriminant(((_1 as Ready).0: std::result::Result<std::option::Option<std::vec::Vec<u8>>, u8>));
-          switchInt(copy _3) -> [0: bb3, 1: bb6, otherwise: bb1];
+          switchInt(move _3) -> [0: bb3, 1: bb6, otherwise: bb1];
       }
 
       bb3: {
           _2 = 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(copy _2) -> [0: bb5, 1: bb7, otherwise: bb1];
+          switchInt(move _2) -> [0: bb5, 1: bb7, otherwise: bb1];
       }
 
       bb4: {
@@ -108,19 +111,22 @@
       }
 
       bb17: {
-          switchInt(copy _4) -> [0: bb11, otherwise: bb10];
+          _10 = discriminant(_1);
+          switchInt(move _10) -> [0: bb11, otherwise: bb10];
       }
 
       bb18 (cleanup): {
-          switchInt(copy _3) -> [0: bb19, otherwise: bb9];
+          _11 = discriminant(((_1 as Ready).0: std::result::Result<std::option::Option<std::vec::Vec<u8>>, u8>));
+          switchInt(move _11) -> [0: bb19, otherwise: bb9];
       }
 
       bb19 (cleanup): {
           goto -> bb9;
       }
 
       bb20 (cleanup): {
-          switchInt(copy _4) -> [0: bb18, otherwise: bb9];
+          _12 = discriminant(_1);
+          switchInt(move _12) -> [0: bb18, otherwise: bb9];
       }
   }
 

tests/mir-opt/early_otherwise_branch_unwind.rs

@@ -13,6 +13,11 @@ fn unwind<T>(val: Option<Option<Option<T>>>) {
     // CHECK-LABEL: fn unwind(
     // CHECK: drop({{.*}}) -> [return: bb{{.*}}, unwind: [[PARENT_UNWIND_BB:bb.*]]];
     // CHECK: [[PARENT_UNWIND_BB]] (cleanup): {
+    // After <https://github.com/rust-lang/rust/issues/155241>, GVN no longer
+    // shares the discriminant locals across the move of `_v`, so
+    // `EarlyOtherwiseBranch` introduces a fresh `discriminant` read here
+    // before the `switchInt`.
+    // CHECK-NEXT: discriminant
     // CHECK-NEXT: switchInt
     match val {
         Some(Some(Some(_v))) => {}
@@ -28,6 +33,11 @@ pub fn poll(val: Poll<Result<Option<Vec<u8>>, u8>>) {
     // CHECK-LABEL: fn poll(
     // CHECK: drop({{.*}}) -> [return: bb{{.*}}, unwind: [[PARENT_UNWIND_BB:bb.*]]];
     // CHECK: [[PARENT_UNWIND_BB]] (cleanup): {
+    // After <https://github.com/rust-lang/rust/issues/155241>, GVN no longer
+    // shares the discriminant locals across the move of `_trailers`, so
+    // `EarlyOtherwiseBranch` introduces a fresh `discriminant` read here
+    // before the `switchInt`.
+    // CHECK-NEXT: discriminant
     // CHECK-NEXT: switchInt
     match val {
         Poll::Ready(Ok(Some(_trailers))) => {}

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

@@ -11,6 +11,9 @@
       let mut _6: bool;
       let mut _7: bool;
       let mut _8: isize;
+      let mut _9: isize;
+      let mut _10: isize;
+      let mut _11: isize;
       scope 1 {
           debug _v => _5;
       }
@@ -21,7 +24,7 @@
           _6 = const true;
           _7 = const true;
           _4 = discriminant(_1);
-          switchInt(copy _4) -> [0: bb4, 1: bb2, otherwise: bb1];
+          switchInt(move _4) -> [0: bb4, 1: bb2, otherwise: bb1];
       }
 
       bb1: {
@@ -30,12 +33,12 @@
 
       bb2: {
           _3 = discriminant(((_1 as Some).0: std::option::Option<std::option::Option<T>>));
-          switchInt(copy _3) -> [0: bb5, 1: bb3, otherwise: bb1];
+          switchInt(move _3) -> [0: bb5, 1: bb3, otherwise: bb1];
       }
 
       bb3: {
           _2 = discriminant(((((_1 as Some).0: std::option::Option<std::option::Option<T>>) as Some).0: std::option::Option<T>));
-          switchInt(copy _2) -> [0: bb6, 1: bb7, otherwise: bb1];
+          switchInt(move _2) -> [0: bb6, 1: bb7, otherwise: bb1];
       }
 
       bb4: {
@@ -101,19 +104,22 @@
       }
 
       bb17: {
-          switchInt(copy _4) -> [1: bb11, otherwise: bb10];
+          _9 = discriminant(_1);
+          switchInt(move _9) -> [1: bb11, otherwise: bb10];
       }
 
       bb18 (cleanup): {
-          switchInt(copy _3) -> [1: bb19, otherwise: bb9];
+          _10 = discriminant(((_1 as Some).0: std::option::Option<std::option::Option<T>>));
+          switchInt(move _10) -> [1: bb19, otherwise: bb9];
       }
 
       bb19 (cleanup): {
           goto -> bb9;
       }
 
       bb20 (cleanup): {
-          switchInt(copy _4) -> [1: bb18, otherwise: bb9];
+          _11 = discriminant(_1);
+          switchInt(move _11) -> [1: bb18, otherwise: bb9];
       }
   }
 

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

@@ -20,8 +20,7 @@
 
       bb0: {
           StorageLive(_2);
--         StorageLive(_3);
-+         nop;
+          StorageLive(_3);
           StorageLive(_4);
           _4 = copy _1;
 -         _3 = move _4() -> [return: bb1, unwind unreachable];
@@ -30,22 +29,15 @@
 
       bb1: {
           StorageDead(_4);
--         StorageLive(_5);
--         _5 = move ((_3 as Some).0: T);
--         _2 = move _5;
--         StorageDead(_5);
-+         nop;
-+         _5 = copy ((_3 as Some).0: T);
-+         _2 = copy _5;
-+         nop;
+          StorageLive(_5);
+          _5 = move ((_3 as Some).0: T);
+          _2 = move _5;
+          StorageDead(_5);
           _7 = discriminant(_3);
--         StorageDead(_3);
-+         nop;
+          StorageDead(_3);
           StorageLive(_6);
--         _6 = move _2;
--         _0 = AlwaysSome::<T>::Some(move _6);
-+         _6 = copy _5;
-+         _0 = copy _3;
+          _6 = move _2;
+          _0 = AlwaysSome::<T>::Some(move _6);
           StorageDead(_6);
           StorageDead(_2);
           return;

tests/mir-opt/gvn_copy_aggregate.rs

@@ -235,8 +235,12 @@ fn remove_storage_dead<T>(f: fn() -> AlwaysSome<T>) -> AlwaysSome<T> {
     // CHECK-LABEL: fn remove_storage_dead(
     // CHECK: [[V1:_.*]] = copy _1() -> [return: [[BB1:bb.*]],
     // CHECK: [[BB1]]: {
-    // CHECK-NOT: StorageDead([[V1]]);
-    // CHECK: _0 = copy [[V1]];
+    // After <https://github.com/rust-lang/rust/issues/155241>, GVN no longer
+    // unifies the rebuilt `AlwaysSome::<T>::Some(_)` with `[[V1]]`, because
+    // the `Move` of `((... as Some).0: T)` invalidates `[[V1]]`'s recorded
+    // value. The aggregate-to-copy rewrite (and the related `StorageDead`
+    // elision) therefore no longer fires here.
+    // CHECK-NOT: _0 = copy [[V1]];
     let v = {
         match f() {
             AlwaysSome::Some(v) => v,

tests/mir-opt/gvn_move_invalidation.move_then_rebuild_droppy.GVN.diff

@@ -0,0 +1,28 @@
+- // MIR for `move_then_rebuild_droppy` before GVN
++ // MIR for `move_then_rebuild_droppy` after GVN
+
+  fn move_then_rebuild_droppy() -> () {
+      let mut _0: ();
+      let mut _1: ();
+      let mut _2: Wrap;
+      let mut _3: Wrap;
+      let mut _4: Wide;
+      let mut _5: Wide;
+
+      bb0: {
+          _4 = Wide { a: const 1_u32, b: const 2_u32, c: const 3_u32, d: const 4_u32 };
+          _2 = Wrap(move _4);
+          _1 = consume::<Wrap>(move _2) -> [return: bb1, unwind unreachable];
+      }
+
+      bb1: {
+          _5 = Wide { a: const 1_u32, b: const 2_u32, c: const 3_u32, d: const 4_u32 };
+          _3 = Wrap(move _5);
+          _1 = consume::<Wrap>(move _3) -> [return: bb2, unwind unreachable];
+      }
+
+      bb2: {
+          return;
+      }
+  }
+

tests/mir-opt/gvn_move_invalidation.move_then_rebuild_plain.GVN.diff

@@ -0,0 +1,28 @@
+- // MIR for `move_then_rebuild_plain` before GVN
++ // MIR for `move_then_rebuild_plain` after GVN
+
+  fn move_then_rebuild_plain() -> () {
+      let mut _0: ();
+      let mut _1: ();
+      let mut _2: (u64,);
+      let mut _3: (u64,);
+
+      bb0: {
+-         _2 = (const 1_u64,);
+-         _1 = consume::<(u64,)>(move _2) -> [return: bb1, unwind unreachable];
++         _2 = const (1_u64,);
++         _1 = consume::<(u64,)>(const (1_u64,)) -> [return: bb1, unwind unreachable];
+      }
+
+      bb1: {
+-         _3 = (const 1_u64,);
+-         _1 = consume::<(u64,)>(move _3) -> [return: bb2, unwind unreachable];
++         _3 = const (1_u64,);
++         _1 = consume::<(u64,)>(const (1_u64,)) -> [return: bb2, unwind unreachable];
+      }
+
+      bb2: {
+          return;
+      }
+  }
+