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
add qRegisterInfo packet support #103
Conversation
2b8a260
to
731b882
Compare
Hey there, thanks for sending this in! I'm actually on vacation right now, so I won't get the chance to give this a thorough review until sometime next week. In the meantime, I'll check in occasionally to approve CI runs, since the first and most obvious thing you'll need to fix is that the code within I would suggest using the |
731b882
to
837a7fe
Compare
Thanks for the quick reply and apologies for disturbing your vacation with this :D Looks like I lost the view of the bigger picture somehow, it should be no_std now.
compared to the existing ones
cargo bloat output
|
Oh, no need to be sorry! I'm about to head back on the road, but I thought I'd throw two more comments your way while I had the chance:
struct Register<'a> {
pub name: &'a str, // added benefit: no need to be limited to just &'static str
pub invalidate_regs: Option<&'a [u16]>, // can be any size
...
}
// and then in the handler
fn get_register_info(&mut self, reg_id: u8, cb: &mut dyn FnOnce(Option<Register<'_>>)) -> Result<(), Self::Error> {
let dynamic_name = String::from("foo");
let dynamic_regs = vec![1, 2, 3];
let reg = Register {
name: &dynamic_name,
invalidate_regs: &dynamic_regs,
...
};
(cb)(reg);
Ok(())
} i.e: you've lifted the decision of how to / how much to allocate into the handler, rather than imposing that decision onto the user. If you poke around the codebase, there should be examples of this pattern being used. e.g: this is similar to how the gdb_de/serialize functions work. |
dac328a
to
84c53df
Compare
Thanks a lot again for taking the time, your explanations and pointers were very helpful!
Regarding changes to the current code, I changed the lifetime of the
|
Ah, yes, as for FnOnce, things get a bit complicated, since it needs to take Fortunately, this situation is similar to one we encountered while working on Host IO support, and though we didn't end up using a callback for that API, I have a feeling using a callback in this API is a good idea. Please check out this comment for a proposed API: #66 (comment) If you can pull that off, that should get the CI green. |
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.
Found some time tonight to give the code a proper comb-over. Left a bunch of comments. Overall, things are looking good!
84c53df
to
08161ff
Compare
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.
Some more comments...
Also, on a broader note: I think this new way of reporting registers should also become an (optional) part of the Arch
trait, akin to how target.xml is handled today. Of course, you'll have to implement it in such a way that if neither the Arch not the Target provide an implementation, the code still gets dead code eliminated...
If you can take a crack at that, that'd be much appreciated. If not, I can take a hand at helping out with that sometime next week.
08161ff
to
386b1c1
Compare
I'll give it a try tomorrow! I was actually planning to try upstream the Berkeley Packet Filter (BPF) arch so getting a better sense on that side of things is a good idea |
Oh, awesome! Always happy to see more As for the implementation: you'll probably want to mirror how
If you can't seem to get things working, just let me know. |
Seems like gdbstub/gdbstub_arch/src/arm/reg/id.rs Lines 27 to 35 in 1fde6f7
|
386b1c1
to
1fde6f7
Compare
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.
So, when I said that you should implement this at the Arch
level, I'd meant that in addition to the dynamic Target
-level implementation 😅
Please re-add the dynamic override implementation, as it is useful for implementations that are generic across multiple architectures.
i.e: there should be 3 "knobs" for specifying qRegisterInfo data:
- the top-level
use_register_info
override, that enables completely disabling the packet (regardless of whether theArch
orTarget
includes implementations of it) - The
Arch
level static Register information - The
Target
level dynamic "override" Register information
Like I said - look at how target.xml is being handled, and match that logic, fallbacks + overrides and all.
huh, would you look at that 👀 I'll try and remember to push up a hotfix right to |
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.
mostly doc and style nits, with one somewhat pertinent question regarding possible binary overhead changes...
thanks for bearing with me through all these review cycles, I think we are almost done :)
Awesome I'll try fixing these now. Thanks for being so patient and the hand holding with very helpful code snippets 😄 |
75b73dd
to
60a2ad6
Compare
60a2ad6
to
2689c37
Compare
Hey, quick update - I've been surprisingly busy lately, so I'll probably get a chance to look at this again sometime this weekend. Sorry about that! |
no worries, thanks for the update! |
2689c37
to
bd3a42c
Compare
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.
Only thing left are some minor comment formatting issues, but honestly, I'll just take care of those myself, since I'll be making some misc. commits prior to releasing 0.6.2 anyways...
Thanks again for the PR!
I'll try and push out 0.6.2 sometime in the next few days :)
///1. Use the target definition python file if one is specified. | ||
///2. If the target definition doesn't have any of the info from the | ||
///target.xml (registers) then proceed to read the `target.xml`. | ||
///3. Fall back on the `qRegisterInfo` packets. | ||
///4. Use hardcoded defaults if available. |
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.
nit: formatting
///1. Use the target definition python file if one is specified. | |
///2. If the target definition doesn't have any of the info from the | |
///target.xml (registers) then proceed to read the `target.xml`. | |
///3. Fall back on the `qRegisterInfo` packets. | |
///4. Use hardcoded defaults if available. | |
/// 1. Use the target definition python file if one is specified. | |
/// 2. If the target definition doesn't have any of the info from the | |
/// target.xml (registers) then proceed to read the `target.xml`. | |
/// 3. Fall back on the `qRegisterInfo` packets. | |
/// 4. Use hardcoded defaults if available. |
/// runtime-configurable target, it's unlikely that you'll need to implement | ||
/// this extension. | ||
pub trait RegisterInfoOverride: Target { | ||
/// Invoke `reg_info.write(reg)` where `reg` is a [`Register` |
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.
nit: weird little formatting issue. reminds you why wrap_comment
is still a nightly feature :)
Oh, also, heads up: in 4a8f325, I renamed all instances of |
@jawilk 0.6.2 has been released :) |
...oh, and thanks for the Coffee! |
Description
This implements the LLDB specifc
qRegisterInfo
packet (documentation here).Relates to #99
I was working on a target where I had some trouble getting the .xml target description formatting right so I opted to use the
qRegisterInfo
packet instead which pretty much worked right away. I haven't looked into unifying them as mentioned in the compatibility issue as of now though.One caveat is that some values in the reply need to be in decimal representation. Due to this I added a
write_dec()
method in theResponseWriter
(I'm not sure this was actually necessary). Since this packet is only used byLLDB
it might be reasonable to just hide it behind a feature flag. Besides that, if enabled, theRegister
struct ingdbstub/src/target/ext/register_info.rs
has a few&'static str
members which lead to a bigger binary as well.API Stability
Checklist
cargo build
compiles withouterrors
orwarnings
cargo clippy
runs withouterrors
orwarnings
cargo fmt
was runexamples/armv4t
->Done but might be better to comment it out because it only applies to
LLDB
examples/armv4t
withRUST_LOG=trace
+ any relevant GDB output under the "Validation" section below->attached the LLDB output instead
./scripts/test_dead_code_elim.sh
and/or./example_no_std/check_size.sh
)->Additional
write_dec()
methodValidation
LLDB output