[DWARF] Suppress .loc directives coming from CFI instructions

[probinson]

Summary

CFI pseudo-instructions emit nothing to the .text section, therefore there's no reason to emit their source locations into the line table.
In practice, a CFI instruction's source location will be one of three things: (a) exactly the same as the preceding instruction; (b) exactly the same as the following instruction; (c) unspecified. The first two cases have no net effect on the line table. The third case will emit a "line 0" record whenever we turn unspecified locations into "line 0" which can happen with -use-unknown-locations or, after the patch in D24180 lands (again), if the CFI instruction happens to be at the top of a basic block. This patch causes the third case to have no net effect on the line table, just like the other cases.

[aprantl]

Seems reasonable.

test/DebugInfo/Generic/no-cfi-loc.ll

2 ↗

(On Diff #80503)

Short comment on what is being checked here?
And is there anything positive that could be checked for, too? Tests that only CHECK-NOT love to break without anybody noticing :-)

[probinson]

test/DebugInfo/Generic/no-cfi-loc.ll

2 ↗

(On Diff #80503)

It ends up emitting a CFI instruction with no debug location. The test is verifying that this does not result in a line-0 record (that we skip the CFI instruction).

Although, I now realize the fact that there is a CFI instruction at all is not obvious! Maybe I should turn this into an MIR test instead.

[aprantl]

test/DebugInfo/Generic/no-cfi-loc.ll

2 ↗

(On Diff #80503)

My main point was that a test like this would work even if you symlink llc to /usr/bin/true. I would suggest to at least check for the function label and one other .loc directive that is expected to be in the output.

[probinson]

test/DebugInfo/Generic/no-cfi-loc.ll

2 ↗

(On Diff #80503)

Sorry, didn't see this before I committed. The new test does look for the .cfi directive, and verifies that no line-0 directive appears before that.
I could add more positive checks if you think that would be more robust.

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

void DwarfDebug::collectVariableInfo(DwarfCompileUnit &TheCU,
   }
 }
 
 // Process beginning of an instruction.
 void DwarfDebug::beginInstruction(const MachineInstr *MI) {
   DebugHandlerBase::beginInstruction(MI);
   assert(CurMI);
 
-  // Check if source location changes, but ignore DBG_VALUE locations.
-  if (MI->isDebugValue())
+  // Check if source location changes, but ignore DBG_VALUE and CFI locations.
+  if (MI->isDebugValue() || MI->isCFIInstruction())
     return;
   const DebugLoc &DL = MI->getDebugLoc();
   if (DL == PrevInstLoc)
     return;
 
   if (!DL) {
     // We have an unspecified location, which might want to be line 0.
     if (UnknownLocations) {

llvm/trunk/test/DebugInfo/MIR/X86/no-cfi-loc.mir

+# Verify that a CFI instruction with no debug location
+# does not result in a line-0 location in the assembler.
+# RUN: %llc_dwarf -start-after=prologepilog -march=x86-64 -use-unknown-locations %s -o - | FileCheck %s
+#
+# CHECK-NOT: .loc 1 0
+# CHECK:     .cfi_def_cfa_offset
+
+--- |
+  ; ModuleID = 'no-cfi-loc.ll'
+  source_filename = "no-cfi-loc.ll"
+  target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+  
+  define void @foo() !dbg !6 {
+  entry:
+    call void @bar(), !dbg !8
+    ret void, !dbg !9
+  }
+  
+  declare void @bar()
+  
+  ; Function Attrs: nounwind
+  declare void @llvm.stackprotector(i8*, i8**) #0
+  
+  attributes #0 = { nounwind }
+  
+  !llvm.dbg.cu = !{!0}
+  !llvm.module.flags = !{!3, !4}
+  !llvm.ident = !{!5}
+  
+  !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 4.0.0 (trunk 288778)", isOptimized: false, runtimeVersion: 0, emissionKind: LineTablesOnly, enums: !2)
+  !1 = !DIFile(filename: "t.c", directory: "/home/probinson/projects/scratch")
+  !2 = !{}
+  !3 = !{i32 2, !"Dwarf Version", i32 4}
+  !4 = !{i32 2, !"Debug Info Version", i32 3}
+  !5 = !{!"clang version 4.0.0 (trunk 288778)"}
+  !6 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 2, type: !7, isLocal: false, isDefinition: true, scopeLine: 2, flags: DIFlagPrototyped, isOptimized: false, unit: !0, variables: !2)
+  !7 = !DISubroutineType(types: !2)
+  !8 = !DILocation(line: 3, column: 2, scope: !6)
+  !9 = !DILocation(line: 4, column: 1, scope: !6)
+
+...
+---
+name:            foo
+alignment:       4
+exposesReturnsTwice: false
+legalized:       false
+regBankSelected: false
+selected:        false
+tracksRegLiveness: true
+calleeSavedRegisters: [ '%bh', '%bl', '%bp', '%bpl', '%bx', '%ebp', '%ebx', 
+                        '%rbp', '%rbx', '%r12', '%r13', '%r14', '%r15', 
+                        '%r12b', '%r13b', '%r14b', '%r15b', '%r12d', '%r13d', 
+                        '%r14d', '%r15d', '%r12w', '%r13w', '%r14w', '%r15w' ]
+frameInfo:       
+  isFrameAddressTaken: false
+  isReturnAddressTaken: false
+  hasStackMap:     false
+  hasPatchPoint:   false
+  stackSize:       8
+  offsetAdjustment: 0
+  maxAlignment:    0
+  adjustsStack:    true
+  hasCalls:        true
+  maxCallFrameSize: 0
+  hasOpaqueSPAdjustment: false
+  hasVAStart:      false
+  hasMustTailInVarArgFunc: false
+body:             |
+  bb.0.entry:
+    frame-setup PUSH64r undef %rax, implicit-def %rsp, implicit %rsp, debug-location !8
+    CFI_INSTRUCTION def_cfa_offset 16
+    CALL64pcrel32 @bar, csr_64, implicit %rsp, implicit-def %rsp, debug-location !8
+    %rax = POP64r implicit-def %rsp, implicit %rsp, debug-location !9
+    RET 0, debug-location !9
+
+...