Auto merge of #125359 - RalfJung:interpret-overflowing-ops, r=oli-obk

interpret: make overflowing binops just normal binops

Follow-up to rust-lang/rust#125173 (Cc `@scottmcm)`

Author: bors

src/concurrency/data_race.rs

@@ -648,17 +648,16 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> {
         place: &MPlaceTy<'tcx, Provenance>,
         rhs: &ImmTy<'tcx, Provenance>,
         op: mir::BinOp,
-        neg: bool,
+        not: bool,
         atomic: AtomicRwOrd,
     ) -> InterpResult<'tcx, ImmTy<'tcx, Provenance>> {
         let this = self.eval_context_mut();
         this.atomic_access_check(place, AtomicAccessType::Rmw)?;
 
         let old = this.allow_data_races_mut(|this| this.read_immediate(place))?;
 
-        // Atomics wrap around on overflow.
-        let val = this.wrapping_binary_op(op, &old, rhs)?;
-        let val = if neg { this.wrapping_unary_op(mir::UnOp::Not, &val)? } else { val };
+        let val = this.binary_op(op, &old, rhs)?;
+        let val = if not { this.unary_op(mir::UnOp::Not, &val)? } else { val };
         this.allow_data_races_mut(|this| this.write_immediate(*val, place))?;
 
         this.validate_atomic_rmw(place, atomic)?;
@@ -700,7 +699,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> {
         this.atomic_access_check(place, AtomicAccessType::Rmw)?;
 
         let old = this.allow_data_races_mut(|this| this.read_immediate(place))?;
-        let lt = this.wrapping_binary_op(mir::BinOp::Lt, &old, &rhs)?.to_scalar().to_bool()?;
+        let lt = this.binary_op(mir::BinOp::Lt, &old, &rhs)?.to_scalar().to_bool()?;
 
         #[rustfmt::skip] // rustfmt makes this unreadable
         let new_val = if min {
@@ -744,7 +743,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> {
         // Read as immediate for the sake of `binary_op()`
         let old = this.allow_data_races_mut(|this| this.read_immediate(place))?;
         // `binary_op` will bail if either of them is not a scalar.
-        let eq = this.wrapping_binary_op(mir::BinOp::Eq, &old, expect_old)?;
+        let eq = this.binary_op(mir::BinOp::Eq, &old, expect_old)?;
         // If the operation would succeed, but is "weak", fail some portion
         // of the time, based on `success_rate`.
         let success_rate = 1.0 - this.machine.cmpxchg_weak_failure_rate;

src/intrinsics/atomic.rs

@@ -4,8 +4,8 @@ use crate::*;
 use helpers::check_arg_count;
 
 pub enum AtomicOp {
-    /// The `bool` indicates whether the result of the operation should be negated
-    /// (must be a boolean-typed operation).
+    /// The `bool` indicates whether the result of the operation should be negated (`UnOp::Not`,
+    /// must be a boolean-typed operation).
     MirOp(mir::BinOp, bool),
     Max,
     Min,
@@ -213,8 +213,8 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> {
                 this.write_immediate(*old, dest)?; // old value is returned
                 Ok(())
             }
-            AtomicOp::MirOp(op, neg) => {
-                let old = this.atomic_rmw_op_immediate(&place, &rhs, op, neg, atomic)?;
+            AtomicOp::MirOp(op, not) => {
+                let old = this.atomic_rmw_op_immediate(&place, &rhs, op, not, atomic)?;
                 this.write_immediate(*old, dest)?; // old value is returned
                 Ok(())
             }

src/intrinsics/mod.rs

@@ -365,8 +365,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                     "frem_algebraic" => mir::BinOp::Rem,
                     _ => bug!(),
                 };
-                let res = this.wrapping_binary_op(op, &a, &b)?;
-                // `wrapping_binary_op` already called `generate_nan` if necessary.
+                let res = this.binary_op(op, &a, &b)?;
+                // `binary_op` already called `generate_nan` if necessary.
                 this.write_immediate(*res, dest)?;
             }
 
@@ -411,12 +411,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                     ),
                     _ => {}
                 }
-                let res = this.wrapping_binary_op(op, &a, &b)?;
+                let res = this.binary_op(op, &a, &b)?;
                 if !float_finite(&res)? {
                     throw_ub_format!("`{intrinsic_name}` intrinsic produced non-finite value as result");
                 }
                 // This cannot be a NaN so we also don't have to apply any non-determinism.
-                // (Also, `wrapping_binary_op` already called `generate_nan` if needed.)
+                // (Also, `binary_op` already called `generate_nan` if needed.)
                 this.write_immediate(*res, dest)?;
             }
 

src/intrinsics/simd.rs

@@ -1,3 +1,5 @@
+use either::Either;
+
 use rustc_apfloat::{Float, Round};
 use rustc_middle::ty::layout::{HasParamEnv, LayoutOf};
 use rustc_middle::{mir, ty, ty::FloatTy};
@@ -82,7 +84,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                     let val = match which {
                         Op::MirOp(mir_op) => {
                             // This already does NaN adjustments
-                            this.wrapping_unary_op(mir_op, &op)?.to_scalar()
+                            this.unary_op(mir_op, &op)?.to_scalar()
                         }
                         Op::Abs => {
                             // Works for f32 and f64.
@@ -217,8 +219,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                     "mul" => Op::MirOp(BinOp::Mul),
                     "div" => Op::MirOp(BinOp::Div),
                     "rem" => Op::MirOp(BinOp::Rem),
-                    "shl" => Op::MirOp(BinOp::Shl),
-                    "shr" => Op::MirOp(BinOp::Shr),
+                    "shl" => Op::MirOp(BinOp::ShlUnchecked),
+                    "shr" => Op::MirOp(BinOp::ShrUnchecked),
                     "and" => Op::MirOp(BinOp::BitAnd),
                     "or" => Op::MirOp(BinOp::BitOr),
                     "xor" => Op::MirOp(BinOp::BitXor),
@@ -243,15 +245,19 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                     let val = match which {
                         Op::MirOp(mir_op) => {
                             // This does NaN adjustments.
-                            let (val, overflowed) = this.overflowing_binary_op(mir_op, &left, &right)?;
-                            if matches!(mir_op, BinOp::Shl | BinOp::Shr) {
-                                // Shifts have extra UB as SIMD operations that the MIR binop does not have.
-                                // See <https://github.com/rust-lang/rust/issues/91237>.
-                                if overflowed {
-                                    let r_val = right.to_scalar().to_bits(right.layout.size)?;
-                                    throw_ub_format!("overflowing shift by {r_val} in `simd_{intrinsic_name}` in SIMD lane {i}");
+                            let val = this.binary_op(mir_op, &left, &right).map_err(|err| {
+                                match err.kind() {
+                                    InterpError::UndefinedBehavior(UndefinedBehaviorInfo::ShiftOverflow { shift_amount, .. }) => {
+                                        // This resets the interpreter backtrace, but it's not worth avoiding that.
+                                        let shift_amount = match shift_amount {
+                                            Either::Left(v) => v.to_string(),
+                                            Either::Right(v) => v.to_string(),
+                                        };
+                                        err_ub_format!("overflowing shift by {shift_amount} in `simd_{intrinsic_name}` in lane {i}").into()
+                                    }
+                                    _ => err
                                 }
-                            }
+                            })?;
                             if matches!(mir_op, BinOp::Eq | BinOp::Ne | BinOp::Lt | BinOp::Le | BinOp::Gt | BinOp::Ge) {
                                 // Special handling for boolean-returning operations
                                 assert_eq!(val.layout.ty, this.tcx.types.bool);
@@ -370,11 +376,11 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                     let op = this.read_immediate(&this.project_index(&op, i)?)?;
                     res = match which {
                         Op::MirOp(mir_op) => {
-                            this.wrapping_binary_op(mir_op, &res, &op)?
+                            this.binary_op(mir_op, &res, &op)?
                         }
                         Op::MirOpBool(mir_op) => {
                             let op = imm_from_bool(simd_element_to_bool(op)?);
-                            this.wrapping_binary_op(mir_op, &res, &op)?
+                            this.binary_op(mir_op, &res, &op)?
                         }
                         Op::MinMax(mmop) => {
                             if matches!(res.layout.ty.kind(), ty::Float(_)) {
@@ -385,7 +391,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                                     MinMax::Min => BinOp::Le,
                                     MinMax::Max => BinOp::Ge,
                                 };
-                                if this.wrapping_binary_op(mirop, &res, &op)?.to_scalar().to_bool()? {
+                                if this.binary_op(mirop, &res, &op)?.to_scalar().to_bool()? {
                                     res
                                 } else {
                                     op
@@ -414,7 +420,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                 let mut res = init;
                 for i in 0..op_len {
                     let op = this.read_immediate(&this.project_index(&op, i)?)?;
-                    res = this.wrapping_binary_op(mir_op, &res, &op)?;
+                    res = this.binary_op(mir_op, &res, &op)?;
                 }
                 this.write_immediate(*res, dest)?;
             }

src/machine.rs

@@ -1025,7 +1025,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> {
         bin_op: mir::BinOp,
         left: &ImmTy<'tcx, Provenance>,
         right: &ImmTy<'tcx, Provenance>,
-    ) -> InterpResult<'tcx, (ImmTy<'tcx, Provenance>, bool)> {
+    ) -> InterpResult<'tcx, ImmTy<'tcx, Provenance>> {
         ecx.binary_ptr_op(bin_op, left, right)
     }
 

src/operator.rs

@@ -14,7 +14,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         bin_op: mir::BinOp,
         left: &ImmTy<'tcx, Provenance>,
         right: &ImmTy<'tcx, Provenance>,
-    ) -> InterpResult<'tcx, (ImmTy<'tcx, Provenance>, bool)> {
+    ) -> InterpResult<'tcx, ImmTy<'tcx, Provenance>> {
         use rustc_middle::mir::BinOp::*;
 
         let this = self.eval_context_ref();
@@ -45,7 +45,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                     Ge => left >= right,
                     _ => bug!(),
                 };
-                (ImmTy::from_bool(res, *this.tcx), false)
+                ImmTy::from_bool(res, *this.tcx)
             }
 
             // Some more operations are possible with atomics.
@@ -60,16 +60,14 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                     right.to_scalar().to_target_usize(this)?,
                     this.machine.layouts.usize,
                 );
-                let (result, overflowing) = this.overflowing_binary_op(bin_op, &left, &right)?;
+                let result = this.binary_op(bin_op, &left, &right)?;
                 // Construct a new pointer with the provenance of `ptr` (the LHS).
                 let result_ptr = Pointer::new(
                     ptr.provenance,
                     Size::from_bytes(result.to_scalar().to_target_usize(this)?),
                 );
-                (
-                    ImmTy::from_scalar(Scalar::from_maybe_pointer(result_ptr, this), left.layout),
-                    overflowing,
-                )
+
+                ImmTy::from_scalar(Scalar::from_maybe_pointer(result_ptr, this), left.layout)
             }
 
             _ => span_bug!(this.cur_span(), "Invalid operator on pointers: {:?}", bin_op),

src/shims/x86/mod.rs

@@ -50,13 +50,16 @@ pub(super) trait EvalContextExt<'mir, 'tcx: 'mir>:
                 let a = this.read_immediate(a)?;
                 let b = this.read_immediate(b)?;
 
-                let (sum, overflow1) = this.overflowing_binary_op(mir::BinOp::Add, &a, &b)?;
-                let (sum, overflow2) = this.overflowing_binary_op(
-                    mir::BinOp::Add,
-                    &sum,
-                    &ImmTy::from_uint(c_in, a.layout),
-                )?;
-                let c_out = overflow1 | overflow2;
+                let (sum, overflow1) =
+                    this.binary_op(mir::BinOp::AddWithOverflow, &a, &b)?.to_pair(this);
+                let (sum, overflow2) = this
+                    .binary_op(
+                        mir::BinOp::AddWithOverflow,
+                        &sum,
+                        &ImmTy::from_uint(c_in, a.layout),
+                    )?
+                    .to_pair(this);
+                let c_out = overflow1.to_scalar().to_bool()? | overflow2.to_scalar().to_bool()?;
 
                 this.write_scalar(Scalar::from_u8(c_out.into()), &this.project_field(dest, 0)?)?;
                 this.write_immediate(*sum, &this.project_field(dest, 1)?)?;
@@ -76,13 +79,11 @@ pub(super) trait EvalContextExt<'mir, 'tcx: 'mir>:
                 let a = this.read_immediate(a)?;
                 let b = this.read_immediate(b)?;
 
-                let (sub, overflow1) = this.overflowing_binary_op(mir::BinOp::Sub, &a, &b)?;
-                let (sub, overflow2) = this.overflowing_binary_op(
-                    mir::BinOp::Sub,
-                    &sub,
-                    &ImmTy::from_uint(b_in, a.layout),
-                )?;
-                let b_out = overflow1 | overflow2;
+                let (sub, overflow1) = this.binary_op(mir::BinOp::SubWithOverflow, &a, &b)?.to_pair(this);
+                let (sub, overflow2) = this
+                    .binary_op(mir::BinOp::SubWithOverflow, &sub, &ImmTy::from_uint(b_in, a.layout))?
+                    .to_pair(this);
+                let b_out = overflow1.to_scalar().to_bool()? | overflow2.to_scalar().to_bool()?;
 
                 this.write_scalar(Scalar::from_u8(b_out.into()), &this.project_field(dest, 0)?)?;
                 this.write_immediate(*sub, &this.project_field(dest, 1)?)?;
@@ -245,7 +246,7 @@ fn bin_op_float<'tcx, F: rustc_apfloat::Float>(
 ) -> InterpResult<'tcx, Scalar<Provenance>> {
     match which {
         FloatBinOp::Arith(which) => {
-            let res = this.wrapping_binary_op(which, left, right)?;
+            let res = this.binary_op(which, left, right)?;
             Ok(res.to_scalar())
         }
         FloatBinOp::Cmp { gt, lt, eq, unord } => {
@@ -744,12 +745,9 @@ fn int_abs<'tcx>(
         let op = this.read_immediate(&this.project_index(&op, i)?)?;
         let dest = this.project_index(&dest, i)?;
 
-        let lt_zero = this.wrapping_binary_op(mir::BinOp::Lt, &op, &zero)?;
-        let res = if lt_zero.to_scalar().to_bool()? {
-            this.wrapping_unary_op(mir::UnOp::Neg, &op)?
-        } else {
-            op
-        };
+        let lt_zero = this.binary_op(mir::BinOp::Lt, &op, &zero)?;
+        let res =
+            if lt_zero.to_scalar().to_bool()? { this.unary_op(mir::UnOp::Neg, &op)? } else { op };
 
         this.write_immediate(*res, &dest)?;
     }
@@ -832,7 +830,7 @@ fn horizontal_bin_op<'tcx>(
             let res = if saturating {
                 Immediate::from(this.saturating_arith(which, &lhs, &rhs)?)
             } else {
-                *this.wrapping_binary_op(which, &lhs, &rhs)?
+                *this.binary_op(which, &lhs, &rhs)?
             };
 
             this.write_immediate(res, &this.project_index(&dest, j)?)?;
@@ -884,8 +882,8 @@ fn conditional_dot_product<'tcx>(
                 let left = this.read_immediate(&this.project_index(&left, j)?)?;
                 let right = this.read_immediate(&this.project_index(&right, j)?)?;
 
-                let mul = this.wrapping_binary_op(mir::BinOp::Mul, &left, &right)?;
-                sum = this.wrapping_binary_op(mir::BinOp::Add, &sum, &mul)?;
+                let mul = this.binary_op(mir::BinOp::Mul, &left, &right)?;
+                sum = this.binary_op(mir::BinOp::Add, &sum, &mul)?;
             }
         }
 
@@ -1276,11 +1274,8 @@ fn psign<'tcx>(
         let left = this.read_immediate(&this.project_index(&left, i)?)?;
         let right = this.read_scalar(&this.project_index(&right, i)?)?.to_int(dest.layout.size)?;
 
-        let res = this.wrapping_binary_op(
-            mir::BinOp::Mul,
-            &left,
-            &ImmTy::from_int(right.signum(), dest.layout),
-        )?;
+        let res =
+            this.binary_op(mir::BinOp::Mul, &left, &ImmTy::from_int(right.signum(), dest.layout))?;
 
         this.write_immediate(*res, &dest)?;
     }

tests/fail/intrinsics/simd-shl-too-far.rs

@@ -10,6 +10,6 @@ fn main() {
     unsafe {
         let x = i32x2(1, 1);
         let y = i32x2(100, 0);
-        simd_shl(x, y); //~ERROR: overflowing shift by 100 in `simd_shl` in SIMD lane 0
+        simd_shl(x, y); //~ERROR: overflowing shift by 100 in `simd_shl` in lane 0
     }
 }

tests/fail/intrinsics/simd-shl-too-far.stderr

@@ -1,8 +1,8 @@
-error: Undefined Behavior: overflowing shift by 100 in `simd_shl` in SIMD lane 0
+error: Undefined Behavior: overflowing shift by 100 in `simd_shl` in lane 0
   --> $DIR/simd-shl-too-far.rs:LL:CC
    |
 LL |         simd_shl(x, y);
-   |         ^^^^^^^^^^^^^^ overflowing shift by 100 in `simd_shl` in SIMD lane 0
+   |         ^^^^^^^^^^^^^^ overflowing shift by 100 in `simd_shl` in lane 0
    |
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
    = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information

tests/fail/intrinsics/simd-shr-too-far.rs

@@ -10,6 +10,6 @@ fn main() {
     unsafe {
         let x = i32x2(1, 1);
         let y = i32x2(20, 40);
-        simd_shr(x, y); //~ERROR: overflowing shift by 40 in `simd_shr` in SIMD lane 1
+        simd_shr(x, y); //~ERROR: overflowing shift by 40 in `simd_shr` in lane 1
     }
 }