Add 'read/write register' commands #22

thomashk0 · 2020-08-30T16:53:48Z

A proposal (probably to be reworked) API extension to support RSP "read register command".

daniel5151 · 2020-08-30T17:40:11Z

Nice, thanks for splitting up the PRs!

Like I mentioned in #21 (comment), the proposed API is leaking part of the underlying GDB protocol (namely, the reg_number: usize parameter), which is something gdbstub has tried to avoid.

Unfortunately, I can't think of a super simple way to easily hide this detail without a bit of plumbing and trait-rework 😬

Here's what I'm thinking: extend the Register trait with an associated RegId type + gdb_de/serialize_single methods, and plumb that type-based register identification through into the user-facing Target::read_register API.

e.g: for RISC-V, the RiscvCoreRegs struct could have an associated RiscvCoreRegsId enum that looks something like:

enum RiscvCoreRegsId {
    R(usize), // shame there's no way to encode a u5 at the type level, ah well
    PC,
}

That way, the signature of read_registers could be changed to:

fn read_register(
     &mut self,
     dst: &mut [u8],
     reg: arch::riscv::reg::RiscvCoreRegsId,
 )

The end result is an API which enables end-users to uniquely identify a register without having to search through the GDB source code to find the register numbers for their target.

I've got some other ideas on how it be possible to replace the dst parameter with some sort of direct reference into the regs struct (e.g: &mut usize), but I think it'll need some serious type-level shenanigans that can't really experiment with right now...

If all this sounds daunting, I don't blame you.
gdbstub makes heavy use of Rust's powerful type system to eliminate as many runtime checks / conversions as possible. While this is definitely great from a performance / robustness point of view, it does tend to complicate the implementation quite a bit 😅

If you want to take a crack at implementing some of these ideas (or proposing your own alternative!), I'd love to work with you to come up with something awesome 😄.

If that's not really something you'd be interested in at this time, then I'll go ahead and open an issue (something like "Add read/write single registers API"), and try to implement this feature at some point before releasing the next breaking release (which I've been hacking on intermittently for the past couple weeks).

thomashk0 · 2020-08-31T06:51:32Z

Pretty awesome explanation, thanks !

I'm definitely interested to work on something along those lines. Actually its a nice opportunity to play with some advanced Rust :) Hopefully I will have some time to work on it in the next days.

daniel5151 · 2020-08-31T16:34:01Z

Hey, good to hear! If you ever want to throw some ideas around, feel free to shoot me an email / DM me on Discord (daniel5151#4520).

Additionally, I maaaaaaay have nerd-sniped myself while working on a way to remove the dst parameter 😅

Turns out that while it's possible to pull-off, it ends up being quite ugly. Here's a (very detailed) Rust Playground link with my experimentation:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=3f7b726e9dbf8ee2455f06c30ce093de

A lot of the code could be massively simplified if/when GATs are ever stabilized, but who knows when those are gonna land...

I guess for the initial implementation, passing a raw byte-buffer would work. Alternatively, it might be better to pass a &u128/&mut u128 to make things easier for end-users, and do the appropriate to_le_bytes shenanigans internally (though this will break if someone contributes AVX-256/512 extensions).

daniel5151 · 2020-09-03T02:20:38Z

By the way, I wanted to draw your attention to the trait-based-opt-features branch I've been working on. Here's the relevant draft PR: #24

It's a massive API overhaul which uses a pretty nifty type-system + compiler optimization trick to emulate optional trait methods at compile time (along with a bunch of other useful properties). It should make the Target API significantly more robust for end users, with the added benefit of simplifying a lot of the GdbStub internals that handle OptResult and the MaybeUnimpl.

Thankfully, the majority of the changes in trait-based-opt-features are localized to the gdbstub_impl and target modules, with the rest of the repo (notably arch) being left almost completely untouched.

I bring this up since there's a good chance you'll get a nasty looking merge conflict once the trait-based-opt-features is merged to master. That said, since the meat of this feature will likely be contained within the arch module, it'll only be the surface level plumbing that'll have to be adjusted.

So, uh, sorry about that!

thomashk0 · 2020-09-03T07:02:41Z

Thanks for notifying !

I read you code snippet, pretty interesting (it blew my mind the first 5 min :p). If I understood correctly, you try to tie the register identifier with their storage, so that we can have different storage types for different identifiers?

I pushed my (probably naive) solution to the PR. Here are the important points:

There is a new a RegId type in Register trait: adding this imposed to add a default type in all arch implementations (default type in trait is an unstable feature). There is also a new type RegIdNotImplemented, that can be used as a default for other arch.
the trait Target gained a new 'read_register' function, with prototype read_register(&mut self, Arch::Registers::RegId reg_id, &mut [u8]) -> ...

A few remarks:

The register type for a given register ID is not exposed at all (we just pass &mut [u8])
I added a GdbSerializable trait (that can serve both RegId and Registers), not sure if that is a good idea, leave me know

Does the approach seems good to you? I will take a look at PR #24 and try to adapt.

daniel5151

I read you code snippet, pretty interesting (it blew my mind the first 5 min :p). If I understood correctly, you try to tie the register identifier with their storage, so that we can have different storage types for different identifiers?

Heh, glad you liked it! Pretty sneaky, eh?
And yeah, your analysis was spot on. Shame the ergonomics just aren't there yet...

I threw a couple of general comments in the review. Though it seems that some of them might not be relevant, depending on if you choose to go with the approach I've proposed below...

The register type for a given register ID is not exposed at all (we just pass &mut [u8])

Yeah, that's fine. As I showed above, making it bulletproof at the type level seems really tricky 😉

I added a GdbSerializable trait...

I think a big issue is that the current gdb_deserialize method isn't particularly well suited to be implemented on a RegId enum. It forces RegId to have a Default type, as there needs to be some destination to write data into...

I did think of another idea though: instead of having RegId implement GdbSerializable, why not do something a simple as:

trait RegId {
    /// Map raw GDB register numbers to a (RegId, register size). 
    /// Returns None if the register isn't available.
    fn from_raw_id(id: usize) -> Option<(Self, usize)>;
}

// for unsupported / unimplemented systems
impl RegId for () {
    fn from_raw_id(id: usize) -> Option<(Self, usize)> { None }
}

// and in the `GdbStub` implementation:
Command::p(cmd) => {
    let mut dst = [0u8; 16];
    let reg = <<T::Arch as Arch>::Registers as Registers>::RegId::from_raw_id(cmd.reg_id);
    let (reg_id, size) = match reg {
        Some(v) => v,
        None => return Ok(None), // GDB docs say to return nothing for unsupported query
    }
    let dst = &mut dst[0..size];
    // enforce size within the GDB stub, instead of relying on the target
    target.read_register(dst, reg_id).maybe_missing_impl()?;
    res.write_hex_buf(dst)?;
}

I think this approach has a lot of merit, as it avoids introducing any redundant traits or weird type-bounds. Thoughts?

src/protocol/commands/_p.rs

src/arch/riscv/reg/riscv.rs

src/arch/traits.rs

src/arch/riscv/reg/riscv.rs

src/arch/traits.rs

thomashk0 · 2020-09-04T06:52:09Z

Thank you so much, your feedbacks are very helpful !!
I'm much more satisfied with this version (based on your suggestions) ! While the commit touches many files, the changes are really minor.

I still plan to add a few things in the RISC-V RegId within a few days (mostly adding missing registers). Maybe we can wait this is done before merging the PR?

daniel5151

Nice, that's looking pretty slick!
I'm glad we could come up with something that's nice and clean!

I've left a couple of comments inline / below, but this looks just about ready to ship 🚢

I still plan to add a few things in the RISC-V RegId within a few days (mostly adding missing registers). Maybe we can wait this is done before merging the PR?

Yep, just let me know when you're ready to merge 👍

Could you also implement the write_register functionality as well?
- It shouldn't be too difficult, and would keep the API nice and symmetrical
- Hint: When parsing the P packet, you'll want to follow a similar pattern to the G packet, where the hex string is decoded "in place" using the decode_hex_buf helper, with the resulting packet holding a &'a [u8] reference to the decoded buffer.
If it's not too much trouble, could you update the armv4t example with an example implementation of read/write_register?
- The corresponding RegID structure should be self-evident (the ArmCoreRegs struct is very straightforward)
- Running the example should be fairly straightforward (see examples/armv4t/README.md), and i've even included a pre-built test binary (test.elf) with debug symbols. It uses a relative path for the source map, so as long as you run gdb-multiarch from the test_bin directory, it should Just Work
- The code is very simple, so even if you're not familiar with the details of the ARM architecture, it shouldn't be too difficult to get working :)

src/arch/traits.rs

thomashk0 · 2020-09-05T12:57:48Z

Hey! Good news its almost ready to ship 😄

I applied your suggestions and also added the write register command. I'm still not fully satisfied with "what should I return as error?". The doc do not say (as with 'p') that nothing means "unsupported". However, looking at the source code (https://github.com/bsdjhb/gdb/blob/master/gdb/remote.c#L8361), GDB seems to handle the unsupported case the same way as 'p' does... . BTW the error code if any is returned to 'P' is just ignored, anything that starts with E should be accepted...

Yeah, I can add the RegId quite easily for Armvt4 I think (speaking ARM all days @work :p). It is a great idea as it would allow to have tests for that new part of the API. What about doing that in a dedicated issue/PR?

In some architecture, Gdb will actually query single registers, for ex. on RISC-V. This commit introduces support for this command. To support this, several significant modifications are introduced: - Add a `RegId` type in `Register` trait. - the trait Target gained a new 'read_register' function, which is made optional to avoid breaking existing implementations.

The RiscvRegId is now public in the arch::riscv::reg module.

daniel5151

Nice! The parsing / handler / API surface of the write_registers command LGTM!

I've got a few more comments and nits, but they're mostly just minor doc-related fixes.

I'm still not fully satisfied with "what should I return as error?"

Lets just play it safe and go off what the protocol docs say (even if the GDB client implementation doesn't seem to care). i.e: add a res.write_str("E01")? inside the None arm of the P packet handler.

Yeah, I can add the RegId quite easily for Armvt4... What about doing that in a dedicated issue/PR?

Could we just lump it in with this PR? After all, we're already lumping in some RISC-V definitions, so why not add some ARM ones as well 😛 Plus, it would be good to have a working example of how to use the new API alongside the implementation itself.

daniel5151 · 2020-09-05T14:48:50Z

src/arch/traits.rs

@@ -31,8 +31,12 @@ impl RegId for () {
 pub trait Registers: Default {
    /// Register identifier for addressing single registers.
    ///
-    /// If your target does not implement that feature, you can use `RegId = ()`
-    /// as a default, which implements the `RegId` trait.
+    /// Architectures that do not implement single register read can safely use


Suggested change

/// Architectures that do not implement single register read can safely use

/// Architectures that do not support single register accesses can safely use

daniel5151 · 2020-09-05T14:49:10Z

src/arch/traits.rs

+    /// Architectures that do not implement single register read can safely use
+    /// `RegId = ()` as a default.
+    ///
+    /// **Note**: the use `RegId = ()` in most architectures is temporary.


Suggested change

/// **Note**: the use `RegId = ()` in most architectures is temporary.

/// **Note**: the use of `RegId = ()` in most architectures is temporary.

daniel5151 · 2020-09-05T14:51:45Z

src/protocol/commands/_p_upcase.rs

+        let val = decode_hex_buf(body.next()?).ok()?;
+        Some(P { reg_id, val })
+    }
+}


nit: could you add a newline to the end of this file?

src/target.rs

daniel5151 · 2020-09-05T15:00:02Z

src/gdbstub_impl/mod.rs

+                let reg = <<T::Arch as Arch>::Registers as Registers>::RegId::from_raw_id(p.reg_id);
+                let (reg_id, _) = match reg {
+                    Some(v) => v,
+                    None => return Ok(None),


(see main review comment)

daniel5151 · 2020-09-06T16:34:57Z

By the way, I've decided to hold-off on merging #24 until after this PR gets in, so don't worry about dealing with any merge conflicts, I'll just resolve them on my end 😄

thomashk0 · 2020-09-06T17:38:04Z

Oh, cool!
Hopefully, I will manage to free some time tomorrow to complete that :)

This is more in line with GDB specifications of the 'P' packet.

thomashk0 · 2020-09-08T17:27:21Z

Regarding the armv4t example, I only added the write_register. To see it in action :

> target remote :9001
> set $r2=0xDEADBEEF
> info register

I struggle to get gdb send single read register for this architecture, so I removed read_register, as it would never be executed...

daniel5151

Hooray! 🎉

I've done one last full-sweep through the PR, found the last few tiiiiny style nits, but after those are fixed, I'm all ready to smash that merge button!

Thanks again for all the work you've put into the PR. It's a great feature, and I hope you had fun messing about with Rust's associated types and generics 😄

P.S: before merging my massive API overhaul PR, I'll publish one more version of gdbstub 0.2 with this feature, so that you won't have to upgrade to the new API if you don't want to 😉

examples/armv4t/gdb.rs

src/protocol/commands.rs

note: the 'P' handler now also reports an error if write_register is not implemented.

thomashk0 · 2020-09-09T06:54:05Z

Hey Daniel, I hope this last update will be fine :)

Many thanks for you very helpful reviews, I learned so much in the process! I think I will continue to contribute small things in gdbstub at least the 64 bit version of RISC-V + proper FP support!

Having that in 0.2 would be cool and allow my project to directly depend on the crate.io version.

daniel5151

Wonderful! Time to 🚢 this bad boy!

Looking forward to any other contributions you might want to make!

And small heads up, I've made a minor API change in the new version on crates.io, where the resume method takes a Actions iterator instead of a raw &mut dyn impl Iterator<..>. This is entirely surface level, and doesn't change any functionality. Thankfully, semver technically allows versions <1.x.y to make breaking API changes at any time 😋

Oh, and shameless plug, but I would recommend updating to 0.3 once I publish it, since aside from API improvements, it comes with a couple of internal protocol fixes + optimizations. If you're curious what the API looks like, check out the examples directory on the development branch 😄

Cheers!

daniel5151 · 2020-09-09T17:30:18Z

I've pushed version 0.2.2 up to crates.io which includes this PR 😄

thomashk0 mentioned this pull request Aug 30, 2020

Add (partial) support for RISC-V #21

Merged

daniel5151 self-requested a review August 30, 2020 17:40

daniel5151 changed the title ~~add 'read register' command of the RSP protocol~~ Add 'read/write register' commands Aug 31, 2020

thomashk0 force-pushed the rsp-read-register branch from 8fd47b8 to 8009fa2 Compare September 1, 2020 17:21

thomashk0 force-pushed the rsp-read-register branch from 8009fa2 to 2a2b7a7 Compare September 3, 2020 06:43

daniel5151 requested changes Sep 3, 2020

View reviewed changes

thomashk0 force-pushed the rsp-read-register branch from 2a2b7a7 to 9a30c83 Compare September 4, 2020 06:47

daniel5151 requested changes Sep 4, 2020

View reviewed changes

src/arch/traits.rs Outdated Show resolved Hide resolved

src/arch/traits.rs Outdated Show resolved Hide resolved

thomashk0 force-pushed the rsp-read-register branch from a5e4925 to e33d4b6 Compare September 5, 2020 12:45

thomashk0 added 4 commits September 5, 2020 16:45

complete implementation of the RegId trait for RISC-V

a2862ab

The RiscvRegId is now public in the arch::riscv::reg module.

rewording docstrings of RegId and Register traits

fbdd3d4

add 'write register' command of the RSP protocol

e33d4b6

daniel5151 requested changes Sep 5, 2020

View reviewed changes

thomashk0 force-pushed the rsp-read-register branch from 9cea90c to b9a0918 Compare September 6, 2020 17:37

thomashk0 added 2 commits September 6, 2020 21:36

fix minor style issues in register read/write trait

090a638

send "E01" in case register write is not supported

b9a0918

This is more in line with GDB specifications of the 'P' packet.

thomashk0 force-pushed the rsp-read-register branch from 1d3ec13 to b37f622 Compare September 8, 2020 17:24

daniel5151 requested changes Sep 8, 2020

View reviewed changes

examples/armv4t/gdb.rs Outdated Show resolved Hide resolved

examples/armv4t/gdb.rs Show resolved Hide resolved

src/protocol/commands.rs Outdated Show resolved Hide resolved

implement single register write in the armv4t example

b37f622

note: the 'P' handler now also reports an error if write_register is not implemented.

implement read_register for the armv4t example

6161d21

daniel5151 approved these changes Sep 9, 2020

View reviewed changes

daniel5151 merged commit 7cb558c into daniel5151:master Sep 9, 2020

daniel5151 mentioned this pull request Sep 17, 2020

Implement missing arch-specific RegId implementations #29

Open

7 tasks

daniel5151 mentioned this pull request Sep 25, 2021

RIP size on x86-64 #80

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 'read/write register' commands #22

Add 'read/write register' commands #22

thomashk0 commented Aug 30, 2020

daniel5151 commented Aug 30, 2020

thomashk0 commented Aug 31, 2020

daniel5151 commented Aug 31, 2020 •

edited

daniel5151 commented Sep 3, 2020 •

edited

thomashk0 commented Sep 3, 2020

daniel5151 left a comment •

edited

thomashk0 commented Sep 4, 2020 •

edited

daniel5151 left a comment

thomashk0 commented Sep 5, 2020

daniel5151 left a comment •

edited

daniel5151 Sep 5, 2020

daniel5151 Sep 5, 2020

daniel5151 Sep 5, 2020

daniel5151 Sep 5, 2020

daniel5151 commented Sep 6, 2020

thomashk0 commented Sep 6, 2020

thomashk0 commented Sep 8, 2020 •

edited

daniel5151 left a comment

thomashk0 commented Sep 9, 2020

daniel5151 left a comment •

edited

daniel5151 commented Sep 9, 2020

	/// Architectures that do not implement single register read can safely use
	/// Architectures that do not support single register accesses can safely use

	/// Note: the use `RegId = ()` in most architectures is temporary.
	/// Note: the use of `RegId = ()` in most architectures is temporary.

Add 'read/write register' commands #22

Add 'read/write register' commands #22

Conversation

thomashk0 commented Aug 30, 2020

daniel5151 commented Aug 30, 2020

thomashk0 commented Aug 31, 2020

daniel5151 commented Aug 31, 2020 • edited

daniel5151 commented Sep 3, 2020 • edited

thomashk0 commented Sep 3, 2020

daniel5151 left a comment • edited

Choose a reason for hiding this comment

thomashk0 commented Sep 4, 2020 • edited

daniel5151 left a comment

Choose a reason for hiding this comment

thomashk0 commented Sep 5, 2020

daniel5151 left a comment • edited

Choose a reason for hiding this comment

daniel5151 Sep 5, 2020

Choose a reason for hiding this comment

daniel5151 Sep 5, 2020

Choose a reason for hiding this comment

daniel5151 Sep 5, 2020

Choose a reason for hiding this comment

daniel5151 Sep 5, 2020

Choose a reason for hiding this comment

daniel5151 commented Sep 6, 2020

thomashk0 commented Sep 6, 2020

thomashk0 commented Sep 8, 2020 • edited

daniel5151 left a comment

Choose a reason for hiding this comment

thomashk0 commented Sep 9, 2020

daniel5151 left a comment • edited

Choose a reason for hiding this comment

daniel5151 commented Sep 9, 2020

daniel5151 commented Aug 31, 2020 •

edited

daniel5151 commented Sep 3, 2020 •

edited

daniel5151 left a comment •

edited

thomashk0 commented Sep 4, 2020 •

edited

daniel5151 left a comment •

edited

thomashk0 commented Sep 8, 2020 •

edited

daniel5151 left a comment •

edited