[PATCH] D158124: [dsymutil] Add support for mergeable libraries

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added inline comments.


================
Comment at: llvm/docs/CommandGuide/dsymutil.rst:37
+
+ Specify the build variant suffix used to build the executabe file.
+
----------------
This doesn't really explain anything as much as rephrase the command option. What is the  build variant suffix? 


================
Comment at: llvm/docs/CommandGuide/dsymutil.rst:46
+
+ Specify a directory that contain dSYM files to search for.
+
----------------
Can you add a little bit more detail, such as that it's used with relinkable libraries? 


================
Comment at: llvm/include/llvm/DWARFLinker/DWARFLinker.h:762
+      /// Is there a DW_AT_APPLE_origin in the CU?
+      bool AttrAppleOriginSeen = false;
+
----------------
`HasAppleOrigin` would be more in line with the rest of the code base.


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1762-1764
+    if (FinalAttrSize != 0 && ObjFile.Addresses->needToSaveValidRelocs()) {
+      AttributesFixups.push_back(CurAttrFixup);
+    }
----------------
No braces around single-line if blocks.


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1770
   uint16_t Tag = InputDIE.getTag();
+  // Add origin attribute to Compile Unit die.
+  if (Tag == dwarf::DW_TAG_compile_unit) {
----------------
This seems incomplete looking at the code below. Should it say something like "Add origin attribute to Compile Unit die if we have an install name and the DWARF doesn't have the DW_AT_APPLE_origin attribute yet". 


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1771-1781
+  if (Tag == dwarf::DW_TAG_compile_unit) {
+    if (LibraryInstallName.has_value()) {
+      if (!AttrInfo.AttrAppleOriginSeen) {
+        auto StringEntry = DebugStrPool.getEntry(LibraryInstallName.value());
+        Die->addValue(DIEAlloc, dwarf::Attribute(dwarf::DW_AT_APPLE_origin),
+                      dwarf::DW_FORM_strp, DIEInteger(StringEntry.getOffset()));
+        AttrInfo.Name = StringEntry;
----------------
Instead of 3 levels of nesting, how about

```
const bool NeedsAppleOrigin = Tag == dwarf::DW_TAG_compile_unit && LibraryInstallName.has_value() && AttrInfo.AttrAppleOriginSeen;
if (NeedsAppleOrigin) {
  ...
}
```

or even just inline the condition. 


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1858-1860
+  for (AttributeLinkedOffsetFixup &F : AttributesFixups) {
+    F.LinkedOffsetFixupVal += AbbrevNumberSize;
+  }
----------------
Nit: No braces


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1862-1866
+  for (AttributeLinkedOffsetFixup &F : AttributesFixups) {
+    ObjFile.Addresses->updateAndSaveValidRelocs(
+        /*IsDWARF5*/ false, Unit.getOrigUnit().getOffset(),
+        F.LinkedOffsetFixupVal, F.InputAttrStartOffset, F.InputAttrEndOffset);
+  }
----------------
No braces


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1864
+    ObjFile.Addresses->updateAndSaveValidRelocs(
+        /*IsDWARF5*/ false, Unit.getOrigUnit().getOffset(),
+        F.LinkedOffsetFixupVal, F.InputAttrStartOffset, F.InputAttrEndOffset);
----------------
Hardcoding this to not be DWARF5 seems suspicious. If this is the right thing to do, this definitely needs a comment explaining why.


================
Comment at: llvm/tools/dsymutil/DwarfLinkerForBinary.cpp:1232-1235
+  for (const std::unique_ptr<DWARFUnit> &CU : Context.compile_units()) {
+    StoredValidDebugInfoRelocsMap.insert(
+        std::make_pair(CU->getOffset(), std::vector<ValidReloc>()));
+  }
----------------
Nit: braces. Same below. 


================
Comment at: llvm/tools/dsymutil/DwarfLinkerForBinary.h:49-51
+  // Map compilation unit offset to the valid relocations to store
+  DenseMap<uint64_t, std::vector<ValidReloc>> StoredValidDebugInfoRelocsMap;
+  DenseMap<uint64_t, std::vector<ValidReloc>> StoredValidDebugAddrRelocsMap;
----------------
Doxygen group


================
Comment at: llvm/tools/dsymutil/MachODebugMapParser.cpp:219-221
+/// This function resets the state of the
+/// parser that was referring to the last object file and sets
+/// everything up to add symbols to the new one.
----------------
Nit: reflow?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158124/new/

https://reviews.llvm.org/D158124