refactor "print backtrace" code #197
Conversation
this commit refactors the code that prints the backtrace to be comprised of 3 smaller steps - `unwind::target`, talks to the probe and unwinds the target's program retrieving only a series of PC (program counters). This is fallible due to potential IO errors - `symbolicate::frames`, turns those raw PC values into function names and locations; it also expands inlined functions. This is infallible. When there's no debuginfo the PC address is used as the function "name". - `pp::backtrace`, pretty prints the processed frames preserving the current format. this is infallible API-wise (`println!` may still panic however) there's more work that could have been done but has intentionally not been included in this commit, like making `unwind::target` and/or `symbolicate::frames` lazy to only do as much work as required by the `--max_backtrace_len` flag. this commit also fixes issue 196 by making `--max_backtrace_len=1` print the right number of frames. The behavior of `--max_backtrace_len=0` has been changed to *never* print a backtrace (`--force-backtrace --max-backtrace-len=0` does *not* print a backtrace) this commit also (non-exhaustively) groups Cortex-M constants and operations in a module to make them easier to spot (in case we ever add support for other architectures like RISC V). a qualified path, instead of an import, is used to access the items in that module. we don't have automated tests for the backtrace but manual testing indicates that behavior is preserved. the warning message about the stack being corrupted now has less detail however. fixes #196
src/backtrace.rs
Outdated
| live_functions: &HashSet<&str>, | ||
| current_dir: &Path, | ||
| force_backtrace: bool, | ||
| max_backtrace_len: u32, |
There was a problem hiding this comment.
I think I agree with Clippy here, it would be nice to group some of these arguments in a struct (maybe have one for backtrace options, one for stuff extracted from the ELF)
There was a problem hiding this comment.
I agree. For now I have grouped the backtrace settings into a struct. That removes 2 arguments and barely passes the clippy lint.
I'll leave the refactor of grouping ELF stuff into its own struct as follow-up work. In my mind, there should a (fallible?) operation that goes from object::File into our::ProcessedElf which is then passed by reference everywhere. That op should be done early in the program to detect issues like missing debug info early on.
|
bors r+ |
| } | ||
|
|
||
| /// (virtually) unwinds the target's program and prints its backtrace | ||
| pub(crate) fn print( |
There was a problem hiding this comment.
I know I am late to the party, but why are you using pub(crate) although it is a application and not a library?
this commit refactors the code that prints the backtrace to be comprised of 3 smaller steps
unwind::target, talks to the probe and unwinds the target's program retrieving only a series of PC (program counters). This is fallible due to potential IO errorssymbolicate::frames, turns those raw PC values into function names and locations; it also expands inlined functions. This is infallible. When there's no debuginfo the PC address is used as the function "name".pp::backtrace, pretty prints the processed frames preserving the current format. this is infallible API-wise (println!may still panic however)there's more work that could have been done but has intentionally not been included in this commit, like making
unwind::targetand/orsymbolicate::frameslazy to only do as much work as required by the--max_backtrace_lenflag.this commit also fixes issue 196 by making
--max_backtrace_len=1print the right number of frames. The behavior of--max_backtrace_len=0has been changed to never print a backtrace (--force-backtrace --max-backtrace-len=0does not print a backtrace)this commit also (non-exhaustively) groups Cortex-M constants and operations in a module to make them easier to spot (in case we ever add support for other architectures like RISC V). a qualified path, instead of an import, is used to access the items in that module.
we don't have automated tests for the backtrace but manual testing indicates that behavior is preserved. the warning message about the stack being corrupted now has less detail however.
fixes #196