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

[Alexey Lapshin via Phabricator via llvm-commits]

avl added a comment.

Thank you for the update! Please consider inline comments.



================
Comment at: llvm/include/llvm/DWARFLinker/DWARFLinker.h:17
 #include "llvm/DWARFLinker/DWARFLinkerCompileUnit.h"
+#include "llvm/DWARFLinker/DWARFLinkerRelocs.h"
 #include "llvm/DebugInfo/DWARF/DWARFContext.h"
----------------
Looks like nothing from this file is used in DWARFLinker. So it(include and file itself) might be removed.


================
Comment at: llvm/include/llvm/DWARFLinker/DWARFLinker.h:40
+  uint64_t InputAttrEndOffset;
+};
+
----------------
AttributeLinkedOffsetFixup is local to DWARFLinker.cpp no need to place it in a header. It would be OK to declare it inside DWARFLinker.cpp before DWARFLinker::cloneDIE.


================
Comment at: llvm/include/llvm/DWARFLinker/DWARFLinker.h:75
+  /// Returns the file name associated to the AddessesMap
+  virtual std::optional<StringRef> getFileName() = 0;
+
----------------
Can we use more descriptive comment and name here, please? Like getMergedLibraryName() or getLibraryInstallName() or getAppleOriginName() or something else?



================
Comment at: llvm/include/llvm/DWARFLinker/DWARFLinker.h:89
+  /// Add the valid relocations to be serialized to the relocation map
+  virtual void addValidRelocs(RelocMap *RM) = 0;
+
----------------
it looks like it is not used in DWARFLinker, probably we can remove it from DWARFLinker.h.


================
Comment at: llvm/include/llvm/DWARFLinker/DWARFLinkerCompileUnit.h:151
+  void setOriginObject() { AddedOriginObject = true; }
+  bool addedOriginObject() const { return AddedOriginObject; }
+
----------------
AddedOriginObject used for offset calculation previously. Now it looks unused and could be removed completely.


================
Comment at: llvm/include/llvm/DWARFLinkerParallel/AddressesMap.h:71
 
+  /// Save relocation values to be serialized
+  virtual void saveValidRelocs(int64_t LinkedOffset, uint64_t StartOffset,
----------------



================
Comment at: llvm/include/llvm/DWARFLinkerParallel/AddressesMap.h:72
+  /// Save relocation values to be serialized
+  virtual void saveValidRelocs(int64_t LinkedOffset, uint64_t StartOffset,
+                               uint64_t EndOffset) = 0;
----------------
probably relinkAndSaveValidRelocs would be more presize naming?



================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:605
+                                      CompileUnit::DIEInfo &MyInfo) {
+  MyInfo.FileName = RelocMgr.getFileName();
+}
----------------
If I understood correctly, no need to store RelocMgr.getFileName() into MyInfo.FileName. It could be used directly from RelocMgr.


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1801
+  if (Tag == dwarf::DW_TAG_compile_unit) {
+    if (Info.FileName) {
+      if (!AttrInfo.AttrAppleOriginSeen) {
----------------
we can use RelocMgr.getFileName() here(instead of Info.FileName), correct?


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1894
+    ObjFile.Addresses->saveValidRelocs(
+        Unit.getStartOffset() + F.LinkedOffsetFixupVal, F.InputAttrStartOffset,
+        F.InputAttrEndOffset);
----------------
probably move Unit.getStartOffset() from this place into the point where F.LinkedOffsetFixupVal is calculated. That looks a bit unclear why it is added here:

```
CurAttrFixup.LinkedOffsetFixupVal =
        Unit.getStartOffset() + OutOffset - CurAttrFixup.InputAttrStartOffset;
```


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

https://reviews.llvm.org/D158124