From 6bd1a031ab0431163e890f7285d396f677296bc3 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 +++---------------- tests/codegen-llvm/align-byval.rs | 2 +- .../move_arg.f.CopyProp.panic-abort.diff | 37 ----------- .../move_arg.f.CopyProp.panic-unwind.diff | 37 ----------- tests/mir-opt/copy-prop/move_arg.rs | 54 +++++++++++----- .../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 ++- 13 files changed, 75 insertions(+), 150 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 2e9c3a5bf3ee..89ee96317ac9 100644 --- a/compiler/rustc_mir_transform/src/copy_prop.rs +++ b/compiler/rustc_mir_transform/src/copy_prop.rs @@ -16,7 +16,7 @@ /// _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 @@ fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) { 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 @@ fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) { 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 @@ fn is_required(&self) -> bool { } } -/// `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 @@ fn tcx(&self) -> TyCtxt<'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 @@ fn visit_operand(&mut self, operand: &mut Operand<'tcx>, loc: Location) { // 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 @@ fn visit_operand(&mut self, operand: &mut Operand<'tcx>, loc: Location) { 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/codegen-llvm/align-byval.rs b/tests/codegen-llvm/align-byval.rs index 2d6fc0d1e78d..fd238534a335 100644 --- a/tests/codegen-llvm/align-byval.rs +++ b/tests/codegen-llvm/align-byval.rs @@ -1,7 +1,7 @@ // ignore-tidy-linelength //@ add-minicore //@ revisions:m68k x86_64-linux x86_64-windows i686-linux i686-windows - +//@ compile-flags: -Copt-level=1 -Cno-prepopulate-passes //@[m68k] compile-flags: --target m68k-unknown-linux-gnu //@[m68k] needs-llvm-components: m68k //@[x86_64-linux] compile-flags: --target x86_64-unknown-linux-gnu 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 da9904bfa01f..000000000000 --- 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 9b0de655bd26..000000000000 --- 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 b7adae333196..23e20021f588 100644 --- a/tests/mir-opt/copy-prop/move_arg.rs +++ b/tests/mir-opt/copy-prop/move_arg.rs @@ -1,20 +1,46 @@ -// 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) { - // CHECK-LABEL: fn f( - // CHECK: debug a => [[a:_.*]]; - // CHECK: debug b => [[a]]; - // CHECK: g::(copy [[a]], copy [[a]]) - 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() + } + } +} + +#[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 g(_: T, _: T) {} - -fn main() { - f(5) -} +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 2026c1982f29..8ef61b5667dd 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 67763fdce667..2a7182af984d 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 dfc8dd097563..8a2cdd8e5728 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 becc42563210..614d23cf6245 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 fc27b8ff244d..d16aae6ab145 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 d9010e24d03b..1a5fcb6c8fc1 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 73ec08fa23a7..b3b2160ca36e 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 92a0489e472f..19ec6a51d2e7 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