[lldb] Recurse through DW_AT_signature when looking for attributes #107241
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This allows e.g. DWARFDIE::GetName() to return the name of the type when looking at its declaration (which contains only DW_AT_declaration+DW_AT_signature). This is similar to how we recurse through DW_AT_specification when looking for a function name. Llvm dwarf parser has obtained the same functionality through llvm#99495. This fixes a bug where we would confuse a type like NS::Outer::Struct with NS::Struct (because NS::Outer (and its name) was in a type unit).
|
@llvm/pr-subscribers-lldb Author: Pavel Labath (labath) ChangesThis allows e.g. DWARFDIE::GetName() to return the name of the type when looking at its declaration (which contains only This fixes a bug where we would confuse a type like NS::Outer::Struct with NS::Struct (because NS::Outer (and its name) was in a type unit). Full diff: https://github.com/llvm/llvm-project/pull/107241.diff 4 Files Affected:
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
index 0a13c457a307ae..e94b3198c63d90 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
@@ -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}) {
if (DWARFDIE d = die.GetReferencedDIE(attr))
if (m_seen.insert(die.GetDIE()).second)
m_worklist.push_back(d);
@@ -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();
@@ -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)) {
- die = spec;
- continue;
- }
// Add this DIE's contribution at the end of the chain.
auto push_ctx = [&](CompilerContextKind kind, llvm::StringRef name) {
@@ -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)});
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
index e2660735ea7dec..77ef59d605c9c4 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
@@ -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);
@@ -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;
@@ -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;
}
@@ -425,10 +416,9 @@ 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;
}
@@ -436,10 +426,9 @@ uint64_t DWARFDebugInfoEntry::GetAttributeValueAsUnsigned(
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;
}
@@ -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;
}
@@ -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)
@@ -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;
}
@@ -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))
@@ -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));
}
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
index 3816c6500717af..20c07c95ec47b5 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
@@ -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;
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;
diff --git a/lldb/test/Shell/SymbolFile/DWARF/x86/type-unit-same-basename.cpp b/lldb/test/Shell/SymbolFile/DWARF/x86/type-unit-same-basename.cpp
new file mode 100644
index 00000000000000..f7f5a30aaba9ef
--- /dev/null
+++ b/lldb/test/Shell/SymbolFile/DWARF/x86/type-unit-same-basename.cpp
@@ -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
|
Michael137
reviewed
Sep 4, 2024
| @@ -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}) { | |||
There was a problem hiding this comment.
Did you mean for one of these to be DW_AT_signature?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Michael137
reviewed
Sep 4, 2024
| uint64_t | ||
| GetAttributeValueAsUnsigned(const DWARFUnit *cu, const dw_attr_t attr, | ||
| uint64_t fail_value, | ||
| bool check_elaborating_dies = false) const; |
There was a problem hiding this comment.
Out of curiosity, when do we not want to recurse through the specifications/origin/signature?
There was a problem hiding this comment.
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.
Michael137
reviewed
Sep 4, 2024
| @@ -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)) { | |||
There was a problem hiding this comment.
Could you elaborate on why we don't need this anymore?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Michael137
approved these changes
Sep 5, 2024
We couldn't make a meaningful test in the current form because the class is defined in the cpp file, and it's only user uses it in a context where DW_AT_signature doesn't make sense. We could move it to the header file, and then test it through its public API, but I'm somewhat reluctant to do that, as we don't have a use case for it. I guess that could be interpreted as a reason to not make this change to it (and that's the thing I'd probably do if you insisted on testing), but it's also a very trivial change that's very unlikely to go wrong. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This allows e.g. DWARFDIE::GetName() to return the name of the type when looking at its declaration (which contains only
DW_AT_declaration+DW_AT_signature). This is similar to how we recurse through DW_AT_specification when looking for a function name. Llvm dwarf parser has obtained the same functionality through #99495.
This fixes a bug where we would confuse a type like NS::Outer::Struct with NS::Struct (because NS::Outer (and its name) was in a type unit).