Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
May 28, 2024
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d6de8d9
Reapply [lldb][DWARF] Delay struct/class/union definition DIE searchi…
ZequanWu May 15, 2024
2172c66
Fix tests failure on MacOS by query SymbolFileDWARFDebugMap first to …
ZequanWu May 15, 2024
a2442a2
Remove checking for foreward declaration attr and apply https://githu…
ZequanWu May 28, 2024
10b74de
format: tab -> spaces
ZequanWu May 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
397 changes: 218 additions & 179 deletions lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
View file
Open in desktop

Large diffs are not rendered by default.

197 changes: 88 additions & 109 deletions lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
View file
Open in desktop

Large diffs are not rendered by default.

4 changes: 4 additions & 0 deletions lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,10 @@ bool DebugNamesDWARFIndex::ProcessEntry(
DWARFDIE die = dwarf.GetDIE(*ref);
if (!die)
return true;
// Clang erroneously emits index entries for declaration DIEs in case when the
// definition is in a type unit (llvm.org/pr77696). Weed those out.
if (die.GetAttributeValueAsUnsigned(DW_AT_declaration, 0))
return true;
return callback(die);
}

Expand Down
51 changes: 32 additions & 19 deletions lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,6 +481,13 @@ static ConstString GetDWARFMachOSegmentName() {
return g_dwarf_section_name;
}

llvm::DenseMap<lldb::opaque_compiler_type_t, DIERef> &
SymbolFileDWARF::GetForwardDeclCompilerTypeToDIE() {
if (SymbolFileDWARFDebugMap *debug_map_symfile = GetDebugMapSymfile())
return debug_map_symfile->GetForwardDeclCompilerTypeToDIE();
return m_forward_decl_compiler_type_to_die;
}

UniqueDWARFASTTypeMap &SymbolFileDWARF::GetUniqueDWARFASTTypeMap() {
SymbolFileDWARFDebugMap *debug_map_symfile = GetDebugMapSymfile();
if (debug_map_symfile)
Expand Down Expand Up @@ -1632,27 +1639,33 @@ bool SymbolFileDWARF::CompleteType(CompilerType &compiler_type) {
return true;
}

DWARFDIE dwarf_die = 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);

Type *type = GetDIEToType().lookup(dwarf_die.GetDIE());
// Once we start resolving this type, remove it from the forward
// declaration map in case anyone's child members or other types require this
// type to get resolved.
DWARFDIE dwarf_die = GetDIE(die_it->second);
GetForwardDeclCompilerTypeToDIE().erase(die_it);
Type *type = nullptr;
if (DWARFASTParser *dwarf_ast = GetDWARFParser(*dwarf_die.GetCU()))
type = dwarf_ast->FindDefinitionTypeForDIE(dwarf_die);
if (!type)
return false;

Log *log = GetLog(DWARFLog::DebugInfo | DWARFLog::TypeCompletion);
if (log)
GetObjectFile()->GetModule()->LogMessageVerboseBacktrace(
log, "{0:x8}: {1} ({2}) '{3}' resolving forward declaration...",
dwarf_die.GetID(), DW_TAG_value_to_name(dwarf_die.Tag()),
dwarf_die.Tag(), type->GetName().AsCString());
assert(compiler_type);
if (DWARFASTParser *dwarf_ast = GetDWARFParser(*dwarf_die.GetCU()))
return dwarf_ast->CompleteTypeFromDWARF(dwarf_die, type, compiler_type);
die_it = GetForwardDeclCompilerTypeToDIE().find(
compiler_type_no_qualifiers.GetOpaqueQualType());
if (die_it != GetForwardDeclCompilerTypeToDIE().end()) {
dwarf_die = GetDIE(die_it->getSecond());
GetForwardDeclCompilerTypeToDIE().erase(die_it);
}
return false;

if (Log *log = GetLog(DWARFLog::DebugInfo | DWARFLog::TypeCompletion))
GetObjectFile()->GetModule()->LogMessageVerboseBacktrace(
log, "{0:x8}: {1} ({2}) '{3}' resolving forward declaration...",
dwarf_die.GetID(), DW_TAG_value_to_name(dwarf_die.Tag()),
dwarf_die.Tag(), type->GetName().AsCString());
assert(compiler_type);
if (DWARFASTParser *dwarf_ast = GetDWARFParser(*dwarf_die.GetCU()))
return dwarf_ast->CompleteTypeFromDWARF(dwarf_die, type, compiler_type);
return true;
}

Type *SymbolFileDWARF::ResolveType(const DWARFDIE &die,
Expand Down
15 changes: 8 additions & 7 deletions lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,12 +335,8 @@ class SymbolFileDWARF : public SymbolFileCommon {

virtual DIEToTypePtr &GetDIEToType() { return m_die_to_type; }

typedef llvm::DenseMap<lldb::opaque_compiler_type_t, DIERef>
CompilerTypeToDIE;

virtual CompilerTypeToDIE &GetForwardDeclCompilerTypeToDIE() {
return m_forward_decl_compiler_type_to_die;
}
virtual llvm::DenseMap<lldb::opaque_compiler_type_t, DIERef> &
GetForwardDeclCompilerTypeToDIE();

typedef llvm::DenseMap<const DWARFDebugInfoEntry *, lldb::VariableSP>
DIEToVariableSP;
Expand Down Expand Up @@ -533,9 +529,14 @@ class SymbolFileDWARF : public SymbolFileCommon {
NameToOffsetMap m_function_scope_qualified_name_map;
std::unique_ptr<DWARFDebugRanges> m_ranges;
UniqueDWARFASTTypeMap m_unique_ast_type_map;
// A map from DIE to lldb_private::Type. For record type, the key might be
// either declaration DIE or definition DIE.
DIEToTypePtr m_die_to_type;
DIEToVariableSP m_die_to_variable_sp;
CompilerTypeToDIE m_forward_decl_compiler_type_to_die;
// 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;
llvm::DenseMap<dw_offset_t, std::unique_ptr<SupportFileList>>
m_type_unit_support_files;
std::vector<uint32_t> m_lldb_cu_to_dwarf_unit;
Expand Down
9 changes: 9 additions & 0 deletions lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,11 @@ class SymbolFileDWARFDebugMap : public SymbolFileCommon {
lldb::TypeSP FindCompleteObjCDefinitionTypeForDIE(
const DWARFDIE &die, ConstString type_name, bool must_be_implementation);

llvm::DenseMap<lldb::opaque_compiler_type_t, DIERef> &
GetForwardDeclCompilerTypeToDIE() {
return m_forward_decl_compiler_type_to_die;
}

UniqueDWARFASTTypeMap &GetUniqueDWARFASTTypeMap() {
return m_unique_ast_type_map;
}
Expand Down Expand Up @@ -321,6 +326,10 @@ class SymbolFileDWARFDebugMap : public SymbolFileCommon {
std::vector<uint32_t> m_func_indexes; // Sorted by address
std::vector<uint32_t> m_glob_indexes;
std::map<std::pair<ConstString, llvm::sys::TimePoint<>>, OSOInfoSP> m_oso_map;
// 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;
Copy link
Member

@Michael137 Michael137 May 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

if (oso_dwarf->HasForwardDeclForCompilerType(compiler_type)) {
oso_dwarf->CompleteType(compiler_type);
success = true;
return IterationAction::Stop;
}
. If exists, then it assumes that symbol file should have the definition DIE and able to complete it. Since 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.

UniqueDWARFASTTypeMap m_unique_ast_type_map;
LazyBool m_supports_DW_AT_APPLE_objc_complete_type;
DebugMap m_debug_map;
Expand Down
2 changes: 1 addition & 1 deletion lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ SymbolFileDWARF::DIEToVariableSP &SymbolFileDWARFDwo::GetDIEToVariable() {
return GetBaseSymbolFile().GetDIEToVariable();
}

SymbolFileDWARF::CompilerTypeToDIE &
llvm::DenseMap<lldb::opaque_compiler_type_t, DIERef> &
SymbolFileDWARFDwo::GetForwardDeclCompilerTypeToDIE() {
return GetBaseSymbolFile().GetForwardDeclCompilerTypeToDIE();
}
Expand Down
3 changes: 2 additions & 1 deletion lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,8 @@ class SymbolFileDWARFDwo : public SymbolFileDWARF {

DIEToVariableSP &GetDIEToVariable() override;

CompilerTypeToDIE &GetForwardDeclCompilerTypeToDIE() override;
llvm::DenseMap<lldb::opaque_compiler_type_t, DIERef> &
GetForwardDeclCompilerTypeToDIE() override;

UniqueDWARFASTTypeMap &GetUniqueDWARFASTTypeMap() override;

Expand Down
107 changes: 54 additions & 53 deletions lldb/source/Plugins/SymbolFile/DWARF/UniqueDWARFASTType.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,66 +13,67 @@
using namespace lldb_private::dwarf;
using namespace lldb_private::plugin::dwarf;

bool UniqueDWARFASTTypeList::Find(const DWARFDIE &die,
const lldb_private::Declaration &decl,
const int32_t byte_size,
UniqueDWARFASTType &entry) const {
for (const UniqueDWARFASTType &udt : m_collection) {
UniqueDWARFASTType *UniqueDWARFASTTypeList::Find(
const DWARFDIE &die, const lldb_private::Declaration &decl,
const int32_t byte_size, bool is_forward_declaration) {
for (UniqueDWARFASTType &udt : m_collection) {
// Make sure the tags match
if (udt.m_die.Tag() == die.Tag()) {
// Validate byte sizes of both types only if both are valid.
if (udt.m_byte_size < 0 || byte_size < 0 ||
udt.m_byte_size == byte_size) {
// Make sure the file and line match
if (udt.m_declaration == decl) {
// The type has the same name, and was defined on the same file and
// line. Now verify all of the parent DIEs match.
DWARFDIE parent_arg_die = die.GetParent();
DWARFDIE parent_pos_die = udt.m_die.GetParent();
bool match = true;
bool done = false;
while (!done && match && parent_arg_die && parent_pos_die) {
const dw_tag_t parent_arg_tag = parent_arg_die.Tag();
const dw_tag_t parent_pos_tag = parent_pos_die.Tag();
if (parent_arg_tag == parent_pos_tag) {
switch (parent_arg_tag) {
case DW_TAG_class_type:
case DW_TAG_structure_type:
case DW_TAG_union_type:
case DW_TAG_namespace: {
const char *parent_arg_die_name = parent_arg_die.GetName();
if (parent_arg_die_name ==
nullptr) // Anonymous (i.e. no-name) struct
{
match = false;
} else {
const char *parent_pos_die_name = parent_pos_die.GetName();
if (parent_pos_die_name == nullptr ||
((parent_arg_die_name != parent_pos_die_name) &&
strcmp(parent_arg_die_name, parent_pos_die_name)))
match = false;
}
} break;

case DW_TAG_compile_unit:
case DW_TAG_partial_unit:
done = true;
break;
default:
break;
}
// If they are not both definition DIEs or both declaration DIEs, then
// don't check for byte size and declaration location, because declaration
// DIEs usually don't have those info.
bool matching_size_declaration =
udt.m_is_forward_declaration != is_forward_declaration
? true
: (udt.m_byte_size < 0 || byte_size < 0 ||
udt.m_byte_size == byte_size) &&
udt.m_declaration == decl;
if (!matching_size_declaration)
continue;
// The type has the same name, and was defined on the same file and
// line. Now verify all of the parent DIEs match.
DWARFDIE parent_arg_die = die.GetParent();
DWARFDIE parent_pos_die = udt.m_die.GetParent();
bool match = true;
bool done = false;
while (!done && match && parent_arg_die && parent_pos_die) {
const dw_tag_t parent_arg_tag = parent_arg_die.Tag();
const dw_tag_t parent_pos_tag = parent_pos_die.Tag();
if (parent_arg_tag == parent_pos_tag) {
switch (parent_arg_tag) {
case DW_TAG_class_type:
case DW_TAG_structure_type:
case DW_TAG_union_type:
case DW_TAG_namespace: {
const char *parent_arg_die_name = parent_arg_die.GetName();
if (parent_arg_die_name == nullptr) {
// Anonymous (i.e. no-name) struct
match = false;
} else {
const char *parent_pos_die_name = parent_pos_die.GetName();
if (parent_pos_die_name == nullptr ||
((parent_arg_die_name != parent_pos_die_name) &&
strcmp(parent_arg_die_name, parent_pos_die_name)))
match = false;
}
parent_arg_die = parent_arg_die.GetParent();
parent_pos_die = parent_pos_die.GetParent();
}
} break;

if (match) {
entry = udt;
return true;
case DW_TAG_compile_unit:
case DW_TAG_partial_unit:
done = true;
break;
default:
break;
}
}
parent_arg_die = parent_arg_die.GetParent();
parent_pos_die = parent_pos_die.GetParent();
}

if (match) {
return &udt;
}
}
}
return false;
return nullptr;
}
36 changes: 13 additions & 23 deletions lldb/source/Plugins/SymbolFile/DWARF/UniqueDWARFASTType.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,31 +23,19 @@ class UniqueDWARFASTType {
// Constructors and Destructors
UniqueDWARFASTType() : m_type_sp(), m_die(), m_declaration() {}

UniqueDWARFASTType(lldb::TypeSP &type_sp, const DWARFDIE &die,
const Declaration &decl, int32_t byte_size)
: m_type_sp(type_sp), m_die(die), m_declaration(decl),
m_byte_size(byte_size) {}

UniqueDWARFASTType(const UniqueDWARFASTType &rhs)
: m_type_sp(rhs.m_type_sp), m_die(rhs.m_die),
m_declaration(rhs.m_declaration), m_byte_size(rhs.m_byte_size) {}
m_declaration(rhs.m_declaration), m_byte_size(rhs.m_byte_size),
m_is_forward_declaration(rhs.m_is_forward_declaration) {}

~UniqueDWARFASTType() = default;

UniqueDWARFASTType &operator=(const UniqueDWARFASTType &rhs) {
if (this != &rhs) {
m_type_sp = rhs.m_type_sp;
m_die = rhs.m_die;
m_declaration = rhs.m_declaration;
m_byte_size = rhs.m_byte_size;
}
return *this;
}

lldb::TypeSP m_type_sp;
DWARFDIE m_die;
Declaration m_declaration;
int32_t m_byte_size = -1;
// True if the m_die is a forward declaration DIE.
bool m_is_forward_declaration = true;
};

class UniqueDWARFASTTypeList {
Expand All @@ -62,8 +50,9 @@ class UniqueDWARFASTTypeList {
m_collection.push_back(entry);
}

bool Find(const DWARFDIE &die, const Declaration &decl,
const int32_t byte_size, UniqueDWARFASTType &entry) const;
UniqueDWARFASTType *Find(const DWARFDIE &die, const Declaration &decl,
const int32_t byte_size,
bool is_forward_declaration);

protected:
typedef std::vector<UniqueDWARFASTType> collection;
Expand All @@ -80,14 +69,15 @@ class UniqueDWARFASTTypeMap {
m_collection[name.GetCString()].Append(entry);
}

bool Find(ConstString name, const DWARFDIE &die, const Declaration &decl,
const int32_t byte_size, UniqueDWARFASTType &entry) const {
UniqueDWARFASTType *Find(ConstString name, const DWARFDIE &die,
const Declaration &decl, const int32_t byte_size,
bool is_forward_declaration) {
const char *unique_name_cstr = name.GetCString();
collection::const_iterator pos = m_collection.find(unique_name_cstr);
collection::iterator pos = m_collection.find(unique_name_cstr);
if (pos != m_collection.end()) {
return pos->second.Find(die, decl, byte_size, entry);
return pos->second.Find(die, decl, byte_size, is_forward_declaration);
}
return false;
return nullptr;
}

protected:
Expand Down
36 changes: 36 additions & 0 deletions lldb/test/Shell/SymbolFile/DWARF/delayed-definition-die-searching.test
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
# Test definition DIE searching is delayed until complete type is required.

# UNSUPPORTED: system-windows

# RUN: split-file %s %t
# RUN: %clangxx_host %t/main.cpp %t/t1_def.cpp -gdwarf -o %t.out
# RUN: %lldb -b %t.out -s %t/lldb.cmd | FileCheck %s

# CHECK: (lldb) p v1
# CHECK: DWARFASTParserClang::ParseTypeFromDWARF{{.*}}DW_TAG_structure_type (DW_TAG_structure_type) name = 't2<t1>'
# CHECK: DWARFASTParserClang::ParseTypeFromDWARF{{.*}}DW_TAG_structure_type (DW_TAG_structure_type) name = 't1'
# CHECK: DW_TAG_structure_type (DW_TAG_structure_type) 't2<t1>' resolving forward declaration...
# CHECK: (t2<t1>) {}
# CHECK: (lldb) p v2
# CHECK: DWARFASTParserClang::ParseTypeFromDWARF{{.*}}DW_TAG_structure_type (DW_TAG_structure_type) name = 't1'
# CHECK: DW_TAG_structure_type (DW_TAG_structure_type) 't1' resolving forward declaration...

#--- lldb.cmd
log enable dwarf comp
p v1
p v2

#--- main.cpp
template<typename T>
struct t2 {
};
struct t1;
t2<t1> v1; // this CU doesn't have definition DIE for t1, but only declaration DIE for it.
int main() {
}

#--- t1_def.cpp
struct t1 { // this CU contains definition DIE for t1.
int x;
};
t1 v2;
Loading