Skip to content

Commit a84ab0c

Browse files
borsbors
committed
Auto merge of #143509 - cjgillot:copy-prop-noborrow, r=tmiasko
Do not unify borrowed locals in CopyProp. Instead of trying yet another scheme to unify borrowed locals in CopyProp, let's just stop trying. We had already enough miscompilations because of this. I'm convinced it's possible to have both unification of some borrowed locals and soundness, but I don't have a simple and convincing formulation yet. Fixes #143491
2 parents de031bb + b1fdb4b commit a84ab0c

15 files changed

Lines changed: 832 additions & 283 deletions

‎compiler/rustc_mir_transform/src/ssa.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -322,8 +322,8 @@ fn compute_copy_classes(ssa: &mut SsaLocals, body: &Body<'_>) {
322322
// visited before `local`, and we just have to copy the representing local.
323323
let head = copies[rhs];
324324

325-
// Do not unify two borrowed locals.
326-
if borrowed_classes.contains(local) && borrowed_classes.contains(head) {
325+
// Do not unify borrowed locals.
326+
if borrowed_classes.contains(local) || borrowed_classes.contains(head) {
327327
continue;
328328
}
329329

‎tests/mir-opt/copy-prop/borrowed_local.borrow_in_loop.CopyProp.panic-abort.diff‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
- // MIR for `borrow_in_loop` before CopyProp
2+
+ // MIR for `borrow_in_loop` after CopyProp
3+
4+
fn borrow_in_loop() -> () {
5+
let mut _0: ();
6+
let mut _1: bool;
7+
let _3: bool;
8+
let mut _4: !;
9+
let mut _5: ();
10+
let mut _7: bool;
11+
let mut _9: bool;
12+
let mut _10: bool;
13+
let mut _11: &bool;
14+
let _12: &bool;
15+
let mut _13: bool;
16+
let mut _14: bool;
17+
let mut _15: bool;
18+
let mut _16: !;
19+
scope 1 {
20+
debug c => _1;
21+
let mut _2: &bool;
22+
let mut _17: &bool;
23+
scope 2 {
24+
debug p => _2;
25+
let _6: bool;
26+
scope 3 {
27+
debug a => _6;
28+
let _8: bool;
29+
scope 4 {
30+
debug b => _8;
31+
}
32+
}
33+
}
34+
}
35+
36+
bb0: {
37+
StorageLive(_1);
38+
StorageLive(_2);
39+
_17 = const borrow_in_loop::promoted[0];
40+
_2 = &(*_17);
41+
- StorageLive(_4);
42+
goto -> bb1;
43+
}
44+
45+
bb1: {
46+
- StorageLive(_6);
47+
StorageLive(_7);
48+
_7 = copy (*_2);
49+
_6 = Not(move _7);
50+
StorageDead(_7);
51+
- StorageLive(_8);
52+
StorageLive(_9);
53+
_9 = copy (*_2);
54+
_8 = Not(move _9);
55+
StorageDead(_9);
56+
- StorageLive(_10);
57+
- _10 = copy _6;
58+
- _1 = move _10;
59+
- StorageDead(_10);
60+
+ _1 = copy _6;
61+
StorageLive(_11);
62+
StorageLive(_12);
63+
_12 = &_1;
64+
_11 = &(*_12);
65+
_2 = move _11;
66+
StorageDead(_11);
67+
StorageDead(_12);
68+
StorageLive(_13);
69+
- StorageLive(_14);
70+
- _14 = copy _6;
71+
- StorageLive(_15);
72+
- _15 = copy _8;
73+
- _13 = Ne(move _14, move _15);
74+
+ _13 = Ne(copy _6, copy _8);
75+
switchInt(move _13) -> [0: bb3, otherwise: bb2];
76+
}
77+
78+
bb2: {
79+
- StorageDead(_15);
80+
- StorageDead(_14);
81+
_0 = const ();
82+
StorageDead(_13);
83+
- StorageDead(_8);
84+
- StorageDead(_6);
85+
- StorageDead(_4);
86+
StorageDead(_2);
87+
StorageDead(_1);
88+
return;
89+
}
90+
91+
bb3: {
92+
- StorageDead(_15);
93+
- StorageDead(_14);
94+
- _5 = const ();
95+
StorageDead(_13);
96+
- StorageDead(_8);
97+
- StorageDead(_6);
98+
goto -> bb1;
99+
}
100+
}
101+

‎tests/mir-opt/copy-prop/borrowed_local.borrow_in_loop.CopyProp.panic-unwind.diff‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
- // MIR for `borrow_in_loop` before CopyProp
2+
+ // MIR for `borrow_in_loop` after CopyProp
3+
4+
fn borrow_in_loop() -> () {
5+
let mut _0: ();
6+
let mut _1: bool;
7+
let _3: bool;
8+
let mut _4: !;
9+
let mut _5: ();
10+
let mut _7: bool;
11+
let mut _9: bool;
12+
let mut _10: bool;
13+
let mut _11: &bool;
14+
let _12: &bool;
15+
let mut _13: bool;
16+
let mut _14: bool;
17+
let mut _15: bool;
18+
let mut _16: !;
19+
scope 1 {
20+
debug c => _1;
21+
let mut _2: &bool;
22+
let mut _17: &bool;
23+
scope 2 {
24+
debug p => _2;
25+
let _6: bool;
26+
scope 3 {
27+
debug a => _6;
28+
let _8: bool;
29+
scope 4 {
30+
debug b => _8;
31+
}
32+
}
33+
}
34+
}
35+
36+
bb0: {
37+
StorageLive(_1);
38+
StorageLive(_2);
39+
_17 = const borrow_in_loop::promoted[0];
40+
_2 = &(*_17);
41+
- StorageLive(_4);
42+
goto -> bb1;
43+
}
44+
45+
bb1: {
46+
- StorageLive(_6);
47+
StorageLive(_7);
48+
_7 = copy (*_2);
49+
_6 = Not(move _7);
50+
StorageDead(_7);
51+
- StorageLive(_8);
52+
StorageLive(_9);
53+
_9 = copy (*_2);
54+
_8 = Not(move _9);
55+
StorageDead(_9);
56+
- StorageLive(_10);
57+
- _10 = copy _6;
58+
- _1 = move _10;
59+
- StorageDead(_10);
60+
+ _1 = copy _6;
61+
StorageLive(_11);
62+
StorageLive(_12);
63+
_12 = &_1;
64+
_11 = &(*_12);
65+
_2 = move _11;
66+
StorageDead(_11);
67+
StorageDead(_12);
68+
StorageLive(_13);
69+
- StorageLive(_14);
70+
- _14 = copy _6;
71+
- StorageLive(_15);
72+
- _15 = copy _8;
73+
- _13 = Ne(move _14, move _15);
74+
+ _13 = Ne(copy _6, copy _8);
75+
switchInt(move _13) -> [0: bb3, otherwise: bb2];
76+
}
77+
78+
bb2: {
79+
- StorageDead(_15);
80+
- StorageDead(_14);
81+
_0 = const ();
82+
StorageDead(_13);
83+
- StorageDead(_8);
84+
- StorageDead(_6);
85+
- StorageDead(_4);
86+
StorageDead(_2);
87+
StorageDead(_1);
88+
return;
89+
}
90+
91+
bb3: {
92+
- StorageDead(_15);
93+
- StorageDead(_14);
94+
- _5 = const ();
95+
StorageDead(_13);
96+
- StorageDead(_8);
97+
- StorageDead(_6);
98+
goto -> bb1;
99+
}
100+
}
101+

‎tests/mir-opt/copy-prop/borrowed_local.borrowed.CopyProp.panic-abort.diff‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,14 +7,13 @@
77
let mut _3: &T;
88

99
bb0: {
10-
- _2 = copy _1;
10+
_2 = copy _1;
1111
_3 = &_1;
1212
_0 = opaque::<&T>(copy _3) -> [return: bb1, unwind unreachable];
1313
}
1414

1515
bb1: {
16-
- _0 = opaque::<T>(copy _2) -> [return: bb2, unwind unreachable];
17-
+ _0 = opaque::<T>(copy _1) -> [return: bb2, unwind unreachable];
16+
_0 = opaque::<T>(copy _2) -> [return: bb2, unwind unreachable];
1817
}
1918

2019
bb2: {

‎tests/mir-opt/copy-prop/borrowed_local.borrowed.CopyProp.panic-unwind.diff‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,14 +7,13 @@
77
let mut _3: &T;
88

99
bb0: {
10-
- _2 = copy _1;
10+
_2 = copy _1;
1111
_3 = &_1;
1212
_0 = opaque::<&T>(copy _3) -> [return: bb1, unwind continue];
1313
}
1414

1515
bb1: {
16-
- _0 = opaque::<T>(copy _2) -> [return: bb2, unwind continue];
17-
+ _0 = opaque::<T>(copy _1) -> [return: bb2, unwind continue];
16+
_0 = opaque::<T>(copy _2) -> [return: bb2, unwind continue];
1817
}
1918

2019
bb2: {

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

Lines changed: 36 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,10 +50,11 @@ fn compare_address() -> bool {
5050
fn borrowed<T: Copy + Freeze>(x: T) -> bool {
5151
// CHECK-LABEL: fn borrowed(
5252
// CHECK: bb0: {
53+
// CHECK-NEXT: _2 = copy _1;
5354
// CHECK-NEXT: _3 = &_1;
5455
// CHECK-NEXT: _0 = opaque::<&T>(copy _3)
5556
// CHECK: bb1: {
56-
// CHECK-NEXT: _0 = opaque::<T>(copy _1)
57+
// CHECK-NEXT: _0 = opaque::<T>(copy _2)
5758
mir! {
5859
{
5960
let a = x;
@@ -94,11 +95,45 @@ fn non_freeze<T: Copy>(x: T) -> bool {
9495
}
9596
}
9697

98+
/// We must not unify a borrowed local with another that may be written-to before the borrow is
99+
/// read again. As we have no aliasing model yet, this means forbidding unifying borrowed locals.
100+
fn borrow_in_loop() {
101+
// CHECK-LABEL: fn borrow_in_loop(
102+
// CHECK: debug c => [[c:_.*]];
103+
// CHECK: debug p => [[p:_.*]];
104+
// CHECK: debug a => [[a:_.*]];
105+
// CHECK: debug b => [[b:_.*]];
106+
// CHECK-NOT: &[[a]]
107+
// CHECK-NOT: &[[b]]
108+
// CHECK: [[a]] = Not({{.*}});
109+
// CHECK-NOT: &[[a]]
110+
// CHECK-NOT: &[[b]]
111+
// CHECK: [[b]] = Not({{.*}});
112+
// CHECK-NOT: &[[a]]
113+
// CHECK-NOT: &[[b]]
114+
// CHECK: &[[c]]
115+
// CHECK-NOT: &[[a]]
116+
// CHECK-NOT: &[[b]]
117+
let mut c;
118+
let mut p = &false;
119+
loop {
120+
let a = !*p;
121+
let b = !*p;
122+
c = a;
123+
p = &c;
124+
if a != b {
125+
return;
126+
}
127+
}
128+
}
129+
97130
fn main() {
98131
assert!(!compare_address());
99132
non_freeze(5);
133+
borrow_in_loop();
100134
}
101135

102136
// EMIT_MIR borrowed_local.compare_address.CopyProp.diff
103137
// EMIT_MIR borrowed_local.borrowed.CopyProp.diff
104138
// EMIT_MIR borrowed_local.non_freeze.CopyProp.diff
139+
// EMIT_MIR borrowed_local.borrow_in_loop.CopyProp.diff

‎tests/mir-opt/copy-prop/write_to_borrowed.main.CopyProp.diff‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,10 @@
1616
_3 = const 'b';
1717
_5 = copy _3;
1818
_6 = &_3;
19-
- _4 = copy _5;
19+
_4 = copy _5;
2020
(*_1) = copy (*_6);
2121
_6 = &_5;
22-
- _7 = dump_var::<char>(copy _4) -> [return: bb1, unwind unreachable];
23-
+ _7 = dump_var::<char>(copy _5) -> [return: bb1, unwind unreachable];
22+
_7 = dump_var::<char>(copy _4) -> [return: bb1, unwind unreachable];
2423
}
2524

2625
bb1: {

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,13 +27,13 @@ fn main() {
2727
_5 = _3;
2828
// CHECK-NEXT: _6 = &_3;
2929
_6 = &_3;
30-
// CHECK-NOT: {{_.*}} = {{_.*}};
30+
// CHECK-NEXT: _4 = copy _5;
3131
_4 = _5;
3232
// CHECK-NEXT: (*_1) = copy (*_6);
3333
*_1 = *_6;
3434
// CHECK-NEXT: _6 = &_5;
3535
_6 = &_5;
36-
// CHECK-NEXT: _7 = dump_var::<char>(copy _5)
36+
// CHECK-NEXT: _7 = dump_var::<char>(copy _4)
3737
Call(_7 = dump_var(_4), ReturnTo(bb1), UnwindUnreachable())
3838
}
3939
bb1 = { Return() }

0 commit comments

Comments
 (0)