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

ZequanWu · 2024-05-30T15:56:59Z

This fixes #92328 (comment) by not differentiating DW_TAG_class_type and DW_TAG_structure_type in UniqueDWARFASTTypeList, because it's possible that DIE for a type is DW_TAG_class_type in one CU but is DW_TAG_structure_type in a different CU.

…t_type in UniqueDWARFASTTypeList.

llvmbot · 2024-05-30T15:57:29Z

@llvm/pr-subscribers-lldb

Author: Zequan Wu (ZequanWu)

Changes

This fixes #92328 (comment).

This contains two fixes:

Do not differentiate DW_TAG_class_type and DW_TAG_structure_type in UniqueDWARFASTTypeList, because it's possible that DIE for a type is DW_TAG_class_type in one CU but is DW_TAG_structure_type in a different CU.
In case of we failed to find existing clang type (created from declaration) in UniqueDWARFASTTypeMap for some other reasons, start clang type definition in DWARFASTParserClang::CompleteRecordType to ensure we always started its definition before adding children. This should work because we know the DIE passing in DWARFASTParserClang::CompleteRecordType is always a definition DIE.

Full diff: https://github.com/llvm/llvm-project/pull/93839.diff

2 Files Affected:

(modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp (+5)
(modified) lldb/source/Plugins/SymbolFile/DWARF/UniqueDWARFASTType.cpp (+10-2)

diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index e0b1b430b266f..78969d4752f80 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -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);
     }
 
     AccessType default_accessibility = eAccessNone;
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/UniqueDWARFASTType.cpp b/lldb/source/Plugins/SymbolFile/DWARF/UniqueDWARFASTType.cpp
index 4762356034cab..3d201e96f92c3 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/UniqueDWARFASTType.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/UniqueDWARFASTType.cpp
@@ -13,12 +13,18 @@
 using namespace lldb_private::dwarf;
 using namespace lldb_private::plugin::dwarf;
 
+static bool IsStructOrClassTag(llvm::dwarf::Tag Tag) {
+  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.
@@ -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:

Michael137 · 2024-05-30T16:00:30Z

lldb/source/Plugins/SymbolFile/DWARF/UniqueDWARFASTType.cpp

@@ -13,12 +13,18 @@
 using namespace lldb_private::dwarf;
 using namespace lldb_private::plugin::dwarf;

+static bool IsStructOrClassTag(llvm::dwarf::Tag Tag) {


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

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

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?

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.

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

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));

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

Michael137 · 2024-05-30T16:08:07Z

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

+      // 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);


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

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.

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?

…rClang::CompleteRecordType.

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

Update assertion message. Co-authored-by: Michael Buch <[email protected]>

github-actions · 2024-05-30T16:35:56Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 16a5fd3fdb91ffb39b97dbd3a7e9346ba406360d e7fc16ec5f31693191188b3b95728c4320465923 -- lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp lldb/source/Plugins/SymbolFile/DWARF/UniqueDWARFASTType.cpp

View the diff from clang-format here.

diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index 74bc5ac93a..dc4cfc96b8 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -2233,9 +2233,9 @@ bool DWARFASTParserClang::CompleteRecordType(const DWARFDIE &die,
       // created.
       TypeSystemClang::StartTagDeclarationDefinition(clang_type);
     } else {
-      assert(
-          clang_type.IsBeingDefined() &&
-          "Trying to complete a definition without a prior call to StartTagDeclarationDefinition.");
+      assert(clang_type.IsBeingDefined() &&
+             "Trying to complete a definition without a prior call to "
+             "StartTagDeclarationDefinition.");
     }
 
     AccessType default_accessibility = eAccessNone;

Michael137

Lets do the cleanup of IsClassOrStruct as a follow-up since there seem quite a few places that could benefit from it

… for class/union types" and two follow-up commits. The reason is the crash we've discovered when processing -gsimple-template-names binaries. I'm committing a minimal reproducer as a separate patch. This reverts the following commits: - 51dd4ea (#92328) - 3d9d485 (#93839) - afe6ab7 (#94400)

…ng when parsing declaration DIEs. (#98361) This is a reapply of #92328 and #93839. It now passes the [test](de3f1b6), which crashes with the original reverted changes.

…ng when parsing declaration DIEs. (#98361) Summary: This is a reapply of #92328 and #93839. It now passes the [test](de3f1b6), which crashes with the original reverted changes. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60251550

[lldb][DWARF] Do not differentiate DW_TAG_class_type and DW_TAG_struc…

90cbcf8

…t_type in UniqueDWARFASTTypeList.

ZequanWu requested a review from JDevlieghere as a code owner May 30, 2024 15:57

llvmbot added the lldb label May 30, 2024

ZequanWu requested a review from Michael137 May 30, 2024 15:57

ZequanWu mentioned this pull request May 30, 2024

Reapply [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. #92328

Merged

Michael137 reviewed May 30, 2024

View reviewed changes

Just assert if c/c++ clang_type is not being defined in DWARFASTParse…

d78b4d1

…rClang::CompleteRecordType.

Michael137 reviewed May 30, 2024

View reviewed changes

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp Outdated Show resolved Hide resolved

Apply suggestions from code review

e7fc16e

Update assertion message. Co-authored-by: Michael Buch <[email protected]>

Michael137 approved these changes May 30, 2024

View reviewed changes

format

a09d625

ZequanWu merged commit 3d9d485 into llvm:main May 30, 2024
5 checks passed

ZequanWu deleted the lldb-fix branch May 30, 2024 17:05

ZequanWu mentioned this pull request Jul 10, 2024

Reapply [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. #98361

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ZequanWu commented May 30, 2024 •

edited

Loading

llvmbot commented May 30, 2024

Michael137 May 30, 2024

Michael137 May 30, 2024

ZequanWu May 30, 2024

Michael137 May 30, 2024

ZequanWu May 30, 2024

Michael137 May 30, 2024 •

edited

Loading

felipepiovezan May 30, 2024

Michael137 May 30, 2024 •

edited

Loading

ZequanWu May 30, 2024

Michael137 May 30, 2024

ZequanWu May 30, 2024

github-actions bot commented May 30, 2024

Michael137 left a comment

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

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

Conversation

ZequanWu commented May 30, 2024 • edited Loading

llvmbot commented May 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michael137 May 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michael137 May 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 30, 2024

Michael137 left a comment

Choose a reason for hiding this comment

ZequanWu commented May 30, 2024 •

edited

Loading

Michael137 May 30, 2024 •

edited

Loading

Michael137 May 30, 2024 •

edited

Loading