Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Interpret: AllocRange Debug impl, and use it more consistently #98811

Merged
merged 3 commits into from
Jul 5, 2022
Merged

Interpret: AllocRange Debug impl, and use it more consistently #98811

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c36572c
add AllocRange Debug impl; remove redundant AllocId Display impl
RalfJung Jul 2, 2022
d31cbb5
make AllocRef APIs more consistent
RalfJung Jul 2, 2022
0832d1d
more use of format! variable capture
RalfJung Jul 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 17 additions & 29 deletions compiler/rustc_const_eval/src/interpret/memory.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
kind: MemoryKind<M::MemoryKind>,
) -> InterpResult<'tcx> {
let (alloc_id, offset, tag) = self.ptr_get_alloc_id(ptr)?;
trace!("deallocating: {}", alloc_id);
trace!("deallocating: {alloc_id:?}");

if offset.bytes() != 0 {
throw_ub_format!(
Expand All @@ -289,10 +289,10 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
// Deallocating global memory -- always an error
return Err(match self.tcx.get_global_alloc(alloc_id) {
Some(GlobalAlloc::Function(..)) => {
err_ub_format!("deallocating {}, which is a function", alloc_id)
err_ub_format!("deallocating {alloc_id:?}, which is a function")
}
Some(GlobalAlloc::Static(..) | GlobalAlloc::Memory(..)) => {
err_ub_format!("deallocating {}, which is static memory", alloc_id)
err_ub_format!("deallocating {alloc_id:?}, which is static memory")
}
None => err_ub!(PointerUseAfterFree(alloc_id)),
}
Expand All @@ -302,21 +302,19 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
debug!(?alloc);

if alloc.mutability == Mutability::Not {
throw_ub_format!("deallocating immutable allocation {}", alloc_id);
throw_ub_format!("deallocating immutable allocation {alloc_id:?}");
}
if alloc_kind != kind {
throw_ub_format!(
"deallocating {}, which is {} memory, using {} deallocation operation",
alloc_id,
"deallocating {alloc_id:?}, which is {} memory, using {} deallocation operation",
alloc_kind,
kind
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
);
}
if let Some((size, align)) = old_size_and_align {
if size != alloc.size() || align != alloc.align {
throw_ub_format!(
"incorrect layout on deallocation: {} has size {} and alignment {}, but gave size {} and alignment {}",
alloc_id,
"incorrect layout on deallocation: {alloc_id:?} has size {} and alignment {}, but gave size {} and alignment {}",
alloc.size().bytes(),
alloc.align.bytes(),
size.bytes(),
Expand Down Expand Up @@ -815,7 +813,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> std::fmt::Debug for DumpAllocs<'a,
continue;
}

write!(fmt, "{}", id)?;
write!(fmt, "{id:?}")?;
match self.ecx.memory.alloc_map.get(id) {
Some(&(kind, ref alloc)) => {
// normal alloc
Expand Down Expand Up @@ -859,25 +857,21 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> std::fmt::Debug for DumpAllocs<'a,

/// Reading and writing.
impl<'tcx, 'a, Tag: Provenance, Extra> AllocRefMut<'a, 'tcx, Tag, Extra> {
/// `range` is relative to this allocation reference, not the base of the allocation.
pub fn write_scalar(
&mut self,
range: AllocRange,
val: ScalarMaybeUninit<Tag>,
) -> InterpResult<'tcx> {
let range = self.range.subrange(range);
debug!(
"write_scalar in {} at {:#x}, size {}: {:?}",
self.alloc_id,
range.start.bytes(),
range.size.bytes(),
val
);
debug!("write_scalar at {:?}{range:?}: {val:?}", self.alloc_id);
Ok(self
.alloc
.write_scalar(&self.tcx, range, val)
.map_err(|e| e.to_interp_error(self.alloc_id))?)
}

/// `offset` is relative to this allocation reference, not the base of the allocation.
pub fn write_ptr_sized(
&mut self,
offset: Size,
Expand All @@ -896,6 +890,7 @@ impl<'tcx, 'a, Tag: Provenance, Extra> AllocRefMut<'a, 'tcx, Tag, Extra> {
}

impl<'tcx, 'a, Tag: Provenance, Extra> AllocRef<'a, 'tcx, Tag, Extra> {
/// `range` is relative to this allocation reference, not the base of the allocation.
pub fn read_scalar(
&self,
range: AllocRange,
Expand All @@ -906,31 +901,24 @@ impl<'tcx, 'a, Tag: Provenance, Extra> AllocRef<'a, 'tcx, Tag, Extra> {
.alloc
.read_scalar(&self.tcx, range, read_provenance)
.map_err(|e| e.to_interp_error(self.alloc_id))?;
debug!(
"read_scalar in {} at {:#x}, size {}: {:?}",
self.alloc_id,
range.start.bytes(),
range.size.bytes(),
res
);
debug!("read_scalar at {:?}{range:?}: {res:?}", self.alloc_id);
Ok(res)
}

pub fn read_integer(
&self,
offset: Size,
size: Size,
) -> InterpResult<'tcx, ScalarMaybeUninit<Tag>> {
self.read_scalar(alloc_range(offset, size), /*read_provenance*/ false)
/// `range` is relative to this allocation reference, not the base of the allocation.
pub fn read_integer(&self, range: AllocRange) -> InterpResult<'tcx, ScalarMaybeUninit<Tag>> {
self.read_scalar(range, /*read_provenance*/ false)
Copy link
Member Author

@RalfJung RalfJung Jul 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oli-obk I know you said you didn't like these alloc_range(...) calls. OTOH, I tried changing all these functions to offset-size pairs, and it has the usual problems of a function where two arguments have the same type -- call sites become rather ambiguous-looking:

                alloc.write_scalar(Size::ZERO, a_size, a_val)?;

This is not an access of size zero.

By using alloc_range, at least one has to only learn once what that function does. I originally intended to use this as AllocRange { start: Size::ZERO, size: a_size }, which is very clear but also very verbose. I wish we could write _ { start: Size::ZERO, size: a_size } and have rustc infer the type... ;)

I am open to other ideas.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recently saw a suggestion for start..+len, while neat, I find it an extension to the language that definitely does not carry its weight. The additional sigils aren't very obvious or googleable.

for slices we at least have x[start..][..len]. If indexing could return non-references we could go this route. We could use a method, which is likely not worse than what we have:

        let align = vtable
            .read_integer(alloc_range(
                pointer_size * u64::try_from(COMMON_VTABLE_ENTRIES_ALIGN).unwrap(),
                pointer_size,
            ))?
            .check_init()?;

could become

        let align = vtable
            .offset(pointer_size * u64::try_from(COMMON_VTABLE_ENTRIES_ALIGN).unwrap())
            .with_len(pointer_size)
            .read_integer()?
            .check_init()?;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw, I think my main complaint is that read_scalar and friends take an AllocRange argument instead of first calling a method to offset/shrink the alloc and then just having no positional arguments to read_scalar

Copy link
Member Author

@RalfJung RalfJung Jul 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recently saw a suggestion for start..+len, while neat, I find it an extension to the language that definitely does not carry its weight. The additional sigils aren't very obvious or googleable.

We could have a macro, alloc_range!(start..+len)? Though not sure if the parser will allow tat.

fwiw, I think my main complaint is that read_scalar and friends take an AllocRange argument instead of first calling a method to offset/shrink the alloc and then just having no positional arguments to read_scalar

Oh, interesting idea; I had not considered that. But what argument would the restrict-range function take? Another AllocRange? Or does offset change the start without changing the end (i.e., it changes both start and size, the way things are currently represented)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or does offset change the start without changing the end (i.e., it changes both start and size, the way things are currently represented)?

yea, this. Could probably use a better name

}

/// `offset` is relative to this allocation reference, not the base of the allocation.
pub fn read_pointer(&self, offset: Size) -> InterpResult<'tcx, ScalarMaybeUninit<Tag>> {
self.read_scalar(
alloc_range(offset, self.tcx.data_layout().pointer_size),
/*read_provenance*/ true,
)
}

/// `range` is relative to this allocation reference, not the base of the allocation.
pub fn check_bytes(
&self,
range: AllocRange,
Expand Down
10 changes: 5 additions & 5 deletions compiler/rustc_const_eval/src/interpret/traits.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use std::convert::TryFrom;

use rustc_middle::mir::interpret::{InterpResult, Pointer, PointerArithmetic};
use rustc_middle::mir::interpret::{alloc_range, InterpResult, Pointer, PointerArithmetic};
use rustc_middle::ty::{
self, Ty, TyCtxt, COMMON_VTABLE_ENTRIES_ALIGN, COMMON_VTABLE_ENTRIES_DROPINPLACE,
COMMON_VTABLE_ENTRIES_SIZE,
Expand Down Expand Up @@ -102,18 +102,18 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
)?
.expect("cannot be a ZST");
let size = vtable
.read_integer(
.read_integer(alloc_range(
pointer_size * u64::try_from(COMMON_VTABLE_ENTRIES_SIZE).unwrap(),
pointer_size,
)?
))?
.check_init()?;
let size = size.to_machine_usize(self)?;
let size = Size::from_bytes(size);
let align = vtable
.read_integer(
.read_integer(alloc_range(
pointer_size * u64::try_from(COMMON_VTABLE_ENTRIES_ALIGN).unwrap(),
pointer_size,
)?
))?
.check_init()?;
let align = align.to_machine_usize(self)?;
let align = Align::from_bytes(align).map_err(|e| err_ub!(InvalidVtableAlignment(e)))?;
Expand Down
8 changes: 7 additions & 1 deletion compiler/rustc_middle/src/mir/interpret/allocation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,12 +160,18 @@ impl AllocError {
}

/// The information that makes up a memory access: offset and size.
#[derive(Copy, Clone, Debug)]
#[derive(Copy, Clone)]
pub struct AllocRange {
pub start: Size,
pub size: Size,
}

impl fmt::Debug for AllocRange {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "[{:#x}..{:#x}]", self.start.bytes(), self.end().bytes())
}
}

/// Free-starting constructor for less syntactic overhead.
#[inline(always)]
pub fn alloc_range(start: Size, size: Size) -> AllocRange {
Expand Down
20 changes: 7 additions & 13 deletions compiler/rustc_middle/src/mir/interpret/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,45 +334,39 @@ impl fmt::Display for UndefinedBehaviorInfo<'_> {
p,
),
PointerUseAfterFree(a) => {
write!(f, "pointer to {} was dereferenced after this allocation got freed", a)
write!(f, "pointer to {a:?} was dereferenced after this allocation got freed")
}
PointerOutOfBounds { alloc_id, alloc_size, ptr_offset, ptr_size: Size::ZERO, msg } => {
write!(
f,
"{}{alloc_id} has size {alloc_size}, so pointer at offset {ptr_offset} is out-of-bounds",
msg,
alloc_id = alloc_id,
"{msg}{alloc_id:?} has size {alloc_size}, so pointer at offset {ptr_offset} is out-of-bounds",
alloc_size = alloc_size.bytes(),
ptr_offset = ptr_offset,
)
}
PointerOutOfBounds { alloc_id, alloc_size, ptr_offset, ptr_size, msg } => write!(
f,
"{}{alloc_id} has size {alloc_size}, so pointer to {ptr_size} byte{ptr_size_p} starting at offset {ptr_offset} is out-of-bounds",
msg,
alloc_id = alloc_id,
"{msg}{alloc_id:?} has size {alloc_size}, so pointer to {ptr_size} byte{ptr_size_p} starting at offset {ptr_offset} is out-of-bounds",
alloc_size = alloc_size.bytes(),
ptr_size = ptr_size.bytes(),
ptr_size_p = pluralize!(ptr_size.bytes()),
ptr_offset = ptr_offset,
),
DanglingIntPointer(0, CheckInAllocMsg::InboundsTest) => {
write!(f, "null pointer is not a valid pointer for this operation")
}
DanglingIntPointer(0, msg) => {
write!(f, "{}null pointer is not a valid pointer", msg)
write!(f, "{msg}null pointer is not a valid pointer")
}
DanglingIntPointer(i, msg) => {
write!(f, "{}0x{:x} is not a valid pointer", msg, i)
write!(f, "{msg}{:#x} is not a valid pointer", i)
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
}
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),
DerefFunctionPointer(a) => write!(f, "accessing {} which contains a function", a),
WriteToReadOnly(a) => write!(f, "writing to {a:?} which is read-only"),
DerefFunctionPointer(a) => write!(f, "accessing {a:?} which contains a function"),
ValidationFailure { path: None, msg } => {
write!(f, "constructing invalid value: {}", msg)
}
Expand Down
12 changes: 4 additions & 8 deletions compiler/rustc_middle/src/mir/interpret/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,11 +190,7 @@ impl fmt::Debug for AllocId {
}
}

impl fmt::Display for AllocId {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
fmt::Debug::fmt(self, f)
}
}
// No "Display" since AllocIds are not usually user-visible.

#[derive(TyDecodable, TyEncodable)]
enum AllocDiscriminant {
Expand Down Expand Up @@ -470,7 +466,7 @@ impl<'tcx> TyCtxt<'tcx> {
return alloc_id;
}
let id = alloc_map.reserve();
debug!("creating alloc {:?} with id {}", alloc, id);
debug!("creating alloc {:?} with id {:?}", alloc, id);
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
alloc_map.alloc_map.insert(id, alloc.clone());
alloc_map.dedup.insert(alloc, id);
id
Expand Down Expand Up @@ -538,15 +534,15 @@ impl<'tcx> TyCtxt<'tcx> {
pub fn global_alloc(self, id: AllocId) -> GlobalAlloc<'tcx> {
match self.get_global_alloc(id) {
Some(alloc) => alloc,
None => bug!("could not find allocation for {}", id),
None => bug!("could not find allocation for {id:?}"),
}
}

/// Freezes an `AllocId` created with `reserve` by pointing it at an `Allocation`. Trying to
/// call this function twice, even with the same `Allocation` will ICE the compiler.
pub fn set_alloc_id_memory(self, id: AllocId, mem: ConstAllocation<'tcx>) {
if let Some(old) = self.alloc_map.lock().alloc_map.insert(id, GlobalAlloc::Memory(mem)) {
bug!("tried to set allocation ID {}, but it was already existing as {:#?}", id, old);
bug!("tried to set allocation ID {id:?}, but it was already existing as {old:#?}");
}
}

Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_middle/src/mir/interpret/pointer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ impl Provenance for AllocId {
}
// Print offset only if it is non-zero.
if ptr.offset.bytes() > 0 {
write!(f, "+0x{:x}", ptr.offset.bytes())?;
write!(f, "+{:#x}", ptr.offset.bytes())?;
}
Ok(())
}
Expand Down Expand Up @@ -181,7 +181,7 @@ impl<Tag: Provenance> fmt::Debug for Pointer<Option<Tag>> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self.provenance {
Some(tag) => Provenance::fmt(&Pointer::new(tag, self.offset), f),
None => write!(f, "0x{:x}", self.offset.bytes()),
None => write!(f, "{:#x}", self.offset.bytes()),
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_middle/src/mir/interpret/value.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ impl<Tag: Provenance> fmt::LowerHex for Scalar<Tag> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
Scalar::Ptr(ptr, _size) => write!(f, "pointer to {:?}", ptr),
Scalar::Int(int) => write!(f, "0x{:x}", int),
Scalar::Int(int) => write!(f, "{:#x}", int),
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_middle/src/mir/pretty.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -716,12 +716,12 @@ pub fn write_allocations<'tcx>(
}
write!(w, "{}", display_allocation(tcx, alloc.inner()))
};
write!(w, "\n{}", id)?;
write!(w, "\n{id:?}")?;
match tcx.get_global_alloc(id) {
// This can't really happen unless there are bugs, but it doesn't cost us anything to
// gracefully handle it and allow buggy rustc to be debugged via allocation printing.
None => write!(w, " (deallocated)")?,
Some(GlobalAlloc::Function(inst)) => write!(w, " (fn: {})", inst)?,
Some(GlobalAlloc::Function(inst)) => write!(w, " (fn: {inst})")?,
Some(GlobalAlloc::Static(did)) if !tcx.is_foreign_item(did) => {
match tcx.eval_static_initializer(did) {
Ok(alloc) => {
Expand Down
4 changes: 4 additions & 0 deletions compiler/rustc_middle/src/ty/consts/int.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -452,6 +452,10 @@ impl fmt::Debug for ScalarInt {
impl fmt::LowerHex for ScalarInt {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
self.check_data();
if f.alternate() {
// Like regular ints, alternate flag adds leading `0x`.
write!(f, "0x")?;
}
// Format as hex number wide enough to fit any value of the given `size`.
// So data=20, size=1 will be "0x14", but with size=4 it'll be "0x00000014".
// Using a block `{self.data}` here to force a copy instead of using `self.data`
Expand Down