[lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. #90663
Conversation
…parsing declaration DIEs.
|
@llvm/pr-subscribers-lldb Author: Zequan Wu (ZequanWu) ChangesThis is the implementation for https://discourse.llvm.org/t/rfc-delay-definition-die-searching-when-parse-a-declaration-die-for-record-type/78526. MotivationCurrently, lldb eagerly searches for definition DIE when parsing a declaration DIE for struct/class/union definition DIE. It will search for all definition DIEs with the same unqualified name (just ImplementationInstead of searching for definition DIEs when parsing declaration DIEs, we always construct the record type from the DIE regardless if it's definition or declaration. At the same time, use a map The key difference is ResultIt fixes the stack overflow of lldb for the internal binary built with simple template name. When constructing the fully qualified name built with Patch is 34.35 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/90663.diff 7 Files Affected:
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index bea11e0e3840af..7ad661c9a9d49b 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -252,7 +252,8 @@ static void ForcefullyCompleteType(CompilerType type) {
static void PrepareContextToReceiveMembers(TypeSystemClang &ast,
ClangASTImporter &ast_importer,
clang::DeclContext *decl_ctx,
- DWARFDIE die,
+ const DWARFDIE &decl_ctx_die,
+ const DWARFDIE &die,
const char *type_name_cstr) {
auto *tag_decl_ctx = clang::dyn_cast<clang::TagDecl>(decl_ctx);
if (!tag_decl_ctx)
@@ -279,6 +280,13 @@ static void PrepareContextToReceiveMembers(TypeSystemClang &ast,
type_name_cstr ? type_name_cstr : "", die.GetOffset());
}
+ // By searching for the definition DIE of the decl_ctx type, we will either:
+ // 1. Found the the definition DIE and start its definition with
+ // TypeSystemClang::StartTagDeclarationDefinition.
+ // 2. Unable to find it, then need to forcefully complete it.
+ die.GetDWARF()->FindDefinitionDIE(decl_ctx_die);
+ if (tag_decl_ctx->isCompleteDefinition() || tag_decl_ctx->isBeingDefined())
+ return;
// We don't have a type definition and/or the import failed. We must
// forcefully complete the type to avoid crashes.
ForcefullyCompleteType(type);
@@ -620,10 +628,11 @@ DWARFASTParserClang::ParseTypeModifier(const SymbolContext &sc,
if (tag == DW_TAG_typedef) {
// DeclContext will be populated when the clang type is materialized in
// Type::ResolveCompilerType.
- PrepareContextToReceiveMembers(
- m_ast, GetClangASTImporter(),
- GetClangDeclContextContainingDIE(die, nullptr), die,
- attrs.name.GetCString());
+ DWARFDIE decl_ctx_die;
+ clang::DeclContext *decl_ctx =
+ GetClangDeclContextContainingDIE(die, &decl_ctx_die);
+ PrepareContextToReceiveMembers(m_ast, GetClangASTImporter(), decl_ctx,
+ decl_ctx_die, die, attrs.name.GetCString());
if (attrs.type.IsValid()) {
// Try to parse a typedef from the (DWARF embedded in the) Clang
@@ -1100,32 +1109,6 @@ DWARFASTParserClang::ParseSubroutine(const DWARFDIE &die,
// struct and see if this is actually a C++ method
Type *class_type = dwarf->ResolveType(decl_ctx_die);
if (class_type) {
- if (class_type->GetID() != decl_ctx_die.GetID() ||
- IsClangModuleFwdDecl(decl_ctx_die)) {
-
- // We uniqued the parent class of this function to another
- // class so we now need to associate all dies under
- // "decl_ctx_die" to DIEs in the DIE for "class_type"...
- DWARFDIE class_type_die = dwarf->GetDIE(class_type->GetID());
-
- if (class_type_die) {
- std::vector<DWARFDIE> failures;
-
- CopyUniqueClassMethodTypes(decl_ctx_die, class_type_die,
- class_type, failures);
-
- // FIXME do something with these failures that's
- // smarter than just dropping them on the ground.
- // Unfortunately classes don't like having stuff added
- // to them after their definitions are complete...
-
- Type *type_ptr = dwarf->GetDIEToType()[die.GetDIE()];
- if (type_ptr && type_ptr != DIE_IS_BEING_PARSED) {
- return type_ptr->shared_from_this();
- }
- }
- }
-
if (attrs.specification.IsValid()) {
// We have a specification which we are going to base our
// function prototype off of, so we need this type to be
@@ -1260,6 +1243,39 @@ DWARFASTParserClang::ParseSubroutine(const DWARFDIE &die,
}
}
}
+ // By here, we should have already completed the c++ class_type
+ // because if either specification or abstract_origin is present, we
+ // call GetClangDeclContextForDIE to resolve the DW_TAG_subprogram
+ // refered by this one until we reached the DW_TAG_subprogram without
+ // specification or abstract_origin (the else branch above). Then the
+ // above GetFullCompilerType() will complete the class_type if it's
+ // not completed yet. After that, we will have the mapping from DIEs
+ // in class_type_die to DeclContexts in m_die_to_decl_ctx.
+ if (class_type->GetID() != decl_ctx_die.GetID() ||
+ IsClangModuleFwdDecl(decl_ctx_die)) {
+
+ // We uniqued the parent class of this function to another
+ // class so we now need to associate all dies under
+ // "decl_ctx_die" to DIEs in the DIE for "class_type"...
+ DWARFDIE class_type_die = dwarf->GetDIE(class_type->GetID());
+
+ if (class_type_die) {
+ std::vector<DWARFDIE> failures;
+
+ CopyUniqueClassMethodTypes(decl_ctx_die, class_type_die,
+ class_type, failures);
+
+ // FIXME do something with these failures that's
+ // smarter than just dropping them on the ground.
+ // Unfortunately classes don't like having stuff added
+ // to them after their definitions are complete...
+
+ Type *type_ptr = dwarf->GetDIEToType()[die.GetDIE()];
+ if (type_ptr && type_ptr != DIE_IS_BEING_PARSED) {
+ return type_ptr->shared_from_this();
+ }
+ }
+ }
}
}
}
@@ -1651,6 +1667,7 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
ConstString unique_typename(attrs.name);
Declaration unique_decl(attrs.decl);
+ uint64_t byte_size = attrs.byte_size.value_or(0);
if (attrs.name) {
if (Language::LanguageIsCPlusPlus(cu_language)) {
@@ -1664,13 +1681,34 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
}
if (dwarf->GetUniqueDWARFASTTypeMap().Find(
- unique_typename, die, unique_decl, attrs.byte_size.value_or(-1),
- *unique_ast_entry_up)) {
+ unique_typename, die, unique_decl, byte_size,
+ attrs.is_forward_declaration, *unique_ast_entry_up)) {
type_sp = unique_ast_entry_up->m_type_sp;
if (type_sp) {
dwarf->GetDIEToType()[die.GetDIE()] = type_sp.get();
LinkDeclContextToDIE(
GetCachedClangDeclContextForDIE(unique_ast_entry_up->m_die), die);
+ if (!attrs.is_forward_declaration) {
+ dwarf->GetDeclarationDIEToDefinitionDIE().try_emplace(
+ unique_ast_entry_up->m_die.GetDIE(), *die.GetDIERef());
+ // If the parameter DIE is definition and the entry in the map is
+ // declaration, then we need to update the entry to point to the
+ // definition DIE.
+ if (unique_ast_entry_up->m_is_forward_declaration) {
+ unique_ast_entry_up->m_die = die;
+ unique_ast_entry_up->m_byte_size = byte_size;
+ unique_ast_entry_up->m_declaration = unique_decl;
+ unique_ast_entry_up->m_is_forward_declaration = false;
+ // Need to update Type ID to refer to the definition DIE. because
+ // it's used in ParseSubroutine to determine if we need to copy cxx
+ // method types from a declaration DIE to this definition DIE.
+ type_sp->SetID(die.GetID());
+ if (attrs.class_language != eLanguageTypeObjC &&
+ attrs.class_language != eLanguageTypeObjC_plus_plus)
+ TypeSystemClang::StartTagDeclarationDefinition(
+ type_sp->GetForwardCompilerType());
+ }
+ }
return type_sp;
}
}
@@ -1707,112 +1745,22 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
attrs.is_forward_declaration = true;
}
- if (attrs.class_language == eLanguageTypeObjC ||
- attrs.class_language == eLanguageTypeObjC_plus_plus) {
- if (!attrs.is_complete_objc_class &&
- die.Supports_DW_AT_APPLE_objc_complete_type()) {
- // We have a valid eSymbolTypeObjCClass class symbol whose name
- // matches the current objective C class that we are trying to find
- // and this DIE isn't the complete definition (we checked
- // is_complete_objc_class above and know it is false), so the real
- // definition is in here somewhere
- type_sp =
- dwarf->FindCompleteObjCDefinitionTypeForDIE(die, attrs.name, true);
-
- if (!type_sp) {
- SymbolFileDWARFDebugMap *debug_map_symfile =
- dwarf->GetDebugMapSymfile();
- if (debug_map_symfile) {
- // We weren't able to find a full declaration in this DWARF,
- // see if we have a declaration anywhere else...
- type_sp = debug_map_symfile->FindCompleteObjCDefinitionTypeForDIE(
- die, attrs.name, true);
- }
- }
-
- if (type_sp) {
- if (log) {
- dwarf->GetObjectFile()->GetModule()->LogMessage(
- log,
- "SymbolFileDWARF({0:p}) - {1:x16}: {2} type "
- "\"{3}\" is an "
- "incomplete objc type, complete type is {4:x8}",
- static_cast<void *>(this), die.GetOffset(),
- DW_TAG_value_to_name(tag), attrs.name.GetCString(),
- type_sp->GetID());
- }
-
- // We found a real definition for this type elsewhere so lets use
- // it and cache the fact that we found a complete type for this
- // die
- dwarf->GetDIEToType()[die.GetDIE()] = type_sp.get();
- return type_sp;
- }
- }
- }
-
if (attrs.is_forward_declaration) {
- // We have a forward declaration to a type and we need to try and
- // find a full declaration. We look in the current type index just in
- // case we have a forward declaration followed by an actual
- // declarations in the DWARF. If this fails, we need to look
- // elsewhere...
- if (log) {
- dwarf->GetObjectFile()->GetModule()->LogMessage(
- log,
- "SymbolFileDWARF({0:p}) - {1:x16}: {2} type \"{3}\" is a "
- "forward declaration, trying to find complete type",
- static_cast<void *>(this), die.GetOffset(), DW_TAG_value_to_name(tag),
- attrs.name.GetCString());
- }
-
// See if the type comes from a Clang module and if so, track down
// that type.
type_sp = ParseTypeFromClangModule(sc, die, log);
if (type_sp)
return type_sp;
-
- // type_sp = FindDefinitionTypeForDIE (dwarf_cu, die,
- // type_name_const_str);
- type_sp = dwarf->FindDefinitionTypeForDWARFDeclContext(die);
-
- if (!type_sp) {
- SymbolFileDWARFDebugMap *debug_map_symfile = dwarf->GetDebugMapSymfile();
- if (debug_map_symfile) {
- // We weren't able to find a full declaration in this DWARF, see
- // if we have a declaration anywhere else...
- type_sp = debug_map_symfile->FindDefinitionTypeForDWARFDeclContext(die);
- }
- }
-
- if (type_sp) {
- if (log) {
- dwarf->GetObjectFile()->GetModule()->LogMessage(
- log,
- "SymbolFileDWARF({0:p}) - {1:x16}: {2} type \"{3}\" is a "
- "forward declaration, complete type is {4:x8}",
- static_cast<void *>(this), die.GetOffset(),
- DW_TAG_value_to_name(tag), attrs.name.GetCString(),
- type_sp->GetID());
- }
-
- // We found a real definition for this type elsewhere so lets use
- // it and cache the fact that we found a complete type for this die
- dwarf->GetDIEToType()[die.GetDIE()] = type_sp.get();
- clang::DeclContext *defn_decl_ctx =
- GetCachedClangDeclContextForDIE(dwarf->GetDIE(type_sp->GetID()));
- if (defn_decl_ctx)
- LinkDeclContextToDIE(defn_decl_ctx, die);
- return type_sp;
- }
}
+
assert(tag_decl_kind != -1);
UNUSED_IF_ASSERT_DISABLED(tag_decl_kind);
- bool clang_type_was_created = false;
- clang::DeclContext *decl_ctx = GetClangDeclContextContainingDIE(die, nullptr);
+ DWARFDIE decl_ctx_die;
+ clang::DeclContext *decl_ctx =
+ GetClangDeclContextContainingDIE(die, &decl_ctx_die);
- PrepareContextToReceiveMembers(m_ast, GetClangASTImporter(), decl_ctx, die,
- attrs.name.GetCString());
+ PrepareContextToReceiveMembers(m_ast, GetClangASTImporter(), decl_ctx,
+ decl_ctx_die, die, attrs.name.GetCString());
if (attrs.accessibility == eAccessNone && decl_ctx) {
// Check the decl context that contains this class/struct/union. If
@@ -1850,20 +1798,20 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
tag_decl_kind, template_param_infos);
clang_type =
m_ast.CreateClassTemplateSpecializationType(class_specialization_decl);
- clang_type_was_created = true;
m_ast.SetMetadata(class_template_decl, metadata);
m_ast.SetMetadata(class_specialization_decl, metadata);
}
- if (!clang_type_was_created) {
- clang_type_was_created = true;
+ if (!clang_type) {
clang_type = m_ast.CreateRecordType(
decl_ctx, GetOwningClangModule(die), attrs.accessibility,
attrs.name.GetCString(), tag_decl_kind, attrs.class_language, &metadata,
attrs.exports_symbols);
}
-
+ if (!attrs.is_forward_declaration)
+ dwarf->GetDeclarationDIEToDefinitionDIE().try_emplace(die.GetDIE(),
+ *die.GetDIERef());
// Store a forward declaration to this class type in case any
// parameters in any class methods need it for the clang types for
// function prototypes.
@@ -1880,7 +1828,8 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
unique_ast_entry_up->m_type_sp = type_sp;
unique_ast_entry_up->m_die = die;
unique_ast_entry_up->m_declaration = unique_decl;
- unique_ast_entry_up->m_byte_size = attrs.byte_size.value_or(0);
+ unique_ast_entry_up->m_byte_size = byte_size;
+ unique_ast_entry_up->m_is_forward_declaration = attrs.is_forward_declaration;
dwarf->GetUniqueDWARFASTTypeMap().Insert(unique_typename,
*unique_ast_entry_up);
@@ -1921,38 +1870,33 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
GetClangASTImporter().SetRecordLayout(record_decl, layout);
}
}
- } else if (clang_type_was_created) {
- // Start the definition if the class is not objective C since the
- // underlying decls respond to isCompleteDefinition(). Objective
- // C decls don't respond to isCompleteDefinition() so we can't
- // start the declaration definition right away. For C++
- // class/union/structs we want to start the definition in case the
- // class is needed as the declaration context for a contained class
- // or type without the need to complete that type..
-
- if (attrs.class_language != eLanguageTypeObjC &&
- attrs.class_language != eLanguageTypeObjC_plus_plus)
- TypeSystemClang::StartTagDeclarationDefinition(clang_type);
-
- // Leave this as a forward declaration until we need to know the
- // details of the type. lldb_private::Type will automatically call
- // the SymbolFile virtual function
- // "SymbolFileDWARF::CompleteType(Type *)" When the definition
- // needs to be defined.
- assert(!dwarf->GetForwardDeclCompilerTypeToDIE().count(
- ClangUtil::RemoveFastQualifiers(clang_type)
- .GetOpaqueQualType()) &&
- "Type already in the forward declaration map!");
- // Can't assume m_ast.GetSymbolFile() is actually a
- // SymbolFileDWARF, it can be a SymbolFileDWARFDebugMap for Apple
- // binaries.
- dwarf->GetForwardDeclCompilerTypeToDIE().try_emplace(
- ClangUtil::RemoveFastQualifiers(clang_type).GetOpaqueQualType(),
- *die.GetDIERef());
- m_ast.SetHasExternalStorage(clang_type.GetOpaqueQualType(), true);
}
+ // Start the definition if the class is not objective C since the
+ // underlying decls respond to isCompleteDefinition(). Objective
+ // C decls don't respond to isCompleteDefinition() so we can't
+ // start the declaration definition right away. For C++
+ // class/union/structs we want to start the definition in case the
+ // class is needed as the declaration context for a contained class
+ // or type without the need to complete that type..
+
+ if (attrs.class_language != eLanguageTypeObjC &&
+ attrs.class_language != eLanguageTypeObjC_plus_plus)
+ TypeSystemClang::StartTagDeclarationDefinition(clang_type);
}
+ // Leave this as a forward declaration until we need to know the
+ // details of the type. lldb_private::Type will automatically call
+ // the SymbolFile virtual function
+ // "SymbolFileDWARF::CompleteType(Type *)" When the definition
+ // needs to be defined.
+ assert(!dwarf->GetForwardDeclCompilerTypeToDIE().count(
+ ClangUtil::RemoveFastQualifiers(clang_type).GetOpaqueQualType()) &&
+ "Type already in the forward declaration map!");
+ dwarf->GetForwardDeclCompilerTypeToDIE().try_emplace(
+ ClangUtil::RemoveFastQualifiers(clang_type).GetOpaqueQualType(),
+ *die.GetDIERef());
+ m_ast.SetHasExternalStorage(clang_type.GetOpaqueQualType(), true);
+
// If we made a clang type, set the trivial abi if applicable: We only
// do this for pass by value - which implies the Trivial ABI. There
// isn't a way to assert that something that would normally be pass by
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index 49f13d2c89e380..5a317db7e74028 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -1631,13 +1631,19 @@ bool SymbolFileDWARF::CompleteType(CompilerType &compiler_type) {
return true;
}
- DWARFDIE dwarf_die = GetDIE(die_it->getSecond());
+ DWARFDIE dwarf_die = FindDefinitionDIE(GetDIE(die_it->getSecond()));
if (dwarf_die) {
// Once we start resolving this type, remove it from the forward
// declaration map in case anyone child members or other types require this
// type to get resolved. The type will get resolved when all of the calls
// to SymbolFileDWARF::ResolveClangOpaqueTypeDefinition are done.
- GetForwardDeclCompilerTypeToDIE().erase(die_it);
+ // Need to get a new iterator because FindDefinitionDIE might add new
+ // entries into the map and invalidate previous iterator.
+ auto die_it = GetForwardDeclCompilerTypeToDIE().find(
+ compiler_type_no_qualifiers.GetOpaqueQualType());
+ if (die_it != GetForwardDeclCompilerTypeToDIE().end()) {
+ GetForwardDeclCompilerTypeToDIE().erase(die_it);
+ }
Type *type = GetDIEToType().lookup(dwarf_die.GetDIE());
@@ -1654,6 +1660,101 @@ bool SymbolFileDWARF::CompleteType(CompilerType &compiler_type) {
return false;
}
+DWARFDIE SymbolFileDWARF::FindDefinitionDIE(const DWARFDIE &die) {
+ auto def_die_it = GetDeclarationDIEToDefinitionDIE().find(die.GetDIE());
+ if (def_die_it != GetDeclarationDIEToDefinitionDIE().end())
+ return GetDIE(def_die_it->getSecond());
+
+ ParsedDWARFTypeAttributes attrs(die);
+ const dw_tag_t tag = die.Tag();
+ TypeSP type_sp;
+ Log *log = GetLog(DWARFLog::TypeCompletion | DWARFLog::Lookups);
+ if (log) {
+ GetObjectFile()->GetModule()->LogMessage(
+ log,
+ "SymbolFileDWARF({0:p}) - {1:x16}: {2} type \"{3}\" is a "
+ "forward declaration DIE, trying to find definition DIE",
+ static_cast<void *>(this), die.GetOffset(), DW_TAG_value_to_name(tag),
+ attrs.name.GetCString());
+ }
+ // We haven't parse definition die for this type, starting to search for it.
+ // After we found the definition die, the GetDeclarationDIEToDefinitionDIE()...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
does this cause multiple (an open ended amount?) of declarations for a type to be created if the type declarations from multiple CUs are encountered separately? Or still only one due to the extra map? |
This only creates one even if type declarations are from different CUs. The definition DIE lookup mechanism is the same as before, via manual index, which is able to search cross CUs. We check if a type is created before using the |
|
Hmm - but the byte size wouldn't be known from only a declaration, right? so how'd that key work between a declaration and a definition? |
For declaration and definition DIEs, we just look at the name. Byte size and declaration locations are only used as extra info to differentiate two definition DIEs. See the comment in: https://github.com/llvm/llvm-project/pull/90663/files#diff-82e596d532f38e5212da4007f8ffda731ac8c948ab98eaac21f30fc96ca62d30R24-R32 |
| if (dwarf_die) { | ||
| // Once we start resolving this type, remove it from the forward | ||
| // declaration map in case anyone child members or other types require this | ||
| // type to get resolved. The type will get resolved when all of the calls | ||
| // to SymbolFileDWARF::ResolveClangOpaqueTypeDefinition are done. | ||
| GetForwardDeclCompilerTypeToDIE().erase(die_it); | ||
| // Need to get a new iterator because FindDefinitionDIE might add new |
There was a problem hiding this comment.
Would it be possible to erase the iterator before calling FindDefinitionDIE?
There was a problem hiding this comment.
Now, we change m_forward_decl_compiler_type_to_die to be updated with definition DIE. So, we may need to erase twice, because the first erase is always necessary if we failed to find definition DIE for it. The second erase is because calling FindDefinitionDIE might add new entry to the definition DIE because the first one removed it.
| CompilerTypeToDIE m_forward_decl_compiler_type_to_die; | ||
| // A map from a struct/class/union/enum DIE (might be a declaration or a | ||
| // definition) to its definition DIE. | ||
| DIEToDIE m_die_to_def_die; |
There was a problem hiding this comment.
Would it be possible to avoid creating the extra map, for example by modifying the m_forward_decl_compiler_type_to_die to point to the definition DIE -- after that DIE has been located?
There was a problem hiding this comment.
(I also can't help but wonder why do we need that m_forward_decl_compiler_type_to_die in the first place, given that the ClangASTMetadata associated with the type already contains a user ID, which should allow us to retrieve original DIE. I know this isn't related to your patch, I'm just writing it in case you or someone has any thoughts on this.)
| DWARFDIE die, | ||
| const char *type_name_cstr) { | ||
| void DWARFASTParserClang::PrepareContextToReceiveMembers( | ||
| TypeSystemClang &ast, clang::DeclContext *decl_ctx, |
There was a problem hiding this comment.
This arg is not necessary now that this is a member
| // Return true if we found the definition DIE for it. is_forward_declaration | ||
| // is set to true if the parameter die is a declaration. | ||
| virtual bool | ||
| FindDefinitionDIE(const lldb_private::plugin::dwarf::DWARFDIE &die, |
There was a problem hiding this comment.
Delete lldb_private::plugin::dwarf::
| } | ||
|
|
||
| // Leave this as a forward declaration until we need to know the |
There was a problem hiding this comment.
I'm having trouble reconciling this comment with the one above (line 1974). How can we leave this as a forward declaration if we have already (conditionally, if we are processing a definition DIE of a non-objc type) started its definition (line 1984). What exactly is this trying to say?
There was a problem hiding this comment.
I didn't modify the original comments. It doesn't make sense to me as well. I append "If this is a declaration DIE" in front of this comment.
| @@ -108,6 +108,9 @@ class DWARFASTParserClang : public lldb_private::plugin::dwarf::DWARFASTParser { | |||
| lldb_private::ConstString GetDIEClassTemplateParams( | |||
| const lldb_private::plugin::dwarf::DWARFDIE &die) override; | |||
There was a problem hiding this comment.
delete lldb_private::plugin::dwarf:: (and elsewhere in this file)
|
|
||
| if (attrs.class_language != eLanguageTypeObjC && | ||
| attrs.class_language != eLanguageTypeObjC_plus_plus) | ||
| TypeSystemClang::StartTagDeclarationDefinition(clang_type); |
There was a problem hiding this comment.
Is it a problem that clang_type can already have its definition started (and completed) on line 1944 (if the DIE has no children)? Should this maybe be in some sort of an else branch?
There was a problem hiding this comment.
Oh, I didn't notice it. Moved this into an else branch.
| @@ -1632,6 +1669,96 @@ DWARFASTParserClang::GetCPlusPlusQualifiedName(const DWARFDIE &die) { | |||
| return qualified_name; | |||
| } | |||
|
|
|||
| bool DWARFASTParserClang::FindDefinitionDIE(const DWARFDIE &die, | |||
| bool &is_forward_declaration) { | |||
There was a problem hiding this comment.
I'm having trouble finding where is this by-ref argument actually used in the caller. Can we get rid of it (perhaps by moving the IsForwardDeclaration call to the caller)?
There was a problem hiding this comment.
I was about to use it in SymbolFileDWARF::CompleteType: https://github.com/llvm/llvm-project/pull/90663/files#diff-edef3a65d5d569bbb75a4158d35b827aa5d42ee03ccd3b0c1d4f354afa12210cR1645. But with some refactor change, it's no longer useful anymore.
Removed.
| : (udt.m_byte_size < 0 || byte_size < 0 || | ||
| udt.m_byte_size == byte_size) && | ||
| udt.m_declaration == decl; | ||
| if (matching_size_declaration) { |
There was a problem hiding this comment.
For readability, can we do:
if (!matching_size_declaration)
continue;
?
| type_sp = unique_ast_entry_up->m_type_sp; | ||
| if (type_sp) { | ||
| dwarf->GetDIEToType()[die.GetDIE()] = type_sp.get(); | ||
| LinkDeclContextToDIE( | ||
| GetCachedClangDeclContextForDIE(unique_ast_entry_up->m_die), die); | ||
| if (!attrs.is_forward_declaration) { | ||
| // If the parameter DIE is definition and the entry in the map is |
There was a problem hiding this comment.
I find parameter DIE a bit confusing. Made me think of templates or functions. Could we just reference the parameter, e.g., If the 'die' being parsed is a definition ... or something like that
Good question. Though this is mostly meant to be "NFC" (with very large quotes), I can imagine us doing something like forcing the parsing of a specific type ( |
Yea that could work. But if it's going to be very painful or fragile to test then don't let that hold back the PR |
There was a problem hiding this comment.
Though this is mostly meant to be "NFC" (with very large quotes)
Yeah, this is mostly "NFC". A noticeable difference is we now set the type created from declaration with TypeSystemClang::SetHasExternalStorage without knowing if there's a definition or not. Based on my knowledge, it simply tells clang that the type can be completed by external AST source and ultimately calls SymbolFileDWARF::CompleteType to complete it. If there isn't one, we just fail complete it. Maybe we should also force completion of the type in this case?
Update:
Actually the behaviour of forced type completion is remained the same: Only force containing type completion when trying to parse a contained type and the containing type has no definition.
| DWARFDIE die, | ||
| const char *type_name_cstr) { | ||
| void DWARFASTParserClang::PrepareContextToReceiveMembers( | ||
| TypeSystemClang &ast, clang::DeclContext *decl_ctx, |
| // Return true if we found the definition DIE for it. is_forward_declaration | ||
| // is set to true if the parameter die is a declaration. | ||
| virtual bool | ||
| FindDefinitionDIE(const lldb_private::plugin::dwarf::DWARFDIE &die, |
| @@ -1632,6 +1669,96 @@ DWARFASTParserClang::GetCPlusPlusQualifiedName(const DWARFDIE &die) { | |||
| return qualified_name; | |||
| } | |||
|
|
|||
| bool DWARFASTParserClang::FindDefinitionDIE(const DWARFDIE &die, | |||
| bool &is_forward_declaration) { | |||
There was a problem hiding this comment.
I was about to use it in SymbolFileDWARF::CompleteType: https://github.com/llvm/llvm-project/pull/90663/files#diff-edef3a65d5d569bbb75a4158d35b827aa5d42ee03ccd3b0c1d4f354afa12210cR1645. But with some refactor change, it's no longer useful anymore.
Removed.
|
|
||
| if (attrs.class_language != eLanguageTypeObjC && | ||
| attrs.class_language != eLanguageTypeObjC_plus_plus) | ||
| TypeSystemClang::StartTagDeclarationDefinition(clang_type); |
There was a problem hiding this comment.
Oh, I didn't notice it. Moved this into an else branch.
| } | ||
|
|
||
| // Leave this as a forward declaration until we need to know the |
There was a problem hiding this comment.
I didn't modify the original comments. It doesn't make sense to me as well. I append "If this is a declaration DIE" in front of this comment.
| @@ -108,6 +108,9 @@ class DWARFASTParserClang : public lldb_private::plugin::dwarf::DWARFASTParser { | |||
| lldb_private::ConstString GetDIEClassTemplateParams( | |||
| const lldb_private::plugin::dwarf::DWARFDIE &die) override; | |||
| : (udt.m_byte_size < 0 || byte_size < 0 || | ||
| udt.m_byte_size == byte_size) && | ||
| udt.m_declaration == decl; | ||
| if (matching_size_declaration) { |
| type_sp = unique_ast_entry_up->m_type_sp; | ||
| if (type_sp) { | ||
| dwarf->GetDIEToType()[die.GetDIE()] = type_sp.get(); | ||
| LinkDeclContextToDIE( | ||
| GetCachedClangDeclContextForDIE(unique_ast_entry_up->m_die), die); | ||
| if (!attrs.is_forward_declaration) { | ||
| // If the parameter DIE is definition and the entry in the map is |
In terms of testing, since this only delays definition DIE searching not type completion, we need to construct a test so that lldb finds the declaration DIE first without trigger a type completion on it and somehow test the incomplete type. The first part is tricky. I'm not sure how to achieve it. |
You should be able to make a test case with two files. One file contains the class definition and the other uses only a forward declaration. You could test by running the binary stopping only in the one with the forward declaration. So file 1 would be something like Then having Then run to "Breakpoint 1 here" and then run |
The tests your described testing this change doesn't break things by delaying definition DIE searching, which I think is already covered by existing tests (created for other purposes, but also covers this case). I was thinking about testing the definition DIE searching is actually delayed. Maybe there isn't a way to do it. |
You could enable logging and check for specific logging after steps. In the test I described above if you just print the "Foo *foo" variable, it won't need to complete the definition, you could check for logging, and then if you print "*foo", then it should complete the definition and you should see logging. Not sure if that is what you meant |
|
I was able to reproduce the failure of these three: lldb-api :: lang/c/forward/TestForwardDeclaration.py locally. Reverting this patch and a7eff59 which depended on it made the failure go away. I reverted the patches for now to get the bots clean. |
|
@ZequanWu in the future, if one of your commits break a bot, make sure to revert it immediately, you can always re-land it later with a fix or an explanation why it wasn't your commit that broke the bots. Reverting a commit is cheap, red bots are expensive :-) |
Thanks. Can you provide instructions to repro the failure locally?
Will do. |
|
On May 13, 2024, at 6:04 PM, Zequan Wu ***@***.***> wrote:
I was able to reproduce the failure of these three:
lldb-api :: lang/c/forward/TestForwardDeclaration.py
lldb-api :: lang/cpp/unique-types3/TestUniqueTypes3.py
lldb-api :: types/TestRecursiveTypes.py
locally. Reverting this patch and a7eff59 <a7eff59> which depended on it made the failure go away.
I reverted the patches for now to get the bots clean.
Thanks. Can you provide instructions to repro the failure locally?
I didn't do anything out of the ordinary. I'm on macOS Sonoma 14.4.1, with the latest Xcode(15.3). I ran the CMakery this way:
cmake -B `pwd`/all-in-one -G Ninja -C `pwd`/llvm-project/lldb/cmake/caches/Apple-lldb-base.cmake -DLLVM_ENABLE_PROJECTS="clang;lldb" -DLLVM_ENABLE_RUNTIMES="libcxx;libcxxabi;compiler-rt;libunwind" -DLIBCXX_INCLUDE_TESTS=OFF -DCMAKE_BUILD_TYPE=Debug -DCMAKE_OSX_DEPLOYMENT_TARGET:STRING=12.3 -DLLDB_BUILD_FRAMEWORK=YES -DCMAKE_CXX_STANDARD:STRING=17 llvm-project/llvm
Then:
$ cd all-in-one
$ ninja check-lldb
and got the failure.
Jim
… @ZequanWu <https://github.com/ZequanWu> in the future, if one of your commits break a bot, make sure to revert it immediately, you can always re-land it later with a fix or an explanation why it wasn't your commit that broke the bots. Reverting a commit is cheap, red bots are expensive :-)
Will do.
—
Reply to this email directly, view it on GitHub <#90663 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW2VA2CFK2NIEANWQG3ZCFPJJAVCNFSM6AAAAABHA7HMYWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMBZGA4DCMZSGY>.
You are receiving this because you are on a team that was mentioned.
|
|
BTW, do you know what's up with this test:
SymbolFile/DWARF/delayed-definition-die-searching.test
your commit deleted that file I think, I added it back when I did the revert (possibly a mistake)... It passes on my macOS system but is failing on Ubuntu after the revert. I think I'll just disable it for now.
Jim
… On May 13, 2024, at 6:09 PM, Jim Ingham ***@***.***> wrote:
>
>
>
>> On May 13, 2024, at 6:04 PM, Zequan Wu ***@***.***> wrote:
>>
>>
>> I was able to reproduce the failure of these three:
>>
>> lldb-api :: lang/c/forward/TestForwardDeclaration.py
>> lldb-api :: lang/cpp/unique-types3/TestUniqueTypes3.py
>> lldb-api :: types/TestRecursiveTypes.py
>>
>> locally. Reverting this patch and a7eff59 <a7eff59> which depended on it made the failure go away.
>>
>> I reverted the patches for now to get the bots clean.
>>
>> Thanks. Can you provide instructions to repro the failure locally?
>>
> I didn't do anything out of the ordinary. I'm on macOS Sonoma 14.4.1, with the latest Xcode(15.3). I ran the CMakery this way:
>
> cmake -B `pwd`/all-in-one -G Ninja -C `pwd`/llvm-project/lldb/cmake/caches/Apple-lldb-base.cmake -DLLVM_ENABLE_PROJECTS="clang;lldb" -DLLVM_ENABLE_RUNTIMES="libcxx;libcxxabi;compiler-rt;libunwind" -DLIBCXX_INCLUDE_TESTS=OFF -DCMAKE_BUILD_TYPE=Debug -DCMAKE_OSX_DEPLOYMENT_TARGET:STRING=12.3 -DLLDB_BUILD_FRAMEWORK=YES -DCMAKE_CXX_STANDARD:STRING=17 llvm-project/llvm
>
> Then:
>
> $ cd all-in-one
> $ ninja check-lldb
>
> and got the failure.
>
> Jim
>
>
>> @ZequanWu <https://github.com/ZequanWu> in the future, if one of your commits break a bot, make sure to revert it immediately, you can always re-land it later with a fix or an explanation why it wasn't your commit that broke the bots. Reverting a commit is cheap, red bots are expensive :-)
>>
>> Will do.
>>
>> —
>> Reply to this email directly, view it on GitHub <#90663 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW2VA2CFK2NIEANWQG3ZCFPJJAVCNFSM6AAAAABHA7HMYWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMBZGA4DCMZSGY>.
>> You are receiving this because you are on a team that was mentioned.
>>
>
>
|
This change adds the new test, so deleting it as part of the reverting is expected. |
|
Reverting those two commits seems to have caused this build failure on Ubuntu:
Step 4 (build) warnings: build (warnings)
../llvm-project/clang/lib/Lex/PPDirectives.cpp:548:28: warning: comparison of integers of different signs: 'unsigned int' and 'int' [-Wsign-compare]
1 warning generated.
../llvm-project/clang/lib/Frontend/TextDiagnostic.cpp:148:36: warning: comparison of integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
1 warning generated.
../llvm-project/llvm/lib/Target/AMDGPU/AMDGPUIGroupLP.cpp:1079:20: warning: comparison of integers of different signs: 'typename iterator_traits<const SDep *>::difference_type' (aka 'int') and 'unsigned int' [-Wsign-compare]
../llvm-project/llvm/lib/Target/AMDGPU/AMDGPUIGroupLP.cpp:1089:24: warning: comparison of integers of different signs: 'typename iterator_traits<SDep *>::difference_type' (aka 'int') and 'unsigned int' [-Wsign-compare]
../llvm-project/llvm/lib/Target/AMDGPU/AMDGPUIGroupLP.cpp:1120:20: warning: comparison of integers of different signs: 'typename iterator_traits<const SDep *>::difference_type' (aka 'int') and 'unsigned int' [-Wsign-compare]
../llvm-project/llvm/lib/Target/AMDGPU/AMDGPUIGroupLP.cpp:1130:24: warning: comparison of integers of different signs: 'typename iterator_traits<SDep *>::difference_type' (aka 'int') and 'unsigned int' [-Wsign-compare]
../llvm-project/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp:1107:7: warning: implicit conversion from 'long long' to 'const std::time_t' (aka 'const long') changes value from -1096193779200 to -977118720 [-Wconstant-conversion]
../llvm-project/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp:1109:7: warning: implicit conversion from 'long long' to 'const std::time_t' (aka 'const long') changes value from 971890963199 to 1228354303 [-Wconstant-conversion]
../llvm-project/lld/MachO/SyntheticSections.cpp:2063:25: warning: comparison of integers of different signs: 'int' and 'const uint32_t' (aka 'const unsigned int') [-Wsign-compare]
1 warning generated.
Step 6 (test) failure: build (failure)
clang: warning: argument unused during compilation: '-fmodules-cache-path=/home/tcwg-buildbot/worker/lldb-arm-ubuntu/build/lldb-test-build.noindex/module-cache-clang/lldb-shell' [-Wunused-command-line-argument]
Would you mind taking a look, this is not my area of llvm...
https://lab.llvm.org/buildbot/#/builders/17/builds/53025
Thanks!
Jim
… On May 13, 2024, at 6:22 PM, Zequan Wu ***@***.***> wrote:
your commit deleted that file I think, I added it back when I did the revert (possibly a mistake)... It passes on my macOS system but is failing on Ubuntu after the revert. I think I'll just disable it for now.
This change adds the new test, so deleting it as part of the reverting is expected.
—
Reply to this email directly, view it on GitHub <#90663 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW76LSTVQBSKCNZZU2TZCFROLAVCNFSM6AAAAABHA7HMYWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMBZGA4TKNZYHA>.
You are receiving this because you are on a team that was mentioned.
|
You forgot to delete the newly added test SymbolFile/DWARF/delayed-definition-die-searching.test in the reverting: 37b8e5f |
|
Can you take care of cleaning this up, this seems like a slightly complex patch and not in an area I'm familiar with.
Thanks!
Jim
… On May 13, 2024, at 6:35 PM, Zequan Wu ***@***.***> wrote:
Reverting those two commits seems to have caused this build failure on Ubuntu:
You forgot to delete the newly added test SymbolFile/DWARF/delayed-definition-die-searching.test in the reverting: 37b8e5f <37b8e5f>
—
Reply to this email directly, view it on GitHub <#90663 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW2JLCLVQIGYHQNLA3LZCFS5XAVCNFSM6AAAAABHA7HMYWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMBZGEYTCNJSGI>.
You are receiving this because you are on a team that was mentioned.
|
Yes, will do. Sorry for the mess without reverting it earlier. |
|
Thanks!
Jim
… On May 13, 2024, at 6:39 PM, Zequan Wu ***@***.***> wrote:
Can you take care of cleaning this up, this seems like a slightly complex patch and not in an area I'm familiar with.
Yes, will do. Sorry for the mess without reverting it earlier.
—
Reply to this email directly, view it on GitHub <#90663 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVWZEVS6JU6RU5C7JCELZCFTNXAVCNFSM6AAAAABHA7HMYWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMBZGEYTOMBRG4>.
You are receiving this because you are on a team that was mentioned.
|
The bot log should have the cmake line and all the commands that were run there. |
|
I had a fix to this: Let It fixes those failed tests shown on the macos bot. However, I noticed that lldb crashes on three tests related to clang module (they also crashes when the fix is not given, but not crash after reverting this PR): I found it's the following commands causing crash. The clang module in Update: |
|
It actually still crashes at the same place even without this PR using the following command, you can try it on trunk: But I don't know why this change will make this crash explicitly shows up when running check-lldb. |
…ng when parsing declaration DIEs. (llvm#90663)" This reverts commit 9a7262c.
…ing when parsing declaration DIEs. (#92328) This reapplies 9a7262c (#90663) and added #91808 as a fix. It was causing tests on macos to fail because `SymbolFileDWARF::GetForwardDeclCompilerTypeToDIE` returned the map owned by this symol file. When there were two symbol files, two different maps were created for caching from compiler type to DIE even if they are for the same module. The solution is to do the same as `SymbolFileDWARF::GetUniqueDWARFASTTypeMap`: inquery SymbolFileDWARFDebugMap first to get the shared underlying SymbolFile so the map is shared among multiple SymbolFileDWARF.
…ing when parsing declaration DIEs. (llvm#92328) This reapplies llvm@9a7262c (llvm#90663) and added llvm#91808 as a fix. It was causing tests on macos to fail because `SymbolFileDWARF::GetForwardDeclCompilerTypeToDIE` returned the map owned by this symol file. When there were two symbol files, two different maps were created for caching from compiler type to DIE even if they are for the same module. The solution is to do the same as `SymbolFileDWARF::GetUniqueDWARFASTTypeMap`: inquery SymbolFileDWARFDebugMap first to get the shared underlying SymbolFile so the map is shared among multiple SymbolFileDWARF.
If ParseStructureLikeDIE (or ParseEnum) encountered a declaration DIE, it would call FindDefinitionTypeForDIE. This returned a fully formed type, which it achieved by recursing back into ParseStructureLikeDIE with the definition DIE. This obscured the control flow and caused us to repeat some work (e.g. the UniqueDWARFASTTypeMap lookup), but it mostly worked until we tried to delay the definition search in llvm#90663. After this patch, the two ParseStructureLikeDIE calls were no longer recursive, but rather the second call happened as a part of the CompleteType() call. This opened the door to inconsistencies, as the second ParseStructureLikeDIE call was not aware it was called to process a definition die for an existing type. To make that possible, this patch removes the recusive type resolution from this function, and leaves just the "find definition die" functionality. After finding the definition DIE, we just go back to the original ParseStructureLikeDIE call, and have it finish the parsing process with the new DIE. While this patch is motivated by the work on delaying the definition searching, I believe it is also useful on its own.
…IEs (#96484) If ParseStructureLikeDIE (or ParseEnum) encountered a declaration DIE, it would call FindDefinitionTypeForDIE. This returned a fully formed type, which it achieved by recursing back into ParseStructureLikeDIE with the definition DIE. This obscured the control flow and caused us to repeat some work (e.g. the UniqueDWARFASTTypeMap lookup), but it mostly worked until we tried to delay the definition search in #90663. After this patch, the two ParseStructureLikeDIE calls were no longer recursive, but rather the second call happened as a part of the CompleteType() call. This opened the door to inconsistencies, as the second ParseStructureLikeDIE call was not aware it was called to process a definition die for an existing type. To make that possible, this patch removes the recusive type resolution from this function, and leaves just the "find definition die" functionality. After finding the definition DIE, we just go back to the original ParseStructureLikeDIE call, and have it finish the parsing process with the new DIE. While this patch is motivated by the work on delaying the definition searching, I believe it is also useful on its own.
This is the implementation for https://discourse.llvm.org/t/rfc-delay-definition-die-searching-when-parse-a-declaration-die-for-record-type/78526.
Motivation
Currently, lldb eagerly searches for definition DIE when parsing a declaration DIE for struct/class/union definition DIE. It will search for all definition DIEs with the same unqualified name (just
DW_AT_name) and then find out those DIEs with same fully qualified name. Then lldb will try to resolve those DIEs to create the Types from definition DIEs. It works fine most time. However, when built with-gsimple-template-names, the search graph expands very quickly, because for the specialized-template classes, they don’t have template parameter names encoded insideDW_AT_name. They haveDW_TAG_template_type_parameterto reference the types used as template parameters. In order to identify if a definition DIE matches a declaration DIE, lldb needs to resolve all template parameter types first and those template parameter types might be template classes as well, and so on… So, the search graph explodes, causing a lot unnecessary searching/type-resolving to just get the fully qualified names for a specialized-template class. This causes lldb stack overflow for us internally on template-heavy libraries.Implementation
Instead of searching for definition DIEs when parsing declaration DIEs, we always construct the record type from the DIE regardless if it's definition or declaration. The process of searching for definition DIE is refactored to
DWARFASTParserClang::FindDefinitionTypeForDIEwhich is invoked when 1) completing the type onSymbolFileDWARF::CompleteType. 2) the record type needs to start its definition as a containing type so that nested classes can be added into it inPrepareContextToReceiveMembers.The key difference is
SymbolFileDWARF::ResolveTypereturn aType*that might be created from declaration DIE, which means it hasn't starts its definition yet. We also need to change according in places where we want the type to start definition, likePrepareContextToReceiveMembers(I'm not aware of any other places, but this should be a simple call toSymbolFileDWARF::FindDefinitionDIE)Result
It fixes the stack overflow of lldb for the internal binary built with simple template name. When constructing the fully qualified name built with
-gsimple-template-names, it gets the name of the type parameter by resolving the referenced DIE, which might be a declaration (we won't try to search for the definition DIE to just get the name).I got rough measurement about the time using the same commands (set breakpoint, run, expr this, exit). For the binary built without
-gsimple-template-names, this change has no impact on time, still taking 41 seconds to complete. When built with-gsimple-template-names, it also takes about 41 seconds to complete wit this change.