[dsymutil][DWARFLinker][NFC] Make interface of AddressMap more general.

[avl]

Summary

Current interface of AddressMap assumes that relocations exist.
That is correct for not-linked object file but is not correct
for linked executable. This patch changes interface in such way
that AddressMap could be used not only with not-linked object files:

hasValidRelocationAt()

replaced with:

hasLiveMemoryLocation()
hasLiveAddressRange()

[JDevlieghere]

What's the scenario where you run the DWARF linker with already-linked binaries?

llvm/include/llvm/DWARFLinker/DWARFLinker.h

67–78

73–75

[avl]

What's the scenario where you run the DWARF linker with already-linked binaries?

That is currently discussed proposal of dsymutil-like tool for ELF - http://lists.llvm.org/pipermail/llvm-dev/2020-August/144579.html
Though the proposal is still being discussed it is useful to have interfaces to be in general form so that DWARFLinker could be re-used in various scenarios.

[avl]

addressed comments(changed text of commentary).

[JDevlieghere]

Oops, sorry this got lost in my review queue. This LGTM.

llvm/include/llvm/DWARFLinker/DWARFLinker.h

public:
   /// Returns false if represented addresses are from not-linked
   /// object file.
   virtual bool areRelocationsResolved() const = 0;
 
   /// Checks that there are valid relocations against a .debug_info
   /// section. Reset current relocation pointer if neccessary.
   virtual bool hasValidRelocs(bool ResetRelocsPtr = true) = 0;
 
-  /// Checks that there is a relocation against .debug_info
-  /// table between \p StartOffset and \p NextOffset.
-  ///
-  /// This function must be called with offsets in strictly ascending
-  /// order because it never looks back at relocations it already 'went past'.
-  /// \returns true and sets Info.InDebugMap if it is the case.
-  virtual bool hasValidRelocationAt(uint64_t StartOffset, uint64_t EndOffset,
-                                    CompileUnit::DIEInfo &Info) = 0;
+  /// Checks that the specified DIE has a DW_AT_Location attribute
+  /// that references into a live code section. This function
+  /// must be called with DIE offsets in strictly ascending order.
+  virtual bool hasLiveMemoryLocation(const DWARFDie &DIE,
+                                     CompileUnit::DIEInfo &Info) = 0;
+
+  /// Checks that the specified DIE has a DW_AT_Low_pc attribute
+  /// that references into a live code section. This function
+  /// must be called with DIE offsets in strictly ascending order.
+  virtual bool hasLiveAddressRange(const DWARFDie &DIE,
 
   /// Apply the valid relocations to the buffer \p Data, taking into
   /// account that Data is at \p BaseOffset in the debug_info section.
   ///
   /// This function must be called with monotonic \p BaseOffset values.
   ///
   /// \returns true whether any reloc has been applied.
   virtual bool applyValidRelocs(MutableArrayRef<char> Data, uint64_t BaseOffset,
                                 bool IsLittleEndian) = 0;
private:
   unsigned shouldKeepDIE(AddressesMap &RelocMgr, RangesTy &Ranges,
                          const DWARFDie &DIE, const DWARFFile &File,
                          CompileUnit &Unit, CompileUnit::DIEInfo &MyInfo,
                          unsigned Flags);
 
   /// Check if a variable describing DIE should be kept.
   /// \returns updated TraversalFlags.
   unsigned shouldKeepVariableDIE(AddressesMap &RelocMgr, const DWARFDie &DIE,
-                                 CompileUnit &Unit,
                                  CompileUnit::DIEInfo &MyInfo, unsigned Flags);
 
   unsigned shouldKeepSubprogramDIE(AddressesMap &RelocMgr, RangesTy &Ranges,
                                    const DWARFDie &DIE, const DWARFFile &File,
                                    CompileUnit &Unit,
                                    CompileUnit::DIEInfo &MyInfo,
                                    unsigned Flags);
 

llvm/lib/DWARFLinker/DWARFLinker.cpp

void DWARFLinker::cleanupAuxiliarryData(LinkContext &Context) {
   for (auto I = DIELocs.begin(), E = DIELocs.end(); I != E; ++I)
     (*I)->~DIELoc();
 
   DIEBlocks.clear();
   DIELocs.clear();
   DIEAlloc.Reset();
 }
 
-/// Get the starting and ending (exclusive) offset for the
-/// attribute with index \p Idx descibed by \p Abbrev. \p Offset is
-/// supposed to point to the position of the first attribute described
-/// by \p Abbrev.
-/// \return [StartOffset, EndOffset) as a pair.
-static std::pair<uint64_t, uint64_t>
-getAttributeOffsets(const DWARFAbbreviationDeclaration *Abbrev, unsigned Idx,
-                    uint64_t Offset, const DWARFUnit &Unit) {
-  DataExtractor Data = Unit.getDebugInfoExtractor();
-
-  for (unsigned I = 0; I < Idx; ++I)
-    DWARFFormValue::skipValue(Abbrev->getFormByIndex(I), Data, &Offset,
-                              Unit.getFormParams());
-
-  uint64_t End = Offset;
-  DWARFFormValue::skipValue(Abbrev->getFormByIndex(Idx), Data, &End,
-                            Unit.getFormParams());
-
-  return std::make_pair(Offset, End);
-}
 
 /// Check if a variable describing DIE should be kept.
 /// \returns updated TraversalFlags.
 unsigned DWARFLinker::shouldKeepVariableDIE(AddressesMap &RelocMgr,
                                             const DWARFDie &DIE,
-                                            CompileUnit &Unit,
                                             CompileUnit::DIEInfo &MyInfo,
                                             unsigned Flags) {
   const auto *Abbrev = DIE.getAbbreviationDeclarationPtr();
 
   // Global variables with constant value can always be kept.
   if (!(Flags & TF_InFunctionScope) &&
       Abbrev->findAttributeIndex(dwarf::DW_AT_const_value)) {
     MyInfo.InDebugMap = true;
     return Flags | TF_Keep;
   }
 
-  Optional<uint32_t> LocationIdx =
-      Abbrev->findAttributeIndex(dwarf::DW_AT_location);
-  if (!LocationIdx)
-    return Flags;
-
-  uint64_t Offset = DIE.getOffset() + getULEB128Size(Abbrev->getCode());
-  const DWARFUnit &OrigUnit = Unit.getOrigUnit();
-  uint64_t LocationOffset, LocationEndOffset;
-  std::tie(LocationOffset, LocationEndOffset) =
-      getAttributeOffsets(Abbrev, *LocationIdx, Offset, OrigUnit);
-
   // See if there is a relocation to a valid debug map entry inside
   // this variable's location. The order is important here. We want to
   // always check if the variable has a valid relocation, so that the
   // DIEInfo is filled. However, we don't want a static variable in a
   // function to force us to keep the enclosing function.
-  if (!RelocMgr.hasValidRelocationAt(LocationOffset, LocationEndOffset,
-                                     MyInfo) ||
+  if (!RelocMgr.hasLiveMemoryLocation(DIE, MyInfo) ||
       (Flags & TF_InFunctionScope))
     return Flags;
 
   if (Options.Verbose) {
     outs() << "Keeping variable DIE:";
     DIDumpOptions DumpOpts;
     DumpOpts.ChildRecurseDepth = 0;
     DumpOpts.Verbose = Options.Verbose;
     DIE.dump(outs(), 8 /* Indent */, DumpOpts);
   }
 
   return Flags | TF_Keep;
 }
 
 /// Check if a function describing DIE should be kept.
 /// \returns updated TraversalFlags.
 unsigned DWARFLinker::shouldKeepSubprogramDIE(
     AddressesMap &RelocMgr, RangesTy &Ranges, const DWARFDie &DIE,
     const DWARFFile &File, CompileUnit &Unit, CompileUnit::DIEInfo &MyInfo,
     unsigned Flags) {
-  const auto *Abbrev = DIE.getAbbreviationDeclarationPtr();
-
   Flags |= TF_InFunctionScope;
 
-  Optional<uint32_t> LowPcIdx = Abbrev->findAttributeIndex(dwarf::DW_AT_low_pc);
-  if (!LowPcIdx)
+  auto LowPc = dwarf::toAddress(DIE.find(dwarf::DW_AT_low_pc));
+  if (!LowPc)
     return Flags;
 
-  uint64_t Offset = DIE.getOffset() + getULEB128Size(Abbrev->getCode());
-  DWARFUnit &OrigUnit = Unit.getOrigUnit();
-  uint64_t LowPcOffset, LowPcEndOffset;
-  std::tie(LowPcOffset, LowPcEndOffset) =
-      getAttributeOffsets(Abbrev, *LowPcIdx, Offset, OrigUnit);
-
-  auto LowPc = dwarf::toAddress(DIE.find(dwarf::DW_AT_low_pc));
   assert(LowPc.hasValue() && "low_pc attribute is not an address.");
-  if (!LowPc ||
-      !RelocMgr.hasValidRelocationAt(LowPcOffset, LowPcEndOffset, MyInfo))
+  if (!RelocMgr.hasLiveAddressRange(DIE, MyInfo))
     return Flags;
 
   if (Options.Verbose) {
     outs() << "Keeping subprogram DIE:";
     DIDumpOptions DumpOpts;
     DumpOpts.ChildRecurseDepth = 0;
     DumpOpts.Verbose = Options.Verbose;
     DIE.dump(outs(), 8 /* Indent */, DumpOpts);
   }
 
   if (DIE.getTag() == dwarf::DW_TAG_label) {
     if (Unit.hasLabelAt(*LowPc))
       return Flags;
+
+    DWARFUnit &OrigUnit = Unit.getOrigUnit();
     // FIXME: dsymutil-classic compat. dsymutil-classic doesn't consider labels
     // that don't fall into the CU's aranges. This is wrong IMO. Debug info
     // generation bugs aside, this is really wrong in the case of labels, where
     // a label marking the end of a function will have a PC == CU's high_pc.
     if (dwarf::toAddress(OrigUnit.getUnitDIE().find(dwarf::DW_AT_high_pc))
             .getValueOr(UINT64_MAX) <= LowPc)
       return Flags;
     Unit.addLabelLowPc(*LowPc, MyInfo.AddrAdjust);

 unsigned DWARFLinker::shouldKeepDIE(AddressesMap &RelocMgr, RangesTy &Ranges,
                                     const DWARFDie &DIE, const DWARFFile &File,
                                     CompileUnit &Unit,
                                     CompileUnit::DIEInfo &MyInfo,
                                     unsigned Flags) {
   switch (DIE.getTag()) {
   case dwarf::DW_TAG_constant:
   case dwarf::DW_TAG_variable:
-    return shouldKeepVariableDIE(RelocMgr, DIE, Unit, MyInfo, Flags);
+    return shouldKeepVariableDIE(RelocMgr, DIE, MyInfo, Flags);
   case dwarf::DW_TAG_subprogram:
   case dwarf::DW_TAG_label:
     return shouldKeepSubprogramDIE(RelocMgr, Ranges, DIE, File, Unit, MyInfo,
                                    Flags);
   case dwarf::DW_TAG_base_type:
     // DWARF Expressions may reference basic types, but scanning them
     // is expensive. Basic types are tiny, so just keep all of them.
   case dwarf::DW_TAG_imported_module:

llvm/tools/dsymutil/DwarfLinkerForBinary.h

bool findValidRelocs(const object::SectionRef &Section,
                          const DebugMapObject &DMO);
 
     void findValidRelocsMachO(const object::SectionRef &Section,
                               const object::MachOObjectFile &Obj,
                               const DebugMapObject &DMO);
     /// @}
 
     bool hasValidRelocationAt(uint64_t StartOffset, uint64_t EndOffset,
-                              CompileUnit::DIEInfo &Info) override;
+                              CompileUnit::DIEInfo &Info);
+
+    bool hasLiveMemoryLocation(const DWARFDie &DIE,
+                               CompileUnit::DIEInfo &Info) override;
+    bool hasLiveAddressRange(const DWARFDie &DIE,
 
     bool applyValidRelocs(MutableArrayRef<char> Data, uint64_t BaseOffset,
                           bool IsLittleEndian) override;
 
     RangesTy &getValidAddressRanges() override { return AddressRanges; }
 
     void clear() override {
       AddressRanges.clear();

llvm/tools/dsymutil/DwarfLinkerForBinary.cpp

bool DwarfLinkerForBinary::AddressManager::hasValidRelocationAt(
 
   Info.AddrAdjust = BinaryAddress + ValidReloc.Addend;
   if (Mapping.ObjectAddress)
     Info.AddrAdjust -= ObjectAddress;
   Info.InDebugMap = true;
   return true;
 }
 
+/// Get the starting and ending (exclusive) offset for the
+/// attribute with index \p Idx descibed by \p Abbrev. \p Offset is
+/// supposed to point to the position of the first attribute described
+/// by \p Abbrev.
+/// \return [StartOffset, EndOffset) as a pair.
+static std::pair<uint64_t, uint64_t>
+getAttributeOffsets(const DWARFAbbreviationDeclaration *Abbrev, unsigned Idx,
+                    uint64_t Offset, const DWARFUnit &Unit) {
+  DataExtractor Data = Unit.getDebugInfoExtractor();
+
+  for (unsigned I = 0; I < Idx; ++I)
+    DWARFFormValue::skipValue(Abbrev->getFormByIndex(I), Data, &Offset,
+                              Unit.getFormParams());
+
+  uint64_t End = Offset;
+  DWARFFormValue::skipValue(Abbrev->getFormByIndex(Idx), Data, &End,
+                            Unit.getFormParams());
+
+  return std::make_pair(Offset, End);
+}
+
+bool DwarfLinkerForBinary::AddressManager::hasLiveMemoryLocation(
+    const DWARFDie &DIE, CompileUnit::DIEInfo &MyInfo) {
+
+  const auto *Abbrev = DIE.getAbbreviationDeclarationPtr();
+
+  Optional<uint32_t> LocationIdx =
+      Abbrev->findAttributeIndex(dwarf::DW_AT_location);
+  if (!LocationIdx)
+    return false;
+
+  uint64_t Offset = DIE.getOffset() + getULEB128Size(Abbrev->getCode());
+  uint64_t LocationOffset, LocationEndOffset;
+  std::tie(LocationOffset, LocationEndOffset) =
+      getAttributeOffsets(Abbrev, *LocationIdx, Offset, *DIE.getDwarfUnit());
+
+  return hasValidRelocationAt(LocationOffset, LocationEndOffset, MyInfo);
+}
+
+bool DwarfLinkerForBinary::AddressManager::hasLiveAddressRange(
+    const DWARFDie &DIE, CompileUnit::DIEInfo &MyInfo) {
+  const auto *Abbrev = DIE.getAbbreviationDeclarationPtr();
+
+  Optional<uint32_t> LowPcIdx = Abbrev->findAttributeIndex(dwarf::DW_AT_low_pc);
+  if (!LowPcIdx)
+    return false;
+
+  uint64_t Offset = DIE.getOffset() + getULEB128Size(Abbrev->getCode());
+  uint64_t LowPcOffset, LowPcEndOffset;
+  std::tie(LowPcOffset, LowPcEndOffset) =
+      getAttributeOffsets(Abbrev, *LowPcIdx, Offset, *DIE.getDwarfUnit());
+
+  return hasValidRelocationAt(LowPcOffset, LowPcEndOffset, MyInfo);
+}
 /// Apply the valid relocations found by findValidRelocs() to
 /// the buffer \p Data, taking into account that Data is at \p BaseOffset
 /// in the debug_info section.
 ///
 /// Like for findValidRelocs(), this function must be called with
 /// monotonic \p BaseOffset values.
 ///
 /// \returns whether any reloc has been applied.