[llvm] [NFC][DebugInfo] Sequence HighPC should be LastRow.address + MinInstLength (PR #154851)
Peter Rong via llvm-commits
llvm-commits at lists.llvm.org
Thu Aug 21 15:50:54 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/4] [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/4] 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/4] 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/4] 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";
}
More information about the llvm-commits
mailing list