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

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere marked 5 inline comments as done.
JDevlieghere added a comment.

In https://reviews.llvm.org/D49137#1165178, @friss wrote:

> maybeUpdateAccelKind() will make a final call about the kind of accelerator tables on the first object it encounters which has accelerator tables. This seems pretty arbitrary. I'd prefer if we considered Apple table the default unless all files are Dwarf5 and have Dwarf5 accelerator tables.
>  Other than that, the implementation looks good. I'd like to know how the LLDB test suite behaves if you force it to use the new accelerator tables.


Failing Tests (5):

  lldb-Suite :: python_api/class_members/TestSBTypeClassMembers.py
  lldb-Suite :: tools/lldb-server/TestAppleSimulatorOSType.py

The python_api is expected, as it also failed before my change in clang to emit the members as children of the DIE (which is missing in this run, therefore making the failure expected). The last failure was unrelated, as all categories failed.



================
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());
+  }
----------------
friss wrote:
> 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.
Correct. The issue is that `loadClangModule` calls `cloneAllCompileUnits` which in turn wants to `emitAcceleratorEntriesForUnit`. Hence at that point we need to have what type of accelerator table we are going to emit, otherwise we might change the decision after the fact. Please see my latest update of the patch for all the details.


================
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 =
----------------
friss wrote:
> 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.
This is now the case in the latest revision of this patch. As soon as we see an Apple accelerator table we update the default to that. 


================
Comment at: llvm/tools/dsymutil/DwarfLinker.h:164-166
     /// Clear compile units and ranges.
     void Clear() {
       Ranges.clear();
----------------
friss wrote:
> 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?
Instead of keeping the whole CU we now only tack the necessary info in EmittedCU (a pointer and an unsigned). 


https://reviews.llvm.org/D49137