Skip to content

avoid triple-backtrace due to panic-during-cleanup #115280

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions library/alloc/src/alloc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -395,9 +395,10 @@ pub mod __alloc_error_handler {
if unsafe { __rust_alloc_error_handler_should_panic != 0 } {
panic!("memory allocation of {size} bytes failed")
} else {
core::panicking::panic_nounwind_fmt(format_args!(
"memory allocation of {size} bytes failed"
))
core::panicking::panic_nounwind_fmt(
format_args!("memory allocation of {size} bytes failed"),
/* force_no_backtrace */ false,
)
}
}
}
Expand Down
15 changes: 14 additions & 1 deletion library/core/src/panic/panic_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ pub struct PanicInfo<'a> {
message: Option<&'a fmt::Arguments<'a>>,
location: &'a Location<'a>,
can_unwind: bool,
force_no_backtrace: bool,
}

impl<'a> PanicInfo<'a> {
Expand All @@ -42,9 +43,10 @@ impl<'a> PanicInfo<'a> {
message: Option<&'a fmt::Arguments<'a>>,
location: &'a Location<'a>,
can_unwind: bool,
force_no_backtrace: bool,
) -> Self {
struct NoPayload;
PanicInfo { location, message, payload: &NoPayload, can_unwind }
PanicInfo { location, message, payload: &NoPayload, can_unwind, force_no_backtrace }
}

#[unstable(
Expand Down Expand Up @@ -141,6 +143,17 @@ impl<'a> PanicInfo<'a> {
pub fn can_unwind(&self) -> bool {
self.can_unwind
}

#[unstable(
feature = "panic_internals",
reason = "internal details of the implementation of the `panic!` and related macros",
issue = "none"
)]
#[doc(hidden)]
#[inline]
pub fn force_no_backtrace(&self) -> bool {
self.force_no_backtrace
}
}

#[stable(feature = "panic_hook_display", since = "1.26.0")]
Expand Down
37 changes: 29 additions & 8 deletions library/core/src/panicking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,12 @@ pub const fn panic_fmt(fmt: fmt::Arguments<'_>) -> ! {
fn panic_impl(pi: &PanicInfo<'_>) -> !;
}

let pi = PanicInfo::internal_constructor(Some(&fmt), Location::caller(), true);
let pi = PanicInfo::internal_constructor(
Some(&fmt),
Location::caller(),
/* can_unwind */ true,
/* force_no_backtrace */ false,
);

// SAFETY: `panic_impl` is defined in safe Rust code and thus is safe to call.
unsafe { panic_impl(&pi) }
Expand All @@ -77,7 +82,7 @@ pub const fn panic_fmt(fmt: fmt::Arguments<'_>) -> ! {
// and unwinds anyway, we will hit the "unwinding out of nounwind function" guard,
// which causes a "panic in a function that cannot unwind".
#[rustc_nounwind]
pub fn panic_nounwind_fmt(fmt: fmt::Arguments<'_>) -> ! {
pub fn panic_nounwind_fmt(fmt: fmt::Arguments<'_>, force_no_backtrace: bool) -> ! {
if cfg!(feature = "panic_immediate_abort") {
super::intrinsics::abort()
}
Expand All @@ -90,7 +95,12 @@ pub fn panic_nounwind_fmt(fmt: fmt::Arguments<'_>) -> ! {
}

// PanicInfo with the `can_unwind` flag set to false forces an abort.
let pi = PanicInfo::internal_constructor(Some(&fmt), Location::caller(), false);
let pi = PanicInfo::internal_constructor(
Some(&fmt),
Location::caller(),
/* can_unwind */ false,
force_no_backtrace,
);

// SAFETY: `panic_impl` is defined in safe Rust code and thus is safe to call.
unsafe { panic_impl(&pi) }
Expand Down Expand Up @@ -123,7 +133,15 @@ pub const fn panic(expr: &'static str) -> ! {
#[lang = "panic_nounwind"] // needed by codegen for non-unwinding panics
#[rustc_nounwind]
pub fn panic_nounwind(expr: &'static str) -> ! {
panic_nounwind_fmt(fmt::Arguments::new_const(&[expr]));
panic_nounwind_fmt(fmt::Arguments::new_const(&[expr]), /* force_no_backtrace */ false);
}

/// Like `panic_nounwind`, but also inhibits showing a backtrace.
#[cfg_attr(not(feature = "panic_immediate_abort"), inline(never), cold)]
#[cfg_attr(feature = "panic_immediate_abort", inline)]
#[rustc_nounwind]
pub fn panic_nounwind_nobacktrace(expr: &'static str) -> ! {
panic_nounwind_fmt(fmt::Arguments::new_const(&[expr]), /* force_no_backtrace */ true);
}

#[inline]
Expand Down Expand Up @@ -172,9 +190,12 @@ fn panic_misaligned_pointer_dereference(required: usize, found: usize) -> ! {
super::intrinsics::abort()
}

panic_nounwind_fmt(format_args!(
"misaligned pointer dereference: address must be a multiple of {required:#x} but is {found:#x}"
))
panic_nounwind_fmt(
format_args!(
"misaligned pointer dereference: address must be a multiple of {required:#x} but is {found:#x}"
),
/* force_no_backtrace */ false,
)
}

/// Panic because we cannot unwind out of a function.
Expand Down Expand Up @@ -205,7 +226,7 @@ fn panic_cannot_unwind() -> ! {
#[rustc_nounwind]
fn panic_in_cleanup() -> ! {
// Keep the text in sync with `UnwindTerminateReason::as_str` in `rustc_middle`.
panic_nounwind("panic in a destructor during cleanup")
panic_nounwind_nobacktrace("panic in a destructor during cleanup")
}

/// This function is used instead of panic_fmt in const eval.
Expand Down
36 changes: 30 additions & 6 deletions library/std/src/panicking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,9 @@ fn default_hook(info: &PanicInfo<'_>) {
pub fn panic_hook_with_disk_dump(info: &PanicInfo<'_>, path: Option<&crate::path::Path>) {
// If this is a double panic, make sure that we print a backtrace
// for this panic. Otherwise only print it if logging is enabled.
let backtrace = if panic_count::get_count() >= 2 {
let backtrace = if info.force_no_backtrace() {
None
} else if panic_count::get_count() >= 2 {
BacktraceStyle::full()
} else {
crate::panic::get_backtrace_style()
Expand Down Expand Up @@ -294,7 +296,7 @@ pub fn panic_hook_with_disk_dump(info: &PanicInfo<'_>, path: Option<&crate::path
}
}
}
// If backtraces aren't supported, do nothing.
// If backtraces aren't supported or are forced-off, do nothing.
None => {}
}
};
Expand Down Expand Up @@ -615,14 +617,23 @@ pub fn begin_panic_handler(info: &PanicInfo<'_>) -> ! {
let loc = info.location().unwrap(); // The current implementation always returns Some
let msg = info.message().unwrap(); // The current implementation always returns Some
crate::sys_common::backtrace::__rust_end_short_backtrace(move || {
// FIXME: can we just pass `info` along rather than taking it apart here, only to have
// `rust_panic_with_hook` construct a new `PanicInfo`?
if let Some(msg) = msg.as_str() {
rust_panic_with_hook(&mut StrPanicPayload(msg), info.message(), loc, info.can_unwind());
rust_panic_with_hook(
&mut StrPanicPayload(msg),
info.message(),
loc,
info.can_unwind(),
info.force_no_backtrace(),
);
} else {
rust_panic_with_hook(
&mut PanicPayload::new(msg),
info.message(),
loc,
info.can_unwind(),
info.force_no_backtrace(),
);
}
})
Expand All @@ -647,7 +658,13 @@ pub const fn begin_panic<M: Any + Send>(msg: M) -> ! {

let loc = Location::caller();
return crate::sys_common::backtrace::__rust_end_short_backtrace(move || {
rust_panic_with_hook(&mut PanicPayload::new(msg), None, loc, true)
rust_panic_with_hook(
&mut PanicPayload::new(msg),
None,
loc,
/* can_unwind */ true,
/* force_no_backtrace */ false,
)
});

struct PanicPayload<A> {
Expand Down Expand Up @@ -693,6 +710,7 @@ fn rust_panic_with_hook(
message: Option<&fmt::Arguments<'_>>,
location: &Location<'_>,
can_unwind: bool,
force_no_backtrace: bool,
) -> ! {
let must_abort = panic_count::increase(true);

Expand All @@ -707,14 +725,20 @@ fn rust_panic_with_hook(
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);
let panicinfo = PanicInfo::internal_constructor(
message,
location,
can_unwind,
force_no_backtrace,
);
rtprintpanic!("{panicinfo}\npanicked after panic::always_abort(), aborting.\n");
}
}
crate::sys::abort_internal();
}

let mut info = PanicInfo::internal_constructor(message, location, can_unwind);
let mut info =
PanicInfo::internal_constructor(message, location, can_unwind, force_no_backtrace);
let hook = HOOK.read().unwrap_or_else(PoisonError::into_inner);
match *hook {
// Some platforms (like wasm) know that printing to stderr won't ever actually
Expand Down
3 changes: 1 addition & 2 deletions src/tools/miri/tests/fail/panic/double_panic.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ second
stack backtrace:
thread 'main' panicked at RUSTLIB/core/src/panicking.rs:LL:CC:
panic in a destructor during cleanup
stack backtrace:
thread caused non-unwinding panic. aborting.
error: abnormal termination: the program aborted execution
--> RUSTLIB/std/src/sys/PLATFORM/mod.rs:LL:CC
Expand All @@ -19,7 +18,7 @@ LL | ABORT();
= 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 `core::panicking::panic_nounwind` at RUSTLIB/core/src/panicking.rs:LL:CC
= note: inside `core::panicking::panic_nounwind_nobacktrace` at RUSTLIB/core/src/panicking.rs:LL:CC
= note: inside `core::panicking::panic_in_cleanup` at RUSTLIB/core/src/panicking.rs:LL:CC
note: inside `main`
--> $DIR/double_panic.rs:LL:CC
Expand Down
10 changes: 5 additions & 5 deletions tests/ui/backtrace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ fn runtest(me: &str) {
#[cfg(not(panic = "abort"))]
{
// Make sure a stack trace is printed
let p = template(me).arg("double-fail").spawn().unwrap();
let p = template(me).arg("double-fail").env("RUST_BACKTRACE","0").spawn().unwrap();
let out = p.wait_with_output().unwrap();
assert!(!out.status.success());
let s = str::from_utf8(&out.stderr).unwrap();
Expand All @@ -106,18 +106,18 @@ fn runtest(me: &str) {
contains_verbose_expected(s, "double"),
"bad output3: {}", s
);
// Make sure it's only one stack trace.
assert_eq!(s.split("stack backtrace").count(), 2);

// Make sure a stack trace isn't printed too many times
//
// Currently it is printed 3 times ("once", "twice" and "panic in a destructor during
// cleanup") but in the future the last one may be removed.
// even with RUST_BACKTRACE=1. It should be printed twice.
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..3 {
for _ in 0..2 {
i += s[i + 10..].find("stack backtrace").unwrap() + 10;
}
assert!(s[i + 10..].find("stack backtrace").is_none(),
Expand Down
1 change: 1 addition & 0 deletions tests/ui/panics/panic-in-cleanup.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
// error-pattern: panic in a destructor during cleanup
// normalize-stderr-test: "\n +[0-9]+:[^\n]+" -> ""
// normalize-stderr-test: "\n +at [^\n]+" -> ""
// normalize-stderr-test: "(core/src/panicking\.rs):[0-9]+:[0-9]+" -> "$1:$$LINE:$$COL"
// needs-unwind
// ignore-emscripten "RuntimeError" junk in output
// ignore-msvc SEH doesn't do panic-during-cleanup the same way as everyone else
Expand Down
7 changes: 3 additions & 4 deletions tests/ui/panics/panic-in-cleanup.run.stderr
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
thread 'main' panicked at $DIR/panic-in-cleanup.rs:21:5:
thread 'main' panicked at $DIR/panic-in-cleanup.rs:22:5:
explicit panic
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'main' panicked at $DIR/panic-in-cleanup.rs:15:9:
thread 'main' panicked at $DIR/panic-in-cleanup.rs:16:9:
BOOM
stack backtrace:
thread 'main' panicked at library/core/src/panicking.rs:126:5:
thread 'main' panicked at library/core/src/panicking.rs:$LINE:$COL:
panic in a destructor during cleanup
stack backtrace:
thread caused non-unwinding panic. aborting.
1 change: 1 addition & 0 deletions tests/ui/panics/panic-in-ffi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
// error-pattern: panic in a function that cannot unwind
// normalize-stderr-test: "\n +[0-9]+:[^\n]+" -> ""
// normalize-stderr-test: "\n +at [^\n]+" -> ""
// normalize-stderr-test: "(core/src/panicking\.rs):[0-9]+:[0-9]+" -> "$1:$$LINE:$$COL"
// needs-unwind
// ignore-emscripten "RuntimeError" junk in output
#![feature(c_unwind)]
Expand Down
4 changes: 2 additions & 2 deletions tests/ui/panics/panic-in-ffi.run.stderr
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
thread 'main' panicked at $DIR/panic-in-ffi.rs:12:5:
thread 'main' panicked at $DIR/panic-in-ffi.rs:13:5:
Test
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'main' panicked at library/core/src/panicking.rs:126:5:
thread 'main' panicked at library/core/src/panicking.rs:$LINE:$COL:
panic in a function that cannot unwind
stack backtrace:
thread caused non-unwinding panic. aborting.