[llvm] [NFC][DebugInfo] Allow single instruction sequence in line table to make symbolication of merged functions easier (PR #154851)
Peter Rong via llvm-commits
llvm-commits at lists.llvm.org
Fri Aug 22 14:47:19 PDT 2025
https://github.com/DataCorrupted updated https://github.com/llvm/llvm-project/pull/154851
>From d1d3366602276009068aa2086ae0d9bc6ccbd1bf Mon Sep 17 00:00:00 2001
From: Peter Rong <PeterRong at meta.com>
Date: Thu, 21 Aug 2025 15:14:18 -0700
Subject: [PATCH 1/6] [DebugLine] Sequence HighPC should be LastRow.address +
MinInstLength
---
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp | 2 +-
.../DebugInfo/DWARF/DWARFDebugLineTest.cpp | 110 ++++++++++++++++++
2 files changed, 111 insertions(+), 1 deletion(-)
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp b/llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
index 08b58669b3eb3..052b54b41dca0 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
@@ -581,7 +581,7 @@ void DWARFDebugLine::ParsingState::appendRowToMatrix() {
LineTable->appendRow(Row);
if (Row.EndSequence) {
// Record the end of instruction sequence.
- Sequence.HighPC = Row.Address.Address;
+ Sequence.HighPC = Row.Address.Address + LineTable->Prologue.MinInstLength;
Sequence.LastRowIndex = RowNumber + 1;
Sequence.SectionIndex = Row.Address.SectionIndex;
if (Sequence.isValid())
diff --git a/llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp b/llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp
index 2fe52600df923..b262539b37033 100644
--- a/llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp
+++ b/llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp
@@ -2148,4 +2148,114 @@ TEST_F(DebugLineBasicFixture, LookupAddressRangeWithStmtSequenceOffset) {
EXPECT_EQ(Rows[1], 3u);
}
}
+
+/// Test that HighPC is now inclusive in Sequence containsPC checks.
+//
+/// With the old exclusive HighPC logic, lookups at the last row's address would
+/// fail. With the new inclusive HighPC logic, lookups at the HighPC address
+/// should succeed.
+TEST_F(DebugLineBasicFixture, HighPCInclusiveLookup) {
+ if (!setupGenerator())
+ GTEST_SKIP();
+
+ // Create a line table with a sequence that covers addresses [0x1000, 0x1010]
+ // We'll test lookups at various addresses including the HighPC (0x1010)
+ LineTable < = Gen->addLineTable();
+
+ // Set address to 0x1000 and create first row
+ LT.addExtendedOpcode(9, DW_LNE_set_address, {{0x1000U, LineTable::Quad}});
+ LT.addStandardOpcode(DW_LNS_copy, {});
+
+ // Advance to 0x1008 and create second row
+ LT.addStandardOpcode(DW_LNS_advance_pc, {{0x8, LineTable::ULEB}});
+ LT.addStandardOpcode(DW_LNS_copy, {});
+
+ // Advance to 0x1010 and create third row (this will be the HighPC)
+ LT.addStandardOpcode(DW_LNS_advance_pc, {{0x8, LineTable::ULEB}});
+ LT.addStandardOpcode(DW_LNS_copy, {});
+
+ // End the sequence - this makes HighPC = 0x1010
+ LT.addExtendedOpcode(1, DW_LNE_end_sequence, {});
+
+ generate();
+
+ // Parse the line table
+ auto ExpectedLineTable = Line.getOrParseLineTable(LineData, 0, *Context,
+ nullptr, RecordRecoverable);
+ ASSERT_THAT_EXPECTED(ExpectedLineTable, Succeeded());
+ const auto *Table = *ExpectedLineTable;
+
+ // Verify we have one sequence with the expected address range
+ ASSERT_EQ(Table->Sequences.size(), 1u);
+ const auto &Seq = Table->Sequences[0];
+ EXPECT_EQ(Seq.LowPC, 0x1000U);
+ EXPECT_EQ(Seq.HighPC, 0x1011U);
+
+ auto LastRow = Table->Rows.back();
+
+ // Lookup the last Row - this is the key test for inclusive HighPC
+ // With the old exclusive logic, this would return UnknownRowIndex
+ // With the new inclusive logic, this should succeed
+ {
+ uint32_t RowIndex = Table->lookupAddress(
+ {LastRow.Address.Address, object::SectionedAddress::UndefSection});
+ // EXPECT_NE(RowIndex, Table->UnknownRowIndex) << "Lookup at HighPC found no
+ // row";
+ EXPECT_EQ(RowIndex, Table->Rows.size() - 2)
+ << "Lookup at HighPC should find the second to the last row";
+ }
+}
+
+/// Test that a sequence with only one row works correctly with inclusive
+/// HighPC. This is an important edge case where LowPC == HighPC, and the
+/// inclusive HighPC logic should still allow lookups at that single address.
+TEST_F(DebugLineBasicFixture, SingleInstSequenceInclusiveHighPC) {
+ if (!setupGenerator())
+ GTEST_SKIP();
+
+ // Create a line table with a sequence that has only one row
+ // This creates a sequence where LowPC == HighPC
+ LineTable < = Gen->addLineTable();
+
+ // Set address to 0x3000 and create the only row
+ LT.addExtendedOpcode(9, DW_LNE_set_address, {{0x3000U, LineTable::Quad}});
+ LT.addStandardOpcode(DW_LNS_copy, {});
+
+ // End the sequence immediately - this makes LowPC == HighPC == 0x3000
+ LT.addExtendedOpcode(1, DW_LNE_end_sequence, {});
+
+ generate();
+
+ // Parse the line table
+ auto ExpectedLineTable = Line.getOrParseLineTable(LineData, 0, *Context,
+ nullptr, RecordRecoverable);
+ ASSERT_THAT_EXPECTED(ExpectedLineTable, Succeeded());
+ const auto *Table = *ExpectedLineTable;
+
+ // Verify we have one sequence with LowPC == HighPC
+ ASSERT_EQ(Table->Sequences.size(), 1u);
+ const auto &Seq = Table->Sequences[0];
+ EXPECT_EQ(Seq.LowPC, 0x3000U);
+ EXPECT_EQ(Seq.HighPC, 0x3001U);
+
+ // Verify we have exactly one row (plus the end_sequence row)
+ EXPECT_EQ(Table->Rows.size(), 2u);
+ EXPECT_EQ(Table->Rows[0].Address.Address, 0x3000U);
+ EXPECT_TRUE(Table->Rows[1].EndSequence);
+
+ auto LastRow = Table->Rows.back();
+
+ // Lookup the last Row - this is the key test for inclusive HighPC
+ // With the old exclusive logic, this would return UnknownRowIndex
+ // With the new inclusive logic, this should succeed
+ {
+ uint32_t RowIndex = Table->lookupAddress(
+ {LastRow.Address.Address, object::SectionedAddress::UndefSection});
+ // EXPECT_NE(RowIndex, Table->UnknownRowIndex) << "Lookup at HighPC found no
+ // row";
+ EXPECT_EQ(RowIndex, Table->Rows.size() - 2)
+ << "Lookup at HighPC should find the last row";
+ }
+}
+
} // end anonymous namespace
>From b26ae4d6de538ed087b9312152eba505ad071e8a Mon Sep 17 00:00:00 2001
From: Peter Rong <PeterRong at meta.com>
Date: Thu, 21 Aug 2025 15:20:11 -0700
Subject: [PATCH 2/6] Comment out my change
---
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp b/llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
index 052b54b41dca0..7700a0d32d4d8 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
@@ -581,7 +581,10 @@ void DWARFDebugLine::ParsingState::appendRowToMatrix() {
LineTable->appendRow(Row);
if (Row.EndSequence) {
// Record the end of instruction sequence.
- Sequence.HighPC = Row.Address.Address + LineTable->Prologue.MinInstLength;
+ Sequence.HighPC = Row.Address.Address;
+ // Proposed change, without this the added test will fail. With this
+ // GSYMTest/TestDWARFNoLines will fail Sequence.HighPC = Row.Address.Address
+ // + LineTable->Prologue.MinInstLength
Sequence.LastRowIndex = RowNumber + 1;
Sequence.SectionIndex = Row.Address.SectionIndex;
if (Sequence.isValid())
>From 17d24cae549d8480839319e6c8702ee6f48103cf Mon Sep 17 00:00:00 2001
From: Peter Rong <PeterRong at meta.com>
Date: Thu, 21 Aug 2025 15:21:49 -0700
Subject: [PATCH 3/6] using if 0
---
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp b/llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
index 7700a0d32d4d8..9ba302666e2b4 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
@@ -582,9 +582,11 @@ void DWARFDebugLine::ParsingState::appendRowToMatrix() {
if (Row.EndSequence) {
// Record the end of instruction sequence.
Sequence.HighPC = Row.Address.Address;
+#if 0
// Proposed change, without this the added test will fail. With this
- // GSYMTest/TestDWARFNoLines will fail Sequence.HighPC = Row.Address.Address
- // + LineTable->Prologue.MinInstLength
+ // GSYMTest/TestDWARFNoLines will fail
+ Sequence.HighPC = Row.Address.Address + LineTable->Prologue.MinInstLength;
+#endif
Sequence.LastRowIndex = RowNumber + 1;
Sequence.SectionIndex = Row.Address.SectionIndex;
if (Sequence.isValid())
>From f61ed8f940b57767d7b247789d5931af8743ecb3 Mon Sep 17 00:00:00 2001
From: Peter Rong <PeterRong at meta.com>
Date: Thu, 21 Aug 2025 15:50:23 -0700
Subject: [PATCH 4/6] update test
---
.../DebugInfo/DWARF/DWARFDebugLineTest.cpp | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp b/llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp
index b262539b37033..62c2f18266072 100644
--- a/llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp
+++ b/llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp
@@ -2154,7 +2154,7 @@ TEST_F(DebugLineBasicFixture, LookupAddressRangeWithStmtSequenceOffset) {
/// With the old exclusive HighPC logic, lookups at the last row's address would
/// fail. With the new inclusive HighPC logic, lookups at the HighPC address
/// should succeed.
-TEST_F(DebugLineBasicFixture, HighPCInclusiveLookup) {
+TEST_F(DebugLineBasicFixture, LookupLastRow) {
if (!setupGenerator())
GTEST_SKIP();
@@ -2189,7 +2189,7 @@ TEST_F(DebugLineBasicFixture, HighPCInclusiveLookup) {
ASSERT_EQ(Table->Sequences.size(), 1u);
const auto &Seq = Table->Sequences[0];
EXPECT_EQ(Seq.LowPC, 0x1000U);
- EXPECT_EQ(Seq.HighPC, 0x1011U);
+ EXPECT_EQ(Seq.HighPC, 0x1010U);
auto LastRow = Table->Rows.back();
@@ -2199,8 +2199,7 @@ TEST_F(DebugLineBasicFixture, HighPCInclusiveLookup) {
{
uint32_t RowIndex = Table->lookupAddress(
{LastRow.Address.Address, object::SectionedAddress::UndefSection});
- // EXPECT_NE(RowIndex, Table->UnknownRowIndex) << "Lookup at HighPC found no
- // row";
+ // Both last and the second to the last row have the same PC, so the second to the last should pop up first.
EXPECT_EQ(RowIndex, Table->Rows.size() - 2)
<< "Lookup at HighPC should find the second to the last row";
}
@@ -2209,7 +2208,7 @@ TEST_F(DebugLineBasicFixture, HighPCInclusiveLookup) {
/// Test that a sequence with only one row works correctly with inclusive
/// HighPC. This is an important edge case where LowPC == HighPC, and the
/// inclusive HighPC logic should still allow lookups at that single address.
-TEST_F(DebugLineBasicFixture, SingleInstSequenceInclusiveHighPC) {
+TEST_F(DebugLineBasicFixture, SingleInstSeq) {
if (!setupGenerator())
GTEST_SKIP();
@@ -2236,7 +2235,7 @@ TEST_F(DebugLineBasicFixture, SingleInstSequenceInclusiveHighPC) {
ASSERT_EQ(Table->Sequences.size(), 1u);
const auto &Seq = Table->Sequences[0];
EXPECT_EQ(Seq.LowPC, 0x3000U);
- EXPECT_EQ(Seq.HighPC, 0x3001U);
+ EXPECT_EQ(Seq.HighPC, 0x3000U);
// Verify we have exactly one row (plus the end_sequence row)
EXPECT_EQ(Table->Rows.size(), 2u);
@@ -2251,8 +2250,6 @@ TEST_F(DebugLineBasicFixture, SingleInstSequenceInclusiveHighPC) {
{
uint32_t RowIndex = Table->lookupAddress(
{LastRow.Address.Address, object::SectionedAddress::UndefSection});
- // EXPECT_NE(RowIndex, Table->UnknownRowIndex) << "Lookup at HighPC found no
- // row";
EXPECT_EQ(RowIndex, Table->Rows.size() - 2)
<< "Lookup at HighPC should find the last row";
}
>From a77eca3c8b67fec624ec021d0d93f4ca128feed1 Mon Sep 17 00:00:00 2001
From: Peter Rong <PeterRong at meta.com>
Date: Thu, 21 Aug 2025 16:57:19 -0700
Subject: [PATCH 5/6] Test
---
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp b/llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
index 9ba302666e2b4..3cb7a6f6b91d1 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
@@ -582,7 +582,7 @@ void DWARFDebugLine::ParsingState::appendRowToMatrix() {
if (Row.EndSequence) {
// Record the end of instruction sequence.
Sequence.HighPC = Row.Address.Address;
-#if 0
+#if 1
// Proposed change, without this the added test will fail. With this
// GSYMTest/TestDWARFNoLines will fail
Sequence.HighPC = Row.Address.Address + LineTable->Prologue.MinInstLength;
>From 18ad6e28ee5a2a22e994e33569632a58e2c06a5c Mon Sep 17 00:00:00 2001
From: Peter Rong <PeterRong at meta.com>
Date: Fri, 22 Aug 2025 14:46:45 -0700
Subject: [PATCH 6/6] supress assert
Signed-off-by: Peter Rong <PeterRong at meta.com>
---
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp b/llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
index 3cb7a6f6b91d1..7d4f4559253c9 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
@@ -1316,7 +1316,7 @@ uint32_t DWARFDebugLine::LineTable::findRowInSeq(
RowIter FirstRow = Rows.begin() + Seq.FirstRowIndex;
RowIter LastRow = Rows.begin() + Seq.LastRowIndex;
assert(FirstRow->Address.Address <= Row.Address.Address &&
- Row.Address.Address < LastRow[-1].Address.Address);
+ Row.Address.Address <= LastRow[-1].Address.Address);
RowIter RowPos = std::upper_bound(FirstRow + 1, LastRow - 1, Row,
DWARFDebugLine::Row::orderByAddress) -
1;
More information about the llvm-commits
mailing list