-
-
Notifications
You must be signed in to change notification settings - Fork 48
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 support for qXfer:libraries{,-svr4}:read
#142
Conversation
A toy project I've been working on involves loading a custom kernel and a custom guest into a KVM instance, and these commands enable `gdb` to be able to debug both the guest and the kernel at the same time by reporting where both images are loaded in the address space. Initially this commit only implemented `qXfer:libraries:read` but it turned out that gdb never actually used that command. When implementing `qXfer:libraries-svr4:read`, however, gdb invoked that automatically (presumably it's target-specific). I ended up including both here for completeness.
Ah, very nice! I'll try and find some time soon to give this a more thorough look through. In the meantime, can you update the PR with some of the checkboxes + validation outlined in the PR template? Not sure if you missed that, or GitHub just didn't auto-suggest using it... Weird. https://github.com/daniel5151/gdbstub/blob/master/.github/pull_request_template.md |
No rush! What I'm working on is still proof-of-concept at the moment and may not see the light of day anyway. This'll help me debug things in the future and I figured I'd upstream after poking at it.
heh no full disclosure I saw it and it seemed quite long so I replaced it with a description and figured I'd see what happened. In any case I've filled in the bits here, but I'm not sure how to handle the |
Ha, all good, appreciate the honesty! 😄 As for the i.e: so long as the fake / stub implementation wouldn't make GDB blow up when connecting to the e.g: maybe the stub implementation can just report a single entry (corresponding to the in-tree test binary)? This is useful for two reasons:
|
Sounds good! I've filled in a few stubs which at least got |
Yeah, lets nix the If someone ends up needing it, they can send a PR to fill in that gap (along with doing the work of digging into how we could validate it in-tree) |
/// | ||
/// If `offset` is greater than the length of the underlying data, return | ||
/// `Ok(0)`. | ||
fn get_libraries_svr4( |
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.
Maybe it's not as part of this PR... but having another API force the user return "raw XML" (vs. having gdbstub
generate it on-the-fly) seems a little unfortunate from a usability POV.
e.g: In this case, I wonder if we could instead have an API that looks something like this instead:
fn get_libraries_srv4(
&self,
main_lm: Target::Arch::Usize,
report_lib: &mut dyn FnMut(
/*name:*/ &str,
/*lm:*/ Target::Arch::Usize,
/*l_addr:*/ Target::Arch::Usize,
/*l_ld:*/ Target::Arch::Usize,
/*lmid:*/ Option<Target::Arch::Usize>
)
) -> TargetResult<usize, Self>;
Though, as I typed this out, I remembered that I basically had the same through process while reviewing #54, and after reviewing that thread... I think the implementation in gdbstub
would get a bit too hairy (especially when considering how we'd need to handle non-zero offset
requests...)
Maybe a middle ground could be to keep the "raw XML" API, but also include some helper XML builders that would make it easier for end-users to generate the properly formatted XML they need to return?
e.g:
struct LibrariesSvr4XmlBuilder<'a, T> { ... }
impl<'a, T> LibrariesSvr4XmlBuilder<'a, T> where T: Target {
pub fn new_fixed(buf: &'a mut [u8]) -> Self { ... }
// using a growable vec
#[cfg(feature = "alloc")]
pub fn new_dynamic(&mut Vec<u8>) -> Self { ... }
// returns error if buffer is full
pub fn add_lib(&mut self, name: &str, lm: T::Arch::Usize, l_addr: T::Arch::Usize, ...) -> TargetResult<(), T> { ... }
}
Under the hood, this could re-use the ManagedVec
type https://github.com/daniel5151/gdbstub/blob/master/src/util/managed_vec.rs
Hmmm...
...but in any case, I don't think this is something that needs to happen as part of this PR.
I think adding this (and other) XML builders would be a workstream in its own right, and some more thinking is required on what I'd want maintain a cohesive-ish API across all the different kinds of XML that'd be getting generated.
TL;DR: feel free to ignore this comment - I basically just wrote it for future me 😅
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.
I think that this is a really good idea yeah, and I was wondering how bad it would be to prototype this. I like your idea of a LibrariesSvr4XmlBuilder
as a sort of maximally general way of doing this so if raw XML were available it could be given but otherwise it's just as convenient to use the builder. The builder could also have a helper function to take all the raw parameters to get_libraries_svr4
plus a closure and do it all on-the-fly.
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.
I am now tracking this and other instances of generating XML via #143
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.
Took a more proper look through. LGTM, barring those other discussions we had (wrt. updating the armv4t
example, removing the untested libraries
support, etc.)
Unknown how to actually get gdb to use it yet.
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.
Looks like there's one last TODO to resolve, but aside from that, this should be ready to rock
Ah, also: could you update some of the docs at https://docs.rs/gdbstub/latest/gdbstub/target/ext/section_offsets/index.html, given they relate to #20? |
Ok pushed up a resolution of the TODO plus some more docs on the |
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.
Nightly rustfmt
strikes again...
Barring that one fix, I think this is now merge-ready!
Oops, sorry about that! |
Awesome! Thanks again for the PR. I'll go ahead and publish EDIT: |
Thanks! |
A toy project I've been working on involves loading a custom kernel and a custom guest into a KVM instance, and these commands enable
gdb
to be able to debug both the guest and the kernel at the same time by reporting where both images are loaded in the address space.Initially this commit only implemented
qXfer:libraries:read
but it turned out that gdb never actually used that command. When implementingqXfer:libraries-svr4:read
, however, gdb invoked that automatically (presumably it's target-specific). I ended up including both here for completeness.This relates to #20 although I'm not sure if it closes it outright. The implementations here feel a little simplistic in terms of leaving all of the heavy-lifting to the taget. The implementations are copy/pasted from the memory-map command with edits for naming (as these are "just" returning XMLs as well).
An example gdb session with this support and using
qXfer:libraries-svr4:read
looks like:Previously there were enough hooks in
gdbstub
to debug either the guest or the kernel, but this enables gdb to see both and work symbolically with both.PS: very nice crate! The thorough documentation has made it super easy to integrate and the crate has clearly had a lot of thought put into it. A pleasure to use!
API Stability
Checklist
rustdoc
formatting looks good (viacargo doc
)examples/armv4t
withRUST_LOG=trace
+ any relevant GDB output under the "Validation" section below./example_no_std/check_size.sh
before/after changes under the "Validation" section belowexamples/armv4t
./example_no_std/check_size.sh
)Arch
implementationValidation
GDB output
armv4t output
Before/After `./example_no_std/check_size.sh` output
Before
After