From 73831192fb83a8bf1af9ad4346d7454f1bdb1cd8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Mi=C4=85sko?= Date: Sat, 18 Oct 2025 14:52:23 +0200 Subject: [PATCH] Turn moves into copies after copy propagation Previously copy propagation presumed that there is further unspecified distinction between move operands and copy operands in assignments and propagated moves from assignments into terminators. This is inconsistent with current operational semantics. Turn moves into copies after copy propagation to preserve existing behavior. Fixes https://github.com/rust-lang/rust/issues/137936. Fixes https://github.com/rust-lang/rust/issues/146423. --- compiler/rustc_mir_transform/src/copy_prop.rs | 63 +++---------------- .../move_arg.f.CopyProp.panic-abort.diff | 37 ----------- .../move_arg.f.CopyProp.panic-unwind.diff | 37 ----------- tests/mir-opt/copy-prop/move_arg.rs | 51 +++++++++++---- .../reborrow.remut.CopyProp.panic-abort.diff | 2 +- .../reborrow.remut.CopyProp.panic-unwind.diff | 2 +- .../reborrow.reraw.CopyProp.panic-abort.diff | 2 +- .../reborrow.reraw.CopyProp.panic-unwind.diff | 2 +- tests/ui/lint/large_assignments/inline_mir.rs | 2 +- .../lint/large_assignments/inline_mir.stderr | 10 ++- .../ui/lint/large_assignments/move_into_fn.rs | 2 +- .../large_assignments/move_into_fn.stderr | 10 ++- 12 files changed, 74 insertions(+), 146 deletions(-) delete mode 100644 tests/mir-opt/copy-prop/move_arg.f.CopyProp.panic-abort.diff delete mode 100644 tests/mir-opt/copy-prop/move_arg.f.CopyProp.panic-unwind.diff diff --git a/compiler/rustc_mir_transform/src/copy_prop.rs b/compiler/rustc_mir_transform/src/copy_prop.rs index 2e9c3a5bf3eeb..89ee96317ac93 100644 --- a/compiler/rustc_mir_transform/src/copy_prop.rs +++ b/compiler/rustc_mir_transform/src/copy_prop.rs @@ -16,7 +16,7 @@ use crate::ssa::SsaLocals; /// _d = move? _c /// where each of the locals is only assigned once. /// -/// We want to replace all those locals by `_a`, either copied or moved. +/// We want to replace all those locals by `copy _a`. pub(super) struct CopyProp; impl<'tcx> crate::MirPass<'tcx> for CopyProp { @@ -34,11 +34,13 @@ impl<'tcx> crate::MirPass<'tcx> for CopyProp { debug!(copy_classes = ?ssa.copy_classes()); let mut any_replacement = false; - let mut storage_to_remove = DenseBitSet::new_empty(body.local_decls.len()); + // Locals that participate in copy propagation either as a source or a destination. + let mut unified = DenseBitSet::new_empty(body.local_decls.len()); for (local, &head) in ssa.copy_classes().iter_enumerated() { if local != head { any_replacement = true; - storage_to_remove.insert(head); + unified.insert(head); + unified.insert(local); } } @@ -46,11 +48,7 @@ impl<'tcx> crate::MirPass<'tcx> for CopyProp { return; } - let fully_moved = fully_moved_locals(&ssa, body); - debug!(?fully_moved); - - Replacer { tcx, copy_classes: ssa.copy_classes(), fully_moved, storage_to_remove } - .visit_body_preserves_cfg(body); + Replacer { tcx, copy_classes: ssa.copy_classes(), unified }.visit_body_preserves_cfg(body); crate::simplify::remove_unused_definitions(body); } @@ -60,44 +58,10 @@ impl<'tcx> crate::MirPass<'tcx> for CopyProp { } } -/// `SsaLocals` computed equivalence classes between locals considering copy/move assignments. -/// -/// This function also returns whether all the `move?` in the pattern are `move` and not copies. -/// A local which is in the bitset can be replaced by `move _a`. Otherwise, it must be -/// replaced by `copy _a`, as we cannot move multiple times from `_a`. -/// -/// If an operand copies `_c`, it must happen before the assignment `_d = _c`, otherwise it is UB. -/// This means that replacing it by a copy of `_a` if ok, since this copy happens before `_c` is -/// moved, and therefore that `_d` is moved. -#[instrument(level = "trace", skip(ssa, body))] -fn fully_moved_locals(ssa: &SsaLocals, body: &Body<'_>) -> DenseBitSet { - let mut fully_moved = DenseBitSet::new_filled(body.local_decls.len()); - - for (_, rvalue, _) in ssa.assignments(body) { - let Rvalue::Use(Operand::Copy(place) | Operand::Move(place)) = rvalue else { - continue; - }; - - let Some(rhs) = place.as_local() else { continue }; - if !ssa.is_ssa(rhs) { - continue; - } - - if let Rvalue::Use(Operand::Copy(_)) = rvalue { - fully_moved.remove(rhs); - } - } - - ssa.meet_copy_equivalence(&mut fully_moved); - - fully_moved -} - /// Utility to help performing substitution of `*pattern` by `target`. struct Replacer<'a, 'tcx> { tcx: TyCtxt<'tcx>, - fully_moved: DenseBitSet, - storage_to_remove: DenseBitSet, + unified: DenseBitSet, copy_classes: &'a IndexSlice, } @@ -108,13 +72,7 @@ impl<'tcx> MutVisitor<'tcx> for Replacer<'_, 'tcx> { #[tracing::instrument(level = "trace", skip(self))] fn visit_local(&mut self, local: &mut Local, ctxt: PlaceContext, _: Location) { - let new_local = self.copy_classes[*local]; - match ctxt { - // Do not modify the local in storage statements. - PlaceContext::NonUse(NonUseContext::StorageLive | NonUseContext::StorageDead) => {} - // We access the value. - _ => *local = new_local, - } + *local = self.copy_classes[*local]; } #[tracing::instrument(level = "trace", skip(self))] @@ -123,7 +81,7 @@ impl<'tcx> MutVisitor<'tcx> for Replacer<'_, 'tcx> { // A move out of a projection of a copy is equivalent to a copy of the original // projection. && !place.is_indirect_first_projection() - && !self.fully_moved.contains(place.local) + && self.unified.contains(place.local) { *operand = Operand::Copy(place); } @@ -134,10 +92,9 @@ impl<'tcx> MutVisitor<'tcx> for Replacer<'_, 'tcx> { fn visit_statement(&mut self, stmt: &mut Statement<'tcx>, loc: Location) { // When removing storage statements, we need to remove both (#107511). if let StatementKind::StorageLive(l) | StatementKind::StorageDead(l) = stmt.kind - && self.storage_to_remove.contains(l) + && self.unified.contains(l) { stmt.make_nop(true); - return; } self.super_statement(stmt, loc); diff --git a/tests/mir-opt/copy-prop/move_arg.f.CopyProp.panic-abort.diff b/tests/mir-opt/copy-prop/move_arg.f.CopyProp.panic-abort.diff deleted file mode 100644 index da9904bfa01f5..0000000000000 --- a/tests/mir-opt/copy-prop/move_arg.f.CopyProp.panic-abort.diff +++ /dev/null @@ -1,37 +0,0 @@ -- // MIR for `f` before CopyProp -+ // MIR for `f` after CopyProp - - fn f(_1: T) -> () { - debug a => _1; - let mut _0: (); - let _2: T; - let _3: (); - let mut _4: T; - let mut _5: T; - scope 1 { -- debug b => _2; -+ debug b => _1; - } - - bb0: { -- StorageLive(_2); -- _2 = copy _1; - StorageLive(_3); -- StorageLive(_4); -- _4 = copy _1; -- StorageLive(_5); -- _5 = copy _2; -- _3 = g::(move _4, move _5) -> [return: bb1, unwind unreachable]; -+ _3 = g::(copy _1, copy _1) -> [return: bb1, unwind unreachable]; - } - - bb1: { -- StorageDead(_5); -- StorageDead(_4); - StorageDead(_3); - _0 = const (); -- StorageDead(_2); - return; - } - } - diff --git a/tests/mir-opt/copy-prop/move_arg.f.CopyProp.panic-unwind.diff b/tests/mir-opt/copy-prop/move_arg.f.CopyProp.panic-unwind.diff deleted file mode 100644 index 9b0de655bd262..0000000000000 --- a/tests/mir-opt/copy-prop/move_arg.f.CopyProp.panic-unwind.diff +++ /dev/null @@ -1,37 +0,0 @@ -- // MIR for `f` before CopyProp -+ // MIR for `f` after CopyProp - - fn f(_1: T) -> () { - debug a => _1; - let mut _0: (); - let _2: T; - let _3: (); - let mut _4: T; - let mut _5: T; - scope 1 { -- debug b => _2; -+ debug b => _1; - } - - bb0: { -- StorageLive(_2); -- _2 = copy _1; - StorageLive(_3); -- StorageLive(_4); -- _4 = copy _1; -- StorageLive(_5); -- _5 = copy _2; -- _3 = g::(move _4, move _5) -> [return: bb1, unwind continue]; -+ _3 = g::(copy _1, copy _1) -> [return: bb1, unwind continue]; - } - - bb1: { -- StorageDead(_5); -- StorageDead(_4); - StorageDead(_3); - _0 = const (); -- StorageDead(_2); - return; - } - } - diff --git a/tests/mir-opt/copy-prop/move_arg.rs b/tests/mir-opt/copy-prop/move_arg.rs index 498340534324b..23e20021f5884 100644 --- a/tests/mir-opt/copy-prop/move_arg.rs +++ b/tests/mir-opt/copy-prop/move_arg.rs @@ -1,17 +1,46 @@ -// skip-filecheck -// EMIT_MIR_FOR_EACH_PANIC_STRATEGY // Test that we do not move multiple times from the same local. //@ test-mir-pass: CopyProp +//@ compile-flags: --crate-type=lib -Cpanic=abort +#![feature(custom_mir, core_intrinsics)] +use core::intrinsics::mir::*; +use core::mem::MaybeUninit; +extern crate core; -// EMIT_MIR move_arg.f.CopyProp.diff -pub fn f(a: T) { - let b = a; - g(a, b); +#[custom_mir(dialect = "runtime", phase = "initial")] +pub fn moved_and_copied(_1: T) { + // CHECK-LABEL: fn moved_and_copied( + // CHECK: _0 = f::(copy _1, copy _1) + mir! { + { + let _2 = _1; + Call(RET = f(Move(_1), Move(_2)), ReturnTo(bb1), UnwindUnreachable()) + } + bb1 = { + Return() + } + } } -#[inline(never)] -pub fn g(_: T, _: T) {} - -fn main() { - f(5) +#[custom_mir(dialect = "runtime", phase = "initial")] +pub fn moved_twice(_1: MaybeUninit) { + // In a future we would like to propagate moves instead of copies here. The resulting program + // would have an undefined behavior due to overlap in a call terminator, so we need to change + // operational semantics to explain why the original program has undefined behavior. + // https://github.com/rust-lang/unsafe-code-guidelines/issues/556 + // + // CHECK-LABEL: fn moved_twice( + // CHECK: _0 = f::>(copy _1, copy _1) + mir! { + { + let _2 = Move(_1); + let _3 = Move(_1); + Call(RET = f(Move(_2), Move(_3)), ReturnTo(bb1), UnwindUnreachable()) + } + bb1 = { + Return() + } + } } + +#[inline(never)] +pub fn f(_: T, _: T) {} diff --git a/tests/mir-opt/copy-prop/reborrow.remut.CopyProp.panic-abort.diff b/tests/mir-opt/copy-prop/reborrow.remut.CopyProp.panic-abort.diff index 2026c1982f299..8ef61b5667dd5 100644 --- a/tests/mir-opt/copy-prop/reborrow.remut.CopyProp.panic-abort.diff +++ b/tests/mir-opt/copy-prop/reborrow.remut.CopyProp.panic-abort.diff @@ -31,7 +31,7 @@ - StorageLive(_6); - _6 = move _4; - _5 = opaque::<&mut u8>(move _6) -> [return: bb1, unwind unreachable]; -+ _5 = opaque::<&mut u8>(move _2) -> [return: bb1, unwind unreachable]; ++ _5 = opaque::<&mut u8>(copy _2) -> [return: bb1, unwind unreachable]; } bb1: { diff --git a/tests/mir-opt/copy-prop/reborrow.remut.CopyProp.panic-unwind.diff b/tests/mir-opt/copy-prop/reborrow.remut.CopyProp.panic-unwind.diff index 67763fdce6676..2a7182af984d6 100644 --- a/tests/mir-opt/copy-prop/reborrow.remut.CopyProp.panic-unwind.diff +++ b/tests/mir-opt/copy-prop/reborrow.remut.CopyProp.panic-unwind.diff @@ -31,7 +31,7 @@ - StorageLive(_6); - _6 = move _4; - _5 = opaque::<&mut u8>(move _6) -> [return: bb1, unwind continue]; -+ _5 = opaque::<&mut u8>(move _2) -> [return: bb1, unwind continue]; ++ _5 = opaque::<&mut u8>(copy _2) -> [return: bb1, unwind continue]; } bb1: { diff --git a/tests/mir-opt/copy-prop/reborrow.reraw.CopyProp.panic-abort.diff b/tests/mir-opt/copy-prop/reborrow.reraw.CopyProp.panic-abort.diff index dfc8dd0975638..8a2cdd8e5728e 100644 --- a/tests/mir-opt/copy-prop/reborrow.reraw.CopyProp.panic-abort.diff +++ b/tests/mir-opt/copy-prop/reborrow.reraw.CopyProp.panic-abort.diff @@ -31,7 +31,7 @@ - StorageLive(_6); - _6 = move _4; - _5 = opaque::<&mut u8>(move _6) -> [return: bb1, unwind unreachable]; -+ _5 = opaque::<&mut u8>(move _2) -> [return: bb1, unwind unreachable]; ++ _5 = opaque::<&mut u8>(copy _2) -> [return: bb1, unwind unreachable]; } bb1: { diff --git a/tests/mir-opt/copy-prop/reborrow.reraw.CopyProp.panic-unwind.diff b/tests/mir-opt/copy-prop/reborrow.reraw.CopyProp.panic-unwind.diff index becc425632104..614d23cf62457 100644 --- a/tests/mir-opt/copy-prop/reborrow.reraw.CopyProp.panic-unwind.diff +++ b/tests/mir-opt/copy-prop/reborrow.reraw.CopyProp.panic-unwind.diff @@ -31,7 +31,7 @@ - StorageLive(_6); - _6 = move _4; - _5 = opaque::<&mut u8>(move _6) -> [return: bb1, unwind continue]; -+ _5 = opaque::<&mut u8>(move _2) -> [return: bb1, unwind continue]; ++ _5 = opaque::<&mut u8>(copy _2) -> [return: bb1, unwind continue]; } bb1: { diff --git a/tests/ui/lint/large_assignments/inline_mir.rs b/tests/ui/lint/large_assignments/inline_mir.rs index fc27b8ff244d1..d16aae6ab145b 100644 --- a/tests/ui/lint/large_assignments/inline_mir.rs +++ b/tests/ui/lint/large_assignments/inline_mir.rs @@ -20,5 +20,5 @@ pub fn main() { let data = [10u8; 9999]; let cell = std::cell::UnsafeCell::new(data); //~ ERROR large_assignments - std::hint::black_box(cell); + std::hint::black_box(cell); //~ ERROR large_assignments } diff --git a/tests/ui/lint/large_assignments/inline_mir.stderr b/tests/ui/lint/large_assignments/inline_mir.stderr index d9010e24d03be..1a5fcb6c8fc18 100644 --- a/tests/ui/lint/large_assignments/inline_mir.stderr +++ b/tests/ui/lint/large_assignments/inline_mir.stderr @@ -11,5 +11,13 @@ note: the lint level is defined here LL | #![deny(large_assignments)] | ^^^^^^^^^^^^^^^^^ -error: aborting due to 1 previous error +error: moving 9999 bytes + --> $DIR/inline_mir.rs:23:5 + | +LL | std::hint::black_box(cell); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ value moved from here + | + = note: The current maximum size is 1000, but it can be customized with the move_size_limit attribute: `#![move_size_limit = "..."]` + +error: aborting due to 2 previous errors diff --git a/tests/ui/lint/large_assignments/move_into_fn.rs b/tests/ui/lint/large_assignments/move_into_fn.rs index 73ec08fa23a74..b3b2160ca36e1 100644 --- a/tests/ui/lint/large_assignments/move_into_fn.rs +++ b/tests/ui/lint/large_assignments/move_into_fn.rs @@ -16,7 +16,7 @@ fn main() { // Looking at llvm-ir output, we can see that there is no memcpy involved in // this function call. Instead, just a pointer is passed to the function. So // the lint shall not trigger here. - take_data(data); + take_data(data); //~ ERROR large_assignments } fn take_data(data: Data) {} diff --git a/tests/ui/lint/large_assignments/move_into_fn.stderr b/tests/ui/lint/large_assignments/move_into_fn.stderr index 92a0489e472f9..19ec6a51d2e74 100644 --- a/tests/ui/lint/large_assignments/move_into_fn.stderr +++ b/tests/ui/lint/large_assignments/move_into_fn.stderr @@ -11,5 +11,13 @@ note: the lint level is defined here LL | #![deny(large_assignments)] | ^^^^^^^^^^^^^^^^^ -error: aborting due to 1 previous error +error: moving 9999 bytes + --> $DIR/move_into_fn.rs:19:15 + | +LL | take_data(data); + | ^^^^ value moved from here + | + = note: The current maximum size is 1000, but it can be customized with the move_size_limit attribute: `#![move_size_limit = "..."]` + +error: aborting due to 2 previous errors