Support for DWARFv5 embedded source code extension #849
Conversation
There was a problem hiding this comment.
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.
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.
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.
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.
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.
This reverts commit 7dc28dd.
|
@Mrmaxmeier I'm sorry, we'll have to revert this. The check for whether a file has sources is inordinately expensive in terms of memory and is causing issues in our infrastructure. |
|
Welp, sorry about that 🫠 |
|
Not at all, thank you for the contribution. Sorry we can't support it at the time :/ |
) This reverts the "feature part" of commit 7dc28dd (#849). The dep updates can stay. Unfortunately opening a debug session to check for the presence of sources is prohibitively expensive in terms of memory (going from ~450MB to ~3.7GB in a Python process that calls Object.features on an example 1.3GB file). In order to support this we'll have to find a more parsimonious way to check if a file contains sources.
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_paththough. 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 ingimlithat has not made it into a release yet. I'll update the PR once a new version ofgimliis available on crates.io.Thanks!