Dynamic `Arch` selection

[daniel5151]

Extracted from a back-and-forth on https://github.com/daniel5151/gdbstub/issues/31#issuecomment-795811837

Will likely tie into #12

---

With the current Arch architecture, it's actually pretty easy to "punch through" the type-safe abstractions by implementing an Arch that looks something like this:

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

pub enum PassthroughArch {}

impl Arch for PassthroughArch {
    type Usize = u64; // something reasonably big
    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 */ }
        // can re-use existing `impl Registers` types from `gdbstub_arch`, 
        // calling `gdb_serialize` and `gdb_deserialize` directly...
        if using_arm {
            let mut regs = gdbstub_arch::arm::regs::ArmCoreRegs::default();
            regs.pc = self.pc;
            // ...
            let mut i = 0;
            regs.gdb_serialize(|b| { regs.buffer[i] = b.unwrap_or(0); i++; })
        }
    }

    // similarly for `write_registers`
}

This can then be combined with the recently added TargetDescriptionXmlOverride IDET to effectively create a runtime configurable Arch + Target (see #43)

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

• Bypasses one of gdbstub's most useful abstractions, forcing end users to care about the unfortunate details of how GDB de/serializes register data (e.g: sending data in the order defined by target.xml, input/output buffer management, etc...)

• 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) when running in an std environment.

• In the current version of gdbstub (0.4.5 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.

• • *

There are a couple things that can be done to mitigate this issue:

1. Provide a single "blessed" implementation of PassthroughArch, so that folks don't have to write all this boilerplate themselves
   • This is easy, and can be done as a first-time-contributor's PR, as it doesn't require digging into gdbstub's guts.
2. Set aside some time to reconsider the underlying Arch API, and modify it to enable more elegant runtime configuration, while also retaining as many ergonomic features / abstraction layers as the current static-oriented API.
   • This is significantly harder, and is something that I would probably want to tackle myself, as it may have sweeping changes across the codebase

[DrChat]

To add to this discussion - gdbstub's RegId abstraction does not allow for dynamic dispatch due to RegId::from_raw_id returning both an ID and size, the latter of which cannot be known at compile-time.

I'm wondering if there's any drawbacks to modifying the signature of SingleRegisterAccess::read_register to take a &mut Write in lieu of a buffer, that way it can effectively directly call ResponseWriter::write_hex_buf instead of using a temporary buffer. This will allow us to remove size information from from_raw_id - instead read_register will determine the size of a register. The most obvious drawback I can think of now is that it'll allow the size of a register returned from write_register to deviate from the target spec (as of now it will panic in core::slice::copy_from_slice in the armv4t example).

A similar approach could be taken for SingleRegisterAccess::write_register for symmetry, though the current approach taken is perfectly fine.

[daniel5151]

Damn, you're right. RegId totally isn't object safe.

Your idea of modifying read_register to accept something like a output: &mut dyn FnMut(&[u8]) is intriguing, as while it'll certainly be a bit more clunky to use, it does alleviate the need for a temporary buffer...

I guess the broader question is whether or not gdbstub should provide any "guard rails" to avoid accidentally sending invalid register data back to the client. One way to get the best of both worlds might be to keep RegId::from_raw_id's signature as-is, and instead of using the size value as a way to truncate the buffer passed to SingleRegisterAccess::read_register, it would instead be used to validate the length of any outgoing data sent via the &mut dyn FnMut(&[u8]) callback.

In the case of PassthroughRegId, you could simply return usize::MAX for the size, which would be equivalent to "send back as much data as you'd like". Alternatively, since these changes will already require a breaking API change, we could also change RegId::from_raw_id to return a Option<Self, Option<NonZeroUsize>> instead, where the Some(usize, None) would represent "no size hint available".

If you can hack together a POC implementation, I'd be more than happy to work with you to tweak the API to me more amenable to your use case. Since I just released 0.5 a few days ago, I'm not sure if I'd want to cut a breaking 0.6 release just yet, so we'd want to land these changes on a [currently non-existent] dev/0.6 branch.

---

Also, could you elaborate on what you mean by "(as of now it will panic in core::slice::copy_from_slice in the armv4t example)"? I'm not sure I follow.

[DrChat]

Sure! I'd be happy to put in some work to improve this API :)

I like your suggestion of providing a size hint via an Option<Self, Option<NonZeroUsize>> return value. I suppose to take it further, if a RegId implementation returns Some(NonZeroUsize) - we would enforce that a register be sized appropriately. Otherwise, it can be any size.

---

https://github.com/daniel5151/gdbstub/blob/9e0526d1ed560d21aef8cfd0d8ac03a416ca86b5/examples/armv4t/gdb/mod.rs#L205-L219

copy_from_slice will panic if the source or destination have a size mismatch, which ensures that (this particular case) cannot pass invalid-sized registers.

[DrChat]

Also - I noticed that there is a new pc() function on the Registers trait. It doesn't appear to be used at this point, but for obvious reasons it isn't going to work when the architecture is dynamically selected :)

P.S: Have you had any thoughts about the weird x86 boot process? Specifically its mode-switching between 16-bit, to 32-bit, to 64-bit? I've been wondering if GDB can support that. It seems that it allows you to switch the target architecture via set arch, but I haven't figured out how to get the gdbstub side to adjust accordingly.

[daniel5151]

Also - I noticed that there is a new pc() function on the Registers trait.

sigh this is another vestigial bit of infrastructure left over from when I took a crack at implementing GDB Agent support (see the feature-draft/agent branch). In hindsight, I could achieve the same functionality by having a get_pc() method as part of the Agent IDET itself... oops.

I should probably just mark that bit of the Arch API as deprecated and mark it for removal in 0.6...

P.S: Have you had any thoughts about the weird x86 boot process? Specifically its mode-switching between 16-bit, to 32-bit, to 64-bit?

Hmmm, that's an interesting point...

Taking a very cursory look at the "prior art" in the open-source GDB stub space, it seems that most stubs assume a single static architecture throughout execution. e.g: QEMU seems to have the same issue, and as such, requires some client side workarounds to debug early-boot code.

Another consideration is that the GDB RSP doesn't have any built-in arch-switching signaling mechanism, so even though you can set arch on the client side, it's up to you to signal that switch to gdbstub.

TL;DR, I think there are two approaches you can take here:

1. Do the QEMU thing where you just stick to the lowest-common-denominator arch, and then do those translation shenanigans as outlined in the linked StackOverflow thread
2. Implement the MonitorCmd IDET and add a custom arch-switching mechanism to mirror the client-side set arch. I'm not super sure on the specifics on what an implementation would look like, so you'll be breaking even more new ground if you go that route.

[bet4it]

Add this to 0.6 milestone?

[daniel5151]

Which bit specifically?

Dynamic Arch selection as a whole is a larger design problem, one which I've already had a couple of false-starts on fixing in the past. Fixing this issue entirely will require a holistic reexamination of the current Arch infrastructure, and it'll be a non-trivial time commitment to rewrite / tweak the API to be more dynamic while still retaining as many of the performance and ergonomic benefits that come with having it be statically defined.

At the moment, my gut feeling is to keep 0.6 the "bare metal" release (with an emphasis on the new state-machine API + no panic support), and punting the bulk of this issue to 0.7.