Implement X packet for gdbstub.

[gz]

Description

Closes #81

API Stability

• This PR does not require a breaking API change

Checklist

• Implementation
  • cargo build compiles without errors or warnings
  • cargo clippy runs without errors or warnings
  • cargo fmt was run
  • All tests pass

[daniel5151]

Well shoot, that was fast 😅

It's 11:00pm here in seattle, and while I was going to head off to bed, I guess I'll just leave this comment right now...

---

Yep, that's pretty much the code I expected. Nice!

My immediate follow-up thought is whether or not this "optimization" packet should be part of the "required" base packets, or if there ought to be some way to "opt-in" to it (or more likely, "opt-out" of it).

After all, this current implementation will unconditionally add quite a bit of extra binary bloat to the base no_std gdbstub configuration (thanks to the new packet parsing + binary buffer decoding code), which you can confirm yourself by running the ./check_size.sh script from the example_no_std directory.

Instead... I think it might make sense to do something a bit more clever, whereby we recycle some of the IDET infrastructure to make X an optional on-by-default optimization. I'm envisioning something like:

trait Target {
    // ...

    #[inline(always)]
    fn use_X_packet(&self) -> bool { true } // on by default
}

Note that this wouldn't be a fully-fledged IDET, but gdbstub would still use this new use_X_packet function similarly to other IDET functions as a way to dead-code-eliminate any X packet parsing + handling code if use_X_packet returns false.

Admittedly, this will add some complexity to an otherwise simple PR, but the more I think about it, the more I feel like this should probably be the approach we take here...

Feel free to take a crack at this, though there's a non-zero chance I'll end up pushing to this PR directly myself sometime this weekend...

[daniel5151]

Alright, I pushed up some changes. Could you pull these changes down and make sure everything still works?

I'd also appreciate it if you could quickly sanity-check that overriding the use_x_upcase_packet does indeed disable X packet support.

[bet4it]

Oh no, don't do this. Why such a basic command is set to optional?
What type of command do you think is qualified to be "base packet"?

this current implementation will unconditionally add quite a bit of extra binary bloat to the base no_std gdbstub configuration

I think you are insane about the size of binary. Is the size more important than correctness or elegance?

Why would GDB waste bandwidth trying to write data with a length of zero?

In fact, GDB wants to write memory with X command by default, because it's more efficient. But the server may not implement it, so GDB makes a request with zero length to detect it, and fallbacks to M if it doesn't exist. So if you really want to make the binary smaller, why not set M to optional?

[daniel5151]

@bet4it the following excerpt from the spec should answer your questions:

At a minimum, a stub is required to support the ‘?’ command to tell GDB the reason for halting, ‘g’ and ‘G’ commands for register access, and the ‘m’ and ‘M’ commands for memory access.

https://sourceware.org/gdb/onlinedocs/gdb/Overview.html#Overview

Making M optional isn't possible, since it's part of the required base protocol.

---

I think you are insane about the size of binary. Is the size more important than correctness or elegance?

I'm not sure if you've noticed yet, but gdbstub is also a playground for me to push the limits of writing no_std rust. The "small binary size" aspect of the project is something I do for fun - it's not of strict practical purpose.

That said, this is still my project, so for the time being, I will continue to keep binary size as an important part of the project - even if it requires a less "elegant" internal code :)

EDIT: ah, and thank you for the explanation re: the zero-length write. This makes sense :)

[bet4it]

If you care so much about the size, why you implement RLE?

[daniel5151]

I actually do have a TODO to see if I can make RLE optional 😉: https://github.com/daniel5151/gdbstub/blob/dev/0.6/src/protocol/response_writer.rs#L26

As a broader point, it's important to remember that gdbstub isn't something that's tied to my day-job in any way, so there are many engineering decisions in the library that fall waaaaay off the traditional time + effort vs. reward curve. e.g: the fact that I try very hard to keep the library no_panic, the "personal challenge" of keeping the minimum configuration binary size low, the heavy-reliance on the type-system (to an arguably "unnecessary" extent), etc...

If I were writing this library with deadlines + deliverables in mind, I would be taking a very different approach to the overall library design 😄

---

Anyways, to get back on track: @gz is this good to merge? Everything seems to be working as expected?

[gz]

Tried it on my (still a bit broken implementation) but X packets arrived (or not) depending on what's returned in use_x_upcase_packet

[daniel5151]

Yep, that's fine. The GDB client is allowed to probe for the existence of the packet, so it'll send it unconditionally (whether it's implemented or not). If the target responds with a non-empty packet, that means it supports the X packet.

Basically, I just wanted to double check that if the X packet support was enabled, the protocol would indeed continue to use X (rather than falling back to M)

[daniel5151]

So... does the 👍 mean that things are confirmed working, and this is ready to merge?

[gz]

Ah yes,things are working. Sorry if I wasnt clear earlier!