-
Notifications
You must be signed in to change notification settings - Fork 12.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
The const propagator cannot trace references. #73613
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -575,8 +575,16 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { | |
} | ||
|
||
// Do not try creating references (#67862) | ||
Rvalue::Ref(_, _, place_ref) => { | ||
trace!("skipping Ref({:?})", place_ref); | ||
Rvalue::AddressOf(_, place) | Rvalue::Ref(_, _, place) => { | ||
trace!("skipping AddressOf | Ref for {:?}", place); | ||
|
||
// This may be creating mutable references or immutable references to cells. | ||
// If that happens, the pointed to value could be mutated via that reference. | ||
// Since we aren't tracking references, the const propagator loses track of what | ||
// value the local has right now. | ||
// Thus, all locals that have their reference taken | ||
// must not take part in propagation. | ||
Self::remove_const(&mut self.ecx, place.local); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried simply asserting here, but that won't work for
|
||
|
||
return None; | ||
} | ||
|
@@ -716,6 +724,9 @@ enum ConstPropMode { | |
OnlyInsideOwnBlock, | ||
/// The `Local` can be propagated into but reads cannot be propagated. | ||
OnlyPropagateInto, | ||
/// The `Local` cannot be part of propagation at all. Any statement | ||
/// referencing it either for reading or writing will not get propagated. | ||
NoPropagation, | ||
} | ||
|
||
struct CanConstProp { | ||
|
@@ -781,7 +792,9 @@ impl<'tcx> Visitor<'tcx> for CanConstProp { | |
// end of the block anyway, and inside the block we overwrite previous | ||
// states as applicable. | ||
ConstPropMode::OnlyInsideOwnBlock => {} | ||
other => { | ||
ConstPropMode::NoPropagation => {} | ||
ConstPropMode::OnlyPropagateInto => {} | ||
other @ ConstPropMode::FullConstProp => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Part of exhaustively matching on things, but also part of the bugfix, because we don't go from |
||
trace!( | ||
"local {:?} can't be propagated because of multiple assignments", | ||
local, | ||
|
@@ -813,7 +826,7 @@ impl<'tcx> Visitor<'tcx> for CanConstProp { | |
| MutatingUse(MutatingUseContext::Borrow) | ||
| MutatingUse(MutatingUseContext::AddressOf) => { | ||
trace!("local {:?} can't be propagaged because it's used: {:?}", local, context); | ||
self.can_const_prop[local] = ConstPropMode::OnlyPropagateInto; | ||
self.can_const_prop[local] = ConstPropMode::NoPropagation; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Main part of the bugfix, while we can do things for borrows, this is super hard to reason about and I don't think we should do it without dataflow |
||
} | ||
} | ||
} | ||
|
@@ -858,19 +871,22 @@ impl<'mir, 'tcx> MutVisitor<'tcx> for ConstPropagator<'mir, 'tcx> { | |
} | ||
} | ||
} | ||
if can_const_prop == ConstPropMode::OnlyInsideOwnBlock { | ||
trace!( | ||
"found local restricted to its block. Will remove it from const-prop after block is finished. Local: {:?}", | ||
place.local | ||
); | ||
self.locals_of_current_block.insert(place.local); | ||
} | ||
|
||
if can_const_prop == ConstPropMode::OnlyPropagateInto { | ||
trace!("can't propagate into {:?}", place); | ||
if place.local != RETURN_PLACE { | ||
Self::remove_const(&mut self.ecx, place.local); | ||
match can_const_prop { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. more exhaustively matching. |
||
ConstPropMode::OnlyInsideOwnBlock => { | ||
trace!( | ||
"found local restricted to its block. \ | ||
Will remove it from const-prop after block is finished. Local: {:?}", | ||
place.local | ||
); | ||
self.locals_of_current_block.insert(place.local); | ||
} | ||
ConstPropMode::OnlyPropagateInto | ConstPropMode::NoPropagation => { | ||
trace!("can't propagate into {:?}", place); | ||
if place.local != RETURN_PLACE { | ||
Self::remove_const(&mut self.ecx, place.local); | ||
} | ||
} | ||
ConstPropMode::FullConstProp => {} | ||
} | ||
} else { | ||
// Const prop failed, so erase the destination, ensuring that whatever happens | ||
|
@@ -890,6 +906,12 @@ impl<'mir, 'tcx> MutVisitor<'tcx> for ConstPropagator<'mir, 'tcx> { | |
); | ||
Self::remove_const(&mut self.ecx, place.local); | ||
} | ||
} else { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This arm didn't exist before, it's the |
||
trace!( | ||
"cannot propagate into {:?}, because the type of the local is generic.", | ||
place, | ||
); | ||
Self::remove_const(&mut self.ecx, place.local); | ||
} | ||
} else { | ||
match statement.kind { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,22 +46,8 @@ | |
// mir::Constant | ||
// + span: $DIR/bad_op_unsafe_oob_for_slices.rs:7:23: 7:24 | ||
// + literal: Const { ty: usize, val: Value(Scalar(0x0000000000000003)) } | ||
- _7 = Len((*_1)); // scope 2 at $DIR/bad_op_unsafe_oob_for_slices.rs:7:18: 7:25 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is "fallout" of checking for |
||
- _8 = Lt(_6, _7); // scope 2 at $DIR/bad_op_unsafe_oob_for_slices.rs:7:18: 7:25 | ||
+ _7 = const 3usize; // scope 2 at $DIR/bad_op_unsafe_oob_for_slices.rs:7:18: 7:25 | ||
+ // ty::Const | ||
+ // + ty: usize | ||
+ // + val: Value(Scalar(0x0000000000000003)) | ||
+ // mir::Constant | ||
+ // + span: $DIR/bad_op_unsafe_oob_for_slices.rs:7:18: 7:25 | ||
+ // + literal: Const { ty: usize, val: Value(Scalar(0x0000000000000003)) } | ||
+ _8 = const false; // scope 2 at $DIR/bad_op_unsafe_oob_for_slices.rs:7:18: 7:25 | ||
+ // ty::Const | ||
+ // + ty: bool | ||
+ // + val: Value(Scalar(0x00)) | ||
+ // mir::Constant | ||
+ // + span: $DIR/bad_op_unsafe_oob_for_slices.rs:7:18: 7:25 | ||
+ // + literal: Const { ty: bool, val: Value(Scalar(0x00)) } | ||
_7 = Len((*_1)); // scope 2 at $DIR/bad_op_unsafe_oob_for_slices.rs:7:18: 7:25 | ||
_8 = Lt(_6, _7); // scope 2 at $DIR/bad_op_unsafe_oob_for_slices.rs:7:18: 7:25 | ||
assert(move _8, "index out of bounds: the len is {} but the index is {}", move _7, _6) -> bb1; // scope 2 at $DIR/bad_op_unsafe_oob_for_slices.rs:7:18: 7:25 | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
#![feature(raw_ref_op)] | ||
|
||
// EMIT_MIR rustc.foo.ConstProp.diff | ||
fn foo() { | ||
let mut u = (1,); | ||
*&mut u.0 = 5; | ||
let y = { u.0 } == 5; | ||
} | ||
|
||
// EMIT_MIR rustc.bar.ConstProp.diff | ||
fn bar() { | ||
let mut v = (1,); | ||
unsafe { | ||
*&raw mut v.0 = 5; | ||
} | ||
let y = { v.0 } == 5; | ||
} | ||
|
||
fn main() { | ||
foo(); | ||
bar(); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,75 @@ | ||
- // MIR for `bar` before ConstProp | ||
+ // MIR for `bar` after ConstProp | ||
|
||
fn bar() -> () { | ||
let mut _0: (); // return place in scope 0 at $DIR/const_prop_miscompile.rs:11:10: 11:10 | ||
let mut _1: (i32,); // in scope 0 at $DIR/const_prop_miscompile.rs:12:9: 12:14 | ||
let _2: (); // in scope 0 at $DIR/const_prop_miscompile.rs:13:5: 15:6 | ||
let mut _3: *mut i32; // in scope 0 at $DIR/const_prop_miscompile.rs:14:10: 14:22 | ||
let mut _5: i32; // in scope 0 at $DIR/const_prop_miscompile.rs:16:13: 16:20 | ||
scope 1 { | ||
debug v => _1; // in scope 1 at $DIR/const_prop_miscompile.rs:12:9: 12:14 | ||
let _4: bool; // in scope 1 at $DIR/const_prop_miscompile.rs:16:9: 16:10 | ||
scope 2 { | ||
} | ||
scope 3 { | ||
debug y => _4; // in scope 3 at $DIR/const_prop_miscompile.rs:16:9: 16:10 | ||
} | ||
} | ||
|
||
bb0: { | ||
StorageLive(_1); // scope 0 at $DIR/const_prop_miscompile.rs:12:9: 12:14 | ||
- _1 = (const 1i32,); // scope 0 at $DIR/const_prop_miscompile.rs:12:17: 12:21 | ||
+ _1 = const (1i32,); // scope 0 at $DIR/const_prop_miscompile.rs:12:17: 12:21 | ||
// ty::Const | ||
- // + ty: i32 | ||
+ // + ty: (i32,) | ||
// + val: Value(Scalar(0x00000001)) | ||
// mir::Constant | ||
- // + span: $DIR/const_prop_miscompile.rs:12:18: 12:19 | ||
- // + literal: Const { ty: i32, val: Value(Scalar(0x00000001)) } | ||
+ // + span: $DIR/const_prop_miscompile.rs:12:17: 12:21 | ||
+ // + literal: Const { ty: (i32,), val: Value(Scalar(0x00000001)) } | ||
StorageLive(_2); // scope 1 at $DIR/const_prop_miscompile.rs:13:5: 15:6 | ||
StorageLive(_3); // scope 2 at $DIR/const_prop_miscompile.rs:14:10: 14:22 | ||
_3 = &raw mut (_1.0: i32); // scope 2 at $DIR/const_prop_miscompile.rs:14:10: 14:22 | ||
(*_3) = const 5i32; // scope 2 at $DIR/const_prop_miscompile.rs:14:9: 14:26 | ||
// ty::Const | ||
// + ty: i32 | ||
// + val: Value(Scalar(0x00000005)) | ||
// mir::Constant | ||
// + span: $DIR/const_prop_miscompile.rs:14:25: 14:26 | ||
// + literal: Const { ty: i32, val: Value(Scalar(0x00000005)) } | ||
StorageDead(_3); // scope 2 at $DIR/const_prop_miscompile.rs:14:26: 14:27 | ||
_2 = const (); // scope 2 at $DIR/const_prop_miscompile.rs:13:5: 15:6 | ||
// ty::Const | ||
// + ty: () | ||
// + val: Value(Scalar(<ZST>)) | ||
// mir::Constant | ||
// + span: $DIR/const_prop_miscompile.rs:13:5: 15:6 | ||
// + literal: Const { ty: (), val: Value(Scalar(<ZST>)) } | ||
StorageDead(_2); // scope 1 at $DIR/const_prop_miscompile.rs:15:5: 15:6 | ||
StorageLive(_4); // scope 1 at $DIR/const_prop_miscompile.rs:16:9: 16:10 | ||
StorageLive(_5); // scope 1 at $DIR/const_prop_miscompile.rs:16:13: 16:20 | ||
_5 = (_1.0: i32); // scope 1 at $DIR/const_prop_miscompile.rs:16:15: 16:18 | ||
_4 = Eq(move _5, const 5i32); // scope 1 at $DIR/const_prop_miscompile.rs:16:13: 16:25 | ||
// ty::Const | ||
// + ty: i32 | ||
// + val: Value(Scalar(0x00000005)) | ||
// mir::Constant | ||
// + span: $DIR/const_prop_miscompile.rs:16:24: 16:25 | ||
// + literal: Const { ty: i32, val: Value(Scalar(0x00000005)) } | ||
StorageDead(_5); // scope 1 at $DIR/const_prop_miscompile.rs:16:24: 16:25 | ||
_0 = const (); // scope 0 at $DIR/const_prop_miscompile.rs:11:10: 17:2 | ||
// ty::Const | ||
// + ty: () | ||
// + val: Value(Scalar(<ZST>)) | ||
// mir::Constant | ||
// + span: $DIR/const_prop_miscompile.rs:11:10: 17:2 | ||
// + literal: Const { ty: (), val: Value(Scalar(<ZST>)) } | ||
StorageDead(_4); // scope 1 at $DIR/const_prop_miscompile.rs:17:1: 17:2 | ||
StorageDead(_1); // scope 0 at $DIR/const_prop_miscompile.rs:17:1: 17:2 | ||
return; // scope 0 at $DIR/const_prop_miscompile.rs:17:2: 17:2 | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
- // MIR for `foo` before ConstProp | ||
+ // MIR for `foo` after ConstProp | ||
|
||
fn foo() -> () { | ||
let mut _0: (); // return place in scope 0 at $DIR/const_prop_miscompile.rs:4:10: 4:10 | ||
let mut _1: (i32,); // in scope 0 at $DIR/const_prop_miscompile.rs:5:9: 5:14 | ||
let mut _2: &mut i32; // in scope 0 at $DIR/const_prop_miscompile.rs:6:6: 6:14 | ||
let mut _4: i32; // in scope 0 at $DIR/const_prop_miscompile.rs:7:13: 7:20 | ||
scope 1 { | ||
debug u => _1; // in scope 1 at $DIR/const_prop_miscompile.rs:5:9: 5:14 | ||
let _3: bool; // in scope 1 at $DIR/const_prop_miscompile.rs:7:9: 7:10 | ||
scope 2 { | ||
debug y => _3; // in scope 2 at $DIR/const_prop_miscompile.rs:7:9: 7:10 | ||
} | ||
} | ||
|
||
bb0: { | ||
StorageLive(_1); // scope 0 at $DIR/const_prop_miscompile.rs:5:9: 5:14 | ||
- _1 = (const 1i32,); // scope 0 at $DIR/const_prop_miscompile.rs:5:17: 5:21 | ||
+ _1 = const (1i32,); // scope 0 at $DIR/const_prop_miscompile.rs:5:17: 5:21 | ||
// ty::Const | ||
- // + ty: i32 | ||
+ // + ty: (i32,) | ||
// + val: Value(Scalar(0x00000001)) | ||
// mir::Constant | ||
- // + span: $DIR/const_prop_miscompile.rs:5:18: 5:19 | ||
- // + literal: Const { ty: i32, val: Value(Scalar(0x00000001)) } | ||
+ // + span: $DIR/const_prop_miscompile.rs:5:17: 5:21 | ||
+ // + literal: Const { ty: (i32,), val: Value(Scalar(0x00000001)) } | ||
StorageLive(_2); // scope 1 at $DIR/const_prop_miscompile.rs:6:6: 6:14 | ||
_2 = &mut (_1.0: i32); // scope 1 at $DIR/const_prop_miscompile.rs:6:6: 6:14 | ||
(*_2) = const 5i32; // scope 1 at $DIR/const_prop_miscompile.rs:6:5: 6:18 | ||
// ty::Const | ||
// + ty: i32 | ||
// + val: Value(Scalar(0x00000005)) | ||
// mir::Constant | ||
// + span: $DIR/const_prop_miscompile.rs:6:17: 6:18 | ||
// + literal: Const { ty: i32, val: Value(Scalar(0x00000005)) } | ||
StorageDead(_2); // scope 1 at $DIR/const_prop_miscompile.rs:6:18: 6:19 | ||
StorageLive(_3); // scope 1 at $DIR/const_prop_miscompile.rs:7:9: 7:10 | ||
StorageLive(_4); // scope 1 at $DIR/const_prop_miscompile.rs:7:13: 7:20 | ||
_4 = (_1.0: i32); // scope 1 at $DIR/const_prop_miscompile.rs:7:15: 7:18 | ||
_3 = Eq(move _4, const 5i32); // scope 1 at $DIR/const_prop_miscompile.rs:7:13: 7:25 | ||
// ty::Const | ||
// + ty: i32 | ||
// + val: Value(Scalar(0x00000005)) | ||
// mir::Constant | ||
// + span: $DIR/const_prop_miscompile.rs:7:24: 7:25 | ||
// + literal: Const { ty: i32, val: Value(Scalar(0x00000005)) } | ||
StorageDead(_4); // scope 1 at $DIR/const_prop_miscompile.rs:7:24: 7:25 | ||
_0 = const (); // scope 0 at $DIR/const_prop_miscompile.rs:4:10: 8:2 | ||
// ty::Const | ||
// + ty: () | ||
// + val: Value(Scalar(<ZST>)) | ||
// mir::Constant | ||
// + span: $DIR/const_prop_miscompile.rs:4:10: 8:2 | ||
// + literal: Const { ty: (), val: Value(Scalar(<ZST>)) } | ||
StorageDead(_3); // scope 1 at $DIR/const_prop_miscompile.rs:8:1: 8:2 | ||
StorageDead(_1); // scope 0 at $DIR/const_prop_miscompile.rs:8:1: 8:2 | ||
return; // scope 0 at $DIR/const_prop_miscompile.rs:8:2: 8:2 | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this
match
should also be made exhaustive? It currently has a fallback case, basically assuming "every rvalue I don't know will definitely be harmless". That seems like a rather strong assumption.