Toggle support for QStartNoAckMode

[bet4it]

Description

This PR makes the feature QStartNoAckMode toggleable.

Closes #134

API Stability

• This PR does not require a breaking API change

Checklist

• Documentation
  • Ensured any public-facing rustdoc formatting looks good (via cargo doc)
  • (if appropriate) Added feature to "Debugging Features" in README.md
• Validation
  • Included output of running examples/armv4t with RUST_LOG=trace + any relevant GDB output under the "Validation" section below
  • Included output of running ./example_no_std/check_size.sh before/after changes under the "Validation" section below
• If implementing a new protocol extension IDET
  • Included a basic sample implementation in examples/armv4t
  • IDET can be optimized out (confirmed via ./example_no_std/check_size.sh)
  • OR implementation requires introducing non-optional binary bloat (please elaborate under "Description")
• If upstreaming an Arch implementation
  • I have tested this code in my project, and to the best of my knowledge, it is working as intended.

Validation

Before/After `./example_no_std/check_size.sh` output

Before

target/release/gdbstub-nostd  :
section               size    addr
.interp                 28     792
.note.gnu.property      32     824
.note.gnu.build-id      36     856
.note.ABI-tag           32     892
.gnu.hash               28     928
.dynsym                360     960
.dynstr                204    1320
.gnu.version            30    1524
.gnu.version_r          64    1560
.rela.dyn              408    1624
.init                   27    4096
.text                15158    4128
.fini                   13   19288
.rodata                954   20480
.eh_frame_hdr          244   21436
.eh_frame             1252   21680
.init_array              8   28088
.fini_array              8   28096
.dynamic               432   28104
.got                   136   28536
.data                    8   28672
.bss                     8   28680
.comment                27       0
Total                19497

After 2bc3d4f

target/release/gdbstub-nostd  :
section               size    addr
.interp                 28     792
.note.gnu.property      32     824
.note.gnu.build-id      36     856
.note.ABI-tag           32     892
.gnu.hash               28     928
.dynsym                360     960
.dynstr                204    1320
.gnu.version            30    1524
.gnu.version_r          64    1560
.rela.dyn              408    1624
.init                   27    4096
.text                15158    4128
.fini                   13   19288
.rodata                930   20480
.eh_frame_hdr          244   21412
.eh_frame             1252   21656
.init_array              8   28088
.fini_array              8   28096
.dynamic               432   28104
.got                   136   28536
.data                    8   28672
.bss                     8   28680
.comment                27       0
Total                19473

After 3e7e6a8

target/release/gdbstub-nostd  :
section               size    addr
.interp                 28     792
.note.gnu.property      32     824
.note.gnu.build-id      36     856
.note.ABI-tag           32     892
.gnu.hash               28     928
.dynsym                360     960
.dynstr                204    1320
.gnu.version            30    1524
.gnu.version_r          64    1560
.rela.dyn              408    1624
.init                   27    4096
.text                14949    4128
.fini                   13   19080
.rodata                930   20480
.eh_frame_hdr          244   21412
.eh_frame             1252   21656
.init_array              8   28088
.fini_array              8   28096
.dynamic               432   28104
.got                   136   28536
.data                    8   28672
.bss                     8   28680
.comment                27       0
Total                19264

[daniel5151]

Good start, but you'll also need to tweak the code in commands.rs to compile-out the QStartNoAckMode packet parsing if no ack mode is disabled.

Some pointers on that:

• https://github.com/daniel5151/gdbstub/blob/bf4ad90/src/protocol/commands.rs#L249-L251
• https://github.com/daniel5151/gdbstub/blob/bf4ad90/src/protocol/commands.rs#L94

And doing so will have knock-on effects in stub/core_impl, whereby you'll need to split QStartNoAckMode handling out of Base and into its own packet group.

The end result is that check_size.sh should actually report a smaller size than it does today.

[bet4it]

gdbstub/src/protocol/commands.rs

Lines 249 to 251 in bf4ad90

	x_upcase_packet use 'a {
	"X" => _x_upcase::X<'a>,
	}

It seems that this code is used when the packet received is parsed? But QStartNoAckMode is enabled globally.

[daniel5151]

Yes, to be clear: I'm saying that you should move the QStartNoAckMode code out of the base use 'a category, and move it into a new no_ack_mode category (the same way X packet parsing could have been shoved into base use 'a, but was instead put into it's own section)

[bet4it]

Even if we don't include QStartNoAckMode in features string, client can still enable it by set remote noack-packet on before connecting, so we could leave the handle code there?

[daniel5151]

gdbstub is all about "you don't pay for what you don't use".

If the gdbstub implementation has decided to statically disable support for noack mode (e.g: by overriding the method to return a static false boolean from use_no_ack_mode), then the stub shouldn't have to pay the packet parsing cost of QStartNoAckMode.

The details of the use_no_ack_mode implementation shouldn't be important to the consumer of the library (i.e: the fact that we skip reporting ;QStartNoAckMode+ in the features string). From the gdbstub user's perspective, the usage contract should be as simple as: "if I disable support for noack mode, then my stub binary shrinks, in exchange for a slightly slower protocol" (and the docs of the use_no_ack_mode method should reflect that fact).

[daniel5151]

Nice, this LGTM! (docs outstanding)

I'll merge this in later today, and cut a new release with this feature included.

Thanks again for the contribution!

[daniel5151]

I don't think these docs are up to snuff...
That said, for expediency's sake, I don't mind cleaning these up after merge.

In the future though, docs should avoid mentioning the nitty-gritty details of their underlying implementation whenever possible, and instead, be written from the perspective of user-visible behavior. The point of gdbstub is that users really shouldn't care what a QStartNoAckMode packet is.

i.e: i'll tweak these docs to paraphrase from https://sourceware.org/gdb/onlinedocs/gdb/Packet-Acknowledgment.html#Packet-Acknowledgment

[daniel5151]

gdbstub 0.6.6 has been published to crates.io