Refactor debuginfo to use feature flags #470
Conversation
|
|
||
| [package.metadata.docs.rs] | ||
| all-features = true | ||
|
|
||
| [features] | ||
| default = ["breakpad", "elf", "macho", "ms", "sourcebundle", "wasm"] |
There was a problem hiding this comment.
ms is kind of an odd one in this list as it's the only one not named after a format. Probably pdb makes more sense? Though you also chose elf and macho instead of dwarf so you could make an argument for pe I guess.
There was a problem hiding this comment.
Yah, this was definitely a bit of a weird split, but I think it makes sense because pdb and pe would presumably always go together, whereas dwarf can be used separately with both elf, macho, and wasm. That being said I could of course split it into two features.
| @@ -430,6 +430,7 @@ pub struct FileInfo<'data> { | |||
|
|
|||
| impl<'data> FileInfo<'data> { | |||
| /// Creates a `FileInfo` from a joined path by trying to split it. | |||
| #[cfg(any(feature = "breakpad", feature = "ms", feature = "sourcebundle"))] | |||
There was a problem hiding this comment.
I'm probably going to display my lack of knowing the symbolic API in my comments here... but why is this only for some formats?
Also, this is a pub(crate) function, so I would expect the compiler to be smart enough to not include it if it isn't used due to the disabled features. Is it really needed to annotate this explicitly?
There was a problem hiding this comment.
Yes, this was only used with these 3 features enabled, the attribute is merely to silence dead_code warnings.
| if self.finished { | ||
| None |
There was a problem hiding this comment.
is this branch possible? doesn't matter much though, it's harmless
There was a problem hiding this comment.
No idea, this code was moved from private.rs since this struct is only used by the breakpad parser.
There was a problem hiding this comment.
You're right @flub, this should be impossible. If self.finished were true we'd already have returned in line 52.
| use crate::shared::FunctionStack; | ||
|
|
||
| /// This is a fake BcSymbolMap used when macho support is turned off since they are unfortunately | ||
| /// part of the dwarf interface |
There was a problem hiding this comment.
maybe the api of BcSumbolMap was a poor choice and personally i'd be open to changing it if it results in something better.
The toplevel API is DwarfDebugSession::load_symbolmap() I think, which you already made conditional to macho. I think if you might be able to remove the bcsymbolmap arg from UnitRef::resolve_function_name and instead put it as a field in the UnitRef struct if macho is enabled and then it can probably be made to work with some more conditional code.
This way BcSymbolMap could completely disappear from the public API if macho is not enabled? What do you think?
There was a problem hiding this comment.
I initially started on that route, but it seemed like I was changing a bit too much and figured such a change could be better left for later.
As mentioned in #469, the debuginfo crate assumed it was being used from sentry-cli and thus included support for all of the formats and all of the dependencies used when parsing/handling each format, which was...excessive, as there are essentially zero dependencies that are shared between all formats.
Probably the biggest knock on effect of this change was that goblin only provides the peek/Hint functionality if all formats are enabled, but since I think it was cleaner to only enable the format features specifically enabled in the debuginfo crate, I had to just use the underlying functions/constants that were being wrapped.
There was also a rather unfortunate coupling between DWARF and BCSymbolMap, so when
machois not actually enabled, it just provides a null implementation of BCSymbolMap so that the API didn't need to change, but that shouldn't matter as it will always beNonein the case you aren't using Dwarf from macho files.Resolves: #469