[DWARFLinker] Refactor cloneAddressAttribute().

[avl]

Summary

As a preparation for implementing DWARFv5 address ranges generation,
this patch refactors cloneAddressAttribute() method. It has special
handling for addresses which can be relocated in some unrelated value,
for applying relocations twice, for indexed addresses. Instead of
all these special handlings this patch uses general handling:

Read attribute value from InputDIE and apply PCOffset.

Another thing is that current handling of DW_FORM_addrx misses the
fact that relocations might be applied twice in some cases. This
patch fixes this problem also.

[friss]

This feels like a nice generalization (and I like the shape of the new code better), but I'm wondering if it's not too general. For example a DT_AT_location can be DW_FORM_addr, and applying PCOffset to it would be wrong. I'm not sure what PCOffset is set to in a variable DIE, but it does feel somewhat wrong even if it's 0. I cannot be 100% sure, but I believe that fear of mishandling non-PC addresses was the original thinking that led to spelling out all the supported cases.

[avl]

In D143269#4103095, @friss wrote:

This feels like a nice generalization (and I like the shape of the new code better), but I'm wondering if it's not too general. For example a DT_AT_location can be DW_FORM_addr, and applying PCOffset to it would be wrong. I'm not sure what PCOffset is set to in a variable DIE, but it does feel somewhat wrong even if it's 0. I cannot be 100% sure, but I believe that fear of mishandling non-PC addresses was the original thinking that led to spelling out all the supported cases.

It looks like DW_AT_location can not have DW_FORM_addr attribute. DWARF spec says that DW_AT_location has "exprloc, loclistptr" classes, while DW_FORM_addr is used for "address" class. It is also said for DW_FORM_addr:

• Debugging information entries may have attributes with the form
DW_FORM_addr (see Section 7.5.4 on page 207). These attributes represent
locations within the virtual address space of the program, and require
relocation.
• A DWARF expression may contain a DW_OP_addr (see Section 2.5.1.1 on
page 26) which contains a location within the virtual address space of the
program, and require relocation.

Thus, it looks like lt is Ok to always apply PCOffset to attribute of DW_FORM_addr form.

[avl]

ping.

[JDevlieghere]

LGTM

llvm/lib/DWARFLinker/DWARFLinker.cpp

1174

1206–1208

[avl]

Thank you for the review!

llvm/include/llvm/DWARFLinker/DWARFLinker.h

public:
 
   /// Apply the valid relocations to the buffer \p Data, taking into
   /// account that Data is at \p BaseOffset in the .debug_info section.
   ///
   /// \returns true whether any reloc has been applied.
   virtual bool applyValidRelocs(MutableArrayRef<char> Data, uint64_t BaseOffset,
                                 bool IsLittleEndian) = 0;
 
-  /// Relocate the given address offset if a valid relocation exists.
-  virtual llvm::Expected<uint64_t> relocateIndexedAddr(uint64_t StartOffset,
-                                                       uint64_t EndOffset) = 0;
-
   /// Returns all valid functions address ranges(i.e., those ranges
   /// which points to sections with code).
   virtual RangesTy &getValidAddressRanges() = 0;
 
   /// Erases all data.
   virtual void clear() = 0;
 };
 
private:
     struct AttributesInfo {
       /// Names.
       DwarfStringPoolEntryRef Name, MangledName, NameWithoutTemplate;
 
       /// Offsets in the string pool.
       uint32_t NameOffset = 0;
       uint32_t MangledNameOffset = 0;
 
-      /// Value of AT_low_pc in the input DIE
-      uint64_t OrigLowPc = std::numeric_limits<uint64_t>::max();
-
-      /// Value of AT_high_pc in the input DIE
-      uint64_t OrigHighPc = 0;
-
-      /// Value of DW_AT_call_return_pc in the input DIE
-      uint64_t OrigCallReturnPc = 0;
-
-      /// Value of DW_AT_call_pc in the input DIE
-      uint64_t OrigCallPc = 0;
-
       /// Offset to apply to PC addresses inside a function.
       int64_t PCOffset = 0;
 
       /// Does the DIE have a low_pc attribute?
       bool HasLowPc = false;
 
       /// Does the DIE have a ranges attribute?
       bool HasRanges = false;
private:
     unsigned cloneBlockAttribute(DIE &Die, const DWARFFile &File,
                                  CompileUnit &Unit, AttributeSpec AttrSpec,
                                  const DWARFFormValue &Val, unsigned AttrSize,
                                  bool IsLittleEndian);
 
     /// Clone an attribute referencing another DIE and add
     /// it to \p Die.
     /// \returns the size of the new attribute.
-    unsigned cloneAddressAttribute(DIE &Die, AttributeSpec AttrSpec,
-                                   unsigned AttrSize, const DWARFFormValue &Val,
+    unsigned cloneAddressAttribute(DIE &Die, const DWARFDie &InputDIE,
+                                   AttributeSpec AttrSpec, unsigned AttrSize,
+                                   const DWARFFormValue &Val,
                                    const CompileUnit &Unit,
                                    AttributesInfo &Info);
 
     /// Clone a scalar attribute  and add it to \p Die.
     /// \returns the size of the new attribute.
     unsigned cloneScalarAttribute(DIE &Die, const DWARFDie &InputDIE,
                                   const DWARFFile &File, CompileUnit &U,
                                   AttributeSpec AttrSpec,

llvm/lib/DWARFLinker/DWARFLinker.cpp

unsigned DWARFLinker::DIECloner::cloneBlockAttribute(
   else
     Block->setSize(Bytes.size());
 
   Die.addValue(DIEAlloc, Value);
   return AttrSize;
 }
 
 unsigned DWARFLinker::DIECloner::cloneAddressAttribute(
-    DIE &Die, AttributeSpec AttrSpec, unsigned AttrSize,
-    const DWARFFormValue &Val, const CompileUnit &Unit, AttributesInfo &Info) {
-  if (LLVM_UNLIKELY(Linker.Options.Update)) {
-    if (AttrSpec.Attr == dwarf::DW_AT_low_pc)
-      Info.HasLowPc = true;
+    DIE &Die, const DWARFDie &InputDIE, AttributeSpec AttrSpec,
+    unsigned AttrSize, const DWARFFormValue &Val, const CompileUnit &Unit,
+    AttributesInfo &Info) {
+
+  if (LLVM_UNLIKELY(Linker.Options.Update)) {
     Die.addValue(DIEAlloc, dwarf::Attribute(AttrSpec.Attr),
                  dwarf::Form(AttrSpec.Form), DIEInteger(Val.getRawUValue()));
     return AttrSize;
   }
 
-  dwarf::Form Form = AttrSpec.Form;
-  uint64_t Addr = 0;
-  if (Form == dwarf::DW_FORM_addrx) {
-    if (std::optional<uint64_t> AddrOffsetSectionBase =
-            Unit.getOrigUnit().getAddrOffsetSectionBase()) {
-      uint64_t StartOffset =
-          *AddrOffsetSectionBase +
-          Val.getRawUValue() * Unit.getOrigUnit().getAddressByteSize();
-      uint64_t EndOffset =
-          StartOffset + Unit.getOrigUnit().getAddressByteSize();
-      if (llvm::Expected<uint64_t> RelocAddr =
-              ObjFile.Addresses->relocateIndexedAddr(StartOffset, EndOffset))
-        Addr = *RelocAddr;
-      else
-        Linker.reportWarning(toString(RelocAddr.takeError()), ObjFile);
-    } else
-      Linker.reportWarning("no base offset for address table", ObjFile);
-
-    // Generation of DWARFv5 .debug_addr table is not supported yet.
-    // Convert attribute into the dwarf::DW_FORM_addr.
-    Form = dwarf::DW_FORM_addr;
-  } else
-    Addr = *Val.getAsAddress();
+  // Cloned Die may have address attributes relocated to a
+  // totally unrelated value. This can happen:
+  //   - If high_pc is an address (Dwarf version == 2), then it might have been
+  //     relocated to a totally unrelated value (because the end address in the
+  //     object file might be start address of another function which got moved
+  //     independently by the linker).
+  //   - If address relocated in an inline_subprogram that happens at the
+  //     beginning of its inlining function.
+  //  To avoid above cases and to not apply relocation twice (in applyValidRelocs
+  //  and here), read address attribute from InputDIE and apply Info.PCOffset
+  //  here.
+
+  std::optional<DWARFFormValue> AddrAttribute = InputDIE.find(AttrSpec.Attr);
+  if (!AddrAttribute)
+    llvm_unreachable("Cann't find attribute.");
+
+  std::optional<uint64_t> Addr = AddrAttribute->getAsAddress();
+  if (!Addr) {
+    Linker.reportWarning("Cann't read address attribute value.", ObjFile);
+    Addr = 0;
+  }
 
-  if (AttrSpec.Attr == dwarf::DW_AT_low_pc) {
-    if (Die.getTag() == dwarf::DW_TAG_inlined_subroutine ||
-        Die.getTag() == dwarf::DW_TAG_lexical_block ||
-        Die.getTag() == dwarf::DW_TAG_label) {
-      // The low_pc of a block or inline subroutine might get
-      // relocated because it happens to match the low_pc of the
-      // enclosing subprogram. To prevent issues with that, always use
-      // the low_pc from the input DIE if relocations have been applied.
-      Addr = (Info.OrigLowPc != std::numeric_limits<uint64_t>::max()
-                  ? Info.OrigLowPc
-                  : Addr) +
-             Info.PCOffset;
-    } else if (Die.getTag() == dwarf::DW_TAG_compile_unit) {
-      if (std::optional<uint64_t> LowPC = Unit.getLowPc())
-        Addr = *LowPC;
-      else
-        return 0;
-    }
-    Info.HasLowPc = true;
-  } else if (AttrSpec.Attr == dwarf::DW_AT_high_pc) {
-    if (Die.getTag() == dwarf::DW_TAG_compile_unit) {
-      if (uint64_t HighPc = Unit.getHighPc())
-        Addr = HighPc;
-      else
-        return 0;
-    } else
-      // If we have a high_pc recorded for the input DIE, use
-      // it. Otherwise (when no relocations where applied) just use the
-      // one we just decoded.
-      Addr = (Info.OrigHighPc ? Info.OrigHighPc : Addr) + Info.PCOffset;
-  } else if (AttrSpec.Attr == dwarf::DW_AT_call_return_pc) {
-    // Relocate a return PC address within a call site entry.
-    if (Die.getTag() == dwarf::DW_TAG_call_site)
-      Addr = (Info.OrigCallReturnPc ? Info.OrigCallReturnPc : Addr) +
-             Info.PCOffset;
-  } else if (AttrSpec.Attr == dwarf::DW_AT_call_pc) {
-    // Relocate the address of a branch instruction within a call site entry.
-    if (Die.getTag() == dwarf::DW_TAG_call_site)
-      Addr = (Info.OrigCallPc ? Info.OrigCallPc : Addr) + Info.PCOffset;
+  if (InputDIE.getTag() == dwarf::DW_TAG_compile_unit &&
+      AttrSpec.Attr == dwarf::DW_AT_low_pc) {
+  } else if (InputDIE.getTag() == dwarf::DW_TAG_compile_unit &&
+             AttrSpec.Attr == dwarf::DW_AT_high_pc) {
+  } else {
+    *Addr += Info.PCOffset;
   }
 
+  dwarf::Form Form = AttrSpec.Form;
+
+  // FIXME: Generation of DWARFv5 .debug_addr table is not supported yet.
+  // Convert attribute into the dwarf::DW_FORM_addr.
+  if (Form == dwarf::DW_FORM_addrx)
+    Form = dwarf::DW_FORM_addr;
+
   Die.addValue(DIEAlloc, static_cast<dwarf::Attribute>(AttrSpec.Attr),
-               static_cast<dwarf::Form>(Form), DIEInteger(Addr));
+               static_cast<dwarf::Form>(Form), DIEInteger(*Addr));
   return Unit.getOrigUnit().getAddressByteSize();
 }
 
 unsigned DWARFLinker::DIECloner::cloneScalarAttribute(
     DIE &Die, const DWARFDie &InputDIE, const DWARFFile &File,
     CompileUnit &Unit, AttributeSpec AttrSpec, const DWARFFormValue &Val,
     unsigned AttrSize, AttributesInfo &Info) {
   uint64_t Value;
unsigned DWARFLinker::DIECloner::cloneAttribute(
   case dwarf::DW_FORM_block1:
   case dwarf::DW_FORM_block2:
   case dwarf::DW_FORM_block4:
   case dwarf::DW_FORM_exprloc:
     return cloneBlockAttribute(Die, File, Unit, AttrSpec, Val, AttrSize,
                                IsLittleEndian);
   case dwarf::DW_FORM_addr:
   case dwarf::DW_FORM_addrx:
-    return cloneAddressAttribute(Die, AttrSpec, AttrSize, Val, Unit, Info);
+    return cloneAddressAttribute(Die, InputDIE, AttrSpec, AttrSize, Val, Unit,
+                                 Info);
   case dwarf::DW_FORM_data1:
   case dwarf::DW_FORM_data2:
   case dwarf::DW_FORM_data4:
   case dwarf::DW_FORM_data8:
   case dwarf::DW_FORM_udata:
   case dwarf::DW_FORM_sdata:
   case dwarf::DW_FORM_sec_offset:
   case dwarf::DW_FORM_flag:
DIE *DWARFLinker::DIECloner::cloneDIE(const DWARFDie &InputDIE,
   // We could copy the data only if we need to apply a relocation to it. After
   // testing, it seems there is no performance downside to doing the copy
   // unconditionally, and it makes the code simpler.
   SmallString<40> DIECopy(Data.getData().substr(Offset, NextOffset - Offset));
   Data =
       DWARFDataExtractor(DIECopy, Data.isLittleEndian(), Data.getAddressSize());
 
   // Modify the copy with relocated addresses.
-  if (ObjFile.Addresses->applyValidRelocs(DIECopy, Offset,
-                                          Data.isLittleEndian())) {
-    // If we applied relocations, we store the value of high_pc that was
-    // potentially stored in the input DIE. If high_pc is an address
-    // (Dwarf version == 2), then it might have been relocated to a
-    // totally unrelated value (because the end address in the object
-    // file might be start address of another function which got moved
-    // independently by the linker). The computation of the actual
-    // high_pc value is done in cloneAddressAttribute().
-    AttrInfo.OrigHighPc =
-        dwarf::toAddress(InputDIE.find(dwarf::DW_AT_high_pc), 0);
-    // Also store the low_pc. It might get relocated in an
-    // inline_subprogram that happens at the beginning of its
-    // inlining function.
-    AttrInfo.OrigLowPc = dwarf::toAddress(InputDIE.find(dwarf::DW_AT_low_pc),
-                                          std::numeric_limits<uint64_t>::max());
-    AttrInfo.OrigCallReturnPc =
-        dwarf::toAddress(InputDIE.find(dwarf::DW_AT_call_return_pc), 0);
-    AttrInfo.OrigCallPc =
-        dwarf::toAddress(InputDIE.find(dwarf::DW_AT_call_pc), 0);
-  }
+  ObjFile.Addresses->applyValidRelocs(DIECopy, Offset, Data.isLittleEndian());
 
   // Reset the Offset to 0 as we will be working on the local copy of
   // the data.
   Offset = 0;
 
   const auto *Abbrev = InputDIE.getAbbreviationDeclarationPtr();
   Offset += getULEB128Size(Abbrev->getCode());
 

llvm/test/tools/dsymutil/X86/dwarf5-call-site-entry-reloc.test

+## Test binaries created with the following commands:
+
+## $ cat call-dwarf5.c
+## __attribute__((noinline, noreturn)) void foo() {
+##     asm volatile("" ::: "memory");
+##       __builtin_unreachable();
+## }
+## __attribute__((noinline)) void bar() {
+##     asm volatile("nop" :::);
+##      foo();
+## }
+
+## int main() { bar(); }
+
+## $ clang -gdwarf-5 call-dwarf5.c -fomit-frame-pointer -c -Os -o call-dwarf5.o
+## $ clang -gdwarf-5 call-dwarf5.o -o call-dwarf5
+
+## The test requires the return PC to match a relocation (in this case the
+## DW_AT_high_pc of main). Without this change the value would get relocated
+## twice.
+
+#RUN: dsymutil -oso-prepend-path %p/../Inputs -y %s -o %t.dSYM
+#RUN: llvm-dwarfdump %t.dSYM | FileCheck %s -implicit-check-not=DW_AT_call_return_pc
+
+#CHECK: DW_AT_call_return_pc  (0x0000000100000f72)
+#CHECK: DW_AT_call_return_pc  (0x0000000100000f78)
+
+---
+triple:          'x86_64-apple-darwin'
+objects:
+  - filename:        'call-dwarf5.o'
+    timestamp:       1675373912
+    symbols:
+      - { sym: _foo, objAddr: 0x0, binAddr: 0x100000F69, size: 0x2 }
+      - { sym: _bar, objAddr: 0x2, binAddr: 0x100000F6B, size: 0x7 }
+      - { sym: _main, objAddr: 0x9, binAddr: 0x100000F72, size: 0x6 }
+...

llvm/tools/dsymutil/DwarfLinkerForBinary.h

public:
     bool isLiveVariable(const DWARFDie &DIE,
                         CompileUnit::DIEInfo &Info) override;
     bool isLiveSubprogram(const DWARFDie &DIE,
                           CompileUnit::DIEInfo &Info) override;
 
     bool applyValidRelocs(MutableArrayRef<char> Data, uint64_t BaseOffset,
                           bool IsLittleEndian) override;
 
-    llvm::Expected<uint64_t> relocateIndexedAddr(uint64_t StartOffset,
-                                                 uint64_t EndOffset) override;
-
     RangesTy &getValidAddressRanges() override { return AddressRanges; }
 
     void clear() override {
       AddressRanges.clear();
       ValidDebugInfoRelocs.clear();
       ValidDebugAddrRelocs.clear();
     }
   };

llvm/tools/dsymutil/DwarfLinkerForBinary.cpp

for (const ValidReloc &CurReloc : Relocs) {
     }
     assert(CurReloc.Size <= sizeof(Buf));
     memcpy(&Data[CurReloc.Offset - BaseOffset], Buf, CurReloc.Size);
   }
 
   return Relocs.size() > 0;
 }
 
-llvm::Expected<uint64_t>
-DwarfLinkerForBinary::AddressManager::relocateIndexedAddr(uint64_t StartOffset,
-                                                          uint64_t EndOffset) {
-  std::vector<ValidReloc> Relocs =
-      getRelocations(ValidDebugAddrRelocs, StartOffset, EndOffset);
-  if (Relocs.size() == 0)
-    return createStringError(
-        std::make_error_code(std::errc::invalid_argument),
-        "no relocation for offset %llu in debug_addr section", StartOffset);
-
-  return relocate(Relocs[0]);
-}
-
 bool linkDwarf(raw_fd_ostream &OutFile, BinaryHolder &BinHolder,
                const DebugMap &DM, LinkOptions Options) {
   DwarfLinkerForBinary Linker(OutFile, BinHolder, std::move(Options));
   return Linker.link(DM);
 }
 
 } // namespace dsymutil
 } // namespace llvm

llvm/tools/llvm-dwarfutil/DebugInfoLinker.cpp

 //
 // exec: [LowPC, HighPC] is not inside address ranges of .text sections
 //
 // universal: maxpc and bfd
 class ObjFileAddressMap : public AddressesMap {
 public:
   ObjFileAddressMap(DWARFContext &Context, const Options &Options,
                     object::ObjectFile &ObjFile)
-      : Opts(Options), Context(Context) {
+      : Opts(Options) {
     // Remember addresses of existing text sections.
     for (const object::SectionRef &Sect : ObjFile.sections()) {
       if (!Sect.isText())
         continue;
       const uint64_t Size = Sect.getSize();
       if (Size == 0)
         continue;
       const uint64_t StartAddr = Sect.getAddress();
bool applyValidRelocs(MutableArrayRef<char>, uint64_t, bool) override {
     // no need to apply relocations to the linked binary.
     return false;
   }
 
   RangesTy &getValidAddressRanges() override { return DWARFAddressRanges; };
 
   void clear() override { DWARFAddressRanges.clear(); }
 
-  llvm::Expected<uint64_t> relocateIndexedAddr(uint64_t StartOffset,
-                                               uint64_t EndOffset) override {
-    // No relocations in linked binary. Return just address value.
-
-    const char *AddrPtr =
-        Context.getDWARFObj().getAddrSection().Data.data() + StartOffset;
-    support::endianness Endianess =
-        Context.getDWARFObj().isLittleEndian() ? support::little : support::big;
-
-    assert(EndOffset > StartOffset);
-    switch (EndOffset - StartOffset) {
-    case 1:
-      return *AddrPtr;
-    case 2:
-      return support::endian::read16(AddrPtr, Endianess);
-    case 4:
-      return support::endian::read32(AddrPtr, Endianess);
-    case 8:
-      return support::endian::read64(AddrPtr, Endianess);
-    }
-
-    llvm_unreachable("relocateIndexedAddr unhandled case!");
-  }
-
 protected:
   // returns true if specified address range is inside address ranges
   // of executable sections.
   bool isInsideExecutableSectionsAddressRange(uint64_t LowPC,
                                               std::optional<uint64_t> HighPC) {
     std::optional<AddressRange> Range =
         TextAddressRanges.getRangeThatContains(LowPC);
 
bool isDeadAddress(uint64_t LowPC, uint16_t Version, TombstoneKind Tombstone,
     return isDeadAddressRange(LowPC, std::nullopt, Version, Tombstone,
                               AddressByteSize);
   }
 
 private:
   RangesTy DWARFAddressRanges;
   AddressRanges TextAddressRanges;
   const Options &Opts;
-  DWARFContext &Context;
 };
 
 static bool knownByDWARFUtil(StringRef SecName) {
   return llvm::StringSwitch<bool>(SecName)
       .Case(".debug_info", true)
       .Case(".debug_types", true)
       .Case(".debug_abbrev", true)
       .Case(".debug_loc", true)