miri: categorize errors into "unsupported" and "UB"

Also slightly refactor pointer bounds checks to avoid creating unnecessary temporary Errors

src/librustc/mir/interpret/allocation.rs

@@ -314,7 +314,7 @@ impl<'tcx, Tag: Copy, Extra: AllocationExtra<Tag>> Allocation<Tag, Extra> {
                 &self.get_bytes(cx, ptr, size_with_null)?[..size]
             }
             // This includes the case where `offset` is out-of-bounds to begin with.
-            None => throw_unsup!(UnterminatedCString(ptr.erase_tag())),
+            None => throw_ub!(UnterminatedCString(ptr.erase_tag())),
         })
     }
 
@@ -573,7 +573,7 @@ impl<'tcx, Tag, Extra> Allocation<Tag, Extra> {
     fn check_defined(&self, ptr: Pointer<Tag>, size: Size) -> InterpResult<'tcx> {
         self.undef_mask
             .is_range_defined(ptr.offset, ptr.offset + size)
-            .or_else(|idx| throw_unsup!(ReadUndefBytes(idx)))
+            .or_else(|idx| throw_ub!(InvalidUndefBytes(Some(Pointer::new(ptr.alloc_id, idx)))))
     }
 
     pub fn mark_definedness(&mut self, ptr: Pointer<Tag>, size: Size, new_state: bool) {

src/librustc/mir/interpret/mod.rs

@@ -161,7 +161,13 @@ pub struct AllocId(pub u64);
 
 impl fmt::Debug for AllocId {
     fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result {
-        write!(fmt, "alloc{}", self.0)
+        fmt::Display::fmt(self, fmt)
+    }
+}
+
+impl fmt::Display for AllocId {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(f, "alloc{}", self.0)
     }
 }
 
@@ -351,12 +357,6 @@ impl<'s> AllocDecodingSession<'s> {
     }
 }
 
-impl fmt::Display for AllocId {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        write!(f, "{}", self.0)
-    }
-}
-
 /// An allocation in the global (tcx-managed) memory can be either a function pointer,
 /// a static, or a "real" allocation with some data in it.
 #[derive(Debug, Clone, Eq, PartialEq, Hash, RustcDecodable, RustcEncodable, HashStable)]

src/librustc/mir/interpret/pointer.rs

@@ -213,20 +213,4 @@ impl<'tcx, Tag> Pointer<Tag> {
     pub fn erase_tag(self) -> Pointer {
         Pointer { alloc_id: self.alloc_id, offset: self.offset, tag: () }
     }
-
-    /// Test if the pointer is "inbounds" of an allocation of the given size.
-    /// A pointer is "inbounds" even if its offset is equal to the size; this is
-    /// a "one-past-the-end" pointer.
-    #[inline(always)]
-    pub fn check_inbounds_alloc(
-        self,
-        allocation_size: Size,
-        msg: CheckInAllocMsg,
-    ) -> InterpResult<'tcx, ()> {
-        if self.offset > allocation_size {
-            throw_unsup!(PointerOutOfBounds { ptr: self.erase_tag(), msg, allocation_size })
-        } else {
-            Ok(())
-        }
-    }
 }

src/librustc/mir/interpret/value.rs

@@ -429,18 +429,19 @@ impl<'tcx, Tag> Scalar<Tag> {
     }
 
     pub fn to_bool(self) -> InterpResult<'tcx, bool> {
-        match self {
-            Scalar::Raw { data: 0, size: 1 } => Ok(false),
-            Scalar::Raw { data: 1, size: 1 } => Ok(true),
-            _ => throw_unsup!(InvalidBool),
+        let val = self.to_u8()?;
+        match val {
+            0 => Ok(false),
+            1 => Ok(true),
+            _ => throw_ub!(InvalidBool(val)),
         }
     }
 
     pub fn to_char(self) -> InterpResult<'tcx, char> {
         let val = self.to_u32()?;
         match ::std::char::from_u32(val) {
             Some(c) => Ok(c),
-            None => throw_unsup!(InvalidChar(val as u128)),
+            None => throw_ub!(InvalidChar(val)),
         }
     }
 
@@ -583,7 +584,7 @@ impl<'tcx, Tag> ScalarMaybeUndef<Tag> {
     pub fn not_undef(self) -> InterpResult<'static, Scalar<Tag>> {
         match self {
             ScalarMaybeUndef::Scalar(scalar) => Ok(scalar),
-            ScalarMaybeUndef::Undef => throw_unsup!(ReadUndefBytes(Size::ZERO)),
+            ScalarMaybeUndef::Undef => throw_ub!(InvalidUndefBytes(None)),
         }
     }
 

src/librustc_mir/const_eval/machine.rs

@@ -240,7 +240,8 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,
         Ok(Some(match ecx.load_mir(instance.def, None) {
             Ok(body) => *body,
             Err(err) => {
-                if let err_unsup!(NoMirFor(ref path)) = err.kind {
+                if let err_unsup!(NoMirFor(did)) = err.kind {
+                    let path = ecx.tcx.def_path_str(did);
                     return Err(ConstEvalErrKind::NeedsRfc(format!(
                         "calling extern function `{}`",
                         path

src/librustc_mir/interpret/eval_context.rs

@@ -138,7 +138,7 @@ pub enum LocalValue<Tag = (), Id = AllocId> {
 impl<'tcx, Tag: Copy + 'static> LocalState<'tcx, Tag> {
     pub fn access(&self) -> InterpResult<'tcx, Operand<Tag>> {
         match self.value {
-            LocalValue::Dead => throw_unsup!(DeadLocal),
+            LocalValue::Dead => throw_ub!(DeadLocal),
             LocalValue::Uninitialized => {
                 bug!("The type checker should prevent reading from a never-written local")
             }
@@ -152,7 +152,7 @@ impl<'tcx, Tag: Copy + 'static> LocalState<'tcx, Tag> {
         &mut self,
     ) -> InterpResult<'tcx, Result<&mut LocalValue<Tag>, MemPlace<Tag>>> {
         match self.value {
-            LocalValue::Dead => throw_unsup!(DeadLocal),
+            LocalValue::Dead => throw_ub!(DeadLocal),
             LocalValue::Live(Operand::Indirect(mplace)) => Ok(Err(mplace)),
             ref mut local @ LocalValue::Live(Operand::Immediate(_))
             | ref mut local @ LocalValue::Uninitialized => Ok(Ok(local)),
@@ -326,7 +326,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                 if self.tcx.is_mir_available(did) {
                     Ok(self.tcx.optimized_mir(did).unwrap_read_only())
                 } else {
-                    throw_unsup!(NoMirFor(self.tcx.def_path_str(def_id)))
+                    throw_unsup!(NoMirFor(def_id))
                 }
             }
             _ => Ok(self.tcx.instance_mir(instance)),

src/librustc_mir/interpret/intern.rs

@@ -327,7 +327,7 @@ pub fn intern_const_alloc_recursive<M: CompileTimeMachine<'mir, 'tcx>>(
         if let Err(error) = interned {
             // This can happen when e.g. the tag of an enum is not a valid discriminant. We do have
             // to read enum discriminants in order to find references in enum variant fields.
-            if let err_unsup!(ValidationFailure(_)) = error.kind {
+            if let err_ub!(ValidationFailure(_)) = error.kind {
                 let err = crate::const_eval::error_to_const_error(&ecx, error);
                 match err.struct_error(
                     ecx.tcx,
@@ -390,7 +390,7 @@ pub fn intern_const_alloc_recursive<M: CompileTimeMachine<'mir, 'tcx>>(
             }
         } else if ecx.memory.dead_alloc_map.contains_key(&alloc_id) {
             // dangling pointer
-            throw_unsup!(ValidationFailure("encountered dangling pointer in final constant".into()))
+            throw_ub_format!("encountered dangling pointer in final constant")
         } else if ecx.tcx.alloc_map.lock().get(alloc_id).is_none() {
             // We have hit an `AllocId` that is neither in local or global memory and isn't marked
             // as dangling by local memory.

src/librustc_mir/interpret/intrinsics.rs

@@ -134,7 +134,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                 let bits = self.force_bits(val, layout_of.size)?;
                 let kind = match layout_of.abi {
                     ty::layout::Abi::Scalar(ref scalar) => scalar.value,
-                    _ => throw_unsup!(TypeNotPrimitive(ty)),
+                    _ => bug!("{} called on invalid type {:?}", intrinsic_name, ty),
                 };
                 let (nonzero, intrinsic_name) = match intrinsic_name {
                     sym::cttz_nonzero => (true, sym::cttz),

src/librustc_mir/interpret/machine.rs

@@ -281,7 +281,7 @@ pub trait Machine<'mir, 'tcx>: Sized {
         int: u64,
     ) -> InterpResult<'tcx, Pointer<Self::PointerTag>> {
         Err((if int == 0 {
-            err_unsup!(InvalidNullPointerUsage)
+            err_ub!(InvalidNullPointerUsage)
         } else {
             err_unsup!(ReadBytesAsPointer)
         })

src/librustc_mir/interpret/memory.rs

@@ -215,7 +215,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
         kind: MemoryKind<M::MemoryKinds>,
     ) -> InterpResult<'tcx, Pointer<M::PointerTag>> {
         if ptr.offset.bytes() != 0 {
-            throw_unsup!(ReallocateNonBasePtr)
+            throw_ub_format!("reallocating {:?} which does not point to the beginning of an object", ptr);
         }
 
         // For simplicities' sake, we implement reallocate as "alloc, copy, dealloc".
@@ -251,37 +251,32 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
         trace!("deallocating: {}", ptr.alloc_id);
 
         if ptr.offset.bytes() != 0 {
-            throw_unsup!(DeallocateNonBasePtr)
+            throw_ub_format!("deallocating {:?} which does not point to the beginning of an object", ptr);
         }
 
         let (alloc_kind, mut alloc) = match self.alloc_map.remove(&ptr.alloc_id) {
             Some(alloc) => alloc,
             None => {
                 // Deallocating static memory -- always an error
                 return Err(match self.tcx.alloc_map.lock().get(ptr.alloc_id) {
-                    Some(GlobalAlloc::Function(..)) => err_unsup!(DeallocatedWrongMemoryKind(
-                        "function".to_string(),
-                        format!("{:?}", kind),
-                    )),
-                    Some(GlobalAlloc::Static(..)) | Some(GlobalAlloc::Memory(..)) => err_unsup!(
-                        DeallocatedWrongMemoryKind("static".to_string(), format!("{:?}", kind))
-                    ),
-                    None => err_unsup!(DoubleFree),
+                    Some(GlobalAlloc::Function(..)) => err_ub_format!("deallocating a function"),
+                    Some(GlobalAlloc::Static(..)) | Some(GlobalAlloc::Memory(..)) =>
+                        err_ub_format!("deallocating static memory"),
+                    None => err_ub!(PointerUseAfterFree(ptr.alloc_id)),
                 }
                 .into());
             }
         };
 
         if alloc_kind != kind {
-            throw_unsup!(DeallocatedWrongMemoryKind(
-                format!("{:?}", alloc_kind),
-                format!("{:?}", kind),
-            ))
+            throw_ub_format!("deallocating `{:?}` memory using `{:?}` deallocation operation", alloc_kind, kind);
         }
         if let Some((size, align)) = old_size_and_align {
             if size != alloc.size || align != alloc.align {
-                let bytes = alloc.size;
-                throw_unsup!(IncorrectAllocationInformation(size, bytes, align, alloc.align))
+                throw_ub_format!(
+                    "incorrect layout on deallocation: allocation has size {} and alignment {}, but gave size {} and alignment {}",
+                    alloc.size.bytes(), alloc.align.bytes(), size.bytes(), align.bytes(),
+                )
             }
         }
 
@@ -338,7 +333,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
             } else {
                 // The biggest power of two through which `offset` is divisible.
                 let offset_pow2 = 1 << offset.trailing_zeros();
-                throw_unsup!(AlignmentCheckFailed {
+                throw_ub!(AlignmentCheckFailed {
                     has: Align::from_bytes(offset_pow2).unwrap(),
                     required: align,
                 })
@@ -360,7 +355,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
                 assert!(size.bytes() == 0);
                 // Must be non-NULL.
                 if bits == 0 {
-                    throw_unsup!(InvalidNullPointerUsage)
+                    throw_ub!(InvalidNullPointerUsage)
                 }
                 // Must be aligned.
                 if let Some(align) = align {
@@ -375,7 +370,9 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
                 // It is sufficient to check this for the end pointer. The addition
                 // checks for overflow.
                 let end_ptr = ptr.offset(size, self)?;
-                end_ptr.check_inbounds_alloc(allocation_size, msg)?;
+                if end_ptr.offset > allocation_size { // equal is okay!
+                    throw_ub!(PointerOutOfBounds { ptr: end_ptr.erase_tag(), msg, allocation_size })
+                }
                 // Test align. Check this last; if both bounds and alignment are violated
                 // we want the error to be about the bounds.
                 if let Some(align) = align {
@@ -385,7 +382,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
                         // got picked we might be aligned even if this check fails.
                         // We instead have to fall back to converting to an integer and checking
                         // the "real" alignment.
-                        throw_unsup!(AlignmentCheckFailed { has: alloc_align, required: align });
+                        throw_ub!(AlignmentCheckFailed { has: alloc_align, required: align });
                     }
                     check_offset_align(ptr.offset.bytes(), align)?;
                 }
@@ -402,7 +399,9 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
         let (size, _align) = self
             .get_size_and_align(ptr.alloc_id, AllocCheck::MaybeDead)
             .expect("alloc info with MaybeDead cannot fail");
-        ptr.check_inbounds_alloc(size, CheckInAllocMsg::NullPointerTest).is_err()
+        // An inbounds pointer is never null!  And "inbounds" includes one-past-the-end.
+        let inbounds = ptr.offset <= size;
+        !inbounds
     }
 }
 
@@ -432,13 +431,13 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
         let alloc = tcx.alloc_map.lock().get(id);
         let alloc = match alloc {
             Some(GlobalAlloc::Memory(mem)) => Cow::Borrowed(mem),
-            Some(GlobalAlloc::Function(..)) => throw_unsup!(DerefFunctionPointer),
-            None => throw_unsup!(DanglingPointerDeref),
+            Some(GlobalAlloc::Function(..)) => throw_ub!(DerefFunctionPointer(id)),
+            None => throw_ub!(PointerUseAfterFree(id)),
             Some(GlobalAlloc::Static(def_id)) => {
                 // We got a "lazy" static that has not been computed yet.
                 if tcx.is_foreign_item(def_id) {
                     trace!("get_static_alloc: foreign item {:?}", def_id);
-                    throw_unsup!(ReadForeignStatic)
+                    throw_unsup!(ReadForeignStatic(def_id))
                 }
                 trace!("get_static_alloc: Need to compute {:?}", def_id);
                 let instance = Instance::mono(tcx.tcx, def_id);
@@ -524,7 +523,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
             // to give us a cheap reference.
             let alloc = Self::get_static_alloc(memory_extra, tcx, id)?;
             if alloc.mutability == Mutability::Not {
-                throw_unsup!(ModifiedConstantMemory)
+                throw_ub!(WriteToReadOnly(id))
             }
             match M::STATIC_KIND {
                 Some(kind) => Ok((MemoryKind::Machine(kind), alloc.into_owned())),
@@ -538,7 +537,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
             Ok(a) => {
                 let a = &mut a.1;
                 if a.mutability == Mutability::Not {
-                    throw_unsup!(ModifiedConstantMemory)
+                    throw_ub!(WriteToReadOnly(id))
                 }
                 Ok(a)
             }
@@ -568,7 +567,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
         if self.get_fn_alloc(id).is_some() {
             return if let AllocCheck::Dereferenceable = liveness {
                 // The caller requested no function pointers.
-                throw_unsup!(DerefFunctionPointer)
+                throw_ub!(DerefFunctionPointer(id))
             } else {
                 Ok((Size::ZERO, Align::from_bytes(1).unwrap()))
             };
@@ -596,12 +595,12 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
                 if let AllocCheck::MaybeDead = liveness {
                     // Deallocated pointers are allowed, we should be able to find
                     // them in the map.
-                    Ok(*self.dead_alloc_map.get(&id).expect(
-                        "deallocated pointers should all be recorded in \
-                            `dead_alloc_map`",
-                    ))
+                    Ok(*self
+                        .dead_alloc_map
+                        .get(&id)
+                        .expect("deallocated pointers should all be recorded in `dead_alloc_map`"))
                 } else {
-                    throw_unsup!(DanglingPointerDeref)
+                    throw_ub!(PointerUseAfterFree(id))
                 }
             }
         }
@@ -626,10 +625,10 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
     ) -> InterpResult<'tcx, FnVal<'tcx, M::ExtraFnVal>> {
         let ptr = self.force_ptr(ptr)?; // We definitely need a pointer value.
         if ptr.offset.bytes() != 0 {
-            throw_unsup!(InvalidFunctionPointer)
+            throw_ub!(InvalidFunctionPointer(ptr.erase_tag()))
         }
         let id = M::canonical_alloc_id(self, ptr.alloc_id);
-        self.get_fn_alloc(id).ok_or_else(|| err_unsup!(ExecuteMemory).into())
+        self.get_fn_alloc(id).ok_or_else(|| err_ub!(InvalidFunctionPointer(ptr.erase_tag())).into())
     }
 
     pub fn mark_immutable(&mut self, id: AllocId) -> InterpResult<'tcx> {

src/librustc_mir/interpret/operand.rs

@@ -344,7 +344,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
         let len = mplace.len(self)?;
         let bytes = self.memory.read_bytes(mplace.ptr, Size::from_bytes(len as u64))?;
         let str = ::std::str::from_utf8(bytes)
-            .map_err(|err| err_unsup!(ValidationFailure(err.to_string())))?;
+            .map_err(|err| err_ub_format!("this string is not valid UTF-8: {}", err))?;
         Ok(str)
     }
 
@@ -458,7 +458,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
         layout: Option<TyLayout<'tcx>>,
     ) -> InterpResult<'tcx, OpTy<'tcx, M::PointerTag>> {
         let base_op = match place.local {
-            mir::RETURN_PLACE => throw_unsup!(ReadFromReturnPointer),
+            mir::RETURN_PLACE => throw_ub!(ReadFromReturnPlace),
             local => {
                 // Do not use the layout passed in as argument if the base we are looking at
                 // here is not the entire place.

src/librustc_mir/interpret/place.rs

@@ -926,7 +926,7 @@ where
             // most likey we *are* running `typeck` right now. Investigate whether we can bail out
             // on `typeck_tables().has_errors` at all const eval entry points.
             debug!("Size mismatch when transmuting!\nsrc: {:#?}\ndest: {:#?}", src, dest);
-            throw_unsup!(TransmuteSizeDiff(src.layout.ty, dest.layout.ty));
+            throw_inval!(TransmuteSizeDiff(src.layout.ty, dest.layout.ty));
         }
         // Unsized copies rely on interpreting `src.meta` with `dest.layout`, we want
         // to avoid that here.