Skip to content

[lldb][DWARF] Fix adding children to clang type that hasn't started definition. #93839

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

Merged
merged 4 commits into from
May 30, 2024
Merged

[lldb][DWARF] Fix adding children to clang type that hasn't started definition. #93839

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
90cbcf8
[lldb][DWARF] Do not differentiate DW_TAG_class_type and DW_TAG_struc…
ZequanWu May 30, 2024
d78b4d1
Just assert if c/c++ clang_type is not being defined in DWARFASTParse…
ZequanWu May 30, 2024
e7fc16e
Apply suggestions from code review
ZequanWu May 30, 2024
a09d625
format
ZequanWu May 30, 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
5 changes: 5 additions & 0 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 @@ -2232,6 +2232,11 @@ bool DWARFASTParserClang::CompleteRecordType(const DWARFDIE &die,
// For objective C we don't start the definition when the class is
// created.
TypeSystemClang::StartTagDeclarationDefinition(clang_type);
} else if (!clang_type.IsBeingDefined()) {
// In case of some weired DWARF causing we don't start definition on this
// definition DIE because we failed to find existing clang_type from
// UniqueDWARFASTTypeMap due to overstrict checking.
TypeSystemClang::StartTagDeclarationDefinition(clang_type);
Copy link
Member

@Michael137 Michael137 May 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hmmm in the crashing test case the DWARF itself seems reasonable right? So the comment is a bit misleading.

I like the idea of doing both TypeSystemClang::StartTagDeclarationDefinition and TypeSystemClang::CompleteTagDeclarationDefinition in CompleteRecordType (in fact that's what we're trying to do in https://discourse.llvm.org/t/rfc-lldb-more-reliable-completion-of-record-types/77442#changes-5), but I'd like to understand how we get here after #92328 but not prior. Let me re-read your comment on said PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the DWARF is reasonable. I added this just in case of UniqueDWARFASTTypeMap failing to find the existing type again for some other reasons in the future... This checks doesn't get trigger for the test you reported before. Maybe it's not necessary, because the following loop also just check for the fully qualified names: https://github.com/llvm/llvm-project/blob/main/lldb/source/Plugins/SymbolFile/DWARF/UniqueDWARFASTType.cpp#L39-L71.

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, I'd prefer not to add this then if it's going to be an untested codepath. Perhaps it's worth making this an lldbassert?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

}

AccessType default_accessibility = eAccessNone;
Expand Down
12 changes: 10 additions & 2 deletions lldb/source/Plugins/SymbolFile/DWARF/UniqueDWARFASTType.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,18 @@
using namespace lldb_private::dwarf;
using namespace lldb_private::plugin::dwarf;

static bool IsStructOrClassTag(llvm::dwarf::Tag Tag) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we have this function somewhere already. Might be worth checking (possibly even in the llvm headers?)

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes in SymbolFileDWARF::FindDefinitionTypeForDWARFDeclContext we have the same kind of check. Maybe one could consolidate those in one API? But might not be worth the churn, so feel free to ignore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's here:

llvm-project/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

Line 136 in ed35a92

bool IsStructOrClassTag(llvm::dwarf::Tag Tag) {
, maybe move it to namespace lldb_private::plugin::dwarf?

Copy link
Member

Choose a reason for hiding this comment

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

Are there more tag equality checks around LLDB that could benefit from re-using the following check:

udt.m_die.Tag() == die.Tag() || (IsStructOrClassTag(udt.m_die.Tag()) &&
                                         IsStructOrClassTag(die.Tag()))

There's at least two now. Not sure where we'd put such an API. Perhaps @felipepiovezan or @adrian-prantl have some input on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we can probably do it in a different change.

Copy link
Member

@Michael137 Michael137 May 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There's also

/// Returns true if `tag` is a class_type of structure_type tag.  
static bool IsClassOrStruct(dw_tag_t tag) {                       
  return tag == DW_TAG_class_type || tag == DW_TAG_structure_type;
}

In AppleDWARFIndex FYI

And it uses that for the same type of check:

 return tag == expected_tag ||                                         
        (IsClassOrStruct(tag) && IsClassOrStruct(expected_tag));

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, we definitely need to clean all of these up.
There might be one inside llvm/ too..

return Tag == llvm::dwarf::Tag::DW_TAG_class_type ||
Tag == llvm::dwarf::Tag::DW_TAG_structure_type;
}

UniqueDWARFASTType *UniqueDWARFASTTypeList::Find(
const DWARFDIE &die, const lldb_private::Declaration &decl,
const int32_t byte_size, bool is_forward_declaration) {
for (UniqueDWARFASTType &udt : m_collection) {
// Make sure the tags match
if (udt.m_die.Tag() == die.Tag()) {
if (udt.m_die.Tag() == die.Tag() || (IsStructOrClassTag(udt.m_die.Tag()) &&
IsStructOrClassTag(die.Tag()))) {
// If they are not both definition DIEs or both declaration DIEs, then
// don't check for byte size and declaration location, because declaration
// DIEs usually don't have those info.
Expand All @@ -39,7 +45,9 @@ UniqueDWARFASTType *UniqueDWARFASTTypeList::Find(
while (!done && match && parent_arg_die && parent_pos_die) {
const dw_tag_t parent_arg_tag = parent_arg_die.Tag();
const dw_tag_t parent_pos_tag = parent_pos_die.Tag();
if (parent_arg_tag == parent_pos_tag) {
if (parent_arg_tag == parent_pos_tag ||
(IsStructOrClassTag(parent_arg_tag) &&
IsStructOrClassTag(parent_pos_tag))) {
switch (parent_arg_tag) {
case DW_TAG_class_type:
case DW_TAG_structure_type:
Expand Down