Add support for x packet#186
Conversation
| __delegate!(fn guard_rail_implicit_sw_breakpoints(&self) -> bool); | ||
|
|
||
| __delegate!(fn use_no_ack_mode(&self) -> bool); | ||
| __delegate!(fn use_x_lowcase_packet(&self) -> bool); |
There was a problem hiding this comment.
I am not sure about this here, though
There was a problem hiding this comment.
This is def right, and in-fact, you've just made me realize that we haven't been correctly delegating some of the new methods that were added here! I'll need to send out a follow-up bug fix to address that, lol.
There was a problem hiding this comment.
I believe you weren't able to get GDB to use the x packet because you didn't add the necessary qSupported feature negotiation logic. As per the docs - the stub needs to report binary-upload is supported, validate that the client's supported features includes binary-upload, and if the two line up - hook that into gdbstub's protocol feature logic to enable the codepath.
Also, since this is a (relatively?) new packet, you'll want to make sure you're running a (relative?) recent GDB version that supports it.
Of course, if the user has explicitly disable use_x_lowcase_packet in gdbstub, gdbstub should avoid advertising support for binary-upload in qSupported.
Once you do that, re-run validation, and you should hopefully see GDB using the x packet correctly.
| __delegate!(fn guard_rail_implicit_sw_breakpoints(&self) -> bool); | ||
|
|
||
| __delegate!(fn use_no_ack_mode(&self) -> bool); | ||
| __delegate!(fn use_x_lowcase_packet(&self) -> bool); |
There was a problem hiding this comment.
This is def right, and in-fact, you've just made me realize that we haven't been correctly delegating some of the new methods that were added here! I'll need to send out a follow-up bug fix to address that, lol.
dblnz
force-pushed
the
feat/add-x-packet-support
branch
from
6303b47 to
2fb8272
Compare
|
I managed to get this working and I updated the PR and description with the output from a debugging session using |
daniel5151
left a comment
daniel5151
left a comment
There was a problem hiding this comment.
Looks like there's a little bug lurking in the implementation (that shouldn't be hard to fix).
Aside from that, the implementation is looking good to me!
Validation is also looking good, though could you please also include
- The output from the GDB (or LLDB) session that you used to validate the implementation (so I can confirm both the client and server-side are working properly)
- particularly useful in this case, given that it might've helped spot the bug in the current impl :)
- The output of the
./example_no_std/check_size.shscript
| crate::__dead_code_marker!("x_lowcase_packet", "impl"); | ||
|
|
||
| let handler_status = match command { | ||
| XLowcasePacket::x(cmd) => { |
There was a problem hiding this comment.
Note to self: we should be able to share the implementation of this method with the m handler in handle_base, and avoid copy-pasting.
I'll look into this myself after this diff lands, since I want to make sure however we do it, it doesn't result in poor codegen on minimal stubs.
I'm curious what to bug you are referring to. Something to do with the binary output? I had some issues with copy/pasting from my terminal (my tmux and windows terminal don't play well together 😄 ). I'll see how I get that fix before pasting here again |
I'm talking about the extra space. Should be "b", not "b " |
Oh, I thought it was something else. Thanks! I'll get it fixed |
dblnz
force-pushed
the
feat/add-x-packet-support
branch
from
2fb8272 to
07e340e
Compare
|
I updated the description with the output I got from LLDB and |
- Add `gdbstub` support for `x` packet that responds with `b` prefixed packed to indicate a binary format - This is by default disabled, implement the `use_x_lowcase_packet` method of `Target` to enable it - When this is enabled, `gdbstub` responds with `binary-upload+` to `qSupported` packet, to indicate support of this command. Signed-off-by: Doru Blânzeanu <dblnz@pm.me>
dblnz
force-pushed
the
feat/add-x-packet-support
branch
from
07e340e to
ae0cc48
Compare
daniel5151
left a comment
daniel5151
left a comment
There was a problem hiding this comment.
Nice, this is looking great!
Thanks for testing and polishing this up.
I'll try to find a bit of time this weekend-ish to push another revision or two to this PR myself (tightening up the docs, maybe rewriting the packet handler code so it can be shared with the m packet), and once that's done - we'll go ahead and merge this in.
Thank you again for your contribution!
2 participants
Description
This PR implements the
xpacket, based off the GDB documentation here.Closes #163
lldbrun a singlexcommand with addr 0 and len 0. I'll give it another try but I am not familiar with LLDB that much.xby defaultAPI 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
LLDB output
armv4t output
Before/After `./example_no_std/check_size.sh` output
Before
After