Rollup merge of #69002 - RalfJung:miri-op-overflow, r=oli-obk,wesleywiser

miri: improve and simplify overflow detection

This simplifies the overflow detection for signed binary operators, and adds overflow detection to unary operators so that const-prop doesn't have to crudely hand-roll that.

It also fixes some bugs in the operator implementation that however, I think, were not observable.

r? @oli-obk @wesleywiser

Author: Dylan-DPC

src/librustc_mir/interpret/operator.rs

@@ -134,9 +134,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
             let mut r = r as u32;
             let size = left_layout.size;
             oflo |= r >= size.bits() as u32;
-            if oflo {
-                r %= size.bits() as u32;
-            }
+            r %= size.bits() as u32;
             let result = if signed {
                 let l = self.sign_extend(l, left_layout) as i128;
                 let result = match bin_op {
@@ -168,6 +166,8 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
             )
         }
 
+        let size = left_layout.size;
+
         // Operations that need special treatment for signed integers
         if left_layout.abi.is_signed() {
             let op: Option<fn(&i128, &i128) -> bool> = match bin_op {
@@ -195,32 +195,31 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
             if let Some(op) = op {
                 let l128 = self.sign_extend(l, left_layout) as i128;
                 let r = self.sign_extend(r, right_layout) as i128;
-                let size = left_layout.size;
+                // We need a special check for overflowing remainder:
+                // "int_min % -1" overflows and returns 0, but after casting things to a larger int
+                // type it does *not* overflow nor give an unrepresentable result!
                 match bin_op {
-                    Rem | Div => {
-                        // int_min / -1
+                    Rem => {
                         if r == -1 && l == (1 << (size.bits() - 1)) {
-                            return Ok((Scalar::from_uint(l, size), true, left_layout.ty));
+                            return Ok((Scalar::from_int(0, size), true, left_layout.ty));
                         }
                     }
                     _ => {}
                 }
-                trace!("{}, {}, {}", l, l128, r);
-                let (result, mut oflo) = op(l128, r);
-                trace!("{}, {}", result, oflo);
-                if !oflo && size.bits() != 128 {
-                    let max = 1 << (size.bits() - 1);
-                    oflo = result >= max || result < -max;
-                }
-                // this may be out-of-bounds for the result type, so we have to truncate ourselves
+
+                let (result, oflo) = op(l128, r);
+                // This may be out-of-bounds for the result type, so we have to truncate ourselves.
+                // If that truncation loses any information, we have an overflow.
                 let result = result as u128;
                 let truncated = self.truncate(result, left_layout);
-                return Ok((Scalar::from_uint(truncated, size), oflo, left_layout.ty));
+                return Ok((
+                    Scalar::from_uint(truncated, size),
+                    oflo || self.sign_extend(truncated, left_layout) != result,
+                    left_layout.ty,
+                ));
             }
         }
 
-        let size = left_layout.size;
-
         let (val, ty) = match bin_op {
             Eq => (Scalar::from_bool(l == r), self.tcx.types.bool),
             Ne => (Scalar::from_bool(l != r), self.tcx.types.bool),
@@ -247,6 +246,8 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                     _ => bug!(),
                 };
                 let (result, oflo) = op(l, r);
+                // Truncate to target type.
+                // If that truncation loses any information, we have an overflow.
                 let truncated = self.truncate(result, left_layout);
                 return Ok((
                     Scalar::from_uint(truncated, size),
@@ -341,7 +342,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
         }
     }
 
-    /// Typed version of `checked_binary_op`, returning an `ImmTy`. Also ignores overflows.
+    /// Typed version of `overflowing_binary_op`, returning an `ImmTy`. Also ignores overflows.
     #[inline]
     pub fn binary_op(
         &self,
@@ -353,11 +354,13 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
         Ok(ImmTy::from_scalar(val, self.layout_of(ty)?))
     }
 
-    pub fn unary_op(
+    /// Returns the result of the specified operation, whether it overflowed, and
+    /// the result type.
+    pub fn overflowing_unary_op(
         &self,
         un_op: mir::UnOp,
         val: ImmTy<'tcx, M::PointerTag>,
-    ) -> InterpResult<'tcx, ImmTy<'tcx, M::PointerTag>> {
+    ) -> InterpResult<'tcx, (Scalar<M::PointerTag>, bool, Ty<'tcx>)> {
         use rustc::mir::UnOp::*;
 
         let layout = val.layout;
@@ -371,29 +374,44 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                     Not => !val,
                     _ => bug!("Invalid bool op {:?}", un_op),
                 };
-                Ok(ImmTy::from_scalar(Scalar::from_bool(res), self.layout_of(self.tcx.types.bool)?))
+                Ok((Scalar::from_bool(res), false, self.tcx.types.bool))
             }
             ty::Float(fty) => {
                 let res = match (un_op, fty) {
                     (Neg, FloatTy::F32) => Scalar::from_f32(-val.to_f32()?),
                     (Neg, FloatTy::F64) => Scalar::from_f64(-val.to_f64()?),
                     _ => bug!("Invalid float op {:?}", un_op),
                 };
-                Ok(ImmTy::from_scalar(res, layout))
+                Ok((res, false, layout.ty))
             }
             _ => {
                 assert!(layout.ty.is_integral());
                 let val = self.force_bits(val, layout.size)?;
-                let res = match un_op {
-                    Not => !val,
+                let (res, overflow) = match un_op {
+                    Not => (self.truncate(!val, layout), false), // bitwise negation, then truncate
                     Neg => {
+                        // arithmetic negation
                         assert!(layout.abi.is_signed());
-                        (-(val as i128)) as u128
+                        let val = self.sign_extend(val, layout) as i128;
+                        let (res, overflow) = val.overflowing_neg();
+                        let res = res as u128;
+                        // Truncate to target type.
+                        // If that truncation loses any information, we have an overflow.
+                        let truncated = self.truncate(res, layout);
+                        (truncated, overflow || self.sign_extend(truncated, layout) != res)
                     }
                 };
-                // res needs tuncating
-                Ok(ImmTy::from_uint(self.truncate(res, layout), layout))
+                Ok((Scalar::from_uint(res, layout.size), overflow, layout.ty))
             }
         }
     }
+
+    pub fn unary_op(
+        &self,
+        un_op: mir::UnOp,
+        val: ImmTy<'tcx, M::PointerTag>,
+    ) -> InterpResult<'tcx, ImmTy<'tcx, M::PointerTag>> {
+        let (val, _overflow, ty) = self.overflowing_unary_op(un_op, val)?;
+        Ok(ImmTy::from_scalar(val, self.layout_of(ty)?))
+    }
 }

src/librustc_mir/transform/const_prop.rs

@@ -518,18 +518,19 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
         }
     }
 
-    fn check_unary_op(&mut self, arg: &Operand<'tcx>, source_info: SourceInfo) -> Option<()> {
+    fn check_unary_op(
+        &mut self,
+        op: UnOp,
+        arg: &Operand<'tcx>,
+        source_info: SourceInfo,
+    ) -> Option<()> {
         self.use_ecx(source_info, |this| {
-            let ty = arg.ty(&this.local_decls, this.tcx);
-
-            if ty.is_integral() {
-                let arg = this.ecx.eval_operand(arg, None)?;
-                let prim = this.ecx.read_immediate(arg)?;
-                // Need to do overflow check here: For actual CTFE, MIR
-                // generation emits code that does this before calling the op.
-                if prim.to_bits()? == (1 << (prim.layout.size.bits() - 1)) {
-                    throw_panic!(OverflowNeg)
-                }
+            let val = this.ecx.read_immediate(this.ecx.eval_operand(arg, None)?)?;
+            let (_res, overflow, _ty) = this.ecx.overflowing_unary_op(op, val)?;
+
+            if overflow {
+                assert_eq!(op, UnOp::Neg, "Neg is the only UnOp that can overflow");
+                throw_panic!(OverflowNeg);
             }
 
             Ok(())
@@ -576,11 +577,10 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
         if !overflow_check {
             self.use_ecx(source_info, |this| {
                 let l = this.ecx.read_immediate(this.ecx.eval_operand(left, None)?)?;
-                let (_, overflow, _ty) = this.ecx.overflowing_binary_op(op, l, r)?;
+                let (_res, overflow, _ty) = this.ecx.overflowing_binary_op(op, l, r)?;
 
                 if overflow {
-                    let err = err_panic!(Overflow(op)).into();
-                    return Err(err);
+                    throw_panic!(Overflow(op));
                 }
 
                 Ok(())
@@ -620,9 +620,9 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
             // Additional checking: if overflow checks are disabled (which is usually the case in
             // release mode), then we need to do additional checking here to give lints to the user
             // if an overflow would occur.
-            Rvalue::UnaryOp(UnOp::Neg, arg) if !overflow_check => {
-                trace!("checking UnaryOp(op = Neg, arg = {:?})", arg);
-                self.check_unary_op(arg, source_info)?;
+            Rvalue::UnaryOp(op, arg) if !overflow_check => {
+                trace!("checking UnaryOp(op = {:?}, arg = {:?})", op, arg);
+                self.check_unary_op(*op, arg, source_info)?;
             }
 
             // Additional checking: check for overflows on integer binary operations and report

src/test/ui/consts/const-err2.rs

@@ -17,16 +17,22 @@ fn black_box<T>(_: T) {
 fn main() {
     let a = -std::i8::MIN;
     //~^ ERROR const_err
+    let a_i128 = -std::i128::MIN;
+    //~^ ERROR const_err
     let b = 200u8 + 200u8 + 200u8;
     //~^ ERROR const_err
+    let b_i128 = std::i128::MIN - std::i128::MAX;
+    //~^ ERROR const_err
     let c = 200u8 * 4;
     //~^ ERROR const_err
     let d = 42u8 - (42u8 + 1);
     //~^ ERROR const_err
     let _e = [5u8][1];
-    //~^ ERROR index out of bounds
+    //~^ ERROR const_err
     black_box(a);
+    black_box(a_i128);
     black_box(b);
+    black_box(b_i128);
     black_box(c);
     black_box(d);
 }

src/test/ui/consts/const-err2.stderr

@@ -11,28 +11,40 @@ LL | #![deny(const_err)]
    |         ^^^^^^^^^
 
 error: this expression will panic at runtime
-  --> $DIR/const-err2.rs:20:13
+  --> $DIR/const-err2.rs:20:18
+   |
+LL |     let a_i128 = -std::i128::MIN;
+   |                  ^^^^^^^^^^^^^^^ attempt to negate with overflow
+
+error: this expression will panic at runtime
+  --> $DIR/const-err2.rs:22:13
    |
 LL |     let b = 200u8 + 200u8 + 200u8;
    |             ^^^^^^^^^^^^^ attempt to add with overflow
 
 error: this expression will panic at runtime
-  --> $DIR/const-err2.rs:22:13
+  --> $DIR/const-err2.rs:24:18
+   |
+LL |     let b_i128 = std::i128::MIN - std::i128::MAX;
+   |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ attempt to subtract with overflow
+
+error: this expression will panic at runtime
+  --> $DIR/const-err2.rs:26:13
    |
 LL |     let c = 200u8 * 4;
    |             ^^^^^^^^^ attempt to multiply with overflow
 
 error: this expression will panic at runtime
-  --> $DIR/const-err2.rs:24:13
+  --> $DIR/const-err2.rs:28:13
    |
 LL |     let d = 42u8 - (42u8 + 1);
    |             ^^^^^^^^^^^^^^^^^ attempt to subtract with overflow
 
 error: index out of bounds: the len is 1 but the index is 1
-  --> $DIR/const-err2.rs:26:14
+  --> $DIR/const-err2.rs:30:14
    |
 LL |     let _e = [5u8][1];
    |              ^^^^^^^^
 
-error: aborting due to 5 previous errors
+error: aborting due to 7 previous errors
 

src/test/ui/consts/const-err3.rs

@@ -17,16 +17,22 @@ fn black_box<T>(_: T) {
 fn main() {
     let a = -std::i8::MIN;
     //~^ ERROR const_err
+    let a_i128 = -std::i128::MIN;
+    //~^ ERROR const_err
     let b = 200u8 + 200u8 + 200u8;
     //~^ ERROR const_err
+    let b_i128 = std::i128::MIN - std::i128::MAX;
+    //~^ ERROR const_err
     let c = 200u8 * 4;
     //~^ ERROR const_err
     let d = 42u8 - (42u8 + 1);
     //~^ ERROR const_err
     let _e = [5u8][1];
     //~^ ERROR const_err
     black_box(a);
+    black_box(a_i128);
     black_box(b);
+    black_box(b_i128);
     black_box(c);
     black_box(d);
 }

src/test/ui/consts/const-err3.stderr

@@ -10,29 +10,41 @@ note: the lint level is defined here
 LL | #![deny(const_err)]
    |         ^^^^^^^^^
 
+error: attempt to negate with overflow
+  --> $DIR/const-err3.rs:20:18
+   |
+LL |     let a_i128 = -std::i128::MIN;
+   |                  ^^^^^^^^^^^^^^^
+
 error: attempt to add with overflow
-  --> $DIR/const-err3.rs:20:13
+  --> $DIR/const-err3.rs:22:13
    |
 LL |     let b = 200u8 + 200u8 + 200u8;
    |             ^^^^^^^^^^^^^
 
+error: attempt to subtract with overflow
+  --> $DIR/const-err3.rs:24:18
+   |
+LL |     let b_i128 = std::i128::MIN - std::i128::MAX;
+   |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
 error: attempt to multiply with overflow
-  --> $DIR/const-err3.rs:22:13
+  --> $DIR/const-err3.rs:26:13
    |
 LL |     let c = 200u8 * 4;
    |             ^^^^^^^^^
 
 error: attempt to subtract with overflow
-  --> $DIR/const-err3.rs:24:13
+  --> $DIR/const-err3.rs:28:13
    |
 LL |     let d = 42u8 - (42u8 + 1);
    |             ^^^^^^^^^^^^^^^^^
 
 error: index out of bounds: the len is 1 but the index is 1
-  --> $DIR/const-err3.rs:26:14
+  --> $DIR/const-err3.rs:30:14
    |
 LL |     let _e = [5u8][1];
    |              ^^^^^^^^
 
-error: aborting due to 5 previous errors
+error: aborting due to 7 previous errors
 

src/test/ui/consts/const-int-arithmetic-overflow.rs

@@ -0,0 +1,26 @@
+// run-pass
+// compile-flags: -O
+#![allow(const_err)]
+
+// Make sure arithmetic unary/binary ops actually return the right result, even when overflowing.
+// We have to put them in `const fn` and turn on optimizations to avoid overflow checks.
+
+const fn add(x: i8, y: i8) -> i8 { x+y }
+const fn sub(x: i8, y: i8) -> i8 { x-y }
+const fn mul(x: i8, y: i8) -> i8 { x*y }
+// div and rem are always checked, so we cannot test their result in case of oveflow.
+const fn neg(x: i8) -> i8 { -x }
+
+fn main() {
+    const ADD_OFLOW: i8 = add(100, 100);
+    assert_eq!(ADD_OFLOW, -56);
+
+    const SUB_OFLOW: i8 = sub(100, -100);
+    assert_eq!(SUB_OFLOW, -56);
+
+    const MUL_OFLOW: i8 = mul(-100, -2);
+    assert_eq!(MUL_OFLOW, -56);
+
+    const NEG_OFLOW: i8 = neg(-128);
+    assert_eq!(NEG_OFLOW, -128);
+}