[NFC][DebugInfo] Allow single instruction sequence in line table to make symbolication of merged functions easier #154851

DataCorrupted · 2025-08-21T22:14:38Z

A "Sequence" in the line table consists of multiple machine instructions [LowPC, HighPC) and rows [FirstRowIndex, LastRowIndex).

llvm-project/llvm/include/llvm/DebugInfo/DWARF/DWARFDebugLine.h

Lines 223 to 225 in b3baa4d

    
           bool isValid() const { 
        
             return !Empty && (LowPC < HighPC) && (FirstRowIndex < LastRowIndex); 
        
           }

When adding a Sequence, LastRow is RowNumbe + 1 while HighPC is Row.Address:

llvm-project/llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

Lines 582 to 590 in b3baa4d

    
           if (Row.EndSequence) { 
        
             // Record the end of instruction sequence. 
        
             Sequence.HighPC = Row.Address.Address; 
        
             Sequence.LastRowIndex = RowNumber + 1; 
        
             Sequence.SectionIndex = Row.Address.SectionIndex; 
        
             if (Sequence.isValid()) 
        
               LineTable->appendSequence(Sequence); 
        
             Sequence.reset(); 
        
           }

This means that if you look up LineTable's last row (LT->Rows.back()), you get nothing because its address is not considered to be part of the "Sequence".

Another problem is one-instruction functions can't be its own "Sequence" since LowPC == HighPC, the parser will think this is an invalid Sequence. Normally this is not a problem, but with ICF=safe-thunk, many one-instruction thunks are added. These functions' DebugLine can't be parsed as a stand alone "Sequence", which had made relocation (#149618) and verification (#152807) substantially harder.

I can think of two solutions:

Define Sequence's PC to be [LowPC, HighPC + MinInstLength) shown in this diff
- Unfortunately when I try this one out a bunch of tests in llvm/unittests/DebugInfo/GSYM/GSYMTest.cpp, will fail.
During ICF=safe-thunk, the thunk function can have two instructions (a nop before branch shown below) [ICF] Add a NOP after branch in ICF thunk to improve debugability #154986

llvm-project/lld/MachO/Arch/ARM64.cpp

Lines 177 to 179 in d4b9aca

    
           static constexpr uint32_t icfSafeThunkCode[] = { 
        
               0x14000000, // 08: b    target 
        
           };

github-actions · 2025-08-21T22:53:08Z

✅ With the latest revision this PR passed the C/C++ code formatter.

DataCorrupted · 2025-08-21T23:27:18Z

cc @nocchijiang

Signed-off-by: Peter Rong <[email protected]>

nocchijiang · 2025-08-25T08:09:29Z

Thank you for your work on this. When I was trying to create the minimal repro in the RFC thread, I had a chance to look at the code rebuilding sequences and realized that some of the existing design/implementation are somewhat incompatible with the goals for DW_AT_LLVM_stmt_sequence. For instance, the practice of merging neighboring sequences seems to be problematic. I see from #149618 you are trying to rebuild the sequences (again) just for DW_AT_LLVM_stmt_sequence, and I was wondering about the long-term direction. While this approach has minimal impact on other users, I am not entirely sure if it really is the "right" direction. As for this patch, I think the approach is good, but I cannot say if it would break things for others - I am really not an expert of DWARF-related matters.

Additionally, I believe that it is not a great idea to insert nops before thunks. Compilers sometimes generates functions consisting of a single branch. At ByteDance, we also teach our linker to emit thunks similar to LLD's -icf=safe_thunks, and we'd want to avoid making changes just to accommodate DWARF requirements.

Signed-off-by: Peter Rong <[email protected]>

DataCorrupted · 2025-08-25T17:57:55Z

Compilers sometimes generates functions consisting of a single branch

I thought about this, and if we are to emit a nop after branch, we can assume all merged functions that requires DW_AT_LLVM_stmt_sequence has at least two instructions. If so, we can skip emitting DW_AT_LLVM_stmt_sequence for single-instruction functions, as such they will be symbolicated just fine, as if DW_AT_LLVM_stmt_sequence was never introduced. I've put up another draft for this: #154986

I think the approach is good, but I cannot say if it would break things for others

Unfortunately, it is, breaking somethings in LLDB/GSYM. I don't think I can move forward until I can receive confirmation from more experienced eyes: Are these breaks are benign and can be easily fixed, or did it break some fundamental assumptions in your work?

Some line tables that were considered illegal is now legal: lld :: ELF/undef.s @jh7370
Some tests are relying on the fact that the last row can't be found:
- LLVM :: tools/llvm-dwarfdump/X86/locstats.ll
- LLVM :: tools/llvm-symbolizer/symbol-search.test
- LLVM-Unit :: DebugInfo/GSYM/./DebugInfoGSYMTests/GSYMTest/TestDWARFNoLines @clayborg

DataCorrupted · 2025-09-09T16:49:52Z

Closed since #157529 fixed the root cause

### Context #99710 introduced `.loc_label` so we can terminate a line sequence. However, it did not advance PC properly. This is problematic for 1-instruction functions as it will have zero-length sequence. The test checked in that PR shows the problem: ``` # CHECK-LINE-TABLE: Address Line Column File ISA Discriminator OpIndex Flags # CHECK-LINE-TABLE-NEXT: ------------------ ------ ------ ------ --- ------------- ------- ------------- # CHECK-LINE-TABLE-NEXT: 0x00000028: 05 DW_LNS_set_column (1) # CHECK-LINE-TABLE-NEXT: 0x0000002a: 00 DW_LNE_set_address (0x0000000000000000) # CHECK-LINE-TABLE-NEXT: 0x00000035: 01 DW_LNS_copy # CHECK-LINE-TABLE-NEXT: 0x0000000000000000 1 1 1 0 0 0 is_stmt # CHECK-LINE-TABLE-NEXT: 0x00000036: 00 DW_LNE_end_sequence # CHECK-LINE-TABLE-NEXT: 0x0000000000000000 1 1 1 0 0 0 is_stmt end_sequence ``` Both rows having PC 0x0 is incorrect, and parsers won't be able to parse them. See more explanation why this is wrong in #154851. ### Design This PR attempts to fix this by advancing the PC to the next available Label, and advance to the end of the section if no Label is available. ### Implementation - `emitDwarfLineEndEntry` will advance PC to the `CurrLabel` - If `CurrLabel` is null, its probably a fake LineEntry we introduced in #110192. In that case look for the next Label - If still not label can be found, use `null` and `emitDwarfLineEndEntry` is smart enough to advance PC to the end of the section - Rename `LastLabel` to `PrevLabel`, "last" can mean "previous" or "final", this is ambigous. - Updated the tests to emit a correct label. ### Note This fix should render #154986 and #154851 obsolete, they were temporary fixes and don't resolve the root cause. --------- Signed-off-by: Peter Rong <[email protected]>

This reverts commit aabf18d. #157529 included a test that used clang, which doesn't exists in some CI. #158343 reverts it. This PR reapplies the original patch with the incorrect test removed. ### Context #99710 introduced `.loc_label` so we can terminate a line sequence. However, it did not advance PC properly. This is problematic for 1-instruction functions as it will have zero-length sequence. The test checked in that PR shows the problem: ``` # CHECK-LINE-TABLE: Address Line Column File ISA Discriminator OpIndex Flags # CHECK-LINE-TABLE-NEXT: ------------------ ------ ------ ------ --- ------------- ------- ------------- # CHECK-LINE-TABLE-NEXT: 0x00000028: 05 DW_LNS_set_column (1) # CHECK-LINE-TABLE-NEXT: 0x0000002a: 00 DW_LNE_set_address (0x0000000000000000) # CHECK-LINE-TABLE-NEXT: 0x00000035: 01 DW_LNS_copy # CHECK-LINE-TABLE-NEXT: 0x0000000000000000 1 1 1 0 0 0 is_stmt # CHECK-LINE-TABLE-NEXT: 0x00000036: 00 DW_LNE_end_sequence # CHECK-LINE-TABLE-NEXT: 0x0000000000000000 1 1 1 0 0 0 is_stmt end_sequence ``` Both rows having PC 0x0 is incorrect, and parsers won't be able to parse them. See more explanation why this is wrong in #154851. ### Design This PR attempts to fix this by advancing the PC to the next available Label, and advance to the end of the section if no Label is available. ### Implementation - `emitDwarfLineEndEntry` will advance PC to the `CurrLabel` - If `CurrLabel` is null, its probably a fake LineEntry we introduced in #110192. In that case look for the next Label - If still not label can be found, use `null` and `emitDwarfLineEndEntry` is smart enough to advance PC to the end of the section - Rename `LastLabel` to `PrevLabel`, "last" can mean "previous" or "final", this is ambigous. - Updated the tests to emit a correct label. ### Note This fix should render #154986 and #154851 obsolete, they were temporary fixes and don't resolve the root cause.

…#158379) This reverts commit aabf18d. llvm#157529 included a test that used clang, which doesn't exists in some CI. llvm#158343 reverts it. This PR reapplies the original patch with the incorrect test removed. ### Context llvm#99710 introduced `.loc_label` so we can terminate a line sequence. However, it did not advance PC properly. This is problematic for 1-instruction functions as it will have zero-length sequence. The test checked in that PR shows the problem: ``` # CHECK-LINE-TABLE: Address Line Column File ISA Discriminator OpIndex Flags # CHECK-LINE-TABLE-NEXT: ------------------ ------ ------ ------ --- ------------- ------- ------------- # CHECK-LINE-TABLE-NEXT: 0x00000028: 05 DW_LNS_set_column (1) # CHECK-LINE-TABLE-NEXT: 0x0000002a: 00 DW_LNE_set_address (0x0000000000000000) # CHECK-LINE-TABLE-NEXT: 0x00000035: 01 DW_LNS_copy # CHECK-LINE-TABLE-NEXT: 0x0000000000000000 1 1 1 0 0 0 is_stmt # CHECK-LINE-TABLE-NEXT: 0x00000036: 00 DW_LNE_end_sequence # CHECK-LINE-TABLE-NEXT: 0x0000000000000000 1 1 1 0 0 0 is_stmt end_sequence ``` Both rows having PC 0x0 is incorrect, and parsers won't be able to parse them. See more explanation why this is wrong in llvm#154851. ### Design This PR attempts to fix this by advancing the PC to the next available Label, and advance to the end of the section if no Label is available. ### Implementation - `emitDwarfLineEndEntry` will advance PC to the `CurrLabel` - If `CurrLabel` is null, its probably a fake LineEntry we introduced in llvm#110192. In that case look for the next Label - If still not label can be found, use `null` and `emitDwarfLineEndEntry` is smart enough to advance PC to the end of the section - Rename `LastLabel` to `PrevLabel`, "last" can mean "previous" or "final", this is ambigous. - Updated the tests to emit a correct label. ### Note This fix should render llvm#154986 and llvm#154851 obsolete, they were temporary fixes and don't resolve the root cause.

DataCorrupted added 3 commits August 21, 2025 15:14

[DebugLine] Sequence HighPC should be LastRow.address + MinInstLength

d1d3366

Comment out my change

b26ae4d

using if 0

17d24ca

DataCorrupted changed the title ~~[DebugLine] Sequence HighPC should be LastRow.address + MinInstLength~~ [NFC][DebugInfo] Sequence HighPC should be LastRow.address + MinInstLength Aug 21, 2025

update test

f61ed8f

DataCorrupted changed the title ~~[NFC][DebugInfo] Sequence HighPC should be LastRow.address + MinInstLength~~ [NFC][DebugInfo] Allow single instruction sequence in line table to allow easier symbolication of merged functions Aug 21, 2025

DataCorrupted changed the title ~~[NFC][DebugInfo] Allow single instruction sequence in line table to allow easier symbolication of merged functions~~ [NFC][DebugInfo] Allow single instruction sequence in line table to make symbolication of merged functions easier Aug 21, 2025

Test

a77eca3

DataCorrupted mentioned this pull request Aug 22, 2025

[DWARFVerifier] Verify that DW_AT_LLVM_stmt_sequence is set correctly #152807

Open

supress assert

18ad6e2

Signed-off-by: Peter Rong <[email protected]>

fix test

7f9e2e8

Signed-off-by: Peter Rong <[email protected]>

DataCorrupted mentioned this pull request Aug 25, 2025

[ICF] Add a NOP after branch in ICF thunk to improve debugability #154986

Closed

Update test

566c4fa

DataCorrupted mentioned this pull request Sep 8, 2025

[DebugLine] Correct debug line emittion #157529

Merged

DataCorrupted closed this Sep 9, 2025

DataCorrupted mentioned this pull request Sep 12, 2025

Reapply "[DebugLine] Correct debug line emittion" (#158343) #158379

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[NFC][DebugInfo] Allow single instruction sequence in line table to make symbolication of merged functions easier #154851

[NFC][DebugInfo] Allow single instruction sequence in line table to make symbolication of merged functions easier #154851

Uh oh!

DataCorrupted commented Aug 21, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Aug 21, 2025 •

edited

Loading

Uh oh!

DataCorrupted commented Aug 21, 2025

Uh oh!

nocchijiang commented Aug 25, 2025

Uh oh!

DataCorrupted commented Aug 25, 2025

Uh oh!

DataCorrupted commented Sep 9, 2025

Uh oh!

Uh oh!

	bool isValid() const {
	return !Empty && (LowPC < HighPC) && (FirstRowIndex < LastRowIndex);
	}

	if (Row.EndSequence) {
	// Record the end of instruction sequence.
	Sequence.HighPC = Row.Address.Address;
	Sequence.LastRowIndex = RowNumber + 1;
	Sequence.SectionIndex = Row.Address.SectionIndex;
	if (Sequence.isValid())
	LineTable->appendSequence(Sequence);
	Sequence.reset();
	}

	static constexpr uint32_t icfSafeThunkCode[] = {
	0x14000000, // 08: b target
	};

[NFC][DebugInfo] Allow single instruction sequence in line table to make symbolication of merged functions easier #154851

[NFC][DebugInfo] Allow single instruction sequence in line table to make symbolication of merged functions easier #154851

Uh oh!

Conversation

DataCorrupted commented Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DataCorrupted commented Aug 21, 2025

Uh oh!

nocchijiang commented Aug 25, 2025

Uh oh!

DataCorrupted commented Aug 25, 2025

Uh oh!

DataCorrupted commented Sep 9, 2025

Uh oh!

Uh oh!

DataCorrupted commented Aug 21, 2025 •

edited

Loading

github-actions bot commented Aug 21, 2025 •

edited

Loading