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

catch_unwind panics due to failing debug assert when used during unwinding #68696

Closed
tmiasko opened this issue Jan 31, 2020 · 3 comments · Fixed by #68698
Closed

catch_unwind panics due to failing debug assert when used during unwinding #68696

tmiasko opened this issue Jan 31, 2020 · 3 comments · Fixed by #68698
Labels
A-runtime Area: std's runtime and "pre-main" init for handling backtraces, unwinds, stack overflows C-bug Category: This is a bug. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@tmiasko
Copy link
Contributor

tmiasko commented Jan 31, 2020

In particular catch_unwind panics because of failing assert in

debug_assert!(update_panic_count(0) == 0);

For example:

struct Catch;

impl Drop for Catch {
    fn drop(&mut self) {
        let _ = std::panic::catch_unwind(|| {});
    }
}

#[test]
#[should_panic]
fn it_works() {
    let catch = Catch;
    panic!("ups");
}
$ cargo test -Zbuild-std --target x86_64-unknown-linux-gnu
   Compiling panic v0.1.0 (/.../panic)
    Finished test [unoptimized + debuginfo] target(s) in 1.06s
     Running target/x86_64-unknown-linux-gnu/debug/deps/panic-118296df2d538b47

running 1 test
thread panicked while panicking. aborting.
error: test failed, to rerun pass '--lib'

Caused by:
  process didn't exit successfully: `/.../target/x86_64-unknown-linux-gnu/debug/deps/panic-118296df2d538b47` (signal: 4, SIGILL: illegal instruction)
@jonas-schievink jonas-schievink added A-runtime Area: std's runtime and "pre-main" init for handling backtraces, unwinds, stack overflows C-bug Category: This is a bug. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jan 31, 2020
@Mark-Simulacrum
Copy link
Member

Cc @Amanieu

I think that assert doesn't make much sense if I'm interpreting correctly, though I'm surprised it hasn't been hit before...

@tmiasko
Copy link
Contributor Author

tmiasko commented Jan 31, 2020

The current implementation seems to incorrectly assume that unwinding is not in progress when entering catch_unwind. Not caught before, since std is usually used without debug assertions (it definitely happens in actually existing code, what is unusual is that I am using -Zbuild-std and hence hitting the assertion).

There is a similar although a separate question if following should abort or not:

use std::panic::catch_unwind;

struct CatchPanic;

impl Drop for CatchPanic {
    fn drop(&mut self) {
        let _ = catch_unwind(|| {
            panic!("1");
        });
    }
}

fn main() {
    let _ = catch_unwind(|| {
        let _catch = CatchPanic;
        panic!("2");
    });
}

@Mark-Simulacrum
Copy link
Member

Yes, the second code snippet should definitely abort - we abort on the second live panic, independent of whether it's going to be caught.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-runtime Area: std's runtime and "pre-main" init for handling backtraces, unwinds, stack overflows C-bug Category: This is a bug. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants