[lldb] Fix lookup of types in anonymous namespaces with -gsimple-template-names #123054
Conversation
…late-names Anonymous namespaces are supposed to be optional when looking up types. This was not working in combination with -gsimple-template-names, because the way it was constructing the complete (with template args) name scope (i.e., by generating thescope as a string and then reparsing it) did not preserve the information about the scope kinds. Essentially what the code wants here is to call `GetTypeLookupContext` (that's the function used to get the context in the "regular" code path), but to embelish each name with the template arguments (if they don't have them already). This PR implements exactly that by adding an argument to control which kind of names are we interested in. This should also make the lookup faster as it avoids parsing of the long string, but I haven't attempted to benchmark that. I believe this function can also be used in some other places where we're manually appending template names, but I'm leaving that for another patch.
| // 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. | ||
| } |
There was a problem hiding this comment.
This is already checked in the caller.
|
@llvm/pr-subscribers-lldb Author: Pavel Labath (labath) ChangesAnonymous namespaces are supposed to be optional when looking up types. This was not working in combination with -gsimple-template-names, because the way it was constructing the complete (with template args) name scope (i.e., by generating thescope as a string and then reparsing it) did not preserve the information about the scope kinds. Essentially what the code wants here is to call I believe this function can also be used in some other places where we're manually appending template names, but I'm leaving that for another patch. Full diff: https://github.com/llvm/llvm-project/pull/123054.diff 5 Files Affected:
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
index 6857878b354a05..f2b02d3d6898d0 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
@@ -19,6 +19,8 @@
#include "llvm/ADT/iterator.h"
#include "llvm/BinaryFormat/Dwarf.h"
#include "llvm/DebugInfo/DWARF/DWARFAddressRange.h"
+#include "llvm/DebugInfo/DWARF/DWARFTypePrinter.h"
+#include "llvm/Support/raw_ostream.h"
using namespace lldb_private;
using namespace lldb_private::dwarf;
@@ -376,7 +378,8 @@ lldb_private::Type *DWARFDIE::ResolveTypeUID(const DWARFDIE &die) const {
return nullptr;
}
-static CompilerContext GetContextEntry(DWARFDIE die) {
+static CompilerContext GetContextEntry(DWARFDIE die,
+ bool complete_template_names) {
auto ctx = [die](CompilerContextKind kind) {
return CompilerContext(kind, ConstString(die.GetName()));
};
@@ -386,11 +389,6 @@ static CompilerContext GetContextEntry(DWARFDIE die) {
return ctx(CompilerContextKind::Module);
case DW_TAG_namespace:
return ctx(CompilerContextKind::Namespace);
- case DW_TAG_class_type:
- case DW_TAG_structure_type:
- return ctx(CompilerContextKind::ClassOrStruct);
- case DW_TAG_union_type:
- return ctx(CompilerContextKind::Union);
case DW_TAG_enumeration_type:
return ctx(CompilerContextKind::Enum);
case DW_TAG_subprogram:
@@ -401,12 +399,28 @@ static CompilerContext GetContextEntry(DWARFDIE die) {
return ctx(CompilerContextKind::Typedef);
case DW_TAG_base_type:
return ctx(CompilerContextKind::Builtin);
+ case DW_TAG_class_type:
+ case DW_TAG_structure_type:
+ case DW_TAG_union_type: {
+ CompilerContextKind kind = die.Tag() == DW_TAG_union_type
+ ? CompilerContextKind::Union
+ : CompilerContextKind::ClassOrStruct;
+ llvm::StringRef name = die.GetName();
+ if (!complete_template_names || name.contains('<'))
+ return CompilerContext(kind, ConstString(name));
+
+ std::string name_storage = name.str();
+ llvm::raw_string_ostream os(name_storage);
+ llvm::DWARFTypePrinter<DWARFDIE>(os).appendAndTerminateTemplateParameters(
+ die);
+ return CompilerContext(kind, ConstString(os.str()));
+ }
default:
llvm_unreachable("Check tag type in the caller!");
}
}
-static void GetDeclContextImpl(DWARFDIE die,
+static void GetDeclContextImpl(DWARFDIE die, bool complete_template_names,
llvm::SmallSet<lldb::user_id_t, 4> &seen,
std::vector<CompilerContext> &context) {
// Stop if we hit a cycle.
@@ -428,7 +442,7 @@ static void GetDeclContextImpl(DWARFDIE die,
case DW_TAG_subprogram:
case DW_TAG_variable:
case DW_TAG_typedef:
- context.push_back(GetContextEntry(die));
+ context.push_back(GetContextEntry(die, complete_template_names));
break;
default:
break;
@@ -438,15 +452,16 @@ static void GetDeclContextImpl(DWARFDIE die,
}
}
-std::vector<CompilerContext> DWARFDIE::GetDeclContext() const {
+std::vector<CompilerContext>
+DWARFDIE::GetDeclContext(bool complete_template_names) const {
llvm::SmallSet<lldb::user_id_t, 4> seen;
std::vector<CompilerContext> context;
- GetDeclContextImpl(*this, seen, context);
+ GetDeclContextImpl(*this, complete_template_names, seen, context);
std::reverse(context.begin(), context.end());
return context;
}
-static void GetTypeLookupContextImpl(DWARFDIE die,
+static void GetTypeLookupContextImpl(DWARFDIE die, bool complete_template_names,
llvm::SmallSet<lldb::user_id_t, 4> &seen,
std::vector<CompilerContext> &context) {
// Stop if we hit a cycle.
@@ -461,7 +476,7 @@ static void GetTypeLookupContextImpl(DWARFDIE die,
case DW_TAG_variable:
case DW_TAG_typedef:
case DW_TAG_base_type:
- context.push_back(GetContextEntry(die));
+ context.push_back(GetContextEntry(die, complete_template_names));
break;
// If any of the tags below appear in the parent chain, stop the decl
@@ -484,10 +499,11 @@ static void GetTypeLookupContextImpl(DWARFDIE die,
}
}
-std::vector<CompilerContext> DWARFDIE::GetTypeLookupContext() const {
+std::vector<CompilerContext>
+DWARFDIE::GetTypeLookupContext(bool complete_template_names) const {
llvm::SmallSet<lldb::user_id_t, 4> seen;
std::vector<CompilerContext> context;
- GetTypeLookupContextImpl(*this, seen, context);
+ GetTypeLookupContextImpl(*this, complete_template_names, seen, context);
std::reverse(context.begin(), context.end());
return context;
}
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.h b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.h
index c3239b5b121f94..b2df9a3c43a755 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.h
@@ -73,7 +73,15 @@ class DWARFDIE : public DWARFBaseDIE {
/// Return this DIE's decl context as it is needed to look up types
/// in Clang modules. This context will include any modules or functions that
/// the type is declared in so an exact module match can be efficiently made.
- std::vector<CompilerContext> GetDeclContext() const;
+ ///
+ /// \param[in] complete_template_names
+ /// If true, augments the returned names with template arguments derived
+ /// from the child DIEs, if the names don't contained template arguments
+ /// already. If false, the returned context will contain the names exactly
+ /// as they are spelled in the debug info, regardless of whether that
+ /// includes template arguments or not.
+ std::vector<CompilerContext>
+ GetDeclContext(bool complete_template_names = false) const;
/// Get a context to a type so it can be looked up.
///
@@ -85,7 +93,15 @@ class DWARFDIE : public DWARFBaseDIE {
/// appropriate time, like either the translation unit or at a function
/// context. This is designed to allow users to efficiently look for types
/// using a full or partial CompilerContext array.
- std::vector<CompilerContext> GetTypeLookupContext() const;
+ ///
+ /// \param[in] complete_template_names
+ /// If true, augments the returned names with template arguments derived
+ /// from the child DIEs, if the names don't contained template arguments
+ /// already. If false, the returned context will contain the names exactly
+ /// as they are spelled in the debug info, regardless of whether that
+ /// includes template arguments or not.
+ std::vector<CompilerContext>
+ GetTypeLookupContext(bool complete_template_names = false) const;
DWARFDeclContext GetDWARFDeclContext() const;
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index 360dbaa1beb5eb..0821aa0050fc63 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -10,10 +10,8 @@
#include "llvm/ADT/STLExtras.h"
#include "llvm/DebugInfo/DWARF/DWARFAddressRange.h"
#include "llvm/DebugInfo/DWARF/DWARFDebugLoc.h"
-#include "llvm/DebugInfo/DWARF/DWARFTypePrinter.h"
#include "llvm/Support/Casting.h"
#include "llvm/Support/FileUtilities.h"
-#include "llvm/Support/Format.h"
#include "llvm/Support/FormatAdapters.h"
#include "llvm/Support/Threading.h"
@@ -2740,18 +2738,11 @@ void SymbolFileDWARF::FindTypes(const TypeQuery &query, TypeResults &results) {
// 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->GetTypesWithQuery(query_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.
- }
-
- std::string qualified_name;
- llvm::raw_string_ostream os(qualified_name);
- llvm::DWARFTypePrinter<DWARFDIE> type_printer(os);
- type_printer.appendQualifiedName(die);
- TypeQuery die_query(qualified_name, e_exact_match);
- if (query.ContextMatches(die_query.GetContextRef()))
+ std::vector<CompilerContext> qualified_context =
+ query.GetModuleSearch()
+ ? die.GetDeclContext(/*complete_template_names=*/true)
+ : die.GetTypeLookupContext(/*complete_template_names=*/true);
+ if (query.ContextMatches(qualified_context))
if (Type *matching_type = ResolveType(die, true, true))
results.InsertUnique(matching_type->shared_from_this());
return !results.Done(query); // Keep iterating if we aren't done.
diff --git a/lldb/test/API/lang/cpp/nested-template/TestNestedTemplate.py b/lldb/test/API/lang/cpp/nested-template/TestNestedTemplate.py
index 42db060529a818..7a73619555fa08 100644
--- a/lldb/test/API/lang/cpp/nested-template/TestNestedTemplate.py
+++ b/lldb/test/API/lang/cpp/nested-template/TestNestedTemplate.py
@@ -17,6 +17,11 @@ def do_test(self, debug_flags):
DATA_TYPES_DISPLAYED_CORRECTLY,
substrs=["1 match found"],
)
+ self.expect(
+ "image lookup -A -t 'NS::Struct<int>'",
+ DATA_TYPES_DISPLAYED_CORRECTLY,
+ substrs=["1 match found"],
+ )
@skipIf(compiler=no_match("clang"))
@skipIf(compiler_version=["<", "15.0"])
diff --git a/lldb/test/API/lang/cpp/nested-template/main.cpp b/lldb/test/API/lang/cpp/nested-template/main.cpp
index 06d1094880964c..eef7c021e22eaf 100644
--- a/lldb/test/API/lang/cpp/nested-template/main.cpp
+++ b/lldb/test/API/lang/cpp/nested-template/main.cpp
@@ -5,6 +5,13 @@ struct Outer {
struct Inner {};
};
+namespace NS {
+namespace {
+template <typename T> struct Struct {};
+} // namespace
+} // namespace NS
+
int main() {
Outer::Inner<int> oi;
+ NS::Struct<int> ns_struct;
}
|
| ? CompilerContextKind::Union | ||
| : CompilerContextKind::ClassOrStruct; | ||
| llvm::StringRef name = die.GetName(); | ||
| if (!complete_template_names || name.contains('<')) |
There was a problem hiding this comment.
Just to clarify, in what cases do we have simple-template-names and contain template parameters? Is it because -gsimple-template-names can't always canonicalize some template names?
There was a problem hiding this comment.
That's part of it -- some names (I'm not sure which ones, but they're probably involve non-type template arguments and things like that) don't have enough information to reconstruct the original name. The other part is that we really don't know whether debug info contains simplified template names or not (even for names that could be simplified). There's no CU-level attribute or anything that would convey that information.
There was a problem hiding this comment.
Ah right makes sense
There's no CU-level attribute or anything that would convey that information
I think I asked about this a while back but would it be worth introducing such CU-level DWARF attribute?
There was a problem hiding this comment.
You were probably asking @dwblaikie as I wasn't involved in the original implementation. Anyway, here's what I think.
If we were able to simplify all template names, then I can see how such an attribute could be useful as a replacement for the contains('<') heuristic, though I believe that is just a performance optimization (avoid searching through children DIEs if we see the name is templated) and shouldn't produce false positives. It could also be used for things like "if all CUs are using the same kind of names then don't bother trying to search for the other name kind", though I'm unsure what difference would it make in practice (especially the search with the full name should be very fast as it will just get zero hits).
As it stands now, I think it's usefulness would be very limited. About the only thing I can think of is skipping the query with the simplified name if all CUs report they are using full names. That may make sense if you're seeing slowdown due to the additional simplified-name queries.
Anonymous namespaces are supposed to be optional when looking up types. This was not working in combination with -gsimple-template-names, because the way it was constructing the complete (with template args) name scope (i.e., by generating thescope as a string and then reparsing it) did not preserve the information about the scope kinds.
Essentially what the code wants here is to call
GetTypeLookupContext(that's the function used to get the context in the "regular" code path), but to embelish each name with the template arguments (if they don't have them already). This PR implements exactly that by adding an argument to control which kind of names are we interested in. This should also make the lookup faster as it avoids parsing of the long string, but I haven't attempted to benchmark that.I believe this function can also be used in some other places where we're manually appending template names, but I'm leaving that for another patch.