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

[Alpha Abdoulaye via Phabricator via llvm-commits]

Alpha added 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"
----------------
avl wrote:
> Looks like nothing from this file is used in DWARFLinker. So it(include and file itself) might be removed.
We need the RelocMap type for `addValidRelocs(RelocMap *RM)`


================
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;
+
----------------
avl wrote:
> it looks like it is not used in DWARFLinker, probably we can remove it from DWARFLinker.h.
`addValidRelocs` is called in `DwarfLinkerForBinary::emitRelocations` to add the relocations to the RelocationMap


================
Comment at: llvm/include/llvm/DWARFLinkerParallel/AddressesMap.h:71
 
+  /// Save relocation values to be serialized
+  virtual void saveValidRelocs(int64_t LinkedOffset, uint64_t StartOffset,
----------------
avl wrote:
> 
Updating the comment, but just to clarify, `saveValidRelocs` doesn't "relink" anything, it merely finds relocations that are applied to the object, store them, so they can be serialized later.


================
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;
----------------
avl wrote:
> probably relinkAndSaveValidRelocs would be more presize naming?
> 
Addressed in the comment above


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:605
+                                      CompileUnit::DIEInfo &MyInfo) {
+  MyInfo.FileName = RelocMgr.getFileName();
+}
----------------
avl wrote:
> If I understood correctly, no need to store RelocMgr.getFileName() into MyInfo.FileName. It could be used directly from RelocMgr.
We need the name to be accessible within `cloneDie`, which is why we pass it via `DIEInfo`,  to add the `APPLE_origin` attribute


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1801
+  if (Tag == dwarf::DW_TAG_compile_unit) {
+    if (Info.FileName) {
+      if (!AttrInfo.AttrAppleOriginSeen) {
----------------
avl wrote:
> we can use RelocMgr.getFileName() here(instead of Info.FileName), correct?
Unless I'm mistaken, we do not have access to the AddressManager from within `cloneDIE`.
The other alternative would be adding the libInstallName (which right now is specific to dsymutil and its AddressManager) to the parent `AddressMap`, but is this something we really want to do?


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

https://reviews.llvm.org/D158124