Skip to content

Filter backtrace #48702

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

Closed
wants to merge 11 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
3 changes: 1 addition & 2 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ __pycache__/
/src/libstd_unicode/SpecialCasing.txt
/src/libstd_unicode/UnicodeData.txt
/stage[0-9]+/
/target
target/
Copy link
Member

Choose a reason for hiding this comment

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

Ah unfortunately this has the unintended side effect of ignoring src/librustc_back/target, so perhaps these changes could be left out for now?

/test/
/tmp/
TAGS
Expand All @@ -101,7 +101,6 @@ version.ml
version.texi
.cargo
!src/vendor/**
/src/target/

no_llvm_build

91 changes: 67 additions & 24 deletions src/libstd/sys_common/backtrace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,16 +69,18 @@ fn _print(w: &mut Write, format: PrintFormat) -> io::Result<()> {
inline_context: 0,
}; MAX_NB_FRAMES];
let (nb_frames, context) = unwind_backtrace(&mut frames)?;
let (skipped_before, skipped_after) =
filter_frames(&frames[..nb_frames], format, &context);
if skipped_before + skipped_after > 0 {
let filtered_frames = filter_frames(&frames[..nb_frames], &context, format);
if filtered_frames.len() != nb_frames {
writeln!(w, "note: Some details are omitted, \
run with `RUST_BACKTRACE=full` for a verbose backtrace.")?;
}
writeln!(w, "stack backtrace:")?;

let filtered_frames = &frames[..nb_frames - skipped_after];
for (index, frame) in filtered_frames.iter().skip(skipped_before).enumerate() {
for (index, frame, is_on_filter_edge) in filtered_frames {
// Don't use ANSI escape codes on windows, because most terminals on it don't support them
if is_on_filter_edge && cfg!(not(windows)) {
write!(w, "\x1B[2m")?;
Copy link
Member

Choose a reason for hiding this comment

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

Dealing with ansi codes can be sort of a minefield unfortunately. This isn't handling terminals on Unix, for example, that don't support ansi codes. It also doesn't handle cases on Windows where ansi codes are supported (like MSYS and newer Windows terminals I think).

I'm not entirely certain what this ansi code is doing, but could it be left out for now?

Copy link
Member Author

Choose a reason for hiding this comment

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

This ansi code makes the line darker, as it is less important.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, in that case can this be omitted for now? I'm not sure that libstd is really prepared to accurately place ANSI codes everywhere

}
resolve_symname(*frame, |symname| {
output(w, index, *frame, symname, format)
}, &context)?;
Expand All @@ -88,43 +90,84 @@ fn _print(w: &mut Write, format: PrintFormat) -> io::Result<()> {
if has_more_filenames {
w.write_all(b" <... and possibly more>")?;
}
if cfg!(not(windows)) {
write!(w, "\x1B[0m")?;
}
}

Ok(())
}

/// Returns a number of frames to remove at the beginning and at the end of the
/// backtrace, according to the backtrace format.
fn filter_frames(frames: &[Frame],
format: PrintFormat,
context: &BacktraceContext) -> (usize, usize)
{
if format == PrintFormat::Full {
return (0, 0);
}
fn should_show_frame(frame: &Frame, context: &BacktraceContext) -> bool {
const FILTERED_SYMBOLS: &[&str] = &[
"main",
"rust_begin_unwind",
];
const FILTERED_SYMBOL_PARTS: &[&str] = &[
"panic",
"sys",
"lang_start",
];
let mut should_show = true;
let _ = resolve_symname(*frame, |symname| {
if let Some(mangled_symbol_name) = symname {
for filtered_symbol in FILTERED_SYMBOLS {
if mangled_symbol_name == *filtered_symbol {
should_show = false;
return Ok(());
}
}
for filtered_symbol_part in FILTERED_SYMBOL_PARTS {
if mangled_symbol_name.contains(filtered_symbol_part) {
should_show = false;
return Ok(());
}
}
}
Ok(())
}, context);
should_show
}

let skipped_before = 0;
/// Returns the frames to show as a Vec.
/// If the bool is true the frame is on the edge between showing and not showing.
fn filter_frames<'a>(frames: &'a [Frame],
context: &BacktraceContext,
format: PrintFormat) -> Vec<(usize, &'a Frame, bool)>
Copy link
Member

Choose a reason for hiding this comment

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

Backtraces are currently generated in some sensitive contexts that often work best if we avoid allocation as much as possible, so could a callback or a different scheme be used instead of a vector?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but it is harder, due to having to know if the previous or next frame have to be filtered out.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I think this'll be a requirement for landing though

{
let mut frames_iter = frames.iter().enumerate().peekable();
let mut filtered_frames = Vec::new();
let mut show_prev_frame = false;

let skipped_after = frames.len() - frames.iter().position(|frame| {
let mut is_marker = false;
while let Some((i, frame)) = frames_iter.next() {
let mut is_after_begin_short_backtrace = 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;
is_after_begin_short_backtrace = true;
}
}
Ok(())
}, context);
is_marker
}).unwrap_or(frames.len());
if is_after_begin_short_backtrace && format != PrintFormat::Full {
break;
}

if skipped_before + skipped_after >= frames.len() {
// Avoid showing completely empty backtraces
return (0, 0);
let show_cur_frame = should_show_frame(frame, context);
let show_next_frame = frames_iter
.peek()
.map(|&(_, frame)| should_show_frame(frame, context))
.unwrap_or(false);
if show_cur_frame {
filtered_frames.push((i, frame, false));
} else if show_prev_frame || show_next_frame || format == PrintFormat::Full {
filtered_frames.push((i, frame, true));
}
show_prev_frame = show_cur_frame;
}

(skipped_before, skipped_after)
filtered_frames
}


Expand Down
21 changes: 21 additions & 0 deletions src/test/run-make/filter_backtrace/Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
-include ../tools.mk

# Test that short backtraces don't include several internal symbols

all:
$(RUSTC) main.rs -o $(TMPDIR)/main
# with short backtrace
RUST_BACKTRACE=1 $(TMPDIR)/main 2> $(TMPDIR)/short_bt.stderr || exit 0 && exit 1
$(CGREP) "panicked at" < $(TMPDIR)/short_bt.stderr
$(CGREP) -v "std::panicking" < $(TMPDIR)/short_bt.stderr
$(CGREP) -v "std::sys" < $(TMPDIR)/short_bt.stderr
$(CGREP) -v "__rust_maybe_catch_panic" < $(TMPDIR)/short_bt.stderr
$(CGREP) -v "__rust_begin_short_backtrace" < $(TMPDIR)/short_bt.stderr
# with long backtrace
RUST_BACKTRACE=full $(TMPDIR)/main 2> $(TMPDIR)/long_bt.stderr || exit 0 && exit 1
$(CGREP) "panicked at" < $(TMPDIR)/long_bt.stderr
$(CGREP) "std::panicking" < $(TMPDIR)/long_bt.stderr
$(CGREP) "std::sys" < $(TMPDIR)/long_bt.stderr
$(CGREP) "__rust_maybe_catch_panic" < $(TMPDIR)/long_bt.stderr
# FIXME: prevent tail call optimization for this
$(CGREP) -v "__rust_begin_short_backtrace" < $(TMPDIR)/long_bt.stderr
3 changes: 3 additions & 0 deletions src/test/run-make/filter_backtrace/main.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
fn main() {
(None as Option<()>).unwrap();
}