Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[lldb/DWARF] Fix type definition search with simple template names #95905

Merged
merged 3 commits into from
Jun 20, 2024
Merged

[lldb/DWARF] Fix type definition search with simple template names #95905

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9429eef
[lldb/DWARF] Fix type definition search with simple template names
labath Jun 17, 2024
e5b7c3e
s/""/{}
labath Jun 19, 2024
25c2776
fix loop condition
labath Jun 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
16 changes: 8 additions & 8 deletions lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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);
}
Expand All @@ -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;
}
Expand Down
4 changes: 2 additions & 2 deletions lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
70 changes: 36 additions & 34 deletions lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At some point we should probably extract this into a standalone helper. We're accumulating a lot of these checks

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we could remove the distinction at least from there?

Mind pointing me to the place you're referring to?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (!tag_matches)
return false;
Michael137 marked this conversation as resolved.
Show resolved Hide resolved
if (any_template_params) {
size_t pos = 0;
for (DWARFDIE ctx_die = type_die;
ctx_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
Expand All @@ -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,
Expand Down Expand Up @@ -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;
});
Expand Down
2 changes: 1 addition & 1 deletion lldb/test/Shell/SymbolFile/DWARF/x86/simple-template-names-context.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]*}} {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yay

// CHECK-NEXT: }
// CHECK-NEXT: (ReferencesBoth<'B'>) both_b = {
// CHECK-NEXT: (Outer<'A'>::Inner *) a = 0x{{[0-9A-Fa-f]*}} {}
Expand Down
Loading