Dynamic `Arch` selection

[daniel5151]

Dynamic `Arch` selection

[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.

---

gdbstub/examples/armv4t/gdb/mod.rs

Lines 205 to 219 in 9e0526d

	fn read_register(
	&mut self,
	_tid: (),
	reg_id: gdbstub_arch::arm::reg::id::ArmCoreRegId,
	dst: &mut [u8],
	) -> TargetResult<(), Self> {
	if let Some(i) = cpu_reg_id(reg_id) {
	let w = self.cpu.reg_get(self.cpu.mode(), i);
	dst.copy_from_slice(&w.to_le_bytes());
	Ok(())
	} else {
	Err(().into())
	}
	}

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]

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]

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.