[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–1164

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–1164

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.

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())
+  // Check if source location changes, but ignore DBG_VALUE and CFI locations.
+  // 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) {

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

+; RUN: llc -mtriple=x86_64-linux-gnu -filetype=obj -o - %s | llvm-dwarfdump --debug-dump=line - | 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, 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.
+;
+; Using the output from llvm-dwarfdump, this can be illustrated as:
+;
+; Address            Line   Column File   ISA Discriminator Flags
+; ------------------ ------ ------ ------ --- ------------- -------------
+; 0x0000000000000000      5      0      1   0             0  is_stmt
+; 0x0000000000000004      7      0      1   0             0  is_stmt prologue_end
+; 0x0000000000000014      8      0      1   0             0  is_stmt
+; 0x000000000000001c      8      0      1   0             0  is_stmt end_sequence
+;
+; The address 0x0000000000000004 is still part of the prologue. The correct
+; associated address must be 0x0000000000000008, which is where the call
+; instruction for 'get_arg' is located.
+
+; 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
+
+; Check that the generated line records match the following sequence:
+
+; CHECK: 0x0000000000000000      5      0      1   0             0  is_stmt
+; CHECK: 0x0000000000000008      7      0      1   0             0  is_stmt prologue_end
+; CHECK: 0x0000000000000014      8      0      1   0             0  is_stmt
+; CHECK: 0x000000000000001c      8      0      1   0             0  is_stmt end_sequence
+
+; Function Attrs: noinline norecurse nounwind optnone sspstrong uwtable
+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
+}
+
+; Function Attrs: nounwind readnone speculatable
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+declare void @_Z4funci(i32) #2
+
+declare i32 @_Z7get_argv() #2
+
+attributes #0 = { noinline norecurse nounwind optnone sspstrong uwtable "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="btver2" "target-features"="+aes,+avx,+bmi,+cx16,+f16c,+fxsr,+lzcnt,+mmx,+movbe,+pclmul,+popcnt,+prfchw,+sse,+sse2,+sse3,+sse4.1,+sse4.2,+sse4a,+ssse3,+x87,+xsave,+xsaveopt" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { nounwind readnone speculatable }
+attributes #2 = { "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="btver2" "target-features"="+aes,+avx,+bmi,+cx16,+f16c,+fxsr,+lzcnt,+mmx,+movbe,+pclmul,+popcnt,+prfchw,+sse,+sse2,+sse3,+sse4.1,+sse4.2,+sse4a,+ssse3,+x87,+xsave,+xsaveopt" "unsafe-fp-math"="false" "use-soft-float"="false" }
+
+!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)