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

[Alexey Lapshin via Phabricator via llvm-commits]

avl added a comment.

I have several general suggestions:

1. If I understood correctly, the current solution to calculate relocation offsets may work incorrectly in some cases. Currently, offset to the output DIE is substructed from the offset to the input DIE(with correction for the inserted DW_AT_APPLE_origin attribute).

  // Reflect DIEs removed during transformation from .o to .dSYM with a slide.
  int64_t RelocSlide =
      LibInstallName->empty() ? (int64_t)OutOffset - (int64_t)BaseOffset : 0;
  // Reflect displacement due to DW_AT_APPLE_origin attribute in output file.
  RelocSlide += CU->addedOriginObject() ? 4 : 0;

This does not take into account the fact that offsets to the attributes inside DIE may be changed after cloning (and then corresponding relocations should also change their offsets). These offsets may be changed because new abbreviation numbers may take more or opposite less space, and because some attributes from the original DIE may be skipped. The calculation of relocation offsets during cloning of DIEs by calculating per-attribute offsets in the new DIE, may be used instead:

  class AddressesMap {
    enum class SectionWithRelocationKind : uint8_t {
      DebugInfo,
      DebugAddr
    }
  
    // Returns relocations for specified offsets inside SectionWithRelocationKind section.
    std::optional<ArrayRef<ValidReloc>> getRelocations ( SectionWithRelocationKind Kind, uint64_t StartOffset, uint64_t EndOffset );
  
    // Save relinked relocations.
    void saveRelinkedRelocs (SmallVector<ValidReloc>& Relocs)
  }
    
    Use it in cloneDIE() while cloning attributes:
  
      cloneDIE() {
      ...
  *  SmallVector<ValidReloc> Relocations;
      for (const auto &AttrSpec : Abbrev->attributes()) {
        if (shouldSkipAttribute(Update, AttrSpec, Flags & TF_SkipPC)) {
          DWARFFormValue::skipValue(AttrSpec.Form, Data, &Offset,
                                    U.getFormParams());
          continue;
        }
  
        DWARFFormValue Val = AttrSpec.getFormValue();
        uint64_t AttrSize = Offset;
  *    uint64_t InputStartAttrOffset = Offset;
        Val.extractValue(Data, &Offset, U.getFormParams(), &U);
  *    uint64_t InputEndAttrOffset = Offset;
        AttrSize = Offset - AttrSize;
  
  
        uint64_t FinalAttrSize += cloneAttribute(*Die, InputDIE, File, Unit, Val, AttrSpec,
                                    AttrSize, AttrInfo, IsLittleEndian);
  
  *    if  (FinalAttrSize != 0) {
  *      if ( std::optional<ArrayRef<ValidReloc>> Relocs = ObjFile.Addresses->getRelocations (DebugInfo, InputStartAttrOffset, InputEndAttrOffset) )
  *         Relocations = RelinkRelocs (*Relocs, InputStartAttrOffset, OutOffset);
  *    }
  
        OutOffset += FinalAttrSize;
      }
    
      ....
      Linker.assignAbbrev(NewAbbrev);
      Die->setAbbrevNumber(NewAbbrev.getNumber());
  
      // Add the size of the abbreviation number to the output offset.
      OutOffset += getULEB128Size(Die->getAbbrevNumber());
      ....
  *  updateRelocsWithAbbreviationNumberSize (getULEB128Size(Die->getAbbrevNumber(), Relocations);  
    
  *  ObjFile.Addresses->saveRelinkedRelocs(Relocations)
      }
  
    SmallVector<ValidReloc> RelinkRelocs (ArrayRef<ValidReloc> Relocs, uint64_t InputStartAttrOffset, uint64_t OutAttrStartOffset) {
      SmallVector<ValidReloc> Result;
  
      for (ValidReloc &R : Relocs) {
        uint64_t OffsetToRelocationTargetInsideAttr = R.Offset - InputStartAttrOffset;
      
        Result.push_back({OutAttrStartOffset + OffsetToRelocationTargetInsideAttr, R.Size, R.Addend, R.SymbolName});
      }
    
      return Result;
    }
  
  
  void updateRelocsWithAbbreviationNumberSize (uint64_t AbbrevNumberSize, SmallVector<ValidReloc>& Relocs) {
    for (ValidReloc R : Relocs) {
      Relocs.Offset += AbbrevNumberSize;
    }
  }

2. Can we simplify ValidRelocation and ValidReloc to be general enough to not use subclassing? So that we can use the single structure for DWARFLinker, DWARFLinkerParallel, DwarfLinkerForBinary. ValidReloc structure from the patch contains specific types, also it has a bigger size than the original ValidReloc that means that relocations will require more space.

  struct ValidReloc {
    yaml::Hex64 OffsetInCU;
    yaml::Hex64 Offset;
    yaml::Hex32 Size;
    yaml::Hex64 Addend;
    std::string SymbolName;
    SymbolMapping SymbolMapping;
  }

f.e. following structure does not contain specific DebugMap types but is able to keep the same info.

  class AddressesMap {
  
   struct ValidReloc {
     uint64_t OffsetInCU;  /// <<< Probably we do not need it if offsets would be calculated differently.
     uint64_t Offset;
     uint32_t Size;
     uint64_t Addend;
     /// SymbolName assumed to be stored in StringMap and may be fastly converted to StringMapEntry<SymbolMapping>
     /// by concrete implementation of the AddressesMap using GetStringMapEntryFromKeyData.
     const char* SymbolName;
   }
   
  }

using this version of ValidReloc simplifies the data structure and requires less space.

nit: name ValidReloc is meaningful in DwarfLinkerForBinary context when it is searching for valid relocations.
if we make it more general, probably name it just Reloc ? or Relocation ?

3. Skipping and then inserting DW_AT_APPLE_origin attribute looks a bit screwed. Can we just update it if the attribute is presented(as it is done for other attributes) and add it if the attribute does not exist?

  unsigned DWARFLinker::DIECloner::cloneStringAttribute() {
  
    if (AttrSpec.Attr == dwarf::DW_AT_APPLE_origin) {
     Info.AttrAppleOriginSeen = true;
     
     if (std::optional<StringRef> FileName = ObjFile.Addresses->getFileName()) {
       auto StringEntry = DebugStrPool.getEntry(FileName->value());
       
     }
     
    }
  
      ... handle StringEntry in the standard way ...
  
  }


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

https://reviews.llvm.org/D158124