Skip to content

Ignore more frames on backtrace unwinding. #40264

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 17 commits into from
Closed

Ignore more frames on backtrace unwinding. #40264

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
680e2b7
Ignore more frames on backtrace unwinding.
Yamakaky Mar 4, 2017
2183ef6
Remove unneeded blacklisted functions
Yamakaky Mar 4, 2017
dfaed07
Fix long lines
Yamakaky Mar 4, 2017
c40ea76
Handle panic::catch_unwind.
Yamakaky Mar 4, 2017
33e42a3
Support of _rust... and panic_unwind::__rust...
Yamakaky Mar 4, 2017
d54caab
Fix index calculation
Yamakaky Mar 4, 2017
3353a42
Hide backtrace when RUST_BACKTRACE is empty
Yamakaky Mar 4, 2017
deeaa73
[backtrace] Remove dupplicated checks
Yamakaky Mar 17, 2017
7d667e4
[backtrace] use inline(never) functions instead of blacklist
Yamakaky Mar 21, 2017
00b991e
[backtrace] improve doc
Yamakaky Mar 21, 2017
5205b8d
[backtrace] Re-add MSVC handling
Yamakaky Mar 21, 2017
8a75b20
Use private mangled __rust_begin_short_backtrace_*
Yamakaky Mar 23, 2017
03f9940
Handle threads
Yamakaky Mar 23, 2017
b36446a
Use first found frame instead of last
Yamakaky Mar 23, 2017
0e16333
Better cleaning of benchmarks
Yamakaky Mar 23, 2017
e1ca626
Use unique backtrace start frame for all std
Yamakaky Mar 27, 2017
d72937d
Unused frames
Yamakaky Mar 27, 2017
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
14 changes: 11 additions & 3 deletions src/libstd/rt.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,7 @@ pub use panicking::{begin_panic, begin_panic_fmt, update_panic_count};

#[cfg(not(test))]
#[lang = "start"]
fn lang_start(main: *const u8, argc: isize, argv: *const *const u8) -> isize {
use mem;
fn lang_start(main: fn(), argc: isize, argv: *const *const u8) -> isize {
use panic;
use sys;
use sys_common;
Expand All @@ -54,7 +53,9 @@ fn lang_start(main: *const u8, argc: isize, argv: *const *const u8) -> isize {
sys::args::init(argc, argv);

// Let's run some code!
let res = panic::catch_unwind(mem::transmute::<_, fn()>(main));
let res = panic::catch_unwind(|| {
__rust_begin_backtrace_binary(main)
});
sys_common::cleanup();
res.is_err()
};
Expand All @@ -65,3 +66,10 @@ fn lang_start(main: *const u8, argc: isize, argv: *const *const u8) -> isize {
0
}
}

/// Fixed frame used to clean the backtrace with `RUST_BACKTRACE=1`.
#[no_mangle]
#[inline(never)]
pub fn __rust_begin_backtrace_binary(f: fn()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

pub is not needed here

Copy link
Contributor

Choose a reason for hiding this comment

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

Or you can keep this pub, add #[doc(hidden)] and reuse it in libtest.

Copy link
Contributor Author

@Yamakaky Yamakaky Mar 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

https://play.rust-lang.org/?gist=e7c60682076e24a4f2ad4fe56af59de1&version=stable&backtrace=1

What do you think would be better, #[ignore(..)] the warning, or keep it pub and hide the doc?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ignore.

I've just realized though, since we use contains and not exact matching, then no_mangle can be just removed most probably.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, even easier ^^

Copy link
Contributor

Choose a reason for hiding this comment

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

If possible, I would follow the example of rust_panic and use #[no_mangle] and #[allow(private_no_mangle_fns)]:

rust/src/libstd/panicking.rs

Lines 574 to 577 in a559452

/// A private no-mangle function on which to slap yer breakpoints.
#[no_mangle]
#[allow(private_no_mangle_fns)] // yes we get it, but we like breakpoints
pub fn rust_panic(msg: Box<Any + Send>) -> ! {

f()
}
64 changes: 24 additions & 40 deletions src/libstd/sys_common/backtrace.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,56 +104,30 @@ fn filter_frames(frames: &[Frame],
// We want to filter out frames with some prefixes
// from both top and bottom of the call stack.
static BAD_PREFIXES_TOP: &'static [&'static str] = &[
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove this array entirely? Currently rust_panic is the "canonical frame name" of the panic, and if there's a lower frame we should start from then we should just give that a canonical name instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What to you think about removing the frames from the core::panicking::panic* functions?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine to strive to eliminate them, but we shouldn't have such a long list of what to filter. There should be one frame that we look for and if we find it maybe do a few calculations based off that. Substring searching is basically guarantee to go wrong in the future.

"_ZN3std3sys3imp9backtrace",
// std::sys::imp::backtrace
Copy link
Contributor

Choose a reason for hiding this comment

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

As I said before, the a::b::c forms show up on Windows/MSVC.
Yes, that's probably a demangler problem, but if it's not fixed, then these symbols will not be filtered away.

"ZN3std3sys3imp9backtrace",
"std::sys::imp::backtrace",
"_ZN3std10sys_common9backtrace",
// std::sys_common::backtrace
"ZN3std10sys_common9backtrace",
"std::sys_common::backtrace",
"_ZN3std9panicking",
// std::panicking
"ZN3std9panicking",
"std::panicking",
"_ZN4core9panicking",
// core::panicking
"ZN4core9panicking",
"core::panicking",
"_ZN4core6result13unwrap_failed",
// core::result::unwrap_failed
"ZN4core6result13unwrap_failed",
"core::result::unwrap_failed",

"rust_begin_unwind",
"_ZN4drop",
Copy link
Member

Choose a reason for hiding this comment

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

Er was this added in the previous PR? This definitely seems like something that we shouldn't be dropping, this is just a normal function?

"mingw_set_invalid_parameter_handler",
Copy link
Member

Choose a reason for hiding this comment

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

This and drop above should be removed

];
static BAD_PREFIXES_BOTTOM: &'static [&'static str] = &[
"_ZN3std9panicking",
"ZN3std9panicking",
"std::panicking",
"_ZN3std5panic",
"ZN3std5panic",
"std::panic",
"_ZN4core9panicking",
"ZN4core9panicking",
"core::panicking",
"_ZN3std2rt10lang_start",
"ZN3std2rt10lang_start",
"std::rt::lang_start",
"panic_unwind::__rust_maybe_catch_panic",
"__rust_maybe_catch_panic",
"_rust_maybe_catch_panic",
"__libc_start_main",
"__rust_try",
"_start",
"main",
Copy link
Contributor

Choose a reason for hiding this comment

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

__libc_start_main, _start and main appear at the bottom of the stack and need to be filtered away.
They are removed from the list in this PR, but they may be required again if the "filtering point" is moved from rust_maybe_catch_panic below (e.g. to lang_start), I don't remember exactly what's their relative order.

"BaseThreadInitThunk",
"RtlInitializeExceptionChain",
"__scrt_common_main_seh",
"_ZN4drop",
"mingw_set_invalid_parameter_handler",
"__rust_begin_backtrace_binary",
"__rust_begin_backtrace_test",
];

let is_good_frame = |frame: Frame, bad_prefixes: &[&str]| {
resolve_symname(frame, |symname| {
if let Some(mangled_symbol_name) = symname {
if !bad_prefixes.iter().any(|s| mangled_symbol_name.starts_with(s)) {
if !bad_prefixes.iter().any(|s| mangled_symbol_name.contains(s)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we use no_mangle functions as boundaries, it should be possible to check that the symbol name is exactly equal to the expected one, instead of just a prefix or substring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, some platforms seems to have different behaviours, like __rust_try vs _rust_try if I remember correctly, so we contains it should cover all cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

I know, but that should be the only cases (no ::h49fe0d471b73c20e trailers).
I do not expect anybody to define functions which contain the patterns the code is matching for, but a strict comparison would help keeping us on the safe side, just in case ;)

return Ok(())
}
}
Expand All @@ -164,11 +138,21 @@ fn filter_frames(frames: &[Frame],
let skipped_before = frames.iter().position(|frame| {
is_good_frame(*frame, BAD_PREFIXES_TOP)
}).unwrap_or(frames.len());
let skipped_after = frames[skipped_before..].iter().rev().position(|frame| {
is_good_frame(*frame, BAD_PREFIXES_BOTTOM)
}).unwrap_or(frames.len() - skipped_before);

if skipped_before + skipped_after == frames.len() {
let skipped_after = frames.iter().rev().position(|frame| {
let mut is_rmcp = false;
let _ = resolve_symname(*frame, |symname| {
if let Some(mangled_symbol_name) = symname {
if BAD_PREFIXES_BOTTOM.iter().any(|s| mangled_symbol_name.contains(s)) {
is_rmcp = true;
}
}
Ok(())
}, context);
is_rmcp
}).map(|x| x + 1 /* also ignore the marker frame */).unwrap_or(0);

if skipped_before + skipped_after >= frames.len() {
// Avoid showing completely empty backtraces
return (0, 0);
}
Expand Down Expand Up @@ -198,7 +182,7 @@ pub fn log_enabled() -> Option<PrintFormat> {
}

let val = match env::var_os("RUST_BACKTRACE") {
Some(x) => if &x == "0" {
Some(x) => if &x == "0" || &x == "" {
None
} else if &x == "full" {
Some(PrintFormat::Full)
Expand Down
23 changes: 20 additions & 3 deletions src/libtest/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1363,12 +1363,29 @@ pub fn run_test(opts: &TestOpts,
monitor_ch.send((desc, TrMetrics(mm), Vec::new())).unwrap();
return;
}
DynTestFn(f) => run_test_inner(desc, monitor_ch, opts.nocapture, f),
StaticTestFn(f) => run_test_inner(desc, monitor_ch, opts.nocapture,
Box::new(move |()| f())),
DynTestFn(f) =>
run_test_inner(desc, monitor_ch, opts.nocapture,
Box::new(move |()| __rust_begin_backtrace_test_boxfn(f))),
StaticTestFn(f) =>
run_test_inner(desc, monitor_ch, opts.nocapture,
Box::new(move |()| __rust_begin_backtrace_test(f))),
}
}

/// Fixed frame used to clean the backtrace with `RUST_BACKTRACE=1`.
#[no_mangle]
#[inline(never)]
pub fn __rust_begin_backtrace_test_boxfn(f: Box<FnBox<()>>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Two more unnecessary pubs here and below.

f.call_box(())
}

/// Fixed frame used to clean the backtrace with `RUST_BACKTRACE=1`.
#[no_mangle]
#[inline(never)]
pub fn __rust_begin_backtrace_test(f: fn()) {
f()
}

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