[DWARF] Incorrect prologue end line record.

[CarlosAlbertoEnciso]

Summary

The prologue-end line record must be emitted after the last instruction that is part of the function frame setup code and before the instruction that marks the beginning of the function body.

For the below test:

1 extern int get_arg();
2 extern void func(int x);
3
4 int main()
5 {
6 int a;
7 func(get_arg());
8 }
9

The prologue-end line record is emitted with an incorrect associated address, which causes a debugger to show the beginning of function body to be inside the prologue.

This can be seen in the following trimmed assembler output:

main:
  ...
  .loc	1 7 0 prologue_end
  pushq	%rax
.Lcfi0:
  .cfi_def_cfa_offset 16
  callq	_Z7get_argv
  ...
  retq

The instruction 'pushq %rax' is part of the frame setup code.

The correct location for the prologue-end line information is just before the call to '_Z7get_argv', as illustrated in the following trimmed assembler output:

main:
  ...
  pushq	%rax
.Lcfi0:
  .cfi_def_cfa_offset 16
  .loc	1 7 0 prologue_end
  callq	_Z7get_argv
  ...
  retq

[probinson]

I think the argument (and the test) would be stronger if it was based on the instruction sequence rather than offsets. For example if you generate asm and then show prologue_end is immediately before the call to get_arg.
Also we generally like to trim the IR source by removing unnecessary attribute sets.

I'm actually more curious why part of the prologue is associated with line 5 and part with line 7. That feels like there's some pass mis-handling debug locations, and it would be preferable to find that rather than have DwarfDebug cover it up.

[CarlosAlbertoEnciso]

Following the review comments:

1. Changed the test to be based on the instruction sequence (asm) rather than offsets.
2. Removed unnecessary attribute sets from the IR source.

[CarlosAlbertoEnciso]

In D37625#865095, @probinson wrote:

I'm actually more curious why part of the prologue is associated with line 5 and part with line 7. That feels like there's some pass mis-handling debug locations, and it would be preferable to find that rather than have DwarfDebug cover it up.

I understand your valid point, but I think the check introduced by this proposed patch will guarantee that no line records are associated to instructions that are part of the frame setup code.

May I suggest, to keep the extra check and investigate why part of the prologue is associated with line 5 and part with line 7 as a second part of the problem, unless you feel otherwise.

[aprantl]

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1161 ↗

(On Diff #114590)

Duplicate line?

[probinson]

I see that findPrologueEndLoc() is also checking the FrameSetup flag, so at least this makes the handling consistent, and relying on FrameSetup actually seems cleaner than relying on source locations. If you have time to investigate why the source locations seem wrong, that would be great.
LGTM after you fix the comment Adrian pointed out.

[CarlosAlbertoEnciso]

Following the review comments:

Removed a duplicated line.

[CarlosAlbertoEnciso]

As I do not have commit access, Robert Lougher will submit this for me.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1161 ↗

(On Diff #114590)

Thanks. You are correct; that is a duplicated line. Fixed.

[rob.lougher]

This was reverted in r313057 as it was causing buildbot failure (lldb inline stepping tests).

[pmatos]

Was this ever fixed and reapplied? The last I can find on this was r313057 where @rob.lougher reverted r313047.

[CarlosAlbertoEnciso]

In D37625#901422, @pmatos wrote:

Was this ever fixed and reapplied? The last I can find on this was r313057 where @rob.lougher reverted r313047.

Hi,

Sorry about my delay in replying.

The compiler fix, caused couple of LLDB tests to fail.

llvm/tools/lldb/packages/Python/lldbsuite/test/functionalities/inline-stepping

Those tests have been under discussion with the LLDB people. The final changes to be submitted for review.

After that, the compiler fix should be reapplied.

Thanks.

llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

void DwarfDebug::beginInstruction(const MachineInstr *MI) {
   DebugHandlerBase::beginInstruction(MI);
   assert(CurMI);
 
   const auto *SP = MI->getParent()->getParent()->getFunction()->getSubprogram();
   if (!SP || SP->getUnit()->getEmissionKind() == DICompileUnit::NoDebug)
     return;
 
   // Check if source location changes, but ignore DBG_VALUE and CFI locations.
-  if (MI->isMetaInstruction())
+  // If the instruction is part of the function frame setup code, do not emit
+  // any line record, as there is no correspondence with any user code.
+  if (MI->isMetaInstruction() || MI->getFlag(MachineInstr::FrameSetup))
     return;
   const DebugLoc &DL = MI->getDebugLoc();
   // When we emit a line-0 record, we don't update PrevInstLoc; so look at
   // the last line number actually emitted, to see if it was line 0.
   unsigned LastAsmLine =
       Asm->OutStreamer->getContext().getCurrentDwarfLoc().getLine();
 
   if (DL == PrevInstLoc) {

llvm/trunk/test/DebugInfo/X86/invalid-prologue-end.ll

+; RUN: llc -mtriple=x86_64-linux-gnu -filetype=asm %s -o - | FileCheck %s
+
+; The prologue-end line record must be emitted after the last instruction that
+; is part of the function frame setup code and before the instruction that marks
+; the beginning of the function body.
+;
+; For the given test, generated from:
+;
+; 1 extern int get_arg();
+; 2 extern void func(int x);
+; 3
+; 4 int main()
+; 5 {
+; 6   int a;
+; 7   func(get_arg());
+; 8 }
+; 9
+
+; The prologue-end line record is emitted with an incorrect associated address,
+; which causes a debugger to show the beginning of function body to be inside
+; the prologue.
+
+; This can be seen in the following trimmed assembler output:
+;
+; main:
+;   ...
+;   .loc  1 7 0 prologue_end
+;   pushq %rax
+; .Lcfi0:
+;   .cfi_def_cfa_offset 16
+;   callq _Z7get_argv
+;   ...
+;   retq
+
+; The instruction 'pushq %rax' is part of the frame setup code.
+
+; The correct location for the prologue-end line information is just before
+; the call to '_Z7get_argv', as illustrated in the following trimmed
+; assembler output:
+;
+; main:
+;   ...
+;   pushq %rax
+; .Lcfi0:
+;   .cfi_def_cfa_offset 16
+;   .loc  1 7 0 prologue_end
+;   callq _Z7get_argv
+;   ...
+;   retq
+
+; Check that the generated assembler matches the following sequence:
+
+; CHECK:        pushq %rax
+; CHECK-NEXT: .Lcfi0:
+; CHECK-NEXT:   .cfi_def_cfa_offset 16
+; CHECK-NEXT:   .loc  1 7 0 prologue_end {{.*}}# invalid-prologue-end.cpp:7:0
+; CHECK-NEXT:   callq _Z7get_argv
+
+define i32 @main() #0 !dbg !8 {
+  %1 = alloca i32, align 4
+  call void @llvm.dbg.declare(metadata i32* %1, metadata !12, metadata !DIExpression()), !dbg !13
+  %2 = call i32 @_Z7get_argv(), !dbg !14
+  call void @_Z4funci(i32 %2), !dbg !14
+  ret i32 0, !dbg !15
+}
+
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+declare void @_Z4funci(i32) #2
+declare i32 @_Z7get_argv() #2
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5, !6}
+!llvm.ident = !{!7}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 6.0.0 (trunk 312775)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "invalid-prologue-end.cpp", directory: "D:\5C93830")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 2}
+!6 = !{i32 7, !"PIC Level", i32 2}
+!7 = !{!"clang version 6.0.0 (trunk 312775)"}
+!8 = distinct !DISubprogram(name: "main", scope: !1, file: !1, line: 4, type: !9, isLocal: false, isDefinition: true, scopeLine: 5, flags: DIFlagPrototyped, isOptimized: false, unit: !0, variables: !2)
+!9 = !DISubroutineType(types: !10)
+!10 = !{!11}
+!11 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!12 = !DILocalVariable(name: "a", scope: !8, file: !1, line: 6, type: !11)
+!13 = !DILocation(line: 6, scope: !8)
+!14 = !DILocation(line: 7, scope: !8)
+!15 = !DILocation(line: 8, scope: !8)