Miri error reform #69839

RalfJung · 2020-03-08T22:52:09Z

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 #69762; relative diff.~~

r? @oli-obk

Fixes rust-lang/const-eval#4

src/librustc_mir/interpret/terminator.rs

src/librustc/mir/interpret/mod.rs

src/librustc/mir/interpret/pointer.rs

src/librustc_mir/interpret/memory.rs

oli-obk · 2020-03-09T16:09:01Z

src/test/ui/consts/const-eval/issue-49296.stderr

@@ -4,7 +4,7 @@ error: any use of this value will cause an error
 LL | const X: u64 = *wat(42);
   | ---------------^^^^^^^^-
   |                |
-   |                dangling pointer was dereferenced
+   |                pointer to alloc2 was dereferenced after this allocation got freed


Exposing alloc IDs to users seems suboptimal

Note that other errors exposed alloc IDs already, now it's just more of them.
For Miri I actually find this quite helpful as you can then re-run with "track alloc ID" to figure out when it got deallocated. Maybe there is a way to use formatting flags (like {:?} vs {:#?}) to switch between two different outputs modes for errors?

Let's use it as pressure to make debugging const eval memory possible for users

oli-obk · 2020-03-09T16:10:22Z

src/test/ui/consts/miri_unleashed/mutable_const.stderr

@@ -11,7 +11,7 @@ LL | / const MUTATING_BEHIND_RAW: () = {
 LL | |     // Test that `MUTABLE_BEHIND_RAW` is actually immutable, by doing this at const time.
 LL | |     unsafe {
 LL | |         *MUTABLE_BEHIND_RAW = 99
-   | |         ^^^^^^^^^^^^^^^^^^^^^^^^ tried to modify constant memory
+   | |         ^^^^^^^^^^^^^^^^^^^^^^^^ writing to alloc1 which is read-only


Exposing alloc IDs to users without that being useful to them until we start dumping allocations, too

src/test/ui/error-codes/E0396-fixed.stderr

oli-obk · 2020-03-10T14:19:51Z

@bors r+

bors · 2020-03-10T14:19:53Z

📌 Commit c0721934187d3ba9353a23454ef1afa068b7c3c4 has been approved by oli-obk

bors · 2020-03-10T14:19:54Z

🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened

Also slightly refactor pointer bounds checks to avoid creating unnecessary temporary Errors

RalfJung · 2020-03-18T13:53:49Z

Cc @pietroalbini something seems off, we got a timeout from bors.

@bors retry

bors · 2020-03-18T15:19:13Z

⌛ Testing commit e219dd4 with merge ec4687af1762d7f369389051b065d4e41d04245a...

@oli-obk

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

Centril · 2020-03-18T15:34:49Z

@bors retry rolled up

bors · 2020-03-18T15:53:12Z

⌛ Testing commit e219dd4 with merge 4ef96cbb98281ba9dff672730026bafdd6bc5092...

@oli-obk

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

Centril · 2020-03-18T15:57:09Z

@bors retry rolled up

@ghost

Rollup of 10 pull requests Successful merges: - rust-lang#67749 (keyword docs for else and inkeyword docs for else and in.) - rust-lang#69139 (clean up E0308 explanation) - rust-lang#69189 (Erase regions in writeback) - rust-lang#69837 (Use smaller discriminants for generators) - rust-lang#69838 (Expansion-driven outline module parsing) - rust-lang#69839 (Miri error reform) - rust-lang#69899 (Make methods declared by `newtype_index` macro `const`) - rust-lang#69920 (Remove some imports to the rustc crate) - rust-lang#70075 (Fix repr pretty display) - rust-lang#70106 (Tidy: fix running rustfmt twice) Failed merges: - rust-lang#70051 (Allow `hir().find` to return `None`) - rust-lang#70074 (Expand: nix all fatal errors) r? @ghost

rustup for error reform This is the Miri side of rust-lang/rust#69839.

eddyb · 2020-03-19T07:44:22Z

src/librustc/mir/interpret/error.rs

@@ -296,6 +294,8 @@ pub enum InvalidProgramInfo<'tcx> {
    TypeckError,
    /// An error occurred during layout computation.
    Layout(layout::LayoutError<'tcx>),
+    /// An invalid transmute happened.
+    TransmuteSizeDiff(Ty<'tcx>, Ty<'tcx>),


Given transmute checking, can't this be an ICE instead?

It was a bug! once and that was actually reachable, because Miri was run on invalid MIR. Also see

rust/src/librustc_mir/interpret/place.rs

Lines 924 to 929 in 57e1da5

// FIXME: This should be an assert instead of an error, but if we transmute within an

// array length computation, `typeck` may not have yet been run and errored out. In fact

// most likey we *are* running `typeck` right now. Investigate whether we can bail out

// on `typeck_tables().has_errors` at all const eval entry points.

debug!("Size mismatch when transmuting!\nsrc: {:#?}\ndest: {:#?}", src, dest);

throw_inval!(TransmuteSizeDiff(src.layout.ty, dest.layout.ty));

Oh I see, however:

you can't have MIR built without typeck, it just doesn't stop the compilation (so that comment is somewhat misleading)

I thought typeck_tables().has_errors being true resulted in a fake MIR body

typeck is not at all responsible for this, the check is a MIR pass, which also doesn't stop the compilation I guess, and has no real way of indicating a transmute is bogus

And this is why we have delay_span_bug, for all your "this should error elsewhere, ICE if it never does" needs.

delay_span_bug doesn't help, we still need to raise some error to abort interpretation.

delay_span_bug doesn't help, we still need to raise some error to abort interpretation.

Fair enough, and better to be safe than sorry.

@eddyb

add delay_span_bug to TransmuteSizeDiff, just to be sure See rust-lang#69839 (comment). r? @eddyb

@eddyb

add delay_span_bug to TransmuteSizeDiff, just to be sure See rust-lang#69839 (comment). r? @eddyb

@oli-obk

do not 'return' in 'throw_' macros In rust-lang#69839 we turned a closure into a `try` block, but it turns out that does not work with our `throw_` macros, which `return` so they skip the `try`. Here we fix that. For some reason that means we also have to remove some `;`. r? @oli-obk

rust-highfive assigned oli-obk Mar 8, 2020

RalfJung commented Mar 8, 2020

View reviewed changes

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

This comment has been minimized.

Sign in to view

RalfJung commented Mar 9, 2020

View reviewed changes

src/librustc/mir/interpret/mod.rs Show resolved Hide resolved

RalfJung commented Mar 9, 2020

View reviewed changes

src/librustc/mir/interpret/pointer.rs Show resolved Hide resolved

RalfJung changed the title ~~Miri error cleanup~~ Miri error reform Mar 9, 2020

RalfJung mentioned this pull request Mar 9, 2020

rustup for error reform rust-lang/miri#1219

Merged

RalfJung added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 9, 2020

RalfJung force-pushed the miri-error-cleanup branch from 1b89adc to 37b355f Compare March 9, 2020 15:58

oli-obk reviewed Mar 9, 2020

View reviewed changes

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

oli-obk reviewed Mar 9, 2020

View reviewed changes

src/test/ui/error-codes/E0396-fixed.stderr Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

RalfJung force-pushed the miri-error-cleanup branch from 593cde1 to c072193 Compare March 10, 2020 07:40

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 Mar 10, 2020

This comment has been minimized.

Sign in to view

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 11, 2020

RalfJung added 7 commits March 11, 2020 19:42

miri: categorize errors into "unsupported" and "UB"

f5efb68

Also slightly refactor pointer bounds checks to avoid creating unnecessary temporary Errors

fmt, tweak messages and bless

d02543a

generalize InvalidNullPointerUsage to InvalidIntPointerUsage

9a95b01

fmt, and fix rustfmt-induced rebase hickup

3ebcd78

start Miri messages lower-case

2764d3d

explain why we catch PointerUseAfterFree

3e61442

avoid boolean inversion

d8f8168

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 Mar 18, 2020

Centril mentioned this pull request Mar 18, 2020

Rollup of 10 pull requests #70109

Closed

Centril mentioned this pull request Mar 18, 2020

Rollup of 10 pull requests #70110

Closed

Centril mentioned this pull request Mar 18, 2020

Rollup of 10 pull requests #70112

Merged

bors merged commit a958314 into rust-lang:master Mar 19, 2020

bors added a commit to rust-lang/miri that referenced this pull request Mar 19, 2020

Auto merge of #1219 - RalfJung:error-cleanup, r=RalfJung

0ff05c4

rustup for error reform This is the Miri side of rust-lang/rust#69839.

eddyb reviewed Mar 19, 2020

View reviewed changes

This was referenced Mar 19, 2020

do not 'return' in 'throw_' macros #70138

Merged

add delay_span_bug to TransmuteSizeDiff, just to be sure #70139

Merged

SimonSapin mentioned this pull request Mar 20, 2020

Importing modules from inside include! no longer works #70185

Closed

RalfJung deleted the miri-error-cleanup branch March 20, 2020 16:12

Centril added a commit to Centril/rust that referenced this pull request Mar 21, 2020

Rollup merge of rust-lang#70139 - RalfJung:delay, r=eddyb

f468f9b

add delay_span_bug to TransmuteSizeDiff, just to be sure See rust-lang#69839 (comment). r? @eddyb

Centril added a commit to Centril/rust that referenced this pull request Mar 21, 2020

Rollup merge of rust-lang#70139 - RalfJung:delay, r=eddyb

c4a5cc1

add delay_span_bug to TransmuteSizeDiff, just to be sure See rust-lang#69839 (comment). r? @eddyb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Miri error reform #69839

Miri error reform #69839

RalfJung commented Mar 8, 2020 •

edited

Loading

This comment has been minimized.

oli-obk Mar 9, 2020

RalfJung Mar 9, 2020

oli-obk Mar 10, 2020

oli-obk Mar 9, 2020

This comment has been minimized.

oli-obk commented Mar 10, 2020

bors commented Mar 10, 2020

bors commented Mar 10, 2020

This comment has been minimized.

RalfJung commented Mar 18, 2020

bors commented Mar 18, 2020

Centril commented Mar 18, 2020

bors commented Mar 18, 2020

Centril commented Mar 18, 2020

eddyb Mar 19, 2020

RalfJung Mar 19, 2020

eddyb Mar 19, 2020 •

edited

Loading

RalfJung Mar 19, 2020

eddyb Mar 19, 2020

	// FIXME: This should be an assert instead of an error, but if we transmute within an
	// array length computation, `typeck` may not have yet been run and errored out. In fact
	// most likey we are running `typeck` right now. Investigate whether we can bail out
	// on `typeck_tables().has_errors` at all const eval entry points.
	debug!("Size mismatch when transmuting!\nsrc: {:#?}\ndest: {:#?}", src, dest);
	throw_inval!(TransmuteSizeDiff(src.layout.ty, dest.layout.ty));

Miri error reform #69839

Miri error reform #69839

Conversation

RalfJung commented Mar 8, 2020 • edited Loading

This comment has been minimized.

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

This comment has been minimized.

oli-obk commented Mar 10, 2020

bors commented Mar 10, 2020

bors commented Mar 10, 2020

This comment has been minimized.

RalfJung commented Mar 18, 2020

bors commented Mar 18, 2020

Centril commented Mar 18, 2020

bors commented Mar 18, 2020

Centril commented Mar 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb Mar 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Mar 8, 2020 •

edited

Loading

eddyb Mar 19, 2020 •

edited

Loading