Skip to content
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

Prevent __rust_begin_short_backtrace frames from being tail-call optimised away #75048

Merged
merged 1 commit into from
Aug 8, 2020
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
9 changes: 7 additions & 2 deletions library/std/src/panicking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -434,7 +434,9 @@ 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
rust_panic_with_hook(&mut PanicPayload::new(msg), info.message(), loc);
crate::sys_common::backtrace::__rust_end_short_backtrace(move || {
rust_panic_with_hook(&mut PanicPayload::new(msg), info.message(), loc);
})
}

/// This is the entry point of panicking for the non-format-string variants of
Expand All @@ -453,7 +455,10 @@ pub fn begin_panic<M: Any + Send>(msg: M) -> ! {
intrinsics::abort()
}

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

struct PanicPayload<A> {
inner: Option<A>,
Expand Down
10 changes: 6 additions & 4 deletions library/std/src/rt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,7 @@ fn lang_start_internal(
sys::args::init(argc, argv);

// Let's run some code!
let exit_code = panic::catch_unwind(|| {
sys_common::backtrace::__rust_begin_short_backtrace(move || main())
});
let exit_code = panic::catch_unwind(main);

sys_common::cleanup();
exit_code.unwrap_or(101) as isize
Expand All @@ -64,5 +62,9 @@ fn lang_start<T: crate::process::Termination + 'static>(
argc: isize,
argv: *const *const u8,
) -> isize {
lang_start_internal(&move || main().report(), argc, argv)
lang_start_internal(
&move || crate::sys_common::backtrace::__rust_begin_short_backtrace(main).report(),
argc,
argv,
)
}
39 changes: 34 additions & 5 deletions library/std/src/sys_common/backtrace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ unsafe fn _print_fmt(fmt: &mut fmt::Formatter<'_>, print_fmt: PrintFmt) -> fmt::
bt_fmt.add_context()?;
let mut idx = 0;
let mut res = Ok(());
// Start immediately if we're not using a short backtrace.
let mut start = print_fmt != PrintFmt::Short;
backtrace_rs::trace_unsynchronized(|frame| {
if print_fmt == PrintFmt::Short && idx > MAX_NB_FRAMES {
return false;
Expand All @@ -89,16 +91,24 @@ unsafe fn _print_fmt(fmt: &mut fmt::Formatter<'_>, print_fmt: PrintFmt) -> fmt::
stop = true;
return;
}
if sym.contains("__rust_end_short_backtrace") {
start = true;
return;
}
}
}

res = bt_fmt.frame().symbol(frame, symbol);
if start {
res = bt_fmt.frame().symbol(frame, symbol);
}
});
if stop {
return false;
}
if !hit {
res = bt_fmt.frame().print_raw(frame.ip(), None, None, None);
if start {
res = bt_fmt.frame().print_raw(frame.ip(), None, None, None);
}
}

idx += 1;
Expand All @@ -123,10 +133,29 @@ unsafe fn _print_fmt(fmt: &mut fmt::Formatter<'_>, print_fmt: PrintFmt) -> fmt::
pub fn __rust_begin_short_backtrace<F, T>(f: F) -> T
where
F: FnOnce() -> T,
F: Send,
T: Send,
{
f()
let result = f();

// prevent this frame from being tail-call optimised away
crate::hint::black_box(());

result
}

/// Fixed frame used to clean the backtrace with `RUST_BACKTRACE=1`. Note that
/// this is only inline(never) when backtraces in libstd are enabled, otherwise
/// it's fine to optimize away.
#[cfg_attr(feature = "backtrace", inline(never))]
pub fn __rust_end_short_backtrace<F, T>(f: F) -> T
where
F: FnOnce() -> T,
{
let result = f();

// prevent this frame from being tail-call optimised away
crate::hint::black_box(());

result
}

pub enum RustBacktrace {
Expand Down
5 changes: 4 additions & 1 deletion library/test/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -514,7 +514,10 @@ pub fn run_test(
/// Fixed frame used to clean the backtrace with `RUST_BACKTRACE=1`.
#[inline(never)]
fn __rust_begin_short_backtrace<F: FnOnce()>(f: F) {
f()
f();

// prevent this frame from being tail-call optimised away
black_box(());
}

fn run_test_in_process(
Expand Down
18 changes: 18 additions & 0 deletions src/test/ui/panics/issue-47429-short-backtraces.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// Regression test for #47429: short backtraces were not terminating correctly

// compile-flags: -O
// run-fail
// check-run-results
// exec-env:RUST_BACKTRACE=1

// ignore-msvc see #62897 and `backtrace-debuginfo.rs` test
// ignore-android FIXME #17520
// ignore-cloudabi spawning processes is not supported
// ignore-openbsd no support for libbacktrace without filename
// ignore-wasm no panic or subprocess support
// ignore-emscripten no panic or subprocess support
// ignore-sgx no subprocess support

fn main() {
panic!()
}
5 changes: 5 additions & 0 deletions src/test/ui/panics/issue-47429-short-backtraces.run.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
thread 'main' panicked at 'explicit panic', $DIR/issue-47429-short-backtraces.rs:17:5
stack backtrace:
0: std::panicking::begin_panic
1: issue_47429_short_backtraces::main
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.