-
Notifications
You must be signed in to change notification settings - Fork 13.8k
[const-prop] Handle remaining MIR Rvalue cases #64890
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
Changes from all commits
c8f7e18
a2e3ed5
4d89031
2d22063
8cf6c23
b71ea80
fd20dbe
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 | ||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -8,7 +8,7 @@ use rustc::hir::def::DefKind; | |||||||||||||||||||||||||||||||||||||||||||||
use rustc::hir::def_id::DefId; | ||||||||||||||||||||||||||||||||||||||||||||||
use rustc::mir::{ | ||||||||||||||||||||||||||||||||||||||||||||||
AggregateKind, Constant, Location, Place, PlaceBase, Body, Operand, Rvalue, | ||||||||||||||||||||||||||||||||||||||||||||||
Local, NullOp, UnOp, StatementKind, Statement, LocalKind, | ||||||||||||||||||||||||||||||||||||||||||||||
Local, UnOp, StatementKind, Statement, LocalKind, | ||||||||||||||||||||||||||||||||||||||||||||||
TerminatorKind, Terminator, ClearCrossCrate, SourceInfo, BinOp, | ||||||||||||||||||||||||||||||||||||||||||||||
SourceScope, SourceScopeLocalData, LocalDecl, BasicBlock, | ||||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -118,7 +118,7 @@ impl<'tcx> MirPass<'tcx> for ConstProp { | |||||||||||||||||||||||||||||||||||||||||||||
struct ConstPropMachine; | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine { | ||||||||||||||||||||||||||||||||||||||||||||||
type MemoryKinds= !; | ||||||||||||||||||||||||||||||||||||||||||||||
type MemoryKinds = !; | ||||||||||||||||||||||||||||||||||||||||||||||
type PointerTag = (); | ||||||||||||||||||||||||||||||||||||||||||||||
type ExtraFnVal = !; | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -434,32 +434,23 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { | |||||||||||||||||||||||||||||||||||||||||||||
) -> Option<Const<'tcx>> { | ||||||||||||||||||||||||||||||||||||||||||||||
let span = source_info.span; | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
// if this isn't a supported operation, then return None | ||||||||||||||||||||||||||||||||||||||||||||||
match rvalue { | ||||||||||||||||||||||||||||||||||||||||||||||
Rvalue::Repeat(..) | | ||||||||||||||||||||||||||||||||||||||||||||||
Rvalue::Aggregate(..) | | ||||||||||||||||||||||||||||||||||||||||||||||
Rvalue::NullaryOp(NullOp::Box, _) | | ||||||||||||||||||||||||||||||||||||||||||||||
Rvalue::Discriminant(..) => return None, | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
Rvalue::Use(_) | | ||||||||||||||||||||||||||||||||||||||||||||||
Rvalue::Len(_) | | ||||||||||||||||||||||||||||||||||||||||||||||
Rvalue::Cast(..) | | ||||||||||||||||||||||||||||||||||||||||||||||
Rvalue::NullaryOp(..) | | ||||||||||||||||||||||||||||||||||||||||||||||
Rvalue::CheckedBinaryOp(..) | | ||||||||||||||||||||||||||||||||||||||||||||||
Rvalue::Ref(..) | | ||||||||||||||||||||||||||||||||||||||||||||||
Rvalue::UnaryOp(..) | | ||||||||||||||||||||||||||||||||||||||||||||||
Rvalue::BinaryOp(..) => { } | ||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||
let overflow_check = self.tcx.sess.overflow_checks(); | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
// perform any special checking for specific Rvalue types | ||||||||||||||||||||||||||||||||||||||||||||||
if let Rvalue::UnaryOp(op, arg) = rvalue { | ||||||||||||||||||||||||||||||||||||||||||||||
trace!("checking UnaryOp(op = {:?}, arg = {:?})", op, arg); | ||||||||||||||||||||||||||||||||||||||||||||||
let overflow_check = self.tcx.sess.overflow_checks(); | ||||||||||||||||||||||||||||||||||||||||||||||
// Perform any special handling for specific Rvalue types. | ||||||||||||||||||||||||||||||||||||||||||||||
// Generally, checks here fall into one of two categories: | ||||||||||||||||||||||||||||||||||||||||||||||
// 1. Additional checking to provide useful lints to the user | ||||||||||||||||||||||||||||||||||||||||||||||
// - In this case, we will do some validation and then fall through to the | ||||||||||||||||||||||||||||||||||||||||||||||
// end of the function which evals the assignment. | ||||||||||||||||||||||||||||||||||||||||||||||
// 2. Working around bugs in other parts of the compiler | ||||||||||||||||||||||||||||||||||||||||||||||
// - In this case, we'll return `None` from this function to stop evaluation. | ||||||||||||||||||||||||||||||||||||||||||||||
match rvalue { | ||||||||||||||||||||||||||||||||||||||||||||||
// Additional checking: if overflow checks are disabled (which is usually the case in | ||||||||||||||||||||||||||||||||||||||||||||||
// release mode), then we need to do additional checking here to give lints to the user | ||||||||||||||||||||||||||||||||||||||||||||||
// if an overflow would occur. | ||||||||||||||||||||||||||||||||||||||||||||||
Rvalue::UnaryOp(UnOp::Neg, arg) if !overflow_check => { | ||||||||||||||||||||||||||||||||||||||||||||||
trace!("checking UnaryOp(op = Neg, arg = {:?})", arg); | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
self.use_ecx(source_info, |this| { | ||||||||||||||||||||||||||||||||||||||||||||||
// We check overflow in debug mode already | ||||||||||||||||||||||||||||||||||||||||||||||
// so should only check in release mode. | ||||||||||||||||||||||||||||||||||||||||||||||
if *op == UnOp::Neg && !overflow_check { | ||||||||||||||||||||||||||||||||||||||||||||||
self.use_ecx(source_info, |this| { | ||||||||||||||||||||||||||||||||||||||||||||||
let ty = arg.ty(&this.local_decls, this.tcx); | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
if ty.is_integral() { | ||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -471,60 +462,70 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { | |||||||||||||||||||||||||||||||||||||||||||||
throw_panic!(OverflowNeg) | ||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
Ok(()) | ||||||||||||||||||||||||||||||||||||||||||||||
})?; | ||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
// Additional checking: check for overflows on integer binary operations and report | ||||||||||||||||||||||||||||||||||||||||||||||
// them to the user as lints. | ||||||||||||||||||||||||||||||||||||||||||||||
Rvalue::BinaryOp(op, left, right) => { | ||||||||||||||||||||||||||||||||||||||||||||||
trace!("checking BinaryOp(op = {:?}, left = {:?}, right = {:?})", op, left, right); | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
let r = self.use_ecx(source_info, |this| { | ||||||||||||||||||||||||||||||||||||||||||||||
this.ecx.read_immediate(this.ecx.eval_operand(right, None)?) | ||||||||||||||||||||||||||||||||||||||||||||||
})?; | ||||||||||||||||||||||||||||||||||||||||||||||
if *op == BinOp::Shr || *op == BinOp::Shl { | ||||||||||||||||||||||||||||||||||||||||||||||
let left_bits = place_layout.size.bits(); | ||||||||||||||||||||||||||||||||||||||||||||||
let right_size = r.layout.size; | ||||||||||||||||||||||||||||||||||||||||||||||
let r_bits = r.to_scalar().and_then(|r| r.to_bits(right_size)); | ||||||||||||||||||||||||||||||||||||||||||||||
if r_bits.ok().map_or(false, |b| b >= left_bits as u128) { | ||||||||||||||||||||||||||||||||||||||||||||||
let source_scope_local_data = match self.source_scope_local_data { | ||||||||||||||||||||||||||||||||||||||||||||||
ClearCrossCrate::Set(ref data) => data, | ||||||||||||||||||||||||||||||||||||||||||||||
ClearCrossCrate::Clear => return None, | ||||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||||
let dir = if *op == BinOp::Shr { | ||||||||||||||||||||||||||||||||||||||||||||||
"right" | ||||||||||||||||||||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||||||||||||||||||||
"left" | ||||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||||
let hir_id = source_scope_local_data[source_info.scope].lint_root; | ||||||||||||||||||||||||||||||||||||||||||||||
self.tcx.lint_hir( | ||||||||||||||||||||||||||||||||||||||||||||||
::rustc::lint::builtin::EXCEEDING_BITSHIFTS, | ||||||||||||||||||||||||||||||||||||||||||||||
hir_id, | ||||||||||||||||||||||||||||||||||||||||||||||
span, | ||||||||||||||||||||||||||||||||||||||||||||||
&format!("attempt to shift {} with overflow", dir)); | ||||||||||||||||||||||||||||||||||||||||||||||
return None; | ||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
Ok(()) | ||||||||||||||||||||||||||||||||||||||||||||||
})?; | ||||||||||||||||||||||||||||||||||||||||||||||
} else if let Rvalue::BinaryOp(op, left, right) = rvalue { | ||||||||||||||||||||||||||||||||||||||||||||||
trace!("checking BinaryOp(op = {:?}, left = {:?}, right = {:?})", op, left, right); | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
let r = self.use_ecx(source_info, |this| { | ||||||||||||||||||||||||||||||||||||||||||||||
this.ecx.read_immediate(this.ecx.eval_operand(right, None)?) | ||||||||||||||||||||||||||||||||||||||||||||||
})?; | ||||||||||||||||||||||||||||||||||||||||||||||
if *op == BinOp::Shr || *op == BinOp::Shl { | ||||||||||||||||||||||||||||||||||||||||||||||
let left_bits = place_layout.size.bits(); | ||||||||||||||||||||||||||||||||||||||||||||||
let right_size = r.layout.size; | ||||||||||||||||||||||||||||||||||||||||||||||
let r_bits = r.to_scalar().and_then(|r| r.to_bits(right_size)); | ||||||||||||||||||||||||||||||||||||||||||||||
if r_bits.ok().map_or(false, |b| b >= left_bits as u128) { | ||||||||||||||||||||||||||||||||||||||||||||||
let source_scope_local_data = match self.source_scope_local_data { | ||||||||||||||||||||||||||||||||||||||||||||||
ClearCrossCrate::Set(ref data) => data, | ||||||||||||||||||||||||||||||||||||||||||||||
ClearCrossCrate::Clear => return None, | ||||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||||
let dir = if *op == BinOp::Shr { | ||||||||||||||||||||||||||||||||||||||||||||||
"right" | ||||||||||||||||||||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||||||||||||||||||||
"left" | ||||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||||
let hir_id = source_scope_local_data[source_info.scope].lint_root; | ||||||||||||||||||||||||||||||||||||||||||||||
self.tcx.lint_hir( | ||||||||||||||||||||||||||||||||||||||||||||||
::rustc::lint::builtin::EXCEEDING_BITSHIFTS, | ||||||||||||||||||||||||||||||||||||||||||||||
hir_id, | ||||||||||||||||||||||||||||||||||||||||||||||
span, | ||||||||||||||||||||||||||||||||||||||||||||||
&format!("attempt to shift {} with overflow", dir)); | ||||||||||||||||||||||||||||||||||||||||||||||
return None; | ||||||||||||||||||||||||||||||||||||||||||||||
// If overflow checking is enabled (like in debug mode by default), | ||||||||||||||||||||||||||||||||||||||||||||||
// then we'll already catch overflow when we evaluate the `Assert` statement | ||||||||||||||||||||||||||||||||||||||||||||||
// in MIR. However, if overflow checking is disabled, then there won't be any | ||||||||||||||||||||||||||||||||||||||||||||||
// `Assert` statement and so we have to do additional checking here. | ||||||||||||||||||||||||||||||||||||||||||||||
if !overflow_check { | ||||||||||||||||||||||||||||||||||||||||||||||
self.use_ecx(source_info, |this| { | ||||||||||||||||||||||||||||||||||||||||||||||
let l = this.ecx.read_immediate(this.ecx.eval_operand(left, None)?)?; | ||||||||||||||||||||||||||||||||||||||||||||||
let (_, overflow, _ty) = this.ecx.overflowing_binary_op(*op, l, r)?; | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
if overflow { | ||||||||||||||||||||||||||||||||||||||||||||||
let err = err_panic!(Overflow(*op)).into(); | ||||||||||||||||||||||||||||||||||||||||||||||
return Err(err); | ||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
Ok(()) | ||||||||||||||||||||||||||||||||||||||||||||||
})?; | ||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||
self.use_ecx(source_info, |this| { | ||||||||||||||||||||||||||||||||||||||||||||||
let l = this.ecx.read_immediate(this.ecx.eval_operand(left, None)?)?; | ||||||||||||||||||||||||||||||||||||||||||||||
let (_, overflow, _ty) = this.ecx.overflowing_binary_op(*op, l, r)?; | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
// We check overflow in debug mode already | ||||||||||||||||||||||||||||||||||||||||||||||
// so should only check in release mode. | ||||||||||||||||||||||||||||||||||||||||||||||
if !this.tcx.sess.overflow_checks() && overflow { | ||||||||||||||||||||||||||||||||||||||||||||||
let err = err_panic!(Overflow(*op)).into(); | ||||||||||||||||||||||||||||||||||||||||||||||
return Err(err); | ||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
Ok(()) | ||||||||||||||||||||||||||||||||||||||||||||||
})?; | ||||||||||||||||||||||||||||||||||||||||||||||
} else if let Rvalue::Ref(_, _, place) = rvalue { | ||||||||||||||||||||||||||||||||||||||||||||||
trace!("checking Ref({:?})", place); | ||||||||||||||||||||||||||||||||||||||||||||||
// Work around: avoid ICE in miri. | ||||||||||||||||||||||||||||||||||||||||||||||
// FIXME(wesleywiser) we don't currently handle the case where we try to make a ref | ||||||||||||||||||||||||||||||||||||||||||||||
// from a function argument that hasn't been assigned to in this function. | ||||||||||||||||||||||||||||||||||||||||||||||
if let Place { | ||||||||||||||||||||||||||||||||||||||||||||||
base: PlaceBase::Local(local), | ||||||||||||||||||||||||||||||||||||||||||||||
projection: box [] | ||||||||||||||||||||||||||||||||||||||||||||||
} = place { | ||||||||||||||||||||||||||||||||||||||||||||||
// from a function argument that hasn't been assigned to in this function. The main | ||||||||||||||||||||||||||||||||||||||||||||||
// issue is if an arg is a fat-pointer, miri `expects()` to be able to read the value | ||||||||||||||||||||||||||||||||||||||||||||||
// of that pointer to get size info. However, since this is `ConstProp`, that argument | ||||||||||||||||||||||||||||||||||||||||||||||
// doesn't actually have a backing value and so this causes an ICE. | ||||||||||||||||||||||||||||||||||||||||||||||
Rvalue::Ref(_, _, Place { base: PlaceBase::Local(local), projection: box [] }) => { | ||||||||||||||||||||||||||||||||||||||||||||||
trace!("checking Ref({:?})", place); | ||||||||||||||||||||||||||||||||||||||||||||||
let alive = | ||||||||||||||||||||||||||||||||||||||||||||||
|
/// returns true if `local` can be propagated | |
fn check(body: &Body<'_>) -> IndexVec<Local, bool> { | |
let mut cpv = CanConstProp { | |
can_const_prop: IndexVec::from_elem(true, &body.local_decls), | |
found_assignment: IndexVec::from_elem(false, &body.local_decls), | |
}; | |
for (local, val) in cpv.can_const_prop.iter_enumerated_mut() { | |
// cannot use args at all | |
// cannot use locals because if x < y { y - x } else { x - y } would | |
// lint for x != y | |
// FIXME(oli-obk): lint variables until they are used in a condition | |
// FIXME(oli-obk): lint if return value is constant | |
*val = body.local_kind(local) == LocalKind::Temp; | |
if !*val { | |
trace!("local {:?} can't be propagated because it's not a temporary", local); | |
} | |
} | |
cpv.visit_body(body); | |
cpv.can_const_prop | |
} |
If we aren't allowed to const prop a local, it will LocalValue::Uninitialized
when ConstPropMachine
tries to access it.
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.
I see. So which call to size_and_align_of
is the one that is guarded by the check here?
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.
So that seems to come via this call:
rust/src/librustc_mir/interpret/step.rs
Line 244 in ea45150
let place = self.force_allocation(src)?; |
That's odd though... we call eval_place
before that so if it was a local, access_local
could actually throw an error to never reach the later part?
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.
That seems to be because eval_place()
doesn't call access_local()
. access_local()
returns an Operand
which eval_place()
wouldn't use at all since it just returns a PlaceTy
. Maybe we could generalize access_local()
to just do validation? Perhaps something like:
fn validate_local_access(ecx: InterpCx, frame: Frame, local: Local) -> InterpResult<'tcx>
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.
Ah, we even have a comment there saying
// This works even for dead/uninitialized locals; we check further when writing
Except for unsized locals it doesn't... or well it does but only to initialize them; in this stacktrace we are taking a reference to them.
validate_local_access
"validate" in Miri already refers to this so let's avoid that term. But also, I don't think that would be right: when calling eval_place
for the dest
, we are fine with uninitialized locals as we will write to them!
What basically happens here is that const_prop causes the engine to take a reference to an uninitialized unsized value. That's impossible to implement (taking a reference needs an allocation in memory but to have one we need a size, which we cannot determine for an unsized uninitialized value). Maybe this check is indeed better than the possible alternatives (though it would be nice for the comment here to explain that).
The comment here talks about arguments that are fat pointers; is that correct? Fat pointers are also just sized data (twice as big as normal pointers) and shouldn't be a problem. Unsized arguments however are a problem, if my analysis is correct.
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.
"validate" in Miri already refers to this so let's avoid that term.
Got it, thanks!
The comment here talks about arguments that are fat pointers; is that correct? Fat pointers are also just sized data (twice as big as normal pointers) and shouldn't be a problem. Unsized arguments however are a problem, if my analysis is correct.
Your analysis is correct; it was early and I hadn't yet had coffee when I wrote "fat pointers". The ICE I linked to occurs when processing:
#0 [optimized_mir] processing `<[char] as Foo>::foo`
and specifically with the unsized self
parameter.
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.
Good, I think we reached a conclusion here then. :) Would be good to have that documented in the code, but right now I don't see a good way to handle this inside the engine.
I am really not happy with our (Miri's) treatment of unsized locals in general, and this puts the finger on what doesn't work well...
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
// compile-flags: -O | ||
|
||
fn main() { | ||
let x = (0, 1, 2).1 + 0; | ||
} | ||
|
||
// END RUST SOURCE | ||
// START rustc.main.ConstProp.before.mir | ||
// bb0: { | ||
// ... | ||
// _3 = (const 0i32, const 1i32, const 2i32); | ||
// _2 = (_3.1: i32); | ||
// _1 = Add(move _2, const 0i32); | ||
// ... | ||
// } | ||
// END rustc.main.ConstProp.before.mir | ||
// START rustc.main.ConstProp.after.mir | ||
// bb0: { | ||
// ... | ||
// _3 = (const 0i32, const 1i32, const 2i32); | ||
// _2 = const 1i32; | ||
// _1 = Add(move _2, const 0i32); | ||
// ... | ||
// } | ||
// END rustc.main.ConstProp.after.mir |
Uh oh!
There was an error while loading. Please reload this page.