Skip to content

Commit 66a2d62

Browse files
borsbors
committed
Auto merge of rust-lang#108178 - cjgillot:ssa-deref, r=oli-obk
Do not consider `&mut *x` as mutating `x` in `CopyProp` This PR removes an unfortunate overly cautious case from the current implementation. Found by rust-lang#105274 cc `@saethlin`
2 parents 6a17902 + 209eb8a commit 66a2d62

File tree

6 files changed

+294
-16
lines changed

6 files changed

+294
-16
lines changed

‎compiler/rustc_mir_transform/src/ssa.rs

Lines changed: 40 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ impl SsaLocals {
5353
body: &Body<'tcx>,
5454
borrowed_locals: &BitSet<Local>,
5555
) -> SsaLocals {
56-
let assignment_order = Vec::new();
56+
let assignment_order = Vec::with_capacity(body.local_decls.len());
5757

5858
let assignments = IndexVec::from_elem(Set1::Empty, &body.local_decls);
5959
let dominators =
@@ -179,37 +179,61 @@ struct SsaVisitor {
179179
assignment_order: Vec<Local>,
180180
}
181181

182+
impl SsaVisitor {
183+
fn check_assignment_dominates(&mut self, local: Local, loc: Location) {
184+
let set = &mut self.assignments[local];
185+
let assign_dominates = match *set {
186+
Set1::Empty | Set1::Many => false,
187+
Set1::One(LocationExtended::Arg) => true,
188+
Set1::One(LocationExtended::Plain(assign)) => {
189+
assign.successor_within_block().dominates(loc, &self.dominators)
190+
}
191+
};
192+
// We are visiting a use that is not dominated by an assignment.
193+
// Either there is a cycle involved, or we are reading for uninitialized local.
194+
// Bail out.
195+
if !assign_dominates {
196+
*set = Set1::Many;
197+
}
198+
}
199+
}
200+
182201
impl<'tcx> Visitor<'tcx> for SsaVisitor {
183202
fn visit_local(&mut self, local: Local, ctxt: PlaceContext, loc: Location) {
184203
match ctxt {
185204
PlaceContext::MutatingUse(MutatingUseContext::Store) => {
186205
self.assignments[local].insert(LocationExtended::Plain(loc));
187-
self.assignment_order.push(local);
206+
if let Set1::One(_) = self.assignments[local] {
207+
// Only record if SSA-like, to avoid growing the vector needlessly.
208+
self.assignment_order.push(local);
209+
}
188210
}
189211
// Anything can happen with raw pointers, so remove them.
190212
PlaceContext::NonMutatingUse(NonMutatingUseContext::AddressOf)
191213
| PlaceContext::MutatingUse(_) => self.assignments[local] = Set1::Many,
192214
// Immutable borrows are taken into account in `SsaLocals::new` by
193215
// removing non-freeze locals.
194216
PlaceContext::NonMutatingUse(_) => {
195-
let set = &mut self.assignments[local];
196-
let assign_dominates = match *set {
197-
Set1::Empty | Set1::Many => false,
198-
Set1::One(LocationExtended::Arg) => true,
199-
Set1::One(LocationExtended::Plain(assign)) => {
200-
assign.successor_within_block().dominates(loc, &self.dominators)
201-
}
202-
};
203-
// We are visiting a use that is not dominated by an assignment.
204-
// Either there is a cycle involved, or we are reading for uninitialized local.
205-
// Bail out.
206-
if !assign_dominates {
207-
*set = Set1::Many;
208-
}
217+
self.check_assignment_dominates(local, loc);
209218
}
210219
PlaceContext::NonUse(_) => {}
211220
}
212221
}
222+
223+
fn visit_place(&mut self, place: &Place<'tcx>, ctxt: PlaceContext, loc: Location) {
224+
if place.projection.first() == Some(&PlaceElem::Deref) {
225+
// Do not do anything for storage statements and debuginfo.
226+
if ctxt.is_use() {
227+
// A use through a `deref` only reads from the local, and cannot write to it.
228+
let new_ctxt = PlaceContext::NonMutatingUse(NonMutatingUseContext::Projection);
229+
230+
self.visit_projection(place.as_ref(), new_ctxt, loc);
231+
self.check_assignment_dominates(place.local, loc);
232+
}
233+
return;
234+
}
235+
self.super_place(place, ctxt, loc);
236+
}
213237
}
214238

215239
#[instrument(level = "trace", skip(ssa, body))]

‎tests/mir-opt/copy-prop/reborrow.demiraw.CopyProp.diff

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
- // MIR for `demiraw` before CopyProp
2+
+ // MIR for `demiraw` after CopyProp
3+
4+
fn demiraw(_1: u8) -> () {
5+
debug x => _1; // in scope 0 at $DIR/reborrow.rs:+0:12: +0:17
6+
let mut _0: (); // return place in scope 0 at $DIR/reborrow.rs:+0:23: +0:23
7+
let _2: *mut u8; // in scope 0 at $DIR/reborrow.rs:+1:9: +1:10
8+
let mut _4: &mut u8; // in scope 0 at $DIR/reborrow.rs:+2:22: +2:29
9+
let _6: (); // in scope 0 at $DIR/reborrow.rs:+4:5: +4:14
10+
let mut _7: *mut u8; // in scope 0 at $DIR/reborrow.rs:+4:12: +4:13
11+
scope 1 {
12+
debug a => _2; // in scope 1 at $DIR/reborrow.rs:+1:9: +1:10
13+
let _3: &mut u8; // in scope 1 at $DIR/reborrow.rs:+2:9: +2:10
14+
scope 2 {
15+
debug b => _3; // in scope 2 at $DIR/reborrow.rs:+2:9: +2:10
16+
let _5: *mut u8; // in scope 2 at $DIR/reborrow.rs:+3:9: +3:10
17+
scope 4 {
18+
- debug c => _5; // in scope 4 at $DIR/reborrow.rs:+3:9: +3:10
19+
+ debug c => _2; // in scope 4 at $DIR/reborrow.rs:+3:9: +3:10
20+
}
21+
}
22+
scope 3 {
23+
}
24+
}
25+
26+
bb0: {
27+
- StorageLive(_2); // scope 0 at $DIR/reborrow.rs:+1:9: +1:10
28+
_2 = &raw mut _1; // scope 0 at $DIR/reborrow.rs:+1:13: +1:23
29+
StorageLive(_3); // scope 1 at $DIR/reborrow.rs:+2:9: +2:10
30+
StorageLive(_4); // scope 1 at $DIR/reborrow.rs:+2:22: +2:29
31+
_4 = &mut (*_2); // scope 3 at $DIR/reborrow.rs:+2:22: +2:29
32+
_3 = &mut (*_4); // scope 1 at $DIR/reborrow.rs:+2:22: +2:29
33+
StorageDead(_4); // scope 1 at $DIR/reborrow.rs:+2:31: +2:32
34+
- StorageLive(_5); // scope 2 at $DIR/reborrow.rs:+3:9: +3:10
35+
- _5 = _2; // scope 2 at $DIR/reborrow.rs:+3:13: +3:14
36+
StorageLive(_6); // scope 4 at $DIR/reborrow.rs:+4:5: +4:14
37+
- StorageLive(_7); // scope 4 at $DIR/reborrow.rs:+4:12: +4:13
38+
- _7 = _5; // scope 4 at $DIR/reborrow.rs:+4:12: +4:13
39+
- _6 = opaque::<*mut u8>(move _7) -> bb1; // scope 4 at $DIR/reborrow.rs:+4:5: +4:14
40+
+ _6 = opaque::<*mut u8>(_2) -> bb1; // scope 4 at $DIR/reborrow.rs:+4:5: +4:14
41+
// mir::Constant
42+
// + span: $DIR/reborrow.rs:38:5: 38:11
43+
// + literal: Const { ty: fn(*mut u8) {opaque::<*mut u8>}, val: Value(<ZST>) }
44+
}
45+
46+
bb1: {
47+
- StorageDead(_7); // scope 4 at $DIR/reborrow.rs:+4:13: +4:14
48+
StorageDead(_6); // scope 4 at $DIR/reborrow.rs:+4:14: +4:15
49+
_0 = const (); // scope 0 at $DIR/reborrow.rs:+0:23: +5:2
50+
- StorageDead(_5); // scope 2 at $DIR/reborrow.rs:+5:1: +5:2
51+
StorageDead(_3); // scope 1 at $DIR/reborrow.rs:+5:1: +5:2
52+
- StorageDead(_2); // scope 0 at $DIR/reborrow.rs:+5:1: +5:2
53+
return; // scope 0 at $DIR/reborrow.rs:+5:2: +5:2
54+
}
55+
}
56+

‎tests/mir-opt/copy-prop/reborrow.miraw.CopyProp.diff

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
- // MIR for `miraw` before CopyProp
2+
+ // MIR for `miraw` after CopyProp
3+
4+
fn miraw(_1: u8) -> () {
5+
debug x => _1; // in scope 0 at $DIR/reborrow.rs:+0:10: +0:15
6+
let mut _0: (); // return place in scope 0 at $DIR/reborrow.rs:+0:21: +0:21
7+
let _2: *mut u8; // in scope 0 at $DIR/reborrow.rs:+1:9: +1:10
8+
let _5: (); // in scope 0 at $DIR/reborrow.rs:+4:5: +4:14
9+
let mut _6: *mut u8; // in scope 0 at $DIR/reborrow.rs:+4:12: +4:13
10+
scope 1 {
11+
debug a => _2; // in scope 1 at $DIR/reborrow.rs:+1:9: +1:10
12+
let _3: *mut u8; // in scope 1 at $DIR/reborrow.rs:+2:9: +2:10
13+
scope 2 {
14+
debug b => _3; // in scope 2 at $DIR/reborrow.rs:+2:9: +2:10
15+
let _4: *mut u8; // in scope 2 at $DIR/reborrow.rs:+3:9: +3:10
16+
scope 4 {
17+
- debug c => _4; // in scope 4 at $DIR/reborrow.rs:+3:9: +3:10
18+
+ debug c => _2; // in scope 4 at $DIR/reborrow.rs:+3:9: +3:10
19+
}
20+
}
21+
scope 3 {
22+
}
23+
}
24+
25+
bb0: {
26+
- StorageLive(_2); // scope 0 at $DIR/reborrow.rs:+1:9: +1:10
27+
_2 = &raw mut _1; // scope 0 at $DIR/reborrow.rs:+1:13: +1:23
28+
StorageLive(_3); // scope 1 at $DIR/reborrow.rs:+2:9: +2:10
29+
_3 = &raw mut (*_2); // scope 3 at $DIR/reborrow.rs:+2:22: +2:33
30+
- StorageLive(_4); // scope 2 at $DIR/reborrow.rs:+3:9: +3:10
31+
- _4 = _2; // scope 2 at $DIR/reborrow.rs:+3:13: +3:14
32+
StorageLive(_5); // scope 4 at $DIR/reborrow.rs:+4:5: +4:14
33+
- StorageLive(_6); // scope 4 at $DIR/reborrow.rs:+4:12: +4:13
34+
- _6 = _4; // scope 4 at $DIR/reborrow.rs:+4:12: +4:13
35+
- _5 = opaque::<*mut u8>(move _6) -> bb1; // scope 4 at $DIR/reborrow.rs:+4:5: +4:14
36+
+ _5 = opaque::<*mut u8>(_2) -> bb1; // scope 4 at $DIR/reborrow.rs:+4:5: +4:14
37+
// mir::Constant
38+
// + span: $DIR/reborrow.rs:30:5: 30:11
39+
// + literal: Const { ty: fn(*mut u8) {opaque::<*mut u8>}, val: Value(<ZST>) }
40+
}
41+
42+
bb1: {
43+
- StorageDead(_6); // scope 4 at $DIR/reborrow.rs:+4:13: +4:14
44+
StorageDead(_5); // scope 4 at $DIR/reborrow.rs:+4:14: +4:15
45+
_0 = const (); // scope 0 at $DIR/reborrow.rs:+0:21: +5:2
46+
- StorageDead(_4); // scope 2 at $DIR/reborrow.rs:+5:1: +5:2
47+
StorageDead(_3); // scope 1 at $DIR/reborrow.rs:+5:1: +5:2
48+
- StorageDead(_2); // scope 0 at $DIR/reborrow.rs:+5:1: +5:2
49+
return; // scope 0 at $DIR/reborrow.rs:+5:2: +5:2
50+
}
51+
}
52+

‎tests/mir-opt/copy-prop/reborrow.remut.CopyProp.diff

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
- // MIR for `remut` before CopyProp
2+
+ // MIR for `remut` after CopyProp
3+
4+
fn remut(_1: u8) -> () {
5+
debug x => _1; // in scope 0 at $DIR/reborrow.rs:+0:10: +0:15
6+
let mut _0: (); // return place in scope 0 at $DIR/reborrow.rs:+0:21: +0:21
7+
let _2: &mut u8; // in scope 0 at $DIR/reborrow.rs:+1:9: +1:10
8+
let _5: (); // in scope 0 at $DIR/reborrow.rs:+4:5: +4:14
9+
let mut _6: &mut u8; // in scope 0 at $DIR/reborrow.rs:+4:12: +4:13
10+
scope 1 {
11+
debug a => _2; // in scope 1 at $DIR/reborrow.rs:+1:9: +1:10
12+
let _3: &mut u8; // in scope 1 at $DIR/reborrow.rs:+2:9: +2:10
13+
scope 2 {
14+
debug b => _3; // in scope 2 at $DIR/reborrow.rs:+2:9: +2:10
15+
let _4: &mut u8; // in scope 2 at $DIR/reborrow.rs:+3:9: +3:10
16+
scope 3 {
17+
- debug c => _4; // in scope 3 at $DIR/reborrow.rs:+3:9: +3:10
18+
+ debug c => _2; // in scope 3 at $DIR/reborrow.rs:+3:9: +3:10
19+
}
20+
}
21+
}
22+
23+
bb0: {
24+
- StorageLive(_2); // scope 0 at $DIR/reborrow.rs:+1:9: +1:10
25+
_2 = &mut _1; // scope 0 at $DIR/reborrow.rs:+1:13: +1:19
26+
StorageLive(_3); // scope 1 at $DIR/reborrow.rs:+2:9: +2:10
27+
_3 = &mut (*_2); // scope 1 at $DIR/reborrow.rs:+2:13: +2:20
28+
- StorageLive(_4); // scope 2 at $DIR/reborrow.rs:+3:9: +3:10
29+
- _4 = move _2; // scope 2 at $DIR/reborrow.rs:+3:13: +3:14
30+
StorageLive(_5); // scope 3 at $DIR/reborrow.rs:+4:5: +4:14
31+
- StorageLive(_6); // scope 3 at $DIR/reborrow.rs:+4:12: +4:13
32+
- _6 = move _4; // scope 3 at $DIR/reborrow.rs:+4:12: +4:13
33+
- _5 = opaque::<&mut u8>(move _6) -> bb1; // scope 3 at $DIR/reborrow.rs:+4:5: +4:14
34+
+ _5 = opaque::<&mut u8>(move _2) -> bb1; // scope 3 at $DIR/reborrow.rs:+4:5: +4:14
35+
// mir::Constant
36+
// + span: $DIR/reborrow.rs:14:5: 14:11
37+
// + literal: Const { ty: fn(&mut u8) {opaque::<&mut u8>}, val: Value(<ZST>) }
38+
}
39+
40+
bb1: {
41+
- StorageDead(_6); // scope 3 at $DIR/reborrow.rs:+4:13: +4:14
42+
StorageDead(_5); // scope 3 at $DIR/reborrow.rs:+4:14: +4:15
43+
_0 = const (); // scope 0 at $DIR/reborrow.rs:+0:21: +5:2
44+
- StorageDead(_4); // scope 2 at $DIR/reborrow.rs:+5:1: +5:2
45+
StorageDead(_3); // scope 1 at $DIR/reborrow.rs:+5:1: +5:2
46+
- StorageDead(_2); // scope 0 at $DIR/reborrow.rs:+5:1: +5:2
47+
return; // scope 0 at $DIR/reborrow.rs:+5:2: +5:2
48+
}
49+
}
50+

‎tests/mir-opt/copy-prop/reborrow.reraw.CopyProp.diff

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
- // MIR for `reraw` before CopyProp
2+
+ // MIR for `reraw` after CopyProp
3+
4+
fn reraw(_1: u8) -> () {
5+
debug x => _1; // in scope 0 at $DIR/reborrow.rs:+0:10: +0:15
6+
let mut _0: (); // return place in scope 0 at $DIR/reborrow.rs:+0:21: +0:21
7+
let _2: &mut u8; // in scope 0 at $DIR/reborrow.rs:+1:9: +1:10
8+
let _5: (); // in scope 0 at $DIR/reborrow.rs:+4:5: +4:14
9+
let mut _6: &mut u8; // in scope 0 at $DIR/reborrow.rs:+4:12: +4:13
10+
scope 1 {
11+
debug a => _2; // in scope 1 at $DIR/reborrow.rs:+1:9: +1:10
12+
let _3: *mut u8; // in scope 1 at $DIR/reborrow.rs:+2:9: +2:10
13+
scope 2 {
14+
debug b => _3; // in scope 2 at $DIR/reborrow.rs:+2:9: +2:10
15+
let _4: &mut u8; // in scope 2 at $DIR/reborrow.rs:+3:9: +3:10
16+
scope 3 {
17+
- debug c => _4; // in scope 3 at $DIR/reborrow.rs:+3:9: +3:10
18+
+ debug c => _2; // in scope 3 at $DIR/reborrow.rs:+3:9: +3:10
19+
}
20+
}
21+
}
22+
23+
bb0: {
24+
- StorageLive(_2); // scope 0 at $DIR/reborrow.rs:+1:9: +1:10
25+
_2 = &mut _1; // scope 0 at $DIR/reborrow.rs:+1:13: +1:19
26+
StorageLive(_3); // scope 1 at $DIR/reborrow.rs:+2:9: +2:10
27+
_3 = &raw mut (*_2); // scope 1 at $DIR/reborrow.rs:+2:13: +2:24
28+
- StorageLive(_4); // scope 2 at $DIR/reborrow.rs:+3:9: +3:10
29+
- _4 = move _2; // scope 2 at $DIR/reborrow.rs:+3:13: +3:14
30+
StorageLive(_5); // scope 3 at $DIR/reborrow.rs:+4:5: +4:14
31+
- StorageLive(_6); // scope 3 at $DIR/reborrow.rs:+4:12: +4:13
32+
- _6 = move _4; // scope 3 at $DIR/reborrow.rs:+4:12: +4:13
33+
- _5 = opaque::<&mut u8>(move _6) -> bb1; // scope 3 at $DIR/reborrow.rs:+4:5: +4:14
34+
+ _5 = opaque::<&mut u8>(move _2) -> bb1; // scope 3 at $DIR/reborrow.rs:+4:5: +4:14
35+
// mir::Constant
36+
// + span: $DIR/reborrow.rs:22:5: 22:11
37+
// + literal: Const { ty: fn(&mut u8) {opaque::<&mut u8>}, val: Value(<ZST>) }
38+
}
39+
40+
bb1: {
41+
- StorageDead(_6); // scope 3 at $DIR/reborrow.rs:+4:13: +4:14
42+
StorageDead(_5); // scope 3 at $DIR/reborrow.rs:+4:14: +4:15
43+
_0 = const (); // scope 0 at $DIR/reborrow.rs:+0:21: +5:2
44+
- StorageDead(_4); // scope 2 at $DIR/reborrow.rs:+5:1: +5:2
45+
StorageDead(_3); // scope 1 at $DIR/reborrow.rs:+5:1: +5:2
46+
- StorageDead(_2); // scope 0 at $DIR/reborrow.rs:+5:1: +5:2
47+
return; // scope 0 at $DIR/reborrow.rs:+5:2: +5:2
48+
}
49+
}
50+

‎tests/mir-opt/copy-prop/reborrow.rs

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
// Check that CopyProp considers reborrows as not mutating the pointer.
2+
// unit-test: CopyProp
3+
4+
#![feature(raw_ref_op)]
5+
6+
#[inline(never)]
7+
fn opaque(_: impl Sized) {}
8+
9+
// EMIT_MIR reborrow.remut.CopyProp.diff
10+
fn remut(mut x: u8) {
11+
let a = &mut x;
12+
let b = &mut *a; //< this cannot mutate a.
13+
let c = a; //< so `c` and `a` can be merged.
14+
opaque(c);
15+
}
16+
17+
// EMIT_MIR reborrow.reraw.CopyProp.diff
18+
fn reraw(mut x: u8) {
19+
let a = &mut x;
20+
let b = &raw mut *a; //< this cannot mutate a.
21+
let c = a; //< so `c` and `a` can be merged.
22+
opaque(c);
23+
}
24+
25+
// EMIT_MIR reborrow.miraw.CopyProp.diff
26+
fn miraw(mut x: u8) {
27+
let a = &raw mut x;
28+
let b = unsafe { &raw mut *a }; //< this cannot mutate a.
29+
let c = a; //< so `c` and `a` can be merged.
30+
opaque(c);
31+
}
32+
33+
// EMIT_MIR reborrow.demiraw.CopyProp.diff
34+
fn demiraw(mut x: u8) {
35+
let a = &raw mut x;
36+
let b = unsafe { &mut *a }; //< this cannot mutate a.
37+
let c = a; //< so `c` and `a` can be merged.
38+
opaque(c);
39+
}
40+
41+
fn main() {
42+
remut(0);
43+
reraw(0);
44+
miraw(0);
45+
demiraw(0);
46+
}

0 commit comments

Comments
 (0)