From ef7f0e697bf4f0b8797527ba54f954e452253463 Mon Sep 17 00:00:00 2001 From: Amanieu d'Antras Date: Fri, 28 Apr 2023 14:05:01 +0100 Subject: [PATCH 1/2] Rework handling of recursive panics --- library/std/src/panicking.rs | 120 ++++++++++++++----------- tests/ui/backtrace.rs | 6 +- tests/ui/panics/nested_panic_caught.rs | 24 +++++ 3 files changed, 96 insertions(+), 54 deletions(-) create mode 100644 tests/ui/panics/nested_panic_caught.rs diff --git a/library/std/src/panicking.rs b/library/std/src/panicking.rs index 6d59266b6f838..a6a370409c0e2 100644 --- a/library/std/src/panicking.rs +++ b/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 = 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 { 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) -> ! { - panic_count::increase(); + panic_count::increase(false); struct RewrapBox(Box); diff --git a/tests/ui/backtrace.rs b/tests/ui/backtrace.rs index dd73dd9886a3b..66b378f62d63c 100644 --- a/tests/ui/backtrace.rs +++ b/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(), diff --git a/tests/ui/panics/nested_panic_caught.rs b/tests/ui/panics/nested_panic_caught.rs new file mode 100644 index 0000000000000..d43886e809579 --- /dev/null +++ b/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()); +} From de607f1b5cc99d8ac773205702e184c2461d5e12 Mon Sep 17 00:00:00 2001 From: Amanieu d'Antras Date: Sat, 29 Apr 2023 07:23:54 +0100 Subject: [PATCH 2/2] Add support for nested panics to miri --- src/tools/miri/src/concurrency/thread.rs | 19 ++++++++---- src/tools/miri/src/shims/panic.rs | 5 ++-- .../miri/tests/fail/panic/double_panic.rs | 3 +- .../miri/tests/fail/panic/double_panic.stderr | 29 +++++-------------- .../tests/pass/panic/nested_panic_caught.rs | 25 ++++++++++++++++ .../pass/panic/nested_panic_caught.stderr | 4 +++ 6 files changed, 53 insertions(+), 32 deletions(-) create mode 100644 src/tools/miri/tests/pass/panic/nested_panic_caught.rs create mode 100644 src/tools/miri/tests/pass/panic/nested_panic_caught.stderr diff --git a/src/tools/miri/src/concurrency/thread.rs b/src/tools/miri/src/concurrency/thread.rs index d85cac7bcbb58..25c8df43ee26c 100644 --- a/src/tools/miri/src/concurrency/thread.rs +++ b/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>, + /// + /// 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>, /// Last OS error location in memory. It is a 32-bit integer. pub(crate) last_error: Option>, @@ -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) diff --git a/src/tools/miri/src/shims/panic.rs b/src/tools/miri/src/shims/panic.rs index 18ae01a19f914..7aefdfcb976af 100644 --- a/src/tools/miri/src/shims/panic.rs +++ b/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()?; diff --git a/src/tools/miri/tests/fail/panic/double_panic.rs b/src/tools/miri/tests/fail/panic/double_panic.rs index 9378adb8609df..adb30714269e8 100644 --- a/src/tools/miri/tests/fail/panic/double_panic.rs +++ b/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"); } diff --git a/src/tools/miri/tests/fail/panic/double_panic.stderr b/src/tools/miri/tests/fail/panic/double_panic.stderr index 77d5fc5d7ceb9..b6ac56f15d4b5 100644 --- a/src/tools/miri/tests/fail/panic/double_panic.stderr +++ b/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 `::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:: - 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 diff --git a/src/tools/miri/tests/pass/panic/nested_panic_caught.rs b/src/tools/miri/tests/pass/panic/nested_panic_caught.rs new file mode 100644 index 0000000000000..884813150ad2a --- /dev/null +++ b/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()); +} diff --git a/src/tools/miri/tests/pass/panic/nested_panic_caught.stderr b/src/tools/miri/tests/pass/panic/nested_panic_caught.stderr new file mode 100644 index 0000000000000..4e2593242df75 --- /dev/null +++ b/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: