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

[Alexey Lapshin via Phabricator via llvm-commits]

avl added a comment.

I also tried to guess how it could be used with DWARFLinkerParallel. The differences are:

1. DWARFLinkerParallel handles compile units parallelly. That means ObjFile.Addresses->saveValidRelocs() can be called asynchronously.

2. Unit.getStartOffset() is not known at the moment ObjFile.Addresses->saveValidRelocs() is called.

3. DWARFLinkerParallel tries to release resources as soon as they are not needed anymore. It deletes AddressesMap once all compile units are cloned using Addresses.reset(); So when current patch would try to call emitRelocations all data would already be deleted.

I do not say that we need to implement the DWARFLinkerParallel part with this patch. It would be better
to do it with a separate patch. But, probably it would be good to make the interface part compatible with
DWARFLinkerParallel in this patch. To do this we could probably change the interface in this way:

1. Add the offset of the original compile unit to the method which relinks relocations.

  class DwarfLinkerForBinary::AddressManager<AddressesMapBase> {
    virtual void saveValidRelocs(uint64_t OriginalUnitOffset, int64_t LinkedOffsetFixupVal, uint64_t InputAttrStartOffset,
                                 uint64_t InputAttrEndOffset) {
      ...
      std::vector<ValidReloc>& StoredValidDebugInfoRelocs = StoredValidDebugInfoRelocsMap[OriginalUnitOffset];
      for (ValidReloc R : Relocs) {
         StoredValidDebugInfoRelocs.emplace_back(R.Offset + LinkedOffsetFixupVal, R.Size,
                                              R.Addend, R.SymbolName,
                                              R.SymbolMapping);
      }
      ...
    }

It allows storing relocations for compile units separately and then could be done in parallel.

2. When the AddressesMap object is created - build a map for units relocations:

  DenseMap<uint64_t Offset, std::vector<ValidReloc>> StoredValidDebugInfoRelocsMap;
  
  DwarfLinkerForBinary::AddressManager<AddressesMapBase> (DWARFContext& Context) {
  
    for (const std::unique_ptr<DWARFUnit> &CU : Context.compile_units()) {
      StoredValidDebugInfoRelocs.insert(std::make_pair(CU->getOffset(), std::vector<ValidReloc>>));
    }
  }



3. Add a method which allows updating relocations offset with outputUnit Offset:

  class DwarfLinkerForBinary::AddressManager<AddressesMapBase> {
     
     void updateRelinkedRelocationsWithUnitOffset (uint64_t OriginalUnitOffset, uint64_t OutputUnitOffset) {
      std::vector<ValidReloc>& StoredValidDebugInfoRelocs = StoredValidDebugInfoRelocsMap[OriginalUnitOffset];
      for (ValidReloc R : Relocs) {
        R.Offset += OutputUnitOffset;
      }
     }
  }

that methods would be called by DWARFLinkerParallel when output unit offsets would be known.

4. Add the cleanupInputData() method to the AddressesMap which will release all resources except StoredValidDebugInfoRelocsMap. that allows calling "Addresses.cleanupInputData()" instead of "Addresses.reset();" and then to keep StoredValidDebugInfoRelocsMap for further processing.

What do you think? Could we implement these changes also?


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

https://reviews.llvm.org/D158124