Miri interning: replace ICEs by proper errors #71665

RalfJung · 2020-04-29T10:56:20Z

I also did some refactoring, as I kept being confused by all the parameters to intern_shallow, some of which have invalid combinations (such as a mutable const). So instead InternMode now contains all the information that is needed and invalid combinations are ruled out by the type system.

Also I removed interpreter errors from interning. We already ignored almost all errors, and the ValidationFailure errors that we handled separately actually cannot ever happen here. The only interpreter failure that was actually reachable was the UB on dangling pointers -- and arguably, a dangling raw pointer is not UB, so the error was not even correct. It's just that the rest of the compiler does not like "dangling" AllocId.

It should be possible to review the 3 commits separately.

r? @oli-obk
Cc @rust-lang/wg-const-eval

RalfJung · 2020-04-29T10:57:05Z

src/librustc_mir/interpret/eval_context.rs

@@ -869,6 +869,9 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
        // Our result will later be validated anyway, and there seems no good reason
        // to have to fail early here.  This is also more consistent with
        // `Memory::get_static_alloc` which has to use `const_eval_raw` to avoid cycles.
+        // FIXME: We can hit delay_span_bug if this is an invalid const, interning finds
+        // that problem, but we never run validation to show an error. Can we ensure
+        // this does not happen?


I still need to try and actually case the ICE here -- but my worry is that the delay_span_bug in interning all assume that validation will be run after interning is done, and this code here violates that assumption.

Turns out we can get that to ICE even without Miri:

#![feature(const_transmute)] #![warn(const_err)] use std::mem; const X: &i32 = unsafe { let x = 0; mem::transmute(&x) };

Do you want to add a test for this in this PR or fix it in this PR?

The PR does contain a test for the ICE outside Miri -- mostly accidentally, when I added allow(const_err) I did not realize I was testing an ICE. ;)

For whether this can ICE with Miri, I haven't done another attempt at triggering it (this one specifically is what I am worried about).

src/test/ui/consts/dangling-alloc-id-ice.rs

src/librustc_mir/interpret/intern.rs

oli-obk · 2020-05-01T10:39:49Z

refactorings of the interning logic have historically led to regressions of some sort. While the changes look benign to me, I wonder if we should do a crater run.

RalfJung · 2020-05-01T11:10:22Z

And our test suite got better each time. ;)

But, sure... a check-only run should suffice, right?
@bors try

bors · 2020-05-01T11:10:33Z

⌛ Trying commit 369bef48dff01605aff7bf377e101de0f0046d35 with merge d7b51666469c792a8a3f40849607739bae1e705e...

bors · 2020-05-01T14:12:27Z

☀️ Try build successful - checks-azure
Build commit: d7b51666469c792a8a3f40849607739bae1e705e (d7b51666469c792a8a3f40849607739bae1e705e)

RalfJung · 2020-05-01T14:22:44Z

@craterbot check

craterbot · 2020-05-01T14:22:49Z

👌 Experiment pr-71665 created and queued.
🤖 Automatically detected try build d7b51666469c792a8a3f40849607739bae1e705e
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2020-05-07T10:42:44Z

🚧 Experiment pr-71665 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

bors · 2020-05-09T21:07:32Z

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

…pe signature more precise

Co-Authored-By: Oliver Scherer <github35764891676564198441@oli-obk.de>

craterbot · 2020-05-13T11:28:07Z

🎉 Experiment pr-71665 is completed!
📊 1 regressed and 3 fixed (102442 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

oli-obk · 2020-05-13T11:34:05Z

The only regression is a network failure 🎉

RalfJung · 2020-05-13T11:44:17Z

src/librustc_mir/interpret/intern.rs

-            // a check in validation.
+            // to validation to error -- it has the much better error messages, pointing out where
+            // in the value the dangling reference lies.
+            // The `delay_span_bug` ensures that we don't forget such a check in validation.
            if tcx.get_global_alloc(alloc_id).is_none() {
                tcx.sess.delay_span_bug(ecx.tcx.span, "tried to intern dangling pointer");


Re: ICEs, this one is fine because interning itself does tcx.sess.span_err, so validation does not even enter the picture. I should probably adjust the comment here.

RalfJung · 2020-05-13T11:46:24Z

src/librustc_mir/interpret/intern.rs

+        match res {
+            Ok(()) => {}
+            Err(error) => {
+                ecx.tcx.sess.delay_span_bug(


This and the vtable delay_span_bug are the ones I am worried about, in case validation does not run.

RalfJung · 2020-05-13T11:48:36Z

src/librustc_mir/interpret/intern.rs

-            throw_ub_format!("encountered dangling pointer in final constant")
+            // Codegen does not like dangling pointers, and generally `tcx` assumes that
+            // all allocations referenced anywhere actually exist. So, make sure we error here.
+            ecx.tcx.sess.span_err(ecx.tcx.span, "encountered dangling pointer in final constant");


I assume it is too late here to adjust the allocations containing the dangling pointer? Because one thing we could do is replace all pointers to it with a (dangling) integer pointer (taking the alignment of the allocation as its base address). That would maintain data structure consistency (no dangling AllocId in tcx) and avoid having to hard-error here.

But that would of course affect this as well.

We could pull the remove invocation upfront to where elements are added to todo and put (AllocId, Allocation) pairs into todo. Then we'll only have to figure out how to make the visitor allow mutating the value in place. Likely that will require a sort of MutVisitor/Visitor duplication like mir::Visitor has.

Simpler would probably be to do a first pass over an Allocation and replace all dangling AllocIds in its relocation with integer pointers, but then we have a hard time telling apart user's integer pointers from our synthetic ones.

So the only thing that I can think of is to make this site a delay_span_bug and to do the "replace all dangling AllocIdspass just before invokingintern_const_alloc`

I don't think we can make this a delay_span_bug here, dangling raw pointers would not be a "bug" if we have synthetic ones.

I guess the weirdest part about the synthetic pointers is that inequal pointers could become equal, which doesn't seem great. Maybe we should just synthesize 1-byte global allocations and make them point to that?

Anyway I think that is something which can be resolved later, after landing this PR.

I guess the weirdest part about the synthetic pointers is that inequal pointers could become equal, which doesn't seem great. Maybe we should just synthesize 1-byte global allocations and make them point to that?

I'd prefer to just use integer pointers. There's no way their identity is important (if they were pointing to a static, they wouldn't be dangling), so deduplication is totally permissible.

Opened an issue for this: #72215

oli-obk · 2020-05-13T15:49:42Z

@bors r+

bors · 2020-05-13T15:49:43Z

📌 Commit e73ee41 has been approved by oli-obk

@oli-obk

Miri interning: replace ICEs by proper errors Fixes rust-lang#71316 I also did some refactoring, as I kept being confused by all the parameters to `intern_shallow`, some of which have invalid combinations (such as a mutable const). So instead `InternMode` now contains all the information that is needed and invalid combinations are ruled out by the type system. Also I removed interpreter errors from interning. We already ignored almost all errors, and the `ValidationFailure` errors that we handled separately actually cannot ever happen here. The only interpreter failure that was actually reachable was the UB on dangling pointers -- and arguably, a dangling raw pointer is not UB, so the error was not even correct. It's just that the rest of the compiler does not like "dangling" `AllocId`. It should be possible to review the 3 commits separately. r? @oli-obk Cc @rust-lang/wg-const-eval

@oli-obk

Miri interning: replace ICEs by proper errors Fixes rust-lang#71316 I also did some refactoring, as I kept being confused by all the parameters to `intern_shallow`, some of which have invalid combinations (such as a mutable const). So instead `InternMode` now contains all the information that is needed and invalid combinations are ruled out by the type system. Also I removed interpreter errors from interning. We already ignored almost all errors, and the `ValidationFailure` errors that we handled separately actually cannot ever happen here. The only interpreter failure that was actually reachable was the UB on dangling pointers -- and arguably, a dangling raw pointer is not UB, so the error was not even correct. It's just that the rest of the compiler does not like "dangling" `AllocId`. It should be possible to review the 3 commits separately. r? @oli-obk Cc @rust-lang/wg-const-eval

bors · 2020-05-16T07:14:58Z

⌛ Testing commit e73ee41 with merge 584252b...

bors · 2020-05-16T10:32:40Z

☀️ Test successful - checks-azure
Approved by: oli-obk
Pushing 584252b to master...

rust-highfive assigned oli-obk Apr 29, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 29, 2020

RalfJung commented Apr 29, 2020

View reviewed changes

src/test/ui/consts/dangling-alloc-id-ice.rs Show resolved Hide resolved

oli-obk reviewed Apr 29, 2020

View reviewed changes

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

oli-obk reviewed Apr 29, 2020

View reviewed changes

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

oli-obk reviewed Apr 29, 2020

View reviewed changes

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

oli-obk reviewed Apr 29, 2020

View reviewed changes

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

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 1, 2020

RalfJung mentioned this pull request May 2, 2020

ICE with unleash-miri: mutable allocation in constant #71316

Closed

RalfJung force-pushed the miri-intern-no-ice branch from 369bef4 to 423f0f1 Compare May 4, 2020 09:29

RalfJung mentioned this pull request May 8, 2020

Make const_err a future-incompatibility lint and eventually a hard error #71800

Closed

RalfJung and others added 6 commits May 10, 2020 11:45

Miri interning: replace ICEs by proper errors, make intern_shallow ty…

c400f75

…pe signature more precise

remove some dead code, and assert we do not swallow allocating errors

8e48a30

avoid raising interpreter errors from interning

ff39457

fmt

d3b2e1f

Typo

a06740c

Co-Authored-By: Oliver Scherer <github35764891676564198441@oli-obk.de>

rebase fallout

e73ee41

RalfJung force-pushed the miri-intern-no-ice branch from 423f0f1 to e73ee41 Compare May 10, 2020 09:49

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels May 13, 2020

RalfJung commented May 13, 2020

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 13, 2020

This was referenced May 14, 2020

Rollup of 9 pull requests #72203

Closed

Miri interning: what to do with dangling pointers? #72215

Closed

RalfJung mentioned this pull request May 15, 2020

Rollup of 5 pull requests #72226

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label May 16, 2020

bors merged commit 584252b into rust-lang:master May 16, 2020

bors mentioned this pull request May 16, 2020

[DRAFT] Make lang items private #72249

Closed

RalfJung deleted the miri-intern-no-ice branch May 17, 2020 08:59

This was referenced May 17, 2020

ICE mutable allocation in constant ui/associated-type-bounds/union-bounds.rs with -Zmir-opt-level=2 #68296

Closed

Static checks for &mut in final value of constant with #![feature(const_mut_refs)] #71212

Closed

RalfJung mentioned this pull request Jun 2, 2020

ICE: delay_span_bug const-construct-call.rs with mir-opt-level=2 #72679

Closed

dimpolo mentioned this pull request Oct 15, 2020

Old vtable example fails with: "error: untyped pointers are not allowed in constant" #77995

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Miri interning: replace ICEs by proper errors #71665

Miri interning: replace ICEs by proper errors #71665

RalfJung commented Apr 29, 2020

RalfJung Apr 29, 2020

RalfJung May 6, 2020

oli-obk May 13, 2020

RalfJung May 13, 2020 •

edited

Loading

oli-obk commented May 1, 2020

RalfJung commented May 1, 2020

bors commented May 1, 2020

bors commented May 1, 2020

RalfJung commented May 1, 2020

craterbot commented May 1, 2020

craterbot commented May 7, 2020

bors commented May 9, 2020

craterbot commented May 13, 2020

oli-obk commented May 13, 2020

RalfJung May 13, 2020

RalfJung May 13, 2020

RalfJung May 13, 2020 •

edited

Loading

oli-obk May 13, 2020

RalfJung May 13, 2020

oli-obk May 13, 2020

oli-obk May 13, 2020

RalfJung May 14, 2020

oli-obk commented May 13, 2020

bors commented May 13, 2020

bors commented May 16, 2020

bors commented May 16, 2020

Miri interning: replace ICEs by proper errors #71665

Miri interning: replace ICEs by proper errors #71665

Conversation

RalfJung commented Apr 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung May 13, 2020 • edited Loading

Choose a reason for hiding this comment

oli-obk commented May 1, 2020

RalfJung commented May 1, 2020

bors commented May 1, 2020

bors commented May 1, 2020

RalfJung commented May 1, 2020

craterbot commented May 1, 2020

craterbot commented May 7, 2020

bors commented May 9, 2020

craterbot commented May 13, 2020

oli-obk commented May 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung May 13, 2020 • 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

oli-obk commented May 13, 2020

bors commented May 13, 2020

bors commented May 16, 2020

bors commented May 16, 2020

RalfJung May 13, 2020 •

edited

Loading

RalfJung May 13, 2020 •

edited

Loading