Tracepoint extension support #160
Conversation
|
I wasn't sure how the zero panic verification guarantee can be checked - there isn't a how to section in the README about it. I tried to write code as panic-free as I could, but I may have missed some pieces. |
|
Thanks for sending in this PR - I'm excited to dig in here! Unfortunately, I'm just about to leave for a vacation where I'll be completely AFK, so I'll only be able to take a look sometime in the range of ~Dec 20th to ~Dec 23rd. Just wanted to give you a heads up, so that you're not worried about the lack of movement here 😅 |
There was a problem hiding this comment.
Hello again, and happy holidays!
I've finally found some time to sit down and give this PR a review.
Please find a plethora of comments attached.
Broadly speaking, I think this is an impressive chunk of work, implementing what appears to be a very annoying and non-trivial part of the GDB RSP. From an organizational and syntactical POV, there's nothing that a few review comments can't polish up, and the overall vision here appears to be consistent and well put together. Kudos!
That said, I do have some concerns about the amount of API surface area we're taking on here, and the feasibility of testing all of it. Its great that the you've got some things working in the armv4t example, but from looking at the code (notably: obvious errors such as handlers which send responses with spaces delimiting various part of the packet), it seems that you've got a lot of code here that theoretically works, but hasn't been directly validated.
That's not to say we shouldn't try to land this work!
I think we should certainly try to get this PR landed... but to temper expectations for end-users, I might suggest we land this code under a mod experimental, with some documentation mentioning that this code covers a lot of surface area, and may not be fully tested.
Alternatively, if you're so inclined, I'd be happy to see more investment into the armv4t example code, with some corresponding logs that show all these codepaths having been smoke-tested. Or, of course, logs from whatever project you're implementing this feature for (and ideally, a link to the implementation itself - assuming you're working on something open-source).
I wasn't sure how the zero panic verification guarantee can be checked - there isn't a how to section in the README about it. I tried to write code as panic-free as I could, but I may have missed some pieces.
CI has a check for this, but it seems that the check doesn't run if clippy fails. Oops.
I should probably re-jig CI a bit so that clippy failing still gives no_panic feedback... my apologies.
src/protocol/commands/_QTBuffer.rs
Outdated
| // Our response has to be a hex encoded buffer that fits within | ||
| // our packet size, which means we actually have half as much space | ||
| // as our slice would indicate. |
There was a problem hiding this comment.
so, interestingly enough, this is not the approach gdbstub has taken thus-far when it comes to responses.
While we certainly have a fixed buffer for incoming packets (i.e: the buffer we are parsing from here), we assume that the GDB client is capable of accepting any amount of data we stream out as part of our responses. This aligns with my particular reading of the GDB spec, which discusses the size of packets the stub can accept... but doesn't make a judgement on the size of response packets the client can receive.
as such - I would suggest skipping this buffer-slicing step entirely, and instead offering the handler a callback function they can write an arbirarily sized &[u8] into, which gdbstub can then simply stream out. If you poke around some of the other target APIs, you'll see examples of this callback-based / streaming-based pattern being employed to great effect.
There was a problem hiding this comment.
alternatively, if you think that from a purely ergonomic POV, its nicer to provide end-users with a buffer to write data into... lets make sure to pass along the entire buffer we have access to, and let downstream response-writer code stream out the corresponding hex bytes.
There was a problem hiding this comment.
The tracepoint packet documentation specifically calling out
The reply consists as many hex-encoded bytes as the target can deliver in a packet
makes me think that limiting our response to one packet is important and we can't just stream. Some other places like e.g.
Any number of actions may be packed together in a single ‘QTDP’ packet, as long as the packet does not exceed the maximum packet length (400 bytes, for many stubs).
I could see it not being important for, however, if we're the stub and because the QTDP packets are bidirectional...but QTBuffer responses are only ever being returned by the stub.
The packet buffer size in general is kind of confusing to me. Like you said, in most other places gdbstub just ignores the concept of a packet size and assumes it can stream data and it works fine, but then we have documentation like these occasionally...?
There was a problem hiding this comment.
I think its fair to say that both the GDB RSP and gdbstub aren't always consistent about how these sorts of variable-length packets are implemented...
There are many instances where gdbstub has opted to recycle the trailing packet buffer in order to provide the Target a &mut [u8] it can write data into... while other times, it has opted to hand a callback-based object to the Target for it to write data into. But even in the former case, as I alluded to earlier / elsewhere, the data written in that buffer is nonetheless streamed out via a sequence of what are essentially putc calls, without any bound on output packet size.
As I mull over this further, I'm realizing that I should really spin up a tracking issue to document and discuss potential solutions for these sorts of project-wide inconsistencies. It may even tie into #159, and possibly extend #88, in the sense that gdbstub may need to care more about dealing with backpressure on outbound write operations...
But in any case, for this PR and packet specifically - I'm not sure I'll push hard in either direction. Feel free to do what you think is best, and I'll add this API to the growing list of APIs that aught to be more consistent.
|
Thanks for the feedback! I'll look over and resolve them. For some background, we're using gdbstub in order to build out debugging introspection for a project we have. Our current tooling is via Python, and we're using gdbstub via some PyO3 bindings I hacked together (which I do intend to open source eventually, once I find the time), but that also isn't very "interesting" code. I'm working on this tracepoint support in tandem with building out the rest of the debugging stack, and so the API surface here was the minimal amount I needed in order to get gdb to not error out with "not supported" and implement the tracepoint functionality. We do have functionality using this, however, so none of it should be untested code (although some parts, like |
cczetier
force-pushed
the
tracepoint_public
branch
from
93a789d to
4e7dcca
Compare
|
I added additional error handling to |
|
Hey! Just wanted to drop in and mention that I see all the commits flying here, and that I'll definitely try to find some time soon to look into them, hopefully at some point in the next couple days before the weekend. Things have gotten unexpectedly busy on my end now that the new year is back in full swing... but I'll try to give timely feedback here to keep the ball rolling. Thanks for all the work an iteration you're doing here! |
|
Hey, really sorry for the lack of activity here. A ton of personal and work stuff has come up over the past few weeks, and I've yet to find a moment of focus time to give feedback here... My apologies :( I'm going to try and set aside some time this weekend to give this a review. |
There was a problem hiding this comment.
I've spent some time looking at this PR, as well as the "Tracepoint Packets" part of the GDB RSP spec, and I think I've got a good idea / guess behind the how-and-why of the Create/Define/Query packet spec.
My take on the matter is that the GDB RSP didn't want to have a single "mega-packet" to define tracepoints, as it would've more likely than not blown past the packet limit set by various stubs. Instead, it opts to "drip-feed" the full context behind a particular tracepoint over the span of several QTDP packets, relying on the stub to gradually build up a full picture of the tracepoint its expected to create.
This is backed up by the careful wording in the QTDP:- packet:
This packet may only be sent immediately after another ‘QTDP’ packet that ended with a ‘-’
...implying that what we're really dealing with here is one big composite "create a new tracepoint" packet, as opposed to two distinct "create" and "define" tracepoint packets!
As such, I'm not convinced tracepoint_create and tracepoint_define expose the right level of abstraction we want in the API. Instead - I think we want to have methods like tracepoint_create_begin, tracepoint_create_continue, and then one final (synthetic) tracepoint_create_complete, and then later, tracepoint_enumerate_start and tracepoint_enumerate_step should operate on NewTracepoint and DefineTracepoint, instead of this "synthetic" TracepointItem type (which should go away).
Fundamentally, I don't believe we want end-user implementations to directly store the bundles of info contained within NewTracepoint and DefineTracepoint. i.e: storing a Vec<TracepointItem> directly, and then operating on that representation in other parts of the tracepoint API, seems like the wrong approach for end-users.
Instead, I believe the API methods / types (and in turn, the arvm4t example that shows off how they are used) should reflect the true intent of this API: that user-defined stubs are responsible for converting between the drip-fed "wire protocol" data representation of tracepoints (as encapsulated by the NewTracepoint and DefineTracepoint types), into a stub-specific Tracepoint type.
Similarly, when implementing tracepoint_enumerate_start and tracepoint_enumerate_step, users shouldn't just report some long-lived stashed reprs of NewTracepoint and DefineTracepoint - they should instead create them "on the fly" using whatever tracepoint representation it "committed" to as part of tracepoint_create_complete.
The key ethos behind gdbstub is that we aren't just in the business of munging the raw GDB RSP and presenting it to the end user. Rather - we interpret the intent behind the operations the GDB RSP is modeling, and present a new, sane API on-top of the base protocol semantics, making it easy for end users to interface with GDB without having to deal with much of the underlying "insanity".
Does this reasoning make sense?
That's not a rhetorical question or anything - I'd like to get your honest thoughts on the matter here. I'm just making an educated assumption based on my experience with the rest of the GDB RSP, but you're the one who's actually using this in a real project.
I didn't finish reviewing the whole PR, but given the cascading refactors that might fall out of this observation, it might make sense for me to hold off until we iterate some more here.
Apologies again for the delay in getting this feedback, and for not pointing this out sooner. I wish I could've found some time earlier to sit down and page in all the context here, but its been quite a busy January...
Let me know what you think, and where we want to go from here.
I think this is in reality a fairly small difference in perspectives. In our project we aren't storing a You're right in that
Switching to the alternative API surface doesn't get rid of |
|
Note that the All that is to say - we can (and should!) cut corners on the Regarding the create path - you're correct that the necessary code-level tweaks aren't that significant (inches, not miles). The key thing I want to refine here is the clarity of the resulting API, both via its documentation, but also via method naming, and the types that are used in the API. On the enumerate path, indeed, I think its pretty fair to say that the specific thing concerning me about the current API is the But what if there was a way? Here's the idea I just had:
I believe this is a reasonable approach that avoids having The reason I think this approach can work is that What do you think? |
|
I implemented the target provided state machine code, which has the benefit of the target not needing to keep the state for enumeration at all, although the target code is a bit more gross for calculating the Unfortunately, trying to enumerate uploaded tracepoints with any actions (including just I added QTDPsrc support and enumerating those source items as part of the refactor. This complicates the state machine logic a bit, but is still workable. I need to do some more work gating these bits behind a The documentation for the entire enumeration machinery is kind of unwieldy, but I'm also not entirely sure how to make it better... |
There was a problem hiding this comment.
I really like this latest iteration! I think we're getting super close to landing this PR. Thanks again for all the iterations and back and forth here - I really appreciate it.
I've got a few more questions / comments about the current API... but the broad shape and approach taken here looks to be quite solid.
| // Report our tracepoint | ||
| (f)(&self.tracepoints[&tp].0); | ||
|
|
||
| let ret = if !self.tracepoints[&tp].1.is_empty() { |
There was a problem hiding this comment.
consider the following simplification to the enumeration API:
what if, instead of having each of enumerate_{start,action,source} use the exact same TracepointEnumerateStep (which requires target authors to carefully avoid accidentally "looping" between enumerating actions/sources), we instead simplified the API such that each method could only transition to the "next" category.
i.e: instead of the following state machine:
start -> source
start -> action
start -> done
source -> source
source -> action
source -> done
action -> source
action -> action
action -> done
we instead had
start -> source
source -> source
source -> action
action -> action
action -> done
...which guarantees forward-progress through the various states, and simplifies the target implementation substantially (no more multi-armed if statements).
the downside, of course, is that each tracepoint will always result in each of the 3 enumerate methods being invoked on it, without any ability to "short circuit".
I'll let you tell me if you think this sort of inneficiency will result in tangible slowdowns in the real world, but if the overhead is likely to be minimal... I'd err on the side of having an API that's less likely to be misused.
There was a problem hiding this comment.
The current API is a bit misuse prone, in that it's possible to trap yourself in a loop around action/source enumeration, but I think any other API would be equally misuse prone in other ways:; you can always trap yourself in a loop over your tracepoints, for example, and if you had types of TracepointEnumerateStep { More, MoveTo(Tracepoint), Done } where More means you continue pumping the current type of item and Done means move to the next type, you still need to correctly implement Done vs. MoveTo logic or else you could MoveTo from actions and skip all your source items.
One approach would be if you have struct Start; struct Actions; struct Source; struct Done(Option<MoveTo>); and TracepointEnumerateStep<Current, Next> so that you can enforce that enumerate_source is the only transition that can MoveTo a new tracepoint or stop enumeration via parameterization, but that runs into issues around moving source enumeration to a separate IDET: we'd always need to have the enumerate_source function available and implemented, since we can't conditionally have the Next type parameter be either Source or Done depending on if the trait is implement or not (or, well, maybe with an associated generic on the trait, but that's gross).
I'm not worry about slowdown, tracepoint enumeration is fairly rare and only(?) happens on attach: on program we're using target extended-remote mode for multiple PIDs and so need the download functionality to install tracepoints in multiple PIDs at the same time, but switching between them and triggering the tracepoint download happens interactively for the user and isn't performance critical (at least for our usecase!).
There was a problem hiding this comment.
Also, to be clear, the current API is like:
start -> source
source -> source
source -> action
action -> action
action -> next(1)
start -> source
source -> action
action -> action
action -> next(2)
start -> source
source -> action
action -> done
You don't return done multiple times, it signals that you have no more information at all, and it pumps each information type in a row before moving to the next type
There was a problem hiding this comment.
Fair enough! Happy to leave it here, and maybe noodle around with it myself whenever I finally find the time to start tackling the big 0.8 backlog
|
There was an issue where we were returning an |
There was a problem hiding this comment.
I'm happy with this implementation! It was a long road to get here, and I really appreciate you sticking with me here - I think we landed on something quite solid here, especially given all the wackiness in the GDB RSP, as well as some of the architectural limitations currently in gdbstub. 🎉
The only things left are:
- A quick polish-pass through the docs (see my comments)
- Making sure the PR description is up-to-date. i.e: re-running the various manual validations for dead-code-elimination, updating the
armv4ttrace logs, etc... - Making
rustfmtandclippyhappy :)
I'm super excited to get this in!
|
It's great this is getting close to being merged! I'm going to be working on another project for a while, and so if there are more final comments I may be slow to respond - I'm still interested in working to get this merged, however. |
There was a problem hiding this comment.
Latest round of edits is looking good! Not much feedback here, aide from that one comment wrt. making a failure mode a loud error, instead of silently accepting it.
As a reminder, you'll still need to:
- Make sure the PR description is up-to-date. i.e: re-running the various manual validations for dead-code-elimination, updating the
armv4ttrace logs, etc... - Make
rustfmtandclippyhappy, so the PR checks are all green
| .handle_error()?, | ||
| ) | ||
| } else { | ||
| // If the target doesn't support tracepoint sources but told |
There was a problem hiding this comment.
This feels like it should result in a Error, no? i.e: the only way this code path could be hit is due to a faulty Target implementation?
If so, lets upgrade this to a loud error that notifies the user of their mistake
There was a problem hiding this comment.
Added new error variant, and now throwing it here.
I fixed up the clippy errors (or, well, suppressed them, which is almost the same thing...), along with updated the OP command with up-to-date logs. |
|
Have you been running |
|
I am running clippy locally - I'm on |
|
Ah, that's do it.
|
|
Running an up-to-date |
cczetier
force-pushed
the
tracepoint_public
branch
from
03698fc to
fc191a2
Compare
|
Yup, that was it. This gives me a clean |
|
...And now also gives a clean |
|
CI is all green! Thanks again for seeing this though! I know we've still got a way to go until I'll go ahead and merge this PR, and then cut a shiny new 0.7.4 release ASAP. |
|
Release 0.7.4 is out 🎉 |
Description
This PR adds basic tracepoint extension support to GDB stub. Closes #157.
API Stability
Checklist
rustdocformatting looks good (viacargo doc)examples/armv4twithRUST_LOG=trace+ any relevant GDB output under the "Validation" section below./example_no_std/check_size.shbefore/after changes under the "Validation" section belowexamples/armv4t./example_no_std/check_size.sh)ArchimplementationValidation
GDB output
loading section ".text" into memory from [0x55550000..0x55550078] Setting PC to 0x55550000 Waiting for a GDB connection on "127.0.0.1:9001"...
(The start of the
cargo runoutput is corrupted by binary data that's printed, so I cut out the portion of the output relevant for tracepoint packets)Before/After `./example_no_std/check_size.sh` output
Before
After