Skip to content
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 qOffsets packet #30

Merged
merged 4 commits into from Sep 28, 2020
Merged

Conversation

mchesser
Copy link
Contributor

Implement the qOffsets packet allowing the stub to report section offsets back to gdb so that debug symbols are resolved to the correct addresses if the target performs any relocations.

Partially addresses: #20


I saw the comment, about in progress API changes -- if necessary, I'm happy to update my branch once the API changes are complete.

This packet allows the stub to report section offsets back to gdb so
that debug symbols are resolved to the correct addresses if the target
performs any relocations
@daniel5151 daniel5151 self-requested a review September 24, 2020 14:12
Copy link
Owner

@daniel5151 daniel5151 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, this looks great!

The inline comments are mostly around naming / docs, but the core implementation is spot-on. Couldn't have written it better myself!

And don't worry about rebasing on-top of my local changes. I've been bike-shedding how the project's modules are laid out, so it wouldn't be a particularly easy rebase. I'll just integrate these changes myself when I get the chance.


Regarding the doc comments: While it's tempting to mention the underlying qOffsets packet directly in the docs, I think it'd be best to re-word the docs to talk about the provided functionality, rather than the underlying implementation.

e.g: instead of "Handle the qOffsets command", maybe it could be "Report the section offsets the target used when relocating the running binary" (or something similar).

After all, one of the guiding principles behind gdbstub is to abstract over the gnarly underlying GDB protocol, and provide a "friendly" API on-top of the ~40 year old protocol 😄

src/target/mod.rs Outdated Show resolved Hide resolved
src/target/ext/offsets.rs Outdated Show resolved Hide resolved
src/target/ext/offsets.rs Outdated Show resolved Hide resolved
src/gdbstub_impl/mod.rs Outdated Show resolved Hide resolved
src/target/ext/mod.rs Outdated Show resolved Hide resolved
src/target/ext/offsets.rs Outdated Show resolved Hide resolved
src/target/ext/offsets.rs Outdated Show resolved Hide resolved
@mchesser
Copy link
Contributor Author

So I was slightly wrong about the semantics of the different offset responses -- I've been working with "stripped" binaries that are missing section metadata and only have segment information available, so I misunderstood the different between section offsets and segment address.

I also finally found the proper modern way of indicating the base address of the dynamic linker for System V binaries (where GDB is capable of extracting the link_map from memory). It appears there is another optional command that you can respond with qXfer:auxv:read. The auxiliary vector contains a entry (AT_BASE) that represents the exact address that I want to notify GDB of, you can see here: https://github.com/bminor/binutils-gdb/blob/9e790a80160676e7fd3fb8be6cf3c1c77d9ded81/gdb/solib-svr4.c#L2349 that it is used for obtaining the library list.

The commit where they added qOffsets support lldb is enlightening, looks like currently qOffsets is really only supported because that is what QEMU uses, everyone else responds with auxv.

I've tried adding some docs pointing readers to the correct information, but let me know what you think.

Copy link
Owner

@daniel5151 daniel5151 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhhh, the plot thickens!
Thanks for drilling down into the details, I really appreciate it.

Could you update #20 with a quick summary of what you've discovered regarding qXfer::auxv::read and qXfer::library::read? This could be in the form of a new comment, or preferably, by updating the top-level comment (for better visibility).

In terms of code/docs, I've got a few more minor nits (gotta remember to add those periods!), but aside from those, the PR LGTM 👍

src/target/mod.rs Outdated Show resolved Hide resolved
src/target/ext/section_offsets.rs Outdated Show resolved Hide resolved
src/target/ext/section_offsets.rs Outdated Show resolved Hide resolved
src/gdbstub_impl/mod.rs Outdated Show resolved Hide resolved
src/target/ext/section_offsets.rs Outdated Show resolved Hide resolved
src/target/ext/section_offsets.rs Outdated Show resolved Hide resolved
src/target/ext/section_offsets.rs Outdated Show resolved Hide resolved
@daniel5151 daniel5151 merged commit 1d0df99 into daniel5151:master Sep 28, 2020
@daniel5151
Copy link
Owner

Awesome work!
Thanks again for your contribution! 🎉

I've got a few more unmerged changes sitting around on my local machine (such as target extended-remote support and improved error handling), so once those are cleaned up + merged into master, I'll probably go ahead and publish 0.4.0 (possibly with a -beta1 tag) crates.io.

Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants