remove an ineffective check in const_prop

This commit is contained in:
Ralf Jung 2022-08-07 10:36:42 -04:00
parent 4065b89b1e
commit 4173e971b8
5 changed files with 60 additions and 26 deletions

View file

@ -642,7 +642,7 @@ where
// avoid force_allocation. // avoid force_allocation.
let src = match self.read_immediate_raw(src)? { let src = match self.read_immediate_raw(src)? {
Ok(src_val) => { Ok(src_val) => {
assert!(!src.layout.is_unsized(), "cannot have unsized immediates"); assert!(!src.layout.is_unsized(), "cannot copy unsized immediates");
assert!( assert!(
!dest.layout.is_unsized(), !dest.layout.is_unsized(),
"the src is sized, so the dest must also be sized" "the src is sized, so the dest must also be sized"

View file

@ -100,6 +100,8 @@ where
// This makes several assumptions about what layouts we will encounter; we match what // This makes several assumptions about what layouts we will encounter; we match what
// codegen does as good as we can (see `extract_field` in `rustc_codegen_ssa/src/mir/operand.rs`). // codegen does as good as we can (see `extract_field` in `rustc_codegen_ssa/src/mir/operand.rs`).
let field_val: Immediate<_> = match (*base, base.layout.abi) { let field_val: Immediate<_> = match (*base, base.layout.abi) {
// if the entire value is uninit, then so is the field (can happen in ConstProp)
(Immediate::Uninit, _) => Immediate::Uninit,
// the field contains no information, can be left uninit // the field contains no information, can be left uninit
_ if field_layout.is_zst() => Immediate::Uninit, _ if field_layout.is_zst() => Immediate::Uninit,
// the field covers the entire type // the field covers the entire type
@ -124,6 +126,7 @@ where
b_val b_val
}) })
} }
// everything else is a bug
_ => span_bug!( _ => span_bug!(
self.cur_span(), self.cur_span(),
"invalid field access on immediate {}, layout {:#?}", "invalid field access on immediate {}, layout {:#?}",

View file

@ -248,16 +248,7 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine<'mir, 'tcx>
local: Local, local: Local,
) -> InterpResult<'tcx, &'a interpret::Operand<Self::Provenance>> { ) -> InterpResult<'tcx, &'a interpret::Operand<Self::Provenance>> {
let l = &frame.locals[local]; let l = &frame.locals[local];
// Applying restrictions here is meaningless since they can be circumvented via `force_allocation`.
if matches!(
l.value,
LocalValue::Live(interpret::Operand::Immediate(interpret::Immediate::Uninit))
) {
// For us "uninit" means "we don't know its value, might be initiailized or not".
// So stop here.
throw_machine_stop_str!("tried to access alocal with unknown value ")
}
l.access() l.access()
} }
@ -431,7 +422,13 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
fn get_const(&self, place: Place<'tcx>) -> Option<OpTy<'tcx>> { fn get_const(&self, place: Place<'tcx>) -> Option<OpTy<'tcx>> {
let op = match self.ecx.eval_place_to_op(place, None) { let op = match self.ecx.eval_place_to_op(place, None) {
Ok(op) => op, Ok(op) => {
if matches!(*op, interpret::Operand::Immediate(Immediate::Uninit)) {
// Make sure nobody accidentally uses this value.
return None;
}
op
}
Err(e) => { Err(e) => {
trace!("get_const failed: {}", e); trace!("get_const failed: {}", e);
return None; return None;
@ -643,6 +640,14 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
if rvalue.needs_subst() { if rvalue.needs_subst() {
return None; return None;
} }
if !rvalue
.ty(&self.ecx.frame().body.local_decls, *self.ecx.tcx)
.is_sized(self.ecx.tcx, self.param_env)
{
// the interpreter doesn't support unsized locals (only unsized arguments),
// but rustc does (in a kinda broken way), so we have to skip them here
return None;
}
if self.tcx.sess.mir_opt_level() >= 4 { if self.tcx.sess.mir_opt_level() >= 4 {
self.eval_rvalue_with_identities(rvalue, place) self.eval_rvalue_with_identities(rvalue, place)
@ -660,18 +665,20 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
self.use_ecx(|this| match rvalue { self.use_ecx(|this| match rvalue {
Rvalue::BinaryOp(op, box (left, right)) Rvalue::BinaryOp(op, box (left, right))
| Rvalue::CheckedBinaryOp(op, box (left, right)) => { | Rvalue::CheckedBinaryOp(op, box (left, right)) => {
let l = this.ecx.eval_operand(left, None); let l = this.ecx.eval_operand(left, None).and_then(|x| this.ecx.read_immediate(&x));
let r = this.ecx.eval_operand(right, None); let r =
this.ecx.eval_operand(right, None).and_then(|x| this.ecx.read_immediate(&x));
let const_arg = match (l, r) { let const_arg = match (l, r) {
(Ok(ref x), Err(_)) | (Err(_), Ok(ref x)) => this.ecx.read_immediate(x)?, (Ok(x), Err(_)) | (Err(_), Ok(x)) => x, // exactly one side is known
(Err(e), Err(_)) => return Err(e), (Err(e), Err(_)) => return Err(e), // neither side is known
(Ok(_), Ok(_)) => return this.ecx.eval_rvalue_into_place(rvalue, place), (Ok(_), Ok(_)) => return this.ecx.eval_rvalue_into_place(rvalue, place), // both sides are known
}; };
if !matches!(const_arg.layout.abi, abi::Abi::Scalar(..)) { if !matches!(const_arg.layout.abi, abi::Abi::Scalar(..)) {
// We cannot handle Scalar Pair stuff. // We cannot handle Scalar Pair stuff.
return this.ecx.eval_rvalue_into_place(rvalue, place); // No point in calling `eval_rvalue_into_place`, since only one side is known
throw_machine_stop_str!("cannot optimize this")
} }
let arg_value = const_arg.to_scalar().to_bits(const_arg.layout.size)?; let arg_value = const_arg.to_scalar().to_bits(const_arg.layout.size)?;
@ -696,7 +703,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
this.ecx.write_immediate(*const_arg, &dest) this.ecx.write_immediate(*const_arg, &dest)
} }
} }
_ => this.ecx.eval_rvalue_into_place(rvalue, place), _ => throw_machine_stop_str!("cannot optimize this"),
} }
} }
_ => this.ecx.eval_rvalue_into_place(rvalue, place), _ => this.ecx.eval_rvalue_into_place(rvalue, place),
@ -1073,7 +1080,11 @@ impl<'tcx> MutVisitor<'tcx> for ConstPropagator<'_, 'tcx> {
if let Some(ref value) = self.eval_operand(&cond) { if let Some(ref value) = self.eval_operand(&cond) {
trace!("assertion on {:?} should be {:?}", value, expected); trace!("assertion on {:?} should be {:?}", value, expected);
let expected = Scalar::from_bool(*expected); let expected = Scalar::from_bool(*expected);
let value_const = self.ecx.read_scalar(&value).unwrap(); let Ok(value_const) = self.ecx.read_scalar(&value) else {
// FIXME should be used use_ecx rather than a local match... but we have
// quite a few of these read_scalar/read_immediate that need fixing.
return
};
if expected != value_const { if expected != value_const {
// Poison all places this operand references so that further code // Poison all places this operand references so that further code
// doesn't use the invalid value // doesn't use the invalid value

View file

@ -6,6 +6,7 @@ use crate::const_prop::ConstPropMachine;
use crate::const_prop::ConstPropMode; use crate::const_prop::ConstPropMode;
use crate::MirLint; use crate::MirLint;
use rustc_const_eval::const_eval::ConstEvalErr; use rustc_const_eval::const_eval::ConstEvalErr;
use rustc_const_eval::interpret::Immediate;
use rustc_const_eval::interpret::{ use rustc_const_eval::interpret::{
self, InterpCx, InterpResult, LocalState, LocalValue, MemoryKind, OpTy, Scalar, StackPopCleanup, self, InterpCx, InterpResult, LocalState, LocalValue, MemoryKind, OpTy, Scalar, StackPopCleanup,
}; };
@ -229,7 +230,13 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
fn get_const(&self, place: Place<'tcx>) -> Option<OpTy<'tcx>> { fn get_const(&self, place: Place<'tcx>) -> Option<OpTy<'tcx>> {
let op = match self.ecx.eval_place_to_op(place, None) { let op = match self.ecx.eval_place_to_op(place, None) {
Ok(op) => op, Ok(op) => {
if matches!(*op, interpret::Operand::Immediate(Immediate::Uninit)) {
// Make sure nobody accidentally uses this value.
return None;
}
op
}
Err(e) => { Err(e) => {
trace!("get_const failed: {}", e); trace!("get_const failed: {}", e);
return None; return None;
@ -515,6 +522,14 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
if rvalue.needs_subst() { if rvalue.needs_subst() {
return None; return None;
} }
if !rvalue
.ty(&self.ecx.frame().body.local_decls, *self.ecx.tcx)
.is_sized(self.ecx.tcx, self.param_env)
{
// the interpreter doesn't support unsized locals (only unsized arguments),
// but rustc does (in a kinda broken way), so we have to skip them here
return None;
}
self.use_ecx(source_info, |this| this.ecx.eval_rvalue_into_place(rvalue, place)) self.use_ecx(source_info, |this| this.ecx.eval_rvalue_into_place(rvalue, place))
} }
@ -624,7 +639,11 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> {
if let Some(ref value) = self.eval_operand(&cond, source_info) { if let Some(ref value) = self.eval_operand(&cond, source_info) {
trace!("assertion on {:?} should be {:?}", value, expected); trace!("assertion on {:?} should be {:?}", value, expected);
let expected = Scalar::from_bool(*expected); let expected = Scalar::from_bool(*expected);
let value_const = self.ecx.read_scalar(&value).unwrap(); let Ok(value_const) = self.ecx.read_scalar(&value) else {
// FIXME should be used use_ecx rather than a local match... but we have
// quite a few of these read_scalar/read_immediate that need fixing.
return
};
if expected != value_const { if expected != value_const {
enum DbgVal<T> { enum DbgVal<T> {
Val(T), Val(T),
@ -641,9 +660,9 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> {
let mut eval_to_int = |op| { let mut eval_to_int = |op| {
// This can be `None` if the lhs wasn't const propagated and we just // This can be `None` if the lhs wasn't const propagated and we just
// triggered the assert on the value of the rhs. // triggered the assert on the value of the rhs.
self.eval_operand(op, source_info).map_or(DbgVal::Underscore, |op| { self.eval_operand(op, source_info)
DbgVal::Val(self.ecx.read_immediate(&op).unwrap().to_const_int()) .and_then(|op| self.ecx.read_immediate(&op).ok())
}) .map_or(DbgVal::Underscore, |op| DbgVal::Val(op.to_const_int()))
}; };
let msg = match msg { let msg = match msg {
AssertKind::DivisionByZero(op) => { AssertKind::DivisionByZero(op) => {

View file

@ -41,7 +41,8 @@
StorageLive(_4); // scope 2 at $DIR/mutable_variable_unprop_assign.rs:+4:9: +4:10 StorageLive(_4); // scope 2 at $DIR/mutable_variable_unprop_assign.rs:+4:9: +4:10
_4 = (_2.1: i32); // scope 2 at $DIR/mutable_variable_unprop_assign.rs:+4:13: +4:16 _4 = (_2.1: i32); // scope 2 at $DIR/mutable_variable_unprop_assign.rs:+4:13: +4:16
StorageLive(_5); // scope 3 at $DIR/mutable_variable_unprop_assign.rs:+5:9: +5:10 StorageLive(_5); // scope 3 at $DIR/mutable_variable_unprop_assign.rs:+5:9: +5:10
_5 = (_2.0: i32); // scope 3 at $DIR/mutable_variable_unprop_assign.rs:+5:13: +5:16 - _5 = (_2.0: i32); // scope 3 at $DIR/mutable_variable_unprop_assign.rs:+5:13: +5:16
+ _5 = const 1_i32; // scope 3 at $DIR/mutable_variable_unprop_assign.rs:+5:13: +5:16
nop; // scope 0 at $DIR/mutable_variable_unprop_assign.rs:+0:11: +6:2 nop; // scope 0 at $DIR/mutable_variable_unprop_assign.rs:+0:11: +6:2
StorageDead(_5); // scope 3 at $DIR/mutable_variable_unprop_assign.rs:+6:1: +6:2 StorageDead(_5); // scope 3 at $DIR/mutable_variable_unprop_assign.rs:+6:1: +6:2
StorageDead(_4); // scope 2 at $DIR/mutable_variable_unprop_assign.rs:+6:1: +6:2 StorageDead(_4); // scope 2 at $DIR/mutable_variable_unprop_assign.rs:+6:1: +6:2