[PATCH] D49137: [dsymutil] Implement DWARF5 accelerator tables.
Frederic Riss via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jul 17 09:40:39 PDT 2018
friss added inline comments.
================
Comment at: llvm/tools/dsymutil/DwarfLinker.cpp:2094
for (const auto &CU : DwarfContext->compile_units()) {
- maybeUpdateMaxDwarfVersion(CU->getVersion());
-
+ updateDwarfVersion(CU->getVersion());
// Recursively get all modules imported by this one.
----------------
Why would we need to update the dwarf versions, but not the Accelerator type here?
================
Comment at: llvm/tools/dsymutil/DwarfLinker.cpp:2305-2311
+ for (LinkContext &LinkContext : ObjectContexts) {
+ if (!LinkContext.ObjectFile)
+ continue;
+ updateAccelKind(*LinkContext.DwarfContext);
+ for (const auto &CU : LinkContext.DwarfContext->compile_units())
+ updateDwarfVersion(CU->getVersion());
+ }
----------------
This new iteration is needed because of the registerModuleReference call bellow, right? I'm not sure this is sufficient as I think the registerModuleReference can pull in new debug information from dependencies of the referenced modules.
================
Comment at: llvm/tools/dsymutil/DwarfLinker.cpp:2313-2327
+ if (Options.TheAccelTableKind == AccelTableKind::Default) {
+ // If we've only seen either Apple or DWARF accelerator tables, we pick the
+ // respective one. If we've seen neither or both, the choice is determined
+ // based on the lower DWARF version encountered.
+ if ((HasAppleAccelTables && HasDwarfAccelTables) ||
+ (!HasDwarfAccelTables && !HasAppleAccelTables))
+ Options.TheAccelTableKind =
----------------
I'd rather we are even more conservative. I'd prefer if we chose Dwarf5 tables only if all the input is Dwarf5 and none of it has Apple tables.
================
Comment at: llvm/tools/dsymutil/DwarfLinker.h:164-166
/// Clear compile units and ranges.
void Clear() {
Ranges.clear();
----------------
JDevlieghere wrote:
> labath wrote:
> > I don't know why you deleted the other clear statement, but in any case, the comment is now out of date.
> We can't clear the CUs yet as we need them during emission of the debug_names section to map DIEs to CUs.
What kind of memory does the CU hold? Do we see an increase in overall memory consumption?
https://reviews.llvm.org/D49137
More information about the llvm-commits
mailing list