Dynamic `RegId` Support #58

DrChat · 2021-05-27T20:28:13Z

Description

As per a discussion in #53, (comment), gdbstub's RegId abstraction does not have a way to support dynamic register files due to the register size field returned from RegId::from_raw_id.

As suggested by @daniel5151, we can make the register size an optional hint, and enforce register sizing based on that.

This changeset also removes the array allocated in the single register access handler, and instead passes the target a closure that behaves in a similar manner to the one passed to Registers::gdb_serialize.

API Stability

This will break SemVer compatibility, though for a good reason.

Checklist

Implementation
- cargo build compiles without errors or warnings
- cargo clippy runs without errors or warnings
- cargo fmt was run
- All tests pass
Documentation
- rustdoc + approprate inline code comments
- Updated CHANGELOG.md
- (if appropriate) Added feature to "Debugging Features" in README.md

Validation

TODO...
P.S: How do I query a single register using a GDB client?

GDB output

TODO...

armv4t output

TODO...

DrChat · 2021-05-27T20:33:02Z

PS C:\Dev\Repositories\gdbstub> cargo fmt --all -- --check

Not too sure what's up with that cargo fmt CI check... Mine is happy locally.

DrChat · 2021-05-27T20:36:09Z

gdbstub_arch/src/arm/reg/id.rs

@@ -31,6 +33,6 @@ impl RegId for ArmCoreRegId {
            25 => Self::Cpsr,
            _ => return None,
        };
-        Some((reg, 4))
+        Some((reg, Some(NonZeroUsize::new(4).unwrap())))


Any recommendations for these unwrap() calls? I know they're not allowed in gdbstub code, but this will just get optimized out by the compiler.

I mean, since you're already in a function that returns Option<_>, you can just use NonZeroUsize::new(x)?.

At the end of the day, any reasonably optimizing compiler should generate equivalent code when using ? or unwrap in these trivial cases (i.e: it'll elide the None branch entirely)

👍 using the ? operator is a nicer shorthand :)
I wish there was a new() compile-time constructor that threw a compiler error if the constant evaluated to 0 - that'd be a bit nicer.

sigh, preaching to the choir...

😂 time to submit some RFCs to Rust I guess

daniel5151

Yeah, this change is looking pretty reasonable!

I've left some comments, but all in all, this seems to be on the right track.

I'll also pop open a dev/0.6 branch which is based off the current master, and we'll switch the branch target for this PR over to it. EDIT: I just did that, so this PR will land in dev/0.6

gdbstub_arch/src/riscv/reg/id.rs

gdbstub_arch/src/x86/reg/id.rs

src/arch.rs

src/gdbstub_impl/ext/single_register_access.rs

src/target/ext/base/single_register_access.rs

daniel5151 · 2021-05-27T21:16:35Z

With regards to rustfmt: gdbstub uses rustfmt nightly (for that sweet, sweet wrap_comments functionality), hence why the CI is getting mad.

I should probably add that to the PR template...

As for how to read single registers from the GDB client... that's a bit tricky.

Here is the relevant code in GDB itself that fetches register values using the RSP: https://github.com/bminor/binutils-gdb/blob/master/gdb/remote.c#L8552-L8598

As you can see, it'll only fall back to using fetch_register_using_p (i.e: read_register) if the register isn't part of the standard g payload (i.e: read_registers).

The motivating example that came up when this feature was first implemented was the RISC-V Priv register, which can only be queried via read_register. AFAIK, there isn't really an equivalent register like that in the ARMv4T arch, so I'm not sure if you'll be able to trigger this particular codepath via the armv4t example...

Instead, if there's a similar scenario that you can test in one of the architectures that your project uses, I would appreciate it if you could post the GDB output + protocol from your project. If not, that's probably fine, since the diff is pretty small, and the implementation seems reasonable.

DrChat · 2021-05-27T21:24:39Z

As always, thanks for the ⚡ fast review :)

I will work through your comments and make the appropriate changes.
Let me see if I can't figure out a way to test these changes with our target. For reference, I'm interested in the PowerPC OEA SPRs (things like the decrementer and such).
I won't be able to provide the test output unfortunately.

daniel5151 · 2021-05-27T21:29:33Z

No problem!
What better way to take a break from coding + reviewing PRs at work than by coding + reviewing PRs on your side projects 😋

I won't be able to provide the test output [...]

Unfortunate, but totally understandable. Since this is landing in the dev/0.6 branch, I'm sure I'll circle back and give it a proper once-over at some point as well. That said, given how straightforward the changes are, I feel like things should Just Work™️

daniel5151 · 2021-06-01T16:03:58Z

Howdy! I'm back after a refreshing long weekend, and it's time to get back into the swing of things.

A cursory review of the PR indicates that there are still some comments that need to get resolved. Namely, there's still a matter of updating the documentation, replacing the current unwrap calls with ? invocations, using a newtype instead of a bare callback, etc...

Since this is landing in dev/0.6, there's no rush to merge, but it certainly would be nice to close it out sooner rather than later :)

p.s: feel free to un-draft the PR

DrChat · 2021-06-01T16:33:27Z

Appreciate it! Yeah, I still have some work to do with addressing those comments. Got distracted with other stuff so I just pushed off the changes I had :)

DrChat · 2021-06-01T19:15:17Z

Going to need some thought on that latest commit - I appear to have hit an odd compiler error:

error: implementation of `FnOnce` is not general enough                                                                           
  --> src\gdbstub_impl\ext\single_register_access.rs:32:71
   |
32 |                 ops.read_register(id, reg_id, SendRegisterOutput::new(&mut send_output))
   |                                                                       ^^^^^^^^^^^^^^^^ implementation of `FnOnce` is not general enough
   |
   = note: closure with signature `fn(&'2 [u8])` must implement `FnOnce<(&'1 [u8],)>`, for any lifetime `'1`...
   = note: ...but it actually implements `FnOnce<(&'2 [u8],)>`, for some specific lifetime `'2`

I see the following issue on Rust that discusses this specific error: rust-lang/rust#70263

daniel5151 · 2021-06-01T20:27:41Z

Ignore what I just said. It's actually fixable simply by not using a variable for the closure, and instead defining it inline. the explanation is something something lifetimes something something Rust magic ✨

let mut err = Ok(());

// TODO: Limit the number of bytes transferred on the wire to the register size
// (if specified).
ops.read_register(
    id,
    reg_id, 
    SendRegisterOutput::new(&mut |buf| match res.write_hex_buf(buf) {
        Ok(_) => {}
        Err(e) => err = Err(e),
    }),
)
.handle_error()?;

err?;

DrChat · 2021-06-01T21:00:45Z

¯\_(ツ)_/¯ Well, the compiler was probably right but I don't know why.

src/target/ext/base/single_register_access.rs

daniel5151

You've also still got to swap those .unwrap() calls for ? invocations.

src/arch.rs

DrChat · 2021-06-01T22:55:05Z

Good to go! Will still need to do testing though...
Does the ARM toy example have any SPRs or such that aren't available via the standard g query?

Edit: Ah, appears not. Perhaps we can add one? I see SPSR in the toy emulator.

daniel5151 · 2021-06-01T23:32:33Z

If you'd like to add SPRS to the armv4t example, that would be great! That said, I'm not sure how easy it'll be, and it might require tweaking the underling Arch implementation, which can be a bit of a hassle...

If not, then let me know once you've tested it and confirmed that things are working as expected.

DrChat · 2021-06-02T14:03:12Z

Ack. It appears that GDB does not expose SPSR (for some reason).
I will have to do some testing on my own, I'll report back later.

P.S: Can we put down some links to GDB source above the from_raw_id implementations?
It'd be nice to have inline documentation saying where the magic numbers came from.

daniel5151 · 2021-06-02T15:32:22Z

P.S: Can we put down some links to GDB source above the from_raw_id implementations?

That's a good idea. It shouldn't be too hard, since IIRC they should just be the same links as those linked to by the associated Arch. Up to you if you'd like to do it as part of this PR, or if we should just punt it to a follow up task that someone can tackle at some point.

DrChat · 2021-06-02T15:37:53Z

Perfect - I'll do that as part of this PR!

Also, it looks like GDB prefers the P packet when writing registers, which you can trigger with:

set $r31 = 0x1

daniel5151 · 2021-06-02T15:47:57Z

Also, it looks like GDB prefers the P packet when writing registers

Yep, I know. Too bad we aren't actually modifying the P packet in this PR ¯\_(ツ)_/¯

DrChat · 2021-06-02T15:54:20Z

True - just a little note for myself so I can use it to test from_raw_id :)

src/arch.rs

src/gdbstub_impl/ext/single_register_access.rs

daniel5151 · 2021-06-13T19:37:09Z

Any updates on this PR? Did you have a chance to validate that things are working as expected?

daniel5151

a couple of minor doc tweaks

daniel5151 · 2021-06-14T00:39:01Z

src/target/ext/base/single_register_access.rs

@@ -65,3 +65,19 @@ pub trait SingleRegisterAccess<Id>: Target {
 /// See [`SingleRegisterAccess`]
 pub type SingleRegisterAccessOps<'a, Id, T> =
    &'a mut dyn SingleRegisterAccess<Id, Arch = <T as Target>::Arch, Error = <T as Target>::Error>;
+
+/// An interface to send register data to the GDB remote debugger.


Suggested change

/// An interface to send register data to the GDB remote debugger.

/// An interface to send register data back to the GDB client.

daniel5151 · 2021-06-14T00:40:07Z

src/target/ext/base/single_register_access.rs

+        Self { inner }
+    }
+
+    /// Write out raw register bytes to the GDB debugger.


Suggested change

/// Write out raw register bytes to the GDB debugger.

/// Write raw register bytes to GDB. Bytes must be sent in the target's native endian order.

GDB stands for GNU DeBugger, so "GDB debugger" is redundant (like ATM Machine)

DrChat · 2021-06-14T02:34:40Z

Hey - no updates as of now. I got hung up on the issue that gdb does not actually expose SPRs for common PPC architectures (even OEA SPRs) and went to different tasking.

I can apply your suggested changes on Monday and then I probably won't be working on this for a few weeks.

daniel5151 · 2021-06-14T15:53:14Z

Gotcha. All good!

FYI, it seems that 0.6 will be coming out a bit sooner than expected (thanks to a recently reported bug in the breakpoint/watchpoints API + the in-progress state-machine refactoring I've been working on), so there's a non-zero chance I'll just pull the trigger and merge this PR as-is at some point in the coming weeks. I think these changes are worthwhile, and I'd rather have them land as part of 0.6, rather than some future 0.7 release.

daniel5151 · 2021-06-14T16:58:55Z

P.S: you should be able to manually define custom SPRs by returning a custom target.xml. Just because all the architectures currently implemented in gdbstub_arch stick to using the built-in target.xml features that ship with GDB, doesn't mean that you aren't able to define your own target-specific registers (at least in theory).

See https://sourceware.org/gdb/current/onlinedocs/gdb/Target-Description-Format.html#Target-Description-Format for more details (with special attention to G.2.5 Features and G.2.6 Types)

DrChat · 2021-06-14T19:41:57Z

Oh - huh, I never thought of that!
I'll do more research into that. Thanks for letting me know!

daniel5151 · 2021-06-23T06:18:42Z

Hey, heads up, I decided to see what exactly defining a custom target.xml file would entail, and before I knew it, I ended up adding a custom target.xml to the armv4t example. See commit 5562eec

If you run the armv4t example now, you'll see two new non-standard registers show up: custom and time. The latter of which (time) is set up such that it doesn't get sent back to the GDB stub as part of the regular g packet response, which forces the stub to read it via the p packet 😄

Hopefully this new example helps you out with defining custom target.xml files. Also, you'll need to rebase this PR on-top of these changes, and make sure that the 'p' packet still works as intended.

Cheers

DrChat · 2021-06-23T13:41:24Z

Ah - perfect! Thanks for looking into that!
Sorry I haven't been putting much time into this - I've been working unrelated stuff lately.

daniel5151 · 2021-06-23T15:45:41Z

Yeah, no worries. Playing around with custom target.xml files is something I've been meaning to do for a while now :)

bet4it · 2021-07-02T13:22:16Z

Hope this PR will be merged in 0.6

daniel5151 · 2021-07-02T15:14:18Z

Yep, I'll formally add it to the roadmap, and I'll look into merging it into dev/0.6 sometime soon-ish.

I've been a bit busy lately, and haven't had too much time to work on gdbstub. Hopefully I'll have some time over the July 4th long weekend to polish up some changes I've been working on + integrate these changes. Not sure about a hard release date for 0.6 yet, but I'd wager it's still a few weeks away.

bet4it · 2021-08-19T11:39:24Z

Any updates?

daniel5151 · 2021-08-19T15:49:02Z

As you might have guessed, my plan to "polish up my local changes + integrate these changes" hadn't panned out as I hoped 😅

I've been quite busy with work / life recently, so while I've been able to carve out some time in the morning to review PRs, I haven't had the chance to get back into "hands-on programming" mode with gdbstub.

With the vFile stuff coming in though, dev/0.6 will have yet more genuinely useful changes, so i'll really need to block out some time where I can hammer things out...

daniel5151 self-requested a review May 27, 2021 20:34

DrChat commented May 27, 2021

View reviewed changes

Allow variable register sizes for SingleRegisterAccess

a4ff570

DrChat force-pushed the dyn_regid branch from 4797d28 to a4ff570 Compare May 27, 2021 20:39

daniel5151 requested changes May 27, 2021

View reviewed changes

daniel5151 changed the base branch from master to dev/0.6 May 27, 2021 21:04

Clean-up use of Some(NonZeroUsize::new(N).unwrap())

272090e

DrChat force-pushed the dyn_regid branch from 4a800f7 to d56a382 Compare June 1, 2021 21:00

DrChat force-pushed the dyn_regid branch from d56a382 to 9ed7389 Compare June 1, 2021 21:03

Wrap raw read_register callback, and introduce a weird compiler error

37c82ec

DrChat force-pushed the dyn_regid branch from 9ed7389 to 37c82ec Compare June 1, 2021 21:05

Handle overflow when writing out registers

6b19ed0

daniel5151 reviewed Jun 1, 2021

View reviewed changes

src/target/ext/base/single_register_access.rs Show resolved Hide resolved

Update documentation

59e45c4

DrChat force-pushed the dyn_regid branch from 04faf0a to 59e45c4 Compare June 1, 2021 22:22

DrChat requested a review from daniel5151 June 1, 2021 22:34

daniel5151 requested changes Jun 1, 2021

View reviewed changes

src/arch.rs Outdated Show resolved Hide resolved

daniel5151 marked this pull request as ready for review June 1, 2021 22:45

DrChat added 2 commits June 1, 2021 17:48

Documentation update

834bd68

Substitue unwrap calls for passing up None

1fcbb0d

DrChat force-pushed the dyn_regid branch from d1cff02 to 1fcbb0d Compare June 1, 2021 22:54

daniel5151 requested changes Jun 2, 2021

View reviewed changes

src/arch.rs Outdated Show resolved Hide resolved

src/gdbstub_impl/ext/single_register_access.rs Outdated Show resolved Hide resolved

DrChat added 2 commits June 2, 2021 10:58

Move n variable usage to allow for DCE

0d82197

Missing period

04e264f

DrChat force-pushed the dyn_regid branch from 17449eb to 04e264f Compare June 2, 2021 16:03

daniel5151 requested changes Jun 14, 2021

View reviewed changes

daniel5151 added this to the 0.6 milestone Jul 2, 2021

bet4it mentioned this pull request Aug 4, 2021

Support Host I/O operations #66

Merged

13 tasks

daniel5151 force-pushed the dev/0.6 branch 2 times, most recently from abdaca5 to 7b15a18 Compare September 19, 2021 18:21

bet4it mentioned this pull request Sep 26, 2021

Dynamic read_register + RegId support #85

Merged

13 tasks

daniel5151 closed this in #85 Sep 29, 2021

	/// An interface to send register data to the GDB remote debugger.
	/// An interface to send register data back to the GDB client.

	/// Write out raw register bytes to the GDB debugger.
	/// Write raw register bytes to GDB. Bytes must be sent in the target's native endian order.

Dynamic RegId Support #58

Dynamic RegId Support #58

Conversation

DrChat commented May 27, 2021 • edited

Description

API Stability

Checklist

Validation

DrChat commented May 27, 2021

DrChat May 27, 2021

Choose a reason for hiding this comment

daniel5151 May 27, 2021

Choose a reason for hiding this comment

DrChat May 27, 2021

Choose a reason for hiding this comment

daniel5151 May 27, 2021

Choose a reason for hiding this comment

DrChat May 27, 2021

Choose a reason for hiding this comment

daniel5151 left a comment • edited

Choose a reason for hiding this comment

daniel5151 commented May 27, 2021

DrChat commented May 27, 2021

daniel5151 commented May 27, 2021

daniel5151 commented Jun 1, 2021

DrChat commented Jun 1, 2021

DrChat commented Jun 1, 2021

daniel5151 commented Jun 1, 2021 • edited

DrChat commented Jun 1, 2021 • edited

daniel5151 left a comment

Choose a reason for hiding this comment

DrChat commented Jun 1, 2021 • edited

daniel5151 commented Jun 1, 2021 • edited

DrChat commented Jun 2, 2021 • edited

daniel5151 commented Jun 2, 2021

DrChat commented Jun 2, 2021

daniel5151 commented Jun 2, 2021

DrChat commented Jun 2, 2021

daniel5151 commented Jun 13, 2021

daniel5151 left a comment

Choose a reason for hiding this comment

daniel5151 Jun 14, 2021

Choose a reason for hiding this comment

daniel5151 Jun 14, 2021

Choose a reason for hiding this comment

DrChat commented Jun 14, 2021

daniel5151 commented Jun 14, 2021

daniel5151 commented Jun 14, 2021 • edited

DrChat commented Jun 14, 2021

daniel5151 commented Jun 23, 2021

DrChat commented Jun 23, 2021

daniel5151 commented Jun 23, 2021

bet4it commented Jul 2, 2021

daniel5151 commented Jul 2, 2021

bet4it commented Aug 19, 2021

daniel5151 commented Aug 19, 2021

Dynamic `RegId` Support #58

Dynamic `RegId` Support #58

DrChat commented May 27, 2021 •

edited

daniel5151 left a comment •

edited

daniel5151 commented Jun 1, 2021 •

edited

DrChat commented Jun 1, 2021 •

edited

DrChat commented Jun 1, 2021 •

edited

daniel5151 commented Jun 1, 2021 •

edited

DrChat commented Jun 2, 2021 •

edited

daniel5151 commented Jun 14, 2021 •

edited