[Lldb-commits] [lldb] [lldb][DWARFIndex] Use IDX_parent to implement GetFullyQualifiedType query (PR #79932)

[Felipe de Azevedo Piovezan via lldb-commits]

================
@@ -218,6 +219,104 @@ void DebugNamesDWARFIndex::GetCompleteObjCClass(
   m_fallback.GetCompleteObjCClass(class_name, must_be_implementation, callback);
 }
 
+namespace {
+using Entry = llvm::DWARFDebugNames::Entry;
+
+/// If `entry` and all of its parents have an `IDX_parent`, use that information
+/// to build and return a list of at most `max_parents` parent Entries.
+/// `entry` itself is not included in the list.
+/// If any parent does not have an `IDX_parent`, or the Entry data is corrupted,
+/// nullopt is returned.
+static std::optional<llvm::SmallVector<Entry, 4>>
+getParentChain(Entry entry, uint32_t max_parents) {
+  llvm::SmallVector<Entry, 4> parent_entries;
+
+  do {
+    if (!entry.hasParentInformation())
+      return std::nullopt;
+
+    llvm::Expected<std::optional<Entry>> parent = entry.getParentDIEEntry();
+    if (!parent) { // Bad data.
+      consumeError(parent.takeError());
+      return std::nullopt;
+    }
+
+    // Last parent in the chain
+    if (!parent->has_value())
+      break;
+
+    parent_entries.push_back(**parent);
+    entry = **parent;
+  } while (parent_entries.size() < max_parents);
+
+  return parent_entries;
+}
+} // namespace
+
+void DebugNamesDWARFIndex::GetFullyQualifiedType(
+    const DWARFDeclContext &context,
+    llvm::function_ref<bool(DWARFDIE die)> callback) {
+  if (context.GetSize() == 0)
+    return;
+
+  // Fallback: use the base class implementation.
+  auto fallback_impl = [&](const DebugNames::Entry &entry) {
+    return ProcessEntry(entry, [&](DWARFDIE die) {
+      return GetFullyQualifiedTypeImpl(context, die, callback);
+    });
+  };
+
+  llvm::StringRef leaf_name = context[0].name;
+  llvm::SmallVector<llvm::StringRef> parent_names;
+  for (auto idx : llvm::seq<int>(1, context.GetSize()))
+    parent_names.emplace_back(context[idx].name);
+
+  for (const DebugNames::Entry &entry :
+       m_debug_names_up->equal_range(leaf_name)) {
+    if (!isType(entry.tag()))
+      continue;
+
+    // Grab at most one extra parent, subsequent parents are not necessary to
+    // test equality.
+    auto parent_chain = getParentChain(entry, parent_names.size() + 1);
+
+    if (!parent_chain) {
+      if (!fallback_impl(entry))
+        return;
+      continue;
+    }
+
+    if (SameParentChain(parent_names, *parent_chain) &&
+        !ProcessEntry(entry, callback))
+      return;
+  }
+}
+
+bool DebugNamesDWARFIndex::SameParentChain(
+    llvm::ArrayRef<llvm::StringRef> parent_names,
+    llvm::ArrayRef<DebugNames::Entry> parent_entries) const {
+
+  if (parent_entries.size() != parent_names.size())
+    return false;
+
+  auto SameAsEntryATName = [this](llvm::StringRef name,
+                                  const DebugNames::Entry &entry) {
+    auto maybe_dieoffset = entry.getDIEUnitOffset();
+    if (!maybe_dieoffset)
+      return false;
+    auto die_ref = ToDIERef(entry);
+    if (!die_ref)
+      return false;
+    return name == m_debug_info.PeekDIEName(*die_ref);
+  };
+
----------------
felipepiovezan wrote:

I'm not attached to the lambda, but I would like to clarify my original intent with the lambda and then double check if you still prefer the inlined version with that intent in mind.

My intent was not to re-use code, but rather to give name to things and reduce cognitive burden. If we go with the suggestion, this is what we end up with:

```
  for (auto [parent_name, parent_entry] :
        llvm::zip_equal(parent_names, parent_entries))
    auto maybe_dieoffset = entry.getDIEUnitOffset();
     if (!maybe_dieoffset)
       return false;
     auto die_ref = ToDIERef(entry);
     if (!die_ref)
       return false;
     return name == m_debug_info.PeekDIEName(*die_ref);
```

A reader needs to go through that entire block and conclude: "Oh, this is just comparing the AT_name with `name`.

With the current version, the code _tells_ you what it is doing 
`if (!SameAsEntryATName(parent_name, parent_entry))`

With that in mind, if you still prefer the inlined version, I'll be happy to update it!

(the same rationale applies to the other comment about lambdas)



https://github.com/llvm/llvm-project/pull/79932