Uh oh!
There was an error while loading. Please reload this page.
Add initial debug fmt for Backtrace - #69038
Conversation
rust-highfive
commented
Feb 10, 2020
r? @shepmaster (rust_highfive has picked a reviewer for you, use r? to override) |
yaahc
commented
Feb 10, 2020
I'm starting with this most basic version of a Debug format that just derives everything and doesn't force any symbol resolution as a starting point for the rest of the PR, I still haven't tested how this looks in practice because compile times but I'm working on getting a unit test that I can use as a playground to render the format pre and post resolution so I can paste that into this issue for review. |
yaahc
commented
Feb 10, 2020
This is the current format from this PR, lmk what further changes are necessary. As generated by this small snippet #[cfg(test)]mod tests {usesuper::*;#[test]fndebug_backtrace_fmt(){let bt = Backtrace::capture();eprintln!("uncaptured: {:?}", bt);let bt = Backtrace::force_capture();eprintln!("captured: {:?}", bt);eprintln!("display print: {}", bt);eprintln!("resolved: {:?}", bt);unimplemented!();}} |
dtolnay
commented
Feb 10, 2020
r? @dtolnay |
dtolnay
left a comment
There was a problem hiding this comment.
That representation looks extremely verbose to me. I expect that debug-printed backtraces are going to be a thing that humans see somewhat often (unfortunately) from unwrapped or debug printed error objects containing a backtrace. Could you see if you can strip out everything that wouldn't be informative to the reader?
Before:
Backtrace{inner:Captured(Mutex{data:Capture{actual_start:2,resolved:true,frames:[BacktraceFrame{frame:Frame{ip:0x55c1fc866e5d,symbol_address:0x55c1fc866de0,},symbols:[BacktraceSymbol{name:Some(
std::backtrace::Backtrace::create,),filename:None,lineno:None,},],},BacktraceFrame{frame:Frame{ip:0x55c1fc866dd0,symbol_address:0x55c1fc866dc0,},symbols:[BacktraceSymbol{name:Some(
std::backtrace::Backtrace::force_capture,),filename:None,lineno:None,},],},
...
...
...],},},),}More reasonable:
Backtrace["std::backtrace::Backtrace::create","std::backtrace::Backtrace::force_capture","core::ops::function::FnOnce::call_once @ /rustc/src/libcore/ops/function.rs:231",]or list-of-maps style as suggested in #65280:
Backtrace[{fn:"std::backtrace::Backtrace::create"},{fn:"std::backtrace::Backtrace::force_capture"},{fn:"core::ops::function::FnOnce::call_once", file:"/rustc/src/libcore/ops/function.rs", line:231},]Sure, but I'm not sure how to handle the fact that a backtrace frame can have multiple symbols. Also, I'd personally prefer the to use How do you feel about this format |
dtolnay
commented
Feb 10, 2020
|
rust-highfive
commented
Feb 10, 2020
The job 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 |
yaahc
commented
Feb 10, 2020
The logic within |
Okay, independent of my previous comment, here is the current format @dtolnay with the changes you've requested. Fixing the |
Current version, this adds some |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
rust-highfive
commented
Feb 11, 2020
The job 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 |
yaahc
commented
Feb 11, 2020
Okay here is the current version I manually added |
Uh oh!
There was an error while loading. Please reload this page.
yaahc
commented
Feb 11, 2020
and here we go again, for possibly the last time she says with great hubris |
Sigh... Why are these lines being skipped in the display impl ._. investigates Edit: its because of |
dtolnay
commented
Feb 11, 2020
Nice! Almost there I think. Thanks for sticking with this.
|
rust-highfive
commented
Feb 11, 2020
The job 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 believe the display impl prints the hashes for Here's the comparison between the alt and default display impls
They feel equally easy to read to me, so I have no preference, if you want my vote I'm gonna vote for no Edit: One thing I think might be important is how easily the debug impl can be parsed, so if you think that adding |
yaahc
commented
Feb 11, 2020
Oh I just noticed the resolved / non-opt debug print for Backtrace had hashes, thats definitely wrong, fixing. |
yaahc
commented
Feb 11, 2020
And now its backwards, because apparently they switched the meanings of alt where they use the alt print for PrintFmt::Short which is selected when alt is false 🙃 Gonna just make an executive decision here and not try to maintain exact compat with old behavior. |
yaahc
commented
Feb 11, 2020
Final-final-v2-format.jpg |
dtolnay
left a comment
There was a problem hiding this comment.
This is great. Looks good to me without the deliberately failing test. I don't know if there is a way to instantiate a hardcoded backtrace for testing purposes, but I wouldn't worry about it if it turns out to be not easy.
rust-highfive
commented
Feb 11, 2020
The job 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 |
dtolnay
commented
Feb 11, 2020
@bors r+ |
bors
commented
Feb 11, 2020
📌 Commit ec8ee7f has been approved by |
Add initial debug fmt for Backtrace Fixes the first point in rust-lang#65280 related to rust-lang#53487
Rollup of 7 pull requests Successful merges: - #67954 (Support new LLVM pass manager) - #68981 ( Account for type params on method without parentheses) - #69002 (miri: improve and simplify overflow detection) - #69038 (Add initial debug fmt for Backtrace) - #69040 (Cleanup SGX entry code) - #69086 (Update compiler-builtins to 0.1.25) - #69095 (Minified theme check) Failed merges: r? @ghost
Fixes the first point in #65280
related to #53487