From 35e622d2185ff094f8fbe528f24e8900b9369efb Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Thu, 30 Mar 2023 18:01:42 +0000 Subject: [PATCH 1/5] Do not keep extra references to scopes and decls. --- .../src/const_prop_lint.rs | 31 ++++++++----------- 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/compiler/rustc_mir_transform/src/const_prop_lint.rs b/compiler/rustc_mir_transform/src/const_prop_lint.rs index 176027b3b93e4..63cd51af0ce0d 100644 --- a/compiler/rustc_mir_transform/src/const_prop_lint.rs +++ b/compiler/rustc_mir_transform/src/const_prop_lint.rs @@ -9,7 +9,6 @@ use rustc_const_eval::interpret::{ }; use rustc_hir::def::DefKind; use rustc_hir::HirId; -use rustc_index::vec::IndexSlice; use rustc_middle::mir::visit::Visitor; use rustc_middle::mir::*; use rustc_middle::ty::layout::{LayoutError, LayoutOf, LayoutOfHelpers, TyAndLayout}; @@ -130,8 +129,6 @@ struct ConstPropagator<'mir, 'tcx> { ecx: InterpCx<'mir, 'tcx, ConstPropMachine<'mir, 'tcx>>, tcx: TyCtxt<'tcx>, param_env: ParamEnv<'tcx>, - source_scopes: &'mir IndexSlice>, - local_decls: &'mir IndexSlice>, // Because we have `MutVisitor` we can't obtain the `SourceInfo` from a `Location`. So we store // the last known `SourceInfo` here and just keep revisiting it. source_info: Option, @@ -209,14 +206,15 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { ) .expect("failed to push initial stack frame"); - ConstPropagator { - ecx, - tcx, - param_env, - source_scopes: &dummy_body.source_scopes, - local_decls: &dummy_body.local_decls, - source_info: None, - } + ConstPropagator { ecx, tcx, param_env, source_info: None } + } + + fn body(&self) -> &'mir Body<'tcx> { + self.ecx.frame().body + } + + fn local_decls(&self) -> &'mir LocalDecls<'tcx> { + &self.body().local_decls } fn get_const(&self, place: Place<'tcx>) -> Option> { @@ -251,7 +249,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { } fn lint_root(&self, source_info: SourceInfo) -> Option { - source_info.scope.lint_root(self.source_scopes) + source_info.scope.lint_root(&self.body().source_scopes) } fn use_ecx(&mut self, source_info: SourceInfo, f: F) -> Option @@ -368,7 +366,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { let r = r.clone()?; // We need the type of the LHS. We cannot use `place_layout` as that is the type // of the result, which for checked binops is not the same! - let left_ty = left.ty(self.local_decls, self.tcx); + let left_ty = left.ty(self.local_decls(), self.tcx); let left_size = self.ecx.layout_of(left_ty).ok()?.size; let right_size = r.layout.size; let r_bits = r.to_scalar().to_bits(right_size).ok(); @@ -481,10 +479,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { if rvalue.needs_subst() { return None; } - if !rvalue - .ty(&self.ecx.frame().body.local_decls, *self.ecx.tcx) - .is_sized(*self.ecx.tcx, self.param_env) - { + if !rvalue.ty(self.local_decls(), self.tcx).is_sized(self.tcx, self.param_env) { // the interpreter doesn't support unsized locals (only unsized arguments), // but rustc does (in a kinda broken way), so we have to skip them here return None; @@ -498,7 +493,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { assert!( self.get_const(local.into()).is_none() || self - .layout_of(self.local_decls[local].ty) + .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()), From b833fcae90b42a9b55224288d5ec9cfd86835d0d Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Thu, 30 Mar 2023 18:08:09 +0000 Subject: [PATCH 2/5] Manipulate Location instead of SourceInfo. --- .../src/const_prop_lint.rs | 86 ++++++++----------- 1 file changed, 36 insertions(+), 50 deletions(-) diff --git a/compiler/rustc_mir_transform/src/const_prop_lint.rs b/compiler/rustc_mir_transform/src/const_prop_lint.rs index 63cd51af0ce0d..47bcba0f25007 100644 --- a/compiler/rustc_mir_transform/src/const_prop_lint.rs +++ b/compiler/rustc_mir_transform/src/const_prop_lint.rs @@ -1,7 +1,7 @@ //! Propagates constants for early reporting of statically known //! assertion failures -use either::{Left, Right}; +use either::Left; use rustc_const_eval::interpret::Immediate; use rustc_const_eval::interpret::{ @@ -129,9 +129,6 @@ struct ConstPropagator<'mir, 'tcx> { ecx: InterpCx<'mir, 'tcx, ConstPropMachine<'mir, 'tcx>>, tcx: TyCtxt<'tcx>, param_env: ParamEnv<'tcx>, - // Because we have `MutVisitor` we can't obtain the `SourceInfo` from a `Location`. So we store - // the last known `SourceInfo` here and just keep revisiting it. - source_info: Option, } impl<'tcx> LayoutOfHelpers<'tcx> for ConstPropagator<'_, 'tcx> { @@ -206,7 +203,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { ) .expect("failed to push initial stack frame"); - ConstPropagator { ecx, tcx, param_env, source_info: None } + ConstPropagator { ecx, tcx, param_env } } fn body(&self) -> &'mir Body<'tcx> { @@ -252,12 +249,12 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { source_info.scope.lint_root(&self.body().source_scopes) } - fn use_ecx(&mut self, source_info: SourceInfo, f: F) -> Option + fn use_ecx(&mut self, location: Location, 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 = Right(source_info.span); + self.ecx.frame_mut().loc = Left(location); match f(self) { Ok(val) => Some(val), Err(error) => { @@ -276,7 +273,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { } /// Returns the value, if any, of evaluating `c`. - fn eval_constant(&mut self, c: &Constant<'tcx>, source_info: SourceInfo) -> Option> { + fn eval_constant(&mut self, c: &Constant<'tcx>, location: Location) -> Option> { // FIXME we need to revisit this for #67176 if c.needs_subst() { return None; @@ -290,45 +287,41 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { // manually normalized. let val = self.tcx.try_normalize_erasing_regions(self.param_env, c.literal).ok()?; - self.use_ecx(source_info, |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), None)) } /// Returns the value, if any, of evaluating `place`. - fn eval_place(&mut self, place: Place<'tcx>, source_info: SourceInfo) -> Option> { + fn eval_place(&mut self, place: Place<'tcx>, location: Location) -> Option> { trace!("eval_place(place={:?})", place); - self.use_ecx(source_info, |this| this.ecx.eval_place_to_op(place, None)) + self.use_ecx(location, |this| this.ecx.eval_place_to_op(place, None)) } /// 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>, source_info: SourceInfo) -> Option> { + fn eval_operand(&mut self, op: &Operand<'tcx>, location: Location) -> Option> { match *op { - Operand::Constant(ref c) => self.eval_constant(c, source_info), - Operand::Move(place) | Operand::Copy(place) => self.eval_place(place, source_info), + Operand::Constant(ref c) => self.eval_constant(c, location), + Operand::Move(place) | Operand::Copy(place) => self.eval_place(place, location), } } fn report_assert_as_lint( &self, lint: &'static lint::Lint, - source_info: SourceInfo, + location: Location, message: &'static str, panic: AssertKind, ) { - if let Some(lint_root) = self.lint_root(source_info) { + let source_info = self.body().source_info(location); + if let Some(lint_root) = self.lint_root(*source_info) { self.tcx.struct_span_lint_hir(lint, lint_root, source_info.span, message, |lint| { lint.span_label(source_info.span, format!("{:?}", panic)) }); } } - fn check_unary_op( - &mut self, - op: UnOp, - arg: &Operand<'tcx>, - source_info: SourceInfo, - ) -> Option<()> { - if let (val, true) = self.use_ecx(source_info, |this| { + fn check_unary_op(&mut self, op: UnOp, arg: &Operand<'tcx>, location: Location) -> Option<()> { + if let (val, true) = self.use_ecx(location, |this| { let val = this.ecx.read_immediate(&this.ecx.eval_operand(arg, None)?)?; let (_res, overflow, _ty) = this.ecx.overflowing_unary_op(op, &val)?; Ok((val, overflow)) @@ -338,7 +331,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { assert_eq!(op, UnOp::Neg, "Neg is the only UnOp that can overflow"); self.report_assert_as_lint( lint::builtin::ARITHMETIC_OVERFLOW, - source_info, + location, "this arithmetic operation will overflow", AssertKind::OverflowNeg(val.to_const_int()), ); @@ -353,14 +346,13 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { op: BinOp, left: &Operand<'tcx>, right: &Operand<'tcx>, - source_info: SourceInfo, + location: Location, ) -> Option<()> { - let r = self.use_ecx(source_info, |this| { + let r = self.use_ecx(location, |this| { this.ecx.read_immediate(&this.ecx.eval_operand(right, None)?) }); - let l = self.use_ecx(source_info, |this| { - this.ecx.read_immediate(&this.ecx.eval_operand(left, None)?) - }); + let l = self + .use_ecx(location, |this| this.ecx.read_immediate(&this.ecx.eval_operand(left, None)?)); // Check for exceeding shifts *even if* we cannot evaluate the LHS. if matches!(op, BinOp::Shr | BinOp::Shl) { let r = r.clone()?; @@ -371,10 +363,10 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { let right_size = r.layout.size; let r_bits = r.to_scalar().to_bits(right_size).ok(); if r_bits.map_or(false, |b| b >= left_size.bits() as u128) { - debug!("check_binary_op: reporting assert for {:?}", source_info); + debug!("check_binary_op: reporting assert for {:?}", location); self.report_assert_as_lint( lint::builtin::ARITHMETIC_OVERFLOW, - source_info, + location, "this arithmetic operation will overflow", AssertKind::Overflow( op, @@ -396,13 +388,13 @@ 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(source_info, |this| { + if self.use_ecx(location, |this| { let (_res, overflow, _ty) = this.ecx.overflowing_binary_op(op, &l, &r)?; Ok(overflow) })? { self.report_assert_as_lint( lint::builtin::ARITHMETIC_OVERFLOW, - source_info, + location, "this arithmetic operation will overflow", AssertKind::Overflow(op, l.to_const_int(), r.to_const_int()), ); @@ -412,7 +404,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { Some(()) } - fn check_rvalue(&mut self, rvalue: &Rvalue<'tcx>, source_info: SourceInfo) -> Option<()> { + fn check_rvalue(&mut self, rvalue: &Rvalue<'tcx>, location: Location) -> Option<()> { // Perform any special handling for specific Rvalue types. // Generally, checks here fall into one of two categories: // 1. Additional checking to provide useful lints to the user @@ -427,11 +419,11 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { // lint. Rvalue::UnaryOp(op, arg) => { trace!("checking UnaryOp(op = {:?}, arg = {:?})", op, arg); - self.check_unary_op(*op, arg, source_info)?; + self.check_unary_op(*op, arg, location)?; } Rvalue::BinaryOp(op, box (left, right)) => { trace!("checking BinaryOp(op = {:?}, left = {:?}, right = {:?})", op, left, right); - self.check_binary_op(*op, left, right, source_info)?; + self.check_binary_op(*op, left, right, location)?; } Rvalue::CheckedBinaryOp(op, box (left, right)) => { trace!( @@ -440,7 +432,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { left, right ); - self.check_binary_op(*op, left, right, source_info)?; + self.check_binary_op(*op, left, right, location)?; } // Do not try creating references (#67862) @@ -516,14 +508,13 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { fn visit_constant(&mut self, constant: &Constant<'tcx>, location: Location) { trace!("visit_constant: {:?}", constant); self.super_constant(constant, location); - self.eval_constant(constant, self.source_info.unwrap()); + self.eval_constant(constant, location); } fn visit_assign(&mut self, place: &Place<'tcx>, rvalue: &Rvalue<'tcx>, location: Location) { self.super_assign(place, rvalue, location); - let source_info = self.source_info.unwrap(); - let Some(()) = self.check_rvalue(rvalue, source_info) else { return }; + let Some(()) = self.check_rvalue(rvalue, location) else { return }; match self.ecx.machine.can_const_prop[place.local] { // Do nothing if the place is indirect. @@ -531,7 +522,7 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { ConstPropMode::NoPropagation => self.ensure_not_propagated(place.local), ConstPropMode::OnlyInsideOwnBlock | ConstPropMode::FullConstProp => { if self - .use_ecx(source_info, |this| this.ecx.eval_rvalue_into_place(rvalue, *place)) + .use_ecx(location, |this| this.ecx.eval_rvalue_into_place(rvalue, *place)) .is_none() { // Const prop failed, so erase the destination, ensuring that whatever happens @@ -557,8 +548,6 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) { trace!("visit_statement: {:?}", statement); - let source_info = statement.source_info; - self.source_info = Some(source_info); // We want to evaluate operands before any change to the assigned-to value, // so we recurse first. @@ -571,8 +560,7 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { _ if place.is_indirect() => {} ConstPropMode::NoPropagation => self.ensure_not_propagated(place.local), ConstPropMode::FullConstProp | ConstPropMode::OnlyInsideOwnBlock => { - if self.use_ecx(source_info, |this| this.ecx.statement(statement)).is_some() - { + 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); @@ -594,12 +582,10 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { } fn visit_terminator(&mut self, terminator: &Terminator<'tcx>, location: Location) { - let source_info = terminator.source_info; - self.source_info = Some(source_info); self.super_terminator(terminator, location); match &terminator.kind { TerminatorKind::Assert { expected, ref msg, ref cond, .. } => { - if let Some(ref value) = self.eval_operand(&cond, source_info) { + if let Some(ref value) = self.eval_operand(&cond, location) { trace!("assertion on {:?} should be {:?}", value, expected); let expected = Scalar::from_bool(*expected); let Ok(value_const) = self.ecx.read_scalar(&value) else { @@ -623,7 +609,7 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, '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, source_info) + self.eval_operand(op, location) .and_then(|op| self.ecx.read_immediate(&op).ok()) .map_or(DbgVal::Underscore, |op| DbgVal::Val(op.to_const_int())) }; @@ -664,7 +650,7 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { if let Some(msg) = msg { self.report_assert_as_lint( lint::builtin::UNCONDITIONAL_PANIC, - source_info, + location, "this operation will panic at runtime", msg, ); From f8cfafcceb3b4418a4e29f98a6d2b09975171872 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Thu, 30 Mar 2023 18:19:20 +0000 Subject: [PATCH 3/5] Extract check_assertion. --- .../src/const_prop_lint.rs | 143 +++++++++--------- 1 file changed, 71 insertions(+), 72 deletions(-) diff --git a/compiler/rustc_mir_transform/src/const_prop_lint.rs b/compiler/rustc_mir_transform/src/const_prop_lint.rs index 47bcba0f25007..c8ed30cdcf303 100644 --- a/compiler/rustc_mir_transform/src/const_prop_lint.rs +++ b/compiler/rustc_mir_transform/src/const_prop_lint.rs @@ -480,6 +480,76 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { Some(()) } + fn check_assertion( + &mut self, + expected: bool, + msg: &AssertKind>, + cond: &Operand<'tcx>, + location: Location, + ) -> Option { + let ref value = self.eval_operand(&cond, location)?; + 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))?; + + if expected != value_const { + // Poison all places this operand references so that further code + // doesn't use the invalid value + match cond { + Operand::Move(ref place) | Operand::Copy(ref place) => { + Self::remove_const(&mut self.ecx, place.local); + } + Operand::Constant(_) => {} + } + enum DbgVal { + Val(T), + Underscore, + } + impl std::fmt::Debug for DbgVal { + fn fmt(&self, fmt: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Val(val) => val.fmt(fmt), + Self::Underscore => fmt.write_str("_"), + } + } + } + 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) + .and_then(|op| self.ecx.read_immediate(&op).ok()) + .map_or(DbgVal::Underscore, |op| DbgVal::Val(op.to_const_int())) + }; + let msg = match msg { + AssertKind::DivisionByZero(op) => AssertKind::DivisionByZero(eval_to_int(op)), + AssertKind::RemainderByZero(op) => AssertKind::RemainderByZero(eval_to_int(op)), + AssertKind::Overflow(bin_op @ (BinOp::Div | BinOp::Rem), op1, op2) => { + // Division overflow is *UB* in the MIR, and different than the + // other overflow checks. + AssertKind::Overflow(*bin_op, eval_to_int(op1), eval_to_int(op2)) + } + AssertKind::BoundsCheck { ref len, ref index } => { + let len = eval_to_int(len); + let index = eval_to_int(index); + AssertKind::BoundsCheck { len, index } + } + // Remaining overflow errors are already covered by checks on the binary operators. + AssertKind::Overflow(..) | AssertKind::OverflowNeg(_) => return None, + // Need proper const propagator for these. + _ => return None, + }; + self.report_assert_as_lint( + lint::builtin::UNCONDITIONAL_PANIC, + location, + "this operation will panic at runtime", + msg, + ); + } + + None + } + fn ensure_not_propagated(&self, local: Local) { if cfg!(debug_assertions) { assert!( @@ -585,78 +655,7 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { self.super_terminator(terminator, location); match &terminator.kind { TerminatorKind::Assert { expected, ref msg, ref cond, .. } => { - if let Some(ref value) = self.eval_operand(&cond, location) { - trace!("assertion on {:?} should be {:?}", value, expected); - let expected = Scalar::from_bool(*expected); - let Ok(value_const) = self.ecx.read_scalar(&value) else { - // FIXME should be used use_ecx rather than a local match... but we have - // quite a few of these read_scalar/read_immediate that need fixing. - return - }; - if expected != value_const { - enum DbgVal { - Val(T), - Underscore, - } - impl std::fmt::Debug for DbgVal { - fn fmt(&self, fmt: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - Self::Val(val) => val.fmt(fmt), - Self::Underscore => fmt.write_str("_"), - } - } - } - 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) - .and_then(|op| self.ecx.read_immediate(&op).ok()) - .map_or(DbgVal::Underscore, |op| DbgVal::Val(op.to_const_int())) - }; - let msg = match msg { - AssertKind::DivisionByZero(op) => { - Some(AssertKind::DivisionByZero(eval_to_int(op))) - } - AssertKind::RemainderByZero(op) => { - Some(AssertKind::RemainderByZero(eval_to_int(op))) - } - AssertKind::Overflow(bin_op @ (BinOp::Div | BinOp::Rem), op1, op2) => { - // Division overflow is *UB* in the MIR, and different than the - // other overflow checks. - Some(AssertKind::Overflow( - *bin_op, - eval_to_int(op1), - eval_to_int(op2), - )) - } - AssertKind::BoundsCheck { ref len, ref index } => { - let len = eval_to_int(len); - let index = eval_to_int(index); - Some(AssertKind::BoundsCheck { len, index }) - } - // Remaining overflow errors are already covered by checks on the binary operators. - AssertKind::Overflow(..) | AssertKind::OverflowNeg(_) => None, - // Need proper const propagator for these. - _ => None, - }; - // Poison all places this operand references so that further code - // doesn't use the invalid value - match cond { - Operand::Move(ref place) | Operand::Copy(ref place) => { - Self::remove_const(&mut self.ecx, place.local); - } - Operand::Constant(_) => {} - } - if let Some(msg) = msg { - self.report_assert_as_lint( - lint::builtin::UNCONDITIONAL_PANIC, - location, - "this operation will panic at runtime", - msg, - ); - } - } - } + self.check_assertion(*expected, msg, cond, location); } // None of these have Operands to const-propagate. TerminatorKind::Goto { .. } From 7d47a910eba199e321a908cec62ee35bc41535e1 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Thu, 30 Mar 2023 18:40:47 +0000 Subject: [PATCH 4/5] Only visit reachable blocks in ConstProp lint. --- .../src/const_prop_lint.rs | 40 +++++++++++++++---- tests/ui/const_prop/unreachable-bounds.rs | 6 +++ tests/ui/const_prop/unreachable-overflow.rs | 10 +++++ 3 files changed, 48 insertions(+), 8 deletions(-) create mode 100644 tests/ui/const_prop/unreachable-bounds.rs create mode 100644 tests/ui/const_prop/unreachable-overflow.rs diff --git a/compiler/rustc_mir_transform/src/const_prop_lint.rs b/compiler/rustc_mir_transform/src/const_prop_lint.rs index c8ed30cdcf303..99bdb831c7508 100644 --- a/compiler/rustc_mir_transform/src/const_prop_lint.rs +++ b/compiler/rustc_mir_transform/src/const_prop_lint.rs @@ -9,6 +9,7 @@ use rustc_const_eval::interpret::{ }; use rustc_hir::def::DefKind; use rustc_hir::HirId; +use rustc_index::bit_set::BitSet; use rustc_middle::mir::visit::Visitor; use rustc_middle::mir::*; use rustc_middle::ty::layout::{LayoutError, LayoutOf, LayoutOfHelpers, TyAndLayout}; @@ -129,6 +130,8 @@ struct ConstPropagator<'mir, 'tcx> { ecx: InterpCx<'mir, 'tcx, ConstPropMachine<'mir, 'tcx>>, tcx: TyCtxt<'tcx>, param_env: ParamEnv<'tcx>, + worklist: Vec, + visited_blocks: BitSet, } impl<'tcx> LayoutOfHelpers<'tcx> for ConstPropagator<'_, 'tcx> { @@ -203,7 +206,13 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { ) .expect("failed to push initial stack frame"); - ConstPropagator { ecx, tcx, param_env } + ConstPropagator { + ecx, + tcx, + param_env, + worklist: vec![START_BLOCK], + visited_blocks: BitSet::new_empty(body.basic_blocks.len()), + } } fn body(&self) -> &'mir Body<'tcx> { @@ -496,12 +505,10 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { if expected != value_const { // Poison all places this operand references so that further code // doesn't use the invalid value - match cond { - Operand::Move(ref place) | Operand::Copy(ref place) => { - Self::remove_const(&mut self.ecx, place.local); - } - Operand::Constant(_) => {} + if let Some(place) = cond.place() { + Self::remove_const(&mut self.ecx, place.local); } + enum DbgVal { Val(T), Underscore, @@ -566,7 +573,12 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { fn visit_body(&mut self, body: &Body<'tcx>) { - for (bb, data) in body.basic_blocks.iter_enumerated() { + while let Some(bb) = self.worklist.pop() { + if !self.visited_blocks.insert(bb) { + continue; + } + + let data = &body.basic_blocks[bb]; self.visit_basic_block_data(bb, data); } } @@ -657,6 +669,17 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { TerminatorKind::Assert { expected, ref msg, ref cond, .. } => { self.check_assertion(*expected, msg, cond, location); } + TerminatorKind::SwitchInt { ref discr, ref targets } => { + if let Some(ref value) = self.eval_operand(&discr, location) + && let Some(value_const) = self.use_ecx(location, |this| this.ecx.read_scalar(&value)) + && let Ok(constant) = value_const.try_to_int() + && let Ok(constant) = constant.to_bits(constant.size()) + { + let target = targets.target_for_value(constant); + self.worklist.push(target); + return; + } + } // None of these have Operands to const-propagate. TerminatorKind::Goto { .. } | TerminatorKind::Resume @@ -668,10 +691,11 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { | TerminatorKind::GeneratorDrop | TerminatorKind::FalseEdge { .. } | TerminatorKind::FalseUnwind { .. } - | TerminatorKind::SwitchInt { .. } | TerminatorKind::Call { .. } | TerminatorKind::InlineAsm { .. } => {} } + + self.worklist.extend(terminator.successors()); } fn visit_basic_block_data(&mut self, block: BasicBlock, data: &BasicBlockData<'tcx>) { diff --git a/tests/ui/const_prop/unreachable-bounds.rs b/tests/ui/const_prop/unreachable-bounds.rs new file mode 100644 index 0000000000000..8cf98e154eaa0 --- /dev/null +++ b/tests/ui/const_prop/unreachable-bounds.rs @@ -0,0 +1,6 @@ +// Use `build-pass` to ensure const-prop lint runs. +// build-pass + +fn main() { + [()][if false { 1 } else { return }] +} diff --git a/tests/ui/const_prop/unreachable-overflow.rs b/tests/ui/const_prop/unreachable-overflow.rs new file mode 100644 index 0000000000000..2875135424d20 --- /dev/null +++ b/tests/ui/const_prop/unreachable-overflow.rs @@ -0,0 +1,10 @@ +// Use `build-pass` to ensure const-prop lint runs. +// build-pass + +fn main() { + let x = 2u32; + let y = 3u32; + if y <= x { + dbg!(x - y); + } +} From 98255cc2424b8ad3de462dc4241e14e23b1aa5c5 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Tue, 4 Apr 2023 17:14:53 +0000 Subject: [PATCH 5/5] Comment logic around worklist. --- compiler/rustc_mir_transform/src/const_prop_lint.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/compiler/rustc_mir_transform/src/const_prop_lint.rs b/compiler/rustc_mir_transform/src/const_prop_lint.rs index 99bdb831c7508..942ccd5a9d19e 100644 --- a/compiler/rustc_mir_transform/src/const_prop_lint.rs +++ b/compiler/rustc_mir_transform/src/const_prop_lint.rs @@ -675,10 +675,13 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { && let Ok(constant) = value_const.try_to_int() && let Ok(constant) = constant.to_bits(constant.size()) { + // We managed to evaluate the discriminant, so we know we only need to visit + // one target. let target = targets.target_for_value(constant); self.worklist.push(target); return; } + // We failed to evaluate the discriminant, fallback to visiting all successors. } // None of these have Operands to const-propagate. TerminatorKind::Goto { .. }