diff --git a/compiler/rustc_mir_transform/src/gvn.rs b/compiler/rustc_mir_transform/src/gvn.rs index e9a20aa016550..bff419aec3bfd 100644 --- a/compiler/rustc_mir_transform/src/gvn.rs +++ b/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 . + #[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 . + // + // 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) { diff --git a/tests/mir-opt/early_otherwise_branch_unwind.poll.EarlyOtherwiseBranch.diff b/tests/mir-opt/early_otherwise_branch_unwind.poll.EarlyOtherwiseBranch.diff index b6450e43b09e6..e0323d7800b60 100644 --- a/tests/mir-opt/early_otherwise_branch_unwind.poll.EarlyOtherwiseBranch.diff +++ b/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>, 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>, u8>) as Ok).0: std::option::Option>)); - switchInt(copy _2) -> [0: bb5, 1: bb7, otherwise: bb1]; + switchInt(move _2) -> [0: bb5, 1: bb7, otherwise: bb1]; } bb4: { @@ -108,11 +111,13 @@ } 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>, u8>)); + switchInt(move _11) -> [0: bb19, otherwise: bb9]; } bb19 (cleanup): { @@ -120,7 +125,8 @@ } bb20 (cleanup): { - switchInt(copy _4) -> [0: bb18, otherwise: bb9]; + _12 = discriminant(_1); + switchInt(move _12) -> [0: bb18, otherwise: bb9]; } } diff --git a/tests/mir-opt/early_otherwise_branch_unwind.rs b/tests/mir-opt/early_otherwise_branch_unwind.rs index cbccf11729ab3..9638177ddcfa5 100644 --- a/tests/mir-opt/early_otherwise_branch_unwind.rs +++ b/tests/mir-opt/early_otherwise_branch_unwind.rs @@ -13,6 +13,11 @@ fn unwind(val: Option>>) { // CHECK-LABEL: fn unwind( // CHECK: drop({{.*}}) -> [return: bb{{.*}}, unwind: [[PARENT_UNWIND_BB:bb.*]]]; // CHECK: [[PARENT_UNWIND_BB]] (cleanup): { + // After , 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>, u8>>) { // CHECK-LABEL: fn poll( // CHECK: drop({{.*}}) -> [return: bb{{.*}}, unwind: [[PARENT_UNWIND_BB:bb.*]]]; // CHECK: [[PARENT_UNWIND_BB]] (cleanup): { + // After , 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))) => {} diff --git a/tests/mir-opt/early_otherwise_branch_unwind.unwind.EarlyOtherwiseBranch.diff b/tests/mir-opt/early_otherwise_branch_unwind.unwind.EarlyOtherwiseBranch.diff index 2b2c007e082a7..0a4530e60b47b 100644 --- a/tests/mir-opt/early_otherwise_branch_unwind.unwind.EarlyOtherwiseBranch.diff +++ b/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>)); - 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>) as Some).0: std::option::Option)); - switchInt(copy _2) -> [0: bb6, 1: bb7, otherwise: bb1]; + switchInt(move _2) -> [0: bb6, 1: bb7, otherwise: bb1]; } bb4: { @@ -101,11 +104,13 @@ } 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>)); + switchInt(move _10) -> [1: bb19, otherwise: bb9]; } bb19 (cleanup): { @@ -113,7 +118,8 @@ } bb20 (cleanup): { - switchInt(copy _4) -> [1: bb18, otherwise: bb9]; + _11 = discriminant(_1); + switchInt(move _11) -> [1: bb18, otherwise: bb9]; } } diff --git a/tests/mir-opt/gvn_copy_aggregate.remove_storage_dead.GVN.diff b/tests/mir-opt/gvn_copy_aggregate.remove_storage_dead.GVN.diff index c9cfc7efcd1a6..2ea134c7d9c4e 100644 --- a/tests/mir-opt/gvn_copy_aggregate.remove_storage_dead.GVN.diff +++ b/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::::Some(move _6); -+ _6 = copy _5; -+ _0 = copy _3; + _6 = move _2; + _0 = AlwaysSome::::Some(move _6); StorageDead(_6); StorageDead(_2); return; diff --git a/tests/mir-opt/gvn_copy_aggregate.rs b/tests/mir-opt/gvn_copy_aggregate.rs index c961388241545..7b536c9b273b5 100644 --- a/tests/mir-opt/gvn_copy_aggregate.rs +++ b/tests/mir-opt/gvn_copy_aggregate.rs @@ -235,8 +235,12 @@ fn remove_storage_dead(f: fn() -> AlwaysSome) -> AlwaysSome { // CHECK-LABEL: fn remove_storage_dead( // CHECK: [[V1:_.*]] = copy _1() -> [return: [[BB1:bb.*]], // CHECK: [[BB1]]: { - // CHECK-NOT: StorageDead([[V1]]); - // CHECK: _0 = copy [[V1]]; + // After , GVN no longer + // unifies the rebuilt `AlwaysSome::::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, diff --git a/tests/mir-opt/gvn_move_invalidation.move_then_rebuild_droppy.GVN.diff b/tests/mir-opt/gvn_move_invalidation.move_then_rebuild_droppy.GVN.diff new file mode 100644 index 0000000000000..31d068b54d3d7 --- /dev/null +++ b/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::(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::(move _3) -> [return: bb2, unwind unreachable]; + } + + bb2: { + return; + } + } + diff --git a/tests/mir-opt/gvn_move_invalidation.move_then_rebuild_plain.GVN.diff b/tests/mir-opt/gvn_move_invalidation.move_then_rebuild_plain.GVN.diff new file mode 100644 index 0000000000000..b4c53d095006a --- /dev/null +++ b/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; + } + } + diff --git a/tests/mir-opt/gvn_move_invalidation.rs b/tests/mir-opt/gvn_move_invalidation.rs new file mode 100644 index 0000000000000..904d49056f1f8 --- /dev/null +++ b/tests/mir-opt/gvn_move_invalidation.rs @@ -0,0 +1,121 @@ +// Regression test for : +// after `Operand::Move`-ing a non-`Copy` (`needs_drop`) place, GVN must invalidate +// the moved-from local so it cannot be reused as the destination of an +// aggregate-to-copy rewrite. Otherwise GVN may rewrite a fresh aggregate into +// `Operand::Copy(earlier_local)` and `StorageRemover` will downgrade the original +// `Move` to `Copy`, turning a single move into a double-use of freed memory. +// +//@ test-mir-pass: GVN +//@ compile-flags: -Cpanic=abort + +#![feature(custom_mir, core_intrinsics)] +#![allow(internal_features)] + +use std::intrinsics::mir::*; + +#[inline(never)] +fn consume(_: T) {} + +// A struct with a non-trivial layout (more than a scalar pair, so GVN cannot +// fold it to a constant) and explicit drop glue, so `needs_drop` reports true +// and the invalidation in `simplify_operand` fires. Two `Wide { .. }` rvalues +// are still bit-identical aggregates and would receive the same `VnIndex` +// without the fix. +pub struct Wide { + a: u32, + b: u32, + c: u32, + d: u32, +} +impl Drop for Wide { + fn drop(&mut self) {} +} +pub struct Wrap(Wide); + +// EMIT_MIR gvn_move_invalidation.move_then_rebuild_droppy.GVN.diff +#[custom_mir(dialect = "runtime", phase = "post-cleanup")] +fn move_then_rebuild_droppy() { + // CHECK-LABEL: fn move_then_rebuild_droppy( + // + // Two distinct `Wrap(Wide { .. })` aggregates — bitwise-identical, same + // `VnIndex` by construction — separated by a call that consumes the first. + // GVN must NOT rewrite the second `Wrap(..)` into `_3 = copy _2` (and the + // following `consume(move _3)` into `consume(copy _2)`): doing so would + // make the two `move _2`/`move _3` operands two uses of `_2`'s freed + // contents. + // + // CHECK: bb0: { + // CHECK: _4 = Wide + // CHECK: _2 = Wrap(move _4); + // CHECK-NEXT: = consume::(move _2) + // + // CHECK: bb1: { + // CHECK-NOT: _3 = copy _2; + // CHECK-NOT: = consume::(copy _2) + // CHECK: _5 = Wide + // CHECK: _3 = Wrap(move _5); + // CHECK-NEXT: = consume::(move _3) + mir! { + let _1: (); + let _2: Wrap; + let _3: Wrap; + let _4: Wide; + let _5: Wide; + { + _4 = Wide { a: 1_u32, b: 2_u32, c: 3_u32, d: 4_u32 }; + _2 = Wrap(Move(_4)); + Call(_1 = consume::(Move(_2)), ReturnTo(bb_second), UnwindUnreachable()) + } + bb_second = { + _5 = Wide { a: 1_u32, b: 2_u32, c: 3_u32, d: 4_u32 }; + _3 = Wrap(Move(_5)); + Call(_1 = consume::(Move(_3)), ReturnTo(bb_done), UnwindUnreachable()) + } + bb_done = { + Return() + } + } +} + +// For comparison: when the moved type has no drop glue, the GVN unification +// still fires (post-fix) because moving a `!needs_drop` value is semantically +// equivalent to copying it and the source location is not invalidated. Here +// GVN folds the tuple to a constant and reuses the same constant for both +// calls. +// +// EMIT_MIR gvn_move_invalidation.move_then_rebuild_plain.GVN.diff +#[custom_mir(dialect = "runtime", phase = "post-cleanup")] +fn move_then_rebuild_plain() { + // CHECK-LABEL: fn move_then_rebuild_plain( + // + // CHECK: bb0: { + // CHECK: _2 = const (1_u64,); + // CHECK-NEXT: = consume::<(u64,)>(const (1_u64,)) + // + // CHECK: bb1: { + // GVN happily folds the second aggregate to the same constant — `(u64,)` is + // `!needs_drop` and the post-move bit pattern is preserved. + // CHECK: _3 = const (1_u64,); + // CHECK-NEXT: = consume::<(u64,)>(const (1_u64,)) + mir! { + let _1: (); + let _2: (u64,); + let _3: (u64,); + { + _2 = (1_u64,); + Call(_1 = consume::<(u64,)>(Move(_2)), ReturnTo(bb_second), UnwindUnreachable()) + } + bb_second = { + _3 = (1_u64,); + Call(_1 = consume::<(u64,)>(Move(_3)), ReturnTo(bb_done), UnwindUnreachable()) + } + bb_done = { + Return() + } + } +} + +fn main() { + move_then_rebuild_droppy(); + move_then_rebuild_plain(); +} diff --git a/tests/mir-opt/pre-codegen/try_identity.old.PreCodegen.after.mir b/tests/mir-opt/pre-codegen/try_identity.old.PreCodegen.after.mir index 889e80d26e1cc..ac485f485b1cc 100644 --- a/tests/mir-opt/pre-codegen/try_identity.old.PreCodegen.after.mir +++ b/tests/mir-opt/pre-codegen/try_identity.old.PreCodegen.after.mir @@ -19,14 +19,14 @@ fn old(_1: Result) -> Result { } bb1: { - _3 = copy ((_1 as Ok).0: T); - _0 = copy _1; + _3 = move ((_1 as Ok).0: T); + _0 = Result::::Ok(copy _3); goto -> bb3; } bb2: { - _4 = copy ((_1 as Err).0: E); - _0 = copy _1; + _4 = move ((_1 as Err).0: E); + _0 = Result::::Err(copy _4); goto -> bb3; } diff --git a/tests/ui/mir/gvn-fnonce-miscompile.rs b/tests/ui/mir/gvn-fnonce-miscompile.rs new file mode 100644 index 0000000000000..65c98b0bd46f2 --- /dev/null +++ b/tests/ui/mir/gvn-fnonce-miscompile.rs @@ -0,0 +1,22 @@ +//! Regression test for a GVN miscompile that turned two `move`d closures into a double-use of +//! the same stack slot, because GVN unified the value-numbers of two distinct empty `Vec`-tuple +//! aggregates and rewrote the later one into `Operand::Copy(earlier_local)` after that earlier +//! local had already been moved out of by an `FnOnce` call. +//! +//! See . +//! +//! Without the fix, the second `with(...)` call observes the first closure's `Vec` (or freed +//! memory) instead of a freshly-constructed empty `Vec`, leading to either an `assert_eq!` +//! failure or heap corruption (SIGABRT) at `-Copt-level=2` and above. + +//@ run-pass +//@ compile-flags: -Copt-level=3 -Zmir-enable-passes=+GVN + +fn with(f: impl FnOnce(Vec)) { + f(Vec::new()) +} + +fn main() { + with(|mut v| v.resize(2, 1)); + with(|v| assert_eq!(v.len(), 0, "second closure must observe a fresh, empty Vec")); +}