[lldb/DWARF] Fix type definition search with simple template names #95905
Conversation
With simple template names the template arguments aren't embedded in the DW_AT_name attribute of the type. The code in FindDefinitionTypeForDWARFDeclContext was comparing the synthesized template arguments on the leaf (most deeply nested) DIE, but was not sufficient, as the difference get be at any level above that (Foo<T>::Bar vs. Foo<U>::Bar). This patch makes sure we compare the entire context. As a drive-by I also remove the completely unnecessary ConstStringification of the GetDIEClassTemplateParams result.
|
@llvm/pr-subscribers-lldb Author: Pavel Labath (labath) ChangesWith simple template names the template arguments aren't embedded in the DW_AT_name attribute of the type. The code in As a drive-by I also remove the completely unnecessary ConstStringification of the GetDIEClassTemplateParams result. Full diff: https://github.com/llvm/llvm-project/pull/95905.diff 5 Files Affected:
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.h b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.h
index 66db396279e06..abaeb2502cbbd 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.h
@@ -58,7 +58,7 @@ class DWARFASTParser {
virtual void EnsureAllDIEsInDeclContextHaveBeenParsed(
CompilerDeclContext decl_context) = 0;
- virtual ConstString GetDIEClassTemplateParams(const DWARFDIE &die) = 0;
+ virtual std::string GetDIEClassTemplateParams(const DWARFDIE &die) = 0;
static std::optional<SymbolFile::ArrayInfo>
ParseChildArrayInfo(const DWARFDIE &parent_die,
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index 02c91ddc57f83..cffe3f7dd6128 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -838,16 +838,16 @@ DWARFASTParserClang::ParseTypeModifier(const SymbolContext &sc,
return type_sp;
}
-ConstString
+std::string
DWARFASTParserClang::GetDIEClassTemplateParams(const DWARFDIE &die) {
if (llvm::StringRef(die.GetName()).contains("<"))
- return ConstString();
+ return "";
TypeSystemClang::TemplateParameterInfos template_param_infos;
- if (ParseTemplateParameterInfos(die, template_param_infos)) {
- return ConstString(m_ast.PrintTemplateParams(template_param_infos));
- }
- return ConstString();
+ if (ParseTemplateParameterInfos(die, template_param_infos))
+ return m_ast.PrintTemplateParams(template_param_infos);
+
+ return "";
}
TypeSP DWARFASTParserClang::ParseEnum(const SymbolContext &sc,
@@ -1632,7 +1632,7 @@ DWARFASTParserClang::GetCPlusPlusQualifiedName(const DWARFDIE &die) {
case DW_TAG_union_type: {
if (const char *class_union_struct_name = parent_decl_ctx_die.GetName()) {
qualified_name.insert(
- 0, GetDIEClassTemplateParams(parent_decl_ctx_die).AsCString(""));
+ 0, GetDIEClassTemplateParams(parent_decl_ctx_die));
qualified_name.insert(0, "::");
qualified_name.insert(0, class_union_struct_name);
}
@@ -1650,7 +1650,7 @@ DWARFASTParserClang::GetCPlusPlusQualifiedName(const DWARFDIE &die) {
qualified_name.append("::");
qualified_name.append(name);
- qualified_name.append(GetDIEClassTemplateParams(die).AsCString(""));
+ qualified_name.append(GetDIEClassTemplateParams(die));
return qualified_name;
}
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
index 136a49e462fbb..7b5ddbaa2a6b5 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
@@ -104,9 +104,9 @@ class DWARFASTParserClang : public lldb_private::plugin::dwarf::DWARFASTParser {
///
/// \param die The struct/class DWARFDIE containing template parameters.
/// \return A string, including surrounding '<>', of the template parameters.
- /// If the DIE's name already has '<>', returns an empty ConstString because
+ /// If the DIE's name already has '<>', returns an empty string because
/// it's assumed that the caller is using the DIE name anyway.
- lldb_private::ConstString GetDIEClassTemplateParams(
+ std::string GetDIEClassTemplateParams(
const lldb_private::plugin::dwarf::DWARFDIE &die) override;
protected:
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index 369c742a5ee02..1dd19dbaac137 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -3073,14 +3073,43 @@ SymbolFileDWARF::FindDefinitionTypeForDWARFDeclContext(const DWARFDIE &die) {
// See comments below about -gsimple-template-names for why we attempt to
// compute missing template parameter names.
- ConstString template_params;
- if (type_system) {
- DWARFASTParser *dwarf_ast = type_system->GetDWARFParser();
- if (dwarf_ast)
- template_params = dwarf_ast->GetDIEClassTemplateParams(die);
+ std::vector<std::string> template_params;
+ DWARFDeclContext die_dwarf_decl_ctx;
+ DWARFASTParser *dwarf_ast = type_system ? type_system->GetDWARFParser() : nullptr;
+ for (DWARFDIE ctx_die = die; ctx_die && !isUnitType(ctx_die.Tag());
+ ctx_die = ctx_die.GetParentDeclContextDIE()) {
+ die_dwarf_decl_ctx.AppendDeclContext(ctx_die.Tag(), ctx_die.GetName());
+ template_params.push_back(
+ (ctx_die.IsStructUnionOrClass() && dwarf_ast)
+ ? dwarf_ast->GetDIEClassTemplateParams(ctx_die)
+ : "");
}
+ const bool any_template_params = llvm::any_of(
+ template_params, [](llvm::StringRef p) { return !p.empty(); });
- const DWARFDeclContext die_dwarf_decl_ctx = die.GetDWARFDeclContext();
+ auto die_matches = [&](DWARFDIE type_die) {
+ // Resolve the type if both have the same tag or {class, struct} tags.
+ const bool tag_matches =
+ type_die.Tag() == tag ||
+ (IsStructOrClassTag(type_die.Tag()) && IsStructOrClassTag(tag));
+ if (!tag_matches)
+ return false;
+ if (any_template_params) {
+ size_t pos = 0;
+ for (DWARFDIE ctx_die = type_die;
+ type_die && !isUnitType(ctx_die.Tag()) &&
+ pos < template_params.size();
+ ctx_die = ctx_die.GetParentDeclContextDIE(), ++pos) {
+ if (template_params[pos].empty())
+ continue;
+ if (template_params[pos] != dwarf_ast->GetDIEClassTemplateParams(ctx_die))
+ return false;
+ }
+ if (pos != template_params.size())
+ return false;
+ }
+ return true;
+ };
m_index->GetFullyQualifiedType(die_dwarf_decl_ctx, [&](DWARFDIE type_die) {
// Make sure type_die's language matches the type system we are
// looking for. We don't want to find a "Foo" type from Java if we
@@ -3089,13 +3118,7 @@ SymbolFileDWARF::FindDefinitionTypeForDWARFDeclContext(const DWARFDIE &die) {
!type_system->SupportsLanguage(GetLanguage(*type_die.GetCU())))
return true;
- const dw_tag_t type_tag = type_die.Tag();
- // Resolve the type if both have the same tag or {class, struct} tags.
- const bool try_resolving_type =
- type_tag == tag ||
- (IsStructOrClassTag(type_tag) && IsStructOrClassTag(tag));
-
- if (!try_resolving_type) {
+ if (!die_matches(type_die)) {
if (log) {
GetObjectFile()->GetModule()->LogMessage(
log,
@@ -3123,27 +3146,6 @@ SymbolFileDWARF::FindDefinitionTypeForDWARFDeclContext(const DWARFDIE &die) {
if (!resolved_type || resolved_type == DIE_IS_BEING_PARSED)
return true;
- // With -gsimple-template-names, the DIE name may not contain the template
- // parameters. If the declaration has template parameters but doesn't
- // contain '<', check that the child template parameters match.
- if (template_params) {
- llvm::StringRef test_base_name =
- GetTypeForDIE(type_die)->GetBaseName().GetStringRef();
- auto i = test_base_name.find('<');
-
- // Full name from clang AST doesn't contain '<' so this type_die isn't
- // a template parameter, but we're expecting template parameters, so
- // bail.
- if (i == llvm::StringRef::npos)
- return true;
-
- llvm::StringRef test_template_params =
- test_base_name.slice(i, test_base_name.size());
- // Bail if template parameters don't match.
- if (test_template_params != template_params.GetStringRef())
- return true;
- }
-
type_sp = resolved_type->shared_from_this();
return false;
});
diff --git a/lldb/test/Shell/SymbolFile/DWARF/x86/simple-template-names-context.cpp b/lldb/test/Shell/SymbolFile/DWARF/x86/simple-template-names-context.cpp
index a8a4d3b8fbd5f..8070b7a19abcc 100644
--- a/lldb/test/Shell/SymbolFile/DWARF/x86/simple-template-names-context.cpp
+++ b/lldb/test/Shell/SymbolFile/DWARF/x86/simple-template-names-context.cpp
@@ -12,7 +12,7 @@
// CHECK: (lldb) target variable
// CHECK-NEXT: (ReferencesBoth<'A'>) both_a = {
// CHECK-NEXT: (Outer<'A'>::Inner *) a = 0x{{[0-9A-Fa-f]*}} {}
-// CHECK-NEXT: (Outer<'A'>::Inner *) b = 0x{{[0-9A-Fa-f]*}} {}
+// CHECK-NEXT: (Outer<'B'>::Inner *) b = 0x{{[0-9A-Fa-f]*}} {}
// CHECK-NEXT: }
// CHECK-NEXT: (ReferencesBoth<'B'>) both_b = {
// CHECK-NEXT: (Outer<'A'>::Inner *) a = 0x{{[0-9A-Fa-f]*}} {}
|
| @@ -12,7 +12,7 @@ | |||
| // CHECK: (lldb) target variable | |||
| // CHECK-NEXT: (ReferencesBoth<'A'>) both_a = { | |||
| // CHECK-NEXT: (Outer<'A'>::Inner *) a = 0x{{[0-9A-Fa-f]*}} {} | |||
| // CHECK-NEXT: (Outer<'A'>::Inner *) b = 0x{{[0-9A-Fa-f]*}} {} | |||
| // CHECK-NEXT: (Outer<'B'>::Inner *) b = 0x{{[0-9A-Fa-f]*}} {} | |||
| // Resolve the type if both have the same tag or {class, struct} tags. | ||
| const bool tag_matches = | ||
| type_die.Tag() == tag || | ||
| (IsStructOrClassTag(type_die.Tag()) && IsStructOrClassTag(tag)); |
There was a problem hiding this comment.
At some point we should probably extract this into a standalone helper. We're accumulating a lot of these checks
There was a problem hiding this comment.
Yeah.. Doesn't exactly help here, but I've been wondering about this in the context of the CompilerContext class. If we don't care about the class vs. struct distinction when resolving types, maybe we could remove the distinction at least from there?
There was a problem hiding this comment.
maybe we could remove the distinction at least from there?
Mind pointing me to the place you're referring to?
There was a problem hiding this comment.
We have both CompilerContextKind::Class and CompilerContextKind::Struct. Since these classes are (mainly?) used for looking things up in the debug info (where we treat the two as (mostly) the same), I'm thinking whether we could merge these two into one.
There was a problem hiding this comment.
Ah I see, yea those two seem to always be used in the same way. Merging them would make sense. Of course that still leaves us with the DW_TAG_structure_type || DW_TAG_class_type checks, but that can always be cleaned up later.
There was a problem hiding this comment.
Yeah, I'm not quite sure how to handle those. We can't just completely erase the difference there because they matter for creation of clang asts. One idea (that just occurred to me) would be to have a sort of a getCanonicalTag() which maps structure_type to class_type (or the other way around), and then do these comparisons on the "canonical" tags.
There was a problem hiding this comment.
they matter for creation of clang asts
FWIW, we do disable the main distinction (access control) between the two in the AST. But fair point, there might be other places that care, not sure off the top of my head.
would be to have a sort of a getCanonicalTag() which maps structure_type to class_type (or the other way around), and then do these comparisons on the "canonical" tags.
Good idea!
There was a problem hiding this comment.
FWIW, I'm pretty sure that the only difference between class and struct is the access control.
Though C++ technically rejects things like forward declaring a thing as a struct, then actually defining it as a class - not sure if that'd ever come up for lldb (for instance when interacting with clang header modules? If the DWARF contained a declaration of a thing and always created it as a struct, but then tried to load a module with that name as a class definition) I guess it'd also maybe break technically valid user code in expression evaluation - if the name was used for a varibale and a type, the user should be able to disambiguate in favor of the type by saying, eg: "sizeof(class MyType)" but if lldb made everything a struct ... hmm, nope, that'd be fine:
$ cat test.cpp
struct MyType { };
int MyType = 3;
static_assert(sizeof(class MyType) == 1);
$ clang++-tot test.cpp -fsyntax-only
I guess printing types in lldb would be technically wrong, since it'd print as "struct MyType {" instead of "class MyType {"?
(but it might mean not having to have the access modifier hack? If everything was made as a struct, and none of the members were given other access modifiers - everything would be accessible by default anyway)
There was a problem hiding this comment.
I guess printing types in lldb would be technically wrong, since it'd print as "struct MyType {" instead of "class MyType {"?
Yes, that's why I said "it matters for ast construction". I think it would nice to still print the type in the original way, which mostly comes up when someone (technically incorrectly) forward-declares something as struct Foo, but defines it as a class.
If we wanted to, we could make all members public regardless of the tag type. I don't know why we don't do that. Is it just because it's nice to see the original access specifiers in the type readouts ? (a rhetorical question) I know that the access specifier can in theory affect the type layout, but I don't think that's true for any real world ABI (and dwarf already encodes the layout anyway).. Some users may declare types inside expressions and their members would be private without the hack, but maybe they should just get what they asked for...
There was a problem hiding this comment.
Yes, that's why I said "it matters for ast construction". I think it would nice to still print the type in the original way, which mostly comes up when someone (technically incorrectly) forward-declares something as
struct Foo, but defines it as a class.
I lost a sentence here.
It would be nice to print the types in the original way. We need the canonicalization to handle the cases where the forward declarations and definitions don't have matching tags.
Our dwarf parsing code treats structures and classes as interchangable. CompilerContextKind is used when looking DIEs for types. This makes sure we always they're treated the same way. See also llvm#95905#discussion_r1645686628.
Our dwarf parsing code treats structures and classes as interchangable. CompilerContextKind is used when looking DIEs for types. This makes sure we always they're treated the same way. See also [#95905#discussion_r1645686628](#95905 (comment)).
…lvm#95905) With simple template names the template arguments aren't embedded in the DW_AT_name attribute of the type. The code in FindDefinitionTypeForDWARFDeclContext was comparing the synthesized template arguments on the leaf (most deeply nested) DIE, but was not sufficient, as the difference get be at any level above that (Foo<T>::Bar vs. Foo<U>::Bar). This patch makes sure we compare the entire context. As a drive-by I also remove the completely unnecessary ConstStringification of the GetDIEClassTemplateParams result.
Our dwarf parsing code treats structures and classes as interchangable. CompilerContextKind is used when looking DIEs for types. This makes sure we always they're treated the same way. See also [llvm#95905#discussion_r1645686628](llvm#95905 (comment)).
With simple template names the template arguments aren't embedded in the DW_AT_name attribute of the type. The code in
FindDefinitionTypeForDWARFDeclContext was comparing the synthesized template arguments on the leaf (most deeply nested) DIE, but was not sufficient, as the difference get be at any level above that (Foo::Bar vs. Foo::Bar). This patch makes sure we compare the entire context.
As a drive-by I also remove the completely unnecessary ConstStringification of the GetDIEClassTemplateParams result.