From fbd10a3cc5b065c3dba31a19ceb27192a8c15841 Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Thu, 4 Jan 2024 15:15:33 +0000 Subject: [PATCH 01/15] Allow passing a layout to the `eval_*` methods --- .../src/const_prop_lint.rs | 37 +++++++++++++------ 1 file changed, 26 insertions(+), 11 deletions(-) diff --git a/compiler/rustc_mir_transform/src/const_prop_lint.rs b/compiler/rustc_mir_transform/src/const_prop_lint.rs index b8fecaf635aa2..75bf7f4866f3f 100644 --- a/compiler/rustc_mir_transform/src/const_prop_lint.rs +++ b/compiler/rustc_mir_transform/src/const_prop_lint.rs @@ -244,7 +244,12 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { } /// Returns the value, if any, of evaluating `c`. - fn eval_constant(&mut self, c: &ConstOperand<'tcx>, location: Location) -> Option> { + fn eval_constant( + &mut self, + c: &ConstOperand<'tcx>, + location: Location, + layout: Option>, + ) -> Option> { // FIXME we need to revisit this for #67176 if c.has_param() { return None; @@ -258,21 +263,31 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { // manually normalized. let val = self.tcx.try_normalize_erasing_regions(self.param_env, c.const_).ok()?; - self.use_ecx(location, |this| this.ecx.eval_mir_constant(&val, Some(c.span), None)) + self.use_ecx(location, |this| this.ecx.eval_mir_constant(&val, Some(c.span), layout)) } /// Returns the value, if any, of evaluating `place`. - fn eval_place(&mut self, place: Place<'tcx>, location: Location) -> Option> { + fn eval_place( + &mut self, + place: Place<'tcx>, + location: Location, + layout: Option>, + ) -> Option> { trace!("eval_place(place={:?})", place); - self.use_ecx(location, |this| this.ecx.eval_place_to_op(place, None)) + self.use_ecx(location, |this| this.ecx.eval_place_to_op(place, layout)) } /// Returns the value, if any, of evaluating `op`. Calls upon `eval_constant` /// or `eval_place`, depending on the variant of `Operand` used. - fn eval_operand(&mut self, op: &Operand<'tcx>, location: Location) -> Option> { + fn eval_operand( + &mut self, + op: &Operand<'tcx>, + location: Location, + layout: Option>, + ) -> Option> { match *op { - Operand::Constant(ref c) => self.eval_constant(c, location), - Operand::Move(place) | Operand::Copy(place) => self.eval_place(place, location), + Operand::Constant(ref c) => self.eval_constant(c, location, layout), + Operand::Move(place) | Operand::Copy(place) => self.eval_place(place, location, layout), } } @@ -453,7 +468,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { cond: &Operand<'tcx>, location: Location, ) -> Option { - let value = &self.eval_operand(cond, location)?; + let value = &self.eval_operand(cond, location, None)?; trace!("assertion on {:?} should be {:?}", value, expected); let expected = Scalar::from_bool(expected); @@ -481,7 +496,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { let mut eval_to_int = |op| { // This can be `None` if the lhs wasn't const propagated and we just // triggered the assert on the value of the rhs. - self.eval_operand(op, location) + self.eval_operand(op, location, None) .and_then(|op| self.ecx.read_immediate(&op).ok()) .map_or(DbgVal::Underscore, |op| DbgVal::Val(op.to_const_int())) }; @@ -546,7 +561,7 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { fn visit_constant(&mut self, constant: &ConstOperand<'tcx>, location: Location) { trace!("visit_constant: {:?}", constant); self.super_constant(constant, location); - self.eval_constant(constant, location); + self.eval_constant(constant, location, None); } fn visit_assign(&mut self, place: &Place<'tcx>, rvalue: &Rvalue<'tcx>, location: Location) { @@ -626,7 +641,7 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { self.check_assertion(*expected, msg, cond, location); } TerminatorKind::SwitchInt { ref discr, ref targets } => { - if let Some(ref value) = self.eval_operand(discr, location) + if let Some(ref value) = self.eval_operand(discr, location, None) && let Some(value_const) = self.use_ecx(location, |this| this.ecx.read_scalar(value)) && let Ok(constant) = value_const.try_to_int() From ac48ad517b75a5a6237d5faaf3175651861283d5 Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Thu, 4 Jan 2024 16:27:05 +0000 Subject: [PATCH 02/15] partially inline `eval_rvalue_into_place` for const prop lint --- compiler/rustc_const_eval/src/util/mod.rs | 4 +- .../src/const_prop_lint.rs | 191 +++++++++++++++++- 2 files changed, 187 insertions(+), 8 deletions(-) diff --git a/compiler/rustc_const_eval/src/util/mod.rs b/compiler/rustc_const_eval/src/util/mod.rs index 1e58bd645cdde..a8060463b69ed 100644 --- a/compiler/rustc_const_eval/src/util/mod.rs +++ b/compiler/rustc_const_eval/src/util/mod.rs @@ -14,7 +14,7 @@ pub use self::type_name::type_name; /// Classify whether an operator is "left-homogeneous", i.e., the LHS has the /// same type as the result. #[inline] -pub(crate) fn binop_left_homogeneous(op: mir::BinOp) -> bool { +pub fn binop_left_homogeneous(op: mir::BinOp) -> bool { use rustc_middle::mir::BinOp::*; match op { Add | AddUnchecked | Sub | SubUnchecked | Mul | MulUnchecked | Div | Rem | BitXor @@ -26,7 +26,7 @@ pub(crate) fn binop_left_homogeneous(op: mir::BinOp) -> bool { /// Classify whether an operator is "right-homogeneous", i.e., the RHS has the /// same type as the LHS. #[inline] -pub(crate) fn binop_right_homogeneous(op: mir::BinOp) -> bool { +pub fn binop_right_homogeneous(op: mir::BinOp) -> bool { use rustc_middle::mir::BinOp::*; match op { Add | AddUnchecked | Sub | SubUnchecked | Mul | MulUnchecked | Div | Rem | BitXor diff --git a/compiler/rustc_mir_transform/src/const_prop_lint.rs b/compiler/rustc_mir_transform/src/const_prop_lint.rs index 75bf7f4866f3f..fff40b8c272d1 100644 --- a/compiler/rustc_mir_transform/src/const_prop_lint.rs +++ b/compiler/rustc_mir_transform/src/const_prop_lint.rs @@ -5,7 +5,7 @@ use std::fmt::Debug; use either::Left; -use rustc_const_eval::interpret::Immediate; +use rustc_const_eval::interpret::{ImmTy, Immediate, Projectable}; use rustc_const_eval::interpret::{ InterpCx, InterpResult, MemoryKind, OpTy, Scalar, StackPopCleanup, }; @@ -21,7 +21,7 @@ use rustc_middle::ty::{ self, ConstInt, Instance, ParamEnv, ScalarInt, Ty, TyCtxt, TypeVisitableExt, }; use rustc_span::Span; -use rustc_target::abi::{HasDataLayout, Size, TargetDataLayout}; +use rustc_target::abi::{self, Abi, HasDataLayout, Size, TargetDataLayout}; use crate::const_prop::CanConstProp; use crate::const_prop::ConstPropMachine; @@ -540,6 +540,188 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { ) } } + + #[instrument(level = "trace", skip(self), ret)] + fn eval_rvalue( + &mut self, + rvalue: &Rvalue<'tcx>, + location: Location, + dest: &Place<'tcx>, + ) -> Option<()> { + if !dest.projection.is_empty() { + return None; + } + use rustc_middle::mir::Rvalue::*; + let dest = self.use_ecx(location, |this| this.ecx.eval_place(*dest))?; + trace!(?dest); + + let val = match *rvalue { + ThreadLocalRef(_) => return None, + + Use(ref operand) => self.eval_operand(operand, location, Some(dest.layout))?, + + CopyForDeref(place) => self.eval_place(place, location, Some(dest.layout))?, + + BinaryOp(bin_op, box (ref left, ref right)) => { + let layout = + rustc_const_eval::util::binop_left_homogeneous(bin_op).then_some(dest.layout); + let left = self.eval_operand(left, location, layout)?; + let left = self.use_ecx(location, |this| this.ecx.read_immediate(&left))?; + + let layout = + rustc_const_eval::util::binop_right_homogeneous(bin_op).then_some(left.layout); + let right = self.eval_operand(right, location, layout)?; + let right = self.use_ecx(location, |this| this.ecx.read_immediate(&right))?; + + let val = self + .use_ecx(location, |this| this.ecx.wrapping_binary_op(bin_op, &left, &right))?; + val.into() + } + + CheckedBinaryOp(bin_op, box (ref left, ref right)) => { + let left = self.eval_operand(left, location, None)?; + let left = self.use_ecx(location, |this| this.ecx.read_immediate(&left))?; + + let layout = + rustc_const_eval::util::binop_right_homogeneous(bin_op).then_some(left.layout); + let right = self.eval_operand(right, location, layout)?; + let right = self.use_ecx(location, |this| this.ecx.read_immediate(&right))?; + + let (val, overflowed) = self.use_ecx(location, |this| { + this.ecx.overflowing_binary_op(bin_op, &left, &right) + })?; + let tuple = Ty::new_tup_from_iter( + self.tcx, + [val.layout.ty, self.tcx.types.bool].into_iter(), + ); + let tuple = self.ecx.layout_of(tuple).ok()?; + let val = + ImmTy::from_scalar_pair(val.to_scalar(), Scalar::from_bool(overflowed), tuple); + val.into() + } + + UnaryOp(un_op, ref operand) => { + let operand = self.eval_operand(operand, location, Some(dest.layout))?; + let val = self.use_ecx(location, |this| this.ecx.read_immediate(&operand))?; + + let val = self.use_ecx(location, |this| this.ecx.wrapping_unary_op(un_op, &val))?; + val.into() + } + + Aggregate(ref kind, ref fields) => { + trace!(?kind); + trace!(?dest.layout); + if dest.layout.is_zst() { + ImmTy::uninit(dest.layout).into() + } else if let Abi::Scalar(abi::Scalar::Initialized { .. }) = dest.layout.abi { + let fields = fields + .iter() + .map(|field| self.eval_operand(field, location, None)) + .collect::>>()?; + trace!(?fields); + let mut field = + fields.into_iter().find(|field| field.layout.abi.is_scalar())?; + field.layout = dest.layout; + field + } else if let Abi::ScalarPair( + abi::Scalar::Initialized { .. }, + abi::Scalar::Initialized { .. }, + ) = dest.layout.abi + { + let fields = fields + .iter() + .map(|field| self.eval_operand(field, location, None)) + .collect::>>()?; + trace!(?fields); + let pair = + fields.iter().find(|field| matches!(field.layout.abi, Abi::ScalarPair(..))); + if let Some(pair) = pair { + let mut pair = pair.clone(); + pair.layout = dest.layout; + pair + } else { + // TODO: build a pair from two scalars + return None; + } + } else { + return None; + } + } + + Repeat(ref op, n) => { + trace!(?op, ?n); + return None; + } + + Len(place) => { + let src = self.eval_place(place, location, None)?; + let len = src.len(&self.ecx).ok()?; + ImmTy::from_scalar(Scalar::from_target_usize(len, self), dest.layout).into() + } + + Ref(..) | AddressOf(..) => return None, + + NullaryOp(ref null_op, ty) => { + let layout = self.use_ecx(location, |this| this.ecx.layout_of(ty))?; + let val = match null_op { + NullOp::SizeOf => layout.size.bytes(), + NullOp::AlignOf => layout.align.abi.bytes(), + NullOp::OffsetOf(fields) => { + layout.offset_of_subfield(self, fields.iter()).bytes() + } + }; + ImmTy::from_scalar(Scalar::from_target_usize(val, self), dest.layout).into() + } + + ShallowInitBox(..) => return None, + + Cast(ref kind, ref value, to) => match kind { + CastKind::IntToInt | CastKind::IntToFloat => { + let value = self.eval_operand(value, location, None)?; + let value = self.ecx.read_immediate(&value).ok()?; + let to = self.ecx.layout_of(to).ok()?; + let res = self.ecx.int_to_int_or_float(&value, to).ok()?; + res.into() + } + CastKind::FloatToFloat | CastKind::FloatToInt => { + let value = self.eval_operand(value, location, None)?; + let value = self.ecx.read_immediate(&value).ok()?; + let to = self.ecx.layout_of(to).ok()?; + let res = self.ecx.float_to_float_or_int(&value, to).ok()?; + res.into() + } + CastKind::Transmute => { + let value = self.eval_operand(value, location, None)?; + let to = self.ecx.layout_of(to).ok()?; + // `offset` for immediates only supports scalar/scalar-pair ABIs, + // so bail out if the target is not one. + if value.as_mplace_or_imm().is_right() { + match (value.layout.abi, to.abi) { + (Abi::Scalar(..), Abi::Scalar(..)) => {} + (Abi::ScalarPair(..), Abi::ScalarPair(..)) => {} + _ => return None, + } + } + value.offset(Size::ZERO, to, &self.ecx).ok()? + } + _ => return None, + }, + + Discriminant(place) => { + let op = self.eval_place(place, location, None)?; + let variant = self.use_ecx(location, |this| this.ecx.read_discriminant(&op))?; + let imm = self.use_ecx(location, |this| { + this.ecx.discriminant_for_variant(op.layout.ty, variant) + })?; + imm.into() + } + }; + trace!(?val); + + self.use_ecx(location, |this| this.ecx.copy_op(&val, &dest, true))?; + + Some(()) + } } impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { @@ -574,10 +756,7 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { _ if place.is_indirect() => {} ConstPropMode::NoPropagation => self.ensure_not_propagated(place.local), ConstPropMode::OnlyInsideOwnBlock | ConstPropMode::FullConstProp => { - if self - .use_ecx(location, |this| this.ecx.eval_rvalue_into_place(rvalue, *place)) - .is_none() - { + if self.eval_rvalue(rvalue, location, place).is_none() { // Const prop failed, so erase the destination, ensuring that whatever happens // from here on, does not know about the previous value. // This is important in case we have From e904a640ac57624aff9635151b4ba53e88b0866c Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Fri, 5 Jan 2024 17:08:58 +0000 Subject: [PATCH 03/15] Stop using `eval_rvalue_into_place` in const prop --- .../src/const_prop_lint.rs | 356 ++++++++++-------- tests/ui/consts/const-err-late.stderr | 8 + tests/ui/consts/issue-65348.rs | 7 +- 3 files changed, 212 insertions(+), 159 deletions(-) diff --git a/compiler/rustc_mir_transform/src/const_prop_lint.rs b/compiler/rustc_mir_transform/src/const_prop_lint.rs index fff40b8c272d1..4ddcabcc90f7e 100644 --- a/compiler/rustc_mir_transform/src/const_prop_lint.rs +++ b/compiler/rustc_mir_transform/src/const_prop_lint.rs @@ -5,14 +5,12 @@ use std::fmt::Debug; use either::Left; -use rustc_const_eval::interpret::{ImmTy, Immediate, Projectable}; -use rustc_const_eval::interpret::{ - InterpCx, InterpResult, MemoryKind, OpTy, Scalar, StackPopCleanup, -}; -use rustc_const_eval::ReportErrorExt; +use rustc_const_eval::interpret::{ImmTy, MPlaceTy, Projectable}; +use rustc_const_eval::interpret::{InterpCx, InterpResult, OpTy, Scalar, StackPopCleanup}; use rustc_hir::def::DefKind; use rustc_hir::HirId; use rustc_index::bit_set::BitSet; +use rustc_index::{Idx, IndexVec}; use rustc_middle::mir::visit::Visitor; use rustc_middle::mir::*; use rustc_middle::ty::layout::{LayoutError, LayoutOf, LayoutOfHelpers, TyAndLayout}; @@ -21,7 +19,7 @@ use rustc_middle::ty::{ self, ConstInt, Instance, ParamEnv, ScalarInt, Ty, TyCtxt, TypeVisitableExt, }; use rustc_span::Span; -use rustc_target::abi::{self, Abi, HasDataLayout, Size, TargetDataLayout}; +use rustc_target::abi::{Abi, FieldIdx, HasDataLayout, Size, TargetDataLayout, VariantIdx}; use crate::const_prop::CanConstProp; use crate::const_prop::ConstPropMachine; @@ -29,11 +27,6 @@ use crate::const_prop::ConstPropMode; use crate::errors::AssertLint; use crate::MirLint; -/// The maximum number of bytes that we'll allocate space for a local or the return value. -/// Needed for #66397, because otherwise we eval into large places and that can cause OOM or just -/// Severely regress performance. -const MAX_ALLOC_LIMIT: u64 = 1024; - pub struct ConstPropLint; impl<'tcx> MirLint<'tcx> for ConstPropLint { @@ -86,6 +79,74 @@ struct ConstPropagator<'mir, 'tcx> { param_env: ParamEnv<'tcx>, worklist: Vec, visited_blocks: BitSet, + locals: IndexVec>, +} + +#[derive(Debug, Clone)] +enum Value<'tcx> { + Immediate(OpTy<'tcx>), + Aggregate { variant: VariantIdx, fields: IndexVec> }, + Uninit, +} + +impl<'tcx> From> for Value<'tcx> { + fn from(v: OpTy<'tcx>) -> Self { + Self::Immediate(v) + } +} + +impl<'tcx> From> for Value<'tcx> { + fn from(v: ImmTy<'tcx>) -> Self { + Self::Immediate(v.into()) + } +} + +impl<'tcx> Value<'tcx> { + fn project( + &self, + proj: impl Iterator>>>, + ) -> Option<&Value<'tcx>> { + let mut this = self; + for proj in proj { + this = match (proj?, this) { + (ProjectionElem::Field(idx, _), Value::Aggregate { fields, .. }) => { + fields.get(idx).unwrap_or(&Value::Uninit) + } + (ProjectionElem::Index(idx), Value::Aggregate { fields, .. }) => { + fields.get(idx).unwrap_or(&Value::Uninit) + } + _ => return None, + }; + } + Some(this) + } + + fn project_mut(&mut self, proj: &[PlaceElem<'_>]) -> Option<&mut Value<'tcx>> { + let mut this = self; + for proj in proj { + this = match (proj, this) { + (PlaceElem::Field(idx, _), Value::Aggregate { fields, .. }) => { + fields.ensure_contains_elem(*idx, || Value::Uninit) + } + (PlaceElem::Field(..), val @ Value::Uninit) => { + *val = Value::Aggregate { + variant: VariantIdx::new(0), + fields: Default::default(), + }; + val.project_mut(&[*proj])? + } + _ => return None, + }; + } + Some(this) + } + + fn immediate(&self) -> Option<&OpTy<'tcx>> { + match self { + Value::Immediate(op) => Some(op), + _ => None, + } + } } impl<'tcx> LayoutOfHelpers<'tcx> for ConstPropagator<'_, 'tcx> { @@ -132,21 +193,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { ConstPropMachine::new(can_const_prop), ); - let ret_layout = ecx - .layout_of(body.bound_return_ty().instantiate(tcx, args)) - .ok() - // Don't bother allocating memory for large values. - // I don't know how return types can seem to be unsized but this happens in the - // `type/type-unsatisfiable.rs` test. - .filter(|ret_layout| { - ret_layout.is_sized() && ret_layout.size < Size::from_bytes(MAX_ALLOC_LIMIT) - }) - .unwrap_or_else(|| ecx.layout_of(tcx.types.unit).unwrap()); - - let ret = ecx - .allocate(ret_layout, MemoryKind::Stack) - .expect("couldn't perform small allocation") - .into(); + let ret = MPlaceTy::fake_alloc_zst(ecx.layout_of(tcx.types.unit).unwrap()).into(); ecx.push_stack_frame( Instance::new(def_id, args), @@ -156,21 +203,13 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { ) .expect("failed to push initial stack frame"); - for local in body.local_decls.indices() { - // Mark everything initially live. - // This is somewhat dicey since some of them might be unsized and it is incoherent to - // mark those as live... We rely on `local_to_place`/`local_to_op` in the interpreter - // stopping us before those unsized immediates can cause issues deeper in the - // interpreter. - ecx.frame_mut().locals[local].make_live_uninit(); - } - ConstPropagator { ecx, tcx, param_env, worklist: vec![START_BLOCK], visited_blocks: BitSet::new_empty(body.basic_blocks.len()), + locals: IndexVec::from_elem_n(Value::Uninit, body.local_decls.len()), } } @@ -182,38 +221,27 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { &self.body().local_decls } - fn get_const(&self, place: Place<'tcx>) -> Option> { - let op = match self.ecx.eval_place_to_op(place, None) { - Ok(op) => { - if op - .as_mplace_or_imm() - .right() - .is_some_and(|imm| matches!(*imm, Immediate::Uninit)) - { - // Make sure nobody accidentally uses this value. - return None; - } - op - } - Err(e) => { - trace!("get_const failed: {:?}", e.into_kind().debug()); - return None; - } - }; - - // Try to read the local as an immediate so that if it is representable as a scalar, we can - // handle it as such, but otherwise, just return the value as is. - Some(match self.ecx.read_immediate_raw(&op) { - Ok(Left(imm)) => imm.into(), - _ => op, - }) + fn get_const(&self, place: Place<'tcx>) -> Option<&Value<'tcx>> { + self.locals[place.local] + .project(place.projection.iter().map(|proj| self.simple_projection(proj))) } /// Remove `local` from the pool of `Locals`. Allows writing to them, /// but not reading from them anymore. - fn remove_const(ecx: &mut InterpCx<'mir, 'tcx, ConstPropMachine<'mir, 'tcx>>, local: Local) { - ecx.frame_mut().locals[local].make_live_uninit(); - ecx.machine.written_only_inside_own_block_locals.remove(&local); + fn remove_const(&mut self, local: Local) { + self.locals[local] = Value::Uninit; + self.ecx.machine.written_only_inside_own_block_locals.remove(&local); + } + + fn access_mut(&mut self, place: &Place<'_>) -> Option<&mut Value<'tcx>> { + match self.ecx.machine.can_const_prop[place.local] { + ConstPropMode::NoPropagation => return None, + ConstPropMode::OnlyInsideOwnBlock => { + self.ecx.machine.written_only_inside_own_block_locals.insert(place.local); + } + ConstPropMode::FullConstProp => {} + } + self.locals[place.local].project_mut(place.projection) } fn lint_root(&self, source_info: SourceInfo) -> Option { @@ -267,14 +295,17 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { } /// Returns the value, if any, of evaluating `place`. + #[instrument(level = "trace", skip(self), ret)] fn eval_place( &mut self, place: Place<'tcx>, - location: Location, layout: Option>, ) -> Option> { - trace!("eval_place(place={:?})", place); - self.use_ecx(location, |this| this.ecx.eval_place_to_op(place, layout)) + match self.get_const(place)? { + Value::Immediate(op) => Some(op.clone()), + Value::Aggregate { .. } => None, + Value::Uninit => None, + } } /// Returns the value, if any, of evaluating `op`. Calls upon `eval_constant` @@ -287,7 +318,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { ) -> Option> { match *op { Operand::Constant(ref c) => self.eval_constant(c, location, layout), - Operand::Move(place) | Operand::Copy(place) => self.eval_place(place, location, layout), + Operand::Move(place) | Operand::Copy(place) => self.eval_place(place, layout), } } @@ -298,8 +329,9 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { } fn check_unary_op(&mut self, op: UnOp, arg: &Operand<'tcx>, location: Location) -> Option<()> { + let arg = self.eval_operand(arg, location, None)?; if let (val, true) = self.use_ecx(location, |this| { - let val = this.ecx.read_immediate(&this.ecx.eval_operand(arg, None)?)?; + let val = this.ecx.read_immediate(&arg)?; let (_res, overflow) = this.ecx.overflowing_unary_op(op, &val)?; Ok((val, overflow)) })? { @@ -327,11 +359,12 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { right: &Operand<'tcx>, location: Location, ) -> Option<()> { - let r = self.use_ecx(location, |this| { - this.ecx.read_immediate(&this.ecx.eval_operand(right, None)?) - }); + let r = self + .eval_operand(right, location, None) + .and_then(|r| self.use_ecx(location, |this| this.ecx.read_immediate(&r))); let l = self - .use_ecx(location, |this| this.ecx.read_immediate(&this.ecx.eval_operand(left, None)?)); + .eval_operand(left, location, None) + .and_then(|l| self.use_ecx(location, |this| this.ecx.read_immediate(&l))); // Check for exceeding shifts *even if* we cannot evaluate the LHS. if matches!(op, BinOp::Shr | BinOp::Shl) { let r = r.clone()?; @@ -426,7 +459,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { // 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); + self.remove_const(place.local); return None; } @@ -478,7 +511,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { // Poison all places this operand references so that further code // doesn't use the invalid value if let Some(place) = cond.place() { - Self::remove_const(&mut self.ecx, place.local); + self.remove_const(place.local); } enum DbgVal { @@ -530,13 +563,13 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { fn ensure_not_propagated(&self, local: Local) { if cfg!(debug_assertions) { + let val = self.get_const(local.into()); assert!( - self.get_const(local.into()).is_none() + matches!(val, Some(Value::Uninit)) || self .layout_of(self.local_decls()[local].ty) .map_or(true, |layout| layout.is_zst()), - "failed to remove values for `{local:?}`, value={:?}", - self.get_const(local.into()), + "failed to remove values for `{local:?}`, value={val:?}", ) } } @@ -552,19 +585,19 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { return None; } use rustc_middle::mir::Rvalue::*; - let dest = self.use_ecx(location, |this| this.ecx.eval_place(*dest))?; - trace!(?dest); + let layout = self.use_ecx(location, |this| this.ecx.eval_place(*dest))?.layout; + trace!(?layout); - let val = match *rvalue { + let val: Value<'_> = match *rvalue { ThreadLocalRef(_) => return None, - Use(ref operand) => self.eval_operand(operand, location, Some(dest.layout))?, + Use(ref operand) => self.eval_operand(operand, location, Some(layout))?.into(), - CopyForDeref(place) => self.eval_place(place, location, Some(dest.layout))?, + CopyForDeref(place) => self.eval_place(place, Some(layout))?.into(), BinaryOp(bin_op, box (ref left, ref right)) => { let layout = - rustc_const_eval::util::binop_left_homogeneous(bin_op).then_some(dest.layout); + rustc_const_eval::util::binop_left_homogeneous(bin_op).then_some(layout); let left = self.eval_operand(left, location, layout)?; let left = self.use_ecx(location, |this| this.ecx.read_immediate(&left))?; @@ -590,63 +623,37 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { let (val, overflowed) = self.use_ecx(location, |this| { this.ecx.overflowing_binary_op(bin_op, &left, &right) })?; - let tuple = Ty::new_tup_from_iter( - self.tcx, - [val.layout.ty, self.tcx.types.bool].into_iter(), - ); - let tuple = self.ecx.layout_of(tuple).ok()?; - let val = - ImmTy::from_scalar_pair(val.to_scalar(), Scalar::from_bool(overflowed), tuple); - val.into() + let overflowed = ImmTy::from_bool(overflowed, self.tcx); + Value::Aggregate { + variant: VariantIdx::new(0), + fields: [Value::from(val), overflowed.into()].into_iter().collect(), + } } UnaryOp(un_op, ref operand) => { - let operand = self.eval_operand(operand, location, Some(dest.layout))?; + let operand = self.eval_operand(operand, location, Some(layout))?; let val = self.use_ecx(location, |this| this.ecx.read_immediate(&operand))?; let val = self.use_ecx(location, |this| this.ecx.wrapping_unary_op(un_op, &val))?; val.into() } - Aggregate(ref kind, ref fields) => { - trace!(?kind); - trace!(?dest.layout); - if dest.layout.is_zst() { - ImmTy::uninit(dest.layout).into() - } else if let Abi::Scalar(abi::Scalar::Initialized { .. }) = dest.layout.abi { - let fields = fields - .iter() - .map(|field| self.eval_operand(field, location, None)) - .collect::>>()?; - trace!(?fields); - let mut field = - fields.into_iter().find(|field| field.layout.abi.is_scalar())?; - field.layout = dest.layout; - field - } else if let Abi::ScalarPair( - abi::Scalar::Initialized { .. }, - abi::Scalar::Initialized { .. }, - ) = dest.layout.abi - { - let fields = fields - .iter() - .map(|field| self.eval_operand(field, location, None)) - .collect::>>()?; - trace!(?fields); - let pair = - fields.iter().find(|field| matches!(field.layout.abi, Abi::ScalarPair(..))); - if let Some(pair) = pair { - let mut pair = pair.clone(); - pair.layout = dest.layout; - pair - } else { - // TODO: build a pair from two scalars - return None; - } - } else { - return None; - } - } + Aggregate(ref kind, ref fields) => Value::Aggregate { + fields: fields + .iter() + .map(|field| { + self.eval_operand(field, location, None) + .map_or(Value::Uninit, Value::Immediate) + }) + .collect(), + variant: match **kind { + AggregateKind::Adt(_, variant, _, _, _) => variant, + AggregateKind::Array(_) + | AggregateKind::Tuple + | AggregateKind::Closure(_, _) + | AggregateKind::Coroutine(_, _) => VariantIdx::new(0), + }, + }, Repeat(ref op, n) => { trace!(?op, ?n); @@ -654,23 +661,29 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { } Len(place) => { - let src = self.eval_place(place, location, None)?; - let len = src.len(&self.ecx).ok()?; - ImmTy::from_scalar(Scalar::from_target_usize(len, self), dest.layout).into() + let len = match self.get_const(place)? { + Value::Immediate(src) => src.len(&self.ecx).ok()?, + Value::Aggregate { fields, .. } => fields.len() as u64, + Value::Uninit => match place.ty(self.local_decls(), self.tcx).ty.kind() { + ty::Array(_, n) => n.try_eval_target_usize(self.tcx, self.param_env)?, + _ => return None, + }, + }; + ImmTy::from_scalar(Scalar::from_target_usize(len, self), layout).into() } Ref(..) | AddressOf(..) => return None, NullaryOp(ref null_op, ty) => { - let layout = self.use_ecx(location, |this| this.ecx.layout_of(ty))?; + let op_layout = self.use_ecx(location, |this| this.ecx.layout_of(ty))?; let val = match null_op { - NullOp::SizeOf => layout.size.bytes(), - NullOp::AlignOf => layout.align.abi.bytes(), + NullOp::SizeOf => op_layout.size.bytes(), + NullOp::AlignOf => op_layout.align.abi.bytes(), NullOp::OffsetOf(fields) => { - layout.offset_of_subfield(self, fields.iter()).bytes() + op_layout.offset_of_subfield(self, fields.iter()).bytes() } }; - ImmTy::from_scalar(Scalar::from_target_usize(val, self), dest.layout).into() + ImmTy::from_scalar(Scalar::from_target_usize(val, self), layout).into() } ShallowInitBox(..) => return None, @@ -702,26 +715,61 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { _ => return None, } } - value.offset(Size::ZERO, to, &self.ecx).ok()? + value.offset(Size::ZERO, to, &self.ecx).ok()?.into() } _ => return None, }, Discriminant(place) => { - let op = self.eval_place(place, location, None)?; - let variant = self.use_ecx(location, |this| this.ecx.read_discriminant(&op))?; + let variant = match self.get_const(place)? { + Value::Immediate(op) => { + let op = op.clone(); + self.use_ecx(location, |this| this.ecx.read_discriminant(&op))? + } + Value::Aggregate { variant, .. } => *variant, + Value::Uninit => return None, + }; let imm = self.use_ecx(location, |this| { - this.ecx.discriminant_for_variant(op.layout.ty, variant) + this.ecx.discriminant_for_variant( + place.ty(this.local_decls(), this.tcx).ty, + variant, + ) })?; imm.into() } }; trace!(?val); - self.use_ecx(location, |this| this.ecx.copy_op(&val, &dest, true))?; + *self.access_mut(dest)? = val; Some(()) } + + fn simple_projection( + &self, + proj: ProjectionElem>, + ) -> Option>> { + Some(match proj { + ProjectionElem::Deref => ProjectionElem::Deref, + ProjectionElem::Field(idx, ty) => ProjectionElem::Field(idx, ty), + ProjectionElem::Index(local) => { + let val = self.get_const(local.into())?; + let op = val.immediate()?; + ProjectionElem::Index(FieldIdx::from_u32( + self.ecx.read_target_usize(op).ok()?.try_into().ok()?, + )) + } + ProjectionElem::ConstantIndex { offset, min_length, from_end } => { + ProjectionElem::ConstantIndex { offset, min_length, from_end } + } + ProjectionElem::Subslice { from, to, from_end } => { + ProjectionElem::Subslice { from, to, from_end } + } + ProjectionElem::Downcast(a, b) => ProjectionElem::Downcast(a, b), + ProjectionElem::OpaqueCast(ty) => ProjectionElem::OpaqueCast(ty), + ProjectionElem::Subtype(ty) => ProjectionElem::Subtype(ty), + }) + } } impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { @@ -772,7 +820,7 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { Nuking the entire site from orbit, it's the only way to be sure", place, ); - Self::remove_const(&mut self.ecx, place.local); + self.remove_const(place.local); } } } @@ -786,28 +834,24 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { self.super_statement(statement, location); match statement.kind { - StatementKind::SetDiscriminant { ref place, .. } => { + StatementKind::SetDiscriminant { ref place, variant_index } => { match self.ecx.machine.can_const_prop[place.local] { // Do nothing if the place is indirect. _ if place.is_indirect() => {} ConstPropMode::NoPropagation => self.ensure_not_propagated(place.local), ConstPropMode::FullConstProp | ConstPropMode::OnlyInsideOwnBlock => { - if self.use_ecx(location, |this| this.ecx.statement(statement)).is_some() { - trace!("propped discriminant into {:?}", place); - } else { - Self::remove_const(&mut self.ecx, place.local); + match self.access_mut(place) { + Some(Value::Aggregate { variant, .. }) => *variant = variant_index, + _ => self.remove_const(place.local), } } } } StatementKind::StorageLive(local) => { - let frame = self.ecx.frame_mut(); - frame.locals[local].make_live_uninit(); + self.remove_const(local); } StatementKind::StorageDead(local) => { - let frame = self.ecx.frame_mut(); - // We don't actually track liveness, so the local remains live. But forget its value. - frame.locals[local].make_live_uninit(); + self.remove_const(local); } _ => {} } @@ -871,7 +915,7 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { self.ecx.machine.can_const_prop[local], ConstPropMode::OnlyInsideOwnBlock ); - Self::remove_const(&mut self.ecx, local); + self.remove_const(local); } self.ecx.machine.written_only_inside_own_block_locals = written_only_inside_own_block_locals; diff --git a/tests/ui/consts/const-err-late.stderr b/tests/ui/consts/const-err-late.stderr index 35c3d000117ef..53badeafa35b6 100644 --- a/tests/ui/consts/const-err-late.stderr +++ b/tests/ui/consts/const-err-late.stderr @@ -30,6 +30,14 @@ LL | black_box((S::::FOO, S::::FOO)); | = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no` +note: erroneous constant encountered + --> $DIR/const-err-late.rs:19:31 + | +LL | black_box((S::::FOO, S::::FOO)); + | ^^^^^^^^^^^^^ + | + = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no` + error: aborting due to 2 previous errors For more information about this error, try `rustc --explain E0080`. diff --git a/tests/ui/consts/issue-65348.rs b/tests/ui/consts/issue-65348.rs index 5eafa831d6317..01bf2a3fa4287 100644 --- a/tests/ui/consts/issue-65348.rs +++ b/tests/ui/consts/issue-65348.rs @@ -8,15 +8,16 @@ impl Generic { const ARRAY_FIELD: Generic<(i32, [T; 0])> = Generic((0, [])); } -pub const fn array() -> &'static T { +pub const fn array() -> &'static T { + #[allow(unconditional_panic)] &Generic::::ARRAY[0] } -pub const fn newtype_array() -> &'static T { +pub const fn newtype_array() -> &'static T { &Generic::::NEWTYPE_ARRAY.0[0] } -pub const fn array_field() -> &'static T { +pub const fn array_field() -> &'static T { &(Generic::::ARRAY_FIELD.0).1[0] } From 0294a0de09fc74a37d6e7106d430c64e02c8fa7c Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Fri, 5 Jan 2024 17:12:44 +0000 Subject: [PATCH 04/15] Remove location threading --- .../src/const_prop_lint.rs | 89 +++++++++---------- 1 file changed, 40 insertions(+), 49 deletions(-) diff --git a/compiler/rustc_mir_transform/src/const_prop_lint.rs b/compiler/rustc_mir_transform/src/const_prop_lint.rs index 4ddcabcc90f7e..a801d6ec01e78 100644 --- a/compiler/rustc_mir_transform/src/const_prop_lint.rs +++ b/compiler/rustc_mir_transform/src/const_prop_lint.rs @@ -3,8 +3,6 @@ use std::fmt::Debug; -use either::Left; - use rustc_const_eval::interpret::{ImmTy, MPlaceTy, Projectable}; use rustc_const_eval::interpret::{InterpCx, InterpResult, OpTy, Scalar, StackPopCleanup}; use rustc_hir::def::DefKind; @@ -248,12 +246,10 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { source_info.scope.lint_root(&self.body().source_scopes) } - fn use_ecx(&mut self, location: Location, f: F) -> Option + fn use_ecx(&mut self, f: F) -> Option where F: FnOnce(&mut Self) -> InterpResult<'tcx, T>, { - // Overwrite the PC -- whatever the interpreter does to it does not make any sense anyway. - self.ecx.frame_mut().loc = Left(location); match f(self) { Ok(val) => Some(val), Err(error) => { @@ -275,7 +271,6 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { fn eval_constant( &mut self, c: &ConstOperand<'tcx>, - location: Location, layout: Option>, ) -> Option> { // FIXME we need to revisit this for #67176 @@ -291,7 +286,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { // manually normalized. let val = self.tcx.try_normalize_erasing_regions(self.param_env, c.const_).ok()?; - self.use_ecx(location, |this| this.ecx.eval_mir_constant(&val, Some(c.span), layout)) + self.use_ecx(|this| this.ecx.eval_mir_constant(&val, Some(c.span), layout)) } /// Returns the value, if any, of evaluating `place`. @@ -313,11 +308,10 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { fn eval_operand( &mut self, op: &Operand<'tcx>, - location: Location, layout: Option>, ) -> Option> { match *op { - Operand::Constant(ref c) => self.eval_constant(c, location, layout), + Operand::Constant(ref c) => self.eval_constant(c, layout), Operand::Move(place) | Operand::Copy(place) => self.eval_place(place, layout), } } @@ -329,8 +323,8 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { } fn check_unary_op(&mut self, op: UnOp, arg: &Operand<'tcx>, location: Location) -> Option<()> { - let arg = self.eval_operand(arg, location, None)?; - if let (val, true) = self.use_ecx(location, |this| { + let arg = self.eval_operand(arg, None)?; + if let (val, true) = self.use_ecx(|this| { let val = this.ecx.read_immediate(&arg)?; let (_res, overflow) = this.ecx.overflowing_unary_op(op, &val)?; Ok((val, overflow)) @@ -360,11 +354,11 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { location: Location, ) -> Option<()> { let r = self - .eval_operand(right, location, None) - .and_then(|r| self.use_ecx(location, |this| this.ecx.read_immediate(&r))); + .eval_operand(right, None) + .and_then(|r| self.use_ecx(|this| this.ecx.read_immediate(&r))); let l = self - .eval_operand(left, location, None) - .and_then(|l| self.use_ecx(location, |this| this.ecx.read_immediate(&l))); + .eval_operand(left, None) + .and_then(|l| self.use_ecx(|this| this.ecx.read_immediate(&l))); // Check for exceeding shifts *even if* we cannot evaluate the LHS. if matches!(op, BinOp::Shr | BinOp::Shl) { let r = r.clone()?; @@ -400,7 +394,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { if let (Some(l), Some(r)) = (l, r) { // The remaining operators are handled through `overflowing_binary_op`. - if self.use_ecx(location, |this| { + if self.use_ecx(|this| { let (_res, overflow) = this.ecx.overflowing_binary_op(op, &l, &r)?; Ok(overflow) })? { @@ -501,11 +495,11 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { cond: &Operand<'tcx>, location: Location, ) -> Option { - let value = &self.eval_operand(cond, location, None)?; + let value = &self.eval_operand(cond, None)?; trace!("assertion on {:?} should be {:?}", value, expected); let expected = Scalar::from_bool(expected); - let value_const = self.use_ecx(location, |this| this.ecx.read_scalar(value))?; + let value_const = self.use_ecx(|this| this.ecx.read_scalar(value))?; if expected != value_const { // Poison all places this operand references so that further code @@ -529,7 +523,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { let mut eval_to_int = |op| { // This can be `None` if the lhs wasn't const propagated and we just // triggered the assert on the value of the rhs. - self.eval_operand(op, location, None) + self.eval_operand(op, None) .and_then(|op| self.ecx.read_immediate(&op).ok()) .map_or(DbgVal::Underscore, |op| DbgVal::Val(op.to_const_int())) }; @@ -585,44 +579,43 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { return None; } use rustc_middle::mir::Rvalue::*; - let layout = self.use_ecx(location, |this| this.ecx.eval_place(*dest))?.layout; + let layout = self.use_ecx(|this| this.ecx.eval_place(*dest))?.layout; trace!(?layout); let val: Value<'_> = match *rvalue { ThreadLocalRef(_) => return None, - Use(ref operand) => self.eval_operand(operand, location, Some(layout))?.into(), + Use(ref operand) => self.eval_operand(operand, Some(layout))?.into(), CopyForDeref(place) => self.eval_place(place, Some(layout))?.into(), BinaryOp(bin_op, box (ref left, ref right)) => { let layout = rustc_const_eval::util::binop_left_homogeneous(bin_op).then_some(layout); - let left = self.eval_operand(left, location, layout)?; - let left = self.use_ecx(location, |this| this.ecx.read_immediate(&left))?; + let left = self.eval_operand(left, layout)?; + let left = self.use_ecx(|this| this.ecx.read_immediate(&left))?; let layout = rustc_const_eval::util::binop_right_homogeneous(bin_op).then_some(left.layout); - let right = self.eval_operand(right, location, layout)?; - let right = self.use_ecx(location, |this| this.ecx.read_immediate(&right))?; + let right = self.eval_operand(right, layout)?; + let right = self.use_ecx(|this| this.ecx.read_immediate(&right))?; - let val = self - .use_ecx(location, |this| this.ecx.wrapping_binary_op(bin_op, &left, &right))?; + let val = + self.use_ecx(|this| this.ecx.wrapping_binary_op(bin_op, &left, &right))?; val.into() } CheckedBinaryOp(bin_op, box (ref left, ref right)) => { - let left = self.eval_operand(left, location, None)?; - let left = self.use_ecx(location, |this| this.ecx.read_immediate(&left))?; + let left = self.eval_operand(left, None)?; + let left = self.use_ecx(|this| this.ecx.read_immediate(&left))?; let layout = rustc_const_eval::util::binop_right_homogeneous(bin_op).then_some(left.layout); - let right = self.eval_operand(right, location, layout)?; - let right = self.use_ecx(location, |this| this.ecx.read_immediate(&right))?; + let right = self.eval_operand(right, layout)?; + let right = self.use_ecx(|this| this.ecx.read_immediate(&right))?; - let (val, overflowed) = self.use_ecx(location, |this| { - this.ecx.overflowing_binary_op(bin_op, &left, &right) - })?; + let (val, overflowed) = + self.use_ecx(|this| this.ecx.overflowing_binary_op(bin_op, &left, &right))?; let overflowed = ImmTy::from_bool(overflowed, self.tcx); Value::Aggregate { variant: VariantIdx::new(0), @@ -631,10 +624,10 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { } UnaryOp(un_op, ref operand) => { - let operand = self.eval_operand(operand, location, Some(layout))?; - let val = self.use_ecx(location, |this| this.ecx.read_immediate(&operand))?; + let operand = self.eval_operand(operand, Some(layout))?; + let val = self.use_ecx(|this| this.ecx.read_immediate(&operand))?; - let val = self.use_ecx(location, |this| this.ecx.wrapping_unary_op(un_op, &val))?; + let val = self.use_ecx(|this| this.ecx.wrapping_unary_op(un_op, &val))?; val.into() } @@ -642,8 +635,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { fields: fields .iter() .map(|field| { - self.eval_operand(field, location, None) - .map_or(Value::Uninit, Value::Immediate) + self.eval_operand(field, None).map_or(Value::Uninit, Value::Immediate) }) .collect(), variant: match **kind { @@ -675,7 +667,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { Ref(..) | AddressOf(..) => return None, NullaryOp(ref null_op, ty) => { - let op_layout = self.use_ecx(location, |this| this.ecx.layout_of(ty))?; + let op_layout = self.use_ecx(|this| this.ecx.layout_of(ty))?; let val = match null_op { NullOp::SizeOf => op_layout.size.bytes(), NullOp::AlignOf => op_layout.align.abi.bytes(), @@ -690,21 +682,21 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { Cast(ref kind, ref value, to) => match kind { CastKind::IntToInt | CastKind::IntToFloat => { - let value = self.eval_operand(value, location, None)?; + let value = self.eval_operand(value, None)?; let value = self.ecx.read_immediate(&value).ok()?; let to = self.ecx.layout_of(to).ok()?; let res = self.ecx.int_to_int_or_float(&value, to).ok()?; res.into() } CastKind::FloatToFloat | CastKind::FloatToInt => { - let value = self.eval_operand(value, location, None)?; + let value = self.eval_operand(value, None)?; let value = self.ecx.read_immediate(&value).ok()?; let to = self.ecx.layout_of(to).ok()?; let res = self.ecx.float_to_float_or_int(&value, to).ok()?; res.into() } CastKind::Transmute => { - let value = self.eval_operand(value, location, None)?; + let value = self.eval_operand(value, None)?; let to = self.ecx.layout_of(to).ok()?; // `offset` for immediates only supports scalar/scalar-pair ABIs, // so bail out if the target is not one. @@ -724,12 +716,12 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { let variant = match self.get_const(place)? { Value::Immediate(op) => { let op = op.clone(); - self.use_ecx(location, |this| this.ecx.read_discriminant(&op))? + self.use_ecx(|this| this.ecx.read_discriminant(&op))? } Value::Aggregate { variant, .. } => *variant, Value::Uninit => return None, }; - let imm = self.use_ecx(location, |this| { + let imm = self.use_ecx(|this| { this.ecx.discriminant_for_variant( place.ty(this.local_decls(), this.tcx).ty, variant, @@ -791,7 +783,7 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { fn visit_constant(&mut self, constant: &ConstOperand<'tcx>, location: Location) { trace!("visit_constant: {:?}", constant); self.super_constant(constant, location); - self.eval_constant(constant, location, None); + self.eval_constant(constant, None); } fn visit_assign(&mut self, place: &Place<'tcx>, rvalue: &Rvalue<'tcx>, location: Location) { @@ -864,9 +856,8 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { self.check_assertion(*expected, msg, cond, location); } TerminatorKind::SwitchInt { ref discr, ref targets } => { - if let Some(ref value) = self.eval_operand(discr, location, None) - && let Some(value_const) = - self.use_ecx(location, |this| this.ecx.read_scalar(value)) + if let Some(ref value) = self.eval_operand(discr, None) + && let Some(value_const) = self.use_ecx(|this| this.ecx.read_scalar(value)) && let Ok(constant) = value_const.try_to_int() && let Ok(constant) = constant.to_bits(constant.size()) { From 89e6a67310a6d733895fb47937293c34bd464266 Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Fri, 5 Jan 2024 17:19:18 +0000 Subject: [PATCH 05/15] Const prop doesn't need a stack anymore --- .../rustc_mir_transform/src/const_prop.rs | 22 ++++------ .../src/const_prop_lint.rs | 44 ++++++------------- 2 files changed, 22 insertions(+), 44 deletions(-) diff --git a/compiler/rustc_mir_transform/src/const_prop.rs b/compiler/rustc_mir_transform/src/const_prop.rs index c5824c3077028..a517997c1eb91 100644 --- a/compiler/rustc_mir_transform/src/const_prop.rs +++ b/compiler/rustc_mir_transform/src/const_prop.rs @@ -49,24 +49,18 @@ pub(crate) macro throw_machine_stop_str($($tt:tt)*) {{ throw_machine_stop!(Zst) }} -pub(crate) struct ConstPropMachine<'mir, 'tcx> { - /// The virtual call stack. - stack: Vec>, +pub(crate) struct ConstPropMachine { pub written_only_inside_own_block_locals: FxHashSet, pub can_const_prop: IndexVec, } -impl ConstPropMachine<'_, '_> { +impl ConstPropMachine { pub fn new(can_const_prop: IndexVec) -> Self { - Self { - stack: Vec::new(), - written_only_inside_own_block_locals: Default::default(), - can_const_prop, - } + Self { written_only_inside_own_block_locals: Default::default(), can_const_prop } } } -impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine<'mir, 'tcx> { +impl<'mir, 'tcx: 'mir> interpret::Machine<'mir, 'tcx> for ConstPropMachine { compile_time_machine!(<'mir, 'tcx>); const PANIC_ON_ALLOC_FAIL: bool = true; // all allocations are small (see `MAX_ALLOC_LIMIT`) @@ -192,16 +186,16 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine<'mir, 'tcx> #[inline(always)] fn stack<'a>( - ecx: &'a InterpCx<'mir, 'tcx, Self>, + _ecx: &'a InterpCx<'mir, 'tcx, Self>, ) -> &'a [Frame<'mir, 'tcx, Self::Provenance, Self::FrameExtra>] { - &ecx.machine.stack + &[] } #[inline(always)] fn stack_mut<'a>( - ecx: &'a mut InterpCx<'mir, 'tcx, Self>, + _ecx: &'a mut InterpCx<'mir, 'tcx, Self>, ) -> &'a mut Vec> { - &mut ecx.machine.stack + unreachable!() } } diff --git a/compiler/rustc_mir_transform/src/const_prop_lint.rs b/compiler/rustc_mir_transform/src/const_prop_lint.rs index a801d6ec01e78..c1f2447784d97 100644 --- a/compiler/rustc_mir_transform/src/const_prop_lint.rs +++ b/compiler/rustc_mir_transform/src/const_prop_lint.rs @@ -3,8 +3,8 @@ use std::fmt::Debug; -use rustc_const_eval::interpret::{ImmTy, MPlaceTy, Projectable}; -use rustc_const_eval::interpret::{InterpCx, InterpResult, OpTy, Scalar, StackPopCleanup}; +use rustc_const_eval::interpret::{ImmTy, Projectable}; +use rustc_const_eval::interpret::{InterpCx, InterpResult, OpTy, Scalar}; use rustc_hir::def::DefKind; use rustc_hir::HirId; use rustc_index::bit_set::BitSet; @@ -12,10 +12,7 @@ use rustc_index::{Idx, IndexVec}; use rustc_middle::mir::visit::Visitor; use rustc_middle::mir::*; use rustc_middle::ty::layout::{LayoutError, LayoutOf, LayoutOfHelpers, TyAndLayout}; -use rustc_middle::ty::GenericArgs; -use rustc_middle::ty::{ - self, ConstInt, Instance, ParamEnv, ScalarInt, Ty, TyCtxt, TypeVisitableExt, -}; +use rustc_middle::ty::{self, ConstInt, ParamEnv, ScalarInt, Ty, TyCtxt, TypeVisitableExt}; use rustc_span::Span; use rustc_target::abi::{Abi, FieldIdx, HasDataLayout, Size, TargetDataLayout, VariantIdx}; @@ -72,12 +69,13 @@ impl<'tcx> MirLint<'tcx> for ConstPropLint { /// Finds optimization opportunities on the MIR. struct ConstPropagator<'mir, 'tcx> { - ecx: InterpCx<'mir, 'tcx, ConstPropMachine<'mir, 'tcx>>, + ecx: InterpCx<'mir, 'tcx, ConstPropMachine>, tcx: TyCtxt<'tcx>, param_env: ParamEnv<'tcx>, worklist: Vec, visited_blocks: BitSet, locals: IndexVec>, + body: &'mir Body<'tcx>, } #[derive(Debug, Clone)] @@ -180,27 +178,16 @@ impl<'tcx> ty::layout::HasParamEnv<'tcx> for ConstPropagator<'_, 'tcx> { impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { fn new(body: &'mir Body<'tcx>, tcx: TyCtxt<'tcx>) -> ConstPropagator<'mir, 'tcx> { let def_id = body.source.def_id(); - let args = &GenericArgs::identity_for_item(tcx, def_id); let param_env = tcx.param_env_reveal_all_normalized(def_id); let can_const_prop = CanConstProp::check(tcx, param_env, body); - let mut ecx = InterpCx::new( + let ecx = InterpCx::new( tcx, tcx.def_span(def_id), param_env, ConstPropMachine::new(can_const_prop), ); - let ret = MPlaceTy::fake_alloc_zst(ecx.layout_of(tcx.types.unit).unwrap()).into(); - - ecx.push_stack_frame( - Instance::new(def_id, args), - body, - &ret, - StackPopCleanup::Root { cleanup: false }, - ) - .expect("failed to push initial stack frame"); - ConstPropagator { ecx, tcx, @@ -208,15 +195,12 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { worklist: vec![START_BLOCK], visited_blocks: BitSet::new_empty(body.basic_blocks.len()), locals: IndexVec::from_elem_n(Value::Uninit, body.local_decls.len()), + body, } } - fn body(&self) -> &'mir Body<'tcx> { - self.ecx.frame().body - } - fn local_decls(&self) -> &'mir LocalDecls<'tcx> { - &self.body().local_decls + &self.body.local_decls } fn get_const(&self, place: Place<'tcx>) -> Option<&Value<'tcx>> { @@ -243,7 +227,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { } fn lint_root(&self, source_info: SourceInfo) -> Option { - source_info.scope.lint_root(&self.body().source_scopes) + source_info.scope.lint_root(&self.body.source_scopes) } fn use_ecx(&mut self, f: F) -> Option @@ -332,7 +316,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { // `AssertKind` only has an `OverflowNeg` variant, so make sure that is // appropriate to use. assert_eq!(op, UnOp::Neg, "Neg is the only UnOp that can overflow"); - let source_info = self.body().source_info(location); + let source_info = self.body.source_info(location); self.report_assert_as_lint( source_info, AssertLint::ArithmeticOverflow( @@ -370,7 +354,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { let r_bits = r.to_scalar().to_bits(right_size).ok(); if r_bits.is_some_and(|b| b >= left_size.bits() as u128) { debug!("check_binary_op: reporting assert for {:?}", location); - let source_info = self.body().source_info(location); + let source_info = self.body.source_info(location); let panic = AssertKind::Overflow( op, match l { @@ -398,7 +382,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { let (_res, overflow) = this.ecx.overflowing_binary_op(op, &l, &r)?; Ok(overflow) })? { - let source_info = self.body().source_info(location); + let source_info = self.body.source_info(location); self.report_assert_as_lint( source_info, AssertLint::ArithmeticOverflow( @@ -545,7 +529,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { // Need proper const propagator for these. _ => return None, }; - let source_info = self.body().source_info(location); + let source_info = self.body.source_info(location); self.report_assert_as_lint( source_info, AssertLint::UnconditionalPanic(source_info.span, msg), @@ -579,7 +563,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { return None; } use rustc_middle::mir::Rvalue::*; - let layout = self.use_ecx(|this| this.ecx.eval_place(*dest))?.layout; + let layout = self.ecx.layout_of(dest.ty(self.body, self.tcx).ty).ok()?; trace!(?layout); let val: Value<'_> = match *rvalue { From 6ecb2aa58029f922acda8011737db1647643a975 Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Fri, 5 Jan 2024 17:26:59 +0000 Subject: [PATCH 06/15] We're not really using the `ConstPropMachine` anymore --- .../rustc_mir_transform/src/const_prop.rs | 32 +++-------------- .../src/const_prop_lint.rs | 34 ++++++++----------- 2 files changed, 20 insertions(+), 46 deletions(-) diff --git a/compiler/rustc_mir_transform/src/const_prop.rs b/compiler/rustc_mir_transform/src/const_prop.rs index a517997c1eb91..69894b6f07b61 100644 --- a/compiler/rustc_mir_transform/src/const_prop.rs +++ b/compiler/rustc_mir_transform/src/const_prop.rs @@ -5,7 +5,6 @@ use rustc_const_eval::interpret::{ self, compile_time_machine, AllocId, ConstAllocation, FnArg, Frame, ImmTy, InterpCx, InterpResult, OpTy, PlaceTy, Pointer, }; -use rustc_data_structures::fx::FxHashSet; use rustc_index::bit_set::BitSet; use rustc_index::IndexVec; use rustc_middle::mir::visit::{MutatingUseContext, NonMutatingUseContext, PlaceContext, Visitor}; @@ -49,16 +48,7 @@ pub(crate) macro throw_machine_stop_str($($tt:tt)*) {{ throw_machine_stop!(Zst) }} -pub(crate) struct ConstPropMachine { - pub written_only_inside_own_block_locals: FxHashSet, - pub can_const_prop: IndexVec, -} - -impl ConstPropMachine { - pub fn new(can_const_prop: IndexVec) -> Self { - Self { written_only_inside_own_block_locals: Default::default(), can_const_prop } - } -} +pub(crate) struct ConstPropMachine; impl<'mir, 'tcx: 'mir> interpret::Machine<'mir, 'tcx> for ConstPropMachine { compile_time_machine!(<'mir, 'tcx>); @@ -132,23 +122,11 @@ impl<'mir, 'tcx: 'mir> interpret::Machine<'mir, 'tcx> for ConstPropMachine { } fn before_access_local_mut<'a>( - ecx: &'a mut InterpCx<'mir, 'tcx, Self>, - frame: usize, - local: Local, + _ecx: &'a mut InterpCx<'mir, 'tcx, Self>, + _frame: usize, + _local: Local, ) -> InterpResult<'tcx> { - assert_eq!(frame, 0); - match ecx.machine.can_const_prop[local] { - ConstPropMode::NoPropagation => { - throw_machine_stop_str!( - "tried to write to a local that is marked as not propagatable" - ) - } - ConstPropMode::OnlyInsideOwnBlock => { - ecx.machine.written_only_inside_own_block_locals.insert(local); - } - ConstPropMode::FullConstProp => {} - } - Ok(()) + unreachable!() } fn before_access_global( diff --git a/compiler/rustc_mir_transform/src/const_prop_lint.rs b/compiler/rustc_mir_transform/src/const_prop_lint.rs index c1f2447784d97..f2612987c6a2e 100644 --- a/compiler/rustc_mir_transform/src/const_prop_lint.rs +++ b/compiler/rustc_mir_transform/src/const_prop_lint.rs @@ -5,6 +5,7 @@ use std::fmt::Debug; use rustc_const_eval::interpret::{ImmTy, Projectable}; use rustc_const_eval::interpret::{InterpCx, InterpResult, OpTy, Scalar}; +use rustc_data_structures::fx::FxHashSet; use rustc_hir::def::DefKind; use rustc_hir::HirId; use rustc_index::bit_set::BitSet; @@ -76,6 +77,8 @@ struct ConstPropagator<'mir, 'tcx> { visited_blocks: BitSet, locals: IndexVec>, body: &'mir Body<'tcx>, + written_only_inside_own_block_locals: FxHashSet, + can_const_prop: IndexVec, } #[derive(Debug, Clone)] @@ -181,12 +184,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { let param_env = tcx.param_env_reveal_all_normalized(def_id); let can_const_prop = CanConstProp::check(tcx, param_env, body); - let ecx = InterpCx::new( - tcx, - tcx.def_span(def_id), - param_env, - ConstPropMachine::new(can_const_prop), - ); + let ecx = InterpCx::new(tcx, tcx.def_span(def_id), param_env, ConstPropMachine); ConstPropagator { ecx, @@ -196,6 +194,8 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { visited_blocks: BitSet::new_empty(body.basic_blocks.len()), locals: IndexVec::from_elem_n(Value::Uninit, body.local_decls.len()), body, + can_const_prop, + written_only_inside_own_block_locals: Default::default(), } } @@ -212,14 +212,14 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { /// but not reading from them anymore. fn remove_const(&mut self, local: Local) { self.locals[local] = Value::Uninit; - self.ecx.machine.written_only_inside_own_block_locals.remove(&local); + self.written_only_inside_own_block_locals.remove(&local); } fn access_mut(&mut self, place: &Place<'_>) -> Option<&mut Value<'tcx>> { - match self.ecx.machine.can_const_prop[place.local] { + match self.can_const_prop[place.local] { ConstPropMode::NoPropagation => return None, ConstPropMode::OnlyInsideOwnBlock => { - self.ecx.machine.written_only_inside_own_block_locals.insert(place.local); + self.written_only_inside_own_block_locals.insert(place.local); } ConstPropMode::FullConstProp => {} } @@ -775,7 +775,7 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { let Some(()) = self.check_rvalue(rvalue, location) else { return }; - match self.ecx.machine.can_const_prop[place.local] { + match self.can_const_prop[place.local] { // Do nothing if the place is indirect. _ if place.is_indirect() => {} ConstPropMode::NoPropagation => self.ensure_not_propagated(place.local), @@ -811,7 +811,7 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { match statement.kind { StatementKind::SetDiscriminant { ref place, variant_index } => { - match self.ecx.machine.can_const_prop[place.local] { + match self.can_const_prop[place.local] { // Do nothing if the place is indirect. _ if place.is_indirect() => {} ConstPropMode::NoPropagation => self.ensure_not_propagated(place.local), @@ -878,7 +878,7 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { // which were modified in the current block. // Take it out of the ecx so we can get a mutable reference to the ecx for `remove_const`. let mut written_only_inside_own_block_locals = - std::mem::take(&mut self.ecx.machine.written_only_inside_own_block_locals); + std::mem::take(&mut self.written_only_inside_own_block_locals); // This loop can get very hot for some bodies: it check each local in each bb. // To avoid this quadratic behaviour, we only clear the locals that were modified inside @@ -886,17 +886,13 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { // The order in which we remove consts does not matter. #[allow(rustc::potential_query_instability)] for local in written_only_inside_own_block_locals.drain() { - debug_assert_eq!( - self.ecx.machine.can_const_prop[local], - ConstPropMode::OnlyInsideOwnBlock - ); + debug_assert_eq!(self.can_const_prop[local], ConstPropMode::OnlyInsideOwnBlock); self.remove_const(local); } - self.ecx.machine.written_only_inside_own_block_locals = - written_only_inside_own_block_locals; + self.written_only_inside_own_block_locals = written_only_inside_own_block_locals; if cfg!(debug_assertions) { - for (local, &mode) in self.ecx.machine.can_const_prop.iter_enumerated() { + for (local, &mode) in self.can_const_prop.iter_enumerated() { match mode { ConstPropMode::FullConstProp => {} ConstPropMode::NoPropagation | ConstPropMode::OnlyInsideOwnBlock => { From 1f398abcb6e11ea0dfe8e647a87fa2763005f89a Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Fri, 5 Jan 2024 17:31:38 +0000 Subject: [PATCH 07/15] const prop nonsense eliminated --- compiler/rustc_const_eval/src/errors.rs | 7 +-- .../rustc_const_eval/src/interpret/operand.rs | 6 +-- .../rustc_const_eval/src/interpret/place.rs | 19 ++----- .../src/interpret/projection.rs | 49 ++++++++----------- .../rustc_middle/src/mir/interpret/error.rs | 2 - .../src/dataflow_const_prop.rs | 7 ++- 6 files changed, 32 insertions(+), 58 deletions(-) diff --git a/compiler/rustc_const_eval/src/errors.rs b/compiler/rustc_const_eval/src/errors.rs index 171cc89d6adb8..75e21c3217e5e 100644 --- a/compiler/rustc_const_eval/src/errors.rs +++ b/compiler/rustc_const_eval/src/errors.rs @@ -860,9 +860,6 @@ impl<'tcx> ReportErrorExt for InvalidProgramInfo<'tcx> { InvalidProgramInfo::FnAbiAdjustForForeignAbi(_) => { rustc_middle::error::middle_adjust_for_foreign_abi_error } - InvalidProgramInfo::ConstPropNonsense => { - panic!("We had const-prop nonsense, this should never be printed") - } } } fn add_args( @@ -871,9 +868,7 @@ impl<'tcx> ReportErrorExt for InvalidProgramInfo<'tcx> { builder: &mut DiagnosticBuilder<'_, G>, ) { match self { - InvalidProgramInfo::TooGeneric - | InvalidProgramInfo::AlreadyReported(_) - | InvalidProgramInfo::ConstPropNonsense => {} + InvalidProgramInfo::TooGeneric | InvalidProgramInfo::AlreadyReported(_) => {} InvalidProgramInfo::Layout(e) => { // The level doesn't matter, `diag` is consumed without it being used. let dummy_level = Level::Bug; diff --git a/compiler/rustc_const_eval/src/interpret/operand.rs b/compiler/rustc_const_eval/src/interpret/operand.rs index b39b219b46a14..80d4bda4827a9 100644 --- a/compiler/rustc_const_eval/src/interpret/operand.rs +++ b/compiler/rustc_const_eval/src/interpret/operand.rs @@ -643,11 +643,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { let layout = self.layout_of_local(frame, local, layout)?; let op = *frame.locals[local].access()?; if matches!(op, Operand::Immediate(_)) { - if layout.is_unsized() { - // ConstProp marks *all* locals as `Immediate::Uninit` since it cannot - // efficiently check whether they are sized. We have to catch that case here. - throw_inval!(ConstPropNonsense); - } + assert!(!layout.is_unsized()); } Ok(OpTy { op, layout }) } diff --git a/compiler/rustc_const_eval/src/interpret/place.rs b/compiler/rustc_const_eval/src/interpret/place.rs index 639b269ac257a..041a8094fe843 100644 --- a/compiler/rustc_const_eval/src/interpret/place.rs +++ b/compiler/rustc_const_eval/src/interpret/place.rs @@ -519,11 +519,7 @@ where } else { // Unsized `Local` isn't okay (we cannot store the metadata). match frame_ref.locals[local].access()? { - Operand::Immediate(_) => { - // ConstProp marks *all* locals as `Immediate::Uninit` since it cannot - // efficiently check whether they are sized. We have to catch that case here. - throw_inval!(ConstPropNonsense); - } + Operand::Immediate(_) => bug!(), Operand::Indirect(mplace) => Place::Ptr(*mplace), } }; @@ -816,17 +812,8 @@ where // avoid force_allocation. let src = match self.read_immediate_raw(src)? { Right(src_val) => { - // FIXME(const_prop): Const-prop can possibly evaluate an - // unsized copy operation when it thinks that the type is - // actually sized, due to a trivially false where-clause - // predicate like `where Self: Sized` with `Self = dyn Trait`. - // See #102553 for an example of such a predicate. - if src.layout().is_unsized() { - throw_inval!(ConstPropNonsense); - } - if dest.layout().is_unsized() { - throw_inval!(ConstPropNonsense); - } + assert!(!src.layout().is_unsized()); + assert!(!dest.layout().is_unsized()); assert_eq!(src.layout().size, dest.layout().size); // Yay, we got a value that we can write directly. return if layout_compat { diff --git a/compiler/rustc_const_eval/src/interpret/projection.rs b/compiler/rustc_const_eval/src/interpret/projection.rs index 9a034ba22b989..bd60e066f7257 100644 --- a/compiler/rustc_const_eval/src/interpret/projection.rs +++ b/compiler/rustc_const_eval/src/interpret/projection.rs @@ -153,11 +153,7 @@ where // Offset may need adjustment for unsized fields. let (meta, offset) = if field_layout.is_unsized() { - if base.layout().is_sized() { - // An unsized field of a sized type? Sure... - // But const-prop actually feeds us such nonsense MIR! (see test `const_prop/issue-86351.rs`) - throw_inval!(ConstPropNonsense); - } + assert!(!base.layout().is_sized()); let base_meta = base.meta(); // Re-use parent metadata to determine dynamic field layout. // With custom DSTS, this *will* execute user-defined code, but the same @@ -205,29 +201,26 @@ where // see https://github.com/rust-lang/rust/issues/93688#issuecomment-1032929496.) // So we just "offset" by 0. let layout = base.layout().for_variant(self, variant); - if layout.abi.is_uninhabited() { - // `read_discriminant` should have excluded uninhabited variants... but ConstProp calls - // us on dead code. - // In the future we might want to allow this to permit code like this: - // (this is a Rust/MIR pseudocode mix) - // ``` - // enum Option2 { - // Some(i32, !), - // None, - // } - // - // fn panic() -> ! { panic!() } - // - // let x: Option2; - // x.Some.0 = 42; - // x.Some.1 = panic(); - // SetDiscriminant(x, Some); - // ``` - // However, for now we don't generate such MIR, and this check here *has* found real - // bugs (see https://github.com/rust-lang/rust/issues/115145), so we will keep rejecting - // it. - throw_inval!(ConstPropNonsense) - } + // In the future we might want to allow this to permit code like this: + // (this is a Rust/MIR pseudocode mix) + // ``` + // enum Option2 { + // Some(i32, !), + // None, + // } + // + // fn panic() -> ! { panic!() } + // + // let x: Option2; + // x.Some.0 = 42; + // x.Some.1 = panic(); + // SetDiscriminant(x, Some); + // ``` + // However, for now we don't generate such MIR, and this check here *has* found real + // bugs (see https://github.com/rust-lang/rust/issues/115145), so we will keep rejecting + // it. + assert!(!layout.abi.is_uninhabited()); + // This cannot be `transmute` as variants *can* have a smaller size than the entire enum. base.offset(Size::ZERO, layout, self) } diff --git a/compiler/rustc_middle/src/mir/interpret/error.rs b/compiler/rustc_middle/src/mir/interpret/error.rs index 1b4e9c286351a..474e156a131a3 100644 --- a/compiler/rustc_middle/src/mir/interpret/error.rs +++ b/compiler/rustc_middle/src/mir/interpret/error.rs @@ -200,8 +200,6 @@ pub enum InvalidProgramInfo<'tcx> { /// (which unfortunately typeck does not reject). /// Not using `FnAbiError` as that contains a nested `LayoutError`. FnAbiAdjustForForeignAbi(call::AdjustForForeignAbiError), - /// We are runnning into a nonsense situation due to ConstProp violating our invariants. - ConstPropNonsense, } /// Details of why a pointer had to be in-bounds. diff --git a/compiler/rustc_mir_transform/src/dataflow_const_prop.rs b/compiler/rustc_mir_transform/src/dataflow_const_prop.rs index ad12bce9b0232..d5f22b2cdbc42 100644 --- a/compiler/rustc_mir_transform/src/dataflow_const_prop.rs +++ b/compiler/rustc_mir_transform/src/dataflow_const_prop.rs @@ -403,7 +403,12 @@ impl<'a, 'tcx> ConstAnalysis<'a, 'tcx> { operand, &mut |elem, op| match elem { TrackElem::Field(idx) => self.ecx.project_field(op, idx.as_usize()).ok(), - TrackElem::Variant(idx) => self.ecx.project_downcast(op, idx).ok(), + TrackElem::Variant(idx) => { + if op.layout.for_variant(&self.ecx, idx).abi.is_uninhabited() { + return None; + } + self.ecx.project_downcast(op, idx).ok() + } TrackElem::Discriminant => { let variant = self.ecx.read_discriminant(op).ok()?; let discr_value = From 249ec9f08b670218734b451d80b0068e76133bd2 Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Mon, 8 Jan 2024 09:41:48 +0000 Subject: [PATCH 08/15] We don't look into static items anymore during const prop --- src/tools/clippy/tests/ui/modulo_one.rs | 3 +-- src/tools/clippy/tests/ui/modulo_one.stderr | 8 +------- 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/src/tools/clippy/tests/ui/modulo_one.rs b/src/tools/clippy/tests/ui/modulo_one.rs index c1dbe9d9a8787..c332a15f15778 100644 --- a/src/tools/clippy/tests/ui/modulo_one.rs +++ b/src/tools/clippy/tests/ui/modulo_one.rs @@ -33,7 +33,6 @@ fn main() { INT_MIN % NEG_ONE; //~^ ERROR: this operation will panic at runtime //~| ERROR: any number modulo -1 will panic/overflow or result in 0 - // ONLY caught by rustc + // Not caught by lint, we don't look into static items, even if entirely immutable. INT_MIN % STATIC_NEG_ONE; - //~^ ERROR: this operation will panic at runtime } diff --git a/src/tools/clippy/tests/ui/modulo_one.stderr b/src/tools/clippy/tests/ui/modulo_one.stderr index cc211ab6cd345..06bbb0f5d9a80 100644 --- a/src/tools/clippy/tests/ui/modulo_one.stderr +++ b/src/tools/clippy/tests/ui/modulo_one.stderr @@ -12,12 +12,6 @@ error: this operation will panic at runtime LL | INT_MIN % NEG_ONE; | ^^^^^^^^^^^^^^^^^ attempt to compute `i64::MIN % -1_i64`, which would overflow -error: this operation will panic at runtime - --> $DIR/modulo_one.rs:37:5 - | -LL | INT_MIN % STATIC_NEG_ONE; - | ^^^^^^^^^^^^^^^^^^^^^^^^ attempt to compute `i64::MIN % -1_i64`, which would overflow - error: any number modulo 1 will be 0 --> $DIR/modulo_one.rs:8:5 | @@ -57,5 +51,5 @@ error: any number modulo -1 will panic/overflow or result in 0 LL | INT_MIN % NEG_ONE; | ^^^^^^^^^^^^^^^^^ -error: aborting due to 9 previous errors +error: aborting due to 8 previous errors From 3419273f1ff55c02b6c001bcebca9f93a69e6d58 Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Mon, 15 Jan 2024 13:18:50 +0000 Subject: [PATCH 09/15] Avoid some packing/unpacking of the AssertLint enum --- .../src/const_prop_lint.rs | 48 +++++++++---------- compiler/rustc_mir_transform/src/errors.rs | 42 +++++++--------- 2 files changed, 41 insertions(+), 49 deletions(-) diff --git a/compiler/rustc_mir_transform/src/const_prop_lint.rs b/compiler/rustc_mir_transform/src/const_prop_lint.rs index f2612987c6a2e..b26cba38fe2b0 100644 --- a/compiler/rustc_mir_transform/src/const_prop_lint.rs +++ b/compiler/rustc_mir_transform/src/const_prop_lint.rs @@ -20,7 +20,7 @@ use rustc_target::abi::{Abi, FieldIdx, HasDataLayout, Size, TargetDataLayout, Va use crate::const_prop::CanConstProp; use crate::const_prop::ConstPropMachine; use crate::const_prop::ConstPropMode; -use crate::errors::AssertLint; +use crate::errors::{AssertLint, AssertLintKind}; use crate::MirLint; pub struct ConstPropLint; @@ -300,9 +300,21 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { } } - fn report_assert_as_lint(&self, source_info: &SourceInfo, lint: AssertLint) { + fn report_assert_as_lint( + &self, + location: Location, + lint_kind: AssertLintKind, + assert_kind: AssertKind, + ) { + let source_info = self.body.source_info(location); if let Some(lint_root) = self.lint_root(*source_info) { - self.tcx.emit_node_span_lint(lint.lint(), lint_root, source_info.span, lint); + let span = source_info.span; + self.tcx.emit_node_span_lint( + lint_kind.lint(), + lint_root, + span, + AssertLint { span, assert_kind, lint_kind }, + ); } } @@ -316,13 +328,10 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { // `AssertKind` only has an `OverflowNeg` variant, so make sure that is // appropriate to use. assert_eq!(op, UnOp::Neg, "Neg is the only UnOp that can overflow"); - let source_info = self.body.source_info(location); self.report_assert_as_lint( - source_info, - AssertLint::ArithmeticOverflow( - source_info.span, - AssertKind::OverflowNeg(val.to_const_int()), - ), + location, + AssertLintKind::ArithmeticOverflow, + AssertKind::OverflowNeg(val.to_const_int()), ); return None; } @@ -354,7 +363,6 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { let r_bits = r.to_scalar().to_bits(right_size).ok(); if r_bits.is_some_and(|b| b >= left_size.bits() as u128) { debug!("check_binary_op: reporting assert for {:?}", location); - let source_info = self.body.source_info(location); let panic = AssertKind::Overflow( op, match l { @@ -368,10 +376,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { }, r.to_const_int(), ); - self.report_assert_as_lint( - source_info, - AssertLint::ArithmeticOverflow(source_info.span, panic), - ); + self.report_assert_as_lint(location, AssertLintKind::ArithmeticOverflow, panic); return None; } } @@ -382,13 +387,10 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { let (_res, overflow) = this.ecx.overflowing_binary_op(op, &l, &r)?; Ok(overflow) })? { - let source_info = self.body.source_info(location); self.report_assert_as_lint( - source_info, - AssertLint::ArithmeticOverflow( - source_info.span, - AssertKind::Overflow(op, l.to_const_int(), r.to_const_int()), - ), + location, + AssertLintKind::ArithmeticOverflow, + AssertKind::Overflow(op, l.to_const_int(), r.to_const_int()), ); return None; } @@ -529,11 +531,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { // Need proper const propagator for these. _ => return None, }; - let source_info = self.body.source_info(location); - self.report_assert_as_lint( - source_info, - AssertLint::UnconditionalPanic(source_info.span, msg), - ); + self.report_assert_as_lint(location, AssertLintKind::UnconditionalPanic, msg); } None diff --git a/compiler/rustc_mir_transform/src/errors.rs b/compiler/rustc_mir_transform/src/errors.rs index 2ee660ddc9be1..4574cb4d28d25 100644 --- a/compiler/rustc_mir_transform/src/errors.rs +++ b/compiler/rustc_mir_transform/src/errors.rs @@ -201,45 +201,39 @@ impl<'a> DecorateLint<'a, ()> for UnsafeOpInUnsafeFn { } } -pub(crate) enum AssertLint

{ - ArithmeticOverflow(Span, AssertKind

), - UnconditionalPanic(Span, AssertKind

), +pub(crate) struct AssertLint

{ + pub span: Span, + pub assert_kind: AssertKind

, + pub lint_kind: AssertLintKind, +} + +pub(crate) enum AssertLintKind { + ArithmeticOverflow, + UnconditionalPanic, } impl<'a, P: std::fmt::Debug> DecorateLint<'a, ()> for AssertLint

{ fn decorate_lint<'b>(self, diag: &'b mut DiagnosticBuilder<'a, ()>) { - let span = self.span(); - let assert_kind = self.panic(); - let message = assert_kind.diagnostic_message(); - assert_kind.add_args(&mut |name, value| { + let message = self.assert_kind.diagnostic_message(); + self.assert_kind.add_args(&mut |name, value| { diag.arg(name, value); }); - diag.span_label(span, message); + diag.span_label(self.span, message); } fn msg(&self) -> DiagnosticMessage { - match self { - AssertLint::ArithmeticOverflow(..) => fluent::mir_transform_arithmetic_overflow, - AssertLint::UnconditionalPanic(..) => fluent::mir_transform_operation_will_panic, + match self.lint_kind { + AssertLintKind::ArithmeticOverflow => fluent::mir_transform_arithmetic_overflow, + AssertLintKind::UnconditionalPanic => fluent::mir_transform_operation_will_panic, } } } -impl

AssertLint

{ +impl AssertLintKind { pub fn lint(&self) -> &'static Lint { match self { - AssertLint::ArithmeticOverflow(..) => lint::builtin::ARITHMETIC_OVERFLOW, - AssertLint::UnconditionalPanic(..) => lint::builtin::UNCONDITIONAL_PANIC, - } - } - pub fn span(&self) -> Span { - match self { - AssertLint::ArithmeticOverflow(sp, _) | AssertLint::UnconditionalPanic(sp, _) => *sp, - } - } - pub fn panic(self) -> AssertKind

{ - match self { - AssertLint::ArithmeticOverflow(_, p) | AssertLint::UnconditionalPanic(_, p) => p, + AssertLintKind::ArithmeticOverflow => lint::builtin::ARITHMETIC_OVERFLOW, + AssertLintKind::UnconditionalPanic => lint::builtin::UNCONDITIONAL_PANIC, } } } From 2d99ea0be21e86b74639567d0bef685eba49debb Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Mon, 15 Jan 2024 13:33:58 +0000 Subject: [PATCH 10/15] Remove ConstPropMachine and re-use the DummyMachine instead --- .../rustc_mir_transform/src/const_prop.rs | 139 +----------------- .../src/const_prop_lint.rs | 6 +- 2 files changed, 4 insertions(+), 141 deletions(-) diff --git a/compiler/rustc_mir_transform/src/const_prop.rs b/compiler/rustc_mir_transform/src/const_prop.rs index 69894b6f07b61..8b9e507c8c746 100644 --- a/compiler/rustc_mir_transform/src/const_prop.rs +++ b/compiler/rustc_mir_transform/src/const_prop.rs @@ -1,20 +1,12 @@ //! Propagates constants for early reporting of statically known //! assertion failures -use rustc_const_eval::interpret::{ - self, compile_time_machine, AllocId, ConstAllocation, FnArg, Frame, ImmTy, InterpCx, - InterpResult, OpTy, PlaceTy, Pointer, -}; use rustc_index::bit_set::BitSet; use rustc_index::IndexVec; use rustc_middle::mir::visit::{MutatingUseContext, NonMutatingUseContext, PlaceContext, Visitor}; use rustc_middle::mir::*; -use rustc_middle::query::TyCtxtAt; -use rustc_middle::ty::layout::TyAndLayout; -use rustc_middle::ty::{self, ParamEnv, TyCtxt}; -use rustc_span::def_id::DefId; +use rustc_middle::ty::{ParamEnv, TyCtxt}; use rustc_target::abi::Size; -use rustc_target::spec::abi::Abi as CallAbi; /// The maximum number of bytes that we'll allocate space for a local or the return value. /// Needed for #66397, because otherwise we eval into large places and that can cause OOM or just @@ -48,135 +40,6 @@ pub(crate) macro throw_machine_stop_str($($tt:tt)*) {{ throw_machine_stop!(Zst) }} -pub(crate) struct ConstPropMachine; - -impl<'mir, 'tcx: 'mir> interpret::Machine<'mir, 'tcx> for ConstPropMachine { - compile_time_machine!(<'mir, 'tcx>); - - const PANIC_ON_ALLOC_FAIL: bool = true; // all allocations are small (see `MAX_ALLOC_LIMIT`) - - const POST_MONO_CHECKS: bool = false; // this MIR is still generic! - - type MemoryKind = !; - - #[inline(always)] - fn enforce_alignment(_ecx: &InterpCx<'mir, 'tcx, Self>) -> bool { - false // no reason to enforce alignment - } - - #[inline(always)] - fn enforce_validity(_ecx: &InterpCx<'mir, 'tcx, Self>, _layout: TyAndLayout<'tcx>) -> bool { - false // for now, we don't enforce validity - } - - fn load_mir( - _ecx: &InterpCx<'mir, 'tcx, Self>, - _instance: ty::InstanceDef<'tcx>, - ) -> InterpResult<'tcx, &'tcx Body<'tcx>> { - throw_machine_stop_str!("calling functions isn't supported in ConstProp") - } - - fn panic_nounwind(_ecx: &mut InterpCx<'mir, 'tcx, Self>, _msg: &str) -> InterpResult<'tcx> { - throw_machine_stop_str!("panicking isn't supported in ConstProp") - } - - fn find_mir_or_eval_fn( - _ecx: &mut InterpCx<'mir, 'tcx, Self>, - _instance: ty::Instance<'tcx>, - _abi: CallAbi, - _args: &[FnArg<'tcx>], - _destination: &PlaceTy<'tcx>, - _target: Option, - _unwind: UnwindAction, - ) -> InterpResult<'tcx, Option<(&'mir Body<'tcx>, ty::Instance<'tcx>)>> { - Ok(None) - } - - fn call_intrinsic( - _ecx: &mut InterpCx<'mir, 'tcx, Self>, - _instance: ty::Instance<'tcx>, - _args: &[OpTy<'tcx>], - _destination: &PlaceTy<'tcx>, - _target: Option, - _unwind: UnwindAction, - ) -> InterpResult<'tcx> { - throw_machine_stop_str!("calling intrinsics isn't supported in ConstProp") - } - - fn assert_panic( - _ecx: &mut InterpCx<'mir, 'tcx, Self>, - _msg: &rustc_middle::mir::AssertMessage<'tcx>, - _unwind: rustc_middle::mir::UnwindAction, - ) -> InterpResult<'tcx> { - bug!("panics terminators are not evaluated in ConstProp") - } - - fn binary_ptr_op( - _ecx: &InterpCx<'mir, 'tcx, Self>, - _bin_op: BinOp, - _left: &ImmTy<'tcx>, - _right: &ImmTy<'tcx>, - ) -> InterpResult<'tcx, (ImmTy<'tcx>, bool)> { - // We can't do this because aliasing of memory can differ between const eval and llvm - throw_machine_stop_str!("pointer arithmetic or comparisons aren't supported in ConstProp") - } - - fn before_access_local_mut<'a>( - _ecx: &'a mut InterpCx<'mir, 'tcx, Self>, - _frame: usize, - _local: Local, - ) -> InterpResult<'tcx> { - unreachable!() - } - - fn before_access_global( - _tcx: TyCtxtAt<'tcx>, - _machine: &Self, - _alloc_id: AllocId, - alloc: ConstAllocation<'tcx>, - _static_def_id: Option, - is_write: bool, - ) -> InterpResult<'tcx> { - if is_write { - throw_machine_stop_str!("can't write to global"); - } - // If the static allocation is mutable, then we can't const prop it as its content - // might be different at runtime. - if alloc.inner().mutability.is_mut() { - throw_machine_stop_str!("can't access mutable globals in ConstProp"); - } - - Ok(()) - } - - #[inline(always)] - fn expose_ptr(_ecx: &mut InterpCx<'mir, 'tcx, Self>, _ptr: Pointer) -> InterpResult<'tcx> { - throw_machine_stop_str!("exposing pointers isn't supported in ConstProp") - } - - #[inline(always)] - fn init_frame_extra( - _ecx: &mut InterpCx<'mir, 'tcx, Self>, - frame: Frame<'mir, 'tcx>, - ) -> InterpResult<'tcx, Frame<'mir, 'tcx>> { - Ok(frame) - } - - #[inline(always)] - fn stack<'a>( - _ecx: &'a InterpCx<'mir, 'tcx, Self>, - ) -> &'a [Frame<'mir, 'tcx, Self::Provenance, Self::FrameExtra>] { - &[] - } - - #[inline(always)] - fn stack_mut<'a>( - _ecx: &'a mut InterpCx<'mir, 'tcx, Self>, - ) -> &'a mut Vec> { - unreachable!() - } -} - /// The mode that `ConstProp` is allowed to run in for a given `Local`. #[derive(Clone, Copy, Debug, PartialEq)] pub enum ConstPropMode { diff --git a/compiler/rustc_mir_transform/src/const_prop_lint.rs b/compiler/rustc_mir_transform/src/const_prop_lint.rs index b26cba38fe2b0..db25e22663d01 100644 --- a/compiler/rustc_mir_transform/src/const_prop_lint.rs +++ b/compiler/rustc_mir_transform/src/const_prop_lint.rs @@ -18,8 +18,8 @@ use rustc_span::Span; use rustc_target::abi::{Abi, FieldIdx, HasDataLayout, Size, TargetDataLayout, VariantIdx}; use crate::const_prop::CanConstProp; -use crate::const_prop::ConstPropMachine; use crate::const_prop::ConstPropMode; +use crate::dataflow_const_prop::DummyMachine; use crate::errors::{AssertLint, AssertLintKind}; use crate::MirLint; @@ -70,7 +70,7 @@ impl<'tcx> MirLint<'tcx> for ConstPropLint { /// Finds optimization opportunities on the MIR. struct ConstPropagator<'mir, 'tcx> { - ecx: InterpCx<'mir, 'tcx, ConstPropMachine>, + ecx: InterpCx<'mir, 'tcx, DummyMachine>, tcx: TyCtxt<'tcx>, param_env: ParamEnv<'tcx>, worklist: Vec, @@ -184,7 +184,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { let param_env = tcx.param_env_reveal_all_normalized(def_id); let can_const_prop = CanConstProp::check(tcx, param_env, body); - let ecx = InterpCx::new(tcx, tcx.def_span(def_id), param_env, ConstPropMachine); + let ecx = InterpCx::new(tcx, tcx.def_span(def_id), param_env, DummyMachine); ConstPropagator { ecx, From d03eb339aa47a07636342cda3a49f1d97e4213f4 Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Mon, 15 Jan 2024 14:38:45 +0000 Subject: [PATCH 11/15] Implement ConstantIndex handling and use that instead using our own ProjectionElem variant --- .../src/const_prop_lint.rs | 41 +++++++------------ 1 file changed, 15 insertions(+), 26 deletions(-) diff --git a/compiler/rustc_mir_transform/src/const_prop_lint.rs b/compiler/rustc_mir_transform/src/const_prop_lint.rs index db25e22663d01..0f1ed1aa9e7b7 100644 --- a/compiler/rustc_mir_transform/src/const_prop_lint.rs +++ b/compiler/rustc_mir_transform/src/const_prop_lint.rs @@ -101,19 +101,19 @@ impl<'tcx> From> for Value<'tcx> { } impl<'tcx> Value<'tcx> { - fn project( - &self, - proj: impl Iterator>>>, - ) -> Option<&Value<'tcx>> { + fn project(&self, proj: impl Iterator>>) -> Option<&Value<'tcx>> { let mut this = self; for proj in proj { this = match (proj?, this) { (ProjectionElem::Field(idx, _), Value::Aggregate { fields, .. }) => { fields.get(idx).unwrap_or(&Value::Uninit) } - (ProjectionElem::Index(idx), Value::Aggregate { fields, .. }) => { - fields.get(idx).unwrap_or(&Value::Uninit) - } + ( + ProjectionElem::ConstantIndex { offset, min_length: 1, from_end: false }, + Value::Aggregate { fields, .. }, + ) => fields + .get(FieldIdx::from_u32(offset.try_into().ok()?)) + .unwrap_or(&Value::Uninit), _ => return None, }; } @@ -205,7 +205,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { fn get_const(&self, place: Place<'tcx>) -> Option<&Value<'tcx>> { self.locals[place.local] - .project(place.projection.iter().map(|proj| self.simple_projection(proj))) + .project(place.projection.iter().map(|proj| self.try_eval_index_offset(proj))) } /// Remove `local` from the pool of `Locals`. Allows writing to them, @@ -719,29 +719,18 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { Some(()) } - fn simple_projection( - &self, - proj: ProjectionElem>, - ) -> Option>> { + fn try_eval_index_offset(&self, proj: PlaceElem<'tcx>) -> Option> { Some(match proj { - ProjectionElem::Deref => ProjectionElem::Deref, - ProjectionElem::Field(idx, ty) => ProjectionElem::Field(idx, ty), ProjectionElem::Index(local) => { let val = self.get_const(local.into())?; let op = val.immediate()?; - ProjectionElem::Index(FieldIdx::from_u32( - self.ecx.read_target_usize(op).ok()?.try_into().ok()?, - )) - } - ProjectionElem::ConstantIndex { offset, min_length, from_end } => { - ProjectionElem::ConstantIndex { offset, min_length, from_end } - } - ProjectionElem::Subslice { from, to, from_end } => { - ProjectionElem::Subslice { from, to, from_end } + ProjectionElem::ConstantIndex { + offset: self.ecx.read_target_usize(op).ok()?, + min_length: 1, + from_end: false, + } } - ProjectionElem::Downcast(a, b) => ProjectionElem::Downcast(a, b), - ProjectionElem::OpaqueCast(ty) => ProjectionElem::OpaqueCast(ty), - ProjectionElem::Subtype(ty) => ProjectionElem::Subtype(ty), + other => other, }) } } From 6a01dc9ad7c2f5ae88ee68177fddf3971dfd86a0 Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Mon, 15 Jan 2024 14:48:17 +0000 Subject: [PATCH 12/15] Remove unnecessary optional layout being passed along --- .../src/const_prop_lint.rs | 74 +++++++------------ 1 file changed, 26 insertions(+), 48 deletions(-) diff --git a/compiler/rustc_mir_transform/src/const_prop_lint.rs b/compiler/rustc_mir_transform/src/const_prop_lint.rs index 0f1ed1aa9e7b7..76c8cdcc271b9 100644 --- a/compiler/rustc_mir_transform/src/const_prop_lint.rs +++ b/compiler/rustc_mir_transform/src/const_prop_lint.rs @@ -252,11 +252,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { } /// Returns the value, if any, of evaluating `c`. - fn eval_constant( - &mut self, - c: &ConstOperand<'tcx>, - layout: Option>, - ) -> Option> { + fn eval_constant(&mut self, c: &ConstOperand<'tcx>) -> Option> { // FIXME we need to revisit this for #67176 if c.has_param() { return None; @@ -270,16 +266,12 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { // manually normalized. let val = self.tcx.try_normalize_erasing_regions(self.param_env, c.const_).ok()?; - self.use_ecx(|this| this.ecx.eval_mir_constant(&val, Some(c.span), layout)) + self.use_ecx(|this| this.ecx.eval_mir_constant(&val, Some(c.span), None)) } /// Returns the value, if any, of evaluating `place`. #[instrument(level = "trace", skip(self), ret)] - fn eval_place( - &mut self, - place: Place<'tcx>, - layout: Option>, - ) -> Option> { + fn eval_place(&mut self, place: Place<'tcx>) -> Option> { match self.get_const(place)? { Value::Immediate(op) => Some(op.clone()), Value::Aggregate { .. } => None, @@ -289,14 +281,10 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { /// Returns the value, if any, of evaluating `op`. Calls upon `eval_constant` /// or `eval_place`, depending on the variant of `Operand` used. - fn eval_operand( - &mut self, - op: &Operand<'tcx>, - layout: Option>, - ) -> Option> { + fn eval_operand(&mut self, op: &Operand<'tcx>) -> Option> { match *op { - Operand::Constant(ref c) => self.eval_constant(c, layout), - Operand::Move(place) | Operand::Copy(place) => self.eval_place(place, layout), + Operand::Constant(ref c) => self.eval_constant(c), + Operand::Move(place) | Operand::Copy(place) => self.eval_place(place), } } @@ -319,7 +307,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { } fn check_unary_op(&mut self, op: UnOp, arg: &Operand<'tcx>, location: Location) -> Option<()> { - let arg = self.eval_operand(arg, None)?; + let arg = self.eval_operand(arg)?; if let (val, true) = self.use_ecx(|this| { let val = this.ecx.read_immediate(&arg)?; let (_res, overflow) = this.ecx.overflowing_unary_op(op, &val)?; @@ -346,12 +334,10 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { right: &Operand<'tcx>, location: Location, ) -> Option<()> { - let r = self - .eval_operand(right, None) - .and_then(|r| self.use_ecx(|this| this.ecx.read_immediate(&r))); - let l = self - .eval_operand(left, None) - .and_then(|l| self.use_ecx(|this| this.ecx.read_immediate(&l))); + let r = + self.eval_operand(right).and_then(|r| self.use_ecx(|this| this.ecx.read_immediate(&r))); + let l = + self.eval_operand(left).and_then(|l| self.use_ecx(|this| this.ecx.read_immediate(&l))); // Check for exceeding shifts *even if* we cannot evaluate the LHS. if matches!(op, BinOp::Shr | BinOp::Shl) { let r = r.clone()?; @@ -481,7 +467,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { cond: &Operand<'tcx>, location: Location, ) -> Option { - let value = &self.eval_operand(cond, None)?; + let value = &self.eval_operand(cond)?; trace!("assertion on {:?} should be {:?}", value, expected); let expected = Scalar::from_bool(expected); @@ -509,7 +495,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { let mut eval_to_int = |op| { // This can be `None` if the lhs wasn't const propagated and we just // triggered the assert on the value of the rhs. - self.eval_operand(op, None) + self.eval_operand(op) .and_then(|op| self.ecx.read_immediate(&op).ok()) .map_or(DbgVal::Underscore, |op| DbgVal::Val(op.to_const_int())) }; @@ -567,19 +553,15 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { let val: Value<'_> = match *rvalue { ThreadLocalRef(_) => return None, - Use(ref operand) => self.eval_operand(operand, Some(layout))?.into(), + Use(ref operand) => self.eval_operand(operand)?.into(), - CopyForDeref(place) => self.eval_place(place, Some(layout))?.into(), + CopyForDeref(place) => self.eval_place(place)?.into(), BinaryOp(bin_op, box (ref left, ref right)) => { - let layout = - rustc_const_eval::util::binop_left_homogeneous(bin_op).then_some(layout); - let left = self.eval_operand(left, layout)?; + let left = self.eval_operand(left)?; let left = self.use_ecx(|this| this.ecx.read_immediate(&left))?; - let layout = - rustc_const_eval::util::binop_right_homogeneous(bin_op).then_some(left.layout); - let right = self.eval_operand(right, layout)?; + let right = self.eval_operand(right)?; let right = self.use_ecx(|this| this.ecx.read_immediate(&right))?; let val = @@ -588,12 +570,10 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { } CheckedBinaryOp(bin_op, box (ref left, ref right)) => { - let left = self.eval_operand(left, None)?; + let left = self.eval_operand(left)?; let left = self.use_ecx(|this| this.ecx.read_immediate(&left))?; - let layout = - rustc_const_eval::util::binop_right_homogeneous(bin_op).then_some(left.layout); - let right = self.eval_operand(right, layout)?; + let right = self.eval_operand(right)?; let right = self.use_ecx(|this| this.ecx.read_immediate(&right))?; let (val, overflowed) = @@ -606,7 +586,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { } UnaryOp(un_op, ref operand) => { - let operand = self.eval_operand(operand, Some(layout))?; + let operand = self.eval_operand(operand)?; let val = self.use_ecx(|this| this.ecx.read_immediate(&operand))?; let val = self.use_ecx(|this| this.ecx.wrapping_unary_op(un_op, &val))?; @@ -616,9 +596,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { Aggregate(ref kind, ref fields) => Value::Aggregate { fields: fields .iter() - .map(|field| { - self.eval_operand(field, None).map_or(Value::Uninit, Value::Immediate) - }) + .map(|field| self.eval_operand(field).map_or(Value::Uninit, Value::Immediate)) .collect(), variant: match **kind { AggregateKind::Adt(_, variant, _, _, _) => variant, @@ -664,21 +642,21 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { Cast(ref kind, ref value, to) => match kind { CastKind::IntToInt | CastKind::IntToFloat => { - let value = self.eval_operand(value, None)?; + let value = self.eval_operand(value)?; let value = self.ecx.read_immediate(&value).ok()?; let to = self.ecx.layout_of(to).ok()?; let res = self.ecx.int_to_int_or_float(&value, to).ok()?; res.into() } CastKind::FloatToFloat | CastKind::FloatToInt => { - let value = self.eval_operand(value, None)?; + let value = self.eval_operand(value)?; let value = self.ecx.read_immediate(&value).ok()?; let to = self.ecx.layout_of(to).ok()?; let res = self.ecx.float_to_float_or_int(&value, to).ok()?; res.into() } CastKind::Transmute => { - let value = self.eval_operand(value, None)?; + let value = self.eval_operand(value)?; let to = self.ecx.layout_of(to).ok()?; // `offset` for immediates only supports scalar/scalar-pair ABIs, // so bail out if the target is not one. @@ -754,7 +732,7 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { fn visit_constant(&mut self, constant: &ConstOperand<'tcx>, location: Location) { trace!("visit_constant: {:?}", constant); self.super_constant(constant, location); - self.eval_constant(constant, None); + self.eval_constant(constant); } fn visit_assign(&mut self, place: &Place<'tcx>, rvalue: &Rvalue<'tcx>, location: Location) { @@ -827,7 +805,7 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { self.check_assertion(*expected, msg, cond, location); } TerminatorKind::SwitchInt { ref discr, ref targets } => { - if let Some(ref value) = self.eval_operand(discr, None) + if let Some(ref value) = self.eval_operand(discr) && let Some(value_const) = self.use_ecx(|this| this.ecx.read_scalar(value)) && let Ok(constant) = value_const.try_to_int() && let Ok(constant) = constant.to_bits(constant.size()) From c5e371da19eed3154ce6794a3e13bbbc763ca435 Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Tue, 16 Jan 2024 10:55:04 +0000 Subject: [PATCH 13/15] Inline Index conversion into `project` method --- .../src/const_prop_lint.rs | 35 ++++++++----------- 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/compiler/rustc_mir_transform/src/const_prop_lint.rs b/compiler/rustc_mir_transform/src/const_prop_lint.rs index 76c8cdcc271b9..8bbb81354d802 100644 --- a/compiler/rustc_mir_transform/src/const_prop_lint.rs +++ b/compiler/rustc_mir_transform/src/const_prop_lint.rs @@ -101,15 +101,24 @@ impl<'tcx> From> for Value<'tcx> { } impl<'tcx> Value<'tcx> { - fn project(&self, proj: impl Iterator>>) -> Option<&Value<'tcx>> { + fn project( + &self, + proj: &[PlaceElem<'tcx>], + prop: &ConstPropagator<'_, 'tcx>, + ) -> Option<&Value<'tcx>> { let mut this = self; for proj in proj { - this = match (proj?, this) { - (ProjectionElem::Field(idx, _), Value::Aggregate { fields, .. }) => { + this = match (*proj, this) { + (PlaceElem::Field(idx, _), Value::Aggregate { fields, .. }) => { fields.get(idx).unwrap_or(&Value::Uninit) } + (PlaceElem::Index(idx), Value::Aggregate { fields, .. }) => { + let idx = prop.get_const(idx.into())?.immediate()?; + let idx = prop.ecx.read_target_usize(idx).ok()?; + fields.get(FieldIdx::from_u32(idx.try_into().ok()?)).unwrap_or(&Value::Uninit) + } ( - ProjectionElem::ConstantIndex { offset, min_length: 1, from_end: false }, + PlaceElem::ConstantIndex { offset, min_length: 1, from_end: false }, Value::Aggregate { fields, .. }, ) => fields .get(FieldIdx::from_u32(offset.try_into().ok()?)) @@ -204,8 +213,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { } fn get_const(&self, place: Place<'tcx>) -> Option<&Value<'tcx>> { - self.locals[place.local] - .project(place.projection.iter().map(|proj| self.try_eval_index_offset(proj))) + self.locals[place.local].project(&place.projection, self) } /// Remove `local` from the pool of `Locals`. Allows writing to them, @@ -696,21 +704,6 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { Some(()) } - - fn try_eval_index_offset(&self, proj: PlaceElem<'tcx>) -> Option> { - Some(match proj { - ProjectionElem::Index(local) => { - let val = self.get_const(local.into())?; - let op = val.immediate()?; - ProjectionElem::ConstantIndex { - offset: self.ecx.read_target_usize(op).ok()?, - min_length: 1, - from_end: false, - } - } - other => other, - }) - } } impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { From 271821fbc335b22c030f9193a7872105f766eea6 Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Tue, 16 Jan 2024 11:28:07 +0000 Subject: [PATCH 14/15] Switch to using `ImmTy` instead of `OpTy`, as we don't use the `MPlace` variant at all --- .../src/const_prop_lint.rs | 37 ++++++++----------- 1 file changed, 16 insertions(+), 21 deletions(-) diff --git a/compiler/rustc_mir_transform/src/const_prop_lint.rs b/compiler/rustc_mir_transform/src/const_prop_lint.rs index 8bbb81354d802..8d25dc5b718e4 100644 --- a/compiler/rustc_mir_transform/src/const_prop_lint.rs +++ b/compiler/rustc_mir_transform/src/const_prop_lint.rs @@ -4,7 +4,7 @@ use std::fmt::Debug; use rustc_const_eval::interpret::{ImmTy, Projectable}; -use rustc_const_eval::interpret::{InterpCx, InterpResult, OpTy, Scalar}; +use rustc_const_eval::interpret::{InterpCx, InterpResult, Scalar}; use rustc_data_structures::fx::FxHashSet; use rustc_hir::def::DefKind; use rustc_hir::HirId; @@ -83,20 +83,14 @@ struct ConstPropagator<'mir, 'tcx> { #[derive(Debug, Clone)] enum Value<'tcx> { - Immediate(OpTy<'tcx>), + Immediate(ImmTy<'tcx>), Aggregate { variant: VariantIdx, fields: IndexVec> }, Uninit, } -impl<'tcx> From> for Value<'tcx> { - fn from(v: OpTy<'tcx>) -> Self { - Self::Immediate(v) - } -} - impl<'tcx> From> for Value<'tcx> { fn from(v: ImmTy<'tcx>) -> Self { - Self::Immediate(v.into()) + Self::Immediate(v) } } @@ -149,7 +143,7 @@ impl<'tcx> Value<'tcx> { Some(this) } - fn immediate(&self) -> Option<&OpTy<'tcx>> { + fn immediate(&self) -> Option<&ImmTy<'tcx>> { match self { Value::Immediate(op) => Some(op), _ => None, @@ -260,7 +254,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { } /// Returns the value, if any, of evaluating `c`. - fn eval_constant(&mut self, c: &ConstOperand<'tcx>) -> Option> { + fn eval_constant(&mut self, c: &ConstOperand<'tcx>) -> Option> { // FIXME we need to revisit this for #67176 if c.has_param() { return None; @@ -274,14 +268,16 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { // manually normalized. let val = self.tcx.try_normalize_erasing_regions(self.param_env, c.const_).ok()?; - self.use_ecx(|this| this.ecx.eval_mir_constant(&val, Some(c.span), None)) + self.use_ecx(|this| this.ecx.eval_mir_constant(&val, Some(c.span), None))? + .as_mplace_or_imm() + .right() } /// Returns the value, if any, of evaluating `place`. #[instrument(level = "trace", skip(self), ret)] - fn eval_place(&mut self, place: Place<'tcx>) -> Option> { + fn eval_place(&mut self, place: Place<'tcx>) -> Option> { match self.get_const(place)? { - Value::Immediate(op) => Some(op.clone()), + Value::Immediate(imm) => Some(imm.clone()), Value::Aggregate { .. } => None, Value::Uninit => None, } @@ -289,7 +285,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { /// Returns the value, if any, of evaluating `op`. Calls upon `eval_constant` /// or `eval_place`, depending on the variant of `Operand` used. - fn eval_operand(&mut self, op: &Operand<'tcx>) -> Option> { + fn eval_operand(&mut self, op: &Operand<'tcx>) -> Option> { match *op { Operand::Constant(ref c) => self.eval_constant(c), Operand::Move(place) | Operand::Copy(place) => self.eval_place(place), @@ -668,13 +664,12 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { let to = self.ecx.layout_of(to).ok()?; // `offset` for immediates only supports scalar/scalar-pair ABIs, // so bail out if the target is not one. - if value.as_mplace_or_imm().is_right() { - match (value.layout.abi, to.abi) { - (Abi::Scalar(..), Abi::Scalar(..)) => {} - (Abi::ScalarPair(..), Abi::ScalarPair(..)) => {} - _ => return None, - } + match (value.layout.abi, to.abi) { + (Abi::Scalar(..), Abi::Scalar(..)) => {} + (Abi::ScalarPair(..), Abi::ScalarPair(..)) => {} + _ => return None, } + value.offset(Size::ZERO, to, &self.ecx).ok()?.into() } _ => return None, From 1c9e621308f08a96b7f11b48ae9155d7a411bcbe Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Tue, 16 Jan 2024 16:37:43 +0000 Subject: [PATCH 15/15] No need to check min_length --- compiler/rustc_mir_transform/src/const_prop_lint.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/rustc_mir_transform/src/const_prop_lint.rs b/compiler/rustc_mir_transform/src/const_prop_lint.rs index 8d25dc5b718e4..aa22b8c7c58e0 100644 --- a/compiler/rustc_mir_transform/src/const_prop_lint.rs +++ b/compiler/rustc_mir_transform/src/const_prop_lint.rs @@ -112,7 +112,7 @@ impl<'tcx> Value<'tcx> { fields.get(FieldIdx::from_u32(idx.try_into().ok()?)).unwrap_or(&Value::Uninit) } ( - PlaceElem::ConstantIndex { offset, min_length: 1, from_end: false }, + PlaceElem::ConstantIndex { offset, min_length: _, from_end: false }, Value::Aggregate { fields, .. }, ) => fields .get(FieldIdx::from_u32(offset.try_into().ok()?))