[PATCH] D158124: [dsymutil] Add support for mergeable libraries
Alexey Lapshin via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Sep 1 04:27:41 PDT 2023
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
More information about the llvm-commits
mailing list