Use a DFS to compare CTFE snapshots #66946

ecstatic-morse · 2019-12-02T04:11:59Z

This is an alternative fix to #52475 that hopes to address some parts of #54384.

The major development in this PR is the use of an actual depth-first search to compare the heap between snapshots, instead of the current recursive method. We treat the set of pointers on the stack as an ordered list of "roots". Using this as the initial search stack, a DFS can enumerate all reachable memory in a predictable order. This is important, since without a stable ordering, we would need to compute an isomorphism for the allocation graph of each snapshot to compare them.

Equality between snapshots is checked by iterating in lock-step over their allocation graphs and comparing each Allocation using an IgnoreAllocId wrapper with custom PartialEq and Hash impls. If the allocations are equivalent but for AllocIds, all pointers in the allocation are mapped to a DFS index and then compared.

I find the existing implementation pretty inscrutable, so I'm not equipped to speculate on performance differences. At the very least, my method will use less memory when a collision does occur, since we no longer clone Memory in full, just the reachable portions.

Some aspects of #54384 remain unaddressed, most notably what to do when we fail to resolve an AllocId. My PR takes what I believe is the same approach as the existing implementation; it converts the InterpError into a None and continues traversing the allocation graph. I don't know what the correct behavior would be.

r? @RalfJung
cc @oli-obk

RalfJung · 2019-12-02T08:19:12Z

src/librustc/mir/interpret/pointer.rs

+    /// Erase any allocation IDs.
+    ///
+    /// Used when comparing heap snapshots.
+    pub fn erase_id(self) -> Pointer<Tag, ()> {


Please rename to erase_alloc_id.

But isn't it easier to just say ptr1.offset == ptr2.offset? Pointers with AllocId type () are kind of pointless (all puns intended).

Oh, you are recursively doing through even for Place. But how can that make any sense, given that all AllocIds are considered equal at this point...?

👍 to erase_alloc_id.

This is the part that makes AllocIds compare as equal. It means we can use the derived PartialEq impl. The alternative is to overload AllocIdInvariant for more types, which would also be fine.

But why does it make any sense to compare just the offsets?

Other code is responsible for comparing the allocation that each AllocId references, specifically the PartialEq implementation of InterpRef. Am I misunderstanding the question?

src/librustc_mir/interpret/operand.rs

src/librustc_mir/interpret/snapshot.rs

RalfJung · 2019-12-02T08:24:07Z

src/librustc_mir/const_eval.rs

+        if ecx.machine.loop_detector.is_empty() {
+            // FIXME(#49980): make this warning a lint
+            ecx.tcx.sess.span_warn(span,
+                "Constant evaluating a complex constant, this might take some time");
+        }


Why did you move this out? Seems like a layering violation for this code to care about "emptiness" of the loop detector.

Oh I see, you could remove the tcx and span arguments then. That is a good argument, but is_empty is not a great name.

has_been_invoked?

No that's negated... is_unused?

!has_been_invoked is negated but is_unused isn't?

is_being_invoked_the_first_time() 🙃

!has_been_invoked is negated but is_unused isn't?

Well I wanted to avoid the ! is all. It's way too easy to miss.

src/librustc_mir/interpret/snapshot.rs

RalfJung · 2019-12-02T08:27:47Z

src/librustc_mir/interpret/snapshot.rs

-    type Item = FrameSnapshot<'a, 'tcx>;
+/// A wrapper type for the various interpreter data structures that compares them without
+/// considering the numeric value of `AllocId`s.
+struct AllocIdInvariant<'a, T>(&'a T);


"invariant" is an odd term here, that indicates that something doesn't change.

IgnoreAllocId sounds more accurate? But I have no idea why it even makes any sense to do this.^^

Yeah, I would not be opposed to reverting to the AllocId-sensitive method, or removing this entirely. It is pretty cool when you accidentally omit a break and you're told there's a provably infinite loop in your program, and I have some sentimental connection to this code since it was my first real rust PR. However, I think a configurable instruction limit (with a low default) coupled with a real lint would have basically the same effect.

Once loops are implemented, people will actually start hitting bugs/performance issues in this code. I wanted to make sure I understood what's going on now. I'm worried people will see long-running constant evaluations grind to a halt because of this machinery and have no way to turn it off.

cc @oli-obk

RalfJung · 2019-12-02T08:30:07Z

Some aspects of #54384 remain unaddressed, most notably what to do when we fail to resolve an AllocId. My PR takes what I believe is the same approach as the existing implementation; it converts the InterpError into a None and continues traversing the allocation graph. I don't know what the correct behavior would be.

This means we have a dangling pointer, right?

RalfJung · 2019-12-02T08:31:55Z

src/librustc_mir/interpret/memory.rs

+        let id = stack.pop()?;
+        let alloc = memory.get_raw(id);
+
+        if let Ok(alloc) = alloc {


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?

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?

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

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?

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.

src/librustc_mir/interpret/memory.rs

ecstatic-morse · 2019-12-02T17:40:21Z

src/librustc_mir/interpret/snapshot.rs

-        // dependencies.
-        let bytes = self.inspect_with_undef_and_ptr_outside_interpreter(all_bytes);
+        live_allocs.map(|(_, alloc)| alloc.ok().map(AllocIdInvariant))
+            .eq(other_live_allocs.map(|(_, alloc)| alloc.ok().map(AllocIdInvariant)))


@RalfJung. I discard the contents of the InterpError with ok. Do you think we can differentiate between null pointers, pointers to ZSTs and dangling pointers by looking at InterpError like you mentioned above? Do ZST pointers have zero-sized backing allocations?

oli-obk

I started a review, and forgot about it. I'll continue it later. For now just dumping the little status it has

oli-obk · 2019-12-03T09:32:41Z

src/librustc_mir/const_eval.rs

+        if ecx.machine.loop_detector.is_empty() {
+            // FIXME(#49980): make this warning a lint
+            ecx.tcx.sess.span_warn(span,
+                "Constant evaluating a complex constant, this might take some time");
+        }


is_being_invoked_the_first_time() 🙃

src/librustc_mir/interpret/eval_context.rs

ecstatic-morse · 2019-12-08T19:49:00Z

@oli-obk, @RalfJung the necessary changes have been made to handle memory cycles. I also added a (very wordy) high-level explanation of the new code in the module docs. Finally, I resolved many of the concerns, although I would still like to do something to address this one.

src/librustc_mir/interpret/snapshot.rs

oli-obk · 2019-12-09T12:55:23Z

src/librustc_mir/interpret/snapshot.rs

+//!
+//! ```rust,ignore(const-loop)
+//! const fn inf_loop() {
+//!     // Function call to prevent promotion.


inside const fns promotion is not prevented for const fns, also memoization will memoize this 🙃

You'll want const fn zeros(_: &i32) -> [isize; 4] { [0; 4] } and pass a local variable as a reference at the call site.

Doh! Can't believe I missed this. My worry is that the test for issue 52475 isn't really creating new AllocIds, since the &0 is getting promoted out.

src/librustc_mir/interpret/snapshot.rs

This method is based on `erase_tag`, and replaces `AllocId`s with `()`.

Co-Authored-By: Oliver Scherer <[email protected]>

bors · 2019-12-15T04:48:03Z

☔ The latest upstream changes (presumably #67216) made this pull request unmergeable. Please resolve the merge conflicts.

RalfJung · 2019-12-21T16:52:22Z

What is the long-term plan here? From other conversation, it seems like one plan was to remove the loop checker entirely. So we probably should finish that discussion before spending more time on this PR?

Also I got quite a backlog of PRs to handle so @oli-obk if you could take over review that would be much appreciated.

RalfJung · 2019-12-21T16:54:22Z

src/librustc_mir/interpret/snapshot.rs

+//! Instead of comparing `AllocId`s between snapshots, we first map `AllocId`s in each snapshot to
+//! their DFS index, then compare those instead.


That makes sense. However, what I would have expected then is to see Pointer<Tag, DfsIndex> instead of Pointer<Tag, AllocId>, and then one would compare those. What I still don't get is why we have a way to compare pointers ignoring the AllocId. That means we must have a magic scheme somewhere to figure out all the AllocId we ignored and compare their DfsIndex instead? That sounds quite round-about and fragile, on first sight. (I didn't read the code, though.)

oli-obk · 2019-12-21T18:12:09Z

r? @oli-obk

oli-obk · 2019-12-21T23:42:24Z

From other conversation, it seems like one plan was to remove the loop checker entirely. So we probably should finish that discussion before spending more time on this PR?

This was discussed in https://rust-lang.zulipchat.com/#narrow/stream/146212-t-compiler.2Fconst-eval/topic/Time.20limits.20during.20const-eval/near/183588492

and from the last comment

I'm gonna open a PR removing the loop detector after #67260 gets merged.

I am assuming that we can close this PR

@oli-obk

…op-detector, r=RalfJung Remove const eval loop detector Now that there is a configurable instruction limit for CTFE (see rust-lang#67260), we can replace the loop detector with something much simpler. See rust-lang#66946 for more discussion about this. Although the instruction limit is nightly-only, the only practical way to reach the default limit uses nightly-only features as well (although CTFE will still execute code using such features inside an array initializer on stable). This will at the very least require a crater run, since it will result in an error wherever the "long running const eval" warning appeared before. We may need to increase the default for `const_eval_limit` to work around this. Resolves rust-lang#54384 cc rust-lang#49980 r? @oli-obk cc @RalfJung

@oli-obk

…op-detector, r=RalfJung Remove const eval loop detector Now that there is a configurable instruction limit for CTFE (see rust-lang#67260), we can replace the loop detector with something much simpler. See rust-lang#66946 for more discussion about this. Although the instruction limit is nightly-only, the only practical way to reach the default limit uses nightly-only features as well (although CTFE will still execute code using such features inside an array initializer on stable). This will at the very least require a crater run, since it will result in an error wherever the "long running const eval" warning appeared before. We may need to increase the default for `const_eval_limit` to work around this. Resolves rust-lang#54384 cc rust-lang#49980 r? @oli-obk cc @RalfJung

rust-highfive assigned RalfJung Dec 2, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 2, 2019

ecstatic-morse force-pushed the better-infinite-loop branch 2 times, most recently from 56a760c to 077a810 Compare December 2, 2019 04:44

ecstatic-morse changed the title ~~Use a DFS to compare CTFE snapshots~~ [WIP] Use a DFS to compare CTFE snapshots Dec 2, 2019

RalfJung reviewed Dec 2, 2019

View reviewed changes

src/librustc_mir/interpret/operand.rs Outdated Show resolved Hide resolved

RalfJung reviewed Dec 2, 2019

View reviewed changes

src/librustc_mir/interpret/snapshot.rs Outdated Show resolved Hide resolved

RalfJung reviewed Dec 2, 2019

View reviewed changes

src/librustc_mir/interpret/snapshot.rs Outdated Show resolved Hide resolved

RalfJung reviewed Dec 2, 2019

View reviewed changes

src/librustc_mir/interpret/memory.rs Show resolved Hide resolved

ecstatic-morse commented Dec 2, 2019

View reviewed changes

oli-obk reviewed Dec 4, 2019

View reviewed changes

ecstatic-morse force-pushed the better-infinite-loop branch 4 times, most recently from 4027611 to baac4af Compare December 8, 2019 19:41

ecstatic-morse changed the title ~~[WIP] Use a DFS to compare CTFE snapshots~~ Use a DFS to compare CTFE snapshots Dec 8, 2019

oli-obk reviewed Dec 9, 2019

View reviewed changes

ecstatic-morse added 7 commits December 9, 2019 13:01

Add erase_alloc_id method to various Miri primitives

6faae4d

This method is based on `erase_tag`, and replaces `AllocId`s with `()`.

Derive Hash and Eq for some Miri primitives

f0148a5

Add DFS iterator for Memory

840e323

Remove unused Clone impl for Memory

fa5ff3a

Reimplement CTFE snapshot comparison using a DFS

37844e5

Rename is_empty to has_been_invoked

26b9146

Move CtfeMemory alias next to CompileTimeInterpreter

345a8d5

ecstatic-morse and others added 2 commits December 9, 2019 13:01

Fix typo

e017f57

Co-Authored-By: Oliver Scherer <[email protected]>

Hash discriminant of LocalValue

afb4faa

ecstatic-morse force-pushed the better-infinite-loop branch from a168d70 to afb4faa Compare December 9, 2019 21:04

Ensure promotion does not prevent generation of new AllocIds

ba22627

ecstatic-morse mentioned this pull request Dec 11, 2019

Enable loop and while in constants behind a feature flag #67216

Merged

RalfJung reviewed Dec 21, 2019

View reviewed changes

rust-highfive assigned oli-obk and unassigned RalfJung Dec 21, 2019

oli-obk closed this Dec 21, 2019

ecstatic-morse mentioned this pull request Mar 17, 2020

Remove const eval loop detector #70087

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a DFS to compare CTFE snapshots #66946

Use a DFS to compare CTFE snapshots #66946

ecstatic-morse commented Dec 2, 2019 •

edited

Loading

RalfJung Dec 2, 2019

RalfJung Dec 2, 2019

ecstatic-morse Dec 2, 2019 •

edited

Loading

RalfJung Dec 2, 2019

ecstatic-morse Dec 2, 2019

RalfJung Dec 2, 2019

RalfJung Dec 2, 2019

ecstatic-morse Dec 2, 2019

RalfJung Dec 2, 2019

ecstatic-morse Dec 2, 2019 •

edited

Loading

oli-obk Dec 3, 2019

RalfJung Dec 4, 2019

RalfJung Dec 2, 2019

ecstatic-morse Dec 2, 2019

RalfJung commented Dec 2, 2019

RalfJung Dec 2, 2019

ecstatic-morse Dec 8, 2019

oli-obk Dec 9, 2019

oli-obk Dec 9, 2019

ecstatic-morse Dec 9, 2019

ecstatic-morse Dec 2, 2019

oli-obk left a comment

oli-obk Dec 3, 2019

ecstatic-morse commented Dec 8, 2019

oli-obk Dec 9, 2019

ecstatic-morse Dec 9, 2019

bors commented Dec 15, 2019

RalfJung commented Dec 21, 2019

RalfJung Dec 21, 2019

oli-obk commented Dec 21, 2019

oli-obk commented Dec 21, 2019

		//! Instead of comparing `AllocId`s between snapshots, we first map `AllocId`s in each snapshot to
		//! their DFS index, then compare those instead.

Use a DFS to compare CTFE snapshots #66946

Use a DFS to compare CTFE snapshots #66946

Conversation

ecstatic-morse commented Dec 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecstatic-morse Dec 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecstatic-morse Dec 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Dec 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecstatic-morse commented Dec 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Dec 15, 2019

RalfJung commented Dec 21, 2019

Choose a reason for hiding this comment

oli-obk commented Dec 21, 2019

oli-obk commented Dec 21, 2019

ecstatic-morse commented Dec 2, 2019 •

edited

Loading

ecstatic-morse Dec 2, 2019 •

edited

Loading

ecstatic-morse Dec 2, 2019 •

edited

Loading