[PATCH] D13037: dsymutil: Follow references to clang modules and recursively clone the debug info

[Frederic Riss via llvm-commits]

friss added inline comments.

================
Comment at: test/tools/dsymutil/X86/modules.map:1-8
@@ +1,8 @@
+---
+triple:          'x86_64-apple-darwin'
+objects:         
+  - filename:        /Inputs/modules.macho.x86_64
+    timestamp:       1442276341
+    symbols:         
+      - { sym: _main, objAddr: 0x0000000000000000, binAddr: 0x0000000100000F90, size: 0x0000001E }
+...
----------------
The name of that file is a bit confusing because of the context it addresses. I'd like to see the actual module map file committed, and if you could rework that test to look like the other newer ones (and thus get rid of this map in favor of dummy-debug-map.map), this would be great.

================
Comment at: test/tools/dsymutil/X86/modules.test:3
@@ +2,3 @@
+RUN: mkdir -p %T/modules
+RUN: cat %p/modules.map > %T/modules/modules.map
+RUN: llvm-dsymutil -y -f -oso-prepend-path=%p/.. %T/modules/modules.map -o %t.dSYM
----------------
Is that copying really necessary?

================
Comment at: test/tools/dsymutil/X86/modules.test:4-5
@@ +3,4 @@
+RUN: cat %p/modules.map > %T/modules/modules.map
+RUN: llvm-dsymutil -y -f -oso-prepend-path=%p/.. %T/modules/modules.map -o %t.dSYM
+RUN: llvm-dwarfdump --debug-dump=info %t.dSYM | FileCheck %s
+
----------------
I usually ditch the tmp file in favor of -o - | llvm-dwarfdump

================
Comment at: tools/dsymutil/DwarfLinker.cpp:395
@@ +394,3 @@
+  for (auto &I : Info) {
+    I.Keep = true;
+    const auto *DIE = OrigUnit.getDIEAtIndex(Idx++);
----------------
I think that for your work, this line would be sufficient in this loop. The rest will be needed for --update support, but I need to first figure out accelerator tables.

================
Comment at: tools/dsymutil/DwarfLinker.cpp:1145-1146
@@ -1098,1 +1144,4 @@
 
+  void reportWarning(const Twine &Warning, const DWARFUnit *Unit = nullptr,
+                     const DWARFDebugInfoEntryMinimal *DIE = nullptr) const;
+
----------------
Can you do that refactoring in a preparatory NFC commit?

================
Comment at: tools/dsymutil/DwarfLinker.cpp:1479
@@ -1426,1 +1478,3 @@
+  /// FIXME: We may need to use something more resilient than the PCM filename.
+  StringMap<bool> ClangModules;
 };
----------------
StringSet?

================
Comment at: tools/dsymutil/DwarfLinker.cpp:1498-1499
@@ -1442,3 +1497,4 @@
 /// \returns null if resolving fails for any reason.
-const DWARFDebugInfoEntryMinimal *DwarfLinker::resolveDIEReference(
+static const DWARFDebugInfoEntryMinimal *resolveDIEReference(
+    const DwarfLinker &Linker, MutableArrayRef<CompileUnit> Units,
     const DWARFFormValue &RefValue, const DWARFUnit &Unit,
----------------
Making that a static helper could be a separate NFC commit too, right?

================
Comment at: tools/dsymutil/DwarfLinker.cpp:2652-2655
@@ -2594,3 +2651,6 @@
   DataExtractor Data = U.getDebugInfoExtractor();
-  uint32_t NextOffset = U.getDIEAtIndex(Idx + 1)->getOffset();
+  uint32_t NextOffset =
+    (Idx + 1 < U.getNumDIEs())
+    ? U.getDIEAtIndex(Idx + 1)->getOffset()
+    : U.getNextUnitOffset();
   AttributesInfo AttrInfo;
----------------
Took me some time to remember why we need that (or rather why we didn't need it before). Usually, the last DIE is always a NULL. It can happen that the input file is just a single compile_unit DIE though and that would crash here because we don't have a terminating NULL. A comment explaining that we should do that optimization would be nice.  

================
Comment at: tools/dsymutil/DwarfLinker.cpp:3118-3120
@@ -3057,1 +3117,5 @@
 
+bool DwarfLinker::registerModuleReference(
+    const DWARFDebugInfoEntryMinimal &CUDie, const DWARFUnit &Unit,
+    DebugMap &ModuleMap, unsigned Indent) {
+  std::string PCMfile =
----------------
It might help the reader if there was a comment describing the layout of debug info containing a module reference so that it's trivial to understand how this function works.

================
Comment at: tools/dsymutil/DwarfLinker.cpp:3173
@@ +3172,3 @@
+  BinaryHolder ObjHolder(Options.Verbose);
+  auto &Obj = ModuleMap.addDebugMapObject(Path, sys::TimeValue::MinTime());
+  auto ErrOrObj = loadObject(ObjHolder, Obj, ModuleMap);
----------------
this should be sys::TimeValue::PosixZeroTime().

================
Comment at: tools/dsymutil/DwarfLinker.cpp:3187
@@ +3186,3 @@
+    auto *CUDie = CU->getUnitDIE(false);
+    if (false && Options.Verbose) {
+      outs().indent(Indent);
----------------
That condition seems unlikely to trigger.

================
Comment at: tools/dsymutil/dsymutil.h:31
@@ -30,3 +30,3 @@
   bool NoODR;    ///< Do not unique types according to ODR
-
+  std::string PrependPath; //< -oso-prepend-path
   LinkOptions() : Verbose(false), NoOutput(false) {}
----------------
Separate NFC patch?


http://reviews.llvm.org/D13037