Skip to content

[DWARF] Emit a worst-case prologue_end flag for pathological inputs #107849

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Nov 12, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
141 changes: 118 additions & 23 deletions llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2061,6 +2061,16 @@ void DwarfDebug::beginInstruction(const MachineInstr *MI) {
unsigned LastAsmLine =
Asm->OutStreamer->getContext().getCurrentDwarfLoc().getLine();

if (!DL && MI == PrologEndLoc) {
// In rare situations, we might want to place the end of the prologue
// somewhere that doesn't have a source location already. It should be in
// the entry block.
assert(MI->getParent() == &*MI->getMF()->begin());
recordSourceLine(SP->getScopeLine(), 0, SP,
DWARF2_FLAG_PROLOGUE_END | DWARF2_FLAG_IS_STMT);
return;
}

bool PrevInstInDiffBB = PrevInstBB && PrevInstBB != MI->getParent();
if (DL == PrevInstLoc && !PrevInstInDiffBB) {
// If we have an ongoing unspecified location, nothing to do here.
Expand Down Expand Up @@ -2135,32 +2145,109 @@ static std::pair<const MachineInstr *, bool>
findPrologueEndLoc(const MachineFunction *MF) {
// First known non-DBG_VALUE and non-frame setup location marks
// the beginning of the function body.
const MachineInstr *LineZeroLoc = nullptr;
const auto &TII = *MF->getSubtarget().getInstrInfo();
const MachineInstr *NonTrivialInst = nullptr;
const Function &F = MF->getFunction();

// Some instructions may be inserted into prologue after this function. Must
// keep prologue for these cases.
bool IsEmptyPrologue =
!(F.hasPrologueData() || F.getMetadata(LLVMContext::MD_func_sanitize));
for (const auto &MBB : *MF) {
for (const auto &MI : MBB) {
if (!MI.isMetaInstruction()) {
if (!MI.getFlag(MachineInstr::FrameSetup) && MI.getDebugLoc()) {
// Scan forward to try to find a non-zero line number. The
// prologue_end marks the first breakpoint in the function after the
// frame setup, and a compiler-generated line 0 location is not a
// meaningful breakpoint. If none is found, return the first
// location after the frame setup.
if (MI.getDebugLoc().getLine())
return std::make_pair(&MI, IsEmptyPrologue);

LineZeroLoc = &MI;
}
IsEmptyPrologue = false;
}

// Helper lambda to examine each instruction and potentially return it
// as the prologue_end point.
auto ExamineInst = [&](const MachineInstr &MI)
-> std::optional<std::pair<const MachineInstr *, bool>> {
// Is this instruction trivial data shuffling or frame-setup?
bool isCopy = (TII.isCopyInstr(MI) ? true : false);
bool isTrivRemat = TII.isTriviallyReMaterializable(MI);
bool isFrameSetup = MI.getFlag(MachineInstr::FrameSetup);

if (!isFrameSetup && MI.getDebugLoc()) {
// Scan forward to try to find a non-zero line number. The
// prologue_end marks the first breakpoint in the function after the
// frame setup, and a compiler-generated line 0 location is not a
// meaningful breakpoint. If none is found, return the first
// location after the frame setup.
if (MI.getDebugLoc().getLine())
return std::make_pair(&MI, IsEmptyPrologue);
}

// Keep track of the first "non-trivial" instruction seen, i.e. anything
// that doesn't involve shuffling data around or is a frame-setup.
if (!isCopy && !isTrivRemat && !isFrameSetup && !NonTrivialInst)
NonTrivialInst = &MI;

IsEmptyPrologue = false;
return std::nullopt;
};

// Examine all the instructions at the start of the function. This doesn't
// necessarily mean just the entry block: unoptimised code can fall-through
// into an initial loop, and it makes sense to put the initial breakpoint on
// the first instruction of such a loop. However, if we pass branches, we're
// better off synthesising an early prologue_end.
auto CurBlock = MF->begin();
auto CurInst = CurBlock->begin();
while (true) {
// Skip empty blocks, in rare cases the entry can be empty.
if (CurInst == CurBlock->end()) {
++CurBlock;
CurInst = CurBlock->begin();
continue;
}

// Check whether this non-meta instruction a good position for prologue_end.
if (!CurInst->isMetaInstruction()) {
auto FoundInst = ExamineInst(*CurInst);
if (FoundInst)
return *FoundInst;
}

// Try to continue searching, but use a backup-location if substantive
// computation is happening.
auto NextInst = std::next(CurInst);
if (NextInst != CurInst->getParent()->end()) {
// Continue examining the current block.
CurInst = NextInst;
continue;
}

// We've reached the end of the block. Did we just look at a terminator?
if (CurInst->isTerminator()) {
// Some kind of "real" control flow is occurring. At the very least
// we would have to start exploring the CFG, a good signal that the
// prologue is over.
break;
}

// If we've already fallen through into a loop, don't fall through
// further, use a backup-location.
if (CurBlock->pred_size() > 1)
break;

// Fall-through from entry to the next block. This is common at -O0 when
// there's no initialisation in the function. Bail if we're also at the
// end of the function.
if (++CurBlock == MF->end())
break;
CurInst = CurBlock->begin();
}

// We couldn't find any source-location, suggesting all meaningful information
// got optimised away. Set the prologue_end to be the first non-trivial
// instruction, which will get the scope line number. This is better than
// nothing.
// Only do this in the entry block, as we'll be giving it the scope line for
// the function. Return IsEmptyPrologue==true if we've picked the first
// instruction.
if (NonTrivialInst && NonTrivialInst->getParent() == &*MF->begin()) {
IsEmptyPrologue = NonTrivialInst == &*MF->begin()->begin();
return std::make_pair(NonTrivialInst, IsEmptyPrologue);
}
return std::make_pair(LineZeroLoc, IsEmptyPrologue);

// If the entry path is empty, just don't have a prologue_end at all.
return std::make_pair(nullptr, IsEmptyPrologue);
}

/// Register a source line with debug info. Returns the unique label that was
Expand Down Expand Up @@ -2192,12 +2279,20 @@ DwarfDebug::emitInitialLocDirective(const MachineFunction &MF, unsigned CUID) {
bool IsEmptyPrologue = PrologEnd.second;

// If the prolog is empty, no need to generate scope line for the proc.
if (IsEmptyPrologue)
if (IsEmptyPrologue) {
// In degenerate cases, we can have functions with no source locations
// at all. These want a scope line, to avoid a totally empty function.
// Thus, only skip scope line if there's location to place prologue_end.
if (PrologEndLoc)
return PrologEndLoc;
// at all, or only empty ones. These want a scope line, to avoid a totally
// empty function. Thus, only skip the scope line if there's location to
// place prologue_end.
if (PrologEndLoc) {
const DebugLoc &DL = PrologEndLoc->getDebugLoc();
if (!DL || DL->getLine() != 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel slightly confused reading the comment and the current if expression. I read the comment as saying "... Thus, only early-return with this location if it's a valid location", but !DL is not a location we can use?

Should this be if (DL && DL->getLine() != 0) instead of if (!DL || DL->getLine() != 0)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's not clear that the real purpose is simply filtering out line-zero-locations -- those with an empty DebugLoc get given the scope line due to an earlier patch. (It's unfortunate that we've got "empty" nullptr DebugLocs, but also "empty" source locations in the form of line-zero, but here we are).

Without this awkward logic we would end up with zero-length linetable entries for the scope line, before then having a prologue_end linetable entry.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right, yep I see that now, thanks. This all feels a bit tangled up in an unfortunate way, but I've not got any immediate ideas (possibly there's some way of refactoring this, but maybe not), so LGTM.

return PrologEndLoc;

// Later, don't place the prologue_end flag on this line-zero location.
PrologEndLoc = nullptr;
}
}

// Ensure the compile unit is created if the function is called before
// beginFunction().
Expand Down
6 changes: 3 additions & 3 deletions llvm/test/CodeGen/X86/no-non-zero-debug-loc-prologue.ll
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
; RUN: llc -filetype=asm -mtriple=x86_64-apple-macosx12.0.0 -O0 %s -o - | FileCheck %s
; RUN: llc -filetype=asm -mtriple=x86_64-apple-macosx12.0.0 -O0 %s -o - | FileCheck %s --implicit-check-not=prologue_end
; CHECK: Lfunc_begin0:
; CHECK-NEXT: .file{{.+}}
; CHECK-NEXT: .loc 1 1 0 ## test-small.c:1:0{{$}}
; CHECK-NEXT: .cfi_startproc
; CHECK-NEXT: ## %bb.{{[0-9]+}}:
; CHECK-NEXT: .loc 1 0 1 prologue_end{{.*}}
; CHECK-NEXT: .loc 1 0 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(thinking out loud)
I think this test change is fine. AFAICT the original purpose of this test is to check that prologue_end doesn't get added to the first instructions. More than checking that it gets added to the final instruction, or added at all. With that line of reasoning this test update doesn't invalidate the original purpose of the test.

cc @rastogishubham as the author of this test

define void @test() #0 !dbg !9 {
ret void, !dbg !12
}
Expand All @@ -19,4 +19,4 @@ define void @test() #0 !dbg !9 {
!9 = distinct !DISubprogram(name: "test", scope: !10, file: !10, line: 1, type: !11, scopeLine: 1, spFlags: DISPFlagDefinition, unit: !5, retainedNodes: !7)
!10 = !DIFile(filename: "test-small.c", directory: "/Users/shubham/Development/test")
!11 = !DISubroutineType(types: !7)
!12 = !DILocation(line: 0, column: 1, scope: !9)
!12 = !DILocation(line: 0, column: 1, scope: !9)
4 changes: 2 additions & 2 deletions llvm/test/CodeGen/X86/pseudo_cmov_lower2.ll
Original file line number Diff line number Diff line change
Expand Up @@ -203,9 +203,9 @@ declare void @llvm.dbg.value(metadata, metadata, metadata)
; locations in the function.
define double @foo1_g(float %p1, double %p2, double %p3) nounwind !dbg !4 {
; CHECK-LABEL: foo1_g:
; CHECK: .file 1 "." "test.c"
; CHECK-NEXT: .loc 1 3 0
; CHECK: # %bb.0: # %entry
; CHECK-NEXT: .file 1 "." "test.c"
; CHECK-NEXT: .loc 1 3 0 prologue_end
; CHECK-NEXT: xorps %xmm3, %xmm3
; CHECK-NEXT: ucomiss %xmm3, %xmm0
; CHECK-NEXT: movsd {{.*#+}} xmm0 = [1.25E+0,0.0E+0]
Expand Down
134 changes: 134 additions & 0 deletions llvm/test/DebugInfo/MIR/X86/dbg-prologue-backup-loc2.mir
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
# RUN: llc %s -start-before=livedebugvalues -o - | \
# RUN: FileCheck %s --implicit-check-not=prologue_end
#
## When picking a "backup" location of the first non-trivial instruction in
## a function, don't select a location outside of the entry block. We have to
## give it the function's scope-line, and installing that outside of the entry
## block is liable to be misleading.
##
## Produced from the C below with "clang -O2 -g -mllvm
## -stop-before=livedebugvalues", then modified to unrotate and shift early
## insts into the loop block. This means the MIR is meaningless, we only test
## whether the scope-line will leak into the loop block or not.
##
## int glob = 0;
## int foo(int arg, int sum) {
## arg += sum;
## while (arg) {
## glob--;
## arg %= glob;
## }
## return 0;
## }
#
# CHECK-LABEL: foo:
# CHECK: .loc 0 2 0
# CHECK: # %bb.0:
# CHECK-NEXT: movl %edi, %edx
# CHECK-NEXT: .loc 0 0 0 is_stmt 0
# CHECK-NEXT: .Ltmp0:
# CHECK-NEXT: .p2align 4, 0x90
# CHECK-NEXT: .LBB0_1:
# CHECK-LABEL: addl %esi, %edx


--- |
; ModuleID = 'out2.ll'
source_filename = "foo.c"
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

@glob = dso_local local_unnamed_addr global i32 0, align 4, !dbg !0

define dso_local noundef i32 @foo(i32 noundef %arg, i32 noundef %sum) local_unnamed_addr !dbg !9 {
entry:
%add = add nsw i32 %sum, %arg
br label %while.body.preheader

while.body.preheader: ; preds = %entry
%glob.promoted = load i32, ptr @glob, align 4
br label %while.body, !dbg !13

while.body: ; preds = %while.body, %while.body.preheader
%arg.addr.06 = phi i32 [ %rem, %while.body ], [ %add, %while.body.preheader ]
%dec35 = phi i32 [ %dec, %while.body ], [ %glob.promoted, %while.body.preheader ]
%dec = add nsw i32 %dec35, -1, !dbg !14
%0 = add i32 %dec35, -1, !dbg !16
%rem = srem i32 %arg.addr.06, %0, !dbg !16
%tobool.not = icmp eq i32 %rem, 0, !dbg !13
br i1 %tobool.not, label %while.cond.while.end_crit_edge, label %while.body, !dbg !13

while.cond.while.end_crit_edge: ; preds = %while.body
store i32 %dec, ptr @glob, align 4, !dbg !14
br label %while.end, !dbg !13

while.end: ; preds = %while.cond.while.end_crit_edge
ret i32 0, !dbg !17
}

!llvm.dbg.cu = !{!2}
!llvm.module.flags = !{!6, !7}
!llvm.ident = !{!8}

!0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
!1 = distinct !DIGlobalVariable(name: "glob", scope: !2, file: !3, line: 1, type: !5, isLocal: false, isDefinition: true)
!2 = distinct !DICompileUnit(language: DW_LANG_C11, file: !3, producer: "clang", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, globals: !4, splitDebugInlining: false, nameTableKind: None)
!3 = !DIFile(filename: "foo.c", directory: "")
!4 = !{!0}
!5 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
!6 = !{i32 7, !"Dwarf Version", i32 5}
!7 = !{i32 2, !"Debug Info Version", i32 3}
!8 = !{!"clang"}
!9 = distinct !DISubprogram(name: "foo", scope: !3, file: !3, line: 2, type: !10, scopeLine: 2, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !2, retainedNodes: !12)
!10 = !DISubroutineType(types: !11)
!11 = !{!5, !5, !5}
!12 = !{}
!13 = !DILocation(line: 4, column: 3, scope: !9)
!14 = !DILocation(line: 5, column: 9, scope: !15)
!15 = distinct !DILexicalBlock(scope: !9, file: !3, line: 4, column: 15)
!16 = !DILocation(line: 6, column: 9, scope: !15)
!17 = !DILocation(line: 8, column: 3, scope: !9)

...
---
name: foo
alignment: 16
tracksRegLiveness: true
debugInstrRef: true
tracksDebugUserValues: true
liveins:
- { reg: '$edi' }
- { reg: '$esi' }
frameInfo:
maxAlignment: 1
maxCallFrameSize: 0
isCalleeSavedInfoValid: true
machineFunctionInfo:
amxProgModel: None
body: |
bb.0.entry:
liveins: $edi, $esi

$edx = MOV32rr $edi

bb.1.while.body (align 16):
successors: %bb.2(0x04000000), %bb.1(0x7c000000)
liveins: $ecx, $edx, $esi

renamable $edx = nsw ADD32rr killed renamable $edx, renamable $esi, implicit-def dead $eflags
renamable $ecx = MOV32rm $rip, 1, $noreg, @glob, $noreg :: (dereferenceable load (s32) from @glob)
renamable $ecx = DEC32r killed renamable $ecx, implicit-def dead $eflags
$eax = MOV32rr killed $edx
CDQ implicit-def $eax, implicit-def $edx, implicit $eax
IDIV32r renamable $ecx, implicit-def dead $eax, implicit-def $edx, implicit-def dead $eflags, implicit $eax, implicit $edx
TEST32rr renamable $edx, renamable $edx, implicit-def $eflags
JCC_1 %bb.1, 5, implicit killed $eflags

bb.2.while.cond.while.end_crit_edge:
liveins: $ecx, $esi

MOV32mr $rip, 1, $noreg, @glob, $noreg, killed renamable $ecx, debug-location !14 :: (store (s32) into @glob)
$eax = XOR32rr undef $eax, undef $eax, implicit-def dead $eflags, debug-location !17
RET64 $eax, debug-location !17

...
5 changes: 3 additions & 2 deletions llvm/test/DebugInfo/MIR/X86/empty-inline.mir
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,12 @@
#
# CHECK: Address Line Column File ISA Discriminator OpIndex Flags
# CHECK-NEXT: ---
# CHECK-NEXT: 25 0 1 0 0 0 is_stmt
# CHECK-NEXT: 25 0 1 0 0 0 is_stmt prologue_end
# CHECK-NEXT: 0 0 1 0 0 0
# CHECK-NEXT: 29 28 1 0 0 0 is_stmt prologue_end
# CHECK-NEXT: 29 28 1 0 0 0 is_stmt
# CHECK-NEXT: 29 28 1 0 0 0 is_stmt
# CHECK-NEXT: 29 28 1 0 0 0 is_stmt end_sequence

--- |
source_filename = "t.ll"
target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
Expand Down
Loading
Loading