[Lldb-commits] [lldb] Improve type and namespace lookup using parent chain (PR #108907)

via lldb-commits lldb-commits at lists.llvm.org
Mon Sep 16 18:16:01 PDT 2024


https://github.com/jeffreytan81 created https://github.com/llvm/llvm-project/pull/108907

## Context
We have a customer reporting scenario that expanding "this" pointer for a non-trivial class method context takes more than **70~90 seconds**. This is a linux target with .debug_names index table enabled and split dwarf dwo files. 

Profiling shows the majority of the bottleneck is spent in `SymbolFileDWARF::FindTypes()` and `SymbolFileDWARF:: FindNamespace()` two functions searching types/namespaces in .debug_names index table with base name, then parsing/loading the underlying dwo files which can be very slow. 

## Context
This PR improves `SymbolFileDWARF::FindTypes()` and `SymbolFileDWARF:: FindNamespace()` by using the newly added parent chain in .debug_names. 

>From 6e84ab9a14e63c58e1facdbf9a695c093882b37b Mon Sep 17 00:00:00 2001
From: jeffreytan81 <jeffreytan at fb.com>
Date: Mon, 19 Aug 2024 10:57:35 -0700
Subject: [PATCH 1/4] Fix StartDebuggingRequestHandler/ReplModeRequestHandler
 in lldb-dap

---
 lldb/tools/lldb-dap/DAP.h        | 2 --
 lldb/tools/lldb-dap/lldb-dap.cpp | 4 ++--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h
index 57562a14983519..7828272aa15a7d 100644
--- a/lldb/tools/lldb-dap/DAP.h
+++ b/lldb/tools/lldb-dap/DAP.h
@@ -192,8 +192,6 @@ struct DAP {
   std::mutex call_mutex;
   std::map<int /* request_seq */, ResponseCallback /* reply handler */>
       inflight_reverse_requests;
-  StartDebuggingRequestHandler start_debugging_request_handler;
-  ReplModeRequestHandler repl_mode_request_handler;
   ReplMode repl_mode;
   std::string command_escape_prefix = "`";
   lldb::SBFormat frame_format;
diff --git a/lldb/tools/lldb-dap/lldb-dap.cpp b/lldb/tools/lldb-dap/lldb-dap.cpp
index ea84f31aec3a6c..f50a6c17310739 100644
--- a/lldb/tools/lldb-dap/lldb-dap.cpp
+++ b/lldb/tools/lldb-dap/lldb-dap.cpp
@@ -1627,12 +1627,12 @@ void request_initialize(const llvm::json::Object &request) {
       "lldb-dap", "Commands for managing lldb-dap.");
   if (GetBoolean(arguments, "supportsStartDebuggingRequest", false)) {
     cmd.AddCommand(
-        "startDebugging", &g_dap.start_debugging_request_handler,
+        "startDebugging", new StartDebuggingRequestHandler(),
         "Sends a startDebugging request from the debug adapter to the client "
         "to start a child debug session of the same type as the caller.");
   }
   cmd.AddCommand(
-      "repl-mode", &g_dap.repl_mode_request_handler,
+      "repl-mode", new ReplModeRequestHandler(),
       "Get or set the repl behavior of lldb-dap evaluation requests.");
 
   g_dap.progress_event_thread = std::thread(ProgressEventThreadFunction);

>From 30e8b1870004ae6ca9319680f6ef16e173415026 Mon Sep 17 00:00:00 2001
From: jeffreytan81 <jeffreytan at fb.com>
Date: Mon, 16 Sep 2024 12:02:48 -0700
Subject: [PATCH 2/4] Improve type/namespace lookup using parent chain

---
 .../Plugins/SymbolFile/DWARF/DWARFIndex.cpp   |  59 ++++++
 .../Plugins/SymbolFile/DWARF/DWARFIndex.h     |  22 ++
 .../SymbolFile/DWARF/DebugNamesDWARFIndex.cpp | 168 +++++++++++++--
 .../SymbolFile/DWARF/DebugNamesDWARFIndex.h   |  29 +++
 .../SymbolFile/DWARF/SymbolFileDWARF.cpp      | 192 +++++++++++-------
 .../SymbolFile/DWARF/SymbolFileDWARF.h        |   2 +
 6 files changed, 385 insertions(+), 87 deletions(-)

diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp
index eafddbad03f57b..7bdea4eae4529b 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp
@@ -126,3 +126,62 @@ bool DWARFIndex::GetFullyQualifiedTypeImpl(
     return callback(die);
   return true;
 }
+
+void DWARFIndex::GetNamespacesWithParents(
+    ConstString name, llvm::ArrayRef<llvm::StringRef> parent_names,
+    llvm::function_ref<bool(DWARFDIE die)> callback) {
+  GetNamespaces(name, [&](DWARFDIE die) {
+    return ProcessDieMatchParentNames(name, parent_names, die, callback);
+  });
+}
+
+void DWARFIndex::GetTypesWithParents(
+    ConstString name, llvm::ArrayRef<llvm::StringRef> parent_names,
+    llvm::function_ref<bool(DWARFDIE die)> callback) {
+  GetTypes(name, [&](DWARFDIE die) {
+    return ProcessDieMatchParentNames(name, parent_names, die, callback);
+  });
+}
+
+bool DWARFIndex::ProcessDieMatchParentNames(
+    ConstString name, llvm::ArrayRef<llvm::StringRef> query_parent_names,
+    DWARFDIE die, llvm::function_ref<bool(DWARFDIE die)> callback) {
+  std::vector<lldb_private::CompilerContext> type_context =
+      die.GetTypeLookupContext();
+  if (type_context.empty()) {
+    // If both type_context and query_parent_names and empty we have a match.
+    // Otherwise, this one does not match and we keep on searching. 
+    if (query_parent_names.empty())
+      return callback(die);
+    return true;
+  }
+
+  // Type lookup context includes the current DIE as the last element.
+  // so revert it for easy matching.
+  std::reverse(type_context.begin(), type_context.end());
+
+  // type_context includes the name of the current DIE while query_parent_names
+  // doesn't. So start check from index 1 for dwarf_decl_ctx.
+  uint32_t i = 1, j = 0;
+  while (i < type_context.size() && j < query_parent_names.size()) {
+    // If type_context[i] has no name, skip it.
+    // e.g. this can happen for anonymous namespaces.
+    if (type_context[i].name.IsNull() || type_context[i].name.IsEmpty()) {
+      ++i;
+      continue;
+    }
+    // If the name doesn't match, skip it.
+    // e.g. this can happen for inline namespaces.
+    if (query_parent_names[j] != type_context[i].name) {
+      ++i;
+      continue;
+    }
+    ++i;
+    ++j;
+  }
+  // If not all query_parent_names were found in type_context.
+  // This die does not meet the criteria, try next one.
+  if (j != query_parent_names.size())
+    return true;
+  return callback(die);
+}
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.h b/lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.h
index cb3ae8a06d7885..1f457fda0282f5 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.h
@@ -64,6 +64,23 @@ class DWARFIndex {
   virtual void
   GetNamespaces(ConstString name,
                 llvm::function_ref<bool(DWARFDIE die)> callback) = 0;
+
+  /// Get type DIEs whose base name match \param name with \param parent_names
+  /// in its decl parent chain as subset.  A base implementation is provided,
+  /// Specializations should override this if they are able to provide a faster
+  /// implementation.
+  virtual void
+  GetTypesWithParents(ConstString name,
+                      llvm::ArrayRef<llvm::StringRef> parent_names,
+                      llvm::function_ref<bool(DWARFDIE die)> callback);
+  /// Get namespace DIEs whose base name match \param name with \param
+  /// parent_names in its decl parent chain as subset.  A base implementation is
+  /// provided. Specializations should override this if they are able to provide
+  /// a faster implementation.
+  virtual void
+  GetNamespacesWithParents(ConstString name,
+                           llvm::ArrayRef<llvm::StringRef> parent_names,
+                           llvm::function_ref<bool(DWARFDIE die)> callback);
   virtual void
   GetFunctions(const Module::LookupInfo &lookup_info, SymbolFileDWARF &dwarf,
                const CompilerDeclContext &parent_decl_ctx,
@@ -115,6 +132,11 @@ class DWARFIndex {
   bool
   GetFullyQualifiedTypeImpl(const DWARFDeclContext &context, DWARFDIE die,
                             llvm::function_ref<bool(DWARFDIE die)> callback);
+
+  /// Check if the \a die has \a parent_names in its decl parent chain.
+  bool ProcessDieMatchParentNames(
+      ConstString name, llvm::ArrayRef<llvm::StringRef> parent_names,
+      DWARFDIE die, llvm::function_ref<bool(DWARFDIE die)> callback);
 };
 } // namespace dwarf
 } // namespace lldb_private::plugin
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
index 32d8a92305aafa..c84fc0b5c60717 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
@@ -16,6 +16,8 @@
 #include "lldb/Utility/Stream.h"
 #include "llvm/ADT/Sequence.h"
 #include <optional>
+#include "lldb/Core/Progress.h"
+#include "lldb/Utility/LLDBLog.h"
 
 using namespace lldb_private;
 using namespace lldb;
@@ -301,7 +303,8 @@ using Entry = llvm::DWARFDebugNames::Entry;
 /// If any parent does not have an `IDX_parent`, or the Entry data is corrupted,
 /// nullopt is returned.
 std::optional<llvm::SmallVector<Entry, 4>>
-getParentChain(Entry entry, uint32_t max_parents) {
+getParentChain(Entry entry,
+               uint32_t max_parents = std::numeric_limits<uint32_t>::max()) {
   llvm::SmallVector<Entry, 4> parent_entries;
 
   do {
@@ -374,25 +377,40 @@ void DebugNamesDWARFIndex::GetFullyQualifiedType(
   m_fallback.GetFullyQualifiedType(context, callback);
 }
 
+bool DebugNamesDWARFIndex::SameAsEntryATName(
+    llvm::StringRef query_name, const DebugNames::Entry &entry) const {
+  auto maybe_dieoffset = entry.getDIEUnitOffset();
+  if (!maybe_dieoffset)
+    return false;
+
+  // [Optimization] instead of parsing the entry from dwo file, we simply
+  // check if the query_name can point to an entry of the same DIE offset.
+  // This greatly reduced number of dwo file parsed and thus improved the
+  // performance.
+  for (const DebugNames::Entry &query_entry :
+       entry.getNameIndex()->equal_range(query_name)) {
+    auto query_dieoffset = query_entry.getDIEUnitOffset();
+    if (!query_dieoffset)
+      continue;
+
+    if (*query_dieoffset == *maybe_dieoffset) {
+      return true;
+    } else if (*query_dieoffset > *maybe_dieoffset) {
+      // The pool entries of the same name are sequentially cluttered together
+      // so if the query name from `query_name` is after the target entry, this
+      // is definitely not the correct one, we can stop searching.
+      return false;
+    }
+  }
+  return false;
+}
+
 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) {
-    // Peek at the AT_name of `entry` and test equality to `name`.
-    auto maybe_dieoffset = entry.getDIEUnitOffset();
-    if (!maybe_dieoffset)
-      return false;
-    DWARFUnit *unit = GetNonSkeletonUnit(entry);
-    if (!unit)
-      return false;
-    return name == unit->PeekDIEName(unit->GetOffset() + *maybe_dieoffset);
-  };
-
   // If the AT_name of any parent fails to match the expected name, we don't
   // have a match.
   for (auto [parent_name, parent_entry] :
@@ -402,6 +420,36 @@ bool DebugNamesDWARFIndex::SameParentChain(
   return true;
 }
 
+bool DebugNamesDWARFIndex::WithinParentChain(
+    llvm::ArrayRef<llvm::StringRef> query_parent_names,
+    llvm::ArrayRef<DebugNames::Entry> parent_chain) const {
+  if (parent_chain.size() < query_parent_names.size())
+    return false;
+
+  size_t query_idx = 0, chain_idx = 0;
+  while (query_idx < query_parent_names.size() &&
+         chain_idx < parent_chain.size()) {
+    if (SameAsEntryATName(query_parent_names[query_idx],
+                          parent_chain[chain_idx])) {
+      ++query_idx;
+      ++chain_idx;
+    } else {
+      // Name does not match, try next parent_chain entry if the current entry
+      // is namespace because the current one can be an inline namespace.
+      if (parent_chain[chain_idx].tag() != DW_TAG_namespace)
+        return false;
+
+      ++chain_idx;
+      if (query_parent_names.size() - query_idx >
+          parent_chain.size() - chain_idx) {
+        // Parent chain has not enough entries, we can't possibly have a match.
+        return false;
+      }
+    }
+  }
+  return query_idx == query_parent_names.size();
+}
+
 void DebugNamesDWARFIndex::GetTypes(
     ConstString name, llvm::function_ref<bool(DWARFDIE die)> callback) {
   for (const DebugNames::Entry &entry :
@@ -444,6 +492,98 @@ void DebugNamesDWARFIndex::GetNamespaces(
   m_fallback.GetNamespaces(name, callback);
 }
 
+void DebugNamesDWARFIndex::GetNamespacesWithParents(
+    ConstString name, llvm::ArrayRef<llvm::StringRef> parent_names,
+    llvm::function_ref<bool(DWARFDIE die)> callback) {
+  Progress progress("Get namespace from index for %s", name.GetCString());
+  for (const DebugNames::Entry &entry :
+       m_debug_names_up->equal_range(name.GetStringRef())) {
+    lldb_private::dwarf::Tag entry_tag = entry.tag();
+    if (entry_tag == DW_TAG_namespace ||
+        entry_tag == DW_TAG_imported_declaration) {
+      std::optional<llvm::SmallVector<Entry, 4>> parent_chain =
+          getParentChain(entry);
+      if (!parent_chain) {
+        // Fallback: use the base class implementation.
+        if (!ProcessEntry(entry, [&](DWARFDIE die) {
+              return ProcessDieMatchParentNames(name, parent_names, die, callback);
+            }))
+          return;
+        continue;
+      }
+
+      if (parent_chain->size() < parent_names.size())
+        continue;
+      else if (parent_chain->size() == parent_names.size()) {
+        if (SameParentChain(parent_names, *parent_chain) &&
+            !ProcessEntry(entry, callback))
+          return;
+      } else {
+        // parent_chain->size() > parent_names.size()
+        if (WithinParentChain(parent_names, *parent_chain) &&
+            !ProcessEntry(entry, callback))
+          return;
+      }
+    }
+  }
+
+  m_fallback.GetNamespaces(name, callback);
+}
+
+void DebugNamesDWARFIndex::GetTypesWithParents(
+    ConstString name, llvm::ArrayRef<llvm::StringRef> parent_names,
+    llvm::function_ref<bool(DWARFDIE die)> callback) {
+  if (parent_names.empty())
+    return GetTypes(name, callback);
+    
+  Log *log = GetLog(LLDBLog::Temporary);
+  int count = 0;
+  Progress progress("GetTypesWithParents from index for %s", name.GetCString());
+  // For each entry, grab its parent chain and check if we have a match.
+  for (const DebugNames::Entry &entry : m_debug_names_up->equal_range(name)) {
+    if (!isType(entry.tag()))
+      continue;
+
+    // If we get a NULL foreign_tu back, the entry doesn't match the type unit
+    // in the .dwp file, or we were not able to load the .dwo file or the DWO ID
+    // didn't match.
+    std::optional<DWARFTypeUnit *> foreign_tu = GetForeignTypeUnit(entry);
+    if (foreign_tu && foreign_tu.value() == nullptr)
+      continue;
+
+    // Grab at most one extra parent, subsequent parents are not necessary to
+    // test equality.
+    std::optional<llvm::SmallVector<Entry, 4>> parent_chain =
+        getParentChain(entry);
+    ++count;
+    if (!parent_chain) {
+      // Fallback: use the base class implementation.
+      if (!ProcessEntry(entry, [&](DWARFDIE die) {
+            return ProcessDieMatchParentNames(name, parent_names, die, callback);
+          }))
+        return;
+      continue;
+    }
+
+    if (parent_chain->size() < parent_names.size())
+      continue;
+    else if (parent_chain->size() == parent_names.size()) {
+      if (SameParentChain(parent_names, *parent_chain) &&
+          !ProcessEntry(entry, callback))
+        return;
+    } else {
+      // parent_chain->size() > parent_names.size()
+      if (WithinParentChain(parent_names, *parent_chain) &&
+          !ProcessEntry(entry, callback))
+        return;
+    }
+  }
+  LLDB_LOG(log,
+           "GetTypesWithParents searched {0} entries for {1} but found none",
+           count, name.GetCString());
+  m_fallback.GetTypesWithParents(name, parent_names, callback);
+}
+
 void DebugNamesDWARFIndex::GetFunctions(
     const Module::LookupInfo &lookup_info, SymbolFileDWARF &dwarf,
     const CompilerDeclContext &parent_decl_ctx,
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.h b/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.h
index cb15c1d4f994b3..9d76ddaa043e2b 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.h
@@ -52,6 +52,14 @@ class DebugNamesDWARFIndex : public DWARFIndex {
                 llvm::function_ref<bool(DWARFDIE die)> callback) override;
   void GetNamespaces(ConstString name,
                      llvm::function_ref<bool(DWARFDIE die)> callback) override;
+  void
+  GetTypesWithParents(ConstString name,
+                      llvm::ArrayRef<llvm::StringRef> parent_names,
+                      llvm::function_ref<bool(DWARFDIE die)> callback) override;
+  void GetNamespacesWithParents(
+      ConstString name, llvm::ArrayRef<llvm::StringRef> parent_names,
+      llvm::function_ref<bool(DWARFDIE die)> callback) override;
+
   void GetFunctions(const Module::LookupInfo &lookup_info,
                     SymbolFileDWARF &dwarf,
                     const CompilerDeclContext &parent_decl_ctx,
@@ -118,6 +126,27 @@ class DebugNamesDWARFIndex : public DWARFIndex {
   bool SameParentChain(llvm::ArrayRef<llvm::StringRef> parent_names,
                        llvm::ArrayRef<DebugNames::Entry> parent_entries) const;
 
+  /// Returns true if \a parent_names entries are within \a parent_chain.
+  /// This is diffferent from SameParentChain() which checks for exact match.
+  /// This function is required because \a parent_chain can contain inline
+  /// namespace entries which may not be specified in parent_names by client.
+  ///
+  /// \param[in] parent_names
+  ///   The list of parent names to check for.
+  ///
+  /// \param[in] parent_chain
+  ///   The fully qualified parent chain entries from .debug_names index table
+  ///   to check against.
+  ///
+  /// \returns
+  ///   True if all \a parent_names entries are can be sequentially found inside
+  ///   \a parent_chain, otherwise False.
+  bool WithinParentChain(llvm::ArrayRef<llvm::StringRef> parent_names,
+                         llvm::ArrayRef<DebugNames::Entry> parent_chain) const;
+
+  /// Returns true if .debug_names pool entry \p entry has name \p query_name.
+  bool SameAsEntryATName(llvm::StringRef query_name, const DebugNames::Entry &entry) const;
+
   static void MaybeLogLookupError(llvm::Error error,
                                   const DebugNames::NameIndex &ni,
                                   llvm::StringRef name);
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index f721ca00fd3559..833b1ae87dc873 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -2731,6 +2731,22 @@ uint64_t SymbolFileDWARF::GetDebugInfoSize(bool load_all_debug_info) {
   return debug_info_size;
 }
 
+llvm::SmallVector<llvm::StringRef>
+SymbolFileDWARF::GetTypeQueryParentNames(TypeQuery query) {
+  std::vector<lldb_private::CompilerContext> &query_decl_context =
+      query.GetContextRef();
+  llvm::SmallVector<llvm::StringRef> parent_names;
+  if (!query_decl_context.empty()) {
+    auto rbegin = query_decl_context.rbegin(), rend = query_decl_context.rend();
+    for (auto rit = rbegin + 1; rit != rend; ++rit)
+      if ((rit->kind & CompilerContextKind::AnyType) !=
+              CompilerContextKind::Invalid &&
+          !rit->name.IsEmpty())
+        parent_names.push_back(rit->name.GetStringRef());
+  }
+  return parent_names;
+}
+
 void SymbolFileDWARF::FindTypes(const TypeQuery &query, TypeResults &results) {
 
   // Make sure we haven't already searched this SymbolFile before.
@@ -2748,45 +2764,50 @@ void SymbolFileDWARF::FindTypes(const TypeQuery &query, TypeResults &results) {
 
   std::lock_guard<std::recursive_mutex> guard(GetModuleMutex());
 
+  TypeQuery query_full(query);
+  llvm::SmallVector<llvm::StringRef> parent_names =
+      GetTypeQueryParentNames(query_full);
   bool have_index_match = false;
-  m_index->GetTypes(type_basename, [&](DWARFDIE die) {
-    // Check the language, but only if we have a language filter.
-    if (query.HasLanguage()) {
-      if (!query.LanguageMatches(GetLanguageFamily(*die.GetCU())))
-        return true; // Keep iterating over index types, language mismatch.
-    }
+  m_index->GetTypesWithParents(
+      query.GetTypeBasename(), parent_names, [&](DWARFDIE die) {
+        // Check the language, but only if we have a language filter.
+        if (query.HasLanguage()) {
+          if (!query.LanguageMatches(GetLanguageFamily(*die.GetCU())))
+            return true; // Keep iterating over index types, language mismatch.
+        }
 
-    // Check the context matches
-    std::vector<lldb_private::CompilerContext> die_context;
-    if (query.GetModuleSearch())
-      die_context = die.GetDeclContext();
-    else
-      die_context = die.GetTypeLookupContext();
-    assert(!die_context.empty());
-    if (!query.ContextMatches(die_context))
-      return true; // Keep iterating over index types, context mismatch.
-
-    // Try to resolve the type.
-    if (Type *matching_type = ResolveType(die, true, true)) {
-      if (matching_type->IsTemplateType()) {
-        // We have to watch out for case where we lookup a type by basename and
-        // it matches a template with simple template names. Like looking up
-        // "Foo" and if we have simple template names then we will match
-        // "Foo<int>" and "Foo<double>" because all the DWARF has is "Foo" in
-        // the accelerator tables. The main case we see this in is when the
-        // expression parser is trying to parse "Foo<int>" and it will first do
-        // a lookup on just "Foo". We verify the type basename matches before
-        // inserting the type in the results.
-        auto CompilerTypeBasename =
-            matching_type->GetForwardCompilerType().GetTypeName(true);
-        if (CompilerTypeBasename != query.GetTypeBasename())
-          return true; // Keep iterating over index types, basename mismatch.
-      }
-      have_index_match = true;
-      results.InsertUnique(matching_type->shared_from_this());
-    }
-    return !results.Done(query); // Keep iterating if we aren't done.
-  });
+        // Check the context matches
+        std::vector<lldb_private::CompilerContext> die_context;
+        if (query.GetModuleSearch())
+          die_context = die.GetDeclContext();
+        else
+          die_context = die.GetTypeLookupContext();
+        assert(!die_context.empty());
+        if (!query.ContextMatches(die_context))
+          return true; // Keep iterating over index types, context mismatch.
+
+        // Try to resolve the type.
+        if (Type *matching_type = ResolveType(die, true, true)) {
+          if (matching_type->IsTemplateType()) {
+            // We have to watch out for case where we lookup a type by basename
+            // and it matches a template with simple template names. Like
+            // looking up "Foo" and if we have simple template names then we
+            // will match "Foo<int>" and "Foo<double>" because all the DWARF has
+            // is "Foo" in the accelerator tables. The main case we see this in
+            // is when the expression parser is trying to parse "Foo<int>" and
+            // it will first do a lookup on just "Foo". We verify the type
+            // basename matches before inserting the type in the results.
+            auto CompilerTypeBasename =
+                matching_type->GetForwardCompilerType().GetTypeName(true);
+            if (CompilerTypeBasename != query.GetTypeBasename())
+              return true; // Keep iterating over index types, basename
+                           // mismatch.
+          }
+          have_index_match = true;
+          results.InsertUnique(matching_type->shared_from_this());
+        }
+        return !results.Done(query); // Keep iterating if we aren't done.
+      });
 
   if (results.Done(query)) {
     if (log) {
@@ -2811,44 +2832,50 @@ void SymbolFileDWARF::FindTypes(const TypeQuery &query, TypeResults &results) {
     TypeQuery query_simple(query);
     if (UpdateCompilerContextForSimpleTemplateNames(query_simple)) {
       auto type_basename_simple = query_simple.GetTypeBasename();
+      llvm::SmallVector<llvm::StringRef> parent_names =
+          GetTypeQueryParentNames(query_simple);
       // Copy our match's context and update the basename we are looking for
       // so we can use this only to compare the context correctly.
-      m_index->GetTypes(type_basename_simple, [&](DWARFDIE die) {
-        // Check the language, but only if we have a language filter.
-        if (query.HasLanguage()) {
-          if (!query.LanguageMatches(GetLanguageFamily(*die.GetCU())))
-            return true; // Keep iterating over index types, language mismatch.
-        }
-
-        // Check the context matches
-        std::vector<lldb_private::CompilerContext> die_context;
-        if (query.GetModuleSearch())
-          die_context = die.GetDeclContext();
-        else
-          die_context = die.GetTypeLookupContext();
-        assert(!die_context.empty());
-        if (!query_simple.ContextMatches(die_context))
-          return true; // Keep iterating over index types, context mismatch.
-
-        // Try to resolve the type.
-        if (Type *matching_type = ResolveType(die, true, true)) {
-          ConstString name = matching_type->GetQualifiedName();
-          // We have found a type that still might not match due to template
-          // parameters. If we create a new TypeQuery that uses the new type's
-          // fully qualified name, we can find out if this type matches at all
-          // context levels. We can't use just the "match_simple" context
-          // because all template parameters were stripped off. The fully
-          // qualified name of the type will have the template parameters and
-          // will allow us to make sure it matches correctly.
-          TypeQuery die_query(name.GetStringRef(),
-                              TypeQueryOptions::e_exact_match);
-          if (!query.ContextMatches(die_query.GetContextRef()))
-            return true; // Keep iterating over index types, context mismatch.
+      m_index->GetTypesWithParents(
+          query_simple.GetTypeBasename(), parent_names, [&](DWARFDIE die) {
+            // Check the language, but only if we have a language filter.
+            if (query.HasLanguage()) {
+              if (!query.LanguageMatches(GetLanguageFamily(*die.GetCU())))
+                return true; // Keep iterating over index types, language
+                             // mismatch.
+            }
 
-          results.InsertUnique(matching_type->shared_from_this());
-        }
-        return !results.Done(query); // Keep iterating if we aren't done.
-      });
+            // Check the context matches
+            std::vector<lldb_private::CompilerContext> die_context;
+            if (query.GetModuleSearch())
+              die_context = die.GetDeclContext();
+            else
+              die_context = die.GetTypeLookupContext();
+            assert(!die_context.empty());
+            if (!query_simple.ContextMatches(die_context))
+              return true; // Keep iterating over index types, context mismatch.
+
+            // Try to resolve the type.
+            if (Type *matching_type = ResolveType(die, true, true)) {
+              ConstString name = matching_type->GetQualifiedName();
+              // We have found a type that still might not match due to template
+              // parameters. If we create a new TypeQuery that uses the new
+              // type's fully qualified name, we can find out if this type
+              // matches at all context levels. We can't use just the
+              // "match_simple" context because all template parameters were
+              // stripped off. The fully qualified name of the type will have
+              // the template parameters and will allow us to make sure it
+              // matches correctly.
+              TypeQuery die_query(name.GetStringRef(),
+                                  TypeQueryOptions::e_exact_match);
+              if (!query.ContextMatches(die_query.GetContextRef()))
+                return true; // Keep iterating over index types, context
+                             // mismatch.
+
+              results.InsertUnique(matching_type->shared_from_this());
+            }
+            return !results.Done(query); // Keep iterating if we aren't done.
+          });
       if (results.Done(query)) {
         if (log) {
           GetObjectFile()->GetModule()->LogMessage(
@@ -2898,7 +2925,24 @@ SymbolFileDWARF::FindNamespace(ConstString name,
   if (!DeclContextMatchesThisSymbolFile(parent_decl_ctx))
     return namespace_decl_ctx;
 
-  m_index->GetNamespaces(name, [&](DWARFDIE die) {
+  llvm::SmallVector<llvm::StringRef> parent_names;
+  auto parent_ctx = parent_decl_ctx.GetCompilerContext();
+  auto rbegin = parent_ctx.rbegin(), rend = parent_ctx.rend();
+  for (auto rit = rbegin;rit != rend; ++rit) {
+    if (!rit->name.IsEmpty())
+      parent_names.push_back(rit->name);
+  }
+
+  std::string parent_name_strings;
+  for (auto &parent_name : parent_names) {
+    parent_name_strings += "::";
+    parent_name_strings += parent_name;
+  }
+  Log *log1 = GetLog(LLDBLog::Temporary);
+  LLDB_LOGF(log1, "GetNamespacesWithParents() for %s with parent_names %s -- start",
+        name.GetCString(), parent_name_strings.c_str());
+
+  m_index->GetNamespacesWithParents(name, parent_names, [&](DWARFDIE die) {
     if (!DIEInDeclContext(parent_decl_ctx, die, only_root_namespaces))
       return true; // The containing decl contexts don't match
 
@@ -2909,6 +2953,8 @@ SymbolFileDWARF::FindNamespace(ConstString name,
     namespace_decl_ctx = dwarf_ast->GetDeclContextForUIDFromDWARF(die);
     return !namespace_decl_ctx.IsValid();
   });
+  LLDB_LOGF(log1, "GetNamespacesWithParents() for %s with parent_names %s -- end",
+        name.GetCString(), parent_name_strings.c_str());
 
   if (log && namespace_decl_ctx) {
     GetObjectFile()->GetModule()->LogMessage(
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
index 4967b37d753a09..c5cd7fd1bf61ec 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
@@ -506,6 +506,8 @@ class SymbolFileDWARF : public SymbolFileCommon {
   void
   GetCompileOptions(std::unordered_map<lldb::CompUnitSP, Args> &args) override;
 
+  llvm::SmallVector<llvm::StringRef> GetTypeQueryParentNames(TypeQuery query);
+
   lldb::ModuleWP m_debug_map_module_wp;
   SymbolFileDWARFDebugMap *m_debug_map_symfile;
 

>From 49f869286ebe7791f47cc4fef8d8195aef32b76d Mon Sep 17 00:00:00 2001
From: jeffreytan81 <jeffreytan at fb.com>
Date: Mon, 16 Sep 2024 18:03:01 -0700
Subject: [PATCH 3/4] Remove unwanted logging

---
 .../Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp   | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index 833b1ae87dc873..cc2497b7e484ba 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -2928,20 +2928,10 @@ SymbolFileDWARF::FindNamespace(ConstString name,
   llvm::SmallVector<llvm::StringRef> parent_names;
   auto parent_ctx = parent_decl_ctx.GetCompilerContext();
   auto rbegin = parent_ctx.rbegin(), rend = parent_ctx.rend();
-  for (auto rit = rbegin;rit != rend; ++rit) {
+  for (auto rit = rbegin; rit != rend; ++rit) {
     if (!rit->name.IsEmpty())
       parent_names.push_back(rit->name);
   }
-
-  std::string parent_name_strings;
-  for (auto &parent_name : parent_names) {
-    parent_name_strings += "::";
-    parent_name_strings += parent_name;
-  }
-  Log *log1 = GetLog(LLDBLog::Temporary);
-  LLDB_LOGF(log1, "GetNamespacesWithParents() for %s with parent_names %s -- start",
-        name.GetCString(), parent_name_strings.c_str());
-
   m_index->GetNamespacesWithParents(name, parent_names, [&](DWARFDIE die) {
     if (!DIEInDeclContext(parent_decl_ctx, die, only_root_namespaces))
       return true; // The containing decl contexts don't match
@@ -2953,8 +2943,6 @@ SymbolFileDWARF::FindNamespace(ConstString name,
     namespace_decl_ctx = dwarf_ast->GetDeclContextForUIDFromDWARF(die);
     return !namespace_decl_ctx.IsValid();
   });
-  LLDB_LOGF(log1, "GetNamespacesWithParents() for %s with parent_names %s -- end",
-        name.GetCString(), parent_name_strings.c_str());
 
   if (log && namespace_decl_ctx) {
     GetObjectFile()->GetModule()->LogMessage(

>From 6ce666cdd26dd13c568d4e4208c5a24e23ed8316 Mon Sep 17 00:00:00 2001
From: jeffreytan81 <jeffreytan at fb.com>
Date: Mon, 16 Sep 2024 18:08:25 -0700
Subject: [PATCH 4/4] Remove more unwanted logging

---
 .../SymbolFile/DWARF/DebugNamesDWARFIndex.cpp | 25 ++++++++-----------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
index c84fc0b5c60717..c97c9248a4478b 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
@@ -506,12 +506,14 @@ void DebugNamesDWARFIndex::GetNamespacesWithParents(
       if (!parent_chain) {
         // Fallback: use the base class implementation.
         if (!ProcessEntry(entry, [&](DWARFDIE die) {
-              return ProcessDieMatchParentNames(name, parent_names, die, callback);
+              return ProcessDieMatchParentNames(name, parent_names, die,
+                                                callback);
             }))
           return;
         continue;
       }
 
+      // If parent_chain is shorter than parent_names, we can't possibly match.
       if (parent_chain->size() < parent_names.size())
         continue;
       else if (parent_chain->size() == parent_names.size()) {
@@ -519,7 +521,8 @@ void DebugNamesDWARFIndex::GetNamespacesWithParents(
             !ProcessEntry(entry, callback))
           return;
       } else {
-        // parent_chain->size() > parent_names.size()
+        // When parent_chain has more entries than parent_names we still
+        // possibly match if parent_chain contains inline namespaces.
         if (WithinParentChain(parent_names, *parent_chain) &&
             !ProcessEntry(entry, callback))
           return;
@@ -535,10 +538,7 @@ void DebugNamesDWARFIndex::GetTypesWithParents(
     llvm::function_ref<bool(DWARFDIE die)> callback) {
   if (parent_names.empty())
     return GetTypes(name, callback);
-    
-  Log *log = GetLog(LLDBLog::Temporary);
-  int count = 0;
-  Progress progress("GetTypesWithParents from index for %s", name.GetCString());
+
   // For each entry, grab its parent chain and check if we have a match.
   for (const DebugNames::Entry &entry : m_debug_names_up->equal_range(name)) {
     if (!isType(entry.tag()))
@@ -551,20 +551,19 @@ void DebugNamesDWARFIndex::GetTypesWithParents(
     if (foreign_tu && foreign_tu.value() == nullptr)
       continue;
 
-    // Grab at most one extra parent, subsequent parents are not necessary to
-    // test equality.
     std::optional<llvm::SmallVector<Entry, 4>> parent_chain =
         getParentChain(entry);
-    ++count;
     if (!parent_chain) {
       // Fallback: use the base class implementation.
       if (!ProcessEntry(entry, [&](DWARFDIE die) {
-            return ProcessDieMatchParentNames(name, parent_names, die, callback);
+            return ProcessDieMatchParentNames(name, parent_names, die,
+                                              callback);
           }))
         return;
       continue;
     }
 
+    // If parent_chain is shorter than parent_names, we can't possibly match.
     if (parent_chain->size() < parent_names.size())
       continue;
     else if (parent_chain->size() == parent_names.size()) {
@@ -572,15 +571,13 @@ void DebugNamesDWARFIndex::GetTypesWithParents(
           !ProcessEntry(entry, callback))
         return;
     } else {
-      // parent_chain->size() > parent_names.size()
+      // When parent_chain has more entries than parent_names we still possibly
+      // match if parent_chain contains inline namespaces.
       if (WithinParentChain(parent_names, *parent_chain) &&
           !ProcessEntry(entry, callback))
         return;
     }
   }
-  LLDB_LOG(log,
-           "GetTypesWithParents searched {0} entries for {1} but found none",
-           count, name.GetCString());
   m_fallback.GetTypesWithParents(name, parent_names, callback);
 }
 



More information about the lldb-commits mailing list