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

[Alexey Lapshin via Phabricator via llvm-commits]

avl added a comment.

Thank you for the update and measurements!

The DWARFLinker changes look generally OK. Though, I still have two concerns, probably, they could be resolved by separate patches.

First thing is that increasing binary RSS requirements for more than 2G is quite a big change. I think, this increasing is coused mostly by two things:
symbol name and mapping now kept inside ValidReloc structure(previously ValidReloc had only pointer to mapping). StoredValidDebugInfoReloc contains copy of original 
relocation which differ with only Offset field(other fields have the same data). Probably the most memory saving solution would be to keep original ValidReloc structure with new field added. In this case, only single uint64_t for each relocation would be added. No additional containers would be required:

  struct ValidReloc {
     uint64_t Offset;
     uint64_t LinkedOffset;
     uint32_t Size;
     uint64_t Addend;
     const DebugMapObject::DebugMapEntry *Mapping;
  }

Second thing is that interface of AddressesMap looks a bit weak. It does not have updateRelocationsWithUnitOffset() method for DWARFLinkerParallel. The updateAndSaveValidRelocs() method does not have a RelocationKind parameter which would be important when support of .debug_addr would be added. Probably, these changes might be done with separate patches for DWARFLinkerParallel and support of .debug_addr.

So, changes for DWARFLinker is LGTM, assuming above concerns would be addressed by separate patches.

Please, wait approve for the rest of the patch from dsymutil owners.


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

https://reviews.llvm.org/D158124