Use pdb_addr2line::TypeFormatter. #426
Conversation
There was a problem hiding this comment.
This looks good to me, but I think @Swatinem will have to have a look when he gets back.
There was a problem hiding this comment.
lgtm, though I will check if we need this to be Send. (If so, might as well add a test for that separately)
Also @jan-auer voiced some concerns about pulling in another dependency, and suggested to maybe move all of this into the pdb crate.
symbolic-debuginfo/src/pdb.rs
Outdated
| module_infos: Vec<LazyCell<Option<ModuleInfo<'d>>>>, | ||
| /// Cache for module by name lookup for cross module imports. | ||
| module_exports: RefCell<BTreeMap<pdb::ModuleRef, Option<pdb::CrossModuleExports>>>, | ||
| modules: Rc<Vec<Module<'d>>>, |
There was a problem hiding this comment.
Let me double-check if its okay to make this !Send (even though the RefCells previously made this !Sync).
(This also makes me wonder why TypeFormatter needs a Rc<Vec<_>> and is not satisfied with a &[_] instead.)
There was a problem hiding this comment.
(This also makes me wonder why
TypeFormatterneeds aRc<Vec<_>>and is not satisfied with a&[_]instead.)
TypeFormatter would be happy with a slice, if the slice can live for the entire lifetime of the TypeFormatter. But creating such a slice is hard for PdbDebugInfo::build; it currently has the list of modules in a local variable in that function, and the slice can't refer to that local variable. And we also can't store the list of modules on PdbStreams because the module list has a lifetime dependency on streams.debug_info.
There was a problem hiding this comment.
I took another look at this. A slice is indeed not an option: Let's say it was &'list [Module<'dbi>], what would those two lifetimes be? Who would own the Vec, and who would own the DebugInformation?
But at least I've eliminated the Rc now:
I've changed Rc<Vec<Module<'d>> to Vec<Module<'d>>, i.e. I've given the TypeFormatter full ownership of the Vec instead of just shared ownership.
Now everyone who wants to access the list of modules needs to go through the TypeFormatter - TypeFormatter::modules() exposes the slice.
|
Thanks for the review! |
|
I was waiting on @jan-auer to look at this again, but he seems to be a bit occupied. |
This replaces our TypeFormatter with the one from the pdb_addr2line crate, which is more fully-featured. Most importantly, it supports function arguments, both for inlined functions (which use IdIndex) and for procedures (which use TypeIndex). Fixes getsentry#423. As a consequence of this commit, function names for procedures from PDBs will now always contain arguments. These arguments are obtained from the type data, and not from demangling. Consequently, their presence is unaffected by demangler flags, and users cannot opt out of the arguments. Similarly, we never print the return type, and there is no way for users to opt in to showing the return type. This commit also moves some things related to ModuleInfo loading around. This was needed to make the lifetimes work out successfully. Previously, the TypeFormatter was created just for the duration of the function name printing, and it could depend on fields of PdbDebugInfo; this was ok because the ItemMap caching was outside of the TypeFormatter. Now all caching is inside of TypeFormatter, so we don't want to re-create the formatter repeatedly, and instead store it on the PdbDebugInfo. But this means that some things need to be stored *outside* of PdbDebugInfo so that TypeFormatter can have a lifetime dependency on those things, specifically on ModuleInfo and StringTable. So these things move to the PdbStreams struct. The ModuleInfo caching now uses a FrozenMap (from the elsa crate) instead of a Vec of LazyCells. The Vec needed to know its size ahead-of-time and couldn't grow, but PdbStreams cannot know the number of modules at creation time. (At least not in a way where the results of the module iteration are passed on to the other parts of the system that need the module header list.)
|
I've rebased the PR. I've also eliminated the use of @Swatinem, could you take another look? This PR has become relevant to me again because it's blocking mozilla/dump_syms#406. |
Codecov Report
@@ Coverage Diff @@
## master #426 +/- ##
==========================================
+ Coverage 69.19% 69.47% +0.28%
==========================================
Files 84 84
Lines 16873 16638 -235
==========================================
- Hits 11675 11560 -115
+ Misses 5198 5078 -120 |
This replaces symbolic's TypeFormatter with the one from the pdb_addr2line
crate, which is more fully-featured. Most importantly, it supports
function arguments, both for inlined functions (which use IdIndex) and
for procedures (which use TypeIndex).
Fixes #423.
As a consequence of this commit, function names for procedures from PDBs
will now always contain arguments. These arguments are obtained from the
type data, and not from demangling. Consequently, their presence is
unaffected by demangler flags, and users cannot opt out of the arguments.
Similarly, we never print the return type, and there is no way for users
to opt in to showing the return type.
This commit also moves some things related to ModuleInfo loading around.
This was needed to make the lifetimes work out successfully.
Previously, the TypeFormatter was created just for the duration of the
function name printing, and it could depend on fields of PdbDebugInfo;
this was ok because the ItemMap caching was outside of the TypeFormatter.
Now all caching is inside of TypeFormatter, so we don't want to re-create
the formatter repeatedly, and instead store it on the PdbDebugInfo. But
this means that some things need to be stored outside of PdbDebugInfo
so that TypeFormatter can have a lifetime dependency on those things,
specifically on ModuleInfo and StringTable. So these things move to the
PdbStreams struct.
The ModuleInfo caching now uses a FrozenMap (from the elsa crate) instead
of a Vec of LazyCells. The Vec needed to know its size ahead-of-time and
couldn't grow, but PdbStreams cannot know the number of modules at creation
time. (At least not in a way where the results of the module iteration are
passed on to the other parts of the system that need the module header
list.)
r? @Swatinem