[PATCH] D49137: [dsymutil] Implement DWARF5 accelerator tables.

[Frederic Riss via Phabricator via llvm-commits]

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