Skip to content

[lldb] Recurse through DW_AT_signature when looking for attributes #107241

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

Merged
merged 2 commits into from
Sep 10, 2024
Merged

[lldb] Recurse through DW_AT_signature when looking for attributes #107241

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2fd6f97
[lldb] Recurse through DW_AT_signature when looking for attributes
labath Sep 4, 2024
9840edd
fix elaborating iterator
labath Sep 5, 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
18 changes: 4 additions & 14 deletions lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@ class ElaboratingDIEIterator
m_worklist.pop_back();

// And add back any items that elaborate it.
for (dw_attr_t attr : {DW_AT_specification, DW_AT_abstract_origin}) {
for (dw_attr_t attr :
{DW_AT_specification, DW_AT_abstract_origin, DW_AT_specification}) {
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean for one of these to be DW_AT_signature?

Copy link
Member

Choose a reason for hiding this comment

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

Just from cursory reading it seems like ElaboratingDIEIterator is only useful for method DIEs currently? Which might be why no tests were affected here?

If you do need it as part of this patch then there's a couple of comments to fix up in ElaboratingDIEIterator that specifically talk about DW_AT_specification/DW_AT_abstract_origin

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I meant that to be DW_AT_signature. Oops.

This isn't really needed for this patch, but I thought it makes sense to add it for symmetry. The class is only currently used in DWARFDIE::IsMethod which does not make sense for types, but I could imagine this class being used elsewhere as well.

if (DWARFDIE d = die.GetReferencedDIE(attr))
if (m_seen.insert(die.GetDIE()).second)
m_worklist.push_back(d);
Expand Down Expand Up @@ -129,10 +130,10 @@ DWARFDIE
DWARFDIE::GetAttributeValueAsReferenceDIE(const dw_attr_t attr) const {
if (IsValid()) {
DWARFUnit *cu = GetCU();
const bool check_specification_or_abstract_origin = true;
const bool check_elaborating_dies = true;
DWARFFormValue form_value;
if (m_die->GetAttributeValue(cu, attr, form_value, nullptr,
check_specification_or_abstract_origin))
check_elaborating_dies))
return form_value.Reference();
}
return DWARFDIE();
Expand Down Expand Up @@ -377,11 +378,6 @@ static void GetDeclContextImpl(DWARFDIE die,
die = spec;
continue;
}
// To find the name of a type in a type unit, we must follow the signature.
if (DWARFDIE spec = die.GetReferencedDIE(DW_AT_signature)) {
Copy link
Member

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 we don't need this anymore?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because die.GetName() will now find the correct name on its own. And (unlike with DW_AT_specification) the correct context can be reconstructed by following the parent chain in the original dwarf unit. For nested structs like Outer::Middle::Inner we get something like this in the main dwarf unit:

0x00000029:   DW_TAG_structure_type
                DW_AT_declaration	(true)
                DW_AT_signature	(0xd351fbfc6a4cee7c) => points to Outer

0x00000032:     DW_TAG_structure_type
                  DW_AT_declaration	(true)
                  DW_AT_signature	(0x5f726fb54ccb6c95) => points to Outer::Middle

0x0000003b:       DW_TAG_structure_type
                    DW_AT_declaration	(true)
                    DW_AT_signature	(0xd3cee531644665ec) => points to Outer::Middle::Inner

Copy link
Member

Choose a reason for hiding this comment

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

Ahh right, makes sense.

So for DW_AT_specifications, the referring DIE doesn't have the parent chain structure. So we must follow the DIE it referred to in order to reconstruct the context. With DW_AT_signature the context structure is maintained, so this now becomes redundant.

die = spec;
continue;
}

// Add this DIE's contribution at the end of the chain.
auto push_ctx = [&](CompilerContextKind kind, llvm::StringRef name) {
Expand Down Expand Up @@ -434,12 +430,6 @@ static void GetTypeLookupContextImpl(DWARFDIE die,
std::vector<CompilerContext> &context) {
// Stop if we hit a cycle.
while (die && seen.insert(die.GetID()).second) {
// To find the name of a type in a type unit, we must follow the signature.
if (DWARFDIE spec = die.GetReferencedDIE(DW_AT_signature)) {
die = spec;
continue;
}

// Add this DIE's contribution at the end of the chain.
auto push_ctx = [&](CompilerContextKind kind, llvm::StringRef name) {
context.push_back({kind, ConstString(name)});
Expand Down
78 changes: 32 additions & 46 deletions lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,8 +355,7 @@ void DWARFDebugInfoEntry::GetAttributes(DWARFUnit *cu,
// would be a compile unit header).
dw_offset_t DWARFDebugInfoEntry::GetAttributeValue(
const DWARFUnit *cu, const dw_attr_t attr, DWARFFormValue &form_value,
dw_offset_t *end_attr_offset_ptr,
bool check_specification_or_abstract_origin) const {
dw_offset_t *end_attr_offset_ptr, bool check_elaborating_dies) const {
if (const auto *abbrevDecl = GetAbbreviationDeclarationPtr(cu)) {
std::optional<uint32_t> attr_idx = abbrevDecl->findAttributeIndex(attr);

Expand All @@ -380,25 +379,18 @@ dw_offset_t DWARFDebugInfoEntry::GetAttributeValue(
}
}

if (check_specification_or_abstract_origin) {
if (GetAttributeValue(cu, DW_AT_specification, form_value)) {
if (check_elaborating_dies) {
for (dw_attr_t elaborating_attr :
{DW_AT_specification, DW_AT_abstract_origin, DW_AT_signature}) {
if (!GetAttributeValue(cu, elaborating_attr, form_value))
continue;
DWARFDIE die = form_value.Reference();
if (die) {
dw_offset_t die_offset = die.GetDIE()->GetAttributeValue(
die.GetCU(), attr, form_value, end_attr_offset_ptr, false);
if (die_offset)
return die_offset;
}
}

if (GetAttributeValue(cu, DW_AT_abstract_origin, form_value)) {
DWARFDIE die = form_value.Reference();
if (die) {
dw_offset_t die_offset = die.GetDIE()->GetAttributeValue(
die.GetCU(), attr, form_value, end_attr_offset_ptr, false);
if (die_offset)
return die_offset;
}
if (!die)
continue;
dw_offset_t die_offset = die.GetDIE()->GetAttributeValue(
die.GetCU(), attr, form_value, end_attr_offset_ptr, false);
if (die_offset)
return die_offset;
}
}
return 0;
Expand All @@ -412,10 +404,9 @@ dw_offset_t DWARFDebugInfoEntry::GetAttributeValue(
// doesn't change.
const char *DWARFDebugInfoEntry::GetAttributeValueAsString(
const DWARFUnit *cu, const dw_attr_t attr, const char *fail_value,
bool check_specification_or_abstract_origin) const {
bool check_elaborating_dies) const {
DWARFFormValue form_value;
if (GetAttributeValue(cu, attr, form_value, nullptr,
check_specification_or_abstract_origin))
if (GetAttributeValue(cu, attr, form_value, nullptr, check_elaborating_dies))
return form_value.AsCString();
return fail_value;
}
Expand All @@ -425,21 +416,19 @@ const char *DWARFDebugInfoEntry::GetAttributeValueAsString(
// Get the value of an attribute as unsigned and return it.
uint64_t DWARFDebugInfoEntry::GetAttributeValueAsUnsigned(
const DWARFUnit *cu, const dw_attr_t attr, uint64_t fail_value,
bool check_specification_or_abstract_origin) const {
bool check_elaborating_dies) const {
DWARFFormValue form_value;
if (GetAttributeValue(cu, attr, form_value, nullptr,
check_specification_or_abstract_origin))
if (GetAttributeValue(cu, attr, form_value, nullptr, check_elaborating_dies))
return form_value.Unsigned();
return fail_value;
}

std::optional<uint64_t>
DWARFDebugInfoEntry::GetAttributeValueAsOptionalUnsigned(
const DWARFUnit *cu, const dw_attr_t attr,
bool check_specification_or_abstract_origin) const {
bool check_elaborating_dies) const {
DWARFFormValue form_value;
if (GetAttributeValue(cu, attr, form_value, nullptr,
check_specification_or_abstract_origin))
if (GetAttributeValue(cu, attr, form_value, nullptr, check_elaborating_dies))
return form_value.Unsigned();
return std::nullopt;
}
Expand All @@ -450,20 +439,18 @@ DWARFDebugInfoEntry::GetAttributeValueAsOptionalUnsigned(
// relative offsets as needed.
DWARFDIE DWARFDebugInfoEntry::GetAttributeValueAsReference(
const DWARFUnit *cu, const dw_attr_t attr,
bool check_specification_or_abstract_origin) const {
bool check_elaborating_dies) const {
DWARFFormValue form_value;
if (GetAttributeValue(cu, attr, form_value, nullptr,
check_specification_or_abstract_origin))
if (GetAttributeValue(cu, attr, form_value, nullptr, check_elaborating_dies))
return form_value.Reference();
return {};
}

uint64_t DWARFDebugInfoEntry::GetAttributeValueAsAddress(
const DWARFUnit *cu, const dw_attr_t attr, uint64_t fail_value,
bool check_specification_or_abstract_origin) const {
bool check_elaborating_dies) const {
DWARFFormValue form_value;
if (GetAttributeValue(cu, attr, form_value, nullptr,
check_specification_or_abstract_origin))
if (GetAttributeValue(cu, attr, form_value, nullptr, check_elaborating_dies))
return form_value.Address();
return fail_value;
}
Expand All @@ -474,12 +461,13 @@ uint64_t DWARFDebugInfoEntry::GetAttributeValueAsAddress(
// pc>.
//
// Returns the hi_pc or fail_value.
dw_addr_t DWARFDebugInfoEntry::GetAttributeHighPC(
const DWARFUnit *cu, dw_addr_t lo_pc, uint64_t fail_value,
bool check_specification_or_abstract_origin) const {
dw_addr_t
DWARFDebugInfoEntry::GetAttributeHighPC(const DWARFUnit *cu, dw_addr_t lo_pc,
uint64_t fail_value,
bool check_elaborating_dies) const {
DWARFFormValue form_value;
if (GetAttributeValue(cu, DW_AT_high_pc, form_value, nullptr,
check_specification_or_abstract_origin)) {
check_elaborating_dies)) {
dw_form_t form = form_value.Form();
if (form == DW_FORM_addr || form == DW_FORM_addrx ||
form == DW_FORM_GNU_addr_index)
Expand All @@ -499,12 +487,11 @@ dw_addr_t DWARFDebugInfoEntry::GetAttributeHighPC(
// Returns true or sets lo_pc and hi_pc to fail_value.
bool DWARFDebugInfoEntry::GetAttributeAddressRange(
const DWARFUnit *cu, dw_addr_t &lo_pc, dw_addr_t &hi_pc,
uint64_t fail_value, bool check_specification_or_abstract_origin) const {
uint64_t fail_value, bool check_elaborating_dies) const {
lo_pc = GetAttributeValueAsAddress(cu, DW_AT_low_pc, fail_value,
check_specification_or_abstract_origin);
check_elaborating_dies);
if (lo_pc != fail_value) {
hi_pc = GetAttributeHighPC(cu, lo_pc, fail_value,
check_specification_or_abstract_origin);
hi_pc = GetAttributeHighPC(cu, lo_pc, fail_value, check_elaborating_dies);
if (hi_pc != fail_value)
return true;
}
Expand All @@ -514,8 +501,7 @@ bool DWARFDebugInfoEntry::GetAttributeAddressRange(
}

DWARFRangeList DWARFDebugInfoEntry::GetAttributeAddressRanges(
DWARFUnit *cu, bool check_hi_lo_pc,
bool check_specification_or_abstract_origin) const {
DWARFUnit *cu, bool check_hi_lo_pc, bool check_elaborating_dies) const {

DWARFFormValue form_value;
if (GetAttributeValue(cu, DW_AT_ranges, form_value))
Expand All @@ -526,7 +512,7 @@ DWARFRangeList DWARFDebugInfoEntry::GetAttributeAddressRanges(
dw_addr_t lo_pc = LLDB_INVALID_ADDRESS;
dw_addr_t hi_pc = LLDB_INVALID_ADDRESS;
if (GetAttributeAddressRange(cu, lo_pc, hi_pc, LLDB_INVALID_ADDRESS,
check_specification_or_abstract_origin)) {
check_elaborating_dies)) {
if (lo_pc < hi_pc)
ranges.Append(DWARFRangeList::Entry(lo_pc, hi_pc - lo_pc));
}
Expand Down
57 changes: 29 additions & 28 deletions lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,44 +60,45 @@ class DWARFDebugInfoEntry {
return attrs;
}

dw_offset_t
GetAttributeValue(const DWARFUnit *cu, const dw_attr_t attr,
DWARFFormValue &formValue,
dw_offset_t *end_attr_offset_ptr = nullptr,
bool check_specification_or_abstract_origin = false) const;
dw_offset_t GetAttributeValue(const DWARFUnit *cu, const dw_attr_t attr,
DWARFFormValue &formValue,
dw_offset_t *end_attr_offset_ptr = nullptr,
bool check_elaborating_dies = false) const;

const char *GetAttributeValueAsString(
const DWARFUnit *cu, const dw_attr_t attr, const char *fail_value,
bool check_specification_or_abstract_origin = false) const;
const char *
GetAttributeValueAsString(const DWARFUnit *cu, const dw_attr_t attr,
const char *fail_value,
bool check_elaborating_dies = false) const;

uint64_t GetAttributeValueAsUnsigned(
const DWARFUnit *cu, const dw_attr_t attr, uint64_t fail_value,
bool check_specification_or_abstract_origin = false) const;
uint64_t
GetAttributeValueAsUnsigned(const DWARFUnit *cu, const dw_attr_t attr,
uint64_t fail_value,
bool check_elaborating_dies = false) const;
Copy link
Member

@Michael137 Michael137 Sep 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Out of curiosity, when do we not want to recurse through the specifications/origin/signature?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Most of the time, I'd say. DW_AT_specification says "this DIE is an out of line definition of the function declared in ". It normally does not list the name of the function because the name is already given on the other die, but it'd perfectly reasonable for GetName() on the first die to return the value from the other die. The same holds for most of the other attributes (with DW_AT_declaration and DW_AT_sibling being the most notable exceptions). I think this situation is similar enough to how DW_AT_signature works for types for it to get the same treatment.


std::optional<uint64_t> GetAttributeValueAsOptionalUnsigned(
const DWARFUnit *cu, const dw_attr_t attr,
bool check_specification_or_abstract_origin = false) const;
bool check_elaborating_dies = false) const;

DWARFDIE GetAttributeValueAsReference(
const DWARFUnit *cu, const dw_attr_t attr,
bool check_specification_or_abstract_origin = false) const;
DWARFDIE
GetAttributeValueAsReference(const DWARFUnit *cu, const dw_attr_t attr,
bool check_elaborating_dies = false) const;

uint64_t GetAttributeValueAsAddress(
const DWARFUnit *cu, const dw_attr_t attr, uint64_t fail_value,
bool check_specification_or_abstract_origin = false) const;
uint64_t
GetAttributeValueAsAddress(const DWARFUnit *cu, const dw_attr_t attr,
uint64_t fail_value,
bool check_elaborating_dies = false) const;

dw_addr_t
GetAttributeHighPC(const DWARFUnit *cu, dw_addr_t lo_pc, uint64_t fail_value,
bool check_specification_or_abstract_origin = false) const;
dw_addr_t GetAttributeHighPC(const DWARFUnit *cu, dw_addr_t lo_pc,
uint64_t fail_value,
bool check_elaborating_dies = false) const;

bool GetAttributeAddressRange(
const DWARFUnit *cu, dw_addr_t &lo_pc, dw_addr_t &hi_pc,
uint64_t fail_value,
bool check_specification_or_abstract_origin = false) const;
bool GetAttributeAddressRange(const DWARFUnit *cu, dw_addr_t &lo_pc,
dw_addr_t &hi_pc, uint64_t fail_value,
bool check_elaborating_dies = false) const;

DWARFRangeList GetAttributeAddressRanges(
DWARFUnit *cu, bool check_hi_lo_pc,
bool check_specification_or_abstract_origin = false) const;
DWARFRangeList
GetAttributeAddressRanges(DWARFUnit *cu, bool check_hi_lo_pc,
bool check_elaborating_dies = false) const;

const char *GetName(const DWARFUnit *cu) const;

Expand Down
45 changes: 45 additions & 0 deletions lldb/test/Shell/SymbolFile/DWARF/x86/type-unit-same-basename.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// Test that we can correctly disambiguate a nested type (where the outer type
// is in a type unit) from a non-nested type with the same basename. Failure to
// do so can cause us to think a type is a member of itself, which caused
// infinite recursion (crash) in the past.

// REQUIRES: lld

// RUN: %clang --target=x86_64-pc-linux -c %s -o %t-a.o -g -fdebug-types-section -flimit-debug-info -DFILE_A
// RUN: %clang --target=x86_64-pc-linux -c %s -o %t-b.o -g -fdebug-types-section -flimit-debug-info -DFILE_B
// RUN: ld.lld -z undefs %t-a.o %t-b.o -o %t
// RUN: %lldb %t -o "target variable x" -o exit | FileCheck %s

// CHECK: (lldb) target variable
// CHECK-NEXT: (const X) x = {
// CHECK-NEXT: NS::Outer::Struct = {
// CHECK-NEXT: x = 42
// CHECK-NEXT: o = (x = 47)
// CHECK-NEXT: y = 24
// CHECK-NEXT: }
// CHECK-NEXT: }

namespace NS {
struct Struct {
int x = 47;
virtual void anchor();
};
} // namespace NS

#ifdef FILE_A
namespace NS {
struct Outer {
struct Struct {
int x = 42;
NS::Struct o;
int y = 24;
};
};
} // namespace NS

struct X : NS::Outer::Struct {};
extern constexpr X x = {};
#endif
#ifdef FILE_B
void NS::Struct::anchor() {}
#endif
Loading