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

[Alexey Lapshin via Phabricator via llvm-commits]

avl added a comment.

> In dsymutil, the DWARFContext is not passed to the AddressManager.
> I could break down the DwarfFile constructor within DwarfLinkerForBinary::loadObject, and pass the second DwrafContext argument also to the AddressesMap as a new parameter, if we feel that is reasonable architecturally and would not lead to unknown race and/or runtime dependency issues (?)

It would be OK, though we should not change DWARFLinker::AddressesMap. With new class DwarfLinkerForBinaryRelocMap(described in comments) things would look like this: when DwarfLinkerForBinary::AddressManager  would be created it needs to get reference to the DwarfLinkerForBinaryRelocMap object. So that calls to saveValidRelocs()/updateRelinkedRelocationsWithUnitOffset() were forwarded to DwarfLinkerForBinaryRelocMap by DwarfLinkerForBinary::AddressManager. Also, DwarfLinkerForBinaryRelocMap should be initialized with DwarfContext to build units relocation map.

As to the DWARFLinker::AddressesMap, it should have only these three methods :   getLibraryInstallName(), saveValidRelocs(), updateRelinkedRelocationsWithUnitOffset().

> Isn't this already the purpose of clear() ? Like the following from DwarfLinkerForBinary.h
>
> void clear() override {
>
>   ValidDebugInfoRelocs.clear();
>   ValidDebugAddrRelocs.clear();
>
> }

you are right it might be used for resources cleanup. Though with the design using DwarfLinkerForBinaryRelocMap things may be left as it currently is. Because relocations would be kept separately and then DwarfLinkerForBinary::AddressManager may be safely deleted.



================
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"
----------------
Alpha wrote:
> 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)`
yes, but addValidRelocs is not used(and is not supposed to be used) anywhere inside lib/DWARFLinker/*. It is used inside dsymutil/DwarfLinkerForBinary. So this method could be declared and defined in DwarfLinkerForBinary::AddressManager. No neccessary to declare it in base class DWARFLinker::AddressesMap.


================
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;
+
----------------
Alpha wrote:
> 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
That means that addValidRelocs not needed in base class DWARFLinker::AddressesMap and it would be better to move it into the DwarfLinkerForBinary::AddressManager. There are no users of that method inside lib/DWARFLinker/*.

methods declared in DWARFLinker::AddressesMap could be used by DWARFLinker library. If they are not supposed to be used by DWARFLinker library then we should not add them into DWARFLinker::AddressesMap.

In this patch addValidRelocs is called through DWARFLinker::AddressesMap interface:

```
Obj->Addresses->addValidRelocs(static_cast<RelocMap *>(&RM));
```

instead DwarfLinkerForBinary::AddressManager  may get reference to  separate DwarfLinkerForBinaryRelocMap object. When saveValidRelocs is called, the relocations would be stored by that separate object. Which allows to store content of StoredValidDebugInfoRelocsMap not using base interface DWARFLinker::AddressesMap.


```
  class DwarfLinkerForBinaryRelocMap {

      void addValidRelocs(RelocMap *RM);

      void updateRelinkedRelocationsWithUnitOffset (uint64_t OriginalUnitOffset, uint64_t OutputUnitOffset);

      void saveValidRelocs(uint64_t OriginalUnitOffset, int64_t LinkedOffsetFixupVal, uint64_t InputAttrStartOffset,  uint64_t InputAttrEndOffset);

      void init ( DwarfContext& );

      DenseMap<uint64_t Offset, std::vector<ValidReloc>> StoredValidDebugInfoRelocsMap;
  };
    
  struct ObjectWithRelocMap {
    std::unique_ptr<OutDwarfFile> Object;
    DwarfLinkerForBinaryRelocMap OutRelocs;
  };
  std::vector<ObjectWithRelocMap> ObjectsForLinking;
  
  for (const auto &Obj : ObjectsForLinking) {
    Obj.OutRelocs.addValidRelocs(static_cast<RelocMap *>(&RM));
  }  

```


================
Comment at: llvm/include/llvm/DWARFLinkerParallel/AddressesMap.h:71
 
+  /// Save relocation values to be serialized
+  virtual void saveValidRelocs(int64_t LinkedOffset, uint64_t StartOffset,
----------------
Alpha wrote:
> 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.
The saved relocations are not the same as original, correct? DWARFLinker links debug info, when saveValidRelocs is called the offsets of relocations are updated with proper offsets inside linked debug info. It would be good if method name would reflect the fact that new relocations have new offsets. If "relink" is not a good word for it probably we could use some another.


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:605
+                                      CompileUnit::DIEInfo &MyInfo) {
+  MyInfo.FileName = RelocMgr.getFileName();
+}
----------------
Alpha wrote:
> 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
cloneDIE is the method of DIECloner class:

```
DIE *DWARFLinker::DIECloner::cloneDIE()

```

DIECloner has a member DWARFFile &ObjFile:


```
  class DIECloner {
   ...
    DWARFFile &ObjFile;
   ...
   }
```

So we can call ObjFile.Addresses->getLibraryInstallName(); within cloneDIE():

```
DIE *DWARFLinker::DIECloner::cloneDIE() {
...
   ObjFile.Addresses->getLibraryInstallName();
...
}
```


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1801
+  if (Tag == dwarf::DW_TAG_compile_unit) {
+    if (Info.FileName) {
+      if (!AttrInfo.AttrAppleOriginSeen) {
----------------
Alpha wrote:
> 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?
addressed in another comment.


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

https://reviews.llvm.org/D158124