[Feedback] Overall API design

[daniel5151]

Did you end up using gdbstub in your project?
Or maybe you considered using it, but found some major flaw in the API?

In either case, please leave a comment letting me know what you think of gdbstub!

Also, if it's not "classified info", please share what kind of project you're integrating gdbstub into (e.g: emulation, virtualization, embedded, etc...).

• If everything went smoothly, awesome! Positive feedback like that makes me more confident that the library is getting closer to a 1.0.0 release.
• Hit a pain point? Let me know how you think the API could be improved!

Please don't comment about missing protocol features or bugs. Those should be filed as separate issues.

---

This tracking issue will remain open until gdbstub hits version 1.0.0.

[DrChat]

Overall the API is a delight and painless to use. I'm using this library as a frontend to an emulator and learning Rust at the same time, and this is a breeze compared to our old solution.

One tidbit I'm running into now is that the current implementation for handling registers is mostly static - i.e. the target is not passed into RegId or Registers for handling any dynamic state (such as different register sets being available depending on the CPU mode, or the CPU architecture not being known until runtime).
Perhaps it should be rethought a bit so that it can be a bit more dynamic (but not necessarily dynamic enough that the return codes will differ across two sequential invocations on those traits).

---

For Registers, gdb_serialize and gdb_deserialize could be moved into Target, such as:

fn gdb_deserialize(&self, bytes: &[u8]) -> <Self::Arch as Arch>::Registers {
  todo!();
}

Though this may conflict with the ability to provide a default implementation. This will require more thought.

[daniel5151]

@DrChat thanks for the feedback, I really appreciate it!
Glad to hear that gdbstub is working as expected.

---

Funny enough, the API you're proposing is actually what the API was like back in gdbstub 0.1 (see these old docs for details). While this API is certainly more flexible, it does require the user to dig through the GDB internal documentation and learn about obscure things such as the target's internal GDB register ordering, the GDB big-endian serialization format, etc...

In gdbstub 0.2, I reworked the {read,write}_registers API to add an intermediate abstraction layer in the form of the Registers trait, which separated the nitty-gritty protocol specific details of register de/serialization from the day-to-day business of querying the Target for it's register state. While this was certainly less flexible, it was undeniably much more ergonomic from a Target-implementation POV.

Now, that being said, it's important to note that you are not locked-in to using the built-in Arch/Registers/RegId implementations, and that it's pretty easy to "punch through" the abstraction by doing something like this:

// not actually tested, but something like this ought to work

pub enum PassthroughArch {}

impl Arch for PassthroughArch {
    type Registers = PassthroughRegs;
    type RegId = PassthroughRegId;
    ...
}

pub struct PassthroughRegs {
    len: usize,
    buffer: [u8; MAX_SIZE], // a vec could also work
}

// push logic up a level into `Target`
impl Registers for PassthroughRegs {
    fn gdb_serialize(&self, mut write_byte: impl FnMut(Option<u8>)) {
        for byte in &self.buffer[..self.len] { write_byte(Some(byte)) }
    }

    fn gdb_deserialize(&mut self, bytes: &[u8]) -> Result<(), ()> {
        self.buffer[..bytes.len()].copy_from_slice(bytes)
    }
}

pub struct PassthroughRegId(usize);
impl RegId for PassthroughRegId { ... } // elided for brevity, should be straightforward

impl Target {
    type Arch = PassthroughArch;

    // .... later on, in the appropriate IDET ... //

    fn read_registers(
        &mut self, 
        regs: &mut PassthroughRegs<YourTargetSpecificMetadata>
    ) -> TargetResult<(), Self> {
        // write data directly into `regs.buffer` + `regs.len`
        if self.cpu.in_foo_mode() { /* one way */  } else { /* other way */ }
    }

    // similarly for `write_registers`
}

While this will work in gdbstub right now, this approach does have a few downsides:

• It is not zero-copy, as the PassthroughRegs buffer will be be immediately copied into the "true" output buffer on every invocation. Mind you, neither is using a type Registers intermediary, but hey, linear copies are super fast, so it's probably fine.
• The PassthroughRegs type is static, and as such, will have to include a buffer that's large enough to handle the "wost case" largest inbound/outbound payload. This can be mitigated by using variable-length type as the backing storage (such as a Vec).
• In the current version of gdbstub (0.4.4 at the time of writing), each call to {read,write}_registers results in a fresh instance of type Registers being allocated on the stack. The original intent behind this decision was to avoid having a permanent "static" Registers struct take up memory inside struct GdbStub, but honestly, it's not that important. Thankfully, this is a internal issue with a fairly easy fix, and I'm considering switching where the type is stored (in struct GdbStub vs. the stack) in the next minor/patch version of gdbstub.

Lastly, if you've got any concrete proposals on how to restructure API, I'm all ears! gdbstub is not shy about making API breaking changes in the name of simplicity and ergonomics, so all proposals are on the table.

[DrChat]

I appreciate the detailed write-up! I'll have to take a look at implementing a passthrough like you described.

You're right that it's not ideal for users to know the layout of any GDB internals, and the current API is definitely an improvement over the old one.

Though, I just had a thought: Could we define the register file for each architecture using a #[repr(C)] struct (with a module such as zerocopy::byteorder for endianness), and use a small unsafe block to cast the raw bytes from GDB into the register file struct?
Doing so could allow us to just eliminate gdb_serialize and gdb_deserialize altogether. For a passthrough, we'd just set the type of Registers to something like [u8] (something that would make the cast a no-op), and then in our generic architecture we would cast the register file depending on what architecture we're running.

Something like:

            ext::Base::G(cmd) => {
                let regs: &<T::Arch as Arch>::Registers = unsafe {
                    let (head, r, _) = cmd.vals.align_to();
                    if !head.is_empty() {
                        return Err(Error::TargetMismatch);
                    }

                    &r[0]
                };

                match target.base_ops() {
                    BaseOps::SingleThread(ops) => ops.write_registers(&regs),
                    BaseOps::MultiThread(ops) => ops.write_registers(&regs, self.current_mem_tid),
                }
                .handle_error()?;

                HandlerStatus::NeedsOK
            }

This would eliminate one copy when dealing with registers, and we'd be able to keep the register file implementation details internal to gdbstub (with the exception of read_registers and write_registers accessing it, which is fine).

[daniel5151]

To preface, here is a brief overview of the current read/write_registers implementation:

(alloc A) GDB Packet (ASCII, hex encoded, Big Endian) (in GDB-specific order)
Size: N

    <-- hex-en/decoded in-place --> 

(alloc A) GDB Packet (Binary, Big Endian) (in GDB-specific order)
Size: N/2 (each byte takes 2 ASCII bytes)

    <-- gdb_de/serialize --> 

(alloc B) Arch::Registers (arbitrary user-defined Rust struct, Machine Endian - almost always Little Endian) 
Size: >N/2 (not fully N, but likely not #[repr(packed)] as to be N/2)

    <-- read/write_registers --> 

(alloc C / read from registers directly) Target-specific code

Theoretically, your proposal to use zerocopy::byteorder would indeed work, and enable an "automatic" implementation of gdb_de/serialize by leveraging #[repr(C, packed)] + ensuring that the elements of the struct match the GDB order. Plus, you'd be avoiding that extra alloc B allocation. These are both good pros!

Unfortunately, this approach does come with a few cons:

• I'm not sure how zerocopy (or other similar-crates) would interact with #[repr(packed)].
• While you save the extra allocation, you'll still end up paying the cost of swizzling the byteorder when reading/writing on little-endian platforms (i.e: the vast majority of platforms).
• The GDB ordering can be a bit... illogical at times, and tying it directly to the Registers type is a bit unfortunate. e.g: see the ARM core registers "floating point registers"
• (arguably most importantly) it would require adding a nugget of unsafe code to gdbstub, which is something I've [somewhat miraculously] avoided up until now!

Now, none of these cons are deal-breakers, but they do make me wonder whether or not saving the extra copy is worth it.

If you want to keep riffing on this idea, how about you open a new issue and link back to these comments?
Also, if you have the time for it and care strongly about that extra allocation, feel free to take a crack at hacking together a working implementation - I would love to see a PR for it!

[DrChat]

Ah - you have some really good points! I'm still learning a lot about the language, and it's easy to think "ah, well I can just do it the way I would've done it in C" :)

The extra copy and serialize/deserialize isn't that important for me to eliminate (as of now), so I probably won't pursue it too much further. My thought process was moreso to have the ability to directly reuse the existing register definitions in a generic architecture, and on further research it looks like that is do-able using the passthrough method you described.

---

There are a couple of other issues though that need further research, namely that RegId::from_raw_id() and Arch::target_description_xml() are called statically and can't do dynamic dispatch based on the running architecture. It doesn't look like there's a way to pass these values through to the Target either.
For RegId, the GDB ID can simply be passed through to [read/write]_register, but RegId::from_raw_id() also returns a register size parameter, which can vary depending on the target architecture.
Arch::target_description_xml() will need to be handled differently since it does not interact with functionality implemented on Target.

I wonder if there's a way to support both the current static implementation and a more dynamic way (should a Target want to do so).

I'll work on a PR and we can move this discussion there once I open it.

[daniel5151]

but RegId::from_raw_id() also returns a register size parameter, which can vary depending on the target architecture

Ah, this is a very fair point, since the register size parameter determines the size of the buffer sent back to the GDB client...

One quick (albeit breaking) fix would be to change read_register to have a return type of TargetResult<usize, Self>, where usize is the number of bytes written into the dst buffer. This would mirror the standard library's Read::read method, and enable writing a "passthrough" RegId implementation that uses a sufficiently-large register size value to fit (almost) any arch's registers. A bit janky, but it should work.

Arch::target_description_xml() [is] called statically and can't do dynamic dispatch based on the running architecture.

On a broader note, I've been meaning to overhaul and rework Arch::target_description_xml() for other reasons as well (tracked under #12). If possible, it'd be nice to hit two birds with one stone while doing other Arch related breaking changes.

---

Thanks for bringing up these valuable points about runtime configurable Arch implementations, I really appreciate it! Heck, I'd actually planned to use gdbstub in a multi-system emulator myself at some point, so it's good that we're catching and ironing out these issues now 😄

I'm excited to see what you come up with, and if you need any help / want to spitball some ideas, don't hesitate to open it as a draft and get some early feedback.

[DrChat]

Another thing that recently came up - the implementation of resume for MultiThreadOps (and this may apply to SingleThreadOps) is somewhat easy to implement incorrectly.
It's provided an iterable list Actions, so my assumption was that I should iterate through that list and execute all requested actions.

However, when single-stepping a core, the GDB client will request a single-step on the target core and a continue on all other cores.
This means that the GDB client wants the target core to single-step, and allow all other cores to execute while this operation is running.
I iterated through the list of actions in order, so I saw a single-step action followed by a continue action, which causes the single-step operation to simply become a continue operation.

The interface may need some rethinking. Per the GDB protocol, the list of actions are processed from left-to-right, wherein the leftmost action overrides actions specified later.

[daniel5151]

Heck, I don't even provide a proper example of how to work with the current Actions based API in-tree (the armv4t_multicore example is pretty bad), so I'm not surprised it's easy to misuse 😅

But yes, the current resume API is very easy to misuse, which is why I've been working on a pretty substantial resume API overhaul as part of gdbstub 0.5!
The following code should be a good starting point: https://github.com/daniel5151/gdbstub/blob/dev/0.5/src/target/ext/base/multithread.rs#L16-L106

If at all possible, it'd be great if you could give this new API a shot. It's lumped in as part of some other upcoming changes, but they're mostly superficial code reorganization and/or internal refactoring, so it shouldn't be too hard to upgrade from 0.4.

As always, any and all suggestions are much appreciated.

---

Per the GDB protocol, the list of actions are processed from left-to-right, wherein the leftmost action overrides actions specified later.

This is a very good point, and is something I really ought to explicitly document as part of the Implementation requirements section of the resume / set_resume_action docs...

[xobs]

I'm looking to add gdb support to my kernel by hooking an interrupt. Currently, the interrupt handler for the UART supports several single-key sequences to do things like dump the process table, count memory usage, and print process memory maps.

I would like to add a new single-key sequence g to enter gdbstub mode. Once this happens, all UART characters would be forwarded to gdbstub.

From what I understand, the gdbstub API is blocking. That is, I create a GdbStub struct with GdbStub::new(...), then run it with stub.run(). This function then blocks until the remote client disconnects.

This doesn't work inside an interrupt handler.

What I'd rather have is a state machine into which I can pass characters. As this state machine operates, it calls various functions on the Target.

It almost looks like I could do this by turning the recv_packet() call inside-out. Another option may be to add separate functions such as .start() / .pump(u8) / terminate() that replace the generic run() call.

[daniel5151]

Thanks for reaching out @xobs!

I'm really glad you did, since validating gdbstub's bare-metal use case is something I've been meaning to do for a very long time. I have a little hobby OS I work on from time to time, and one of the projects in my ever growing backlog is to add gdbstub support to it 😄

Now, before responding with some preliminary thoughts on how to get gdbstub up and running in a kernel context, I should mention that if this is something you'd like to seriously pursue, I would be more than happy to work with you to get things up and running, and/or work on a PR to tweak gdbstub's API to be more amenable to these kinds of use cases.

In addition, instead of cluttering this general "feedback" issue with our discussion, how about you open a dedicated tracking issue? Maybe something along the lines of "Validate bare-metal no_std gdbstub integration" (we can workshop the title later).

---

EDIT: I ended up spending a few hours after work today hacking together an implementation of a state machine based packet parser. Here's what I came up with: feature/state-machine-recv-packet. Let me know if this is what you had in mind!

---

I've given it some thought, and I think that your state machine approach to reading packets may very well work!

I'll be honest, I never even considered using a state machine to ingest packets, since in the back of my mind, I was always planning on just spinning up a dedicated GdbStub kernel thread and re-using the existing blocking Connection abstraction. Your approach seems a lot leaner and easier to integrate thought!

Diving into the implementation details, it seems that I only ever call Connection::read within recv_packet, so it should be entirely possible to provide an alternative GdbStub implementation / set of methods that uses a "pump" based interface instead. There will be some subtleties around how the API would actually look / work under-the-hood, but we can sort out those details in a separate thread...

Another thing to keep in mind is how to handle GDB client interrupts. gdbstub currently provides a poll-based mechanism to check for interrupts (via GdbInterrupt), which relies on the Connection::peek method under-the-hood to check if any breakpoint packets have arrived. In a interrupt-driven kernel debugging context, you could probably just bypass this poll-based mechanism entirely, and instead have your resume() handler work alongside your interrupt handler to return a StopReason::GdbInterrupt if a breakpoint byte comes down the wire while in GDB gdbstub mode.

---

Let me know if that answered your question! Like I mentioned above, if you want to keep this convo going, lets continue in a dedicated tracking issue.