-
-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RISC-V: List all integer registers for lldb compatibility #149
RISC-V: List all integer registers for lldb compatibility #149
Conversation
lldb requires a full list of registers in the target_description_xml (gdb on the other hand seems to ignore the list completely). The gdb manual sounds like these registers should exist anyway: > The ‘org.gnu.gdb.riscv.cpu’ feature is required for RISC-V targets. It should contain the registers ‘x0’ through ‘x31’, and ‘pc’. Therefore it seems reasonable that the RISC-V arch returns the full list.
gdbstub_arch/src/riscv/mod.rs
Outdated
// are permitted in any medium without royalty provided the copyright | ||
// notice and this notice are preserved. --> | ||
Some(r#"<target version="1.0"> | ||
<architecture>riscv:rv32</architecture> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's quite some duplication, but we can't use replace() of format! here due to require 'static string.
If preferred, I can attempt a macro solution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of hard-coding the string inline with the Rust code, can we just yoink the relevant XML files from GDB, put them alongside this mod.rs
file, and use include_str!
to reference them from the Rust code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, will do!
Oof, ignore most of those CI errors - that's what I get for not pinning the Rust version I use in CI 😅 Still reviewing the code, but at first blush, this seems reasonable... |
Ok swapped out for the original files. It's a cleaner solution, but one downside is that the comments in it (copyright notice) are now sent over as part of the protocol. And I guess the binary size goes up a little bit as we now also store the copyright notice in the binary. |
Tested with both lldb and gdb with my risc-v emulator |
lldb trace
|
In terms of size impact: The original file (from gdb, with copyright notice, dtd and xml header is 2004 bytes). The shortened version I had in my earlier patch is roughly 1.5k. |
The binary size hit is unfortunate... but I think if someone is really trying to squeeze every last byte out of their |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I'll set a reminder to cut a new gdbstub_arch
patch release at some point in the near future :)
Before you do, let me upload the equivalent arm patch |
Actually, I just realized you only have arm4t, which my emulator does not use (it's armv6m, for which I have my own xml definition anyway). So no need to wait for anything from my end |
For what it's worth, you might want to consider upstreaming your custom Arch implementation into |
Sure, will do! |
Apologies for the delay - I finally found a moment to publish a new release of gdbstub_arch 😅 0.3.1 has been published to crates.io, and includes this fix :) |
Description
lldb requires a full list of registers in the target_description_xml (gdb on the other hand seems to ignore the list completely).
The gdb manual sounds like these registers should exist anyway:
Therefore it seems reasonable that the RISC-V arch returns the full list.
API Stability
Checklist
Arch
implementationValidation
LLDB output (before this change)
LLDB output (after this change)
trace (before this change)
trace (after this change)
Before/After `./example_no_std/check_size.sh` output
Before
Can't test - example_no_std doesn't build right now