Skip to content
This repository has been archived by the owner on Jan 30, 2024. It is now read-only.

stackdump integration for better backtraces #322

Open
japaric opened this issue May 23, 2022 · 5 comments
Open

stackdump integration for better backtraces #322

japaric opened this issue May 23, 2022 · 5 comments
Labels
difficulty: medium Somewhat difficult to solve status: needs PR Issue just needs a Pull Request implementing the changes topic: backtrace type: enhancement Enhancement or feature request

Comments

@japaric
Copy link
Member

japaric commented May 23, 2022

the set of stackdump crates provide a mechanism to dump the contents of a device's memory (stackdump-capture) and analyze those stack dumps (stackdump-trace). the trace crate, in particular, can produce stack backtraces more complete than probe-run's as they include information about function arguments (see #62)

we'd like to make use of the trace crate to improve probe-run's backtraces but we'd like to avoid copying the whole stack memory from the device to the host as that's rather slow (see --measure-stack feature in early v0.3.x), which is the mechanism provided by the capture crate.

the integration work would consist of two parts

  • implementing the RegisterData and MemoryRegion traits for the probe_rs API. I suggest using a newtype over &mut probe_rs::Core, e.g. pub Stackdump<'a>(pub &'a mut probe_rs::Core). rationale: some of the probe-rs API uses the Mutex<Session> (e.g. gdb-server); a newtype over the value is not compatible with such API. as this is a reusable component it may be better for it to reside outside of probe-run as a library

  • replacing probe-runs's backtrace implementation with stackdump-trace's API. the API is generic; it needs to be instantiated with the newtype implemented in (1)

potential integration problems

  • RegisterData and MemoryRegion are infallible. a probe_rs implementation can fail due to IO errors but there's no proper way to bubble up the result so one can map the result into None or unwrap all internal Results
  • the 'static bound in stackdump_core::DeviceMemory::add_memory_region may be incompatible with the newtype over a reference; see (1)
@japaric japaric added type: enhancement Enhancement or feature request difficulty: medium Somewhat difficult to solve status: needs PR Issue just needs a Pull Request implementing the changes topic: backtrace labels May 23, 2022
@diondokter
Copy link

I made a branch with some initial changes here: https://github.com/tweedegolf/stackdump/tree/probe

@diondokter
Copy link

So, I made an implementation that works for my CLI, see the stackdump PR above.
Small problem, though, is that probe-rs doesn't support reading the FPU registers in the latest release.
I made a PR to fix that here: probe-rs/probe-rs#1122

But even if that gets merged, that won't be in an official release until probe-rs 0.13.
Theoretically this update isn't absolutely required. But if you miss the fpu registers, then most/all floats will not be able to be read.
What do you guys think?

@japaric
Copy link
Member Author

japaric commented Jun 7, 2022

@diondokter nice! re: fpu registers, I think it's fine if they are not read and stackdump-capture-probe is published with a dependency on probe-rs v0.12.x. as you mentioned unwinding does not depend on the contents of the fpu registers so whenever their contents are missing probe-run could report a ? (unknown) value for those function arguments. probe-run currently does not report the contents of any register (function arguments) in the backtrace so having the data of the general purpose registers would already be a huge improvement.

@Urhengulas
Copy link
Member

So, I made an implementation that works for my CLI, see the stackdump PR above. Small problem, though, is that probe-rs doesn't support reading the FPU registers in the latest release. I made a PR to fix that here: probe-rs/probe-rs#1122

But even if that gets merged, that won't be in an official release until probe-rs 0.13. Theoretically this update isn't absolutely required. But if you miss the fpu registers, then most/all floats will not be able to be read. What do you guys think?

That is great news! 🎉 Firstly, I agree with Jorge, that having non-float arguments being reported is already a good addition which is worth integrating. Secondly, speaking from past experience, it might take some time until the next probe-rs version releases, so I think we can move on without float support and add that as soon as the next probe-rs gets released.

@diondokter
Copy link

diondokter commented Jun 28, 2022

I have released stackdump-capture-probe: https://crates.io/crates/stackdump-capture-probe
This is the adaptor that probe-run should be able to use.

I've got my own CLI as well and this is the usage: https://github.com/tweedegolf/stackdump/blob/master/cli/src/probe.rs

Printing is done like here: https://github.com/tweedegolf/stackdump/blob/master/cli/src/main.rs#L123-L163
This has some line wrapping logic because some data can be annoyingly long.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
difficulty: medium Somewhat difficult to solve status: needs PR Issue just needs a Pull Request implementing the changes topic: backtrace type: enhancement Enhancement or feature request
Projects
None yet
Development

No branches or pull requests

3 participants