[lldb/DWARF] Remove parsing recursion when searching for definition DIEs #96484
Conversation
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.
|
@llvm/pr-subscribers-lldb Author: Pavel Labath (labath) ChangesIf 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. Patch is 32.26 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/96484.diff 7 Files Affected:
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index 52f4d765cbbd4..ad58e0cbb5a59 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -39,10 +39,12 @@
#include "lldb/Utility/StreamString.h"
#include "clang/AST/CXXInheritance.h"
+#include "clang/AST/DeclBase.h"
#include "clang/AST/DeclCXX.h"
#include "clang/AST/DeclObjC.h"
#include "clang/AST/DeclTemplate.h"
#include "clang/AST/Type.h"
+#include "llvm/ADT/StringExtras.h"
#include "llvm/Demangle/Demangle.h"
#include <map>
@@ -835,54 +837,50 @@ DWARFASTParserClang::GetDIEClassTemplateParams(const DWARFDIE &die) {
}
TypeSP DWARFASTParserClang::ParseEnum(const SymbolContext &sc,
- const DWARFDIE &die,
+ const DWARFDIE &decl_die,
ParsedDWARFTypeAttributes &attrs) {
Log *log = GetLog(DWARFLog::TypeCompletion | DWARFLog::Lookups);
- SymbolFileDWARF *dwarf = die.GetDWARF();
- const dw_tag_t tag = die.Tag();
- TypeSP type_sp;
+ SymbolFileDWARF *dwarf = decl_die.GetDWARF();
+ const dw_tag_t tag = decl_die.Tag();
+ DWARFDIE def_die;
if (attrs.is_forward_declaration) {
- type_sp = ParseTypeFromClangModule(sc, die, log);
- if (type_sp)
+ if (TypeSP type_sp = ParseTypeFromClangModule(sc, decl_die, log))
return type_sp;
- type_sp = dwarf->FindDefinitionTypeForDWARFDeclContext(die);
+ def_die = dwarf->FindDefinitionDIE(decl_die);
- if (!type_sp) {
+ if (!def_die) {
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);
+ def_die = debug_map_symfile->FindDefinitionDIE(decl_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 must link its
- // DeclContext to this die.
- if (clang::DeclContext *defn_decl_ctx =
- GetCachedClangDeclContextForDIE(dwarf->GetDIE(type_sp->GetID())))
- LinkDeclContextToDIE(defn_decl_ctx, die);
- return type_sp;
+ if (log) {
+ dwarf->GetObjectFile()->GetModule()->LogMessage(
+ log,
+ "SymbolFileDWARF({0:p}) - {1:x16}}: {2} ({3}) type \"{4}\" is a "
+ "forward declaration, complete DIE is {5}",
+ static_cast<void *>(this), decl_die.GetID(), DW_TAG_value_to_name(tag),
+ tag, attrs.name.GetCString(),
+ def_die ? llvm::utohexstr(def_die.GetID()) : "not found");
}
}
- DEBUG_PRINTF("0x%8.8" PRIx64 ": %s (\"%s\")\n", die.GetID(),
- DW_TAG_value_to_name(tag), type_name_cstr);
+ if (def_die) {
+ attrs = ParsedDWARFTypeAttributes(def_die);
+ } else {
+ // No definition found. Proceed with the declaration die. We can use it to
+ // create a forward-declared type.
+ def_die = decl_die;
+ }
CompilerType enumerator_clang_type;
if (attrs.type.IsValid()) {
- Type *enumerator_type = dwarf->ResolveTypeUID(attrs.type.Reference(), true);
+ Type *enumerator_type =
+ dwarf->ResolveTypeUID(attrs.type.Reference(), true);
if (enumerator_type)
enumerator_clang_type = enumerator_type->GetFullCompilerType();
}
@@ -897,24 +895,28 @@ TypeSP DWARFASTParserClang::ParseEnum(const SymbolContext &sc,
}
CompilerType clang_type = m_ast.CreateEnumerationType(
- attrs.name.GetStringRef(), GetClangDeclContextContainingDIE(die, nullptr),
- GetOwningClangModule(die), attrs.decl, enumerator_clang_type,
+ attrs.name.GetStringRef(), GetClangDeclContextContainingDIE(def_die, nullptr),
+ GetOwningClangModule(def_die), attrs.decl, enumerator_clang_type,
attrs.is_scoped_enum);
- LinkDeclContextToDIE(TypeSystemClang::GetDeclContextForType(clang_type), die);
+ clang::DeclContext *type_decl_ctx =
+ TypeSystemClang::GetDeclContextForType(clang_type);
+ LinkDeclContextToDIE(type_decl_ctx, def_die);
+ if (decl_die != def_die)
+ LinkDeclContextToDIE(type_decl_ctx, decl_die);
- type_sp =
- dwarf->MakeType(die.GetID(), attrs.name, attrs.byte_size, nullptr,
+ TypeSP type_sp =
+ dwarf->MakeType(def_die.GetID(), attrs.name, attrs.byte_size, nullptr,
attrs.type.Reference().GetID(), Type::eEncodingIsUID,
&attrs.decl, clang_type, Type::ResolveState::Forward,
- TypePayloadClang(GetOwningClangModule(die)));
+ TypePayloadClang(GetOwningClangModule(def_die)));
if (TypeSystemClang::StartTagDeclarationDefinition(clang_type)) {
- if (die.HasChildren()) {
+ if (def_die.HasChildren()) {
bool is_signed = false;
enumerator_clang_type.IsIntegerType(is_signed);
ParseChildEnumerators(clang_type, is_signed,
- type_sp->GetByteSize(nullptr).value_or(0), die);
+ type_sp->GetByteSize(nullptr).value_or(0), def_die);
}
TypeSystemClang::CompleteTagDeclarationDefinition(clang_type);
} else {
@@ -922,7 +924,7 @@ TypeSP DWARFASTParserClang::ParseEnum(const SymbolContext &sc,
"DWARF DIE at {0:x16} named \"{1}\" was not able to start its "
"definition.\nPlease file a bug and attach the file at the "
"start of this error message",
- die.GetOffset(), attrs.name.GetCString());
+ def_die.GetOffset(), attrs.name.GetCString());
}
return type_sp;
}
@@ -1635,13 +1637,12 @@ DWARFASTParserClang::GetCPlusPlusQualifiedName(const DWARFDIE &die) {
TypeSP
DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
- const DWARFDIE &die,
+ const DWARFDIE &decl_die,
ParsedDWARFTypeAttributes &attrs) {
- TypeSP type_sp;
CompilerType clang_type;
- const dw_tag_t tag = die.Tag();
- SymbolFileDWARF *dwarf = die.GetDWARF();
- LanguageType cu_language = SymbolFileDWARF::GetLanguage(*die.GetCU());
+ const dw_tag_t tag = decl_die.Tag();
+ SymbolFileDWARF *dwarf = decl_die.GetDWARF();
+ LanguageType cu_language = SymbolFileDWARF::GetLanguage(*decl_die.GetCU());
Log *log = GetLog(DWARFLog::TypeCompletion | DWARFLog::Lookups);
// UniqueDWARFASTType is large, so don't create a local variables on the
@@ -1658,19 +1659,19 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
// For C++, we rely solely upon the one definition rule that says
// only one thing can exist at a given decl context. We ignore the
// file and line that things are declared on.
- std::string qualified_name = GetCPlusPlusQualifiedName(die);
+ std::string qualified_name = GetCPlusPlusQualifiedName(decl_die);
if (!qualified_name.empty())
unique_typename = ConstString(qualified_name);
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 (type_sp) {
+ unique_typename, decl_die, unique_decl,
+ attrs.byte_size.value_or(-1), *unique_ast_entry_up)) {
+ if (TypeSP type_sp = unique_ast_entry_up->m_type_sp) {
LinkDeclContextToDIE(
- GetCachedClangDeclContextForDIE(unique_ast_entry_up->m_die), die);
+ GetCachedClangDeclContextForDIE(unique_ast_entry_up->m_die),
+ decl_die);
return type_sp;
}
}
@@ -1693,7 +1694,7 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
}
if (attrs.byte_size && *attrs.byte_size == 0 && attrs.name &&
- !die.HasChildren() && cu_language == eLanguageTypeObjC) {
+ !decl_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]
@@ -1710,14 +1711,14 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
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()) {
+ decl_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);
+ TypeSP type_sp =
+ dwarf->FindCompleteObjCDefinitionTypeForDIE(decl_die, attrs.name, true);
if (!type_sp) {
SymbolFileDWARFDebugMap *debug_map_symfile =
@@ -1726,7 +1727,7 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
// 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);
+ decl_die, attrs.name, true);
}
}
@@ -1736,7 +1737,7 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
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(),
+ static_cast<void *>(this), decl_die.GetOffset(),
DW_TAG_value_to_name(tag), tag, attrs.name.GetCString(),
type_sp->GetID());
}
@@ -1745,6 +1746,7 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
}
}
+ DWARFDIE def_die;
if (attrs.is_forward_declaration) {
Progress progress(llvm::formatv(
"Parsing type in {0}: '{1}'",
@@ -1761,81 +1763,80 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
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());
+ static_cast<void *>(this), decl_die.GetID(),
+ 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)
+ if (TypeSP type_sp = ParseTypeFromClangModule(sc, decl_die, log))
return type_sp;
- // type_sp = FindDefinitionTypeForDIE (dwarf_cu, die,
- // type_name_const_str);
- type_sp = dwarf->FindDefinitionTypeForDWARFDeclContext(die);
+ def_die = dwarf->FindDefinitionDIE(decl_die);
- if (!type_sp) {
+ if (!def_die) {
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);
+ def_die = debug_map_symfile->FindDefinitionDIE(decl_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 must link its
- // DeclContext to this die.
- if (clang::DeclContext *defn_decl_ctx =
- GetCachedClangDeclContextForDIE(dwarf->GetDIE(type_sp->GetID())))
- LinkDeclContextToDIE(defn_decl_ctx, die);
- return 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}",
+ static_cast<void *>(this), def_die.GetID(), DW_TAG_value_to_name(tag),
+ tag, attrs.name.GetCString(),
+ def_die ? llvm::utohexstr(def_die.GetID()) : "not found");
}
}
+
+ if (def_die) {
+ attrs = ParsedDWARFTypeAttributes(def_die);
+ } else {
+ // No definition found. Proceed with the declaration die. We can use it to
+ // create a forward-declared type.
+ def_die = decl_die;
+ }
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);
+ clang::DeclContext *containing_decl_ctx = GetClangDeclContextContainingDIE(def_die, nullptr);
- PrepareContextToReceiveMembers(m_ast, GetClangASTImporter(), decl_ctx, die,
+ PrepareContextToReceiveMembers(m_ast, GetClangASTImporter(),
+ containing_decl_ctx, def_die,
attrs.name.GetCString());
- if (attrs.accessibility == eAccessNone && decl_ctx) {
+ if (attrs.accessibility == eAccessNone && containing_decl_ctx) {
// Check the decl context that contains this class/struct/union. If
// it is a class we must give it an accessibility.
- const clang::Decl::Kind containing_decl_kind = decl_ctx->getDeclKind();
+ const clang::Decl::Kind containing_decl_kind =
+ containing_decl_ctx->getDeclKind();
if (DeclKindIsCXXClass(containing_decl_kind))
attrs.accessibility = default_accessibility;
}
ClangASTMetadata metadata;
- metadata.SetUserID(die.GetID());
- metadata.SetIsDynamicCXXType(dwarf->ClassOrStructIsVirtual(die));
+ metadata.SetUserID(def_die.GetID());
+ metadata.SetIsDynamicCXXType(dwarf->ClassOrStructIsVirtual(def_die));
TypeSystemClang::TemplateParameterInfos template_param_infos;
- if (ParseTemplateParameterInfos(die, template_param_infos)) {
+ if (ParseTemplateParameterInfos(def_die, template_param_infos)) {
clang::ClassTemplateDecl *class_template_decl =
m_ast.ParseClassTemplateDecl(
- decl_ctx, GetOwningClangModule(die), attrs.accessibility,
- attrs.name.GetCString(), tag_decl_kind, template_param_infos);
+ containing_decl_ctx, GetOwningClangModule(def_die),
+ attrs.accessibility, attrs.name.GetCString(), tag_decl_kind,
+ template_param_infos);
if (!class_template_decl) {
if (log) {
dwarf->GetObjectFile()->GetModule()->LogMessage(
log,
"SymbolFileDWARF({0:p}) - {1:x16}: {2} ({3}) type \"{4}\" "
"clang::ClassTemplateDecl failed to return a decl.",
- static_cast<void *>(this), die.GetOffset(),
+ static_cast<void *>(this), def_die.GetID(),
DW_TAG_value_to_name(tag), tag, attrs.name.GetCString());
}
return TypeSP();
@@ -1843,8 +1844,8 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
clang::ClassTemplateSpecializationDecl *class_specialization_decl =
m_ast.CreateClassTemplateSpecializationDecl(
- decl_ctx, GetOwningClangModule(die), class_template_decl,
- tag_decl_kind, template_param_infos);
+ containing_decl_ctx, GetOwningClangModule(def_die),
+ class_template_decl, tag_decl_kind, template_param_infos);
clang_type =
m_ast.CreateClassTemplateSpecializationType(class_specialization_decl);
clang_type_was_created = true;
@@ -1856,7 +1857,7 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
if (!clang_type_was_created) {
clang_type_was_created = true;
clang_type = m_ast.CreateRecordType(
- decl_ctx, GetOwningClangModule(die), attrs.accessibility,
+ containing_decl_ctx, GetOwningClangModule(def_die), attrs.accessibility,
attrs.name.GetCString(), tag_decl_kind, attrs.class_language, &metadata,
attrs.exports_symbols);
}
@@ -1864,9 +1865,13 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
// 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.
- LinkDeclContextToDIE(m_ast.GetDeclContextForType(clang_type), die);
- type_sp = dwarf->MakeType(
- die.GetID(), attrs.name, attrs.byte_size, nullptr, LLDB_INVALID_UID,
+ clang::DeclContext *type_decl_ctx =
+ TypeSystemClang::GetDeclContextForType(clang_type);
+ LinkDeclContextToDIE(type_decl_ctx, def_die);
+ if (decl_die != def_die)
+ LinkDeclContextToDIE(type_decl_ctx, decl_die);
+ TypeSP type_sp = dwarf->MakeType(
+ def_die.GetID(), attrs.name, attrs.byte_size, nullptr, LLDB_INVALID_UID,
Type::eEncodingIsUID, &attrs.decl, clang_type,
Type::ResolveState::Forward,
TypePayloadClang(OptionalClangModuleID(), attrs.is_complete_objc_class));
@@ -1875,7 +1880,7 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
// copies of the same type over and over in the ASTContext for our
// module
unique_ast_entry_up->m_type_sp = type_sp;
- unique_ast_entry_up->m_die = die;
+ unique_ast_entry_up->m_die = def_die;
unique_ast_entry_up->m_declaration = unique_decl;
unique_ast_entry_up->m_byte_size = attrs.byte_size.value_or(0);
dwarf->GetUniqueDWARFASTTypeMap().Insert(unique_typename,
@@ -1886,18 +1891,17 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
// has child classes or types that require the class to be created
// for use as their decl contexts the class will be ready to accept
// these child definitions.
- if (!die.HasChildren()) {
+ if (!def_die.HasChildren()) {
// No children for this struct/union/class, lets finish it
if (TypeSystemClang::StartTagDeclarationDefinition(clang_type)) {
TypeSystemClang::CompleteTagDeclarationDefinition(clang_type);
} else {
dwarf->GetObjectFile()->GetModule()->ReportError(
- "DWARF DIE at {0:x16} named \"{1}\" was not able to start "
- "its "
+ "DWARF DIE {0:x16} named \"{1}\" was not able to start its "
"definition.\nPlease file a bug and attach the file at the "
"start of this error message",
- die.GetOffset(), attrs.name.GetCString());
+ def_die.GetID(), attrs.name.GetCString());
}
// Setting authority byte size and alignment for empty structures.
@@ -1945,7 +1949,7 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
// binaries.
dwarf->GetForwardDeclCompilerTypeToDIE().try_emplace(
ClangUtil::RemoveFastQualifiers(clang_type).GetOpaqueQualType(),
- *die.GetDIERef());
+ *def_die.GetDIERef());
m_ast.SetHasExternalStorage(clang_type.GetOpaqueQualType(), true);
}
}
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index 0f3eab0186c4e..117e02bb8c5de 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -3038,118 +3038,113 @@ TypeSP Sy...
[truncated]
|
Good catch. I've been meaning to add that, but I forgot. Things should still work even without the DieToType update, as the type will be caught by the UniqueDWARFASTTypeMap, but this is definitely faster. The way that the definition dies are currently handled is a bit clumsy now, but I didn't want to implement anything more elaborate as this should go away once we stop eagerly searching for the definition. |
|
Latest update LGTM |
|
This patch as-is is NFC? (no tests) but without this patch, the other delay-definition-die patch would have had a problem? Is it possible to add a test in this patch that would exercise the thing that would become buggy if the delay-definition-die patch were to be recommitted? |
NFCI, I would say :) I don't think this should change the behavior in any way, but it's pretty hard to guarantee that.
Sort of. The situation is a bit complicated, because this touches pretty much the same code as the other patch, so the other patch will not apply cleanly or become magically correct. It's more like a building block that enables us to rewrite the other patch in a more robust manner -- which brings us to the second way this is complicated: It's not that the other patch was wrong on its own. It was just very sensitive to the other bugs. Previously, if we failed to unique the types correctly, we would "just" get the wrong type (or maybe no type). With the patch, that situation would trigger a hard assert. On its own, that might even be considered a good thing (easier to find bugs), we're it not for the fact that this made the logic of the patch very hard to follow. So, this is my attempt to make it more straight-forward. As for tests, it is possible to write a test which would reproduce a crash with the original patch, but that test is contingent on the existence of another bug. When I reverted that patch, I added a test (in de3f1b6) which triggered the crash. However, now that that bug is fixed (#95905), it does not crash anymore. Now, I happen to know of another bug (which happens to be triggered by the same code, only compiled with type units), but the same thing will happen once that bug is fixed. Given all of that, I don't think another test case is needed with this particular patch. It might be interesting for the final delay patch, if we don't fix the type unit thing by then, but I think of this patch mostly as a cleanup. |
Sure enough - I take any claim as a statement of intent/belief, not of something mathematically proved, etc.
Having a bit of a hard time following this - is the test you added the same as the test you speculated is possible to write in the prior sentence?
OK - I think I'm following.
Perhaps a separate commit could add another RUN line to the existing test you added to demonstrate the reason for the revert? Rather than worrying about which comes first (the type unit patch or the delay patch) But in any case, I /think/ I understand why this patch doesn't need a test (because this patch avoids the delay patch causing a crash (yeah, more complex than that because the patch doesn't apply cleanly over this one) and that crash already has a test committed) - thanks for the explanation. |
True, but in case of this code, even believing you know what it does may be a tough ask. :D
Correct. The reason for revert has already been established with the first test. I'll create a separate patch for the type unit bug, but it will be slightly more complicated than adding a RUN line, because, due to the bug, the lldb will print the wrong type. |
| type_sp = | ||
| dwarf->MakeType(die.GetID(), attrs.name, attrs.byte_size, nullptr, | ||
| TypeSP type_sp = | ||
| dwarf->MakeType(def_die.GetID(), attrs.name, attrs.byte_size, nullptr, |
There was a problem hiding this comment.
If two different enum decl_die (reference the same definition def_die) were called with this function, doesn't it create two CompilerType and two Type from the same def_die? It's not a problem for ParseStructureLikeDIE because it will check if we have already created the type in UniqueDWARFASTTypeMap.
There was a problem hiding this comment.
Yes, that's a very good catch. I noticed the lack of a unique map in the enum implementation, but I just assumed that means we don't care about duplicate definitions for enums. (And to a sense that's true, since it still means we will create a duplicate definition if we start with two definition DIEs.)
This should be fairly simple to fix, I'll create a today.
This is a regression from llvm#96484 caught by @ZequanWu. Note that we will still create separate enum types for types parsed from two definitions. This is different from how we handle classes, but it is not a regression. I'm also adding the DieToType check to the class type parsing code, although in this case, the type uniqueness should be enforced by the UniqueDWARFASTType map.
This is a regression from #96484 caught by @ZequanWu. Note that we will still create separate enum types for types parsed from two definitions. This is different from how we handle classes, but it is not a regression. I'm also adding the DieToType check to the class type parsing code, although in this case, the type uniqueness should be enforced by the UniqueDWARFASTType map.
This is a regression from llvm#96484 caught by @ZequanWu. Note that we will still create separate enum types for types parsed from two definitions. This is different from how we handle classes, but it is not a regression. I'm also adding the DieToType check to the class type parsing code, although in this case, the type uniqueness should be enforced by the UniqueDWARFASTType map.
…IEs (llvm#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 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.
This is a regression from llvm#96484 caught by @ZequanWu. Note that we will still create separate enum types for types parsed from two definitions. This is different from how we handle classes, but it is not a regression. I'm also adding the DieToType check to the class type parsing code, although in this case, the type uniqueness should be enforced by the UniqueDWARFASTType map.
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.