[const-prop] Handle remaining MIR Rvalue cases #64890
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, 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 PointerTag = (); | ||
| type ExtraFnVal = !; | ||
|
|
||
|
|
@@ -434,32 +434,23 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { | |
| ) -> Option<Const<'tcx>> { | ||
| let span = source_info.span; | ||
|
|
||
| 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| { | ||
| 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; | ||
| } | ||
| } | ||
|
|
||
| // 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(()) | ||
| })?; | ||
| } | ||
| } | ||
|
|
||
| // 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. 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 = | ||
|
There was a problem hiding this comment. Why is this still needed, with the new machine hook and the const_prop machine? There was a problem hiding this comment. The alloc_local hook runs too late to fix this ICE because miri calls There was a problem hiding this comment. Does that mean Either way, this should be explained in a comment the code, not just in the PR. There was a problem hiding this comment. The rust/src/librustc_mir/transform/const_prop.rs Lines 625 to 645 in 53aca55 If we aren't allowed to const prop a local, it will There was a problem hiding this comment. I see. So which call to There was a problem hiding this comment. So that seems to come via this call: rust/src/librustc_mir/interpret/step.rs Line 244 in ea45150 That's odd though... we call There was a problem hiding this comment. That seems to be because fn validate_local_access(ecx: InterpCx, frame: Frame, local: Local) -> InterpResult<'tcx>There was a problem hiding this comment. Ah, we even have a comment there saying 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" in Miri already refers to this so let's avoid that term. But also, I don't think that would be right: when calling 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.
Got it, thanks!
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: and specifically with the unsized There was a problem hiding this comment. 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... |
||
| if let LocalValue::Live(_) = self.ecx.frame().locals[*local].value { | ||
| true | ||
|
|
@@ -535,6 +536,15 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { | |
| return None; | ||
| } | ||
| } | ||
|
|
||
| // Work around: avoid extra unnecessary locals. | ||
| // FIXME(wesleywiser): const eval will turn this into a `const Scalar(<ZST>)` that | ||
| // `SimplifyLocals` doesn't know it can remove. | ||
| Rvalue::Aggregate(_, operands) if operands.len() == 0 => { | ||
| return None; | ||
| } | ||
|
|
||
| _ => { } | ||
| } | ||
|
|
||
| self.use_ecx(source_info, |this| { | ||
|
|
||
| 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.