Auto merge of #59627 - LooMaclin:issue_57128_improve_miri_error_reporting_in_check_in_alloc, r=RalfJung

Improve miri error reporting in check_in_alloc

Fixes https://github.com/rust-lang/rust/issues/57128

r? @RalfJung @oli-obk

src/librustc/mir/interpret/allocation.rs

@@ -7,7 +7,7 @@ use super::{
 
 use crate::ty::layout::{Size, Align};
 use syntax::ast::Mutability;
-use std::iter;
+use std::{iter, fmt::{self, Display}};
 use crate::mir;
 use std::ops::{Deref, DerefMut};
 use rustc_data_structures::sorted_map::SortedMap;
@@ -22,6 +22,28 @@ pub enum InboundsCheck {
     MaybeDead,
 }
 
+/// Used by `check_in_alloc` to indicate context of check
+#[derive(Debug, Copy, Clone, RustcEncodable, RustcDecodable, HashStable)]
+pub enum CheckInAllocMsg {
+    MemoryAccessTest,
+    NullPointerTest,
+    PointerArithmeticTest,
+    InboundsTest,
+}
+
+impl Display for CheckInAllocMsg {
+    /// When this is printed as an error the context looks like this
+    /// "{test name} failed: pointer must be in-bounds at offset..."
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(f, "{}", match *self {
+            CheckInAllocMsg::MemoryAccessTest => "Memory access",
+            CheckInAllocMsg::NullPointerTest => "Null pointer test",
+            CheckInAllocMsg::PointerArithmeticTest => "Pointer arithmetic",
+            CheckInAllocMsg::InboundsTest => "Inbounds test",
+        })
+    }
+}
+
 #[derive(Clone, Debug, Eq, PartialEq, PartialOrd, Ord, Hash, RustcEncodable, RustcDecodable)]
 pub struct Allocation<Tag=(),Extra=()> {
     /// The actual bytes of the allocation.
@@ -131,9 +153,10 @@ impl<'tcx, Tag, Extra> Allocation<Tag, Extra> {
     fn check_bounds_ptr(
         &self,
         ptr: Pointer<Tag>,
+        msg: CheckInAllocMsg,
     ) -> EvalResult<'tcx> {
         let allocation_size = self.bytes.len() as u64;
-        ptr.check_in_alloc(Size::from_bytes(allocation_size), InboundsCheck::Live)
+        ptr.check_in_alloc(Size::from_bytes(allocation_size), msg)
     }
 
     /// Checks if the memory range beginning at `ptr` and of size `Size` is "in-bounds".
@@ -143,9 +166,10 @@ impl<'tcx, Tag, Extra> Allocation<Tag, Extra> {
         cx: &impl HasDataLayout,
         ptr: Pointer<Tag>,
         size: Size,
+        msg: CheckInAllocMsg,
     ) -> EvalResult<'tcx> {
         // if ptr.offset is in bounds, then so is ptr (because offset checks for overflow)
-        self.check_bounds_ptr(ptr.offset(size, cx)?)
+        self.check_bounds_ptr(ptr.offset(size, cx)?, msg)
     }
 }
 
@@ -164,9 +188,10 @@ impl<'tcx, Tag: Copy, Extra: AllocationExtra<Tag>> Allocation<Tag, Extra> {
         ptr: Pointer<Tag>,
         size: Size,
         check_defined_and_ptr: bool,
+        msg: CheckInAllocMsg,
     ) -> EvalResult<'tcx, &[u8]>
     {
-        self.check_bounds(cx, ptr, size)?;
+        self.check_bounds(cx, ptr, size, msg)?;
 
         if check_defined_and_ptr {
             self.check_defined(ptr, size)?;
@@ -192,7 +217,7 @@ impl<'tcx, Tag: Copy, Extra: AllocationExtra<Tag>> Allocation<Tag, Extra> {
         size: Size,
     ) -> EvalResult<'tcx, &[u8]>
     {
-        self.get_bytes_internal(cx, ptr, size, true)
+        self.get_bytes_internal(cx, ptr, size, true, CheckInAllocMsg::MemoryAccessTest)
     }
 
     /// It is the caller's responsibility to handle undefined and pointer bytes.
@@ -205,7 +230,7 @@ impl<'tcx, Tag: Copy, Extra: AllocationExtra<Tag>> Allocation<Tag, Extra> {
         size: Size,
     ) -> EvalResult<'tcx, &[u8]>
     {
-        self.get_bytes_internal(cx, ptr, size, false)
+        self.get_bytes_internal(cx, ptr, size, false, CheckInAllocMsg::MemoryAccessTest)
     }
 
     /// Just calling this already marks everything as defined and removes relocations,
@@ -218,7 +243,7 @@ impl<'tcx, Tag: Copy, Extra: AllocationExtra<Tag>> Allocation<Tag, Extra> {
     ) -> EvalResult<'tcx, &mut [u8]>
     {
         assert_ne!(size.bytes(), 0, "0-sized accesses should never even get a `Pointer`");
-        self.check_bounds(cx, ptr, size)?;
+        self.check_bounds(cx, ptr, size, CheckInAllocMsg::MemoryAccessTest)?;
 
         self.mark_definedness(ptr, size, true)?;
         self.clear_relocations(cx, ptr, size)?;

src/librustc/mir/interpret/error.rs

@@ -8,7 +8,7 @@ use crate::ty::layout::{Size, Align, LayoutError};
 use rustc_target::spec::abi::Abi;
 use rustc_macros::HashStable;
 
-use super::{RawConst, Pointer, InboundsCheck, ScalarMaybeUndef};
+use super::{RawConst, Pointer, CheckInAllocMsg, ScalarMaybeUndef};
 
 use backtrace::Backtrace;
 
@@ -247,7 +247,7 @@ pub enum InterpError<'tcx, O> {
     InvalidDiscriminant(ScalarMaybeUndef),
     PointerOutOfBounds {
         ptr: Pointer,
-        check: InboundsCheck,
+        msg: CheckInAllocMsg,
         allocation_size: Size,
     },
     InvalidNullPointerUsage,
@@ -466,14 +466,10 @@ impl<'tcx, O: fmt::Debug> fmt::Debug for InterpError<'tcx, O> {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         use self::InterpError::*;
         match *self {
-            PointerOutOfBounds { ptr, check, allocation_size } => {
+            PointerOutOfBounds { ptr, msg, allocation_size } => {
-                write!(f, "Pointer must be in-bounds{} at offset {}, but is outside bounds of \
+                write!(f, "{} failed: pointer must be in-bounds at offset {}, \
-                           allocation {} which has size {}",
+                          but is outside bounds of allocation {} which has size {}",
-                       match check {
+                    msg, ptr.offset.bytes(), ptr.alloc_id, allocation_size.bytes())
-                           InboundsCheck::Live => " and live",
-                           InboundsCheck::MaybeDead => "",
-                       },
-                       ptr.offset.bytes(), ptr.alloc_id, allocation_size.bytes())
             },
             ValidationFailure(ref err) => {
                 write!(f, "type validation failed: {}", err)

src/librustc/mir/interpret/mod.rs

@@ -19,7 +19,7 @@ pub use self::value::{Scalar, ScalarMaybeUndef, RawConst, ConstValue};
 
 pub use self::allocation::{
     InboundsCheck, Allocation, AllocationExtra,
-    Relocations, UndefMask,
+    Relocations, UndefMask, CheckInAllocMsg,
 };
 
 pub use self::pointer::{Pointer, PointerArithmetic};

src/librustc/mir/interpret/pointer.rs

@@ -5,7 +5,7 @@ use crate::ty::layout::{self, HasDataLayout, Size};
 use rustc_macros::HashStable;
 
 use super::{
-    AllocId, EvalResult, InboundsCheck,
+    AllocId, EvalResult, CheckInAllocMsg
 };
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -177,12 +177,12 @@ impl<'tcx, Tag> Pointer<Tag> {
     pub fn check_in_alloc(
         self,
         allocation_size: Size,
-        check: InboundsCheck,
+        msg: CheckInAllocMsg,
     ) -> EvalResult<'tcx, ()> {
         if self.offset > allocation_size {
             err!(PointerOutOfBounds {
                 ptr: self.erase_tag(),
-                check,
+                msg,
                 allocation_size,
             })
         } else {

src/librustc_mir/interpret/memory.rs

@@ -20,7 +20,7 @@ use syntax::ast::Mutability;
 use super::{
     Pointer, AllocId, Allocation, GlobalId, AllocationExtra,
     EvalResult, Scalar, InterpError, AllocKind, PointerArithmetic,
-    Machine, AllocMap, MayLeak, ErrorHandled, InboundsCheck,
+    Machine, AllocMap, MayLeak, ErrorHandled, CheckInAllocMsg, InboundsCheck,
 };
 
 #[derive(Debug, PartialEq, Eq, Copy, Clone, Hash)]
@@ -252,7 +252,8 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> {
             Scalar::Ptr(ptr) => {
                 // check this is not NULL -- which we can ensure only if this is in-bounds
                 // of some (potentially dead) allocation.
-                let align = self.check_bounds_ptr(ptr, InboundsCheck::MaybeDead)?;
+                let align = self.check_bounds_ptr(ptr, InboundsCheck::MaybeDead,
+                                                  CheckInAllocMsg::NullPointerTest)?;
                 (ptr.offset.bytes(), align)
             }
             Scalar::Bits { bits, size } => {
@@ -293,9 +294,10 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> {
         &self,
         ptr: Pointer<M::PointerTag>,
         liveness: InboundsCheck,
+        msg: CheckInAllocMsg,
     ) -> EvalResult<'tcx, Align> {
         let (allocation_size, align) = self.get_size_and_align(ptr.alloc_id, liveness)?;
-        ptr.check_in_alloc(allocation_size, liveness)?;
+        ptr.check_in_alloc(allocation_size, msg)?;
         Ok(align)
     }
 }
@@ -419,7 +421,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> {
 
     /// Obtain the size and alignment of an allocation, even if that allocation has been deallocated
     ///
-    /// If `liveness` is `InboundsCheck::Dead`, this function always returns `Ok`
+    /// If `liveness` is `InboundsCheck::MaybeDead`, this function always returns `Ok`
     pub fn get_size_and_align(
         &self,
         id: AllocId,

src/librustc_mir/interpret/operand.rs

@@ -7,9 +7,9 @@ use rustc::{mir, ty};
 use rustc::ty::layout::{self, Size, LayoutOf, TyLayout, HasDataLayout, IntegerExt, VariantIdx};
 
 use rustc::mir::interpret::{
-    GlobalId, AllocId, InboundsCheck,
+    GlobalId, AllocId, CheckInAllocMsg,
     ConstValue, Pointer, Scalar,
-    EvalResult, InterpError,
+    EvalResult, InterpError, InboundsCheck,
     sign_extend, truncate,
 };
 use super::{
@@ -645,7 +645,8 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> InterpretCx<'a, 'mir, 'tcx, M>
                     ScalarMaybeUndef::Scalar(Scalar::Ptr(ptr)) => {
                         // The niche must be just 0 (which an inbounds pointer value never is)
                         let ptr_valid = niche_start == 0 && variants_start == variants_end &&
-                            self.memory.check_bounds_ptr(ptr, InboundsCheck::MaybeDead).is_ok();
+                            self.memory.check_bounds_ptr(ptr, InboundsCheck::MaybeDead,
+                                                         CheckInAllocMsg::NullPointerTest).is_ok();
                         if !ptr_valid {
                             return err!(InvalidDiscriminant(raw_discr.erase_tag()));
                         }

src/librustc_mir/interpret/validity.rs

@@ -8,7 +8,7 @@ use rustc::ty::layout::{self, Size, Align, TyLayout, LayoutOf, VariantIdx};
 use rustc::ty;
 use rustc_data_structures::fx::FxHashSet;
 use rustc::mir::interpret::{
-    Scalar, AllocKind, EvalResult, InterpError,
+    Scalar, AllocKind, EvalResult, InterpError, CheckInAllocMsg,
 };
 
 use super::{
@@ -417,7 +417,7 @@ impl<'rt, 'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>>
                         try_validation!(
                             self.ecx.memory
                                 .get(ptr.alloc_id)?
-                                .check_bounds(self.ecx, ptr, size),
+                                .check_bounds(self.ecx, ptr, size, CheckInAllocMsg::InboundsTest),
                             "dangling (not entirely in bounds) reference", self.path);
                     }
                     // Check if we have encountered this pointer+layout combination