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.

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

Jump to bottom

Ignore more frames on backtrace unwinding. #40264

Closed
wants to merge 17 commits into from
Closed

Ignore more frames on backtrace unwinding. #40264

Changes from 4 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
65 changes: 50 additions & 15 deletions src/libstd/sys_common/backtrace.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,47 +107,66 @@ fn filter_frames(frames: &[Frame],
"_ZN3std3sys3imp9backtrace",
"ZN3std3sys3imp9backtrace",
"std::sys::imp::backtrace",

"_ZN3std10sys_common9backtrace",
"ZN3std10sys_common9backtrace",
"std::sys_common::backtrace",

"_ZN3std9panicking",
"ZN3std9panicking",
"std::panicking",

"_ZN4core9panicking",
"ZN4core9panicking",
"core::panicking",

"_ZN4core6result13unwrap_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] = &[
"_ZN4core9panicking",
"ZN4core9panicking",
"core::panicking",

"_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",
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that we want this to be the bottom of a frame, this is just one panic::catch_unwind and there could be many on the stack.

"_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",
Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC, _ZN4drop were some kind of corrupted misinterpreted frames on 32-bit mingw (similar to mingw_set_invalid_parameter_handler). Not sure how reproducible they are on executables other than run-pass/bactrace.rs test.

"mingw_set_invalid_parameter_handler",

"_ZN4core3ops6FnOnce9call_once",
"ZN4core3ops6FnOnce9call_once",
"core::ops::FnOnce::call_once",
Copy link
Member

Choose a reason for hiding this comment

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

Like above, these could be any rust function, so we shouldn't prune these

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 fail to find an example where this frame would be at the beginning of the frame and you would not want to remove it. There will always be the main, or a function if it's a library.


// tests
"_ZN91_$LT$std..panic..AssertUnwindSafe$LT$F$GT$$u20$as$u20$core..ops..FnOnce\
$LT$$LP$$RP$$GT$$GT$9call_once",
"ZN91_$LT$std..panic..AssertUnwindSafe$LT$F$GT$$u20$as$u20$core..ops..FnOnce\
$LT$$LP$$RP$$GT$$GT$9call_once",
"<std::panic::AssertUnwindSafe<F> as core::ops::FnOnce<()>>::call_once",
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that these are correct to have because this is just the beginning of a catch_unwind, 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.

So? It's std stuff, we want to remove it.


"_ZN4test8run_test",
"ZN4test8run_test",
"test::run_test",

"_ZN42_$LT$F$u20$as$u20$test..FnBox$LT$T$GT$$GT$8call_box",
"ZN42_$LT$F$u20$as$u20$test..FnBox$LT$T$GT$$GT$8call_box",
"<F as test::FnBox<T>>::call_box",
Copy link
Member

Choose a reason for hiding this comment

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

These can happen at any time, right? Not just during tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure, but it was fine during my tests.


];

let is_good_frame = |frame: Frame, bad_prefixes: &[&str]| {
Expand All @@ -164,11 +183,27 @@ 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| {
let idx_catch_panic = skipped_before + frames[skipped_before..].iter().rposition(|frame| {
let mut is_rmcp = false;
let _ = resolve_symname(*frame, |symname| {
if let Some(mangled_symbol_name) = symname {
if mangled_symbol_name == "__rust_maybe_catch_panic" {
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be mangled_symbol_name.contains("rust_maybe_catch_panic") (regardless of other concerns) because the symbol can be named panic_unwind::__rust_maybe_catch_panic or _rust_maybe_catch_panic (on Windows).

is_rmcp = true;
}
}
Ok(())
}, context);
is_rmcp
}).unwrap_or(0);
let skipped_after =
frames.len() - idx_catch_panic
+ frames[skipped_before..idx_catch_panic].iter()
.rev()
.position(|frame| {
is_good_frame(*frame, BAD_PREFIXES_BOTTOM)
}).unwrap_or(frames.len() - skipped_before);
}).unwrap_or(0);

if skipped_before + skipped_after == frames.len() {
if skipped_before + skipped_after >= frames.len() {
// Avoid showing completely empty backtraces
return (0, 0);
}
Expand Down