Skip to content

Commit bc4a643

Browse files
jhprattjhpratt
authored
Rollup merge of #142185 - saethlin:refprop-moves, r=cjgillot
Convert moves of references to copies in ReferencePropagation This is a fix for #141101. The root cause of this miscompile is that the SsaLocals analysis that MIR transforms use is supposed to detect locals that are only written to once, in their single assignment. But that analysis is subtly wrong; it does not consider `Operand::Move` to be a write even though the meaning ascribed to `Operand::Move` (at least as a function parameter) by Miri is that the callee may have done arbitrary writes to the caller's Local that the Operand wraps (because `Move` is pass-by-pointer). So Miri conwiders `Operand::Move` to be a write but both the MIR visitor system considers it a read, and so does SsaLocals. I have tried fixing this by changing the `PlaceContext` that is ascribed to an `Operand::Move` to a `MutatingUseContext` but that seems to have borrow checker implications, and changing SsaLocals seems to have wide-ranging regressions in MIR optimizations. So instead of doing those, this PR adds a new kludge to ReferencePropagation, which follows the same line of thinking as the kludge in CopyProp that solves this same problem inside that pass: https://github.com/rust-lang/rust/blob/a5584a8fe16037dc01782064fa41424a6dbe9987/compiler/rustc_mir_transform/src/copy_prop.rs#L65-L98
2 parents 467c89c + 9aa8cfa commit bc4a643

File tree

28 files changed

+194
-88
lines changed

28 files changed

+194
-88
lines changed

‎compiler/rustc_mir_transform/src/ref_prop.rs

Lines changed: 36 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,7 @@ impl<'tcx> crate::MirPass<'tcx> for ReferencePropagation {
7979
#[instrument(level = "trace", skip(self, tcx, body))]
8080
fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
8181
debug!(def_id = ?body.source.def_id());
82+
move_to_copy_pointers(tcx, body);
8283
while propagate_ssa(tcx, body) {}
8384
}
8485

@@ -87,11 +88,43 @@ impl<'tcx> crate::MirPass<'tcx> for ReferencePropagation {
8788
}
8889
}
8990

91+
/// The SSA analysis done by [`SsaLocals`] treats [`Operand::Move`] as a read, even though in
92+
/// general [`Operand::Move`] represents pass-by-pointer where the callee can overwrite the
93+
/// pointee (Miri always considers the place deinitialized). CopyProp has a similar trick to
94+
/// turn [`Operand::Move`] into [`Operand::Copy`] when required for an optimization, but in this
95+
/// pass we just turn all moves of pointers into copies because pointers should be by-value anyway.
96+
fn move_to_copy_pointers<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
97+
let mut visitor = MoveToCopyVisitor { tcx, local_decls: &body.local_decls };
98+
for (bb, data) in body.basic_blocks.as_mut_preserves_cfg().iter_enumerated_mut() {
99+
visitor.visit_basic_block_data(bb, data);
100+
}
101+
102+
struct MoveToCopyVisitor<'a, 'tcx> {
103+
tcx: TyCtxt<'tcx>,
104+
local_decls: &'a IndexVec<Local, LocalDecl<'tcx>>,
105+
}
106+
107+
impl<'a, 'tcx> MutVisitor<'tcx> for MoveToCopyVisitor<'a, 'tcx> {
108+
fn tcx(&self) -> TyCtxt<'tcx> {
109+
self.tcx
110+
}
111+
112+
fn visit_operand(&mut self, operand: &mut Operand<'tcx>, loc: Location) {
113+
if let Operand::Move(place) = *operand {
114+
if place.ty(self.local_decls, self.tcx).ty.is_any_ptr() {
115+
*operand = Operand::Copy(place);
116+
}
117+
}
118+
self.super_operand(operand, loc);
119+
}
120+
}
121+
}
122+
90123
fn propagate_ssa<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) -> bool {
91124
let typing_env = body.typing_env(tcx);
92125
let ssa = SsaLocals::new(tcx, body, typing_env);
93126

94-
let mut replacer = compute_replacement(tcx, body, &ssa);
127+
let mut replacer = compute_replacement(tcx, body, ssa);
95128
debug!(?replacer.targets);
96129
debug!(?replacer.allowed_replacements);
97130
debug!(?replacer.storage_to_remove);
@@ -119,7 +152,7 @@ enum Value<'tcx> {
119152
fn compute_replacement<'tcx>(
120153
tcx: TyCtxt<'tcx>,
121154
body: &Body<'tcx>,
122-
ssa: &SsaLocals,
155+
ssa: SsaLocals,
123156
) -> Replacer<'tcx> {
124157
let always_live_locals = always_storage_live_locals(body);
125158

@@ -138,7 +171,7 @@ fn compute_replacement<'tcx>(
138171
// reborrowed references.
139172
let mut storage_to_remove = DenseBitSet::new_empty(body.local_decls.len());
140173

141-
let fully_replaceable_locals = fully_replaceable_locals(ssa);
174+
let fully_replaceable_locals = fully_replaceable_locals(&ssa);
142175

143176
// Returns true iff we can use `place` as a pointee.
144177
//

‎tests/mir-opt/building/index_array_and_slice.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ struct WithSliceTail(f64, [i32]);
5555
fn index_custom(custom: &WithSliceTail, index: usize) -> &i32 {
5656
// CHECK: bb0:
5757
// CHECK: [[PTR:_.+]] = &raw const (fake) ((*_1).1: [i32]);
58-
// CHECK: [[LEN:_.+]] = PtrMetadata(move [[PTR]]);
58+
// CHECK: [[LEN:_.+]] = PtrMetadata(copy [[PTR]]);
5959
// CHECK: [[LT:_.+]] = Lt(copy _2, copy [[LEN]]);
6060
// CHECK: assert(move [[LT]], "index out of bounds{{.+}}", move [[LEN]], copy _2) -> [success: bb1,
6161

‎tests/mir-opt/dont_reset_cast_kind_without_updating_operand.test.GVN.32bit.panic-abort.diff

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@
112112
StorageDead(_17);
113113
StorageDead(_16);
114114
StorageDead(_14);
115-
_3 = ShallowInitBox(move _13, ());
115+
_3 = ShallowInitBox(copy _13, ());
116116
StorageDead(_13);
117117
StorageDead(_12);
118118
StorageDead(_11);

‎tests/mir-opt/dont_reset_cast_kind_without_updating_operand.test.GVN.64bit.panic-abort.diff

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@
112112
StorageDead(_17);
113113
StorageDead(_16);
114114
StorageDead(_14);
115-
_3 = ShallowInitBox(move _13, ());
115+
_3 = ShallowInitBox(copy _13, ());
116116
StorageDead(_13);
117117
StorageDead(_12);
118118
StorageDead(_11);

‎tests/mir-opt/pre-codegen/issue_117368_print_invalid_constant.main.GVN.32bit.panic-abort.diff

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@
103103
StorageDead(_6);
104104
_4 = copy _5 as *mut [u8] (Transmute);
105105
StorageDead(_5);
106-
_3 = move _4 as *mut u8 (PtrToPtr);
106+
_3 = copy _4 as *mut u8 (PtrToPtr);
107107
StorageDead(_4);
108108
StorageDead(_3);
109109
- StorageDead(_1);

‎tests/mir-opt/pre-codegen/issue_117368_print_invalid_constant.main.GVN.32bit.panic-unwind.diff

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@
4646
StorageDead(_6);
4747
_4 = copy _5 as *mut [u8] (Transmute);
4848
StorageDead(_5);
49-
_3 = move _4 as *mut u8 (PtrToPtr);
49+
_3 = copy _4 as *mut u8 (PtrToPtr);
5050
StorageDead(_4);
5151
StorageDead(_3);
5252
- StorageDead(_1);

‎tests/mir-opt/pre-codegen/issue_117368_print_invalid_constant.main.GVN.64bit.panic-abort.diff

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@
103103
StorageDead(_6);
104104
_4 = copy _5 as *mut [u8] (Transmute);
105105
StorageDead(_5);
106-
_3 = move _4 as *mut u8 (PtrToPtr);
106+
_3 = copy _4 as *mut u8 (PtrToPtr);
107107
StorageDead(_4);
108108
StorageDead(_3);
109109
- StorageDead(_1);

‎tests/mir-opt/pre-codegen/issue_117368_print_invalid_constant.main.GVN.64bit.panic-unwind.diff

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@
4646
StorageDead(_6);
4747
_4 = copy _5 as *mut [u8] (Transmute);
4848
StorageDead(_5);
49-
_3 = move _4 as *mut u8 (PtrToPtr);
49+
_3 = copy _4 as *mut u8 (PtrToPtr);
5050
StorageDead(_4);
5151
StorageDead(_3);
5252
- StorageDead(_1);

‎tests/mir-opt/pre-codegen/slice_index.slice_get_mut_usize.PreCodegen.after.panic-abort.mir

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ fn slice_get_mut_usize(_1: &mut [u32], _2: usize) -> Option<&mut u32> {
3030
StorageDead(_3);
3131
StorageLive(_5);
3232
_5 = &mut (*_1)[_2];
33-
_0 = Option::<&mut u32>::Some(move _5);
33+
_0 = Option::<&mut u32>::Some(copy _5);
3434
StorageDead(_5);
3535
goto -> bb3;
3636
}

‎tests/mir-opt/pre-codegen/slice_index.slice_get_mut_usize.PreCodegen.after.panic-unwind.mir

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ fn slice_get_mut_usize(_1: &mut [u32], _2: usize) -> Option<&mut u32> {
3030
StorageDead(_3);
3131
StorageLive(_5);
3232
_5 = &mut (*_1)[_2];
33-
_0 = Option::<&mut u32>::Some(move _5);
33+
_0 = Option::<&mut u32>::Some(copy _5);
3434
StorageDead(_5);
3535
goto -> bb3;
3636
}

0 commit comments

Comments
 (0)