https://github.com/Michael137 updated https://github.com/llvm/llvm-project/pull/123261
>From 69b53a2a6290733ee90e4d75521f4c2fd6bd1401 Mon Sep 17 00:00:00 2001 From: Michael Buch <michaelbuc...@gmail.com> Date: Thu, 16 Jan 2025 15:16:36 +0000 Subject: [PATCH 1/8] [lldb][DWARF] Change GetAttributes to always visit current DIE before recursing `GetAttributes` returns all attributes on a given DIE, including any attributes in referenced `DW_AT_abstract_origin` or `DW_AT_specification`. However, if an attribute exists on both the referring DIE and the referenced DIE, the first one encountered will be the one that takes precendence when querying the returned `DWARFAttributes`. There is currently no good way of ensuring that an attribute of a definition doesn't get shadowed by one found on the declaration. One use-case where we don't want this to happen is for `DW_AT_object_pointer` (which can exist on both definitions and declarations, see https://github.com/llvm/llvm-project/pull/123089). This patch makes sure we visit the current DIE's attributes before following DIE references. I tried keeping as much of the original `GetAttributes` unchanged and just add an outer `GetAttributes` that keeps track of the DIEs we need to visit next. There's precendent for this iteration order in `llvm::DWARFDie::findRecursively` and also `lldb_private::ElaboratingDIEIterator`. We could use the latter to implement `GetAttributes`, though it also follows `DW_AT_signature` so I decided to leave it for follow-up. --- .../SymbolFile/DWARF/DWARFDebugInfoEntry.cpp | 82 ++- .../SymbolFile/DWARF/DWARFDebugInfoEntry.h | 25 +- .../SymbolFile/DWARF/DWARFDIETest.cpp | 602 ++++++++++++++++++ 3 files changed, 680 insertions(+), 29 deletions(-) diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp index 6d073411de876c..d26d302c33dd28 100644 --- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp +++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp @@ -281,22 +281,28 @@ bool DWARFDebugInfoEntry::GetDIENamesAndRanges( return !ranges.empty(); } -// Get all attribute values for a given DIE, including following any -// specification or abstract origin attributes and including those in the -// results. Any duplicate attributes will have the first instance take -// precedence (this can happen for declaration attributes). -void DWARFDebugInfoEntry::GetAttributes(DWARFUnit *cu, - DWARFAttributes &attributes, - Recurse recurse, - uint32_t curr_depth) const { - const auto *abbrevDecl = GetAbbreviationDeclarationPtr(cu); - if (!abbrevDecl) { - attributes.Clear(); - return; - } +bool DWARFDebugInfoEntry::GetAttributes( + DWARFUnit const *cu, llvm::SmallVector<DWARFDIE> &worklist, + llvm::SmallSet<DWARFDebugInfoEntry const *, 3> &seen, + DWARFAttributes &attributes) const { + assert(!worklist.empty()); + assert(cu); + + DWARFDIE current = worklist.pop_back_val(); + + const auto *entry = current.GetDIE(); + if (!entry) + return false; + + const auto *abbrevDecl = + entry->GetAbbreviationDeclarationPtr(current.GetCU()); + if (!abbrevDecl) + return false; const DWARFDataExtractor &data = cu->GetData(); - lldb::offset_t offset = GetFirstAttributeOffset(); + lldb::offset_t offset = current.GetDIE()->GetFirstAttributeOffset(); + + const bool is_first_die = seen.size() == 1; for (const auto &attribute : abbrevDecl->attributes()) { DWARFFormValue form_value(cu); @@ -309,10 +315,10 @@ void DWARFDebugInfoEntry::GetAttributes(DWARFUnit *cu, switch (attr) { case DW_AT_sibling: case DW_AT_declaration: - if (curr_depth > 0) { + if (seen.size() > 1 && !is_first_die) { // This attribute doesn't make sense when combined with the DIE that // references this DIE. We know a DIE is referencing this DIE because - // curr_depth is not zero + // we've visited more than one DIE already. break; } [[fallthrough]]; @@ -321,13 +327,12 @@ void DWARFDebugInfoEntry::GetAttributes(DWARFUnit *cu, break; } - if (recurse == Recurse::yes && - ((attr == DW_AT_specification) || (attr == DW_AT_abstract_origin))) { + if (attr == DW_AT_specification || attr == DW_AT_abstract_origin) { if (form_value.ExtractValue(data, &offset)) { - DWARFDIE spec_die = form_value.Reference(); - if (spec_die) - spec_die.GetDIE()->GetAttributes(spec_die.GetCU(), attributes, - recurse, curr_depth + 1); + if (DWARFDIE spec_die = form_value.Reference()) { + if (seen.insert(spec_die.GetDIE()).second) + worklist.push_back(spec_die); + } } } else { const dw_form_t form = form_value.Form(); @@ -339,6 +344,39 @@ void DWARFDebugInfoEntry::GetAttributes(DWARFUnit *cu, DWARFFormValue::SkipValue(form, data, &offset, cu); } } + + return true; +} + +DWARFAttributes DWARFDebugInfoEntry::GetAttributes(DWARFUnit *cu, + Recurse recurse) const { + // FIXME: use ElaboratingDIEIterator to follow specifications/abstract origins + // instead of maintaining our own worklist/seen list. + + DWARFAttributes attributes; + + llvm::SmallVector<DWARFDIE, 3> worklist; + worklist.emplace_back(cu, this); + + // Keep track if DIEs already seen to prevent infinite recursion. + // Value of '3' was picked for the same reason that + // DWARFDie::findRecursively does. + llvm::SmallSet<DWARFDebugInfoEntry const *, 3> seen; + seen.insert(this); + + while (!worklist.empty()) { + if (!GetAttributes(cu, worklist, seen, attributes)) { + attributes.Clear(); + break; + } + + // We visited the current DIE already and were asked not to check the + // rest of the worklist. So bail out. + if (recurse == Recurse::no) + break; + } + + return attributes; } // GetAttributeValue diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h index de6bbf1d527899..28381114634319 100644 --- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h +++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h @@ -52,12 +52,21 @@ class DWARFDebugInfoEntry { lldb::offset_t *offset_ptr); using Recurse = DWARFBaseDIE::Recurse; + + // Get all attribute values for a given DIE, including following any + // specification or abstract origin attributes and including those in the + // results. + // + // When following specifications/abstract origins, the attributes + // on the referring DIE are guaranteed to be visited before the attributes of + // the referenced DIE. + // + // \param[in] + // \param[in] + // + // \returns output may have duplicate entries DWARFAttributes GetAttributes(DWARFUnit *cu, - Recurse recurse = Recurse::yes) const { - DWARFAttributes attrs; - GetAttributes(cu, attrs, recurse, 0 /* curr_depth */); - return attrs; - } + Recurse recurse = Recurse::yes) const; dw_offset_t GetAttributeValue(const DWARFUnit *cu, const dw_attr_t attr, DWARFFormValue &formValue, @@ -180,8 +189,10 @@ class DWARFDebugInfoEntry { dw_tag_t m_tag = llvm::dwarf::DW_TAG_null; private: - void GetAttributes(DWARFUnit *cu, DWARFAttributes &attrs, Recurse recurse, - uint32_t curr_depth) const; + // Helper for the public \ref DWARFDebugInfoEntry::GetAttributes API. + bool GetAttributes(DWARFUnit const *cu, llvm::SmallVector<DWARFDIE> &worklist, + llvm::SmallSet<DWARFDebugInfoEntry const *, 3> &seen, + DWARFAttributes &attributes) const; }; } // namespace dwarf } // namespace lldb_private::plugin diff --git a/lldb/unittests/SymbolFile/DWARF/DWARFDIETest.cpp b/lldb/unittests/SymbolFile/DWARF/DWARFDIETest.cpp index 1e4c8f3ba07787..4f544656a6312c 100644 --- a/lldb/unittests/SymbolFile/DWARF/DWARFDIETest.cpp +++ b/lldb/unittests/SymbolFile/DWARF/DWARFDIETest.cpp @@ -394,3 +394,605 @@ TEST(DWARFDIETest, GetContextInFunction) { EXPECT_THAT(foo_struct_die.GetTypeLookupContext(), testing::ElementsAre(make_struct("struct_t"))); } + +struct GetAttributesTestFixture : public testing::TestWithParam<dw_attr_t> {}; + +TEST_P(GetAttributesTestFixture, TestGetAttributes_IterationOrder) { + // Tests that we accumulate all current DIE's attributes first + // before checking the attributes of the specification. + + const char *yamldata = R"( +--- !ELF +FileHeader: + Class: ELFCLASS64 + Data: ELFDATA2LSB + Type: ET_EXEC + Machine: EM_AARCH64 +DWARF: + debug_str: + - func + debug_abbrev: + - ID: 0 + Table: + - Code: 0x1 + Tag: DW_TAG_compile_unit + Children: DW_CHILDREN_yes + Attributes: + - Attribute: DW_AT_language + Form: DW_FORM_data2 + - Code: 0x2 + Tag: DW_TAG_subprogram + Children: DW_CHILDREN_no + Attributes: + - Attribute: DW_AT_high_pc + Form: DW_FORM_data4 + - Attribute: DW_AT_name + Form: DW_FORM_strp + - Attribute: DW_AT_declaration + Form: DW_FORM_flag_present + - Attribute: DW_AT_external + Form: DW_FORM_flag_present + - Attribute: DW_AT_low_pc + Form: DW_FORM_data4 + - Code: 0x3 + Tag: DW_TAG_subprogram + Children: DW_CHILDREN_no + Attributes: + - Attribute: DW_AT_high_pc + Form: DW_FORM_data4 + - Attribute: {0} + Form: DW_FORM_ref4 + - Attribute: DW_AT_low_pc + Form: DW_FORM_data4 + debug_info: + - Version: 5 + UnitType: DW_UT_compile + AddrSize: 8 + Entries: + +# DW_TAG_compile_unit +# DW_AT_language [DW_FORM_data2] (DW_LANG_C_plus_plus) + + - AbbrCode: 0x1 + Values: + - Value: 0x04 + +# DW_TAG_subprogram +# DW_AT_high_pc [DW_FORM_data4] +# DW_AT_name [DW_FORM_strp] ("func") +# DW_AT_low_pc [DW_FORM_data4] + - AbbrCode: 0x2 + Values: + - Value: 0xdeadbeef + - Value: 0x0 + - Value: 0x1 + - Value: 0x1 + - Value: 0xdeadbeef + +# DW_TAG_subprogram +# DW_AT_high_pc [DW_FORM_data4] +# DW_AT_specification [DW_FORM_ref4] ("func") +# DW_AT_low_pc [DW_FORM_data4] + - AbbrCode: 0x3 + Values: + - Value: 0xf00dcafe + - Value: 0xf + - Value: 0xf00dcafe + + - AbbrCode: 0x0 +... +)"; + YAMLModuleTester t(llvm::formatv(yamldata, GetParam()).str()); + + DWARFUnit *unit = t.GetDwarfUnit(); + ASSERT_NE(unit, nullptr); + const DWARFDebugInfoEntry *cu_entry = unit->DIE().GetDIE(); + ASSERT_EQ(cu_entry->Tag(), DW_TAG_compile_unit); + ASSERT_EQ(unit->GetDWARFLanguageType(), DW_LANG_C_plus_plus); + DWARFDIE cu_die(unit, cu_entry); + + auto declaration = cu_die.GetFirstChild(); + ASSERT_TRUE(declaration.IsValid()); + ASSERT_EQ(declaration.Tag(), DW_TAG_subprogram); + + auto definition = declaration.GetSibling(); + ASSERT_TRUE(definition.IsValid()); + ASSERT_EQ(definition.Tag(), DW_TAG_subprogram); + ASSERT_FALSE(definition.GetAttributeValueAsOptionalUnsigned(DW_AT_external)); + + auto attrs = definition.GetAttributes(DWARFDebugInfoEntry::Recurse::yes); + EXPECT_EQ(attrs.Size(), 7U); + + // Check that the attributes on the definition (that are also present + // on the declaration) take precedence. + for (auto attr : {DW_AT_low_pc, DW_AT_high_pc}) { + auto idx = attrs.FindAttributeIndex(attr); + EXPECT_NE(idx, UINT32_MAX); + + DWARFFormValue form_value; + auto success = attrs.ExtractFormValueAtIndex(idx, form_value); + EXPECT_TRUE(success); + + EXPECT_EQ(form_value.Unsigned(), 0xf00dcafe); + } +} + +TEST_P(GetAttributesTestFixture, TestGetAttributes_Cycle) { + // Tests that GetAttributes can deal with cycles in + // specifications/abstract origins. + + const char *yamldata = R"( +--- !ELF +FileHeader: + Class: ELFCLASS64 + Data: ELFDATA2LSB + Type: ET_EXEC + Machine: EM_AARCH64 +DWARF: + debug_abbrev: + - ID: 0 + Table: + - Code: 0x1 + Tag: DW_TAG_compile_unit + Children: DW_CHILDREN_yes + Attributes: + - Attribute: DW_AT_language + Form: DW_FORM_data2 + - Code: 0x2 + Tag: DW_TAG_subprogram + Children: DW_CHILDREN_no + Attributes: + - Attribute: {0} + Form: DW_FORM_ref4 + debug_info: + - Version: 5 + UnitType: DW_UT_compile + AddrSize: 8 + Entries: + + - AbbrCode: 0x1 + Values: + - Value: 0x04 + + - AbbrCode: 0x2 + Values: + - Value: 0x19 + + - AbbrCode: 0x2 + Values: + - Value: 0xf + + - AbbrCode: 0x2 + Values: + - Value: 0x14 + + - AbbrCode: 0x0 +... +)"; + YAMLModuleTester t(llvm::formatv(yamldata, GetParam()).str()); + + DWARFUnit *unit = t.GetDwarfUnit(); + ASSERT_NE(unit, nullptr); + const DWARFDebugInfoEntry *cu_entry = unit->DIE().GetDIE(); + ASSERT_EQ(cu_entry->Tag(), DW_TAG_compile_unit); + ASSERT_EQ(unit->GetDWARFLanguageType(), DW_LANG_C_plus_plus); + DWARFDIE cu_die(unit, cu_entry); + + auto func = cu_die.GetFirstChild(); + ASSERT_TRUE(func.IsValid()); + ASSERT_EQ(func.Tag(), DW_TAG_subprogram); + + auto attrs = func.GetAttributes(DWARFDebugInfoEntry::Recurse::yes); + EXPECT_EQ(attrs.Size(), 3U); + + // TODO: check that we do form a cycle with the DIEs +} + +TEST_P(GetAttributesTestFixture, + TestGetAttributes_SkipNonApplicableAttributes) { + // Tests that GetAttributes will omit attributes found through + // specifications/abstract origins which are not applicable. + + const char *yamldata = R"( +--- !ELF +FileHeader: + Class: ELFCLASS64 + Data: ELFDATA2LSB + Type: ET_EXEC + Machine: EM_AARCH64 +DWARF: + debug_str: + - func + debug_abbrev: + - ID: 0 + Table: + - Code: 0x1 + Tag: DW_TAG_compile_unit + Children: DW_CHILDREN_yes + Attributes: + - Attribute: DW_AT_language + Form: DW_FORM_data2 + - Code: 0x2 + Tag: DW_TAG_subprogram + Children: DW_CHILDREN_no + Attributes: + - Attribute: DW_AT_declaration + Form: DW_FORM_flag_present + - Attribute: DW_AT_name + Form: DW_FORM_strp + - Attribute: DW_AT_sibling + Form: DW_FORM_ref4 + - Code: 0x3 + Tag: DW_TAG_subprogram + Children: DW_CHILDREN_no + Attributes: + - Attribute: DW_AT_declaration + Form: DW_FORM_flag_present + - Attribute: {0} + Form: DW_FORM_ref4 + - Attribute: DW_AT_sibling + Form: DW_FORM_ref4 + debug_info: + - Version: 5 + UnitType: DW_UT_compile + AddrSize: 8 + Entries: + +# DW_TAG_compile_unit +# DW_AT_language [DW_FORM_data2] (DW_LANG_C_plus_plus) + + - AbbrCode: 0x1 + Values: + - Value: 0x04 + +# DW_TAG_subprogram +# DW_AT_declaration +# DW_AT_name [DW_FORM_strp] ("func") +# DW_AT_sibling + - AbbrCode: 0x2 + Values: + - Value: 0x1 + - Value: 0x0 + - Value: 0x18 + +# DW_TAG_subprogram +# DW_AT_declaration +# DW_AT_specification [DW_FORM_ref4] ("func") +# DW_AT_sibling + - AbbrCode: 0x3 + Values: + - Value: 0x1 + - Value: 0xf + - Value: 0xdeadbeef + + - AbbrCode: 0x0 +... +)"; + YAMLModuleTester t(llvm::formatv(yamldata, GetParam()).str()); + + DWARFUnit *unit = t.GetDwarfUnit(); + ASSERT_NE(unit, nullptr); + const DWARFDebugInfoEntry *cu_entry = unit->DIE().GetDIE(); + ASSERT_EQ(cu_entry->Tag(), DW_TAG_compile_unit); + ASSERT_EQ(unit->GetDWARFLanguageType(), DW_LANG_C_plus_plus); + DWARFDIE cu_die(unit, cu_entry); + + auto declaration = cu_die.GetFirstChild(); + ASSERT_TRUE(declaration.IsValid()); + ASSERT_EQ(declaration.Tag(), DW_TAG_subprogram); + + auto definition = declaration.GetSibling(); + ASSERT_TRUE(definition.IsValid()); + ASSERT_EQ(definition.Tag(), DW_TAG_subprogram); + + auto attrs = definition.GetAttributes(DWARFDebugInfoEntry::Recurse::yes); + EXPECT_EQ(attrs.Size(), 4U); + EXPECT_NE(attrs.FindAttributeIndex(DW_AT_name), UINT32_MAX); + EXPECT_NE(attrs.FindAttributeIndex(GetParam()), UINT32_MAX); + + auto sibling_idx = attrs.FindAttributeIndex(DW_AT_sibling); + EXPECT_NE(sibling_idx, UINT32_MAX); + + DWARFFormValue form_value; + auto success = attrs.ExtractFormValueAtIndex(sibling_idx, form_value); + ASSERT_TRUE(success); + + EXPECT_EQ(form_value.Unsigned(), 0xdeadbeef); +} + +TEST_P(GetAttributesTestFixture, TestGetAttributes_NoRecurse) { + // Tests that GetAttributes will not recurse if Recurse::No is passed to it. + + const char *yamldata = R"( +--- !ELF +FileHeader: + Class: ELFCLASS64 + Data: ELFDATA2LSB + Type: ET_EXEC + Machine: EM_AARCH64 +DWARF: + debug_str: + - func + debug_abbrev: + - ID: 0 + Table: + - Code: 0x1 + Tag: DW_TAG_compile_unit + Children: DW_CHILDREN_yes + Attributes: + - Attribute: DW_AT_language + Form: DW_FORM_data2 + - Code: 0x2 + Tag: DW_TAG_subprogram + Children: DW_CHILDREN_no + Attributes: + - Attribute: DW_AT_name + Form: DW_FORM_strp + - Code: 0x3 + Tag: DW_TAG_subprogram + Children: DW_CHILDREN_no + Attributes: + - Attribute: DW_AT_low_pc + Form: DW_FORM_data4 + - Attribute: {0} + Form: DW_FORM_ref4 + debug_info: + - Version: 5 + UnitType: DW_UT_compile + AddrSize: 8 + Entries: + +# DW_TAG_compile_unit +# DW_AT_language [DW_FORM_data2] (DW_LANG_C_plus_plus) + + - AbbrCode: 0x1 + Values: + - Value: 0x04 + +# DW_TAG_subprogram +# DW_AT_name [DW_FORM_strp] ("func") + - AbbrCode: 0x2 + Values: + - Value: 0x0 + +# DW_TAG_subprogram +# DW_AT_low_pc [DW_FORM_data4] +# DW_AT_specification [DW_FORM_ref4] + - AbbrCode: 0x3 + Values: + - Value: 0xdeadbeef + - Value: 0xf + + - AbbrCode: 0x0 +... +)"; + YAMLModuleTester t(llvm::formatv(yamldata, GetParam()).str()); + + DWARFUnit *unit = t.GetDwarfUnit(); + ASSERT_NE(unit, nullptr); + const DWARFDebugInfoEntry *cu_entry = unit->DIE().GetDIE(); + ASSERT_EQ(cu_entry->Tag(), DW_TAG_compile_unit); + ASSERT_EQ(unit->GetDWARFLanguageType(), DW_LANG_C_plus_plus); + DWARFDIE cu_die(unit, cu_entry); + + auto declaration = cu_die.GetFirstChild(); + ASSERT_TRUE(declaration.IsValid()); + ASSERT_EQ(declaration.Tag(), DW_TAG_subprogram); + + auto definition = declaration.GetSibling(); + ASSERT_TRUE(definition.IsValid()); + ASSERT_EQ(definition.Tag(), DW_TAG_subprogram); + + auto attrs = definition.GetAttributes(DWARFDebugInfoEntry::Recurse::no); + EXPECT_EQ(attrs.Size(), 2U); + EXPECT_EQ(attrs.FindAttributeIndex(DW_AT_name), UINT32_MAX); + EXPECT_NE(attrs.FindAttributeIndex(GetParam()), UINT32_MAX); + EXPECT_NE(attrs.FindAttributeIndex(DW_AT_low_pc), UINT32_MAX); +} + +TEST_P(GetAttributesTestFixture, TestGetAttributes_InvalidSpec) { + // Test that GetAttributes doesn't try following invalid + // specifications (but still add it to the list of attributes). + + const char *yamldata = R"( +--- !ELF +FileHeader: + Class: ELFCLASS64 + Data: ELFDATA2LSB + Type: ET_EXEC + Machine: EM_AARCH64 +DWARF: + debug_str: + - func + debug_abbrev: + - ID: 0 + Table: + - Code: 0x1 + Tag: DW_TAG_compile_unit + Children: DW_CHILDREN_yes + Attributes: + - Attribute: DW_AT_language + Form: DW_FORM_data2 + - Code: 0x2 + Tag: DW_TAG_subprogram + Children: DW_CHILDREN_no + Attributes: + - Attribute: DW_AT_name + Form: DW_FORM_strp + - Code: 0x3 + Tag: DW_TAG_subprogram + Children: DW_CHILDREN_no + Attributes: + - Attribute: {0} + Form: DW_FORM_ref4 + debug_info: + - Version: 5 + UnitType: DW_UT_compile + AddrSize: 8 + Entries: + +# DW_TAG_compile_unit +# DW_AT_language [DW_FORM_data2] (DW_LANG_C_plus_plus) + + - AbbrCode: 0x1 + Values: + - Value: 0x04 + +# DW_TAG_subprogram +# DW_AT_name [DW_FORM_strp] ("func") + - AbbrCode: 0x2 + Values: + - Value: 0x0 + +# DW_TAG_subprogram +# DW_AT_specification [DW_FORM_ref4] + - AbbrCode: 0x3 + Values: + - Value: 0xdeadbeef + + - AbbrCode: 0x0 +... +)"; + YAMLModuleTester t(llvm::formatv(yamldata, GetParam()).str()); + + DWARFUnit *unit = t.GetDwarfUnit(); + ASSERT_NE(unit, nullptr); + const DWARFDebugInfoEntry *cu_entry = unit->DIE().GetDIE(); + ASSERT_EQ(cu_entry->Tag(), DW_TAG_compile_unit); + ASSERT_EQ(unit->GetDWARFLanguageType(), DW_LANG_C_plus_plus); + DWARFDIE cu_die(unit, cu_entry); + + auto declaration = cu_die.GetFirstChild(); + ASSERT_TRUE(declaration.IsValid()); + ASSERT_EQ(declaration.Tag(), DW_TAG_subprogram); + + auto definition = declaration.GetSibling(); + ASSERT_TRUE(definition.IsValid()); + ASSERT_EQ(definition.Tag(), DW_TAG_subprogram); + + auto attrs = definition.GetAttributes(DWARFDebugInfoEntry::Recurse::yes); + EXPECT_EQ(attrs.Size(), 1U); + EXPECT_EQ(attrs.FindAttributeIndex(DW_AT_name), UINT32_MAX); + EXPECT_NE(attrs.FindAttributeIndex(GetParam()), UINT32_MAX); +} + +TEST(DWARFDIETest, TestGetAttributes_Worklist) { + // Test that GetAttributes will follow both the abstract origin + // and specification on a single DIE correctly (omitting non-applicable + // attributes in the process). + + // Contrived example where + // f1---> f2 --> f4 + // `-> f3 `-> f5 + // + const char *yamldata = R"( +--- !ELF +FileHeader: + Class: ELFCLASS64 + Data: ELFDATA2LSB + Type: ET_EXEC + Machine: EM_AARCH64 +DWARF: + debug_str: + - foo + - bar + debug_abbrev: + - ID: 0 + Table: + - Code: 0x1 + Tag: DW_TAG_compile_unit + Children: DW_CHILDREN_yes + Attributes: + - Attribute: DW_AT_language + Form: DW_FORM_data2 + - Code: 0x2 + Tag: DW_TAG_subprogram + Children: DW_CHILDREN_no + Attributes: + - Attribute: DW_AT_specification + Form: DW_FORM_ref4 + - Attribute: DW_AT_abstract_origin + Form: DW_FORM_ref4 + - Code: 0x3 + Tag: DW_TAG_subprogram + Children: DW_CHILDREN_no + Attributes: + - Attribute: DW_AT_declaration + Form: DW_FORM_flag_present + - Attribute: DW_AT_artificial + Form: DW_FORM_flag_present + + debug_info: + - Version: 5 + UnitType: DW_UT_compile + AddrSize: 8 + Entries: + + - AbbrCode: 0x1 + Values: + - Value: 0x04 + +# DW_TAG_subprogram ("f1") +# DW_AT_specification [DW_FORM_ref4] ("f2") +# DW_AT_abstract_origin [DW_FORM_ref4] ("f3") + - AbbrCode: 0x2 + Values: + - Value: 0x18 + - Value: 0x21 + +# DW_TAG_subprogram ("f2") +# DW_AT_specification [DW_FORM_ref4] ("f4") +# DW_AT_abstract_origin [DW_FORM_ref4] ("f5") + - AbbrCode: 0x2 + Values: + - Value: 0x22 + - Value: 0x23 + +# DW_TAG_subprogram ("f3") +# DW_AT_declaration [DW_FORM_flag_present] +# DW_AT_artificial [DW_FORM_flag_present] + - AbbrCode: 0x3 + Values: + - Value: 0x1 + - Value: 0x1 + +# DW_TAG_subprogram ("f4") +# DW_AT_declaration [DW_FORM_flag_present] +# DW_AT_artificial [DW_FORM_flag_present] + - AbbrCode: 0x3 + Values: + - Value: 0x1 + - Value: 0x1 + +# DW_TAG_subprogram ("f5") +# DW_AT_declaration [DW_FORM_flag_present] +# DW_AT_artificial [DW_FORM_flag_present] + - AbbrCode: 0x3 + Values: + - Value: 0x1 + - Value: 0x1 + + - AbbrCode: 0x0 +... +)"; + YAMLModuleTester t(yamldata); + + DWARFUnit *unit = t.GetDwarfUnit(); + ASSERT_NE(unit, nullptr); + const DWARFDebugInfoEntry *cu_entry = unit->DIE().GetDIE(); + ASSERT_EQ(cu_entry->Tag(), DW_TAG_compile_unit); + ASSERT_EQ(unit->GetDWARFLanguageType(), DW_LANG_C_plus_plus); + DWARFDIE cu_die(unit, cu_entry); + + auto f1 = cu_die.GetFirstChild(); + ASSERT_TRUE(f1.IsValid()); + ASSERT_EQ(f1.Tag(), DW_TAG_subprogram); + + auto attrs = f1.GetAttributes(DWARFDebugInfoEntry::Recurse::yes); + EXPECT_EQ(attrs.Size(), 7U); + EXPECT_EQ(attrs.FindAttributeIndex(DW_AT_declaration), UINT32_MAX); +} + +INSTANTIATE_TEST_SUITE_P(GetAttributeTests, GetAttributesTestFixture, + testing::Values(DW_AT_specification, + DW_AT_abstract_origin)); >From b48e5614bbcb7f44059b3e755c376f6d976d1389 Mon Sep 17 00:00:00 2001 From: Michael Buch <michaelbuc...@gmail.com> Date: Fri, 17 Jan 2025 00:30:50 +0000 Subject: [PATCH 2/8] fixup! fix comments; make function static --- .../SymbolFile/DWARF/DWARFDebugInfoEntry.cpp | 13 ++++--- .../SymbolFile/DWARF/DWARFDebugInfoEntry.h | 39 ++++++++++--------- .../SymbolFile/DWARF/DWARFDIETest.cpp | 2 - 3 files changed, 27 insertions(+), 27 deletions(-) diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp index d26d302c33dd28..ed8e12ea3038fe 100644 --- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp +++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp @@ -281,10 +281,11 @@ bool DWARFDebugInfoEntry::GetDIENamesAndRanges( return !ranges.empty(); } -bool DWARFDebugInfoEntry::GetAttributes( - DWARFUnit const *cu, llvm::SmallVector<DWARFDIE> &worklist, - llvm::SmallSet<DWARFDebugInfoEntry const *, 3> &seen, - DWARFAttributes &attributes) const { +/// Helper for the public \ref DWARFDebugInfoEntry::GetAttributes API. +static bool GetAttributes(DWARFUnit const *cu, + llvm::SmallVector<DWARFDIE> &worklist, + llvm::SmallSet<DWARFDebugInfoEntry const *, 3> &seen, + DWARFAttributes &attributes) { assert(!worklist.empty()); assert(cu); @@ -348,7 +349,7 @@ bool DWARFDebugInfoEntry::GetAttributes( return true; } -DWARFAttributes DWARFDebugInfoEntry::GetAttributes(DWARFUnit *cu, +DWARFAttributes DWARFDebugInfoEntry::GetAttributes(const DWARFUnit *cu, Recurse recurse) const { // FIXME: use ElaboratingDIEIterator to follow specifications/abstract origins // instead of maintaining our own worklist/seen list. @@ -365,7 +366,7 @@ DWARFAttributes DWARFDebugInfoEntry::GetAttributes(DWARFUnit *cu, seen.insert(this); while (!worklist.empty()) { - if (!GetAttributes(cu, worklist, seen, attributes)) { + if (!::GetAttributes(cu, worklist, seen, attributes)) { attributes.Clear(); break; } diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h index 28381114634319..16f1a5aede47d9 100644 --- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h +++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h @@ -53,19 +53,26 @@ class DWARFDebugInfoEntry { using Recurse = DWARFBaseDIE::Recurse; - // Get all attribute values for a given DIE, including following any - // specification or abstract origin attributes and including those in the - // results. - // - // When following specifications/abstract origins, the attributes - // on the referring DIE are guaranteed to be visited before the attributes of - // the referenced DIE. - // - // \param[in] - // \param[in] - // - // \returns output may have duplicate entries - DWARFAttributes GetAttributes(DWARFUnit *cu, + /// Get all attribute values for a given DIE, including following any + /// specification or abstract origin attributes and including those in the + /// results. + /// + /// When following specifications/abstract origins, the attributes + /// on the referring DIE are guaranteed to be visited before the attributes of + /// the referenced DIE. + /// + /// \param[in] cu DWARFUnit that this entry belongs to. + /// + /// \param[in] recurse If set to \c Recurse::yes, will include attributes + /// on DIEs referenced via \c DW_AT_specification and \c DW_AT_abstract_origin + /// (including across multiple levels of indirection). + /// + /// \returns DWARFAttributes that include all attributes found on this DIE + /// (and possibly referenced DIEs). Attributes may appear multiple times + /// (e.g., if a declaration and definition both specify the same attribute). + /// On failure, the returned DWARFAttributes will be empty. + /// + DWARFAttributes GetAttributes(const DWARFUnit *cu, Recurse recurse = Recurse::yes) const; dw_offset_t GetAttributeValue(const DWARFUnit *cu, const dw_attr_t attr, @@ -187,12 +194,6 @@ class DWARFDebugInfoEntry { /// A copy of the DW_TAG value so we don't have to go through the compile /// unit abbrev table dw_tag_t m_tag = llvm::dwarf::DW_TAG_null; - -private: - // Helper for the public \ref DWARFDebugInfoEntry::GetAttributes API. - bool GetAttributes(DWARFUnit const *cu, llvm::SmallVector<DWARFDIE> &worklist, - llvm::SmallSet<DWARFDebugInfoEntry const *, 3> &seen, - DWARFAttributes &attributes) const; }; } // namespace dwarf } // namespace lldb_private::plugin diff --git a/lldb/unittests/SymbolFile/DWARF/DWARFDIETest.cpp b/lldb/unittests/SymbolFile/DWARF/DWARFDIETest.cpp index 4f544656a6312c..718c733ae91051 100644 --- a/lldb/unittests/SymbolFile/DWARF/DWARFDIETest.cpp +++ b/lldb/unittests/SymbolFile/DWARF/DWARFDIETest.cpp @@ -584,8 +584,6 @@ TEST_P(GetAttributesTestFixture, TestGetAttributes_Cycle) { auto attrs = func.GetAttributes(DWARFDebugInfoEntry::Recurse::yes); EXPECT_EQ(attrs.Size(), 3U); - - // TODO: check that we do form a cycle with the DIEs } TEST_P(GetAttributesTestFixture, >From 638731dd5c63b534e51871e65ccc6d26e05ce8c2 Mon Sep 17 00:00:00 2001 From: Michael Buch <michaelbuc...@gmail.com> Date: Fri, 17 Jan 2025 00:38:22 +0000 Subject: [PATCH 3/8] fixup! add more assertions to cycle test --- .../SymbolFile/DWARF/DWARFDIETest.cpp | 48 +++++++++++++++++-- 1 file changed, 44 insertions(+), 4 deletions(-) diff --git a/lldb/unittests/SymbolFile/DWARF/DWARFDIETest.cpp b/lldb/unittests/SymbolFile/DWARF/DWARFDIETest.cpp index 718c733ae91051..3f61d1607073cf 100644 --- a/lldb/unittests/SymbolFile/DWARF/DWARFDIETest.cpp +++ b/lldb/unittests/SymbolFile/DWARF/DWARFDIETest.cpp @@ -520,6 +520,13 @@ TEST_P(GetAttributesTestFixture, TestGetAttributes_IterationOrder) { TEST_P(GetAttributesTestFixture, TestGetAttributes_Cycle) { // Tests that GetAttributes can deal with cycles in // specifications/abstract origins. + // + // Contrived example: + // + // func1 -> func3 + // ^ | + // | v + // +------func2 const char *yamldata = R"( --- !ELF @@ -578,12 +585,45 @@ TEST_P(GetAttributesTestFixture, TestGetAttributes_Cycle) { ASSERT_EQ(unit->GetDWARFLanguageType(), DW_LANG_C_plus_plus); DWARFDIE cu_die(unit, cu_entry); - auto func = cu_die.GetFirstChild(); - ASSERT_TRUE(func.IsValid()); - ASSERT_EQ(func.Tag(), DW_TAG_subprogram); + auto func1 = cu_die.GetFirstChild(); + ASSERT_TRUE(func1.IsValid()); + ASSERT_EQ(func1.Tag(), DW_TAG_subprogram); + + auto func2 = func1.GetSibling(); + ASSERT_TRUE(func2.IsValid()); + ASSERT_EQ(func2.Tag(), DW_TAG_subprogram); + + auto func3 = func2.GetSibling(); + ASSERT_TRUE(func3.IsValid()); + ASSERT_EQ(func3.Tag(), DW_TAG_subprogram); - auto attrs = func.GetAttributes(DWARFDebugInfoEntry::Recurse::yes); + auto attrs = func1.GetAttributes(DWARFDebugInfoEntry::Recurse::yes); EXPECT_EQ(attrs.Size(), 3U); + + // Confirm that the specifications do form a cycle. + { + DWARFFormValue form_value; + auto success = attrs.ExtractFormValueAtIndex(0, form_value); + ASSERT_TRUE(success); + + EXPECT_EQ(form_value.Reference(), func3); + } + + { + DWARFFormValue form_value; + auto success = attrs.ExtractFormValueAtIndex(1, form_value); + ASSERT_TRUE(success); + + EXPECT_EQ(form_value.Reference(), func2); + } + + { + DWARFFormValue form_value; + auto success = attrs.ExtractFormValueAtIndex(2, form_value); + ASSERT_TRUE(success); + + EXPECT_EQ(form_value.Reference(), func1); + } } TEST_P(GetAttributesTestFixture, >From df30635df13d0beea920f29b8bae95b37f8e6a02 Mon Sep 17 00:00:00 2001 From: Michael Buch <michaelbuc...@gmail.com> Date: Fri, 17 Jan 2025 09:44:22 +0000 Subject: [PATCH 4/8] fixup! fixup documentation --- .../source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp | 3 +++ lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h | 6 +++--- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp index ed8e12ea3038fe..de49399e993cca 100644 --- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp +++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp @@ -282,6 +282,9 @@ bool DWARFDebugInfoEntry::GetDIENamesAndRanges( } /// Helper for the public \ref DWARFDebugInfoEntry::GetAttributes API. +/// Adds all attributes of the DIE at the top of the \c worklist to the +/// \c attributes list. Specifcations and abstract origins are added +/// to the \c worklist if the referenced DIE has not been seen before. static bool GetAttributes(DWARFUnit const *cu, llvm::SmallVector<DWARFDIE> &worklist, llvm::SmallSet<DWARFDebugInfoEntry const *, 3> &seen, diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h index 16f1a5aede47d9..72aeb2743b1e20 100644 --- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h +++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h @@ -53,9 +53,9 @@ class DWARFDebugInfoEntry { using Recurse = DWARFBaseDIE::Recurse; - /// Get all attribute values for a given DIE, including following any - /// specification or abstract origin attributes and including those in the - /// results. + /// Get all attribute values for a given DIE, optionally following any + /// specifications and abstract origins and including their attributes + /// in the result too. /// /// When following specifications/abstract origins, the attributes /// on the referring DIE are guaranteed to be visited before the attributes of >From 941bd7095c1ab0917166fde26083b078af447c4b Mon Sep 17 00:00:00 2001 From: Michael Buch <michaelbuc...@gmail.com> Date: Fri, 17 Jan 2025 10:00:41 +0000 Subject: [PATCH 5/8] fixup! remove unnecessary condition Co-authored-by: Pavel Labath <pa...@labath.sk> --- lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp index de49399e993cca..7ca69a92f401cc 100644 --- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp +++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp @@ -319,7 +319,7 @@ static bool GetAttributes(DWARFUnit const *cu, switch (attr) { case DW_AT_sibling: case DW_AT_declaration: - if (seen.size() > 1 && !is_first_die) { + if (!is_first_die) { // This attribute doesn't make sense when combined with the DIE that // references this DIE. We know a DIE is referencing this DIE because // we've visited more than one DIE already. >From da7780cacaa6d35f174e4e3a5bab983361aa88a7 Mon Sep 17 00:00:00 2001 From: Michael Buch <michaelbuc...@gmail.com> Date: Fri, 17 Jan 2025 10:01:11 +0000 Subject: [PATCH 6/8] fixup! simplify GetAttributes loop Co-authored-by: Pavel Labath <pa...@labath.sk> --- .../Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp index 7ca69a92f401cc..e7aa2ca4d9e75e 100644 --- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp +++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp @@ -368,17 +368,12 @@ DWARFAttributes DWARFDebugInfoEntry::GetAttributes(const DWARFUnit *cu, llvm::SmallSet<DWARFDebugInfoEntry const *, 3> seen; seen.insert(this); - while (!worklist.empty()) { + do { if (!::GetAttributes(cu, worklist, seen, attributes)) { attributes.Clear(); break; } - - // We visited the current DIE already and were asked not to check the - // rest of the worklist. So bail out. - if (recurse == Recurse::no) - break; - } + } while (!worklist.empty() && recurse == Recurse::yes); return attributes; } >From 58021490956849e5a9c1f31607714328b34cc550 Mon Sep 17 00:00:00 2001 From: Michael Buch <michaelbuc...@gmail.com> Date: Fri, 17 Jan 2025 10:06:40 +0000 Subject: [PATCH 7/8] fixup! add assertion on 'seen' size --- .../source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp index e7aa2ca4d9e75e..2415637a3c6d5b 100644 --- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp +++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp @@ -289,8 +289,10 @@ static bool GetAttributes(DWARFUnit const *cu, llvm::SmallVector<DWARFDIE> &worklist, llvm::SmallSet<DWARFDebugInfoEntry const *, 3> &seen, DWARFAttributes &attributes) { - assert(!worklist.empty()); - assert(cu); + assert(!worklist.empty() && "Need at least one DIE to visit."); + assert(cu && "Need a valid CU to extract attributes."); + assert(seen.size() >= 1 && + "Need to have seen at least the currently visited entry."); DWARFDIE current = worklist.pop_back_val(); >From e78b47c9719dc3df9bde1ce9944f9e798a44b633 Mon Sep 17 00:00:00 2001 From: Michael Buch <michaelbuc...@gmail.com> Date: Fri, 17 Jan 2025 11:39:39 +0000 Subject: [PATCH 8/8] fixup! make sure to use the CU of the currently visited DIE --- .../Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp index 2415637a3c6d5b..c2edc52aa964f1 100644 --- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp +++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp @@ -285,20 +285,20 @@ bool DWARFDebugInfoEntry::GetDIENamesAndRanges( /// Adds all attributes of the DIE at the top of the \c worklist to the /// \c attributes list. Specifcations and abstract origins are added /// to the \c worklist if the referenced DIE has not been seen before. -static bool GetAttributes(DWARFUnit const *cu, - llvm::SmallVector<DWARFDIE> &worklist, +static bool GetAttributes(llvm::SmallVector<DWARFDIE> &worklist, llvm::SmallSet<DWARFDebugInfoEntry const *, 3> &seen, DWARFAttributes &attributes) { assert(!worklist.empty() && "Need at least one DIE to visit."); - assert(cu && "Need a valid CU to extract attributes."); assert(seen.size() >= 1 && "Need to have seen at least the currently visited entry."); DWARFDIE current = worklist.pop_back_val(); + const auto *cu = current.GetCU(); + assert(cu); + const auto *entry = current.GetDIE(); - if (!entry) - return false; + assert(entry); const auto *abbrevDecl = entry->GetAbbreviationDeclarationPtr(current.GetCU()); @@ -371,7 +371,7 @@ DWARFAttributes DWARFDebugInfoEntry::GetAttributes(const DWARFUnit *cu, seen.insert(this); do { - if (!::GetAttributes(cu, worklist, seen, attributes)) { + if (!::GetAttributes(worklist, seen, attributes)) { attributes.Clear(); break; } _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
