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

Support unwinding after a panic #693

Merged
merged 10 commits into from Nov 19, 2019
Merged

Conversation

@Aaron1011
Copy link
Contributor

Aaron1011 commented Apr 17, 2019

Fixes #658

This commit adds support for unwinding after a panic. It requires a
companion rustc PR to be merged, in order for the necessary hooks to
work properly.

Currently implemented:

  • Selecting between unwind/abort mode based on the rustc Session
  • Properly popping off stack frames, unwinding back the caller
  • Running 'unwind' blocks in Mir terminators

Not yet implemented:

  • 'Abort' terminators

This PR was getting fairly large, so I decided to open it for review without
implementing 'Abort' terminator support. This could either be added on
to this PR, or merged separately.

I've a test to exercise several different aspects of unwind panicking. Ideally, we would run Miri against the libstd panic tests, but I haven't yet figured out how to do that.

This depends on rust-lang/rust#60026

src/fn_call.rs Outdated Show resolved Hide resolved
src/fn_call.rs Outdated Show resolved Hide resolved
src/fn_call.rs Outdated Show resolved Hide resolved
src/fn_call.rs Outdated Show resolved Hide resolved
src/fn_call.rs Outdated Show resolved Hide resolved
src/fn_call.rs Outdated Show resolved Hide resolved
@oli-obk

This comment has been minimized.

Copy link
Collaborator

oli-obk commented Apr 17, 2019

Please also add a test that check whether double panics cause an abort and don't confuse miri. Something like

struct Foo;
impl Drop for Foo {
    fn drop(&mut self) {
        panic!("second");
    }
}
fn main() {
    let foo = Foo;
    panic!("first");
}
@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

Aaron1011 commented Apr 17, 2019

@oli-obk: Double-panics are handled correctly, but end up causing an error due to uw::_Unwind_Backtrace being unhandled. This is due to the fact that when a double panic occurs, libstd unconditionally tries to print a backtrace.

Should we look into building libstd without the backtrace feature, or should we try to implement it (e.g adding a cfg(mir) into libstd's backtrace code?

@oli-obk

This comment has been minimized.

Copy link
Collaborator

oli-obk commented Apr 18, 2019

Should we look into building libstd without the backtrace feature

That seems doable since we're building a libstd anyway. But you don't have to do that in this PR, adding the test so we see the failure is enough for me right now.

src/fn_call.rs Outdated Show resolved Hide resolved
src/fn_call.rs Outdated Show resolved Hide resolved
src/fn_call.rs Outdated Show resolved Hide resolved
src/fn_call.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
tests/run-pass/catch_panic.rs Outdated Show resolved Hide resolved
@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Apr 29, 2019

Running 'unwind' blocks in Mir terminators

Where is that happening? The cleanup field of the Call terminator still seems to be ignored, and that's where things should have to jump to during unwinding.

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

Aaron1011 commented Apr 29, 2019

@RalfJung: 'unwind' processing is handled here

@Aaron1011 Aaron1011 force-pushed the Aaron1011:feature/panic_unwind_final branch from 1d26324 to 4893036 Apr 30, 2019
src/fn_call.rs Outdated Show resolved Hide resolved
@Aaron1011 Aaron1011 force-pushed the Aaron1011:feature/panic_unwind_final branch 2 times, most recently from 859366f to 6d195bc May 29, 2019
@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

Aaron1011 commented May 30, 2019

@RalfJung: I've updated this PR (and the corresponding rustc PR) to move more of the unwinding logic into rustc. Now, the implementation doesn't manually step through box_me_up - it's handled via the normal main loop.

@Aaron1011 Aaron1011 force-pushed the Aaron1011:feature/panic_unwind_final branch from 634bf34 to b909669 May 30, 2019
src/fn_call.rs Outdated Show resolved Hide resolved
src/fn_call.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/fn_call.rs Outdated Show resolved Hide resolved
src/fn_call.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
tests/run-pass/catch_panic.rs Outdated Show resolved Hide resolved
@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

Aaron1011 commented Nov 17, 2019

This is going to require the ability to read wide strings, which appears to be currently being worked on in rust-lang/rust#66470.

@RalfJung: Is there any chance that we could just disable unwinding tests on Windows for now? Getting this working is going to require rust-lang/rust#66470 to be merged, after which we will need to implement GetModuleHandleW and GetProcAddress in a manner analogous to the current dlsym handling.

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Nov 17, 2019

Is there any chance that we could just disable unwinding tests on Windows for now?

Seems fine to me, if you open an issue.
I also seem to recall that the reason RwLock uses GetProcAddress is to support Windows XP or so, but support for that was since dropped from Rust... so I think (but you might want to confirm that with some Rust team members) that we could patch RwLock on Windows to statically link against the platform methods, and not require GetProcAddress any more.

Miri currently does not support `GetProcAddress`
and `GetModuleHandleW`, both of which end up getting invoked by the
libstd panic hook.
@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

Aaron1011 commented Nov 17, 2019

@RalfJung; I've opened #1059

src/helpers.rs Show resolved Hide resolved
src/shims/foreign_items.rs Outdated Show resolved Hide resolved
@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Nov 19, 2019

@Aaron1011 I pushed a commit with a bunch of comments, but also some simplifications in panic.rs. Could you take a look whether the comments all still seem correct to you?

Then this is ready to land IMO, modulo my question about __rust_start_panic above.

@oli-obk any comments/concerns?

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

Aaron1011 commented Nov 19, 2019

@RalfJung: This should now be ready to merge.

src/shims/foreign_items.rs Outdated Show resolved Hide resolved
src/shims/panic.rs Outdated Show resolved Hide resolved
Aaron1011 added 2 commits Nov 19, 2019
@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

Aaron1011 commented Nov 19, 2019

@RalfJung: I've fixed the two comments you mentioned.

@oli-obk

This comment has been minimized.

Copy link
Collaborator

oli-obk commented Nov 19, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 19, 2019

📌 Commit 2750f60 has been approved by oli-obk

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 19, 2019

⌛️ Testing commit 2750f60 with merge e64caeb...

bors added a commit that referenced this pull request Nov 19, 2019
Support unwinding after a panic

Fixes #658

This commit adds support for unwinding after a panic. It requires a
companion rustc PR to be merged, in order for the necessary hooks to
work properly.

Currently implemented:
* Selecting between unwind/abort mode based on the rustc Session
* Properly popping off stack frames, unwinding back the caller
* Running 'unwind' blocks in Mir terminators

Not yet implemented:
* 'Abort' terminators

This PR was getting fairly large, so I decided to open it for review without
implementing 'Abort' terminator support. This could either be added on
to this PR, or merged separately.

I've a test to exercise several different aspects of unwind panicking. Ideally, we would run Miri against the libstd panic tests, but I haven't yet figured out how to do that.

This depends on rust-lang/rust#60026
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 19, 2019

💔 Test failed - status-appveyor

We should re-enable this once
#1059 is fixed
@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

Aaron1011 commented Nov 19, 2019

@oli-obk: I had forgotten to disable one of the panic tests on Windows. Can you re-try the build?

@oli-obk

This comment has been minimized.

Copy link
Collaborator

oli-obk commented Nov 19, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 19, 2019

📌 Commit 6fe89e4 has been approved by oli-obk

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 19, 2019

⌛️ Testing commit 6fe89e4 with merge e588d95...

bors added a commit that referenced this pull request Nov 19, 2019
Support unwinding after a panic

Fixes #658

This commit adds support for unwinding after a panic. It requires a
companion rustc PR to be merged, in order for the necessary hooks to
work properly.

Currently implemented:
* Selecting between unwind/abort mode based on the rustc Session
* Properly popping off stack frames, unwinding back the caller
* Running 'unwind' blocks in Mir terminators

Not yet implemented:
* 'Abort' terminators

This PR was getting fairly large, so I decided to open it for review without
implementing 'Abort' terminator support. This could either be added on
to this PR, or merged separately.

I've a test to exercise several different aspects of unwind panicking. Ideally, we would run Miri against the libstd panic tests, but I haven't yet figured out how to do that.

This depends on rust-lang/rust#60026
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 19, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: oli-obk
Pushing e588d95 to master...

@bors bors merged commit 6fe89e4 into rust-lang:master Nov 19, 2019
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
homu Test successful
Details
@Aaron1011 Aaron1011 deleted the Aaron1011:feature/panic_unwind_final branch Nov 19, 2019
@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Nov 19, 2019

Finally, after around half a year, this landed. Thanks @Aaron1011, this is awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.