Filter backtrace #48702
Filter backtrace #48702
Conversation
|
r? @BurntSushi (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
ping @BurntSushi |
There was a problem hiding this comment.
Thanks for the PR and sorry for the delay! Dealing with filtering backtraces is a pretty tricky issue though so I'd want to be sure that we're being careful here. This all started in #38165 and some refinements were proposed in #40264 but during that discussion (and while the original PR was baking on nightly) it was found out that we were too aggressive about pruning backtraces, consequently leading to #41364 which removed almost all filtering.
There's a lot of comments on #40264 but I think the general consensus is that we're not shooting for 100% clean backtraces here but rather "mostly clean" backtraces by default. In that sense we're mostly looking to simply prune the top frames related to panic runtime gunk and the bottom frames that are related to thread start and whatnot (only showing user-written Rust code in the middle in theory).
To that end I think the logic here is a bit too aggressive, filtering out frames in the middle by accident possibly. Additionally searching for strings like panic or sys is pretty general, so I think we'd want to refine those as well.
Can you also gist some before/after of what the backtraces look like?
.gitignore
Outdated
| @@ -82,7 +82,7 @@ __pycache__/ | |||
| /src/libstd_unicode/SpecialCasing.txt | |||
| /src/libstd_unicode/UnicodeData.txt | |||
| /stage[0-9]+/ | |||
| /target | |||
| target/ | |||
There was a problem hiding this comment.
Ah unfortunately this has the unintended side effect of ignoring src/librustc_back/target, so perhaps these changes could be left out for now?
src/libstd/sys_common/backtrace.rs
Outdated
| 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")?; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
This ansi code makes the line darker, as it is less important.
There was a problem hiding this comment.
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
src/libstd/sys_common/backtrace.rs
Outdated
| /// 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)> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yes, but it is harder, due to having to know if the previous or next frame have to be filtered out.
There was a problem hiding this comment.
Ok, I think this'll be a requirement for landing though
I want to extend this PR, so that you could set custom filters (eg. don't show the internal details of the rustc query system).
I can make it search a prefix instead. |
|
Please ignore the branch name, I was in a different rust checkout. |
I think that's something that we probably don't want in std itself, but perhaps an external crate which configures the global panic hook? |
No problem |
|
Ping from triage @alexcrichton! The author pushed new commits, can you review them? @bjorn3, the build is failing because you didn't add the license header on the test. Can you add that? |
|
Ping from triage, @alexcrichton ! |
|
ah I was still waiting on https://github.com/rust-lang/rust/pull/48702/files#r175140985 to be addressed |
shepmaster
added
S-waiting-on-author
|
Sorry, forgot about that |
|
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
I don't like this approach, I much prefer my more robust and cleaner solution in #45637 which was rejected by the libs team for not being robust enough. |
pietroalbini
added
T-compiler
pietroalbini
removed
the
S-waiting-on-author
|
Ping from triage @alexcrichton! The author addressed your comment! |
|
@pietroalbini I have only partly addressed the comment, there is still a heap alloc. I haven't got time to fix it completely |
pietroalbini
added
S-waiting-on-author
|
Addressed the comment now. Ready for review @alexcrichton |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Ah sorry for taking awhile to get back to this @bjorn3. I had forgotten about #47824 but I do agree with @Zoxc that it's best solution for now as it invovles far less guesswork and will hopefully be more robust over time. The reopening at #49825 only covers stack frames below the original "main frame" (aka binaries and tests). I think that there's possible work to be done after that lands to filter out the panic infrastructure in libstd (aka everything after the entry point to panics). Would you be interested in taking that on? |
yes |
cc #47824