[DebugInfo/AccelTable] Fix inconsistency in getDIEOffset implementations

[labath]

Summary

Even though the getDIEOffset offset function was common for the two
accelerator table implementations, it was doing two different things:
for the Apple tables, it was returning the die offset relative to the
start of the section, whereas for DWARF v5 tables, it was relative to
the start of the CU.

I resolve this by renaming the function to getDIESectionOffset to make
it obvious what the function returns, and change the DWARF
implementation to return the section offset. I also keep the CU-relative
accessor, but only in the DWARF implementation (there is no way to get
this information for the Apple tables). This was not caught by existing
tests because the hand-written inputs also erroneously used section
offsets instead of CU-relative ones.

While looking at this, I noticed that the Apple implementation was not
fully correct either -- the header contains a DIEOffsetBase field, which
should be added to all offsets (but it wasn't being used). This went unnoticed
because all current writers set this field to zero anyway. I fix this as well
and add a hand-written test which demonstrates the issue.

[labath]

lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp

282

Actually, after reading the "spec" https://llvm.org/docs/SourceLevelDebugging.html#name-accelerator-tables, I'm starting to be unsure about this part. The document says: "base DIE offset that should be added to any atoms that are encoded using the DW_FORM_ref1, DW_FORM_ref2, DW_FORM_ref4, DW_FORM_ref8 or DW_FORM_ref_udata", but we our producers encode this field as DW_FORM_data4.

[JDevlieghere]

Great catch Pavel, I can imagine this preventing quite some confusion in the future. Thanks!

Requesting changes because I also don't think the DIEOffsetBase is correct, as explained inline.

lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp

282

Indeed, I don't think this is correct. While we currently don't have any atoms using this encoding, technically nothing prevents you from defining one with DW_FORM_ref* which would require using the DIEOffsetBase. I don't know the history behind this though, maybe @aprantl knows more?

[labath]

+@aprantl to weigh in on the comment.

lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp

282

So, to be correct, do you think we should check explicitly check for the specified forms here?
(Also, whatever the solution, it should probably be also applied to the getCUOffset function below).

[JDevlieghere]

lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp

282

Yes, I think so. In reality the relationship between the atom type (i.e. DW_ATOM_die_offset) and its form (ie. DW_FORM_data4) is going to be pretty much fixed, but as the form is part of the atom in the header, theoretically it could be anything.

[labath]

• Add the DIEOffsetBase only for the DW_FORM_ref*** family. There's a function in DWARFFormValue that could be of use for this, but it assumes you have the entire DWARFUnit around, and not just it's offset. As it stands now, I just hard-code the list of forms and perform the addition manually.
• Treat CU and die offsets identically.

[JDevlieghere]

LGTM!

include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h

public:
 
 
   public:
     /// Returns the Offset of the Compilation Unit associated with this
     /// Accelerator Entry or None if the Compilation Unit offset is not recorded
     /// in this Accelerator Entry.
     virtual Optional<uint64_t> getCUOffset() const = 0;
 
-    /// Returns the Offset of the Debug Info Entry associated with this
+    /// Returns the Section Offset of the Debug Info Entry associated with this
     /// Accelerator Entry or None if the DIE offset is not recorded in this
-    /// Accelerator Entry.
-    virtual Optional<uint64_t> getDIEOffset() const = 0;
+    /// Accelerator Entry. The returned offset is relative to the start of the
+    /// Section containing the DIE.
+    virtual Optional<uint64_t> getDIESectionOffset() const = 0;
 
     /// Returns the Tag of the Debug Info Entry associated with this
     /// Accelerator Entry or None if the Tag is not recorded in this
     /// Accelerator Entry.
     virtual Optional<dwarf::Tag> getTag() const = 0;
 
     /// Returns the raw values of fields in the Accelerator Entry. In general,
     /// these can only be interpreted with the help of the metadata in the
class Entry final : public DWARFAcceleratorTable::Entry {
 
     Entry(const HeaderData &Data);
     Entry() = default;
 
     void extract(const AppleAcceleratorTable &AccelTable, uint32_t *Offset);
 
   public:
     Optional<uint64_t> getCUOffset() const override;
-    Optional<uint64_t> getDIEOffset() const override;
+    Optional<uint64_t> getDIESectionOffset() const override;
     Optional<dwarf::Tag> getTag() const override;
 
     /// Returns the value of the Atom in this Accelerator Entry, if the Entry
     /// contains such Atom.
     Optional<DWARFFormValue> lookup(HeaderData::AtomType Atom) const;
 
     friend class AppleAcceleratorTable;
     friend class ValueIterator;
class Entry final : public DWARFAcceleratorTable::Entry {
     const Abbrev *Abbr;
 
     Entry(const NameIndex &NameIdx, const Abbrev &Abbr);
 
     /// Returns the Index into the Compilation Unit list of the owning Name
     /// Index or None if this Accelerator Entry does not have an associated
     /// Compilation Unit. It is up to the user to verify that the returned Index
     /// is valid in the owning NameIndex (or use getCUOffset(), which will
-    /// handle that check itself).
+    /// handle that check itself). Note that entries in NameIndexes which index
+    /// just a single Compilation Unit are implicitly associated with that unit,
+    /// so this function will return 0 even without an explicit
+    /// DW_IDX_compile_unit attribute.
     Optional<uint64_t> getCUIndex() const;
 
   public:
     Optional<uint64_t> getCUOffset() const override;
-    Optional<uint64_t> getDIEOffset() const override;
+    Optional<uint64_t> getDIESectionOffset() const override;
     Optional<dwarf::Tag> getTag() const override { return tag(); }
 
     /// .debug_names-specific getter, which always succeeds (DWARF v5 index
     /// entries always have a tag).
     dwarf::Tag tag() const { return Abbr->Tag; }
 
+    /// Returns the Offset of the DIE within the containing CU or TU.
+    Optional<uint64_t> getDIEUnitOffset() const;
+
     /// Return the Abbreviation that can be used to interpret the raw values of
     /// this Accelerator Entry.
     const Abbrev &getAbbrev() const { return *Abbr; }
 
     /// Returns the value of the Index Attribute in this Accelerator Entry, if
     /// the Entry contains such Attribute.
     Optional<DWARFFormValue> lookup(dwarf::Index Index) const;
 

lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp

AppleAcceleratorTable::Entry::lookup(HeaderData::AtomType Atom) const {
   assert(HdrData->Atoms.size() == Values.size());
   for (const auto &Tuple : zip_first(HdrData->Atoms, Values)) {
     if (std::get<0>(Tuple).first == Atom)
       return std::get<1>(Tuple);
   }
   return None;
 }
 
-Optional<uint64_t> AppleAcceleratorTable::Entry::getDIEOffset() const {
-  if (Optional<DWARFFormValue> Off = lookup(dwarf::DW_ATOM_die_offset))
-    return Off->getAsSectionOffset();
+Optional<uint64_t> AppleAcceleratorTable::Entry::getDIESectionOffset() const {
+  if (Optional<DWARFFormValue> OffForm = lookup(dwarf::DW_ATOM_die_offset)) {
+    if (Optional<uint64_t> Off = OffForm->getAsSectionOffset())
+      return *Off + HdrData->DIEOffsetBase;
+  }
   return None;
 }
 
 Optional<uint64_t> AppleAcceleratorTable::Entry::getCUOffset() const {
   if (Optional<DWARFFormValue> Off = lookup(dwarf::DW_ATOM_cu_offset))
     return Off->getAsSectionOffset();
   return None;
 }
DWARFDebugNames::Entry::lookup(dwarf::Index Index) const {
   assert(Abbr->Attributes.size() == Values.size());
   for (const auto &Tuple : zip_first(Abbr->Attributes, Values)) {
     if (std::get<0>(Tuple).Index == Index)
       return std::get<1>(Tuple);
   }
   return None;
 }
 
-Optional<uint64_t> DWARFDebugNames::Entry::getDIEOffset() const {
+Optional<uint64_t> DWARFDebugNames::Entry::getDIEUnitOffset() const {
   if (Optional<DWARFFormValue> Off = lookup(dwarf::DW_IDX_die_offset))
     return Off->getAsSectionOffset();
   return None;
 }
 
 Optional<uint64_t> DWARFDebugNames::Entry::getCUIndex() const {
   if (Optional<DWARFFormValue> Off = lookup(dwarf::DW_IDX_compile_unit))
     return Off->getAsUnsignedConstant();
+  // In a per-CU index, the entries without a DW_IDX_compile_unit attribute
+  // implicitly refer to the single CU.
+  if (NameIdx->getCUCount() == 1)
+    return 0;
   return None;
 }
 
 Optional<uint64_t> DWARFDebugNames::Entry::getCUOffset() const {
   Optional<uint64_t> Index = getCUIndex();
   if (!Index || *Index >= NameIdx->getCUCount())
     return None;
   return NameIdx->getCUOffset(*Index);
 }
 
+Optional<uint64_t> DWARFDebugNames::Entry::getDIESectionOffset() const {
+  Optional<uint64_t> CUOff = getCUOffset();
+  Optional<uint64_t> DIEOff = getDIEUnitOffset();
+  if (CUOff && DIEOff)
+    return *CUOff + *DIEOff;
+  return None;
+}
+
 void DWARFDebugNames::Entry::dump(ScopedPrinter &W) const {
   W.printHex("Abbrev", Abbr->Code);
   W.startLine() << "Tag: " << formatTag(Abbr->Tag) << "\n";
 
   assert(Abbr->Attributes.size() == Values.size());
   for (const auto &Tuple : zip_first(Abbr->Attributes, Values)) {
     W.startLine() << formatIndex(std::get<0>(Tuple).Index) << ": ";
     std::get<1>(Tuple).dump(W.getOStream());

test/tools/llvm-dwarfdump/X86/apple-names-die-offset.s

+# RUN: llvm-mc -triple x86_64-apple-darwin %s -filetype=obj -o %t
+# RUN: llvm-dwarfdump -find=main %t | FileCheck %s
+
+# CHECK: DW_TAG_subprogram
+# CHECK-NEXT: DW_AT_name ("main")
+# CHECK-NEXT: DW_AT_external
+
+  .section  __DWARF,__debug_str,regular,debug
+Ldebug_str:
+Lstring_producer:
+  .asciz  "Hand-written dwarf"
+Lstring_main:
+  .asciz  "main"
+
+  .section  __DWARF,__debug_abbrev,regular,debug
+  .byte 1                       # Abbreviation Code
+  .byte 17                      # DW_TAG_compile_unit
+  .byte 1                       # DW_CHILDREN_yes
+  .byte 37                      # DW_AT_producer
+  .byte 14                      # DW_FORM_strp
+  .byte 19                      # DW_AT_language
+  .byte 5                       # DW_FORM_data2
+  .byte 0                       # EOM(1)
+  .byte 0                       # EOM(2)
+  .byte 2                       # Abbreviation Code
+  .byte 46                      # DW_TAG_subprogram
+  .byte 0                       # DW_CHILDREN_no
+  .byte 3                       # DW_AT_name
+  .byte 14                      # DW_FORM_strp
+  .byte 63                      # DW_AT_external
+  .byte 25                      # DW_FORM_flag_present
+  .byte 0                       # EOM(1)
+  .byte 0                       # EOM(2)
+  .byte 0                       # EOM(3)
+
+  .section  __DWARF,__debug_info,regular,debug
+Ldebug_info:
+  .long Lcu_end0-Lcu_start0   # Length of Unit
+Lcu_start0:
+  .short  4                       # DWARF version number
+  .long 0                       # Offset Into Abbrev. Section
+  .byte 8                       # Address Size (in bytes)
+  .byte 1                       # Abbrev [1] DW_TAG_compile_unit
+  .long Lstring_producer-Ldebug_str # DW_AT_producer
+  .short  12                      # DW_AT_language
+Ldie_main:
+  .byte 2                       # Abbrev [2] DW_TAG_subprogram
+  .long Lstring_main-Ldebug_str # DW_AT_name
+                                        # DW_AT_external
+  .byte 0                       # End Of Children Mark
+Lcu_end0:
+
+  .section  __DWARF,__apple_names,regular,debug
+Lnames_begin:
+  .long 1212240712              ## Header Magic
+  .short  1                       ## Header Version
+  .short  0                       ## Header Hash Function
+  .long 1                       ## Header Bucket Count
+  .long 1                       ## Header Hash Count
+  .long 12                      ## Header Data Length
+  .long 1                       ## HeaderData Die Offset Base
+  .long 1                       ## HeaderData Atom Count
+  .short  1                       ## DW_ATOM_die_offset
+  .short  6                       ## DW_FORM_data4
+  .long 0                       ## Bucket 0
+  .long 2090499946              ## Hash in Bucket 0
+  .long LNames0-Lnames_begin    ## Offset in Bucket 0
+LNames0:
+  .long Lstring_main-Ldebug_str ## main
+  .long 1                       ## Num DIEs
+  .long Ldie_main-Ldebug_info-1
+  .long 0

test/tools/llvm-dwarfdump/X86/debug-names-find.s

.Lnames_abbrev_start0:
   .byte 6                       # DW_FORM_data4
   .byte 0                       # End of abbrev
   .byte 0                       # End of abbrev
   .byte 0                       # End of abbrev list
 .Lnames_abbrev_end0:
 .Lnames_entries0:
 .Lnames0:
   .byte 46                      # Abbrev code
-  .long .Ldie_bar               # DW_IDX_die_offset
+  .long .Ldie_bar-.Lcu_begin0   # DW_IDX_die_offset
   .long 0                       # End of list: bar
 .Lnames1:
   .byte 46                      # Abbrev code
-  .long .Ldie_foo               # DW_IDX_die_offset
+  .long .Ldie_foo-.Lcu_begin0   # DW_IDX_die_offset
   .long 0                       # End of list: foo
 .Lnames2:
   .byte 46                      # Abbrev code
-  .long .Ldie_foo               # DW_IDX_die_offset
+  .long .Ldie_foo-.Lcu_begin0   # DW_IDX_die_offset
   .long 0                       # End of list: _Z3foov
   .p2align  2
 .Lnames_end0:
 
 # Index of compilation unit 1. This one has no hash table.
   .long .Lnames_end1-.Lnames_start1 # Header: contribution length
 .Lnames_start1:
   .short  5                       # Header: version
.Lnames_abbrev_start1:
   .byte 6                       # DW_FORM_data4
   .byte 0                       # End of abbrev
   .byte 0                       # End of abbrev
   .byte 0                       # End of abbrev list
 .Lnames_abbrev_end1:
 .Lnames_entries1:
 .Lnames3:
   .byte 46                      # Abbrev code
-  .long .Ldie_baz               # DW_IDX_die_offset
+  .long .Ldie_baz-.Lcu_begin1   # DW_IDX_die_offset
   .long 0                       # End of list: baz
   .p2align  2
 .Lnames4:
   .byte 46                      # Abbrev code
-  .long .Ldie_bazz              # DW_IDX_die_offset
+  .long .Ldie_bazz-.Lcu_begin1  # DW_IDX_die_offset
   .long 0                       # End of list: baz
   .p2align  2
 .Lnames_end1:

tools/llvm-dwarfdump/llvm-dwarfdump.cpp

 
 bool collectStatsForObjectFile(ObjectFile &Obj, DWARFContext &DICtx,
                                Twine Filename, raw_ostream &OS);
 
 template <typename AccelTable>
 static llvm::Optional<uint64_t> getDIEOffset(const AccelTable &Accel,
                                        StringRef Name) {
   for (const auto &Entry : Accel.equal_range(Name))
-    if (llvm::Optional<uint64_t> Off = Entry.getDIEOffset())
+    if (llvm::Optional<uint64_t> Off = Entry.getDIESectionOffset())
       return *Off;
   return None;
 }
 
 static bool dumpObjectFile(ObjectFile &Obj, DWARFContext &DICtx, Twine Filename,
                            raw_ostream &OS) {
   logAllUnhandledErrors(DICtx.loadRegisterInfo(Obj), errs(),
                         Filename.str() + ": ");