-
-
Notifications
You must be signed in to change notification settings - Fork 76
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
Support for DWARFv5 embedded source code extension #849
Conversation
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 very good. Too bad its not possible to quickly check whether source is available. However I believe we only check that once when files are uploaded to Sentry or downloaded in Symbolicator.
I also think that native only matches on full absolute paths. When in doubt, its possible to double-check by looking at how a SourceBundle
is being constructed from an Object
. In that case it might also be interesting to update that logic to prefer the embedded source vs loading it from disk.
symbolic-debuginfo/src/dwarf.rs
Outdated
) -> Result<Option<SourceFileDescriptor<'_>>, DwarfError> { | ||
// First: see if there's an exact match for the provided path |
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 believe its okay to use a OnceLock
here to collect these into a HashMap
for faster lookups.
Usually we want to optimize for lookups here.
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.
Okay 👍
I'd like to avoid collecting all source code in a big map of owned strings but I'm not sure if that's doable given that the DwarfFileIterator<'_>
returned by files(&self)
give out lifetimes shorter than 'data
IIUC.
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's your take on this implementation? https://github.com/getsentry/symbolic/pull/849/files#diff-ffd2feab131b47be099bceda4b175052d29c014642276be0168bd3d782a569f9L1351
I'm using a map now, but still iterate over the DWARF units on lookup to avoid dealing with owned strings and lifetime issues.
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 thats a reasonable compromise there given that fixing all the lifetimes is probably way too complicated for now.
Because of getsentry/symbolic#849, this means Elf/Wasm debug files can now contain source information. We update `FileType::sources` to reflect this.
Because of getsentry/symbolic#849, Elf/Wasm debug files can now contain source information. We update `FileType::sources` to reflect this.
Hi,
this implements support for source code embedded directly inside of the DWARF debug info. (Fixes #835)
I'm not sure about the logic in
source_by_path
though. Should it only match full paths or relative paths instead?My use-case is fine with either way since I'm iterating over the files in the debug session directly.
This PR currently depends on support ingimli
that has not made it into a release yet. I'll update the PR once a new version ofgimli
is available on crates.io.Thanks!