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

miri: ICE on invalid terminators #69830

Merged
merged 3 commits into from
Mar 12, 2020

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Mar 8, 2020

We've run a lot of MIR in Miri (including some generators) and never seen these.

@tmandry is it correct that Yield and GeneratorDrop get lowered away?

@eddyb @oli-obk what's with this Abort that does not seem to ever actually exist? Codegen does seem to handle it, so I wonder why Miri can get away without that. In fact, codegen handles it twice:

mir::TerminatorKind::Abort => {

Some(&mir::TerminatorKind::Abort) => {

@rust-highfive
Copy link
Collaborator

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 8, 2020
@jonas-schievink
Copy link
Contributor

is it correct that Yield and GeneratorDrop get lowered away?

They get lowered by the generator transform, so if you run before that you'll still encounter them. If you run after it they should all be gone though.

@RalfJung
Copy link
Member Author

RalfJung commented Mar 8, 2020

Yeah we definitely run after that.

@matthewjasper
Copy link
Contributor

Abort is used instead of Resume for functions annotated with #[unwind(aborts)] when compiling with -Cpanic=unwind.

@RalfJung
Copy link
Member Author

RalfJung commented Mar 8, 2020

@matthewjasper Ah! Here's a Miri reproducer then:

#![feature(unwind_attributes)]

#[unwind(aborts)]
fn panic_abort() { panic!() }

fn main() {
    panic_abort();
}

@RalfJung
Copy link
Member Author

RalfJung commented Mar 9, 2020

All right, I think this is ready now.
r? @oli-obk

@rust-highfive rust-highfive assigned oli-obk and unassigned eddyb Mar 9, 2020
@rust-highfive

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 9, 2020

Shouldn't the abort intrinsic also forward to this new machine hook?

You can probably run your miri repro under unleash, too.

@RalfJung
Copy link
Member Author

RalfJung commented Mar 9, 2020

Shouldn't the abort intrinsic also forward to this new machine hook?

So far the core Miri engine knows nothing about the abort intrinsic. You want to see it added to librustc_mir/interpret/intrinsics.rs?

You can probably run your miri repro under unleash, too.

Yeah but we don't actually support aborting in CTFE so I am not entirely sure about the point of that...^^

@RalfJung
Copy link
Member Author

RalfJung commented Mar 9, 2020

I added the abort intrinsic. At some point we should probably just move all the intrinsics from Miri to here... well, all except atomics and those requiring host floating point maybe.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 11, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Mar 11, 2020

📌 Commit 911c75f has been approved by oli-obk

@bors 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 11, 2020
Centril added a commit to Centril/rust that referenced this pull request Mar 11, 2020
…=oli-obk

miri: ICE on invalid terminators

We've run a lot of MIR in Miri (including some generators) and never seen these.

@tmandry is it correct that `Yield` and `GeneratorDrop` get lowered away?

@eddyb @oli-obk what's with this `Abort` that does not seem to ever actually exist? Codegen *does* seem to handle it, so I wonder why Miri can get away without that. In fact, codegen handles it twice:

https://github.com/rust-lang/rust/blob/1d5241c96208ca7d925442b1a5fa45ad18717a6f/src/librustc_codegen_ssa/mir/block.rs#L796

https://github.com/rust-lang/rust/blob/1d5241c96208ca7d925442b1a5fa45ad18717a6f/src/librustc_codegen_ssa/mir/mod.rs#L296
Centril added a commit to Centril/rust that referenced this pull request Mar 11, 2020
…=oli-obk

miri: ICE on invalid terminators

We've run a lot of MIR in Miri (including some generators) and never seen these.

@tmandry is it correct that `Yield` and `GeneratorDrop` get lowered away?

@eddyb @oli-obk what's with this `Abort` that does not seem to ever actually exist? Codegen *does* seem to handle it, so I wonder why Miri can get away without that. In fact, codegen handles it twice:

https://github.com/rust-lang/rust/blob/1d5241c96208ca7d925442b1a5fa45ad18717a6f/src/librustc_codegen_ssa/mir/block.rs#L796

https://github.com/rust-lang/rust/blob/1d5241c96208ca7d925442b1a5fa45ad18717a6f/src/librustc_codegen_ssa/mir/mod.rs#L296
bors added a commit that referenced this pull request Mar 12, 2020
Rollup of 10 pull requests

Successful merges:

 - #68899 (Add Display and Error impls for proc_macro::LexError)
 - #69011 (Document unsafe blocks in core::fmt)
 - #69674 (Rename DefKind::Method and TraitItemKind::Method )
 - #69705 (Toolstate: remove redundant beta-week check.)
 - #69722 (Tweak output for invalid negative impl AST errors)
 - #69747 (Rename rustc guide)
 - #69792 (Implement Error for TryReserveError)
 - #69830 (miri: ICE on invalid terminators)
 - #69921 (rustdoc: remove unused import)
 - #69945 (update outdated comment)

Failed merges:

r? @ghost
@bors bors merged commit fac7122 into rust-lang:master Mar 12, 2020
@RalfJung RalfJung deleted the miri-invalid-terminator branch March 12, 2020 19:49
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Mar 13, 2020
miri engine: fix treatment of abort intrinsic

I screwed up in rust-lang#69830 and added `abort` to the wrong block of intrinsics, namely the one that actually has a return place. So that branch was never actually reached.

r? @oli-obk
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants