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

[Alpha Abdoulaye via Phabricator via llvm-commits]

Alpha marked an inline comment as done.
Alpha added inline comments.


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:541
   MyInfo.InDebugMap = true;
+  MyInfo.FileName = RelocMgr.getFileName();
 
----------------
avl wrote:
> Why do we need to put FileName info into the every DIE info structure?
> 
> LibInstallName is initialized in AddressManager contructor. It looks like we can ask AddressManager for FileName when it is needed, instead of storing it into the DIE info structure.
That is because I need to add it to the string pool when creating the `DW_AT_APPLE_origin` attribute (in `cloneDie` inside `DwarfLinker.cpp`)


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1756
+  int64_t RelocSlide =
+      !Info.FileName ? (int64_t)OutOffset - (int64_t)Offset : 0;
+  // Reflect displacement due to DW_AT_APPLE_origin attribute in output file.
----------------
avl wrote:
> Can we move this check into the applyValidRelocs(a couple of lines below) as AddressManager already knows FileName?
> 
> 
> ```
> bool DwarfLinkerForBinary::AddressManager<AddressesMapBase>::applyValidRelocs(
>     MutableArrayRef<char> Data, uint64_t BaseOffset, bool IsLittleEndian,
>     uint64_t OutOffset, CompileUnit& CU) {
> 
>     int64_t RelocSlide = (getFileName()) ? 0 ; (int64_t)OutOffset - (int64_t)Offset;
>     if (!CU.getOrigUnit().getUnitDIE().find(DW_AT_APPLE_origin))
>       RelocSlide += 4;
> ...
> }
> 
> 
> ```
Sounds reasonable. I will pass the CompileUnit as a pointer though, because there is a different `CompileUnit` implemented  in `dwarflinker_parallel`, so passing a reference would mean either adding templating in *a lot* of places for this very specific use case, or including the `DWARFLinker` header in the parallel implementation file, which doesn't like something we want to do.

That's one less reason to have FileName in the DIE Info, but there is still an outstanding one remaining.


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

https://reviews.llvm.org/D158124