Skip to content

Commit

Permalink
Rollup merge of rust-lang#69839 - RalfJung:miri-error-cleanup, r=oli-obk
Browse files Browse the repository at this point in the history
Miri error reform

Some time ago we started moving Miri errors into a few distinct categories, but we never classified all the old errors. That's what this PR does.

~~This is on top of rust-lang#69762; [relative diff](RalfJung/rust@validity-errors...RalfJung:miri-error-cleanup).~~

r? @oli-obk

Fixes rust-lang/const-eval#4
  • Loading branch information
Centril authored Mar 18, 2020
2 parents 1fc4515 + e219dd4 commit 8ab967e
Show file tree
Hide file tree
Showing 28 changed files with 420 additions and 470 deletions.
5 changes: 3 additions & 2 deletions src/librustc/mir/interpret/allocation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ pub struct Allocation<Tag = (), Extra = ()> {
/// The size of the allocation. Currently, must always equal `bytes.len()`.
pub size: Size,
/// The alignment of the allocation to detect unaligned reads.
/// (`Align` guarantees that this is a power of two.)
pub align: Align,
/// `true` if the allocation is mutable.
/// Also used by codegen to determine if a static should be put into mutable memory,
Expand Down Expand Up @@ -314,7 +315,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())),
})
}

Expand Down Expand Up @@ -573,7 +574,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) {
Expand Down
303 changes: 117 additions & 186 deletions src/librustc/mir/interpret/error.rs

Large diffs are not rendered by default.

14 changes: 7 additions & 7 deletions src/librustc/mir/interpret/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}

Expand Down Expand Up @@ -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)]
Expand Down
16 changes: 0 additions & 16 deletions src/librustc/mir/interpret/pointer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(())
}
}
}
13 changes: 7 additions & 6 deletions src/librustc/mir/interpret/value.rs
Original file line number Diff line number Diff line change
Expand Up @@ -455,18 +455,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)),
}
}

Expand Down Expand Up @@ -609,7 +610,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)),
}
}

Expand Down
3 changes: 2 additions & 1 deletion src/librustc_mir/const_eval/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions src/librustc_mir/interpret/eval_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Expand All @@ -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)),
Expand Down Expand Up @@ -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)),
Expand Down
4 changes: 2 additions & 2 deletions src/librustc_mir/interpret/intern.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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.
Expand Down
6 changes: 3 additions & 3 deletions src/librustc_mir/interpret/intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,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),
Expand Down Expand Up @@ -246,9 +246,9 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
let layout = self.layout_of(substs.type_at(0))?;
let r_val = self.force_bits(r.to_scalar()?, layout.size)?;
if let sym::unchecked_shl | sym::unchecked_shr = intrinsic_name {
throw_ub_format!("Overflowing shift by {} in `{}`", r_val, intrinsic_name);
throw_ub_format!("overflowing shift by {} in `{}`", r_val, intrinsic_name);
} else {
throw_ub_format!("Overflow executing `{}`", intrinsic_name);
throw_ub_format!("overflow executing `{}`", intrinsic_name);
}
}
self.write_scalar(val, dest)?;
Expand Down
4 changes: 3 additions & 1 deletion src/librustc_mir/interpret/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -286,8 +286,10 @@ pub trait Machine<'mir, 'tcx>: Sized {
int: u64,
) -> InterpResult<'tcx, Pointer<Self::PointerTag>> {
Err((if int == 0 {
err_unsup!(InvalidNullPointerUsage)
// This is UB, seriously.
err_ub!(InvalidIntPointerUsage(0))
} else {
// This is just something we cannot support during const-eval.
err_unsup!(ReadBytesAsPointer)
})
.into())
Expand Down
82 changes: 48 additions & 34 deletions src/librustc_mir/interpret/memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,10 @@ 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".
Expand Down Expand Up @@ -251,37 +254,43 @@ 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(),
)
}
}

Expand Down Expand Up @@ -338,7 +347,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,
})
Expand All @@ -360,7 +369,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!(InvalidIntPointerUsage(0))
}
// Must be aligned.
if let Some(align) = align {
Expand All @@ -375,7 +384,10 @@ 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 {
Expand All @@ -385,7 +397,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)?;
}
Expand All @@ -402,7 +414,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()
// If the pointer is out-of-bounds, it may be null.
// Note that one-past-the-end (offset == size) is still inbounds, and never null.
ptr.offset > size
}
}

Expand Down Expand Up @@ -432,13 +446,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);
Expand Down Expand Up @@ -524,7 +538,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())),
Expand All @@ -538,7 +552,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)
}
Expand Down Expand Up @@ -568,7 +582,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()))
};
Expand Down Expand Up @@ -596,12 +610,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))
}
}
}
Expand All @@ -626,10 +640,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> {
Expand Down
Loading

0 comments on commit 8ab967e

Please sign in to comment.