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.

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

Use a DFS to compare CTFE snapshots #66946

Closed
7 changes: 7 additions & 0 deletions src/librustc/mir/interpret/pointer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,13 @@ impl<'tcx, Tag> Pointer<Tag> {
Pointer { alloc_id: self.alloc_id, offset: self.offset, tag: () }
}

/// Replace any allocation IDs in this type with `()`.
///
/// Used when comparing heap snapshots.
pub fn erase_alloc_id(self) -> Pointer<Tag, ()> {
Pointer { alloc_id: (), offset: self.offset, tag: self.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.
Expand Down
20 changes: 20 additions & 0 deletions src/librustc/mir/interpret/value.rs
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,16 @@ impl<'tcx, Tag> Scalar<Tag> {
}
}

/// Replace any allocation IDs in this type with `()`.
///
/// Used when comparing heap snapshots.
pub fn erase_alloc_id(self) -> Scalar<Tag, ()> {
match self {
Scalar::Ptr(ptr) => Scalar::Ptr(ptr.erase_alloc_id()),
Scalar::Raw { data, size } => Scalar::Raw { data, size },
}
}

#[inline]
pub fn ptr_null(cx: &impl HasDataLayout) -> Self {
Scalar::Raw {
Expand Down Expand Up @@ -509,6 +519,16 @@ impl<'tcx, Tag> ScalarMaybeUndef<Tag> {
}
}

/// Replace any allocation IDs in this type with `()`.
///
/// Used when comparing heap snapshots.
pub fn erase_alloc_id(self) -> ScalarMaybeUndef<Tag, ()> {
match self {
ScalarMaybeUndef::Scalar(s) => ScalarMaybeUndef::Scalar(s.erase_alloc_id()),
ScalarMaybeUndef::Undef => ScalarMaybeUndef::Undef,
}
}

#[inline]
pub fn not_undef(self) -> InterpResult<'static, Scalar<Tag>> {
match self {
Expand Down
19 changes: 12 additions & 7 deletions src/librustc_mir/const_eval.rs
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,8 @@ impl interpret::MayLeak for ! {
}
}

pub type CtfeMemory<'mir, 'tcx> = Memory<'mir, 'tcx, CompileTimeInterpreter<'mir, 'tcx>>;

impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir, 'tcx> {
type MemoryKinds = !;
type PointerTag = ();
Expand Down Expand Up @@ -445,7 +447,7 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,
}

fn ptr_to_int(
_mem: &Memory<'mir, 'tcx, Self>,
_mem: &CtfeMemory<'mir, 'tcx>,
_ptr: Pointer,
) -> InterpResult<'tcx, u64> {
Err(
Expand Down Expand Up @@ -514,13 +516,16 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,
}
}

// Emit a warning if this is the first time we have triggered the loop detector. This means
// we have been executing for quite some time.
let span = ecx.frame().span;
ecx.machine.loop_detector.observe_and_analyze(
*ecx.tcx,
span,
&ecx.memory,
&ecx.stack[..],
)
if !ecx.machine.loop_detector.has_been_invoked() {
// FIXME(#49980): make this warning a lint
ecx.tcx.sess.span_warn(span,
"Constant evaluating a complex constant, this might take some time");
}

ecx.machine.loop_detector.observe_and_analyze(&ecx.stack, &ecx.memory)
}

#[inline(always)]
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_mir/interpret/eval_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ pub struct LocalState<'tcx, Tag=(), Id=AllocId> {
}

/// Current value of a local variable
#[derive(Clone, PartialEq, Eq, Debug, HashStable)] // Miri debug-prints these
#[derive(Clone, PartialEq, Eq, Debug, Hash, HashStable)] // Miri debug-prints these
pub enum LocalValue<Tag=(), Id=AllocId> {
/// This local is not currently alive, and cannot be used at all.
Dead,
Expand Down
73 changes: 54 additions & 19 deletions src/librustc_mir/interpret/memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use super::{
Machine, AllocMap, MayLeak, ErrorHandled, CheckInAllocMsg,
};

#[derive(Debug, PartialEq, Copy, Clone)]
#[derive(Debug, PartialEq, Eq, Copy, Clone, Hash)]
pub enum MemoryKind<T> {
/// Stack memory. Error if deallocated except during a stack pop.
Stack,
Expand Down Expand Up @@ -114,24 +114,6 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> HasDataLayout for Memory<'mir, 'tcx, M>
}
}

// FIXME: Really we shouldn't clone memory, ever. Snapshot machinery should instead
// carefully copy only the reachable parts.
ecstatic-morse marked this conversation as resolved.
Show resolved Hide resolved
impl<'mir, 'tcx, M> Clone for Memory<'mir, 'tcx, M>
where
M: Machine<'mir, 'tcx, PointerTag = (), AllocExtra = (), MemoryExtra = ()>,
M::MemoryMap: AllocMap<AllocId, (MemoryKind<M::MemoryKinds>, Allocation)>,
{
fn clone(&self) -> Self {
Memory {
alloc_map: self.alloc_map.clone(),
extra_fn_ptr_map: self.extra_fn_ptr_map.clone(),
dead_alloc_map: self.dead_alloc_map.clone(),
extra: (),
tcx: self.tcx,
}
}
}

impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
pub fn new(tcx: TyCtxtAt<'tcx>, extra: M::MemoryExtra) -> Self {
Memory {
Expand Down Expand Up @@ -950,3 +932,56 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
}
}
}

/// A depth-first search over the allocation graph.
///
/// This is based on the DFS iterator in `rustc_data_structures`, which we cannot use directly
/// because `AllocId` does not implement `Idx` (it is not dense).
pub struct DepthFirstSearch<'mem, 'mir, 'tcx, M: Machine<'mir, 'tcx>> {
memory: &'mem Memory<'mir, 'tcx, M>,
visited: FxHashSet<AllocId>,
stack: Vec<AllocId>,
}

impl<M: Machine<'mir, 'tcx>> DepthFirstSearch<'mem, 'mir, 'tcx, M> {
/// Returns a new DFS iterator that will traverse all allocations reachable from the given
/// `AllocId`s.
///
/// The first node in `roots` will be the first node visited by the DFS.
pub fn with_roots(
memory: &'mem Memory<'mir, 'tcx, M>,
roots: impl IntoIterator<Item = AllocId>,
) -> Self {
let mut stack: Vec<_> = roots.into_iter().collect();
stack.reverse();

DepthFirstSearch {
memory,
visited: stack.iter().copied().collect(),
stack,
}
}
}

impl<M: Machine<'mir, 'tcx>> Iterator for DepthFirstSearch<'mem, 'mir, 'tcx, M> {
type Item = (AllocId, InterpResult<'tcx, &'mem Allocation<M::PointerTag, M::AllocExtra>>);

fn next(&mut self) -> Option<Self::Item> {
let DepthFirstSearch { stack, visited, memory } = self;

let id = stack.pop()?;
let alloc = memory.get_raw(id);

if let Ok(alloc) = alloc {
Copy link
Member

Choose a reason for hiding this comment

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

Is this where you are ignoring InterpError? This seems reasonable.

We should make sure that we don't catch errors requiring allocation though, similar to this. Maybe create a new helper method at the error type for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what you mean by "errors requiring allocation". Are there certain errors that we should bug on instead of ignoring? Is Ub one of these?

Copy link
Contributor

Choose a reason for hiding this comment

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

Any error type that has a String or other heap types in their fields will allocate when being thrown, and the catching will then destroy the allocated values. If this allocate->throw->catch->destroy happens in a high frequency we're wasting a lot of cycles with all the allocating/deallocating.

So, the idea is to make sure that any error that is caught and acted upon (and not just rethrown) does not allocate

Copy link
Contributor

Choose a reason for hiding this comment

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

Idea: Should we make the set of errors that Memory can emit a separate type (with appropriate Into impls) so we know the set of errors that Memory methods can emit and can make sure all of them don't allocate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense. I'll probably wait for someone else to do this? It feels like y'all have a decent idea of what you want here.

let new_pointers = alloc
.relocations()
.values()
.map(|&(_, id)| id)
.filter(|id| visited.insert(*id));

stack.extend(new_pointers);
}

Some((id, alloc))
}
}
21 changes: 21 additions & 0 deletions src/librustc_mir/interpret/operand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,17 @@ impl<'tcx, Tag> Immediate<Tag> {
Immediate::ScalarPair(a, b) => Ok((a.not_undef()?, b.not_undef()?))
}
}

/// Replace any allocation IDs in this type with `()`.
///
/// Used when comparing heap snapshots.
pub(super) fn erase_alloc_id(self) -> Immediate<Tag, ()> {
match self {
Immediate::Scalar(a) => Immediate::Scalar(a.erase_alloc_id()),
Immediate::ScalarPair(a, b)
=> Immediate::ScalarPair(a.erase_alloc_id(), b.erase_alloc_id()),
}
}
}

// ScalarPair needs a type to interpret, so we often have an immediate and a type together
Expand Down Expand Up @@ -176,6 +187,16 @@ impl<Tag> Operand<Tag> {

}
}

/// Replace any allocation IDs in this type with `()`.
///
/// Used when comparing heap snapshots.
pub(super) fn erase_alloc_id(self) -> Operand<Tag, ()> {
match self {
Operand::Immediate(imm) => Operand::Immediate(imm.erase_alloc_id()),
Operand::Indirect(place) => Operand::Indirect(place.erase_alloc_id()),
}
}
}

#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
Expand Down
11 changes: 11 additions & 0 deletions src/librustc_mir/interpret/place.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,17 @@ impl<Tag> MemPlace<Tag> {
}
}

/// Replace any allocation IDs in this type with `()`.
///
/// Used when comparing heap snapshots.
pub fn erase_alloc_id(self) -> MemPlace<Tag, ()> {
MemPlace {
ptr: self.ptr.erase_alloc_id(),
align: self.align,
meta: self.meta.map(Scalar::erase_alloc_id),
}
}

#[inline(always)]
pub fn from_scalar_ptr(ptr: Scalar<Tag>, align: Align) -> Self {
MemPlace {
Expand Down
Loading