Reapply [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. #92328
Conversation
…ng when parsing declaration DIEs. This reapplies 9a7262c and a7eff59 with 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.
…get the shared underlying SymbolFile when calling GetForwardDeclCompilerTypeToDIE
|
The second commit is the fix. |
|
@llvm/pr-subscribers-lldb Author: Zequan Wu (ZequanWu) ChangesThis reapplies 9a7262c and a7eff59 with a fix. It was causing tests on macos to fail because Patch is 60.92 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/92328.diff 11 Files Affected:
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.h b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.h
index 66db396279e06..e144cf0f9bd94 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.h
@@ -60,6 +60,8 @@ class DWARFASTParser {
virtual ConstString GetDIEClassTemplateParams(const DWARFDIE &die) = 0;
+ virtual lldb_private::Type *FindDefinitionTypeForDIE(const DWARFDIE &die) = 0;
+
static std::optional<SymbolFile::ArrayInfo>
ParseChildArrayInfo(const DWARFDIE &parent_die,
const ExecutionContext *exe_ctx = nullptr);
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index f8101aba5c627..2a46be9216121 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -154,6 +154,26 @@ static bool TagIsRecordType(dw_tag_t tag) {
}
}
+static bool IsForwardDeclaration(const DWARFDIE &die,
+ const ParsedDWARFTypeAttributes &attrs,
+ LanguageType cu_language) {
+ if (attrs.is_forward_declaration)
+ return true;
+
+ // Work around an issue with clang at the moment where forward
+ // declarations for objective C classes are emitted as:
+ // DW_TAG_structure_type [2]
+ // DW_AT_name( "ForwardObjcClass" )
+ // DW_AT_byte_size( 0x00 )
+ // DW_AT_decl_file( "..." )
+ // DW_AT_decl_line( 1 )
+ //
+ // Note that there is no DW_AT_declaration and there are no children,
+ // and the byte size is zero.
+ return attrs.byte_size && *attrs.byte_size == 0 && attrs.name &&
+ !die.HasChildren() && cu_language == eLanguageTypeObjC;
+}
+
TypeSP DWARFASTParserClang::ParseTypeFromClangModule(const SymbolContext &sc,
const DWARFDIE &die,
Log *log) {
@@ -249,11 +269,9 @@ static void ForcefullyCompleteType(CompilerType type) {
/// This function serves a similar purpose as RequireCompleteType above, but it
/// avoids completing the type if it is not immediately necessary. It only
/// ensures we _can_ complete the type later.
-static void PrepareContextToReceiveMembers(TypeSystemClang &ast,
- ClangASTImporter &ast_importer,
- clang::DeclContext *decl_ctx,
- DWARFDIE die,
- const char *type_name_cstr) {
+void DWARFASTParserClang::PrepareContextToReceiveMembers(
+ clang::DeclContext *decl_ctx, 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)
return; // Non-tag context are always ready.
@@ -268,7 +286,8 @@ static void PrepareContextToReceiveMembers(TypeSystemClang &ast,
// gmodules case), we can complete the type by doing a full import.
// If this type was not imported from an external AST, there's nothing to do.
- CompilerType type = ast.GetTypeForDecl(tag_decl_ctx);
+ CompilerType type = m_ast.GetTypeForDecl(tag_decl_ctx);
+ ClangASTImporter &ast_importer = GetClangASTImporter();
if (type && ast_importer.CanImport(type)) {
auto qual_type = ClangUtil::GetQualType(type);
if (ast_importer.RequireCompleteType(qual_type))
@@ -279,6 +298,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.
+ FindDefinitionTypeForDIE(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 +646,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(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
@@ -1103,32 +1130,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
@@ -1263,6 +1264,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();
+ }
+ }
+ }
}
}
}
@@ -1635,6 +1669,93 @@ DWARFASTParserClang::GetCPlusPlusQualifiedName(const DWARFDIE &die) {
return qualified_name;
}
+lldb_private::Type *
+DWARFASTParserClang::FindDefinitionTypeForDIE(const DWARFDIE &die) {
+ SymbolFileDWARF *dwarf = die.GetDWARF();
+ ParsedDWARFTypeAttributes attrs(die);
+ bool is_forward_declaration = IsForwardDeclaration(
+ die, attrs, SymbolFileDWARF::GetLanguage(*die.GetCU()));
+ if (!is_forward_declaration)
+ return dwarf->GetDIEToType()[die.GetDIE()];
+
+ const dw_tag_t tag = die.Tag();
+ TypeSP type_sp;
+ Log *log = GetLog(DWARFLog::TypeCompletion | DWARFLog::Lookups);
+ if (log) {
+ dwarf->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()
+ // map will have the new mapping from this declaration die to definition die.
+ 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 && log) {
+ dwarf->GetObjectFile()->GetModule()->LogMessage(
+ log,
+ "SymbolFileDWARF({0:p}) - {1:x16}: {2} ({3}) type \"{4}\" is an "
+ "incomplete objc type, complete type is {5:x8}",
+ static_cast<void *>(this), die.GetOffset(),
+ DW_TAG_value_to_name(tag), tag, attrs.name.GetCString(),
+ type_sp->GetID());
+ }
+ }
+ }
+
+ 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 && 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());
+ }
+ }
+
+ if (!type_sp && log) {
+ dwarf->GetObjectFile()->GetModule()->LogMessage(
+ log,
+ "SymbolFileDWARF({0:p}) - {1:x16}: {2} type \"{3}\" is a "
+ "forward declaration, unable to find definition DIE for it",
+ static_cast<void *>(this), die.GetOffset(), DW_TAG_value_to_name(tag),
+ attrs.name.GetCString());
+ }
+ return type_sp.get();
+}
+
TypeSP
DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
const DWARFDIE &die,
@@ -1646,14 +1767,10 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
LanguageType cu_language = SymbolFileDWARF::GetLanguage(*die.GetCU());
Log *log = GetLog(DWARFLog::TypeCompletion | DWARFLog::Lookups);
- // UniqueDWARFASTType is large, so don't create a local variables on the
- // stack, put it on the heap. This function is often called recursively and
- // clang isn't good at sharing the stack space for variables in different
- // blocks.
- auto unique_ast_entry_up = std::make_unique<UniqueDWARFASTType>();
-
ConstString unique_typename(attrs.name);
Declaration unique_decl(attrs.decl);
+ uint64_t byte_size = attrs.byte_size.value_or(0);
+ attrs.is_forward_declaration = IsForwardDeclaration(die, attrs, cu_language);
if (attrs.name) {
if (Language::LanguageIsCPlusPlus(cu_language)) {
@@ -1666,14 +1783,42 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
unique_decl.Clear();
}
- if (dwarf->GetUniqueDWARFASTTypeMap().Find(
- unique_typename, die, unique_decl, attrs.byte_size.value_or(-1),
- *unique_ast_entry_up)) {
- type_sp = unique_ast_entry_up->m_type_sp;
+ if (UniqueDWARFASTType *unique_ast_entry_type =
+ dwarf->GetUniqueDWARFASTTypeMap().Find(
+ unique_typename, die, unique_decl, byte_size,
+ attrs.is_forward_declaration)) {
+ type_sp = unique_ast_entry_type->m_type_sp;
if (type_sp) {
dwarf->GetDIEToType()[die.GetDIE()] = type_sp.get();
LinkDeclContextToDIE(
- GetCachedClangDeclContextForDIE(unique_ast_entry_up->m_die), die);
+ GetCachedClangDeclContextForDIE(unique_ast_entry_type->m_die), die);
+ if (!attrs.is_forward_declaration) {
+ // If the DIE being parsed in this function is a definition and the
+ // entry in the map is a declaration, then we need to update the entry
+ // to point to the definition DIE.
+ if (unique_ast_entry_type->m_is_forward_declaration) {
+ unique_ast_entry_type->m_die = die;
+ unique_ast_entry_type->m_byte_size = byte_size;
+ unique_ast_entry_type->m_declaration = unique_decl;
+ unique_ast_entry_type->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());
+ clang_type = type_sp->GetForwardCompilerType();
+ if (attrs.class_language != eLanguageTypeObjC &&
+ attrs.class_language != eLanguageTypeObjC_plus_plus)
+ TypeSystemClang::StartTagDeclarationDefinition(clang_type);
+
+ CompilerType compiler_type_no_qualifiers =
+ ClangUtil::RemoveFastQualifiers(clang_type);
+ auto result = dwarf->GetForwardDeclCompilerTypeToDIE().try_emplace(
+ compiler_type_no_qualifiers.GetOpaqueQualType(),
+ *die.GetDIERef());
+ if (!result.second)
+ result.first->second = *die.GetDIERef();
+ }
+ }
return type_sp;
}
}
@@ -1695,125 +1840,21 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
default_accessibility = eAccessPrivate;
}
- if (attrs.byte_size && *attrs.byte_size == 0 && attrs.name &&
- !die.HasChildren() && cu_language == eLanguageTypeObjC) {
- // Work around an issue with clang at the moment where forward
- // declarations for objective C classes are emitted as:
- // DW_TAG_structure_type [2]
- // DW_AT_name( "ForwardObjcClass" )
- // DW_AT_byte_size( 0x00 )
- // DW_AT_decl_file( "..." )
- // DW_AT_decl_line( 1 )
- //
- // Note that there is no DW_AT_declaration and there are no children,
- // and the byte size is zero.
- 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} ({3}) type \"{4}\" is an "
- "incomplete objc type, complete type is {5:x8}",
- static_cast<void *>(this), die.GetOffset(),
- DW_TAG_value_to_name(tag), 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} ({3}) type \"{4}\" is a "
- "forward declaration, trying to find complete type",
- static_cast<void *>(this), die.GetOffset(), DW_TAG_value_to_name(tag),
- 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} ({3}) type \"{4}\" is a "
- "forward declaration, complete type is {5:x8}",
- static_cast<void *>(this), die.GetOffset(),
- DW_TAG_value_to_name(tag), 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...
[truncated]
|
| @@ -2306,6 +2345,11 @@ bool DWARFASTParserClang::CompleteTypeFromDWARF(const DWARFDIE &die, | |||
|
|
|||
| if (!die) | |||
| return false; | |||
| ParsedDWARFTypeAttributes attrs(die); | |||
There was a problem hiding this comment.
I've been wondering how expensive constructing this object is. On a brief glance it seems to be doing a lot of work, but maybe all of that work is actually not that slow? Do you know why we get here for forward declarations with your patch, but didn't before?
There was a problem hiding this comment.
This extra check was added in #91799 to ensure we don't accidentally parse declaration DIE, which was reported at #90663 (comment).
By checking ParsedDWARFTypeAttributes's constructor, looks like it just parses the DIE's attributes, iterates through them, and updates its fields accordingly.
There was a problem hiding this comment.
The parsing happens every time when constructing this object, which makes it a bit expensive, should we add a new field DWARFAttributes m_attributes in DWARFBaseDIE, so that we only parse it once? From a glance at calls to DWARFBaseDIE::GetAttributes, there are more than 10 calls to it. The attribute parsing is repetitive.
Probably not, the DWARFAttributes object is large, 336 bytes.
There was a problem hiding this comment.
Probably not, the
DWARFAttributesobject is large, 336 bytes.
I think that would be very wasteful, since we're just accessing these attributes once (well, maybe twice). After we've constructed the type, we should never need to look at them again, as all the information has been translated to the clang ast.
This extra check was added in #91799 to ensure we don't accidentally parse declaration DIE
How exactly do we get here in that case? Presumably we still go through ParseTypesFromDWARF (where this property is checked already), is that right? Could we make a note somewhere (perhaps by putting a fake/invalid DIE into the type-to-die map) that the die is not a definition?
There was a problem hiding this comment.
How exactly do we get here in that case?
From #90663 (comment), .debug_names somehow contains entries that are declarations. This causes SymbolFileDWARF::FindDefinitionTypeForDWARFDeclContext to return a type created from declaration when searching for definition.
A simple idea I have in mind is to make the GetForwardDeclCompilerTypeToDIE's value type to a pair {DIERef, bool}, and the bool indicate if this is a definition or not. So we know that info without extra attribute parsing. How do you think?
There was a problem hiding this comment.
How exactly do we get here in that case?
From #90663 (comment), .debug_names somehow contains entries that are declarations. This causes
SymbolFileDWARF::FindDefinitionTypeForDWARFDeclContextto return a type created from declaration when searching for definition.
I've re-read that. The debug names situation is troubling, but the thing I don't understand now why is this even specific to debug_names. After this patch, it should be fairly easy to trigger a situation where we're asked to complete a type, and we don't have the definition of that type. Why don't those cases lead to a crash? For example, what would happen if the name was simply not in the index?
If I ignored this entire discussion, I would say that this check makes perfect sense: since we're delaying the search for the definition, it can happen that the we don't have the definition die at the point when we're asked to complete the type. So it seems perfectly reasonable to have this check somewhere. I just want to check whether this is the right place.
A simple idea I have in mind is to make the
GetForwardDeclCompilerTypeToDIE's value type to a pair{DIERef, bool}, and the bool indicate if this is a definition or not. So we know that info without extra attribute parsing. How do you think?
Given that this extra bool is going to cause us to use eight more bytes per type, this doesn't necessarily seem like. This would use more memory, the previous implementation would use more time. Hard to say which one is better without some very precise measurements. Choosing blindly, I would stick with the current implementation, as it's easier to optimize the cpu time than it is to reclaim that memory (the real problem here is that ParsedDWARFTypeAttributes was optimized for the use case where one is going to use most of the attributes it has parsed (which is what happens in ParseTypeFromDWARF). Using it to essentially just check for the presence of DW_AT_declaration is wasteful, and you could write a much faster implementation if you were writing it for this use case specifically (but it's also not clear whether it's worthwhile to have a brand new implementation for this use case).
There was a problem hiding this comment.
Why don't those cases lead to a crash? For example, what would happen if the name was simply not in the index?
That is specific to debug_names because the index gives us a declaration DIE when we are searching for definition DIE in 'FindDefinitionTypeForDWARFDeclContext'. Before, we didn't have the extra check, so we tries to complete the type from declaration DIE, which triggers an assertion in clang. However, it doesn't happen in manual index because we already explicitly checked DW_AT_declaration attributes when creating the manual index. It's guaranteed to find a definition DIE from FindDefinitionTypeForDWARFDeclContext or nothing (early bailout, won't try to complete it).
So it seems perfectly reasonable to have this check somewhere. I just want to check whether this is the right place.
I assume Greg's change at #91808 will also fix this problem by skipping forward declaration DIE when processing it, which is earlier check than this extra check added here.
There was a problem hiding this comment.
Why don't those cases lead to a crash? For example, what would happen if the name was simply not in the index?
That is specific to debug_names because the index gives us a declaration DIE when we are searching for definition DIE in 'FindDefinitionTypeForDWARFDeclContext'. Before, we didn't have the extra check, so we tries to complete the type from declaration DIE, which triggers an assertion in clang. However, it doesn't happen in manual index because we already explicitly checked
DW_AT_declarationattributes when creating the manual index. It's guaranteed to find a definition DIE fromFindDefinitionTypeForDWARFDeclContextor nothing (early bailout, won't try to complete it).
Okay, so when I said "specific to debug_names", I was mainly thinking of the case where the index doesn't find anything (i.e., we attempt to complete a type and the index (any kind of index) says we can't, because it doesn't have the definition). Based on what you've said (and some more pondering), I think can now answer that question for myself (please let me know if this is wrong):
The reason why this crash does not happen when attempting to complete a type with no definition is because this function is the actual implementation of "completing a type from a defintion". I.e., it expects to be called with a definition DIE, and will never be called if there is no definition (we will bail out, and possibly "forcefully" complete the type somewhere higher up the stack).
If that is true, then I think Greg's patch is a better fix for this problem, and it also sidesteps all of the performance/memory tradeoff discussions.
There was a problem hiding this comment.
The reason why this crash does not happen when attempting to complete a type with no definition is because this function is the actual implementation of "completing a type from a defintion". I.e., it expects to be called with a definition DIE, and will never be called if there is no definition (we will bail out, and possibly "forcefully" complete the type somewhere higher up the stack).
Yes, it should only be called with definition DIE. This extra check just a double-check for it with a bit performance cost. If there's no definition DIE, SymbolFileDWARF::CompleteType does an early return. This behaviour is unchanged. Forceful completion behaviour is also remained unchanged, happens when failed to find definition DIE of a containing record type.
If that is true, then I think Greg's patch is a better fix for this problem, and it also sidesteps all of the performance/memory tradeoff discussions.
I agree. If we can ensure the DIE is always a definition DIE for both manual index and debug_names index at a earlier time, there's no need for this check. I'll remove this check when Greg's change is in.
| // A map from CompilerType to the struct/class/union/enum DIE (might be a | ||
| // declaration or a definition) that is used to construct it. | ||
| llvm::DenseMap<lldb::opaque_compiler_type_t, DIERef> | ||
| m_forward_decl_compiler_type_to_die; |
There was a problem hiding this comment.
Could you elaborate on why this wasn't necessary before? We're now mixing CompilerType's (which originate from different ASTContext's) in the same map, which might not be a problem (since we do this for UniqueDWARFASTTypeMap already, whose Types can also originate from different context's) but I would still like to understand it.
There was a problem hiding this comment.
TL;DR: This is because this change let UniqueDWARFASTTypeMap to cache created Type from declaration DIE. Before this, it was only used for caching Type created from definition DIE. And UniqueDWARFASTTypeMap is shared among all SymbolFileDWARFs belongs to one SymbolFileDWARFDebugMap, so should m_forward_decl_compiler_type_to_die which interacts with it.
Here's an example with debug map used:
The declaration DIE for bar is in foo.o and the definition DIE is in main.o. ParseStructureLikeDIE was firstly asked to parse the declaration DIE.
Before, it will always find the definition DIE in main.o and insert the CompilerType to definition DIE to m_forward_decl_compiler_type_to_die which belongs to SymbolFileDWARF(main.o). When TypeSystemClang::CompleteTagDecl wants to complete bar, it asks SymbolFileDWARFDebugMap::CompleteType to complete, which iterates all its SymbolFileDWARF(main.o, foo.o) and check if the any of them has the compiler type in their maps:
llvm-project/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
Lines 808 to 812 in 9144553
bar's compiler type exists in symbol file(main.o)'s map which also has the definition DIE as value, the type completion will success.
If I don't add the fix, we have [bar's compiler type -> bar's declaration DIE] in foo.o's map. When searching for definition DIE, we found that in main.o. Because we have already created its Type from declaration, the UniqueDWARFASTTypeMap will find the entry. Then it updates the entry to points to the definition DIE. It updates main.o's m_forward_decl_compiler_type_to_die to pointing to the definition DIE, which is wrong, since there's no such entry in main.o's map. It should update foo.o's map. The result is that SymbolFileDWARFDebugMap::CompleteType find bar's compiler type exists in foo.o and ask foo.o's symbol file to complete it, but it only has declaration DIE.
The fix is to share one m_forward_decl_compiler_type_to_die among one SymbolFileDWARFDebugMap. With this, when creating compiler type to declaration DIE in the map or updating an entry to point to a definition DIE, we are operating in the same map.
| @@ -2306,6 +2345,11 @@ bool DWARFASTParserClang::CompleteTypeFromDWARF(const DWARFDIE &die, | |||
|
|
|||
| if (!die) | |||
| return false; | |||
| ParsedDWARFTypeAttributes attrs(die); | |||
… here to do a earlier check
|
I removed the checking for DW_AT_declaration attributes when completing type from a DIE and applied #91808 to here to ensure we always have a definition DIE at that point. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
…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.
|
Unfortunately this breaks The stacktrace I got when locally reproducing this: Let me know if you need help reproducing this on macOS! |
|
Huh this is an interesting one. So it looks like we first parse and insert If i hack the DIE tags to align, all is well: It feels a bit off to have situations where we end up succeeding to |
|
For this specific case, we could fix it by making There's few things I noticed with this:
So, basically this PR relies In case of we failed to find existing clang type (created from declaration) in |
…efinition. (#93839) 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. --------- Co-authored-by: Michael Buch <michaelbuch12@gmail.com>
|
Sorry for the late ping, but the LLDB matrix bot failure has gone unnoticed for a bit. This patch caused following test to fail for DWARFv5: https://green.lab.llvm.org/job/llvm.org/view/LLDB/job/lldb-cmake-matrix/321/execution/node/79/log/ You can repro this by running Could you take a look @ZequanWu ? |
It should be fixed by the following diff. We just need to skip the declaration DIEs that are struct/class/union. This failure you see is caused by skipping a declaration DIE Can you (or anyone with commit access) commit above fix for me? I somehow cannot pull/push from github. |
Ah right, I remember encountering these |
It's fine to have those as nameless entries (which we don't emit today), but the entries that are reaching "process entry" have a name by definition. That if statement is a workaround for incorrect dwarf generation. It seems like this test is providing a case where incorrect dwarf is generated? |
Yea i misremembered, dsymutil is the one indexing these inline statics. @ZequanWu could you put up a PR for the proposed patch? |
|
(Zequan is OOO for three weeks, I can take over this (tomorrow) if needed.) |
Ah no worries! I can put up the patch later today then to unblock the CI |
At least we aren't producing that on x86 from the compiler: https://godbolt.org/z/ereKsasWf and the Ah, right - some of the previous context on this is #70639 (which got reverted in #74580 )- though I guess we probably had some discourse and dwarf workgroup discussions about how to do this that aren't likned from the PR... maybe it was all private chat, not sure. |
…ss/union types This is a follow-up of llvm#92328 (comment) Clang attaches `DW_AT_declaration` to static inline data members and `dsymutil` indexes these constants. Skipping these caused the expression evaluator to fail to find such constants when using DWARFv5. Fixes `TestConstStaticIntegralMember.py` on DWARFv5.
…ss/union types (#94400) This is a follow-up of #92328 (comment) Clang attaches `DW_AT_declaration` to static inline data members and `dsymutil` indexes these constants. Skipping these caused the expression evaluator to fail to find such constants when using DWARFv5. Fixes `TestConstStaticIntegralMember.py` on DWARFv5.
| @@ -2345,11 +2345,6 @@ bool DWARFASTParserClang::CompleteTypeFromDWARF(const DWARFDIE &die, | |||
|
|
|||
| if (!die) | |||
| return false; | |||
| ParsedDWARFTypeAttributes attrs(die); | |||
There was a problem hiding this comment.
In case anyone's wondering, Putting this check back in would prevent the crash from 7fdbc30, but it would not actually make the code correct -- lldb would now just say the class has no definition instead of crashing.
It's certainly stuff that @ZequanWu, @labath, and myself are looking into. We could do a better job of communicating that upstream though :) I think we're meeting next week to chat a bit more about it, and can perhaps formalize that into some tracking bugs, etc. |
|
Hi there. Nice to see interest in this patch. Zequan has been OOO for the past couple of weeks, so I've sort of taken this up in the mean time. The problem with the simplified template names is actually already fixed (by #95905), but in the mean time, I've discovered a very similar problem with type units (basically, just take the test case from #95905, but build it with -fdebug-types-section instead). While this could (and should) be fixed in similar way, this led me to believe that the overall approach in this patch was too fragile. When completing a type, it basically does something like:
The last step, besides being completely unnecessary (we already know the type we're supposed to complete), is also a big liability, because here we are implicitly relying on the the map to return the same type that we started with. If it does not then we will end up creating a new type instead of completing the existing one, and things will quickly go sideways. The meeting that David alluded to is tomorrow, and I hope we're going to try to figure out who's going to do what and in what order. In the mean time, I've added you as a reviewer to one of my pending patches. |
Right now, ParseStructureLikeDIE begins the class definition (which
amounts to parsing the opening "{" of a class and promising to be able
to fill it in later) if it finds a definition DIE.
This makes sense in the current setup, where we eagerly search for the
definition die (so that we will either find it in the beginning or don't
find it at all), but with delayed definition searching (llvm#92328), this
created an (in my view, undesirable) inconsistency, where the final
state of the type (whether it has begun its definition) depended on
whether we happened to start out with a definition DIE or not.
This patch attempts to pre-emptively rectify that by establishing a new
invariant: the definition is never started eagerly. It can only be
started in one of two ways:
- we're completing the type, in which case we will start the definition,
parse everything and immediately finish it
- we need to parse a member (typedef, nested class, method) of the class
without needing the definition itself. In this case, we just start the
definition to insert the member we need.
Besides the delayed definition search, I believe this setup has a couple
of other benefits:
- It treats ObjC and C++ classes the same way (we were never starting
the definition of those)
- unifies the handling of types that types that have a definition and
those that do. When adding (e.g.) a nested class we would previously
be going down a different code path depending on whether we've found a
definition DIE for that type. Now, we're always taking the
definition-not-found path (*)
- it reduces the amount of time a class spends in the funny "definition
started". Aside from the addition of stray addition of nested classes,
we always finish the definition right after we start it.
(*) Herein lies a danger, where if we're missing some calls to
PrepareContextToReceiveMembers, we could trigger a crash when
trying to add a member to the not-yet-started-to-be-defined classes.
However, this is something that could happen before as well (if we
did not have a definition for the class), and is something that
would be exacerbated by llvm#92328 (because it could happen even if we
the definition exists, but we haven't found it yet). This way, it
will at least happen consistently, and the fix should consist of
adding a PrepareContextToReceiveMembers in the appropriate place.
|
Thanks for the update! Not sure why but the github notification system seems to fail me recently which I did not get any update emails from this thread until proactively checking now. Sounds good! Let us know the final stack/PR for the delaying forward declaration is up, we are happy to perform early testing against internal targets. cc @clayborg, @kusmour |
…96755) Right now, ParseStructureLikeDIE begins the class definition (which amounts to parsing the opening "{" of a class and promising to be able to fill it in later) if it finds a definition DIE. This makes sense in the current setup, where we eagerly search for the definition die (so that we will either find it in the beginning or don't find it at all), but with delayed definition searching (#92328), this created an (in my view, undesirable) inconsistency, where the final state of the type (whether it has begun its definition) depended on whether we happened to start out with a definition DIE or not. This patch attempts to pre-emptively rectify that by establishing a new invariant: the definition is never started eagerly. It can only be started in one of two ways: - we're completing the type, in which case we will start the definition, parse everything and immediately finish it - we need to parse a member (typedef, nested class, method) of the class without needing the definition itself. In this case, we just start the definition to insert the member we need. Besides the delayed definition search, I believe this setup has a couple of other benefits: - It treats ObjC and C++ classes the same way (we were never starting the definition of those) - unifies the handling of types that types that have a definition and those that do. When adding (e.g.) a nested class we would previously be going down a different code path depending on whether we've found a definition DIE for that type. Now, we're always taking the definition-not-found path (*) - it reduces the amount of time a class spends in the funny "definition started". Aside from the addition of stray addition of nested classes, we always finish the definition right after we start it. (*) Herein lies a danger, where if we're missing some calls to PrepareContextToReceiveMembers, we could trigger a crash when trying to add a member to the not-yet-started-to-be-defined classes. However, this is something that could happen before as well (if we did not have a definition for the class), and is something that would be exacerbated by #92328 (because it could happen even if we the definition exists, but we haven't found it yet). This way, it will at least happen consistently, and the fix should consist of adding a PrepareContextToReceiveMembers in the appropriate place.
…lvm#96755) Right now, ParseStructureLikeDIE begins the class definition (which amounts to parsing the opening "{" of a class and promising to be able to fill it in later) if it finds a definition DIE. This makes sense in the current setup, where we eagerly search for the definition die (so that we will either find it in the beginning or don't find it at all), but with delayed definition searching (llvm#92328), this created an (in my view, undesirable) inconsistency, where the final state of the type (whether it has begun its definition) depended on whether we happened to start out with a definition DIE or not. This patch attempts to pre-emptively rectify that by establishing a new invariant: the definition is never started eagerly. It can only be started in one of two ways: - we're completing the type, in which case we will start the definition, parse everything and immediately finish it - we need to parse a member (typedef, nested class, method) of the class without needing the definition itself. In this case, we just start the definition to insert the member we need. Besides the delayed definition search, I believe this setup has a couple of other benefits: - It treats ObjC and C++ classes the same way (we were never starting the definition of those) - unifies the handling of types that types that have a definition and those that do. When adding (e.g.) a nested class we would previously be going down a different code path depending on whether we've found a definition DIE for that type. Now, we're always taking the definition-not-found path (*) - it reduces the amount of time a class spends in the funny "definition started". Aside from the addition of stray addition of nested classes, we always finish the definition right after we start it. (*) Herein lies a danger, where if we're missing some calls to PrepareContextToReceiveMembers, we could trigger a crash when trying to add a member to the not-yet-started-to-be-defined classes. However, this is something that could happen before as well (if we did not have a definition for the class), and is something that would be exacerbated by llvm#92328 (because it could happen even if we the definition exists, but we haven't found it yet). This way, it will at least happen consistently, and the fix should consist of adding a PrepareContextToReceiveMembers in the appropriate place.
…ng when parsing declaration DIEs. (llvm#98361) This is a reapply of llvm#92328 and llvm#93839. It now passes the [test](llvm@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
This reapplies 9a7262c (#90663) and added #91808 as a fix.
It was causing tests on macos to fail because
SymbolFileDWARF::GetForwardDeclCompilerTypeToDIEreturned 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 asSymbolFileDWARF::GetUniqueDWARFASTTypeMap: inquery SymbolFileDWARFDebugMap first to get the shared underlying SymbolFile so the map is shared among multiple SymbolFileDWARF.