Skip to content

Commit 7aa413d

Browse files
borsbors
committed
Auto merge of #107921 - cjgillot:codegen-overflow-check, r=tmiasko
Make codegen choose whether to emit overflow checks ConstProp and DataflowConstProp currently have a specific code path not to propagate constants when they overflow. This is meant to have the correct behaviour when inlining from a crate with overflow checks (like `core`) into a crate compiled without. This PR shifts the behaviour change to the `Assert(Overflow*)` MIR terminators: if the crate is compiled without overflow checks, just skip emitting the assertions. This is already what happens with `OverflowNeg`. This allows ConstProp and DataflowConstProp to transform `CheckedBinaryOp(Add, u8::MAX, 1)` into `const (0, true)`, and let codegen ignore the `true`. The interpreter is modified to conform to this behaviour. Fixes #35310
2 parents dc89a80 + 9f6c1df commit 7aa413d

24 files changed

+250
-217
lines changed

‎compiler/rustc_codegen_cranelift/src/base.rs

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -347,7 +347,12 @@ fn codegen_fn_body(fx: &mut FunctionCx<'_, '_, '_>, start_block: Block) {
347347
}
348348
TerminatorKind::Assert { cond, expected, msg, target, cleanup: _ } => {
349349
if !fx.tcx.sess.overflow_checks() {
350-
if let mir::AssertKind::OverflowNeg(_) = *msg {
350+
let overflow_not_to_check = match msg {
351+
AssertKind::OverflowNeg(..) => true,
352+
AssertKind::Overflow(op, ..) => op.is_checkable(),
353+
_ => false,
354+
};
355+
if overflow_not_to_check {
351356
let target = fx.get_block(*target);
352357
fx.bcx.ins().jump(target, &[]);
353358
continue;
@@ -567,15 +572,7 @@ fn codegen_stmt<'tcx>(
567572
let lhs = codegen_operand(fx, &lhs_rhs.0);
568573
let rhs = codegen_operand(fx, &lhs_rhs.1);
569574

570-
let res = if !fx.tcx.sess.overflow_checks() {
571-
let val =
572-
crate::num::codegen_int_binop(fx, bin_op, lhs, rhs).load_scalar(fx);
573-
let is_overflow = fx.bcx.ins().iconst(types::I8, 0);
574-
CValue::by_val_pair(val, is_overflow, lval.layout())
575-
} else {
576-
crate::num::codegen_checked_int_binop(fx, bin_op, lhs, rhs)
577-
};
578-
575+
let res = crate::num::codegen_checked_int_binop(fx, bin_op, lhs, rhs);
579576
lval.write_cvalue(fx, res);
580577
}
581578
Rvalue::UnaryOp(un_op, ref operand) => {

‎compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -493,20 +493,6 @@ fn codegen_regular_intrinsic_call<'tcx>(
493493
let res = crate::num::codegen_int_binop(fx, bin_op, x, y);
494494
ret.write_cvalue(fx, res);
495495
}
496-
sym::add_with_overflow | sym::sub_with_overflow | sym::mul_with_overflow => {
497-
intrinsic_args!(fx, args => (x, y); intrinsic);
498-
499-
assert_eq!(x.layout().ty, y.layout().ty);
500-
let bin_op = match intrinsic {
501-
sym::add_with_overflow => BinOp::Add,
502-
sym::sub_with_overflow => BinOp::Sub,
503-
sym::mul_with_overflow => BinOp::Mul,
504-
_ => unreachable!(),
505-
};
506-
507-
let res = crate::num::codegen_checked_int_binop(fx, bin_op, x, y);
508-
ret.write_cvalue(fx, res);
509-
}
510496
sym::saturating_add | sym::saturating_sub => {
511497
intrinsic_args!(fx, args => (lhs, rhs); intrinsic);
512498

‎compiler/rustc_codegen_ssa/src/mir/block.rs

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -563,11 +563,13 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
563563
// with #[rustc_inherit_overflow_checks] and inlined from
564564
// another crate (mostly core::num generic/#[inline] fns),
565565
// while the current crate doesn't use overflow checks.
566-
// NOTE: Unlike binops, negation doesn't have its own
567-
// checked operation, just a comparison with the minimum
568-
// value, so we have to check for the assert message.
569-
if !bx.check_overflow() {
570-
if let AssertKind::OverflowNeg(_) = *msg {
566+
if !bx.cx().check_overflow() {
567+
let overflow_not_to_check = match msg {
568+
AssertKind::OverflowNeg(..) => true,
569+
AssertKind::Overflow(op, ..) => op.is_checkable(),
570+
_ => false,
571+
};
572+
if overflow_not_to_check {
571573
const_cond = Some(expected);
572574
}
573575
}

‎compiler/rustc_codegen_ssa/src/mir/intrinsic.rs

Lines changed: 0 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -218,9 +218,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
218218
args[1].val.unaligned_volatile_store(bx, dst);
219219
return;
220220
}
221-
sym::add_with_overflow
222-
| sym::sub_with_overflow
223-
| sym::mul_with_overflow
224221
| sym::unchecked_div
225222
| sym::unchecked_rem
226223
| sym::unchecked_shl
@@ -232,28 +229,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
232229
let ty = arg_tys[0];
233230
match int_type_width_signed(ty, bx.tcx()) {
234231
Some((_width, signed)) => match name {
235-
sym::add_with_overflow
236-
| sym::sub_with_overflow
237-
| sym::mul_with_overflow => {
238-
let op = match name {
239-
sym::add_with_overflow => OverflowOp::Add,
240-
sym::sub_with_overflow => OverflowOp::Sub,
241-
sym::mul_with_overflow => OverflowOp::Mul,
242-
_ => bug!(),
243-
};
244-
let (val, overflow) =
245-
bx.checked_binop(op, ty, args[0].immediate(), args[1].immediate());
246-
// Convert `i1` to a `bool`, and write it to the out parameter
247-
let val = bx.from_immediate(val);
248-
let overflow = bx.from_immediate(overflow);
249-
250-
let dest = result.project_field(bx, 0);
251-
bx.store(val, dest.llval, dest.align);
252-
let dest = result.project_field(bx, 1);
253-
bx.store(overflow, dest.llval, dest.align);
254-
255-
return;
256-
}
257232
sym::exact_div => {
258233
if signed {
259234
bx.exactsdiv(args[0].immediate(), args[1].immediate())

‎compiler/rustc_codegen_ssa/src/mir/rvalue.rs

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -652,15 +652,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
652652
rhs: Bx::Value,
653653
input_ty: Ty<'tcx>,
654654
) -> OperandValue<Bx::Value> {
655-
// This case can currently arise only from functions marked
656-
// with #[rustc_inherit_overflow_checks] and inlined from
657-
// another crate (mostly core::num generic/#[inline] fns),
658-
// while the current crate doesn't use overflow checks.
659-
if !bx.cx().check_overflow() {
660-
let val = self.codegen_scalar_binop(bx, op, lhs, rhs, input_ty);
661-
return OperandValue::Pair(val, bx.cx().const_bool(false));
662-
}
663-
664655
let (val, of) = match op {
665656
// These are checked using intrinsics
666657
mir::BinOp::Add | mir::BinOp::Sub | mir::BinOp::Mul => {

‎compiler/rustc_const_eval/src/interpret/intrinsics.rs

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -210,19 +210,6 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
210210
let out_val = numeric_intrinsic(intrinsic_name, bits, kind);
211211
self.write_scalar(out_val, dest)?;
212212
}
213-
sym::add_with_overflow | sym::sub_with_overflow | sym::mul_with_overflow => {
214-
let lhs = self.read_immediate(&args[0])?;
215-
let rhs = self.read_immediate(&args[1])?;
216-
let bin_op = match intrinsic_name {
217-
sym::add_with_overflow => BinOp::Add,
218-
sym::sub_with_overflow => BinOp::Sub,
219-
sym::mul_with_overflow => BinOp::Mul,
220-
_ => bug!(),
221-
};
222-
self.binop_with_overflow(
223-
bin_op, /*force_overflow_checks*/ true, &lhs, &rhs, dest,
224-
)?;
225-
}
226213
sym::saturating_add | sym::saturating_sub => {
227214
let l = self.read_immediate(&args[0])?;
228215
let r = self.read_immediate(&args[1])?;

‎compiler/rustc_const_eval/src/interpret/machine.rs

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -147,8 +147,9 @@ pub trait Machine<'mir, 'tcx>: Sized {
147147
true
148148
}
149149

150-
/// Whether CheckedBinOp MIR statements should actually check for overflow.
151-
fn checked_binop_checks_overflow(_ecx: &InterpCx<'mir, 'tcx, Self>) -> bool;
150+
/// Whether Assert(OverflowNeg) and Assert(Overflow) MIR terminators should actually
151+
/// check for overflow.
152+
fn ignore_checkable_overflow_assertions(_ecx: &InterpCx<'mir, 'tcx, Self>) -> bool;
152153

153154
/// Entry point for obtaining the MIR of anything that should get evaluated.
154155
/// So not just functions and shims, but also const/static initializers, anonymous
@@ -466,8 +467,8 @@ pub macro compile_time_machine(<$mir: lifetime, $tcx: lifetime>) {
466467
}
467468

468469
#[inline(always)]
469-
fn checked_binop_checks_overflow(_ecx: &InterpCx<$mir, $tcx, Self>) -> bool {
470-
true
470+
fn ignore_checkable_overflow_assertions(_ecx: &InterpCx<$mir, $tcx, Self>) -> bool {
471+
false
471472
}
472473

473474
#[inline(always)]

‎compiler/rustc_const_eval/src/interpret/operator.rs

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -10,13 +10,9 @@ use super::{ImmTy, Immediate, InterpCx, Machine, PlaceTy};
1010
impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
1111
/// Applies the binary operation `op` to the two operands and writes a tuple of the result
1212
/// and a boolean signifying the potential overflow to the destination.
13-
///
14-
/// `force_overflow_checks` indicates whether overflow checks should be done even when
15-
/// `tcx.sess.overflow_checks()` is `false`.
1613
pub fn binop_with_overflow(
1714
&mut self,
1815
op: mir::BinOp,
19-
force_overflow_checks: bool,
2016
left: &ImmTy<'tcx, M::Provenance>,
2117
right: &ImmTy<'tcx, M::Provenance>,
2218
dest: &PlaceTy<'tcx, M::Provenance>,
@@ -28,10 +24,6 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
2824
"type mismatch for result of {:?}",
2925
op,
3026
);
31-
// As per https://github.com/rust-lang/rust/pull/98738, we always return `false` in the 2nd
32-
// component when overflow checking is disabled.
33-
let overflowed =
34-
overflowed && (force_overflow_checks || M::checked_binop_checks_overflow(self));
3527
// Write the result to `dest`.
3628
if let Abi::ScalarPair(..) = dest.layout.abi {
3729
// We can use the optimized path and avoid `place_field` (which might do

‎compiler/rustc_const_eval/src/interpret/step.rs

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -185,9 +185,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
185185
let left = self.read_immediate(&self.eval_operand(left, None)?)?;
186186
let layout = binop_right_homogeneous(bin_op).then_some(left.layout);
187187
let right = self.read_immediate(&self.eval_operand(right, layout)?)?;
188-
self.binop_with_overflow(
189-
bin_op, /*force_overflow_checks*/ false, &left, &right, &dest,
190-
)?;
188+
self.binop_with_overflow(bin_op, &left, &right, &dest)?;
191189
}
192190

193191
UnaryOp(un_op, ref operand) => {

‎compiler/rustc_const_eval/src/interpret/terminator.rs

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -137,8 +137,14 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
137137
}
138138

139139
Assert { ref cond, expected, ref msg, target, cleanup } => {
140+
let ignored = M::ignore_checkable_overflow_assertions(self)
141+
&& match msg {
142+
mir::AssertKind::OverflowNeg(..) => true,
143+
mir::AssertKind::Overflow(op, ..) => op.is_checkable(),
144+
_ => false,
145+
};
140146
let cond_val = self.read_scalar(&self.eval_operand(cond, None)?)?.to_bool()?;
141-
if expected == cond_val {
147+
if ignored || expected == cond_val {
142148
self.go_to_block(target);
143149
} else {
144150
M::assert_panic(self, msg, cleanup)?;

‎compiler/rustc_middle/src/mir/syntax.rs

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -671,6 +671,12 @@ pub enum TerminatorKind<'tcx> {
671671
/// as parameters, and `None` for the destination. Keep in mind that the `cleanup` path is not
672672
/// necessarily executed even in the case of a panic, for example in `-C panic=abort`. If the
673673
/// assertion does not fail, execution continues at the specified basic block.
674+
///
675+
/// When overflow checking is disabled and this is run-time MIR (as opposed to compile-time MIR
676+
/// that is used for CTFE), the following variants of this terminator behave as `goto target`:
677+
/// - `OverflowNeg(..)`,
678+
/// - `Overflow(op, ..)` if op is a "checkable" operation (add, sub, mul, shl, shr, but NOT
679+
/// div or rem).
674680
Assert {
675681
cond: Operand<'tcx>,
676682
expected: bool,
@@ -1103,10 +1109,6 @@ pub enum Rvalue<'tcx> {
11031109

11041110
/// Same as `BinaryOp`, but yields `(T, bool)` with a `bool` indicating an error condition.
11051111
///
1106-
/// When overflow checking is disabled and we are generating run-time code, the error condition
1107-
/// is false. Otherwise, and always during CTFE, the error condition is determined as described
1108-
/// below.
1109-
///
11101112
/// For addition, subtraction, and multiplication on integers the error condition is set when
11111113
/// the infinite precision result would be unequal to the actual result.
11121114
///

‎compiler/rustc_mir_transform/src/const_prop.rs

Lines changed: 15 additions & 96 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ use rustc_middle::mir::visit::{
1515
};
1616
use rustc_middle::mir::{
1717
BasicBlock, BinOp, Body, Constant, ConstantKind, Local, LocalDecl, LocalKind, Location,
18-
Operand, Place, Rvalue, SourceInfo, Statement, StatementKind, Terminator, TerminatorKind, UnOp,
18+
Operand, Place, Rvalue, SourceInfo, Statement, StatementKind, Terminator, TerminatorKind,
1919
RETURN_PLACE,
2020
};
2121
use rustc_middle::ty::layout::{LayoutError, LayoutOf, LayoutOfHelpers, TyAndLayout};
@@ -503,55 +503,6 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
503503
}
504504
}
505505

506-
fn check_unary_op(&mut self, op: UnOp, arg: &Operand<'tcx>) -> Option<()> {
507-
if self.use_ecx(|this| {
508-
let val = this.ecx.read_immediate(&this.ecx.eval_operand(arg, None)?)?;
509-
let (_res, overflow, _ty) = this.ecx.overflowing_unary_op(op, &val)?;
510-
Ok(overflow)
511-
})? {
512-
// `AssertKind` only has an `OverflowNeg` variant, so make sure that is
513-
// appropriate to use.
514-
assert_eq!(op, UnOp::Neg, "Neg is the only UnOp that can overflow");
515-
return None;
516-
}
517-
518-
Some(())
519-
}
520-
521-
fn check_binary_op(
522-
&mut self,
523-
op: BinOp,
524-
left: &Operand<'tcx>,
525-
right: &Operand<'tcx>,
526-
) -> Option<()> {
527-
let r = self.use_ecx(|this| this.ecx.read_immediate(&this.ecx.eval_operand(right, None)?));
528-
let l = self.use_ecx(|this| this.ecx.read_immediate(&this.ecx.eval_operand(left, None)?));
529-
// Check for exceeding shifts *even if* we cannot evaluate the LHS.
530-
if matches!(op, BinOp::Shr | BinOp::Shl) {
531-
let r = r.clone()?;
532-
// We need the type of the LHS. We cannot use `place_layout` as that is the type
533-
// of the result, which for checked binops is not the same!
534-
let left_ty = left.ty(self.local_decls, self.tcx);
535-
let left_size = self.ecx.layout_of(left_ty).ok()?.size;
536-
let right_size = r.layout.size;
537-
let r_bits = r.to_scalar().to_bits(right_size).ok();
538-
if r_bits.map_or(false, |b| b >= left_size.bits() as u128) {
539-
return None;
540-
}
541-
}
542-
543-
if let (Some(l), Some(r)) = (&l, &r) {
544-
// The remaining operators are handled through `overflowing_binary_op`.
545-
if self.use_ecx(|this| {
546-
let (_res, overflow, _ty) = this.ecx.overflowing_binary_op(op, l, r)?;
547-
Ok(overflow)
548-
})? {
549-
return None;
550-
}
551-
}
552-
Some(())
553-
}
554-
555506
fn propagate_operand(&mut self, operand: &mut Operand<'tcx>) {
556507
match *operand {
557508
Operand::Copy(l) | Operand::Move(l) => {
@@ -587,28 +538,6 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
587538
// 2. Working around bugs in other parts of the compiler
588539
// - In this case, we'll return `None` from this function to stop evaluation.
589540
match rvalue {
590-
// Additional checking: give lints to the user if an overflow would occur.
591-
// We do this here and not in the `Assert` terminator as that terminator is
592-
// only sometimes emitted (overflow checks can be disabled), but we want to always
593-
// lint.
594-
Rvalue::UnaryOp(op, arg) => {
595-
trace!("checking UnaryOp(op = {:?}, arg = {:?})", op, arg);
596-
self.check_unary_op(*op, arg)?;
597-
}
598-
Rvalue::BinaryOp(op, box (left, right)) => {
599-
trace!("checking BinaryOp(op = {:?}, left = {:?}, right = {:?})", op, left, right);
600-
self.check_binary_op(*op, left, right)?;
601-
}
602-
Rvalue::CheckedBinaryOp(op, box (left, right)) => {
603-
trace!(
604-
"checking CheckedBinaryOp(op = {:?}, left = {:?}, right = {:?})",
605-
op,
606-
left,
607-
right
608-
);
609-
self.check_binary_op(*op, left, right)?;
610-
}
611-
612541
// Do not try creating references (#67862)
613542
Rvalue::AddressOf(_, place) | Rvalue::Ref(_, _, place) => {
614543
trace!("skipping AddressOf | Ref for {:?}", place);
@@ -638,7 +567,10 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
638567
| Rvalue::Cast(..)
639568
| Rvalue::ShallowInitBox(..)
640569
| Rvalue::Discriminant(..)
641-
| Rvalue::NullaryOp(..) => {}
570+
| Rvalue::NullaryOp(..)
571+
| Rvalue::UnaryOp(..)
572+
| Rvalue::BinaryOp(..)
573+
| Rvalue::CheckedBinaryOp(..) => {}
642574
}
643575

644576
// FIXME we need to revisit this for #67176
@@ -1079,31 +1011,18 @@ impl<'tcx> MutVisitor<'tcx> for ConstPropagator<'_, 'tcx> {
10791011
// Do NOT early return in this function, it does some crucial fixup of the state at the end!
10801012
match &mut terminator.kind {
10811013
TerminatorKind::Assert { expected, ref mut cond, .. } => {
1082-
if let Some(ref value) = self.eval_operand(&cond) {
1083-
trace!("assertion on {:?} should be {:?}", value, expected);
1084-
let expected = Scalar::from_bool(*expected);
1014+
if let Some(ref value) = self.eval_operand(&cond)
10851015
// FIXME should be used use_ecx rather than a local match... but we have
10861016
// quite a few of these read_scalar/read_immediate that need fixing.
1087-
if let Ok(value_const) = self.ecx.read_scalar(&value) {
1088-
if expected != value_const {
1089-
// Poison all places this operand references so that further code
1090-
// doesn't use the invalid value
1091-
match cond {
1092-
Operand::Move(ref place) | Operand::Copy(ref place) => {
1093-
Self::remove_const(&mut self.ecx, place.local);
1094-
}
1095-
Operand::Constant(_) => {}
1096-
}
1097-
} else {
1098-
if self.should_const_prop(value) {
1099-
*cond = self.operand_from_scalar(
1100-
value_const,
1101-
self.tcx.types.bool,
1102-
source_info.span,
1103-
);
1104-
}
1105-
}
1106-
}
1017+
&& let Ok(value_const) = self.ecx.read_scalar(&value)
1018+
&& self.should_const_prop(value)
1019+
{
1020+
trace!("assertion on {:?} should be {:?}", value, expected);
1021+
*cond = self.operand_from_scalar(
1022+
value_const,
1023+
self.tcx.types.bool,
1024+
source_info.span,
1025+
);
11071026
}
11081027
}
11091028
TerminatorKind::SwitchInt { ref mut discr, .. } => {

0 commit comments

Comments
 (0)