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

[Alexey Lapshin via Phabricator via llvm-commits]

avl added a comment.

In D158124#4632826 <https://reviews.llvm.org/D158124#4632826>, @Alpha wrote:

> Address inline comments.
> However, regarding all the `DwarfLinkerForBinaryRelocMap` and the discussion about wether or not some logic should be in DwrafLinker (as they're all related).
> I agree in the sense that my initial incline was to limit all that logic to AddressManager.
> The issue here is that with `DwarfLinkerForBinary`, we have access to `Obj->Addresses`, which is the `AddressesMap`, and there is no reasonable way do downcast it to `AddressManager` (as dyn_cast doesn't work here), which is why I had to resort to have it being part of the `AddressesMap` map interface and call via virtual dispatch

As you said, to make possible to call addValidRelocs through Obj->Addresses it is neccessary to put some unrelated data into the base interface DWARFLinker::AddressesMap. This is not a good thing. DWARFLinker library does not call to addValidRelocs(static_cast<RelocMap *>(&RM)),
it does not use "class Relocation" and "class RelocMap". We should not make these classes known to DWARFLinker library.

We can hide all RelocMap specifics without downcasting DWARFLinker::AddressesMap to DwarfLinkerForBinary::AddressManager.
DwarfLinkerForBinary::AddressManager can delegate handling to the separate class(f.e. DwarfLinkerForBinaryRelocMap from previous comments).
This allows to not put specific classes into the DWARFLinker interface and to not use downcasting.



================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:620
     return Flags | TF_Keep;
+    break;
   default:
----------------
no need to add break here.


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1787
+                       AttrInfo, IsLittleEndian);
+    if (FinalAttrSize != 0) {
+      AttributesFixups.push_back(CurAttrFixup);
----------------
it would be good to add check for ObjFile.Addresses->getLibraryInstallName() here to not collect AttributeLinkedOffsetFixup if not necessary:


```
std::optional<StringRef> LibraryInstallName = ObjFile.Addresses->getLibraryInstallName();
.....
if (FinalAttrSize != 0 && LibraryInstallName.has_value()) {
```


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

https://reviews.llvm.org/D158124