-
Notifications
You must be signed in to change notification settings - Fork 262
elf: support external and supplementary debuginfo #427
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
Conversation
|
||
let stash = Stash::new(); | ||
let cx = Context::new(&stash, object)?; | ||
Some(Mapping { |
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.
This no longer uses Mapping::mk
because it needs to parse the Object
beforehand. Not sure if there is any way to restructure this to allow that.
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.
Awesome, thanks for this! This is definitely tricky enough though that I'd want some tests in CI for this. If this is something that varies across unix distributions it'd be great if a variety of containers could run as well and we'd run tests in those containers.
To figure that out, though, can you describe to me how this is expected to work? Does separate debuginfo always require the intervention of a separate tool? It looks like with objcopy
you're deleting the debuginfo from one file, adding it to another, and then adding a link of one form or another from the original file to the new file? I'm not entirely sure what dwz
is doing...
This may not be a scenario where tests can run via cargo test
, but it might be a situation where there needs to be an auxiliary crate where cargo run
runs all the tests so you can cargo build
, frob the debuginfo, and then run the binary manually to run all the tests.
Finally, it looks like this is handling /usr/lib
things, which is presumably for system-installed debuginfo? Could the tests link to some library which has debuginfo inserted there and assert that the filenames and line numbers appear ok?
src/symbolize/gimli/elf.rs
Outdated
path.push(hex(byte & 0xf)); | ||
} | ||
path.extend(BUILD_ID_SUFFIX); | ||
if let Some(mapping) = Mapping::new_debug(Path::new(OsStr::from_bytes(&path)), None) { |
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.
Are these paths and this scheme linux-specific? If so should this have an if cfg!
around it perhaps?
src/symbolize/gimli/elf.rs
Outdated
|
||
// Try to locate a supplementary object file. | ||
let tmp_stash = Stash::new(); | ||
if let Some(section) = object.section(&tmp_stash, ".gnu_debugaltlink") { |
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.
Oh deear, I can't seem to find documentation on this... Do you know of some good online docs to link to?
I'm curious about why this is handled here rather than above from the original object...
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 don't know of any documentation for the format of this. The only documentation I found is the dwz man page, but it doesn't give this detail. I based the code on what libbacktrace does, but gdb would have been a better source. And indeed, gdb does the path resolution differently so I'll have to fix that.
It is handled here because this is a section in the external debug file, not the original object.
src/symbolize/gimli/elf.rs
Outdated
|
||
// Try "/usr/lib/debug/parent/filename" | ||
let parent = parent.strip_prefix("/").unwrap(); | ||
let f = Path::new("/usr/lib/debug").join(parent).join(filename); |
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.
Similar to above, if this is linux-specific should this be wrapped in a if cfg!
?
Or perhaps something like a global AtomicBool
which indicates the presence of /usr/lib/debug
should prefix this so we don't keep trying on systems that don't have that directory?
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.
gdb uses a debug-file-directory
setting which defaults to "${libdir}/debug
. This whole external debug scheme is distribution specific, but I think most linux distributions use it, and I think FreeBSD does too. For those that do, I don't know if any deviate from /usr/lib/debug
. So we probably need a if cfg!
around the whole build-id/debuglink support. I think having an AtomicBool
for the presence of /usr/lib/debug
would be worth it too, since I assume someone might add build-id notes to binaries for purposes other than locating debug info.
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.
What about MinGW? Doesn't it support build-id/debuglink too?
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 don't know. Do you have a link to any information about that?
Sure, I can work on doing that.
That's pretty much it. https://sourceware.org/gdb/onlinedocs/gdb/Separate-Debug-Files.html describes this fairly well, but in summary:
Sounds doable.
Yeah. So firstly we'll need to ensure the container has system-installed debuginfo. And then we'll need to get a stack trace that contains that debuginfo. This is what I was asking cuviper about in #410, but I gave up and just used |
Perhaps we could just manually install the auxiliary crate debug info into the system dir. Is that something we can do for the containers in CI, and would that be a good enough test? |
Ok that all sounds good! I'm happy to help out with CI too if desired. |
fc20f4b
to
13b5ed9
Compare
This is ready for further review. I've added a single docker test as part of the main Ubuntu tests. It doesn't really need docker except that it lets us write 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.
This looks perfect, thanks so much!
I'm gonna fiddle locally a bit afterwards to see if I can fixup some of the transmutes to 'static
, I'd ideally like to contain that if possible. In any case though this all looks great to me, thanks again!
Fixes #410
Doesn't check the CRC for GNU debuglink. I'm not sure how important this is, and it would require adding a dependency (
crc32fast
?).Doesn't add any tests; I'm unsure how to test this in CI. I have manually tested that the
backtrace
example can give line number information for__libc_start_main
, which tests the build ID on Fedora, and the GNU debuglink on Ubuntu. I have manually tested the supplementary object file support using thebacktrace
andraw
examples after running the following commands: