Auto merge of rust-lang#110975 - Amanieu:panic_count, r=joshtriplett

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

Author: bors

library/std/src/panicking.rs

@@ -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
@@ -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));
         });
     }
 
@@ -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
@@ -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)
     }
 }
 
@@ -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();
     }
@@ -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();
     }
 
@@ -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>);
 

src/tools/miri/src/concurrency/thread.rs

@@ -133,10 +133,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>>,
@@ -206,7 +211,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,
         }
@@ -216,7 +221,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: _,
@@ -226,7 +231,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)

src/tools/miri/src/shims/panic.rs

@@ -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)?;
@@ -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()?;

src/tools/miri/tests/fail/panic/double_panic.rs

@@ -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"
 
@@ -11,6 +9,7 @@ impl Drop for Foo {
     }
 }
 fn main() {
+    //~^ERROR: panic in a function that cannot unwind
     let _foo = Foo;
     panic!("first");
 }

src/tools/miri/tests/fail/panic/double_panic.stderr

@@ -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
 

src/tools/miri/tests/pass/panic/nested_panic_caught.rs

@@ -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());
+}

src/tools/miri/tests/pass/panic/nested_panic_caught.stderr

@@ -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:

tests/ui/backtrace.rs

@@ -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(),

tests/ui/panics/nested_panic_caught.rs

@@ -0,0 +1,24 @@
+// run-pass
+// needs-unwind
+
+// 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());
+}