Print a helpful message if unwinding aborts when it reaches a nounwind function by Amanieu · Pull Request #92828 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation41 Commits3 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

This is implemented by routing TerminatorKind::Abort back through the panic handler, but with a special flag in the PanicInfo which indicates that the panic handler should not attempt to unwind the stack and should instead abort immediately.

This is useful for the planned change in rust-lang/lang-team#97 which would make Drop impls nounwind by default.

Code

#![feature(c_unwind)]

fn panic() { panic!() }

extern "C" fn nounwind() { panic(); }

fn main() { nounwind(); }

Before

$ ./test
thread 'main' panicked at 'explicit panic', test.rs:4:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Illegal instruction (core dumped)

After

$ ./test
thread 'main' panicked at 'explicit panic', test.rs:4:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'main' panicked at 'panic in a function that cannot unwind', test.rs:7:1
stack backtrace:
   0:     0x556f8f86ec9b - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::hdccefe11a6ac4396
   1:     0x556f8f88ac6c - core::fmt::write::he152b28c41466ebb
   2:     0x556f8f85d6e2 - std::io::Write::write_fmt::h0c261480ab86f3d3
   3:     0x556f8f8654fa - std::panicking::default_hook::{{closure}}::h5d7346f3ff7f6c1b
   4:     0x556f8f86512b - std::panicking::default_hook::hd85803a1376cac7f
   5:     0x556f8f865a91 - std::panicking::rust_panic_with_hook::h4dc1c5a3036257ac
   6:     0x556f8f86f079 - std::panicking::begin_panic_handler::{{closure}}::hdda1d83c7a9d34d2
   7:     0x556f8f86edc4 - std::sys_common::backtrace::__rust_end_short_backtrace::h5b70ed0cce71e95f
   8:     0x556f8f865592 - rust_begin_unwind
   9:     0x556f8f85a764 - core::panicking::panic_no_unwind::h2606ab3d78c87899
  10:     0x556f8f85b910 - test::nounwind::hade6c7ee65050347
  11:     0x556f8f85b936 - test::main::hdc6e02cb36343525
  12:     0x556f8f85b7e3 - core::ops::function::FnOnce::call_once::h4d02663acfc7597f
  13:     0x556f8f85b739 - std::sys_common::backtrace::__rust_begin_short_backtrace::h071d40135adb0101
  14:     0x556f8f85c149 - std::rt::lang_start::{{closure}}::h70dbfbf38b685e93
  15:     0x556f8f85c791 - std::rt::lang_start_internal::h798f1c0268d525aa
  16:     0x556f8f85c131 - std::rt::lang_start::h476a7ee0a0bb663f
  17:     0x556f8f85b963 - main
  18:     0x7f64c0822b25 - __libc_start_main
  19:     0x556f8f85ae8e - _start
  20:                0x0 - <unknown>
thread panicked while panicking. aborting.
Aborted (core dumped)

rustbot added the T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

label

Jan 12, 2022

r? @dtolnay

(rust-highfive has picked a reviewer for you, use r? to override)

This comment has been minimized.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update cg_clif too?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cg_clif doesn't implement TerminatorKind::Abort (it's only used in landing pads).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While it does implement it, it seems it is indeed unreachable until unwinding is implemented. Could you add a fixme so I don't forget to implement it once I implement unwinding.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

        TerminatorKind::Resume | TerminatorKind::Abort => {
            trap_unreachable(fx, "[corruption] Unwinding bb reached.");
        }

I think it's already pretty clear that both are needed for unwinding support.

thread panicked while panicking. aborting.

Why does this happen? Would this now cause #[panic_handler]'s that use core::intrinsics::abort() to recurse infinitely?

A message is certainly helpful but I am not sure whether it should be form of calling the panic handler.

I also think core::intrinsics::abort() should really just abort instead of performing a nounwind panicking. E.g. Rc/Arc count overflow explicitly calls abort() to avoid a function call for code size reasons.

Perhaps we want to introduce a new Terminate terminator that models C++ std::terminate.

core::intrinsics::abort doesn't use TerminatorKind::Abort, it is lowered to the LLVM intrinsic directly. TerminatorKind::Abort is only used when unwinding out of a nounwind function, similar to C++'s std::terminate.

The reason I used the panic handler is so that no_std programs can handle these just like other panic.

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

⌛ Trying commit b4f96b2f29c9f54fbe712aeede521591707bc284 with merge 7d5aad079d9bc00cd2d025efb0338b5f1b8cff0e...

core::intrinsics::abort doesn't use TerminatorKind::Abort, it is lowered to the LLVM intrinsic directly. TerminatorKind::Abort is only used when unwinding out of a nounwind function, similar to C++'s std::terminate.

In that sense probably we want to change the name of the terminator to Terminate? Maybe we also want a toggle to select terminate behaviour (nounwind panic or abort) for size-concerning programs.

The reason I used the panic handler is so that no_std programs can handle these just like other panic.

Obviously not for no_std programs that have an unwinding panic handler, or a std program that uses a panic hook that triggers unwinding. Both requires nightly (c_unwind) at the moment though.

I am thinking whether we should actually have an enum for the unwind action, so instead of Option<BasicBlock>, we should have

enum UnwindAction { None, Terminate, Cleanup(BasicBlock), // LSDA can also encode Handler(catch) but we only use them for the r#try intrinsic so it's not needed here }

then for cg_gcc we can encode Terminate in LSDA (would require change in personality) and for cg_llvm we can generate call to PanicNoUnwind.

Obviously not for no_std programs that have an unwinding panic handler, or a std program that uses a panic hook that triggers unwinding. Both requires nightly (c_unwind) at the moment though.

This is why I added a can_unwind flag to PanicInfo which indicates whether the panic handler is allowed to unwind. The panic handler should check this flag and abort if it is set (after printing the error & backtrace to stderr).

This is why I added a can_unwind flag to PanicInfo which indicates whether the panic handler is allowed to unwind. The panic handler should check this flag and abort if it is set (after printing the error & backtrace to stderr).

I am aware. I mentioned that to point out that decision about this change needs to happen before c_unwind stablises.

☀️ Try build successful - checks-actions
Build commit: 7d5aad079d9bc00cd2d025efb0338b5f1b8cff0e (7d5aad079d9bc00cd2d025efb0338b5f1b8cff0e)

Finished benchmarking commit (7d5aad079d9bc00cd2d025efb0338b5f1b8cff0e): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me. Please r=me once PanicInfo::can_unwind has a tracking issue and the discussion above is wrapped up.

I am thinking whether we should actually have an enum for the unwind action, so instead of Option<BasicBlock>, we should have

enum UnwindAction { None, Terminate, Cleanup(BasicBlock), // LSDA can also encode Handler(catch) but we only use them for the r#try intrinsic so it's not needed here }

then for cg_gcc we can encode Terminate in LSDA (would require change in personality) and for cg_llvm we can generate call to PanicNoUnwind.

I think this is a good idea, but should be done in a separate PR since this is a pretty invasive change to MIR.

This comment has been minimized.

@bors retry network failure

Could not resolve host: ci-mirrors.rust-lang.org

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

Jan 21, 2022

⌛ Testing commit fe9dc6e with merge 55a060472e491d322e43b65b57db636845b4d026...

This comment has been minimized.

bors added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

and removed S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

labels

Jan 21, 2022

📌 Commit 24588e6 has been approved by dtolnay

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

Jan 21, 2022

There weren't any perf regressions and I don't think this needs anything special in regards to rollups.

@bors rollup=maybe

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Jan 22, 2022

Print a helpful message if unwinding aborts when it reaches a nounwind function

This is implemented by routing TerminatorKind::Abort back through the panic handler, but with a special flag in the PanicInfo which indicates that the panic handler should not attempt to unwind the stack and should instead abort immediately.

This is useful for the planned change in rust-lang/lang-team#97 which would make Drop impls nounwind by default.

Code

#![feature(c_unwind)]

fn panic() {
    panic!()
}

extern "C" fn nounwind() {
    panic();
}

fn main() {
    nounwind();
}

Before

$ ./test
thread 'main' panicked at 'explicit panic', test.rs:4:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Illegal instruction (core dumped)

After

$ ./test
thread 'main' panicked at 'explicit panic', test.rs:4:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'main' panicked at 'panic in a function that cannot unwind', test.rs:7:1
stack backtrace:
   0:     0x556f8f86ec9b - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::hdccefe11a6ac4396
   1:     0x556f8f88ac6c - core::fmt::write::he152b28c41466ebb
   2:     0x556f8f85d6e2 - std::io::Write::write_fmt::h0c261480ab86f3d3
   3:     0x556f8f8654fa - std::panicking::default_hook::{{closure}}::h5d7346f3ff7f6c1b
   4:     0x556f8f86512b - std::panicking::default_hook::hd85803a1376cac7f
   5:     0x556f8f865a91 - std::panicking::rust_panic_with_hook::h4dc1c5a3036257ac
   6:     0x556f8f86f079 - std::panicking::begin_panic_handler::{{closure}}::hdda1d83c7a9d34d2
   7:     0x556f8f86edc4 - std::sys_common::backtrace::__rust_end_short_backtrace::h5b70ed0cce71e95f
   8:     0x556f8f865592 - rust_begin_unwind
   9:     0x556f8f85a764 - core::panicking::panic_no_unwind::h2606ab3d78c87899
  10:     0x556f8f85b910 - test::nounwind::hade6c7ee65050347
  11:     0x556f8f85b936 - test::main::hdc6e02cb36343525
  12:     0x556f8f85b7e3 - core::ops::function::FnOnce::call_once::h4d02663acfc7597f
  13:     0x556f8f85b739 - std::sys_common::backtrace::__rust_begin_short_backtrace::h071d40135adb0101
  14:     0x556f8f85c149 - std::rt::lang_start::{{closure}}::h70dbfbf38b685e93
  15:     0x556f8f85c791 - std::rt::lang_start_internal::h798f1c0268d525aa
  16:     0x556f8f85c131 - std::rt::lang_start::h476a7ee0a0bb663f
  17:     0x556f8f85b963 - main
  18:     0x7f64c0822b25 - __libc_start_main
  19:     0x556f8f85ae8e - _start
  20:                0x0 - <unknown>
thread panicked while panicking. aborting.
Aborted (core dumped)

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Jan 22, 2022

Print a helpful message if unwinding aborts when it reaches a nounwind function

This is implemented by routing TerminatorKind::Abort back through the panic handler, but with a special flag in the PanicInfo which indicates that the panic handler should not attempt to unwind the stack and should instead abort immediately.

This is useful for the planned change in rust-lang/lang-team#97 which would make Drop impls nounwind by default.

Code

#![feature(c_unwind)]

fn panic() {
    panic!()
}

extern "C" fn nounwind() {
    panic();
}

fn main() {
    nounwind();
}

Before

$ ./test
thread 'main' panicked at 'explicit panic', test.rs:4:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Illegal instruction (core dumped)

After

$ ./test
thread 'main' panicked at 'explicit panic', test.rs:4:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'main' panicked at 'panic in a function that cannot unwind', test.rs:7:1
stack backtrace:
   0:     0x556f8f86ec9b - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::hdccefe11a6ac4396
   1:     0x556f8f88ac6c - core::fmt::write::he152b28c41466ebb
   2:     0x556f8f85d6e2 - std::io::Write::write_fmt::h0c261480ab86f3d3
   3:     0x556f8f8654fa - std::panicking::default_hook::{{closure}}::h5d7346f3ff7f6c1b
   4:     0x556f8f86512b - std::panicking::default_hook::hd85803a1376cac7f
   5:     0x556f8f865a91 - std::panicking::rust_panic_with_hook::h4dc1c5a3036257ac
   6:     0x556f8f86f079 - std::panicking::begin_panic_handler::{{closure}}::hdda1d83c7a9d34d2
   7:     0x556f8f86edc4 - std::sys_common::backtrace::__rust_end_short_backtrace::h5b70ed0cce71e95f
   8:     0x556f8f865592 - rust_begin_unwind
   9:     0x556f8f85a764 - core::panicking::panic_no_unwind::h2606ab3d78c87899
  10:     0x556f8f85b910 - test::nounwind::hade6c7ee65050347
  11:     0x556f8f85b936 - test::main::hdc6e02cb36343525
  12:     0x556f8f85b7e3 - core::ops::function::FnOnce::call_once::h4d02663acfc7597f
  13:     0x556f8f85b739 - std::sys_common::backtrace::__rust_begin_short_backtrace::h071d40135adb0101
  14:     0x556f8f85c149 - std::rt::lang_start::{{closure}}::h70dbfbf38b685e93
  15:     0x556f8f85c791 - std::rt::lang_start_internal::h798f1c0268d525aa
  16:     0x556f8f85c131 - std::rt::lang_start::h476a7ee0a0bb663f
  17:     0x556f8f85b963 - main
  18:     0x7f64c0822b25 - __libc_start_main
  19:     0x556f8f85ae8e - _start
  20:                0x0 - <unknown>
thread panicked while panicking. aborting.
Aborted (core dumped)

bors added a commit to rust-lang-ci/rust that referenced this pull request

Jan 22, 2022

…askrgr

Rollup of 9 pull requests

Successful merges:

• rust-lang#85967 (add support for the l4-bender linker on the x86_64-unknown-l4re-uclibc tier 3 target)
• rust-lang#92828 (Print a helpful message if unwinding aborts when it reaches a nounwind function)
• rust-lang#93012 (Update pulldown-cmark version to fix markdown list issue)
• rust-lang#93116 (Simplify use of map_or)
• rust-lang#93132 (Increase the format version of rustdoc-json-types)
• rust-lang#93147 (Interner cleanups)
• rust-lang#93153 (Reject unsupported naked functions)
• rust-lang#93170 (Add missing GUI test explanations)
• rust-lang#93172 (rustdoc: remove dashed underline under main heading)

Failed merges:

r? @ghost @rustbot modify labels: rollup

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Jan 25, 2022

make Windows abort_internal Miri-compatible

rust-lang#92828 started calling abort_internal on double-panics, uncovering that on Windows this function does not work in Miri because of its use of inline assembly.

Cc @Amanieu

CAD97 mentioned this pull request

Jul 13, 2022

Labels

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.