Skip to content

Ignore functions before main and thread entry points in backtraces #47824

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Ignore functions before main and thread entry points in backtraces #47824

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 10 additions & 1 deletion src/libstd/rt.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,15 @@
// Re-export some of our utilities which are expected by other crates.
pub use panicking::{begin_panic, begin_panic_fmt, update_panic_count};

#[cfg(feature = "backtrace")]
pub use sys_common::backtrace::begin_short_backtrace;

#[cfg(not(feature = "backtrace"))]
#[unstable(feature = "rt", reason = "this is only exported for use in libtest", issue = "0")]
pub fn begin_short_backtrace<F: FnOnce() -> R, R>(f: F) -> R {
f()
}

// To reduce the generated code of the new `lang_start`, this function is doing
// the real work.
#[cfg(not(test))]
Expand Down Expand Up @@ -56,7 +65,7 @@ fn lang_start_internal(main: &(Fn() -> i32 + Sync + ::panic::RefUnwindSafe),
// Let's run some code!
#[cfg(feature = "backtrace")]
let exit_code = panic::catch_unwind(|| {
::sys_common::backtrace::__rust_begin_short_backtrace(move || main())
::sys_common::backtrace::begin_short_backtrace(move || main())
});
#[cfg(not(feature = "backtrace"))]
let exit_code = panic::catch_unwind(move || main());
Expand Down
8 changes: 4 additions & 4 deletions src/libstd/sys/cloudabi/backtrace.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,17 +95,17 @@ where

pub fn resolve_symname<F>(frame: Frame, callback: F, _: &BacktraceContext) -> io::Result<()>
where
F: FnOnce(Option<&str>) -> io::Result<()>,
F: FnOnce(Option<(&str, usize)>) -> io::Result<()>,
{
unsafe {
let mut info: Dl_info = intrinsics::init();
let symname =
let syminfo =
if dladdr(frame.exact_position as *mut _, &mut info) == 0 || info.dli_sname.is_null() {
None
} else {
CStr::from_ptr(info.dli_sname).to_str().ok()
CStr::from_ptr(info.dli_sname).to_str().ok().map(|s| (s, info.dli_saddr as usize))
};
callback(symname)
callback(syminfo)
}
}

Expand Down
8 changes: 4 additions & 4 deletions src/libstd/sys/unix/backtrace/printing/dladdr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,17 +18,17 @@ use sys_common::backtrace::Frame;
pub fn resolve_symname<F>(frame: Frame,
callback: F,
_: &BacktraceContext) -> io::Result<()>
where F: FnOnce(Option<&str>) -> io::Result<()>
where F: FnOnce(Option<(&str, usize)>) -> io::Result<()>
{
unsafe {
let mut info: Dl_info = intrinsics::init();
let symname = if dladdr(frame.exact_position as *mut _, &mut info) == 0 ||
let syminfo = if dladdr(frame.exact_position as *mut _, &mut info) == 0 ||
info.dli_sname.is_null() {
None
} else {
CStr::from_ptr(info.dli_sname).to_str().ok()
CStr::from_ptr(info.dli_sname).to_str().ok().map(|s| (s, info.dli_saddr as usize))
};
callback(symname)
callback(syminfo)
}
}

Expand Down
8 changes: 4 additions & 4 deletions src/libstd/sys/unix/backtrace/printing/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,11 @@ pub use sys_common::gnu::libbacktrace::foreach_symbol_fileline;
#[cfg(not(target_os = "emscripten"))]
pub fn resolve_symname<F>(frame: Frame, callback: F, bc: &BacktraceContext) -> io::Result<()>
where
F: FnOnce(Option<&str>) -> io::Result<()>
F: FnOnce(Option<(&str, usize)>) -> io::Result<()>
{
::sys_common::gnu::libbacktrace::resolve_symname(frame, |symname| {
if symname.is_some() {
callback(symname)
::sys_common::gnu::libbacktrace::resolve_symname(frame, |syminfo| {
if syminfo.is_some() {
callback(syminfo)
} else {
dladdr::resolve_symname(frame, callback, bc)
}
Expand Down
2 changes: 1 addition & 1 deletion src/libstd/sys/wasm/backtrace.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ pub fn unwind_backtrace(_frames: &mut [Frame])
pub fn resolve_symname<F>(_frame: Frame,
_callback: F,
_: &BacktraceContext) -> io::Result<()>
where F: FnOnce(Option<&str>) -> io::Result<()>
where F: FnOnce(Option<(&str, usize)>) -> io::Result<()>
{
unsupported()
}
Expand Down
10 changes: 6 additions & 4 deletions src/libstd/sys/windows/backtrace/printing/msvc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ type SymGetLineFromInlineContextFn =
pub fn resolve_symname<F>(frame: Frame,
callback: F,
context: &BacktraceContext) -> io::Result<()>
where F: FnOnce(Option<&str>) -> io::Result<()>
where F: FnOnce(Option<(&str, usize)>) -> io::Result<()>
{
let SymFromInlineContext = sym!(&context.dbghelp,
"SymFromInlineContext",
Expand Down Expand Up @@ -57,13 +57,15 @@ pub fn resolve_symname<F>(frame: Frame,
} else {
false
};
let symname = if valid_range {
let syminfo = if valid_range {
let ptr = info.Name.as_ptr() as *const c_char;
CStr::from_ptr(ptr).to_str().ok()
CStr::from_ptr(ptr).to_str().ok().map(|s| {
(s, (frame.symbol_addr as usize).wrapping_sub(displacement as usize))
})
} else {
None
};
callback(symname)
callback(syminfo)
}
}

Expand Down
40 changes: 27 additions & 13 deletions src/libstd/sys_common/backtrace.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,8 @@ fn _print(w: &mut Write, format: PrintFormat) -> io::Result<()> {

let filtered_frames = &frames[..nb_frames - skipped_after];
for (index, frame) in filtered_frames.iter().skip(skipped_before).enumerate() {
resolve_symname(*frame, |symname| {
output(w, index, *frame, symname, format)
resolve_symname(*frame, |syminfo| {
output(w, index, *frame, syminfo.map(|i| i.0), format)
}, &context)?;
let has_more_filenames = foreach_symbol_fileline(*frame, |file, line| {
output_fileline(w, file, line, format)
Expand All @@ -105,14 +105,14 @@ fn filter_frames(frames: &[Frame],

let skipped_before = 0;

// Look for the first occurence of `mark_start`
// There can be multiple in one backtrace
// Skip all frames after that
let skipped_after = frames.len() - frames.iter().position(|frame| {
let mut is_marker = false;
let _ = resolve_symname(*frame, |symname| {
if let Some(mangled_symbol_name) = symname {
// Use grep to find the concerned functions
if mangled_symbol_name.contains("__rust_begin_short_backtrace") {
is_marker = true;
}
let _ = resolve_symname(*frame, |syminfo| {
if syminfo.map(|i| i.1) == Some(MARK_START as usize) {
is_marker = true;
}
Ok(())
}, context);
Expand All @@ -127,13 +127,27 @@ fn filter_frames(frames: &[Frame],
(skipped_before, skipped_after)
}

static MARK_START: fn(&mut FnMut()) = mark_start;

/// Fixed frame used to clean the backtrace with `RUST_BACKTRACE=1`.
/// Fixed frame used to clean the backtrace with `RUST_BACKTRACE=1`
#[inline(never)]
pub fn __rust_begin_short_backtrace<F, T>(f: F) -> T
where F: FnOnce() -> T, F: Send, T: Send
{
f()
fn mark_start(f: &mut FnMut()) {
f();
unsafe {
asm!("" ::: "memory" : "volatile"); // A dummy statement to prevent tail call optimization
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may not compile on platforms like asm.js, so I wonder if we could take a perhaps different route to implement this? Could this call a #[inline(never)] function which doesn't throw using the notail attribute on the call? I think that'd do the same thing, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

f may throw here, so we cannot use notail. We need something that is opaque to LLVM. I suggest we just disable this asm! expressions on platforms which do not support it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes indeed that's why we can't notail the call to f, but I"m thinking something like:

fn mark_start(f: &mut FnMut()) {
    f();
    #[notail]
    foo();

    #[inline(never)]
    fn foo() {}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LLVM could still see that foo has no side effect, and just remove the call.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah indeed, in that case could something more portable than asm! be used, like volatile reads or something like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think volatile operations will work as they could be reordered to happen before the call to f, if f contains no volatile operations itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does LLVM accept empty inline assembly for asm.js?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK no we disable inline assembly completely on asm.js. Does the reordering actually happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was asking about LLVM not rustc though. I do not know if the reordering could happen given LLVM's current optimizations.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least in the tests I was running it wasn't reordering, but I'm not sure if that's a guarantee? I figure we could at least try it as this'll need to be upgraded for emscripten anyway.

}
}

/// Convenience wrapper for `mark_start`
#[unstable(feature = "rt", reason = "this is only exported for use in libtest", issue = "0")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this reason be updated as I think this is used by threading/main now, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are private usages in libstd though, which do not require an export.

pub fn begin_short_backtrace<F: FnOnce() -> R, R>(f: F) -> R {
let mut f = Some(f);
let mut r = None;
mark_start(&mut || {
let f = f.take().unwrap();
r = Some(f());
});
r.unwrap()
}

/// Controls how the backtrace should be formated.
Expand Down
20 changes: 10 additions & 10 deletions src/libstd/sys_common/gnu/libbacktrace.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,34 +63,34 @@ where F: FnMut(&[u8], u32) -> io::Result<()>
pub fn resolve_symname<F>(frame: Frame,
callback: F,
_: &BacktraceContext) -> io::Result<()>
where F: FnOnce(Option<&str>) -> io::Result<()>
where F: FnOnce(Option<(&str, usize)>) -> io::Result<()>
{
let symname = {
let syminfo = {
let state = unsafe { init_state() };
if state.is_null() {
return Err(io::Error::new(
io::ErrorKind::Other,
"failed to allocate libbacktrace state")
)
}
let mut data: *const libc::c_char = ptr::null();
let data_addr = &mut data as *mut *const libc::c_char;
let mut data: (*const libc::c_char, _) = (ptr::null(), 0);
let data_addr = &mut data as *mut _;
let ret = unsafe {
backtrace_syminfo(state,
frame.symbol_addr as libc::uintptr_t,
syminfo_cb,
error_cb,
data_addr as *mut libc::c_void)
};
if ret == 0 || data.is_null() {
if ret == 0 || data.0.is_null() {
None
} else {
unsafe {
CStr::from_ptr(data).to_str().ok()
CStr::from_ptr(data.0).to_str().ok().map(|s| (s, data.1))
}
}
};
callback(symname)
callback(syminfo)
}

////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -145,10 +145,10 @@ extern fn error_cb(_data: *mut libc::c_void, _msg: *const libc::c_char,
extern fn syminfo_cb(data: *mut libc::c_void,
_pc: libc::uintptr_t,
symname: *const libc::c_char,
_symval: libc::uintptr_t,
symval: libc::uintptr_t,
_symsize: libc::uintptr_t) {
let slot = data as *mut *const libc::c_char;
unsafe { *slot = symname; }
let slot = data as *mut (*const libc::c_char, usize);
unsafe { *slot = (symname, symval); }
}
extern fn pcinfo_cb(data: *mut libc::c_void,
_pc: libc::uintptr_t,
Expand Down
2 changes: 1 addition & 1 deletion src/libstd/thread/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,7 @@ impl Builder {
thread_info::set(imp::guard::current(), their_thread);
#[cfg(feature = "backtrace")]
let try_result = panic::catch_unwind(panic::AssertUnwindSafe(|| {
::sys_common::backtrace::__rust_begin_short_backtrace(f)
::sys_common::backtrace::begin_short_backtrace(f)
}));
#[cfg(not(feature = "backtrace"))]
let try_result = panic::catch_unwind(panic::AssertUnwindSafe(f));
Expand Down
16 changes: 6 additions & 10 deletions src/libtest/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
#![feature(set_stdio)]
#![feature(panic_unwind)]
#![feature(staged_api)]
#![feature(rt)]

extern crate getopts;
extern crate term;
Expand Down Expand Up @@ -72,6 +73,7 @@ use std::sync::{Arc, Mutex};
use std::thread;
use std::time::{Instant, Duration};
use std::borrow::Cow;
use std::rt::begin_short_backtrace;

const TEST_WARN_TIMEOUT_S: u64 = 60;
const QUIET_MODE_MAX_COLUMN: usize = 100; // insert a '\n' after 100 tests in quiet mode
Expand Down Expand Up @@ -1344,14 +1346,14 @@ pub fn convert_benchmarks_to_tests(tests: Vec<TestDescAndFn>) -> Vec<TestDescAnd
DynBenchFn(bench) => {
DynTestFn(Box::new(move || {
bench::run_once(|b| {
__rust_begin_short_backtrace(|| bench.run(b))
begin_short_backtrace(|| bench.run(b))
})
}))
}
StaticBenchFn(benchfn) => {
DynTestFn(Box::new(move || {
bench::run_once(|b| {
__rust_begin_short_backtrace(|| benchfn(b))
begin_short_backtrace(|| benchfn(b))
})
}))
}
Expand Down Expand Up @@ -1444,23 +1446,17 @@ pub fn run_test(
}
DynTestFn(f) => {
let cb = move || {
__rust_begin_short_backtrace(f)
begin_short_backtrace(f)
};
run_test_inner(desc, monitor_ch, opts.nocapture, Box::new(cb))
}
StaticTestFn(f) => {
run_test_inner(desc, monitor_ch, opts.nocapture,
Box::new(move || __rust_begin_short_backtrace(f)))
Box::new(move || begin_short_backtrace(f)))
}
}
}

/// Fixed frame used to clean the backtrace with `RUST_BACKTRACE=1`.
#[inline(never)]
fn __rust_begin_short_backtrace<F: FnOnce()>(f: F) {
f()
}

fn calc_result(desc: &TestDesc, task_result: Result<(), Box<Any + Send>>) -> TestResult {
match (&desc.should_panic, task_result) {
(&ShouldPanic::No, Ok(())) |
Expand Down