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

Amanieu · 2022-01-12T22:27:35Z

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)

rust-highfive · 2022-01-12T22:27:38Z

r? @dtolnay

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

library/std/src/panicking.rs

bjorn3 · 2022-01-13T07:45:41Z

compiler/rustc_codegen_ssa/src/mir/block.rs

-                // `abort` does not terminate the block, so we still need to generate
-                // an `unreachable` terminator after it.
-                bx.unreachable();
+                self.codegen_abort_terminator(helper, bx, terminator);


Can you update cg_clif too?

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

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.

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.

bjorn3 · 2022-01-13T07:46:59Z

thread panicked while panicking. aborting.

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

Amanieu · 2022-01-13T07:54:06Z

thread panicked while panicking. aborting.

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

It's from here: https://github.com/rust-lang/rust/pull/92828/files#diff-88e2a536317b831c2e958b9205fde12f5edaabefba963bdd3a7503bbdedf8da9R626

Explicit #[panic_handler] in no_std crates are unaffected unless they try to unwind the stack (they need to check PanicInfo::can_unwind first).

nbdd0121 · 2022-01-15T16:40:19Z

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.

Amanieu · 2022-01-15T17:07:59Z

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.

Amanieu · 2022-01-15T17:40:39Z

@bors try @rust-timer queue

rust-timer · 2022-01-15T17:40:40Z

Awaiting bors try build completion.

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

bors · 2022-01-15T17:40:48Z

⌛ Trying commit b4f96b2f29c9f54fbe712aeede521591707bc284 with merge 7d5aad079d9bc00cd2d025efb0338b5f1b8cff0e...

nbdd0121 · 2022-01-15T17:57:58Z

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.

nbdd0121 · 2022-01-15T18:02:02Z

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.

Amanieu · 2022-01-15T18:04:13Z

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).

nbdd0121 · 2022-01-15T18:06:28Z

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.

bors · 2022-01-15T19:00:10Z

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

rust-timer · 2022-01-15T19:00:11Z

Queued 7d5aad079d9bc00cd2d025efb0338b5f1b8cff0e with parent b13a5bf, future comparison URL.

rust-timer · 2022-01-15T22:24:06Z

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

dtolnay

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

Amanieu · 2022-01-17T00:29:22Z

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.

lqd · 2022-01-21T11:01:21Z

@bors retry network failure

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

bors · 2022-01-21T13:27:14Z

⌛ Testing commit fe9dc6e with merge 55a060472e491d322e43b65b57db636845b4d026...

bors · 2022-01-21T14:33:11Z

💔 Test failed - checks-actions

Amanieu · 2022-01-21T15:45:40Z

@bors r=dtolnay

bors · 2022-01-21T15:45:41Z

📌 Commit 24588e6 has been approved by dtolnay

jackh726 · 2022-01-21T23:47:56Z

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

@bors rollup=maybe

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 ```rust #![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) ```

…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

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`

rust-highfive assigned dtolnay Jan 12, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 12, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 12, 2022

This comment has been minimized.

Sign in to view

Amanieu force-pushed the unwind-abort branch from 2cfac18 to 54fbca6 Compare January 13, 2022 04:18

Amanieu mentioned this pull request Jan 13, 2022

Never allow unwinding from Drop impls rust-lang/lang-team#97

Closed

bjorn3 reviewed Jan 13, 2022

View reviewed changes

library/std/src/panicking.rs Outdated Show resolved Hide resolved

bjorn3 reviewed Jan 13, 2022

View reviewed changes

Amanieu force-pushed the unwind-abort branch from 54fbca6 to b4f96b2 Compare January 13, 2022 16:12

Amanieu mentioned this pull request Jan 15, 2022

Guard against unwinding in cleanup code #92911

Merged

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 15, 2022

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 15, 2022

dtolnay approved these changes Jan 16, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

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

This comment has been minimized.

Sign in to view

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

Old versions of Android generate SIGSEGV from libc::abort

24588e6

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

matthiaskrgr mentioned this pull request Jan 22, 2022

Rollup of 11 pull requests #93189

Closed

matthiaskrgr mentioned this pull request Jan 22, 2022

Rollup of 10 pull requests #93191

Closed

matthiaskrgr mentioned this pull request Jan 22, 2022

Rollup of 9 pull requests #93202

Merged

bors merged commit 9d7c8ed into rust-lang:master Jan 22, 2022

rustbot added this to the 1.60.0 milestone Jan 22, 2022

RalfJung mentioned this pull request Jan 25, 2022

make Windows abort_internal Miri-compatible #93300

Merged

CAD97 mentioned this pull request Jul 13, 2022

Disable unwinding for catch_unwind error payloads. #99032

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Amanieu commented Jan 12, 2022

rust-highfive commented Jan 12, 2022

This comment has been minimized.

bjorn3 Jan 13, 2022

Amanieu Jan 13, 2022

bjorn3 Jan 13, 2022

Amanieu Jan 13, 2022

bjorn3 commented Jan 13, 2022 •

edited

Loading

Amanieu commented Jan 13, 2022

nbdd0121 commented Jan 15, 2022

Amanieu commented Jan 15, 2022

Amanieu commented Jan 15, 2022

rust-timer commented Jan 15, 2022

bors commented Jan 15, 2022

nbdd0121 commented Jan 15, 2022

nbdd0121 commented Jan 15, 2022

Amanieu commented Jan 15, 2022

nbdd0121 commented Jan 15, 2022

bors commented Jan 15, 2022

rust-timer commented Jan 15, 2022

rust-timer commented Jan 15, 2022

dtolnay left a comment

Amanieu commented Jan 17, 2022

This comment has been minimized.

lqd commented Jan 21, 2022

bors commented Jan 21, 2022

This comment has been minimized.

bors commented Jan 21, 2022

Amanieu commented Jan 21, 2022

bors commented Jan 21, 2022

jackh726 commented Jan 21, 2022

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

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

Conversation

Amanieu commented Jan 12, 2022

Code

Before

After

rust-highfive commented Jan 12, 2022

This comment has been minimized.

bjorn3 Jan 13, 2022

Choose a reason for hiding this comment

Amanieu Jan 13, 2022

Choose a reason for hiding this comment

bjorn3 Jan 13, 2022

Choose a reason for hiding this comment

Amanieu Jan 13, 2022

Choose a reason for hiding this comment

bjorn3 commented Jan 13, 2022 • edited Loading

Amanieu commented Jan 13, 2022

nbdd0121 commented Jan 15, 2022

Amanieu commented Jan 15, 2022

Amanieu commented Jan 15, 2022

rust-timer commented Jan 15, 2022

bors commented Jan 15, 2022

nbdd0121 commented Jan 15, 2022

nbdd0121 commented Jan 15, 2022

Amanieu commented Jan 15, 2022

nbdd0121 commented Jan 15, 2022

bors commented Jan 15, 2022

rust-timer commented Jan 15, 2022

rust-timer commented Jan 15, 2022

dtolnay left a comment

Choose a reason for hiding this comment

Amanieu commented Jan 17, 2022

This comment has been minimized.

lqd commented Jan 21, 2022

bors commented Jan 21, 2022

This comment has been minimized.

bors commented Jan 21, 2022

Amanieu commented Jan 21, 2022

bors commented Jan 21, 2022

jackh726 commented Jan 21, 2022

bjorn3 commented Jan 13, 2022 •

edited

Loading