interpret: simplify pointer arithmetic logic

Author: RalfJung

compiler/rustc_abi/src/lib.rs

@@ -517,7 +517,7 @@ impl Size {
     /// Truncates `value` to `self` bits and then sign-extends it to 128 bits
     /// (i.e., if it is negative, fill with 1's on the left).
     #[inline]
-    pub fn sign_extend(self, value: u128) -> u128 {
+    pub fn sign_extend(self, value: u128) -> i128 {
         let size = self.bits();
         if size == 0 {
             // Truncated until nothing is left.
@@ -527,7 +527,7 @@ impl Size {
         let shift = 128 - size;
         // Shift the unsigned value to the left, then shift back to the right as signed
         // (essentially fills with sign bit on the left).
-        (((value << shift) as i128) >> shift) as u128
+        ((value << shift) as i128) >> shift
     }
 
     /// Truncates `value` to `self` bits.
@@ -545,7 +545,7 @@ impl Size {
 
     #[inline]
     pub fn signed_int_min(&self) -> i128 {
-        self.sign_extend(1_u128 << (self.bits() - 1)) as i128
+        self.sign_extend(1_u128 << (self.bits() - 1))
     }
 
     #[inline]

compiler/rustc_const_eval/src/interpret/eval_context.rs

@@ -561,17 +561,6 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
         self.frame().body
     }
 
-    #[inline(always)]
-    pub fn sign_extend(&self, value: u128, ty: TyAndLayout<'_>) -> u128 {
-        assert!(ty.abi.is_signed());
-        ty.size.sign_extend(value)
-    }
-
-    #[inline(always)]
-    pub fn truncate(&self, value: u128, ty: TyAndLayout<'_>) -> u128 {
-        ty.size.truncate(value)
-    }
-
     #[inline]
     pub fn type_is_freeze(&self, ty: Ty<'tcx>) -> bool {
         ty.is_freeze(*self.tcx, self.param_env)

compiler/rustc_const_eval/src/interpret/intrinsics.rs

@@ -211,7 +211,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
                 } else {
                     (val_bits >> shift_bits) | (val_bits << inv_shift_bits)
                 };
-                let truncated_bits = self.truncate(result_bits, layout_val);
+                let truncated_bits = layout_val.size.truncate(result_bits);
                 let result = Scalar::from_uint(truncated_bits, layout_val.size);
                 self.write_scalar(result, dest)?;
             }
@@ -585,13 +585,10 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
         ptr: Pointer<Option<M::Provenance>>,
         offset_bytes: i64,
     ) -> InterpResult<'tcx, Pointer<Option<M::Provenance>>> {
-        // We first compute the pointer with overflow checks, to get a specific error for when it
-        // overflows (though technically this is redundant with the following inbounds check).
-        let result = ptr.signed_offset(offset_bytes, self)?;
         // The offset must be in bounds starting from `ptr`.
         self.check_ptr_access_signed(ptr, offset_bytes, CheckInAllocMsg::PointerArithmeticTest)?;
-        // Done.
-        Ok(result)
+        // This also implies that there is no overflow, so we are done.
+        Ok(ptr.wrapping_signed_offset(offset_bytes, self))
     }
 
     /// Copy `count*size_of::<T>()` many bytes from `*src` to `*dst`.

compiler/rustc_const_eval/src/interpret/memory.rs

@@ -477,7 +477,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
                     throw_ub!(PointerOutOfBounds {
                         alloc_id,
                         alloc_size,
-                        ptr_offset: self.target_usize_to_isize(offset),
+                        ptr_offset: self.sign_extend_to_target_isize(offset),
                         inbounds_size: size,
                         msg,
                     })

compiler/rustc_const_eval/src/interpret/place.rs

@@ -291,10 +291,8 @@ impl<'tcx, Prov: Provenance> Projectable<'tcx, Prov> for PlaceTy<'tcx, Prov> {
                 // projections are type-checked and bounds-checked.
                 assert!(offset + layout.size <= self.layout.size);
 
-                let new_offset = Size::from_bytes(
-                    ecx.data_layout()
-                        .offset(old_offset.unwrap_or(Size::ZERO).bytes(), offset.bytes())?,
-                );
+                // Size `+`, ensures no overflow.
+                let new_offset = old_offset.unwrap_or(Size::ZERO) + offset;
 
                 PlaceTy {
                     place: Place::Local { local, offset: Some(new_offset), locals_addr },

compiler/rustc_const_eval/src/interpret/step.rs

@@ -364,7 +364,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
             // of the first element.
             let elem_size = first.layout.size;
             let first_ptr = first.ptr();
-            let rest_ptr = first_ptr.offset(elem_size, self)?;
+            let rest_ptr = first_ptr.wrapping_offset(elem_size, self);
             // No alignment requirement since `copy_op` above already checked it.
             self.mem_copy_repeatedly(
                 first_ptr,

compiler/rustc_lint/src/types.rs

@@ -315,11 +315,7 @@ fn report_bin_hex_error(
 ) {
     let (t, actually) = match ty {
         attr::IntType::SignedInt(t) => {
-            let actually = if negative {
-                -(size.sign_extend(val) as i128)
-            } else {
-                size.sign_extend(val) as i128
-            };
+            let actually = if negative { -(size.sign_extend(val)) } else { size.sign_extend(val) };
             (t.name_str(), actually.to_string())
         }
         attr::IntType::UnsignedInt(t) => {

compiler/rustc_middle/src/mir/interpret/pointer.rs

@@ -1,4 +1,4 @@
-use super::{AllocId, InterpResult};
+use super::AllocId;
 
 use rustc_data_structures::static_assert_size;
 use rustc_macros::{HashStable, TyDecodable, TyEncodable};
@@ -39,62 +39,13 @@ pub trait PointerArithmetic: HasDataLayout {
     }
 
     #[inline]
-    fn target_usize_to_isize(&self, val: u64) -> i64 {
-        let val = val as i64;
-        // Now wrap-around into the machine_isize range.
-        if val > self.target_isize_max() {
-            // This can only happen if the ptr size is < 64, so we know max_usize_plus_1 fits into
-            // i64.
-            debug_assert!(self.pointer_size().bits() < 64);
-            let max_usize_plus_1 = 1u128 << self.pointer_size().bits();
-            val - i64::try_from(max_usize_plus_1).unwrap()
-        } else {
-            val
-        }
-    }
-
-    /// Helper function: truncate given value-"overflowed flag" pair to pointer size and
-    /// update "overflowed flag" if there was an overflow.
-    /// This should be called by all the other methods before returning!
-    #[inline]
-    fn truncate_to_ptr(&self, (val, over): (u64, bool)) -> (u64, bool) {
-        let val = u128::from(val);
-        let max_ptr_plus_1 = 1u128 << self.pointer_size().bits();
-        (u64::try_from(val % max_ptr_plus_1).unwrap(), over || val >= max_ptr_plus_1)
-    }
-
-    #[inline]
-    fn overflowing_offset(&self, val: u64, i: u64) -> (u64, bool) {
-        // We do not need to check if i fits in a machine usize. If it doesn't,
-        // either the wrapping_add will wrap or res will not fit in a pointer.
-        let res = val.overflowing_add(i);
-        self.truncate_to_ptr(res)
-    }
-
-    #[inline]
-    fn overflowing_signed_offset(&self, val: u64, i: i64) -> (u64, bool) {
-        // We need to make sure that i fits in a machine isize.
-        let n = i.unsigned_abs();
-        if i >= 0 {
-            let (val, over) = self.overflowing_offset(val, n);
-            (val, over || i > self.target_isize_max())
-        } else {
-            let res = val.overflowing_sub(n);
-            let (val, over) = self.truncate_to_ptr(res);
-            (val, over || i < self.target_isize_min())
-        }
-    }
-
-    #[inline]
-    fn offset<'tcx>(&self, val: u64, i: u64) -> InterpResult<'tcx, u64> {
-        let (res, over) = self.overflowing_offset(val, i);
-        if over { throw_ub!(PointerArithOverflow) } else { Ok(res) }
+    fn truncate_to_target_usize(&self, val: u64) -> u64 {
+        self.pointer_size().truncate(val.into()).try_into().unwrap()
     }
 
     #[inline]
-    fn signed_offset<'tcx>(&self, val: u64, i: i64) -> InterpResult<'tcx, u64> {
-        let (res, over) = self.overflowing_signed_offset(val, i);
-        if over { throw_ub!(PointerArithOverflow) } else { Ok(res) }
+    fn sign_extend_to_target_isize(&self, val: u64) -> i64 {
+        self.pointer_size().sign_extend(val.into()).try_into().unwrap()
     }
 }
 
@@ -330,7 +281,7 @@ impl<Prov> Pointer<Option<Prov>> {
     }
 }
 
-impl<'tcx, Prov> Pointer<Prov> {
+impl<Prov> Pointer<Prov> {
     #[inline(always)]
     pub fn new(provenance: Prov, offset: Size) -> Self {
         Pointer { provenance, offset }
@@ -348,43 +299,16 @@ impl<'tcx, Prov> Pointer<Prov> {
         Pointer { provenance: f(self.provenance), ..self }
     }
 
-    #[inline]
-    pub fn offset(self, i: Size, cx: &impl HasDataLayout) -> InterpResult<'tcx, Self> {
-        Ok(Pointer {
-            offset: Size::from_bytes(cx.data_layout().offset(self.offset.bytes(), i.bytes())?),
-            ..self
-        })
-    }
-
-    #[inline]
-    pub fn overflowing_offset(self, i: Size, cx: &impl HasDataLayout) -> (Self, bool) {
-        let (res, over) = cx.data_layout().overflowing_offset(self.offset.bytes(), i.bytes());
-        let ptr = Pointer { offset: Size::from_bytes(res), ..self };
-        (ptr, over)
-    }
-
     #[inline(always)]
     pub fn wrapping_offset(self, i: Size, cx: &impl HasDataLayout) -> Self {
-        self.overflowing_offset(i, cx).0
-    }
-
-    #[inline]
-    pub fn signed_offset(self, i: i64, cx: &impl HasDataLayout) -> InterpResult<'tcx, Self> {
-        Ok(Pointer {
-            offset: Size::from_bytes(cx.data_layout().signed_offset(self.offset.bytes(), i)?),
-            ..self
-        })
-    }
-
-    #[inline]
-    pub fn overflowing_signed_offset(self, i: i64, cx: &impl HasDataLayout) -> (Self, bool) {
-        let (res, over) = cx.data_layout().overflowing_signed_offset(self.offset.bytes(), i);
-        let ptr = Pointer { offset: Size::from_bytes(res), ..self };
-        (ptr, over)
+        let res =
+            cx.data_layout().truncate_to_target_usize(self.offset.bytes().wrapping_add(i.bytes()));
+        Pointer { offset: Size::from_bytes(res), ..self }
     }
 
     #[inline(always)]
     pub fn wrapping_signed_offset(self, i: i64, cx: &impl HasDataLayout) -> Self {
-        self.overflowing_signed_offset(i, cx).0
+        // It's wrapping anyway, so we can just cast to `u64`.
+        self.wrapping_offset(Size::from_bytes(i as u64), cx)
     }
 }

compiler/rustc_middle/src/mir/interpret/value.rs

@@ -397,7 +397,7 @@ impl<'tcx, Prov: Provenance> Scalar<Prov> {
     #[inline]
     pub fn to_int(self, size: Size) -> InterpResult<'tcx, i128> {
         let b = self.to_bits(size)?;
-        Ok(size.sign_extend(b) as i128)
+        Ok(size.sign_extend(b))
     }
 
     /// Converts the scalar to produce an `i8`. Fails if the scalar is a pointer.

compiler/rustc_middle/src/ty/consts/int.rs

@@ -233,7 +233,7 @@ impl ScalarInt {
         let data = i.into();
         // `into` performed sign extension, we have to truncate
         let r = Self::raw(size.truncate(data as u128), size);
-        (r, size.sign_extend(r.data) as i128 != data)
+        (r, size.sign_extend(r.data) != data)
     }
 
     #[inline]
@@ -334,7 +334,7 @@ impl ScalarInt {
     #[inline]
     pub fn to_int(self, size: Size) -> i128 {
         let b = self.to_bits(size);
-        size.sign_extend(b) as i128
+        size.sign_extend(b)
     }
 
     /// Converts the `ScalarInt` to i8.

compiler/rustc_middle/src/ty/util.rs

@@ -78,7 +78,7 @@ impl<'tcx> Discr<'tcx> {
         let (val, oflo) = if signed {
             let min = size.signed_int_min();
             let max = size.signed_int_max();
-            let val = size.sign_extend(self.val) as i128;
+            let val = size.sign_extend(self.val);
             assert!(n < (i128::MAX as u128));
             let n = n as i128;
             let oflo = val > max - n;

src/tools/miri/src/alloc_addresses/mod.rs

@@ -11,7 +11,7 @@ use rand::Rng;
 
 use rustc_data_structures::fx::{FxHashMap, FxHashSet};
 use rustc_span::Span;
-use rustc_target::abi::{Align, HasDataLayout, Size};
+use rustc_target::abi::{Align, Size};
 
 use crate::{concurrency::VClock, *};
 
@@ -307,15 +307,15 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 
         let (prov, offset) = ptr.into_parts(); // offset is relative (AllocId provenance)
         let alloc_id = prov.alloc_id();
-        let base_addr = ecx.addr_from_alloc_id(alloc_id, kind)?;
 
-        // Add offset with the right kind of pointer-overflowing arithmetic.
-        let dl = ecx.data_layout();
-        let absolute_addr = dl.overflowing_offset(base_addr, offset.bytes()).0;
-        Ok(interpret::Pointer::new(
+        // Get a pointer to the beginning of this allocation.
+        let base_addr = ecx.addr_from_alloc_id(alloc_id, kind)?;
+        let base_ptr = interpret::Pointer::new(
             Provenance::Concrete { alloc_id, tag },
-            Size::from_bytes(absolute_addr),
-        ))
+            Size::from_bytes(base_addr),
+        );
+        // Add offset with the right kind of pointer-overflowing arithmetic.
+        Ok(base_ptr.wrapping_offset(offset, ecx))
     }
 
     /// When a pointer is used for a memory access, this computes where in which allocation the
@@ -341,12 +341,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         let base_addr = *ecx.machine.alloc_addresses.borrow().base_addr.get(&alloc_id).unwrap();
 
         // Wrapping "addr - base_addr"
-        #[allow(clippy::cast_possible_wrap)] // we want to wrap here
-        let neg_base_addr = (base_addr as i64).wrapping_neg();
-        Some((
-            alloc_id,
-            Size::from_bytes(ecx.overflowing_signed_offset(addr.bytes(), neg_base_addr).0),
-        ))
+        let rel_offset = ecx.truncate_to_target_usize(addr.bytes().wrapping_sub(base_addr));
+        Some((alloc_id, Size::from_bytes(rel_offset)))
     }
 }
 

src/tools/miri/src/helpers.rs

@@ -606,7 +606,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         }
         // The part between the end_ptr and the end of the place is also frozen.
         // So pretend there is a 0-sized `UnsafeCell` at the end.
-        unsafe_cell_action(&place.ptr().offset(size, this)?, Size::ZERO)?;
+        unsafe_cell_action(&place.ptr().wrapping_offset(size, this), Size::ZERO)?;
         // Done!
         return Ok(());
 
@@ -975,7 +975,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         loop {
             // FIXME: We are re-getting the allocation each time around the loop.
             // Would be nice if we could somehow "extend" an existing AllocRange.
-            let alloc = this.get_ptr_alloc(ptr.offset(len, this)?, size1)?.unwrap(); // not a ZST, so we will get a result
+            let alloc = this.get_ptr_alloc(ptr.wrapping_offset(len, this), size1)?.unwrap(); // not a ZST, so we will get a result
             let byte = alloc.read_integer(alloc_range(Size::ZERO, size1))?.to_u8()?;
             if byte == 0 {
                 break;
@@ -1039,7 +1039,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 break;
             } else {
                 wchars.push(wchar_int.try_into().unwrap());
-                ptr = ptr.offset(size, this)?;
+                ptr = ptr.wrapping_offset(size, this);
             }
         }
 

src/tools/miri/src/shims/foreign_items.rs

@@ -622,7 +622,7 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 {
                     let idx = u64::try_from(idx).unwrap();
                     #[allow(clippy::arithmetic_side_effects)] // idx < num, so this never wraps
-                    let new_ptr = ptr.offset(Size::from_bytes(num - idx - 1), this)?;
+                    let new_ptr = ptr.wrapping_offset(Size::from_bytes(num - idx - 1), this);
                     this.write_pointer(new_ptr, dest)?;
                 } else {
                     this.write_null(dest)?;
@@ -646,7 +646,7 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
                     .iter()
                     .position(|&c| c == val);
                 if let Some(idx) = idx {
-                    let new_ptr = ptr.offset(Size::from_bytes(idx as u64), this)?;
+                    let new_ptr = ptr.wrapping_offset(Size::from_bytes(idx as u64), this);
                     this.write_pointer(new_ptr, dest)?;
                 } else {
                     this.write_null(dest)?;

src/tools/miri/src/shims/unix/env.rs

@@ -82,7 +82,7 @@ impl<'tcx> UnixEnvVars<'tcx> {
         };
         // The offset is used to strip the "{name}=" part of the string.
         let var_ptr = var_ptr
-            .offset(Size::from_bytes(u64::try_from(name.len()).unwrap().strict_add(1)), ecx)?;
+            .wrapping_offset(Size::from_bytes(u64::try_from(name.len()).unwrap().strict_add(1)), ecx);
         Ok(Some(var_ptr))
     }
 

src/tools/miri/src/shims/unix/fs.rs

@@ -928,7 +928,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                     &this.ptr_to_mplace(entry, dirent64_layout),
                 )?;
 
-                let name_ptr = entry.offset(Size::from_bytes(d_name_offset), this)?;
+                let name_ptr = entry.wrapping_offset(Size::from_bytes(d_name_offset), this);
                 this.write_bytes_ptr(name_ptr, name_bytes.iter().copied())?;
 
                 Some(entry)

src/tools/miri/src/shims/unix/linux/mem.rs

@@ -53,7 +53,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
             // We just allocated this, the access is definitely in-bounds and fits into our address space.
             // mmap guarantees new mappings are zero-init.
             this.write_bytes_ptr(
-                ptr.offset(Size::from_bytes(old_size), this).unwrap().into(),
+                ptr.wrapping_offset(Size::from_bytes(old_size), this).into(),
                 std::iter::repeat(0u8).take(usize::try_from(increase).unwrap()),
             )
             .unwrap();