[DWARFLinker] Fix DW_AT_LLVM_stmt_sequence attributes patched to wrong offsets#178486
Open
[DWARFLinker] Fix DW_AT_LLVM_stmt_sequence attributes patched to wrong offsets#178486
Conversation
Member
|
@llvm/pr-subscribers-debuginfo Author: None (alx32) ChangesThis fixes a bug where The root cause is that when sequences get reordered or merged, a row that was originally a sequence start may end up in the middle of a different sequence in the output. The old code was mapping the original row index directly to its output position, but that output position might not have a The fix builds a mapping from each output row to its containing sequence's start row. When patching
This ensures all Testing wise:
Results:
Patch is 99.22 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/178486.diff 2 Files Affected:
diff --git a/llvm/lib/DWARFLinker/Classic/DWARFLinker.cpp b/llvm/lib/DWARFLinker/Classic/DWARFLinker.cpp
index daf3788639451..08921ee3e6cf2 100644
--- a/llvm/lib/DWARFLinker/Classic/DWARFLinker.cpp
+++ b/llvm/lib/DWARFLinker/Classic/DWARFLinker.cpp
@@ -2448,6 +2448,19 @@ void DWARFLinker::DIECloner::generateLineTableForUnit(CompileUnit &Unit) {
insertLineSequence(Seq, OutputRows);
}
+ // Recompute isStartSeqInOutput based on the final row ordering.
+ // A row is a sequence start (will have DW_LNE_set_address emitted) iff:
+ // 1. It's the first row, OR
+ // 2. The previous row has EndSequence = 1
+ // This is necessary because insertLineSequence may merge sequences when
+ // an EndSequence row is replaced by the start of a new sequence, which
+ // removes the EndSequence marker and invalidates the original flag.
+ if (!OutputRows.empty()) {
+ OutputRows[0].isStartSeqInOutput = true;
+ for (size_t i = 1; i < OutputRows.size(); ++i)
+ OutputRows[i].isStartSeqInOutput = OutputRows[i - 1].Row.EndSequence;
+ }
+
// Materialize the tracked rows into final DWARFDebugLine::Row objects.
LineTable.Rows.clear();
LineTable.Rows.reserve(OutputRows.size());
@@ -2478,10 +2491,24 @@ void DWARFLinker::DIECloner::generateLineTableForUnit(CompileUnit &Unit) {
if (!LT->Rows.empty())
constructSeqOffsettoOrigRowMapping(Unit, *LT, SeqOffToOrigRow);
- // Create a map of original row indices to new row indices.
- DenseMap<size_t, size_t> OrigRowToNewRow;
- for (size_t i = 0; i < OutputRows.size(); ++i)
- OrigRowToNewRow[OutputRows[i].OriginalRowIndex] = i;
+ // Build two maps to handle stmt_sequence patching:
+ // 1. OrigRowToOutputRow: maps original row indices to output row
+ // indices (for all rows, not just sequence starts)
+ // 2. OutputRowToSeqStart: maps each output row index to its sequence
+ // start's output row index
+ DenseMap<size_t, size_t> OrigRowToOutputRow;
+ std::vector<size_t> OutputRowToSeqStart(OutputRows.size());
+
+ size_t CurrentSeqStart = 0;
+ for (size_t i = 0; i < OutputRows.size(); ++i) {
+ // Track the current sequence start
+ if (OutputRows[i].isStartSeqInOutput)
+ CurrentSeqStart = i;
+ OutputRowToSeqStart[i] = CurrentSeqStart;
+
+ // Map original row index to output row index
+ OrigRowToOutputRow[OutputRows[i].OriginalRowIndex] = i;
+ }
// Patch DW_AT_LLVM_stmt_sequence attributes in the compile unit DIE
// with the correct offset into the .debug_line section.
@@ -2500,21 +2527,27 @@ void DWARFLinker::DIECloner::generateLineTableForUnit(CompileUnit &Unit) {
}
size_t OrigRowIndex = OrigRowIter->second;
- // 2. Get the new row index from the original row index.
- auto NewRowIter = OrigRowToNewRow.find(OrigRowIndex);
- if (NewRowIter == OrigRowToNewRow.end()) {
- // If the original row index is not found in the map, update the
- // stmt_sequence attribute to the 'invalid offset' magic value.
+ // 2. Find the output row for this original row.
+ auto OutputRowIter = OrigRowToOutputRow.find(OrigRowIndex);
+ if (OutputRowIter == OrigRowToOutputRow.end()) {
+ // Row was dropped during linking.
StmtSeq.set(InvalidOffset);
continue;
}
+ size_t OutputRowIdx = OutputRowIter->second;
+
+ // 3. Find the sequence start for this output row.
+ // If the original row was a sequence start but got merged into
+ // another sequence, this finds the correct sequence start.
+ size_t SeqStartIdx = OutputRowToSeqStart[OutputRowIdx];
- // 3. Get the offset of the new row in the output .debug_line section.
- assert(NewRowIter->second < OutputRowOffsets.size() &&
- "New row index out of bounds");
- uint64_t NewStmtSeqOffset = OutputRowOffsets[NewRowIter->second];
+ // 4. Get the offset of the sequence start in the output .debug_line
+ // section. This offset points to the DW_LNE_set_address opcode.
+ assert(SeqStartIdx < OutputRowOffsets.size() &&
+ "Sequence start index out of bounds");
+ uint64_t NewStmtSeqOffset = OutputRowOffsets[SeqStartIdx];
- // 4. Patch the stmt_list attribute with the new offset.
+ // 5. Patch the stmt_sequence attribute with the new offset.
StmtSeq.set(NewStmtSeqOffset);
}
}
diff --git a/llvm/test/tools/dsymutil/AArch64/stmt-seq-macho.test b/llvm/test/tools/dsymutil/AArch64/stmt-seq-macho.test
index db223cda43247..974ba6f7dffe2 100644
--- a/llvm/test/tools/dsymutil/AArch64/stmt-seq-macho.test
+++ b/llvm/test/tools/dsymutil/AArch64/stmt-seq-macho.test
@@ -1,72 +1,95 @@
## Test that verifies DW_AT_LLVM_stmt_sequence attributes are correctly patched in the dSYM
+## even when line table sequences need to be reordered due to address ordering.
+##
+## This test uses two object files (a.o and b.o) linked with b.o first so that
+## b's functions get lower addresses than a's functions. When dsymutil processes
+## the debug info, it must correctly handle the case where line table sequences
+## from different compilation units need to be interleaved by address.
# RUN: rm -rf %t && split-file %s %t && cd %t
# RUN: yaml2obj %t/stmt_seq_macho.exe.yaml -o %t/stmt_seq_macho.exe
-# RUN: yaml2obj %t/stmt_seq_macho.o.yaml -o %t/stmt_seq_macho.o
+# RUN: yaml2obj %t/a.o.yaml -o %t/a.o
+# RUN: yaml2obj %t/b.o.yaml -o %t/b.o
# RUN: dsymutil --flat --verify-dwarf=none -oso-prepend-path %t %t/stmt_seq_macho.exe -o %t/stmt_seq_macho.dSYM
-# RUN: llvm-dwarfdump --debug-info --debug-line -v %t/stmt_seq_macho.dSYM | sort | FileCheck %s -check-prefix=CHECK_DSYM
# RUN: llvm-dwarfdump --debug-info --debug-line -v %t/stmt_seq_macho.dSYM > %t/stmt_seq_macho.dSYM.txt
-# RUN: cat %t/stmt_seq_macho.dSYM.txt | sort | FileCheck %s -check-prefix=CHECK_DSYM
-# RUN: cat %t/stmt_seq_macho.dSYM.txt | FileCheck %s -check-prefix=CHECK_NO_INVALID_OFFSET
-# RUN: cat stmt_seq_macho.dSYM.txt | grep DW_AT_LLVM_stmt_sequence | sort | uniq -d | wc -l | FileCheck %s -check-prefix=CHECK_NO_DUPLICATES
-# CHECK_NO_DUPLICATES: 0
-# CHECK_NO_INVALID_OFFSET-NOT: DW_AT_LLVM_stmt_sequence{{.*}}0xfffffff
-# CHECK_DSYM: DW_AT_LLVM_stmt_sequence [DW_FORM_sec_offset] ([[OFFSET1:(0x[0-9a-f]+)]])
-# CHECK_DSYM: DW_AT_LLVM_stmt_sequence [DW_FORM_sec_offset] ([[OFFSET2:(0x[0-9a-f]+)]])
-# CHECK_DSYM: DW_AT_LLVM_stmt_sequence [DW_FORM_sec_offset] ([[OFFSET3:(0x[0-9a-f]+)]])
-# CHECK_DSYM: DW_AT_LLVM_stmt_sequence [DW_FORM_sec_offset] ([[OFFSET4:(0x[0-9a-f]+)]])
+## Verify that all stmt_sequence offsets point to DW_LNE_set_address opcodes
+## Extract unique stmt_sequence offsets and verify each appears as a set_address offset
+# RUN: cat %t/stmt_seq_macho.dSYM.txt | grep "DW_AT_LLVM_stmt_sequence" | \
+# RUN: sed 's/.*(\(0x[0-9a-f]*\)).*/\1/' | sort -u > %t/stmt_offsets.txt
+# RUN: cat %t/stmt_seq_macho.dSYM.txt | grep "DW_LNE_set_address" | \
+# RUN: sed 's/^\(0x[0-9a-f]*\):.*/\1/' | sort -u > %t/setaddr_offsets.txt
+# RUN: comm -23 %t/stmt_offsets.txt %t/setaddr_offsets.txt > %t/bad_offsets.txt
+# RUN: test ! -s %t/bad_offsets.txt
-# CHECK_DSYM: [[OFFSET1]]: 00 DW_LNE_set_address
-# CHECK_DSYM: [[OFFSET2]]: 00 DW_LNE_set_address
-# CHECK_DSYM: [[OFFSET3]]: 00 DW_LNE_set_address
-# CHECK_DSYM: [[OFFSET4]]: 00 DW_LNE_set_address
+## Ensure no sentinel/invalid offsets (0xffffffff indicates broken patching)
+# RUN: cat %t/stmt_seq_macho.dSYM.txt | FileCheck %s -check-prefix=CHECK_NO_INVALID_OFFSET
+# CHECK_NO_INVALID_OFFSET-NOT: DW_AT_LLVM_stmt_sequence{{.*}}0xfffffff
#--- stmt_seq_macho.cpp
+// This file is split into a.cpp and b.cpp by the gen script.
+// The functions are compiled into separate object files and linked
+// with b.o first, so function_b gets a lower address than function_a.
+// This triggers reordering in dsymutil's line table processing.
+
+#--- a.cpp
+// Functions in a.o - will have HIGHER addresses in the final executable
#define ATTRIB extern "C" __attribute__((noinline))
-ATTRIB int function1_copy1(int a) {
- return ++a;
+
+ATTRIB int function_a1(int a) {
+ int b = a + 1;
+ return b * 2;
}
-ATTRIB int function3_copy1(int a) {
+ATTRIB int function_a2(int a) {
int b = a + 3;
return b + 1;
}
-ATTRIB int function2_copy1(int a) {
- return a - 22;
-}
+#--- b.cpp
+// Functions in b.o - will have LOWER addresses in the final executable
+#define ATTRIB extern "C" __attribute__((noinline))
-ATTRIB int function3_copy2(int a) {
- int b = a + 3;
- return b + 1;
+ATTRIB int function_b1(int a) {
+ int b = a - 1;
+ return b * 3;
}
-ATTRIB int function2_copy2(int a) {
- int result = a - 22;
- return result;
+ATTRIB int function_b2(int a) {
+ int b = a - 5;
+ return b + 2;
}
-struct logic_error {
- logic_error(const char* s) {}
-};
-
-struct length_error : public logic_error {
- __attribute__((noinline)) explicit length_error(const char* s) : logic_error(s) {}
-};
+#--- main.cpp
+extern "C" int function_a1(int);
+extern "C" int function_a2(int);
+extern "C" int function_b1(int);
+extern "C" int function_b2(int);
int main() {
int sum = 0;
- sum += function2_copy2(3);
- sum += function3_copy2(41);
- sum += function2_copy1(11);
- sum += function1_copy1(42);
- length_error e("test");
+ sum += function_a1(1);
+ sum += function_a2(2);
+ sum += function_b1(3);
+ sum += function_b2(4);
return sum;
}
#--- gen
-# Compile to an object file
+# Compile to separate object files with stmt_sequence attributes
+clang --target=arm64-apple-macos11 \
+ -c \
+ -fdebug-compilation-dir=/private/tmp/stmt_seq \
+ -g \
+ -gdwarf-4 \
+ -fno-unwind-tables \
+ -mllvm -emit-func-debug-line-table-offsets \
+ -fno-exceptions \
+ -mno-outline \
+ -Oz \
+ a.cpp \
+ -o a.o
+
clang --target=arm64-apple-macos11 \
-c \
-fdebug-compilation-dir=/private/tmp/stmt_seq \
@@ -77,23 +100,38 @@ clang --target=arm64-apple-macos11 \
-fno-exceptions \
-mno-outline \
-Oz \
- stmt_seq_macho.cpp \
- -o stmt_seq_macho.o
+ b.cpp \
+ -o b.o
+
+clang --target=arm64-apple-macos11 \
+ -c \
+ -fdebug-compilation-dir=/private/tmp/stmt_seq \
+ -g \
+ -gdwarf-4 \
+ -fno-unwind-tables \
+ -fno-exceptions \
+ -mno-outline \
+ -Oz \
+ main.cpp \
+ -o main.o
-# Link into an executable
+# Link with b.o FIRST so b's functions get LOWER addresses than a's functions.
+# This means when dsymutil processes a.o, its line table sequences will need
+# to be placed AFTER b.o's sequences in the output, triggering reordering.
ld64.lld \
-arch arm64 \
-platform_version macos 11.0.0 11.0.0 \
-o stmt_seq_macho.exe \
- stmt_seq_macho.o \
- -dead_strip \
- --icf=all \
- -oso_prefix $(pwd)/ \
- --keep-icf-stabs
+ b.o a.o main.o \
+ -oso_prefix $(pwd)/
-# Convert executable to YAML for the test
-echo "#--- stmt_seq_macho.o.yaml"
-obj2yaml stmt_seq_macho.o | sed '1a\
+# Convert to YAML for the test
+echo "#--- a.o.yaml"
+obj2yaml a.o | sed '1a\
+IsLittleEndian: true'
+echo ""
+echo "#--- b.o.yaml"
+obj2yaml b.o | sed '1a\
IsLittleEndian: true'
echo ""
echo "#--- stmt_seq_macho.exe.yaml"
@@ -101,7 +139,7 @@ obj2yaml stmt_seq_macho.exe | sed '1a\
IsLittleEndian: true'
#--- stmt-seq-macho.yaml
-#--- stmt_seq_macho.o.yaml
+#--- a.o.yaml
--- !mach-o
IsLittleEndian: true
FileHeader:
@@ -109,119 +147,41 @@ FileHeader:
cputype: 0x100000C
cpusubtype: 0x0
filetype: 0x1
- ncmds: 5
- sizeofcmds: 1176
+ ncmds: 4
+ sizeofcmds: 1080
flags: 0x2000
reserved: 0x0
LoadCommands:
- cmd: LC_SEGMENT_64
- cmdsize: 1032
+ cmdsize: 952
segname: ''
vmaddr: 0
- vmsize: 3125
- fileoff: 1208
- filesize: 3125
+ vmsize: 937
+ fileoff: 1112
+ filesize: 937
maxprot: 7
initprot: 7
- nsects: 12
+ nsects: 11
flags: 0
Sections:
- sectname: __text
segname: __TEXT
addr: 0x0
- size: 148
- offset: 0x4B8
+ size: 20
+ offset: 0x458
align: 2
- reloff: 0x10F0
- nreloc: 8
- flags: 0x80000400
- reserved1: 0x0
- reserved2: 0x0
- reserved3: 0x0
- content: 00040011C0035FD600100011C0035FD600580051C0035FD600100011C0035FD600580051C0035FD6FFC300D1F44F01A9FD7B02A9FD8300916000805200000094F30300AA20058052000000941400130B6001805200000094F30300AA40058052000000947302000B0100009021000091E03F0091000000948002130BFD7B42A9F44F41A9FFC30091C0035FD600000014C0035FD6
- relocations:
- - address: 0x8C
- symbolnum: 4
- pcrel: true
- length: 2
- extern: true
- type: 2
- scattered: false
- value: 0
- - address: 0x74
- symbolnum: 3
- pcrel: true
- length: 2
- extern: true
- type: 2
- scattered: false
- value: 0
- - address: 0x6C
- symbolnum: 1
- pcrel: false
- length: 2
- extern: true
- type: 4
- scattered: false
- value: 0
- - address: 0x68
- symbolnum: 1
- pcrel: true
- length: 2
- extern: true
- type: 3
- scattered: false
- value: 0
- - address: 0x60
- symbolnum: 5
- pcrel: true
- length: 2
- extern: true
- type: 2
- scattered: false
- value: 0
- - address: 0x54
- symbolnum: 6
- pcrel: true
- length: 2
- extern: true
- type: 2
- scattered: false
- value: 0
- - address: 0x48
- symbolnum: 9
- pcrel: true
- length: 2
- extern: true
- type: 2
- scattered: false
- value: 0
- - address: 0x3C
- symbolnum: 7
- pcrel: true
- length: 2
- extern: true
- type: 2
- scattered: false
- value: 0
- - sectname: __cstring
- segname: __TEXT
- addr: 0x94
- size: 5
- offset: 0x54C
- align: 0
reloff: 0x0
nreloc: 0
- flags: 0x2
+ flags: 0x80000400
reserved1: 0x0
reserved2: 0x0
reserved3: 0x0
- content: '7465737400'
+ content: 08781F5300090011C0035FD600100011C0035FD6
- sectname: __debug_loc
segname: __DWARF
- addr: 0x99
- size: 412
- offset: 0x551
+ addr: 0x14
+ size: 188
+ offset: 0x46C
align: 0
reloff: 0x0
nreloc: 0
@@ -229,12 +189,12 @@ LoadCommands:
reserved1: 0x0
reserved2: 0x0
reserved3: 0x0
- content: 08000000000000000C000000000000000100500C0000000000000010000000000000000400A301509F0000000000000000000000000000000008000000000000000C00000000000000030070039F0000000000000000000000000000000010000000000000001400000000000000010050140000000000000018000000000000000400A301509F0000000000000000000000000000000018000000000000001C000000000000000100501C0000000000000020000000000000000400A301509F0000000000000000000000000000000018000000000000001C00000000000000030070039F0000000000000000000000000000000020000000000000002400000000000000010050240000000000000028000000000000000400A301509F00000000000000000000000000000000240000000000000028000000000000000100500000000000000000000000000000000038000000000000004400000000000000030011009F4400000000000000500000000000000001006350000000000000005C0000000000000001006400000000000000000000000000000000
+ content: 0000000000000000080000000000000001005008000000000000000C000000000000000400A301509F0000000000000000000000000000000000000000000000000800000000000000030070019F000000000000000000000000000000000C000000000000001000000000000000010050100000000000000014000000000000000400A301509F000000000000000000000000000000000C000000000000001000000000000000030070039F00000000000000000000000000000000
- sectname: __debug_abbrev
segname: __DWARF
- addr: 0x235
- size: 372
- offset: 0x6ED
+ addr: 0xD0
+ size: 99
+ offset: 0x528
align: 0
reloff: 0x0
nreloc: 0
@@ -244,114 +204,18 @@ LoadCommands:
reserved3: 0x0
- sectname: __debug_info
segname: __DWARF
- addr: 0x3A9
- size: 747
- offset: 0x861
+ addr: 0x133
+ size: 174
+ offset: 0x58B
align: 0
- reloff: 0x1130
- nreloc: 16
+ reloff: 0x808
+ nreloc: 3
flags: 0x2000000
reserved1: 0x0
reserved2: 0x0
reserved3: 0x0
relocations:
- - address: 0x2A7
- symbolnum: 1
- pcrel: false
- length: 3
- extern: false
- type: 0
- scattered: false
- value: 0
- - address: 0x28E
- symbolnum: 1
- pcrel: false
- length: 3
- extern: false
- type: 0
- scattered: false
- value: 0
- - address: 0x253
- symbolnum: 1
- pcrel: false
- length: 3
- extern: false
- type: 0
- scattered: false
- value: 0
- - address: 0x1F5
- symbolnum: 1
- pcrel: false
- length: 3
- extern: false
- type: 0
- scattered: false
- value: 0
- - address: 0x1E1
- symbolnum: 1
- pcrel: false
- length: 3
- extern: false
- type: 0
- scattered: false
- value: 0
- - address: 0x1CE
- symbolnum: 1
- pcrel: false
- length: 3
- extern: false
- type: 0
- scattered: false
- value: 0
- - address: 0x1BA
- symbolnum: 1
- pcrel: false
- length: 3
- extern: false
- type: 0
- scattered: false
- value: 0
- - address: 0x1...
[truncated]
|
Contributor
Author
|
@JDevlieghere - could you have a look - or could you add someone that might be able to have a look ? |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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 fixes a bug where
DW_AT_LLVM_stmt_sequenceattributes in dSYM files were pointing to invalid offsets in the.debug_linesection. These attributes must point toDW_LNE_set_addressopcodes (which mark sequence starts), but after dsymutil reorders line table sequences by address, the original row indices no longer correspond to sequence starts in the output.The root cause is that when sequences get reordered or merged, a row that was originally a sequence start may end up in the middle of a different sequence in the output. The old code was mapping the original row index directly to its output position, but that output position might not have a
DW_LNE_set_addressopcode anymore.The fix builds a mapping from each output row to its containing sequence's start row. When patching
DW_AT_LLVM_stmt_sequence, we now:DW_LNE_set_addressis emitted)This ensures all
DW_AT_LLVM_stmt_sequenceattributes point to validDW_LNE_set_addressopcodes, which is required for debuggers to correctly locate line table sequences for functions.Testing wise:
Modified
stmt-seq-macho.testto create a scenario that triggers the bug:DW_AT_LLVM_stmt_sequenceoffsets and verifies each one appears as aDW_LNE_set_addressoffset in the outputResults: