From 32ff9a03bcfa17296084c9f88ac150f59fc54f4b Mon Sep 17 00:00:00 2001 From: jumbatm Date: Tue, 21 Apr 2020 09:26:46 +1000 Subject: [PATCH 1/5] Also catch #71353 --- src/test/ui/consts/const-eval/ice-generic-assoc-const.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/test/ui/consts/const-eval/ice-generic-assoc-const.rs b/src/test/ui/consts/const-eval/ice-generic-assoc-const.rs index 4444cdfcda9c7..92153d7940aac 100644 --- a/src/test/ui/consts/const-eval/ice-generic-assoc-const.rs +++ b/src/test/ui/consts/const-eval/ice-generic-assoc-const.rs @@ -1,4 +1,5 @@ -// check-pass +// build-pass +#![crate_type = "lib"] pub trait Nullable { const NULL: Self; @@ -13,6 +14,3 @@ impl Nullable for *const T { *self == Self::NULL } } - -fn main() { -} From c61c17c20915ef9aa0d79a05bbf824e4ddbcca8d Mon Sep 17 00:00:00 2001 From: jumbatm Date: Tue, 21 Apr 2020 20:46:18 +1000 Subject: [PATCH 2/5] Skip check_binary_op if rhs needs substs. --- src/librustc_mir/transform/const_prop.rs | 56 ++++++++++++------------ 1 file changed, 29 insertions(+), 27 deletions(-) diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs index 79dba2c5db8fb..1aba45b03c01e 100644 --- a/src/librustc_mir/transform/const_prop.rs +++ b/src/librustc_mir/transform/const_prop.rs @@ -553,19 +553,35 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { right: &Operand<'tcx>, source_info: SourceInfo, ) -> Option<()> { - let r = - self.use_ecx(|this| this.ecx.read_immediate(this.ecx.eval_operand(right, None)?))?; - // Check for exceeding shifts *even if* we cannot evaluate the LHS. - if op == BinOp::Shr || op == BinOp::Shl { - // 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_size_bits = self.ecx.layout_of(left_ty).ok()?.size.bits(); - let right_size = r.layout.size; - let r_bits = r.to_scalar().ok(); - // This is basically `force_bits`. - let r_bits = r_bits.and_then(|r| r.to_bits_or_ptr(right_size, &self.tcx).ok()); - if r_bits.map_or(false, |b| b >= left_size_bits as u128) { + if !right.needs_subst() { + let r = + self.use_ecx(|this| this.ecx.read_immediate(this.ecx.eval_operand(right, None)?))?; + // Check for exceeding shifts *even if* we cannot evaluate the LHS. + if op == BinOp::Shr || op == BinOp::Shl { + // 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_size_bits = self.ecx.layout_of(left_ty).ok()?.size.bits(); + let right_size = r.layout.size; + let r_bits = r.to_scalar().ok(); + // This is basically `force_bits`. + let r_bits = r_bits.and_then(|r| r.to_bits_or_ptr(right_size, &self.tcx).ok()); + if r_bits.map_or(false, |b| b >= left_size_bits as u128) { + self.report_assert_as_lint( + lint::builtin::ARITHMETIC_OVERFLOW, + source_info, + "this arithmetic operation will overflow", + AssertKind::Overflow(op), + )?; + } + } + + // The remaining operators are handled through `overflowing_binary_op`. + if self.use_ecx(|this| { + let l = this.ecx.read_immediate(this.ecx.eval_operand(left, None)?)?; + 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, @@ -575,20 +591,6 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { } } - // The remaining operators are handled through `overflowing_binary_op`. - if self.use_ecx(|this| { - let l = this.ecx.read_immediate(this.ecx.eval_operand(left, None)?)?; - 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, - "this arithmetic operation will overflow", - AssertKind::Overflow(op), - )?; - } - Some(()) } From cf3b744c29a4d82863eb184ea5eb067ea9076d1a Mon Sep 17 00:00:00 2001 From: jumbatm Date: Wed, 22 Apr 2020 06:27:53 +1000 Subject: [PATCH 3/5] Also add substs check before evaluating for lhs. --- src/librustc_mir/transform/const_prop.rs | 26 +++++++++++++----------- 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs index 1aba45b03c01e..87f6168f056d9 100644 --- a/src/librustc_mir/transform/const_prop.rs +++ b/src/librustc_mir/transform/const_prop.rs @@ -576,18 +576,20 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { } } - // The remaining operators are handled through `overflowing_binary_op`. - if self.use_ecx(|this| { - let l = this.ecx.read_immediate(this.ecx.eval_operand(left, None)?)?; - 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, - "this arithmetic operation will overflow", - AssertKind::Overflow(op), - )?; + if !left.needs_subst() { + // The remaining operators are handled through `overflowing_binary_op`. + if self.use_ecx(|this| { + let l = this.ecx.read_immediate(this.ecx.eval_operand(left, None)?)?; + 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, + "this arithmetic operation will overflow", + AssertKind::Overflow(op), + )?; + } } } From 2d25b2b537ebe933e653be3087789597928dc0fc Mon Sep 17 00:00:00 2001 From: jumbatm Date: Wed, 22 Apr 2020 06:29:17 +1000 Subject: [PATCH 4/5] Also add substs check to check_unary_op. --- src/librustc_mir/transform/const_prop.rs | 30 +++++++++++++----------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs index 87f6168f056d9..2ae35ba6d1b55 100644 --- a/src/librustc_mir/transform/const_prop.rs +++ b/src/librustc_mir/transform/const_prop.rs @@ -527,20 +527,22 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { arg: &Operand<'tcx>, source_info: SourceInfo, ) -> Option<()> { - if self.use_ecx(|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(overflow) - })? { - // `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"); - self.report_assert_as_lint( - lint::builtin::ARITHMETIC_OVERFLOW, - source_info, - "this arithmetic operation will overflow", - AssertKind::OverflowNeg, - )?; + if !arg.needs_subst() { + if self.use_ecx(|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(overflow) + })? { + // `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"); + self.report_assert_as_lint( + lint::builtin::ARITHMETIC_OVERFLOW, + source_info, + "this arithmetic operation will overflow", + AssertKind::OverflowNeg, + )?; + } } Some(()) From 2e6fab9ab9ccf2488a406468ee9fc0165612ab67 Mon Sep 17 00:00:00 2001 From: jumbatm Date: Wed, 22 Apr 2020 06:33:37 +1000 Subject: [PATCH 5/5] Explain check_{unary,binary}_op needs-substs behaviour. --- src/librustc_mir/transform/const_prop.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs index 2ae35ba6d1b55..150ffefd207f7 100644 --- a/src/librustc_mir/transform/const_prop.rs +++ b/src/librustc_mir/transform/const_prop.rs @@ -521,6 +521,8 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { None } + /// Check for overflow for a unary op. Like check_binary_op, safe to call even if the operand + /// needs substs. In that case, no work is done. fn check_unary_op( &mut self, op: UnOp, @@ -548,6 +550,8 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { Some(()) } + /// Check a binary operand for overflow. Safe to call with values that need substs -- those + /// values just won't be checked. fn check_binary_op( &mut self, op: BinOp,