[lldb][DWARF] Change GetAttributes to always visit current DIE before recursing

`GetAttributes` returns all attributes on a given DIE, including any attributes in referenced `DW_AT_abstract_origin` or `DW_AT_specification`. However, if an attribute exists on both the referring DIE and the referenced DIE, the first one encountered will be the one that takes precendence when querying the returned `DWARFAttributes`. There is currently no good way of ensuring that an attribute of a definition doesn't get shadowed by one found on the declaration. One use-case where we don't want this to happen is for `DW_AT_object_pointer` (which can exist on both definitions and declarations, see #123089).

This patch makes sure we visit the current DIE's attributes before
following DIE references. I tried keeping as much of the original
`GetAttributes` unchanged and just add an outer `GetAttributes` that
keeps track of the DIEs we need to visit next.

There's precendent for this iteration order in
`llvm::DWARFDie::findRecursively` and also
`lldb_private::ElaboratingDIEIterator`. We could use the latter to
implement `GetAttributes`, though it also follows `DW_AT_signature` so I
decided to leave it for follow-up.

Author: Michael137

lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp

@@ -281,22 +281,28 @@ bool DWARFDebugInfoEntry::GetDIENamesAndRanges(
   return !ranges.empty();
 }
 
-// Get all attribute values for a given DIE, including following any
-// specification or abstract origin attributes and including those in the
-// results. Any duplicate attributes will have the first instance take
-// precedence (this can happen for declaration attributes).
-void DWARFDebugInfoEntry::GetAttributes(DWARFUnit *cu,
-                                        DWARFAttributes &attributes,
-                                        Recurse recurse,
-                                        uint32_t curr_depth) const {
-  const auto *abbrevDecl = GetAbbreviationDeclarationPtr(cu);
-  if (!abbrevDecl) {
-    attributes.Clear();
-    return;
-  }
+bool DWARFDebugInfoEntry::GetAttributes(
+    DWARFUnit const *cu, llvm::SmallVector<DWARFDIE> &worklist,
+    llvm::SmallSet<DWARFDebugInfoEntry const *, 3> &seen,
+    DWARFAttributes &attributes) const {
+  assert(!worklist.empty());
+  assert(cu);
+
+  DWARFDIE current = worklist.pop_back_val();
+
+  const auto *entry = current.GetDIE();
+  if (!entry)
+    return false;
+
+  const auto *abbrevDecl =
+      entry->GetAbbreviationDeclarationPtr(current.GetCU());
+  if (!abbrevDecl)
+    return false;
 
   const DWARFDataExtractor &data = cu->GetData();
-  lldb::offset_t offset = GetFirstAttributeOffset();
+  lldb::offset_t offset = current.GetDIE()->GetFirstAttributeOffset();
+
+  const bool is_first_die = seen.size() == 1;
 
   for (const auto &attribute : abbrevDecl->attributes()) {
     DWARFFormValue form_value(cu);
@@ -309,10 +315,10 @@ void DWARFDebugInfoEntry::GetAttributes(DWARFUnit *cu,
     switch (attr) {
     case DW_AT_sibling:
     case DW_AT_declaration:
-      if (curr_depth > 0) {
+      if (seen.size() > 1 && !is_first_die) {
         // This attribute doesn't make sense when combined with the DIE that
         // references this DIE. We know a DIE is referencing this DIE because
-        // curr_depth is not zero
+        // we've visited more than one DIE already.
         break;
       }
       [[fallthrough]];
@@ -321,13 +327,12 @@ void DWARFDebugInfoEntry::GetAttributes(DWARFUnit *cu,
       break;
     }
 
-    if (recurse == Recurse::yes &&
-        ((attr == DW_AT_specification) || (attr == DW_AT_abstract_origin))) {
+    if (attr == DW_AT_specification || attr == DW_AT_abstract_origin) {
       if (form_value.ExtractValue(data, &offset)) {
-        DWARFDIE spec_die = form_value.Reference();
-        if (spec_die)
-          spec_die.GetDIE()->GetAttributes(spec_die.GetCU(), attributes,
-                                           recurse, curr_depth + 1);
+        if (DWARFDIE spec_die = form_value.Reference()) {
+          if (seen.insert(spec_die.GetDIE()).second)
+            worklist.push_back(spec_die);
+        }
       }
     } else {
       const dw_form_t form = form_value.Form();
@@ -339,6 +344,39 @@ void DWARFDebugInfoEntry::GetAttributes(DWARFUnit *cu,
         DWARFFormValue::SkipValue(form, data, &offset, cu);
     }
   }
+
+  return true;
+}
+
+DWARFAttributes DWARFDebugInfoEntry::GetAttributes(DWARFUnit *cu,
+                                                   Recurse recurse) const {
+  // FIXME: use ElaboratingDIEIterator to follow specifications/abstract origins
+  // instead of maintaining our own worklist/seen list.
+
+  DWARFAttributes attributes;
+
+  llvm::SmallVector<DWARFDIE, 3> worklist;
+  worklist.emplace_back(cu, this);
+
+  // Keep track if DIEs already seen to prevent infinite recursion.
+  // Value of '3' was picked for the same reason that
+  // DWARFDie::findRecursively does.
+  llvm::SmallSet<DWARFDebugInfoEntry const *, 3> seen;
+  seen.insert(this);
+
+  while (!worklist.empty()) {
+    if (!GetAttributes(cu, worklist, seen, attributes)) {
+      attributes.Clear();
+      break;
+    }
+
+    // We visited the current DIE already and were asked not to check the
+    // rest of the worklist. So bail out.
+    if (recurse == Recurse::no)
+      break;
+  }
+
+  return attributes;
 }
 
 // GetAttributeValue

lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h

@@ -52,12 +52,21 @@ class DWARFDebugInfoEntry {
                lldb::offset_t *offset_ptr);
 
   using Recurse = DWARFBaseDIE::Recurse;
+
+  // Get all attribute values for a given DIE, including following any
+  // specification or abstract origin attributes and including those in the
+  // results.
+  //
+  // When following specifications/abstract origins, the attributes
+  // on the referring DIE are guaranteed to be visited before the attributes of
+  // the referenced DIE.
+  //
+  // \param[in]
+  // \param[in]
+  //
+  // \returns output may have duplicate entries
   DWARFAttributes GetAttributes(DWARFUnit *cu,
-                                Recurse recurse = Recurse::yes) const {
-    DWARFAttributes attrs;
-    GetAttributes(cu, attrs, recurse, 0 /* curr_depth */);
-    return attrs;
-  }
+                                Recurse recurse = Recurse::yes) const;
 
   dw_offset_t GetAttributeValue(const DWARFUnit *cu, const dw_attr_t attr,
                                 DWARFFormValue &formValue,
@@ -180,8 +189,10 @@ class DWARFDebugInfoEntry {
   dw_tag_t m_tag = llvm::dwarf::DW_TAG_null;
 
 private:
-  void GetAttributes(DWARFUnit *cu, DWARFAttributes &attrs, Recurse recurse,
-                     uint32_t curr_depth) const;
+  // Helper for the public \ref DWARFDebugInfoEntry::GetAttributes API.
+  bool GetAttributes(DWARFUnit const *cu, llvm::SmallVector<DWARFDIE> &worklist,
+                     llvm::SmallSet<DWARFDebugInfoEntry const *, 3> &seen,
+                     DWARFAttributes &attributes) const;
 };
 } // namespace dwarf
 } // namespace lldb_private::plugin