Skip to content

Commit

Permalink
Rollup merge of rust-lang#110975 - Amanieu:panic_count, r=joshtriplett
Browse files Browse the repository at this point in the history
Rework handling of recursive panics

This PR makes 2 changes to how recursive panics works (a panic while handling a panic).

1. The panic count is no longer used to determine whether to force an immediate abort. This allows code like the following to work without aborting the process immediately:

```rust
struct Double;

impl Drop for Double {
    fn drop(&mut self) {
        // 2 panics are active at once, but this is fine since it is caught.
        std::panic::catch_unwind(|| panic!("twice"));
    }
}

let _d = Double;

panic!("once");
```

Rustc already generates appropriate code so that any exceptions escaping out of a `Drop` called in the unwind path will immediately abort the process.

2. Any panics while the panic hook is executing will force an immediate abort. This is necessary to avoid potential deadlocks like rust-lang#110771 where a panic happens while holding the backtrace lock. We don't even try to print the panic message in this case since the panic may have been caused by `Display` impls.

Fixes rust-lang#110771
  • Loading branch information
Noratrieb authored May 16, 2023
2 parents 76e79ca + 9c58f91 commit 4197be9
Show file tree
Hide file tree
Showing 9 changed files with 148 additions and 86 deletions.
120 changes: 67 additions & 53 deletions library/std/src/panicking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -298,8 +298,18 @@ pub mod panic_count {

pub const ALWAYS_ABORT_FLAG: usize = 1 << (usize::BITS - 1);

// Panic count for the current thread.
thread_local! { static LOCAL_PANIC_COUNT: Cell<usize> = const { Cell::new(0) } }
/// A reason for forcing an immediate abort on panic.
#[derive(Debug)]
pub enum MustAbort {
AlwaysAbort,
PanicInHook,
}

// Panic count for the current thread and whether a panic hook is currently
// being executed..
thread_local! {
static LOCAL_PANIC_COUNT: Cell<(usize, bool)> = const { Cell::new((0, false)) }
}

// Sum of panic counts from all threads. The purpose of this is to have
// a fast path in `count_is_zero` (which is used by `panicking`). In any particular
Expand Down Expand Up @@ -328,34 +338,39 @@ pub mod panic_count {
// panicking thread consumes at least 2 bytes of address space.
static GLOBAL_PANIC_COUNT: AtomicUsize = AtomicUsize::new(0);

// Return the state of the ALWAYS_ABORT_FLAG and number of panics.
// Increases the global and local panic count, and returns whether an
// immediate abort is required.
//
// If ALWAYS_ABORT_FLAG is not set, the number is determined on a per-thread
// base (stored in LOCAL_PANIC_COUNT), i.e. it is the amount of recursive calls
// of the calling thread.
// If ALWAYS_ABORT_FLAG is set, the number equals the *global* number of panic
// calls. See above why LOCAL_PANIC_COUNT is not used.
pub fn increase() -> (bool, usize) {
// This also updates thread-local state to keep track of whether a panic
// hook is currently executing.
pub fn increase(run_panic_hook: bool) -> Option<MustAbort> {
let global_count = GLOBAL_PANIC_COUNT.fetch_add(1, Ordering::Relaxed);
let must_abort = global_count & ALWAYS_ABORT_FLAG != 0;
let panics = if must_abort {
global_count & !ALWAYS_ABORT_FLAG
} else {
LOCAL_PANIC_COUNT.with(|c| {
let next = c.get() + 1;
c.set(next);
next
})
};
(must_abort, panics)
if global_count & ALWAYS_ABORT_FLAG != 0 {
return Some(MustAbort::AlwaysAbort);
}

LOCAL_PANIC_COUNT.with(|c| {
let (count, in_panic_hook) = c.get();
if in_panic_hook {
return Some(MustAbort::PanicInHook);
}
c.set((count + 1, run_panic_hook));
None
})
}

pub fn finished_panic_hook() {
LOCAL_PANIC_COUNT.with(|c| {
let (count, _) = c.get();
c.set((count, false));
});
}

pub fn decrease() {
GLOBAL_PANIC_COUNT.fetch_sub(1, Ordering::Relaxed);
LOCAL_PANIC_COUNT.with(|c| {
let next = c.get() - 1;
c.set(next);
next
let (count, _) = c.get();
c.set((count - 1, false));
});
}

Expand All @@ -366,7 +381,7 @@ pub mod panic_count {
// Disregards ALWAYS_ABORT_FLAG
#[must_use]
pub fn get_count() -> usize {
LOCAL_PANIC_COUNT.with(|c| c.get())
LOCAL_PANIC_COUNT.with(|c| c.get().0)
}

// Disregards ALWAYS_ABORT_FLAG
Expand Down Expand Up @@ -394,7 +409,7 @@ pub mod panic_count {
#[inline(never)]
#[cold]
fn is_zero_slow_path() -> bool {
LOCAL_PANIC_COUNT.with(|c| c.get() == 0)
LOCAL_PANIC_COUNT.with(|c| c.get().0 == 0)
}
}

Expand Down Expand Up @@ -655,23 +670,22 @@ fn rust_panic_with_hook(
location: &Location<'_>,
can_unwind: bool,
) -> ! {
let (must_abort, panics) = panic_count::increase();

// If this is the third nested call (e.g., panics == 2, this is 0-indexed),
// the panic hook probably triggered the last panic, otherwise the
// double-panic check would have aborted the process. In this case abort the
// process real quickly as we don't want to try calling it again as it'll
// probably just panic again.
if must_abort || panics > 2 {
if panics > 2 {
// Don't try to print the message in this case
// - perhaps that is causing the recursive panics.
rtprintpanic!("thread panicked while processing panic. aborting.\n");
} else {
// Unfortunately, this does not print a backtrace, because creating
// a `Backtrace` will allocate, which we must to avoid here.
let panicinfo = PanicInfo::internal_constructor(message, location, can_unwind);
rtprintpanic!("{panicinfo}\npanicked after panic::always_abort(), aborting.\n");
let must_abort = panic_count::increase(true);

// Check if we need to abort immediately.
if let Some(must_abort) = must_abort {
match must_abort {
panic_count::MustAbort::PanicInHook => {
// Don't try to print the message in this case
// - perhaps that is causing the recursive panics.
rtprintpanic!("thread panicked while processing panic. aborting.\n");
}
panic_count::MustAbort::AlwaysAbort => {
// Unfortunately, this does not print a backtrace, because creating
// a `Backtrace` will allocate, which we must to avoid here.
let panicinfo = PanicInfo::internal_constructor(message, location, can_unwind);
rtprintpanic!("{panicinfo}\npanicked after panic::always_abort(), aborting.\n");
}
}
crate::sys::abort_internal();
}
Expand All @@ -697,16 +711,16 @@ fn rust_panic_with_hook(
};
drop(hook);

if panics > 1 || !can_unwind {
// If a thread panics while it's already unwinding then we
// have limited options. Currently our preference is to
// just abort. In the future we may consider resuming
// unwinding or otherwise exiting the thread cleanly.
if !can_unwind {
rtprintpanic!("thread caused non-unwinding panic. aborting.\n");
} else {
rtprintpanic!("thread panicked while panicking. aborting.\n");
}
// Indicate that we have finished executing the panic hook. After this point
// it is fine if there is a panic while executing destructors, as long as it
// it contained within a `catch_unwind`.
panic_count::finished_panic_hook();

if !can_unwind {
// If a thread panics while running destructors or tries to unwind
// through a nounwind function (e.g. extern "C") then we cannot continue
// unwinding and have to abort immediately.
rtprintpanic!("thread caused non-unwinding panic. aborting.\n");
crate::sys::abort_internal();
}

Expand All @@ -716,7 +730,7 @@ fn rust_panic_with_hook(
/// This is the entry point for `resume_unwind`.
/// It just forwards the payload to the panic runtime.
pub fn rust_panic_without_hook(payload: Box<dyn Any + Send>) -> ! {
panic_count::increase();
panic_count::increase(false);

struct RewrapBox(Box<dyn Any + Send>);

Expand Down
19 changes: 13 additions & 6 deletions src/tools/miri/src/concurrency/thread.rs
Original file line number Diff line number Diff line change
Expand Up @@ -132,10 +132,15 @@ pub struct Thread<'mir, 'tcx> {
/// The join status.
join_status: ThreadJoinStatus,

/// The temporary used for storing the argument of
/// the call to `miri_start_panic` (the panic payload) when unwinding.
/// Stack of active panic payloads for the current thread. Used for storing
/// the argument of the call to `miri_start_panic` (the panic payload) when unwinding.
/// This is pointer-sized, and matches the `Payload` type in `src/libpanic_unwind/miri.rs`.
pub(crate) panic_payload: Option<Scalar<Provenance>>,
///
/// In real unwinding, the payload gets passed as an argument to the landing pad,
/// which then forwards it to 'Resume'. However this argument is implicit in MIR,
/// so we have to store it out-of-band. When there are multiple active unwinds,
/// the innermost one is always caught first, so we can store them as a stack.
pub(crate) panic_payloads: Vec<Scalar<Provenance>>,

/// Last OS error location in memory. It is a 32-bit integer.
pub(crate) last_error: Option<MPlaceTy<'tcx, Provenance>>,
Expand Down Expand Up @@ -205,7 +210,7 @@ impl<'mir, 'tcx> Thread<'mir, 'tcx> {
stack: Vec::new(),
top_user_relevant_frame: None,
join_status: ThreadJoinStatus::Joinable,
panic_payload: None,
panic_payloads: Vec::new(),
last_error: None,
on_stack_empty,
}
Expand All @@ -215,7 +220,7 @@ impl<'mir, 'tcx> Thread<'mir, 'tcx> {
impl VisitTags for Thread<'_, '_> {
fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) {
let Thread {
panic_payload,
panic_payloads: panic_payload,
last_error,
stack,
top_user_relevant_frame: _,
Expand All @@ -225,7 +230,9 @@ impl VisitTags for Thread<'_, '_> {
on_stack_empty: _, // we assume the closure captures no GC-relevant state
} = self;

panic_payload.visit_tags(visit);
for payload in panic_payload {
payload.visit_tags(visit);
}
last_error.visit_tags(visit);
for frame in stack {
frame.visit_tags(visit)
Expand Down
5 changes: 2 additions & 3 deletions src/tools/miri/src/shims/panic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
let [payload] = this.check_shim(abi, Abi::Rust, link_name, args)?;
let payload = this.read_scalar(payload)?;
let thread = this.active_thread_mut();
assert!(thread.panic_payload.is_none(), "the panic runtime should avoid double-panics");
thread.panic_payload = Some(payload);
thread.panic_payloads.push(payload);

// Jump to the unwind block to begin unwinding.
this.unwind_to_block(unwind)?;
Expand Down Expand Up @@ -146,7 +145,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {

// The Thread's `panic_payload` holds what was passed to `miri_start_panic`.
// This is exactly the second argument we need to pass to `catch_fn`.
let payload = this.active_thread_mut().panic_payload.take().unwrap();
let payload = this.active_thread_mut().panic_payloads.pop().unwrap();

// Push the `catch_fn` stackframe.
let f_instance = this.get_ptr_fn(catch_unwind.catch_fn)?.as_instance()?;
Expand Down
3 changes: 1 addition & 2 deletions src/tools/miri/tests/fail/panic/double_panic.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
//@error-in-other-file: the program aborted
//@normalize-stderr-test: "\| +\^+" -> "| ^"
//@normalize-stderr-test: "unsafe \{ libc::abort\(\) \}|crate::intrinsics::abort\(\);" -> "ABORT();"
//@normalize-stderr-test: "\n +[0-9]+:[^\n]+" -> "$1"
//@normalize-stderr-test: "\n at [^\n]+" -> "$1"

Expand All @@ -11,6 +9,7 @@ impl Drop for Foo {
}
}
fn main() {
//~^ERROR: panic in a function that cannot unwind
let _foo = Foo;
panic!("first");
}
29 changes: 8 additions & 21 deletions src/tools/miri/tests/fail/panic/double_panic.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -2,30 +2,17 @@ thread 'main' panicked at 'first', $DIR/double_panic.rs:LL:CC
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'main' panicked at 'second', $DIR/double_panic.rs:LL:CC
stack backtrace:
thread panicked while panicking. aborting.
error: abnormal termination: the program aborted execution
--> RUSTLIB/std/src/sys/PLATFORM/mod.rs:LL:CC
|
LL | ABORT();
| ^ the program aborted execution
|
= note: inside `std::sys::PLATFORM::abort_internal` at RUSTLIB/std/src/sys/PLATFORM/mod.rs:LL:CC
= note: inside `std::panicking::rust_panic_with_hook` at RUSTLIB/std/src/panicking.rs:LL:CC
= note: inside closure at RUSTLIB/std/src/panicking.rs:LL:CC
= note: inside `std::sys_common::backtrace::__rust_end_short_backtrace::<[closure@std::panicking::begin_panic_handler::{closure#0}], !>` at RUSTLIB/std/src/sys_common/backtrace.rs:LL:CC
= note: inside `std::panicking::begin_panic_handler` at RUSTLIB/std/src/panicking.rs:LL:CC
note: inside `<Foo as std::ops::Drop>::drop`
error: abnormal termination: panic in a function that cannot unwind
--> $DIR/double_panic.rs:LL:CC
|
LL | panic!("second");
| ^
= note: inside `std::ptr::drop_in_place::<Foo> - shim(Some(Foo))` at RUSTLIB/core/src/ptr/mod.rs:LL:CC
note: inside `main`
--> $DIR/double_panic.rs:LL:CC
LL | / fn main() {
LL | |
LL | | let _foo = Foo;
LL | | panic!("first");
LL | | }
| |_^ panic in a function that cannot unwind
|
LL | }
| ^
= note: this error originates in the macro `$crate::panic::panic_2021` which comes from the expansion of the macro `panic` (in Nightly builds, run with -Z macro-backtrace for more info)
= note: inside `main` at $DIR/double_panic.rs:LL:CC

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

Expand Down
25 changes: 25 additions & 0 deletions src/tools/miri/tests/pass/panic/nested_panic_caught.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
//@normalize-stderr-test: "\| +\^+" -> "| ^"
//@normalize-stderr-test: "\n +[0-9]+:[^\n]+" -> "$1"
//@normalize-stderr-test: "\n at [^\n]+" -> "$1"

// Checks that nested panics work correctly.

use std::panic::catch_unwind;

fn double() {
struct Double;

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

let _d = Double;

panic!("once");
}

fn main() {
assert!(catch_unwind(|| double()).is_err());
}
4 changes: 4 additions & 0 deletions src/tools/miri/tests/pass/panic/nested_panic_caught.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
thread 'main' panicked at 'once', $DIR/nested_panic_caught.rs:LL:CC
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'main' panicked at 'twice', $DIR/nested_panic_caught.rs:LL:CC
stack backtrace:
6 changes: 5 additions & 1 deletion tests/ui/backtrace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -104,13 +104,17 @@ fn runtest(me: &str) {
"bad output3: {}", s);

// Make sure a stack trace isn't printed too many times
//
// Currently it is printed 3 times ("once", "twice" and "panic in a
// function that cannot unwind") but in the future the last one may be
// removed.
let p = template(me).arg("double-fail")
.env("RUST_BACKTRACE", "1").spawn().unwrap();
let out = p.wait_with_output().unwrap();
assert!(!out.status.success());
let s = str::from_utf8(&out.stderr).unwrap();
let mut i = 0;
for _ in 0..2 {
for _ in 0..3 {
i += s[i + 10..].find("stack backtrace").unwrap() + 10;
}
assert!(s[i + 10..].find("stack backtrace").is_none(),
Expand Down
23 changes: 23 additions & 0 deletions tests/ui/panics/nested_panic_caught.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
// run-pass

// Checks that nested panics work correctly.

use std::panic::catch_unwind;

fn double() {
struct Double;

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

let _d = Double;

panic!("once");
}

fn main() {
assert!(catch_unwind(|| double()).is_err());
}

0 comments on commit 4197be9

Please sign in to comment.