Rework handling of recursive panics

Author: Amanieu

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

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