Auto merge of rust-lang#71704 - RalfJung:miri-error-print, r=oli-obk

Miri: tweak error print

I started by adjusting the "invalid use of int as pointer" message (it wasn't really clear what is invalid about the use). But then I realized that these are all `Debug` impls we use for these errors, for some reason, so I fixed that to use `Display` instead.

~~This includes rust-lang#71590 (to get the `Display` impl for `Pointer`), so the diff will look better once that finally lands. Here's the [relative diff](RalfJung/rust@e72ebf5...RalfJung:miri-error-print).~~

r? @oli-obk

Author: bors

src/librustc_middle/mir/interpret/error.rs

@@ -1,4 +1,4 @@
-use super::{AllocId, CheckInAllocMsg, Pointer, RawConst, ScalarMaybeUndef};
+use super::{AllocId, Pointer, RawConst, ScalarMaybeUndef};
 
 use crate::mir::interpret::ConstValue;
 use crate::ty::layout::LayoutError;
@@ -285,7 +285,7 @@ pub enum InvalidProgramInfo<'tcx> {
     TransmuteSizeDiff(Ty<'tcx>, Ty<'tcx>),
 }
 
-impl fmt::Debug for InvalidProgramInfo<'_> {
+impl fmt::Display for InvalidProgramInfo<'_> {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         use InvalidProgramInfo::*;
         match self {
@@ -304,14 +304,38 @@ impl fmt::Debug for InvalidProgramInfo<'_> {
     }
 }
 
+/// Details of why a pointer had to be in-bounds.
+#[derive(Debug, Copy, Clone, RustcEncodable, RustcDecodable, HashStable)]
+pub enum CheckInAllocMsg {
+    MemoryAccessTest,
+    NullPointerTest,
+    PointerArithmeticTest,
+    InboundsTest,
+}
+
+impl fmt::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",
+            }
+        )
+    }
+}
+
 /// Error information for when the program caused Undefined Behavior.
 pub enum UndefinedBehaviorInfo {
     /// Free-form case. Only for errors that are never caught!
     Ub(String),
     /// Unreachable code was executed.
     Unreachable,
-    /// An enum discriminant was set to a value which was outside the range of valid values.
-    InvalidDiscriminant(ScalarMaybeUndef),
     /// A slice/array index projection went out-of-bounds.
     BoundsCheckFailed {
         len: u64,
@@ -335,17 +359,15 @@ pub enum UndefinedBehaviorInfo {
         msg: CheckInAllocMsg,
         allocation_size: Size,
     },
+    /// Using an integer as a pointer in the wrong way.
+    DanglingIntPointer(u64, CheckInAllocMsg),
     /// Used a pointer with bad alignment.
     AlignmentCheckFailed {
         required: Align,
         has: Align,
     },
-    /// Using an integer as a pointer in the wrong way.
-    InvalidIntPointerUsage(u64),
     /// Writing to read-only memory.
     WriteToReadOnly(AllocId),
-    /// Using a pointer-not-to-a-function as function pointer.
-    InvalidFunctionPointer(Pointer),
     // Trying to access the data behind a function pointer.
     DerefFunctionPointer(AllocId),
     /// The value validity check found a problem.
@@ -356,6 +378,10 @@ pub enum UndefinedBehaviorInfo {
     InvalidBool(u8),
     /// Using a non-character `u32` as character.
     InvalidChar(u32),
+    /// An enum discriminant was set to a value which was outside the range of valid values.
+    InvalidDiscriminant(ScalarMaybeUndef),
+    /// Using a pointer-not-to-a-function as function pointer.
+    InvalidFunctionPointer(Pointer),
     /// Using uninitialized data where it is not allowed.
     InvalidUndefBytes(Option<Pointer>),
     /// Working with a local that is not currently live.
@@ -367,29 +393,26 @@ pub enum UndefinedBehaviorInfo {
     },
 }
 
-impl fmt::Debug for UndefinedBehaviorInfo {
+impl fmt::Display for UndefinedBehaviorInfo {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         use UndefinedBehaviorInfo::*;
         match self {
             Ub(msg) => write!(f, "{}", msg),
             Unreachable => write!(f, "entering unreachable code"),
-            InvalidDiscriminant(val) => write!(f, "encountering invalid enum discriminant {}", val),
-            BoundsCheckFailed { ref len, ref index } => write!(
-                f,
-                "indexing out of bounds: the len is {:?} but the index is {:?}",
-                len, index
-            ),
+            BoundsCheckFailed { ref len, ref index } => {
+                write!(f, "indexing out of bounds: the len is {} but the index is {}", len, index)
+            }
             DivisionByZero => write!(f, "dividing by zero"),
             RemainderByZero => write!(f, "calculating the remainder with a divisor of zero"),
             PointerArithOverflow => write!(f, "overflowing in-bounds pointer arithmetic"),
             InvalidMeta(msg) => write!(f, "invalid metadata in wide pointer: {}", msg),
             UnterminatedCString(p) => write!(
                 f,
-                "reading a null-terminated string starting at {:?} with no null found before end of allocation",
+                "reading a null-terminated string starting at {} with no null found before end of allocation",
                 p,
             ),
             PointerUseAfterFree(a) => {
-                write!(f, "pointer to {:?} was dereferenced after this allocation got freed", a)
+                write!(f, "pointer to {} was dereferenced after this allocation got freed", a)
             }
             PointerOutOfBounds { ptr, msg, allocation_size } => write!(
                 f,
@@ -400,25 +423,34 @@ impl fmt::Debug for UndefinedBehaviorInfo {
                 ptr.alloc_id,
                 allocation_size.bytes()
             ),
-            InvalidIntPointerUsage(0) => write!(f, "invalid use of NULL pointer"),
-            InvalidIntPointerUsage(i) => write!(f, "invalid use of {} as a pointer", i),
+            DanglingIntPointer(_, CheckInAllocMsg::NullPointerTest) => {
+                write!(f, "NULL pointer is not allowed for this operation")
+            }
+            DanglingIntPointer(i, msg) => {
+                write!(f, "{} failed: 0x{:x} is not a valid pointer", msg, i)
+            }
             AlignmentCheckFailed { required, has } => write!(
                 f,
                 "accessing memory with alignment {}, but alignment {} is required",
                 has.bytes(),
                 required.bytes()
             ),
-            WriteToReadOnly(a) => write!(f, "writing to {:?} which is read-only", a),
+            WriteToReadOnly(a) => write!(f, "writing to {} which is read-only", a),
+            DerefFunctionPointer(a) => write!(f, "accessing {} which contains a function", a),
+            ValidationFailure(ref err) => write!(f, "type validation failed: {}", err),
+            InvalidBool(b) => {
+                write!(f, "interpreting an invalid 8-bit value as a bool: 0x{:2x}", b)
+            }
+            InvalidChar(c) => {
+                write!(f, "interpreting an invalid 32-bit value as a char: 0x{:8x}", c)
+            }
+            InvalidDiscriminant(val) => write!(f, "enum value has invalid discriminant: {}", val),
             InvalidFunctionPointer(p) => {
-                write!(f, "using {:?} as function pointer but it does not point to a function", p)
+                write!(f, "using {} as function pointer but it does not point to a function", p)
             }
-            DerefFunctionPointer(a) => write!(f, "accessing {:?} which contains a function", a),
-            ValidationFailure(ref err) => write!(f, "type validation failed: {}", err),
-            InvalidBool(b) => write!(f, "interpreting an invalid 8-bit value as a bool: {}", b),
-            InvalidChar(c) => write!(f, "interpreting an invalid 32-bit value as a char: {}", c),
             InvalidUndefBytes(Some(p)) => write!(
                 f,
-                "reading uninitialized memory at {:?}, but this operation requires initialized memory",
+                "reading uninitialized memory at {}, but this operation requires initialized memory",
                 p
             ),
             InvalidUndefBytes(None) => write!(
@@ -455,7 +487,7 @@ pub enum UnsupportedOpInfo {
     ReadBytesAsPointer,
 }
 
-impl fmt::Debug for UnsupportedOpInfo {
+impl fmt::Display for UnsupportedOpInfo {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         use UnsupportedOpInfo::*;
         match self {
@@ -481,7 +513,7 @@ pub enum ResourceExhaustionInfo {
     StepLimitReached,
 }
 
-impl fmt::Debug for ResourceExhaustionInfo {
+impl fmt::Display for ResourceExhaustionInfo {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         use ResourceExhaustionInfo::*;
         match self {
@@ -499,7 +531,6 @@ impl fmt::Debug for ResourceExhaustionInfo {
 pub trait AsAny: Any {
     fn as_any(&self) -> &dyn Any;
 }
-
 impl<T: Any> AsAny for T {
     #[inline(always)]
     fn as_any(&self) -> &dyn Any {
@@ -508,7 +539,7 @@ impl<T: Any> AsAny for T {
 }
 
 /// A trait for machine-specific errors (or other "machine stop" conditions).
-pub trait MachineStopType: AsAny + fmt::Debug + Send {}
+pub trait MachineStopType: AsAny + fmt::Display + Send {}
 impl MachineStopType for String {}
 
 impl dyn MachineStopType {
@@ -538,21 +569,21 @@ pub type InterpResult<'tcx, T = ()> = Result<T, InterpErrorInfo<'tcx>>;
 
 impl fmt::Display for InterpError<'_> {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        // Forward `Display` to `Debug`.
-        fmt::Debug::fmt(self, f)
+        use InterpError::*;
+        match *self {
+            Unsupported(ref msg) => write!(f, "{}", msg),
+            InvalidProgram(ref msg) => write!(f, "{}", msg),
+            UndefinedBehavior(ref msg) => write!(f, "{}", msg),
+            ResourceExhaustion(ref msg) => write!(f, "{}", msg),
+            MachineStop(ref msg) => write!(f, "{}", msg),
+        }
     }
 }
 
+// Forward `Debug` to `Display`, so it does not look awful.
 impl fmt::Debug for InterpError<'_> {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        use InterpError::*;
-        match *self {
-            Unsupported(ref msg) => write!(f, "{:?}", msg),
-            InvalidProgram(ref msg) => write!(f, "{:?}", msg),
-            UndefinedBehavior(ref msg) => write!(f, "{:?}", msg),
-            ResourceExhaustion(ref msg) => write!(f, "{:?}", msg),
-            MachineStop(ref msg) => write!(f, "{:?}", msg),
-        }
+        fmt::Display::fmt(self, f)
     }
 }
 

src/librustc_middle/mir/interpret/mod.rs

@@ -117,16 +117,16 @@ use crate::ty::subst::GenericArgKind;
 use crate::ty::{self, Instance, Ty, TyCtxt};
 
 pub use self::error::{
-    struct_error, ConstEvalErr, ConstEvalRawResult, ConstEvalResult, ErrorHandled, FrameInfo,
-    InterpError, InterpErrorInfo, InterpResult, InvalidProgramInfo, MachineStopType,
+    struct_error, CheckInAllocMsg, ConstEvalErr, ConstEvalRawResult, ConstEvalResult, ErrorHandled,
+    FrameInfo, InterpError, InterpErrorInfo, InterpResult, InvalidProgramInfo, MachineStopType,
     ResourceExhaustionInfo, UndefinedBehaviorInfo, UnsupportedOpInfo,
 };
 
 pub use self::value::{get_slice_bytes, ConstValue, RawConst, Scalar, ScalarMaybeUndef};
 
 pub use self::allocation::{Allocation, AllocationExtra, Relocations, UndefMask};
 
-pub use self::pointer::{CheckInAllocMsg, Pointer, PointerArithmetic};
+pub use self::pointer::{Pointer, PointerArithmetic};
 
 /// Uniquely identifies one of the following:
 /// - A constant

src/librustc_middle/mir/interpret/pointer.rs

@@ -4,33 +4,7 @@ use rustc_macros::HashStable;
 use rustc_target::abi::{HasDataLayout, Size};
 
 use std::convert::TryFrom;
-use std::fmt::{self, Display};
-
-/// 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",
-            }
-        )
-    }
-}
+use std::fmt;
 
 ////////////////////////////////////////////////////////////////////////////////
 // Pointer arithmetic

src/librustc_mir/interpret/machine.rs

@@ -10,8 +10,8 @@ use rustc_middle::ty::{self, Ty};
 use rustc_span::def_id::DefId;
 
 use super::{
-    AllocId, Allocation, AllocationExtra, Frame, ImmTy, InterpCx, InterpResult, Memory, MemoryKind,
-    OpTy, Operand, PlaceTy, Pointer, Scalar,
+    AllocId, Allocation, AllocationExtra, CheckInAllocMsg, Frame, ImmTy, InterpCx, InterpResult,
+    Memory, MemoryKind, OpTy, Operand, PlaceTy, Pointer, Scalar,
 };
 
 /// Data returned by Machine::stack_pop,
@@ -346,7 +346,7 @@ pub trait Machine<'mir, 'tcx>: Sized {
     ) -> InterpResult<'tcx, Pointer<Self::PointerTag>> {
         Err((if int == 0 {
             // This is UB, seriously.
-            err_ub!(InvalidIntPointerUsage(0))
+            err_ub!(DanglingIntPointer(0, CheckInAllocMsg::InboundsTest))
         } else {
             // This is just something we cannot support during const-eval.
             err_unsup!(ReadBytesAsPointer)

src/librustc_mir/interpret/memory.rs

@@ -365,7 +365,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
                 assert!(size.bytes() == 0);
                 // Must be non-NULL.
                 if bits == 0 {
-                    throw_ub!(InvalidIntPointerUsage(0))
+                    throw_ub!(DanglingIntPointer(0, msg))
                 }
                 // Must be aligned.
                 if let Some(align) = align {

src/librustc_mir/interpret/validity.rs

@@ -360,10 +360,10 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
                     place.ptr, size, align
                 );
                 match err.kind {
-                    err_ub!(InvalidIntPointerUsage(0)) => {
+                    err_ub!(DanglingIntPointer(0, _)) => {
                         throw_validation_failure!(format_args!("a NULL {}", kind), self.path)
                     }
-                    err_ub!(InvalidIntPointerUsage(i)) => throw_validation_failure!(
+                    err_ub!(DanglingIntPointer(i, _)) => throw_validation_failure!(
                         format_args!("a {} to unallocated address {}", kind, i),
                         self.path
                     ),

src/librustc_mir/transform/const_prop.rs

@@ -43,8 +43,8 @@ macro_rules! throw_machine_stop_str {
         // We make a new local type for it. The type itself does not carry any information,
         // but its vtable (for the `MachineStopType` trait) does.
         struct Zst;
-        // Debug-printing this type shows the desired string.
-        impl std::fmt::Debug for Zst {
+        // Printing this type shows the desired string.
+        impl std::fmt::Display for Zst {
             fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
                 write!(f, $($tt)*)
             }

src/test/ui/consts/const-eval/ub-nonnull.stderr

@@ -13,7 +13,7 @@ LL | / const OUT_OF_BOUNDS_PTR: NonNull<u8> = { unsafe {
 LL | |     let ptr: &[u8; 256] = mem::transmute(&0u8); // &0 gets promoted so it does not dangle
 LL | |     // Use address-of-element for pointer arithmetic. This could wrap around to NULL!
 LL | |     let out_of_bounds_ptr = &ptr[255];
-   | |                             ^^^^^^^^^ Memory access failed: pointer must be in-bounds at offset 256, but is outside bounds of alloc11 which has size 1
+   | |                             ^^^^^^^^^ memory access failed: pointer must be in-bounds at offset 256, but is outside bounds of alloc11 which has size 1
 LL | |     mem::transmute(out_of_bounds_ptr)
 LL | | } };
    | |____-

src/test/ui/consts/const-eval/ub-wide-ptr.stderr

@@ -186,13 +186,13 @@ error[E0080]: could not evaluate static initializer
   --> $DIR/ub-wide-ptr.rs:121:5
    |
 LL |     mem::transmute::<_, &dyn Trait>((&92u8, 0usize))
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ invalid use of NULL pointer
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ inbounds test failed: 0x0 is not a valid pointer
 
 error[E0080]: could not evaluate static initializer
   --> $DIR/ub-wide-ptr.rs:125:5
    |
 LL |     mem::transmute::<_, &dyn Trait>((&92u8, &3u64))
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Memory access failed: pointer must be in-bounds at offset N, but is outside bounds of allocN which has size N
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ memory access failed: pointer must be in-bounds at offset N, but is outside bounds of allocN which has size N
 
 error: aborting due to 24 previous errors
 

src/test/ui/consts/offset_from_ub.stderr

@@ -66,7 +66,7 @@ error: any use of this value will cause an error
 LL |           intrinsics::ptr_offset_from(self, origin)
    |           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |           |
-   |           invalid use of NULL pointer
+   |           inbounds test failed: 0x0 is not a valid pointer
    |           inside `std::ptr::const_ptr::<impl *const u8>::offset_from` at $SRC_DIR/libcore/ptr/const_ptr.rs:LL:COL
    |           inside `OFFSET_FROM_NULL` at $DIR/offset_from_ub.rs:37:14
    |