Unify read/write_register and read/write_registers into one

[bet4it]

Currently we provide two type of register related API, read/write_register and read/write_registers, corresponding to g/G and p/P commands in GDB RSP.

We can only provide read/write_register methods in the user-level API, the current "bulk" read/write_registers methods can be implemented base on read/write_register, so user don't need to write redundant code.

[daniel5151]

I think this idea is quite interesting, and merits some discussion.

To begin, lets consider some historical context:

• The GBD RSP requires that all targets support the g and G packets, whereas the p and P packets are optional extensions
• As such, early versions of gdbstub only supported g and G, with p and P being added later by Add 'read/write register' commands #22
• Because p and P were only added later in development, not all Arch defns included RegId implementations. Implement missing arch-specific RegId implementations #29 tracks the addition of RegId implementations, and even today, there is still one missing in gdbstub_arch

With those points in mind, lets also consider some of the merits of the current approach:

• If a target uses the same Register struct for its canonical register representation (either by defining a custom Arch, or re-using the struct from gdbstub_arch), implementing g and G can be implementing in a single memcpy operation, which is incredibly fast + terse.
• The "barrier to entry" of having a minimal implementation up-and-running is (arguably) lower with the g and G packets, as the p and P API is more complex to implement (at the moment), requiring manual fiddling with uXX::{from,to}_{le,be}_bytes calls, vs. simply assigning data to struct fields (e.g: regs.rax = *myregs.rax)
  • Similarly, it is easier to get a new Arch implementation up-and-running if you don't need to implement RegId for it

Finally, lets consider the pros:

• I don't know if that last point about Arch-"barrier to entry" is that convincing, as most users don't end up implementing their own Arch (and just use the ones defined in gdbstub_arch).
• Given that most implementations do end up implementing the single-register read/writes anyways, unifying the bulk and single register access methods into one would absolutely reduce overall implementation effort (even if it requires a bit more frontloaded work)
• Unifying the two implementations could simplify gdbstub's Arch code, as we might be able to get rid of the distinction between Registers and RegId entirely - whereby Arch implementations require a RegId

And lastly, the cons:

• As always, my primary concern is about code size in minimal gdbstub configurations. Due to the linear access patterns of the bulk register read/write implementations, I'd bet that the generated code is a lot simpler and "tighter" than an implementation that uses single-register access, which could result in non-linear access patterns across when reading/writing registers by invoking each read/write register handler separately...
• We'd probably have to remove the current PowerPcAltivec32 implementation from gdbstub_arch until someone has a chance to add+test a RegId implementation.

---

To cut a long story short: I lean in favor of this API change - assuming we can settle on a good design. i.e: one that hits all of gdbstub's primary targets: ease of use, performance, and low-overhead (in minimal configurations).

That said, I don't think these changes are necessarily a pressing concern at this time, and I wouldn't take a serious look at how these changes might be implemented until after 0.6 is polished up and out the door. In my mind, this would be the kind of big API change that would headline a future 0.7 release...

That said, if you're keen on getting this in sooner rather than later, feel free to start sketching out the kind of API you'd like to see!

I'd be interesting to see what kind of API + internal refactoring you'd want to do to facilitate this change, either via some code snippets in this discussion, or even via a Draft PR.

[bet4it]

The "barrier to entry" of having a minimal implementation up-and-running is (arguably) lower with the g and G packets, as the p and P API is more complex to implement (at the moment), requiring manual fiddling with uXX::{from,to}_{le,be}_bytes calls, vs. simply assigning data to struct fields (e.g: regs.rax = *myregs.rax)

I think when we implement it, we can change the API to avoid getting the raw bytes. For example, if all the registers in Registers have the same type u32, we can change the API to this:

fn read_register(
    &mut self,
    _tid: (),
    reg_id: ArmCoreRegId,
    reg: &u32,
) -> TargetResult<(), Self> {..}

Then we can call it by:

fn read_registers(
    &mut self,
    regs: &mut ArmCoreRegs,
) -> TargetResult<(), Self> 
    for i in 0..13 {
        read_register(.., ArmCoreRegId::Gpr(i), &regs.core.r[i]);
    }
    read_register(.., ArmCoreRegId::Sp, &regs.core.sp);
    read_register(.., ArmCoreRegId::Lr, &regs.core.lr);
    read_register(.., ArmCoreRegId::Pc, &regs.core.pc);
    read_register(.., ArmCoreRegId::Cpsr, &regs.core.cpsr);
    Ok(())
}

---

As always, my primary concern is about code size in minimal gdbstub configurations.

Oh no, don't tell me the way you choose only depends on which has the minimal code size🙃

[daniel5151]

if all the registers in Registers have the same type u32

Sure... but that's a non-starter, since that assumption doesn't hold for most platforms. The API needs to be generic to work with arbitrary Arch implementations, including those that have register sizes that vary in length.

inb4 "just use a u128 + truncate" - that isn't workable either, since there are platforms that have registers >128 bytes (e.g: for vector extensions).

---

I think the most important consideration that'd need to be addressed with this proposed API change is the fact that if we remove the current bulk register read/write API, we would be removing the current "easy to use" (but less efficient) API for reading/writing registers using "native" types (i.e: `u32, u64, etc...)

Or to rephrase this as an action item: how can we reformulate the current RegId + single register read/write infrastructure to support typed register access?

One untested + half-baked idea that I'm thinking of is doing something like:

enum MyRegId {
    GeneralPupose { idx: usize, val: &mut u32 },
    SpecialReg { val: &mut u16 }
    VectorReg { val: &mut [u8; 256] }
}

Whereby each RegId variant includes a reference to a typed buffer that the implementation can read/write data from/into.

fn write_register(
    &mut self,
    reg: &mut MyRegId,
) -> TargetResult<(), Self> 
    match reg {
        MyRegId::GeneralPupose { idx, val } => self.registers.gp[idx] = *val,
        ...
    }
    Ok(())
}

fn read_register(
    &mut self,
    reg: &mut MyRegId,
) -> TargetResult<(), Self> 
    match reg {
        MyRegId::GeneralPupose { idx, val } => *val = self.registers.gp[idx],
        ...
    }
    Ok(())
}

I think that's a pretty reasonable user-facing API, but I'm a bit fuzzy on how this would be implemented on gdbstub's end... i.e: how would gdbstub call read/write_register from the packet handlers? Where would the &mut uXX buffers be allocated - especially given that the implementation needs to be generic over arbitrary Arch implementation?

---

Oh no, don't tell me the way you choose only depends on which has the minimal code size🙃

Haha, no, of course not. The code size is just one of the several factors I consider when designing APIs, alongside ease of use, robustness, and correctness. It's moreso that any new approach should not result in a massive increase in code-size.

I'll be honest, I wouldn't worry too much about this during the design phase. It's something we can profile after we have a rough implementation up and running, and assuming the generated code doesn't balloon too much, I wouldn't really care much, since the ergonomic benefits would outweigh the slight bump in code size.

[bet4it]

Sure... but that's a non-starter, since that assumption doesn't hold for most platforms. The API needs to be generic to work with arbitrary Arch implementations, including those that have register sizes that vary in length.

I think we can pass different types as one argument using inheritance in C++ or Java. Rust can't do this?

[daniel5151]

Errr, I'm not entirely sure how to respond to that 😅

Given the scope of the changes you've been making, I was under the impression you had a pretty solid grasp of Rust and its various approaches to polymorphism (even if you were a C/C++ programmer at heart 😝). I'm not sure if I'd be able to give you a crash course in that topic as part of this particular discussion...

To be clear, I don't mean to come off as disrespectful - far from it. It's just that getting this API refactor right (i.e: correct + ergonomic + performant) will likely require some pretty clever Rust type system shenanigans, and I was kinda hoping you'd be able to suggest the details of that design yourself...

If not, that's totally fine! I want to make it clear that I do really like this idea, and now that you've brought it up, and I can say with 100% certainty that is something I'll look into implementing at some point in the future.

It's just that I don't have the time / mental energy to work on this particular refactor right now, so I'd only start thinking seriously about it after 0.6 is released.