Rollup merge of #128482 - RalfJung:ptr-signed-offset, r=oli-obk

interpret: on a signed deref check, mention the right pointer in the error

When a negative offset (like `ptr.offset(-10)`) goes out-of-bounds, we currently show an error saying that we expect the *resulting* pointer to be inbounds for 10 bytes. That's confusing, so this PR makes it so that instead we say that we expect the *original* pointer `ptr` to have 10 bytes *to the left*.

I also realized I can simplify the pointer arithmetic logic and handling of "staying inbounds of a target `usize`" quite a bit; the second commit does that.

Author: matthiaskrgr

compiler/rustc_abi/src/lib.rs

@@ -516,7 +516,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.
@@ -526,7 +526,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.
@@ -544,7 +544,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/messages.ftl

@@ -88,10 +88,18 @@ const_eval_exact_div_has_remainder =
     exact_div: {$a} cannot be divided by {$b} without remainder
 
 const_eval_expected_inbounds_pointer =
-    expected {$inbounds_size ->
-        [0] a pointer to some allocation
-        [1] a pointer to 1 byte of memory
-        *[x] a pointer to {$inbounds_size} bytes of memory
+    expected a pointer to {$inbounds_size_abs ->
+        [0] some allocation
+        *[x] {$inbounds_size_is_neg ->
+            [false] {$inbounds_size_abs ->
+                    [1] 1 byte of memory
+                    *[x] {$inbounds_size_abs} bytes of memory
+                }
+            *[true] the end of {$inbounds_size_abs ->
+                    [1] 1 byte of memory
+                    *[x] {$inbounds_size_abs} bytes of memory
+                }
+        }
     }
 
 const_eval_extern_static =
@@ -243,7 +251,7 @@ const_eval_offset_from_different_allocations =
 const_eval_offset_from_overflow =
     `{$name}` called when first pointer is too far ahead of second
 const_eval_offset_from_test =
-    out-of-bounds `offset_from`
+    out-of-bounds `offset_from` origin
 const_eval_offset_from_underflow =
     `{$name}` called when first pointer is too far before second
 const_eval_offset_from_unsigned_overflow =
@@ -269,17 +277,24 @@ const_eval_partial_pointer_copy =
 const_eval_partial_pointer_overwrite =
     unable to overwrite parts of a pointer in memory at {$ptr}
 const_eval_pointer_arithmetic_overflow =
-    overflowing in-bounds pointer arithmetic
+    overflowing pointer arithmetic: the total offset in bytes does not fit in an `isize`
 const_eval_pointer_arithmetic_test = out-of-bounds pointer arithmetic
 const_eval_pointer_out_of_bounds =
     {$bad_pointer_message}: {const_eval_expected_inbounds_pointer}, but got {$pointer} {$ptr_offset_is_neg ->
         [true] which points to before the beginning of the allocation
-        *[false] {$alloc_size_minus_ptr_offset ->
-            [0] which is at or beyond the end of the allocation of size {$alloc_size ->
-                [1] 1 byte
-                *[x] {$alloc_size} bytes
+        *[false] {$inbounds_size_is_neg ->
+            [true] {$ptr_offset_abs ->
+                [0] which is at the beginning of the allocation
+                *[other] which does not have enough space to the beginning of the allocation
+            }
+            *[false] {$alloc_size_minus_ptr_offset ->
+                [0] which is at or beyond the end of the allocation of size {$alloc_size ->
+                    [1] 1 byte
+                    *[x] {$alloc_size} bytes
+                }
+                [1] which is only 1 byte from the end of the allocation
+                *[x] which is only {$alloc_size_minus_ptr_offset} bytes from the end of the allocation
             }
-            *[x] and there are only {$alloc_size_minus_ptr_offset} bytes starting at that pointer
         }
     }
 const_eval_pointer_use_after_free =

compiler/rustc_const_eval/src/const_eval/machine.rs

@@ -295,7 +295,7 @@ impl<'tcx> CompileTimeInterpCx<'tcx> {
             );
         }
 
-        match self.ptr_try_get_alloc_id(ptr) {
+        match self.ptr_try_get_alloc_id(ptr, 0) {
             Ok((alloc_id, offset, _extra)) => {
                 let (_size, alloc_align, _kind) = self.get_alloc_info(alloc_id);
 
@@ -510,7 +510,7 @@ impl<'tcx> interpret::Machine<'tcx> for CompileTimeMachine<'tcx> {
 
                 // If an allocation is created in an another const,
                 // we don't deallocate it.
-                let (alloc_id, _, _) = ecx.ptr_get_alloc_id(ptr)?;
+                let (alloc_id, _, _) = ecx.ptr_get_alloc_id(ptr, 0)?;
                 let is_allocated_in_another_const = matches!(
                     ecx.tcx.try_get_global_alloc(alloc_id),
                     Some(interpret::GlobalAlloc::Memory(_))

compiler/rustc_const_eval/src/errors.rs

@@ -1,4 +1,5 @@
 use std::borrow::Cow;
+use std::fmt::Write;
 
 use either::Either;
 use rustc_errors::codes::*;
@@ -15,7 +16,7 @@ use rustc_middle::mir::interpret::{
 use rustc_middle::ty::{self, Mutability, Ty};
 use rustc_span::Span;
 use rustc_target::abi::call::AdjustForForeignAbiError;
-use rustc_target::abi::{Size, WrappingRange};
+use rustc_target::abi::WrappingRange;
 
 use crate::interpret::InternKind;
 
@@ -575,18 +576,21 @@ impl<'a> ReportErrorExt for UndefinedBehaviorInfo<'a> {
                     .arg("bad_pointer_message", bad_pointer_message(msg, dcx));
             }
             PointerOutOfBounds { alloc_id, alloc_size, ptr_offset, inbounds_size, msg } => {
-                diag.arg("alloc_size", alloc_size.bytes())
-                    .arg("inbounds_size", inbounds_size.bytes())
-                    .arg("bad_pointer_message", bad_pointer_message(msg, dcx));
-                diag.arg(
-                    "pointer",
-                    Pointer::new(
-                        Some(CtfeProvenance::from(alloc_id)),
-                        Size::from_bytes(ptr_offset as u64),
-                    )
-                    .to_string(),
-                );
+                diag.arg("alloc_size", alloc_size.bytes());
+                diag.arg("bad_pointer_message", bad_pointer_message(msg, dcx));
+                diag.arg("pointer", {
+                    let mut out = format!("{:?}", alloc_id);
+                    if ptr_offset > 0 {
+                        write!(out, "+{:#x}", ptr_offset).unwrap();
+                    } else if ptr_offset < 0 {
+                        write!(out, "-{:#x}", ptr_offset.unsigned_abs()).unwrap();
+                    }
+                    out
+                });
+                diag.arg("inbounds_size_is_neg", inbounds_size < 0);
+                diag.arg("inbounds_size_abs", inbounds_size.unsigned_abs());
                 diag.arg("ptr_offset_is_neg", ptr_offset < 0);
+                diag.arg("ptr_offset_abs", ptr_offset.unsigned_abs());
                 diag.arg(
                     "alloc_size_minus_ptr_offset",
                     alloc_size.bytes().saturating_sub(ptr_offset as u64),
@@ -600,7 +604,8 @@ impl<'a> ReportErrorExt for UndefinedBehaviorInfo<'a> {
                     );
                 }
 
-                diag.arg("inbounds_size", inbounds_size.bytes());
+                diag.arg("inbounds_size_is_neg", inbounds_size < 0);
+                diag.arg("inbounds_size_abs", inbounds_size.unsigned_abs());
                 diag.arg("bad_pointer_message", bad_pointer_message(msg, dcx));
             }
             AlignmentCheckFailed(Misalignment { required, has }, msg) => {

compiler/rustc_const_eval/src/interpret/eval_context.rs

@@ -560,17 +560,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

@@ -206,7 +206,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)?;
             }
@@ -243,7 +243,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
                 let (a_offset, b_offset, is_addr) = if M::Provenance::OFFSET_IS_ADDR {
                     (a.addr().bytes(), b.addr().bytes(), /*is_addr*/ true)
                 } else {
-                    match (self.ptr_try_get_alloc_id(a), self.ptr_try_get_alloc_id(b)) {
+                    match (self.ptr_try_get_alloc_id(a, 0), self.ptr_try_get_alloc_id(b, 0)) {
                         (Err(a), Err(b)) => {
                             // Neither pointer points to an allocation, so they are both absolute.
                             (a, b, /*is_addr*/ true)
@@ -312,7 +312,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
                 };
 
                 // Check that the memory between them is dereferenceable at all, starting from the
-                // base pointer: `dist` is `a - b`, so it is based on `b`.
+                // origin pointer: `dist` is `a - b`, so it is based on `b`.
                 self.check_ptr_access_signed(b, dist, CheckInAllocMsg::OffsetFromTest)?;
                 // Then check that this is also dereferenceable from `a`. This ensures that they are
                 // derived from the same allocation.
@@ -580,13 +580,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/machine.rs

@@ -321,15 +321,21 @@ pub trait Machine<'tcx>: Sized {
         ptr: Pointer<Self::Provenance>,
     ) -> InterpResult<'tcx>;
 
-    /// Convert a pointer with provenance into an allocation-offset pair
-    /// and extra provenance info.
+    /// Convert a pointer with provenance into an allocation-offset pair and extra provenance info.
+    /// `size` says how many bytes of memory are expected at that pointer. The *sign* of `size` can
+    /// be used to disambiguate situations where a wildcard pointer sits right in between two
+    /// allocations.
     ///
-    /// The returned `AllocId` must be the same as `ptr.provenance.get_alloc_id()`.
+    /// If `ptr.provenance.get_alloc_id()` is `Some(p)`, the returned `AllocId` must be `p`.
+    /// The resulting `AllocId` will just be used for that one step and the forgotten again
+    /// (i.e., we'll never turn the data returned here back into a `Pointer` that might be
+    /// stored in machine state).
     ///
     /// When this fails, that means the pointer does not point to a live allocation.
     fn ptr_get_alloc(
         ecx: &InterpCx<'tcx, Self>,
         ptr: Pointer<Self::Provenance>,
+        size: i64,
     ) -> Option<(AllocId, Size, Self::ProvenanceExtra)>;
 
     /// Called to adjust global allocations to the Provenance and AllocExtra of this machine.
@@ -658,6 +664,7 @@ pub macro compile_time_machine(<$tcx: lifetime>) {
     fn ptr_get_alloc(
         _ecx: &InterpCx<$tcx, Self>,
         ptr: Pointer<CtfeProvenance>,
+        _size: i64,
     ) -> Option<(AllocId, Size, Self::ProvenanceExtra)> {
         // We know `offset` is relative to the allocation, so we can use `into_parts`.
         let (prov, offset) = ptr.into_parts();