[DebugInfo] LiveDebugValues: correctly discriminate between kinds of variable locations

[jmorse]

Summary

The missing line added by this patch ensures that only spilt variable locations are candidates for being restored from the stack. Otherwise, register or constant-value information can be interpreted as a spill location, through a union.

The added regression test replicates a scenario where this occurs: the stack load from [rsp] causes the register-location DBG_VALUE to be "restored" to rsi, when it should be left alone. See PR43058 for details.

Un x-fail a test that was suffering from this from a previous patch.

This causes a clang-3.4 build to gain 2% more scope-covered bytes, because the scenario in the regression test was interfering with stack variable locations created by LiveDebugVariables.

[aprantl]

Nice!

[jmorse]

A case of mistaken identity meant I linked to this review in commit r370648, the real review for that commit is D58450, sorry for that extra noise.

lib/CodeGen/LiveDebugValues.cpp

void LiveDebugValues::transferSpillOrRestoreInst(MachineInstr &MI,
   }
   // Check if the register or spill location is the location of a debug value.
   for (unsigned ID : OpenRanges.getVarLocs()) {
     if (TKind == TransferKind::TransferSpill &&
         VarLocIDs[ID].isDescribedByReg() == Reg) {
       LLVM_DEBUG(dbgs() << "Spilling Register " << printReg(Reg, TRI) << '('
                         << VarLocIDs[ID].Var.getVar()->getName() << ")\n");
     } else if (TKind == TransferKind::TransferRestore &&
+               VarLocIDs[ID].Kind == VarLoc::SpillLocKind &&
                VarLocIDs[ID].Loc.SpillLocation == *Loc) {
       LLVM_DEBUG(dbgs() << "Restoring Register " << printReg(Reg, TRI) << '('
                         << VarLocIDs[ID].Var.getVar()->getName() << ")\n");
     } else
       continue;
     insertTransferDebugPair(MI, OpenRanges, Transfers, VarLocIDs, ID, TKind,
                             Reg);
     return;

test/CodeGen/ARM/debug-info-blocks.ll

 ; RUN: llc -filetype=obj -O0 < %s | llvm-dwarfdump -v - | FileCheck %s
 
-; XFAIL: *
-; PR43058
-
 ; debug_info content
 ; CHECK: DW_AT_name {{.*}} "foobar_func_block_invoke_0"
 ; CHECK-NOT: DW_TAG_subprogram
 ; CHECK: DW_TAG_variable
 ; CHECK-NOT: DW_TAG
 ; CHECK-NEXT: DW_AT_location [DW_FORM_sec_offset]
 ; CHECK-NEXT:    [0x{{.*}}, 0x{{.*}}): {{.*}} DW_OP_plus_uconst 0x4, DW_OP_deref, DW_OP_plus_uconst 0x18
 ; CHECK-NEXT:    [0x{{.*}}, 0x{{.*}}): {{.*}} DW_OP_plus_uconst 0x4, DW_OP_deref, DW_OP_plus_uconst 0x18

test/DebugInfo/MIR/X86/live-debug-values-restore-collide.mir

+# RUN: llc %s -o - -run-pass=livedebugvalues -mtriple=x86_64-unknown-unknown | FileCheck %s
+#
+# This is a regression test for register/stack variable locations being
+# interpreted as each other. We place a variable location on the stack, based
+# on $rsp. We later make a memory load from $rsp. On faulty versions of LLVM
+# this gets interpreted as a stack restore of the spilt value (it's not).
+#
+# Test that LiveDebugValues does not create any new variable locations.
+#
+# CHECK-LABEL: name: baaar
+# CHECK-LABEL: bb.0.entry:
+# CHECK:       DBG_VALUE $r9d, $noreg, !{{[0-9]*}}, !DIExpression()
+# CHECK:       DBG_VALUE $rsp, 0, !{{[0-9]*}}, !DIExpression(DW_OP_plus_uconst, 16)
+# CHECK-NOT:   DBG_VALUE
+--- |
+  target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+  target triple = "x86_64-unknown-linux-gnu"
+  
+  define void @baaar() !dbg !4 {
+  entry:
+    ret void
+  }
+  
+  !llvm.module.flags = !{!0, !1}
+  !llvm.dbg.cu = !{!2}
+  
+  !0 = !{i32 2, !"Debug Info Version", i32 3}
+  !1 = !{i32 2, !"Dwarf Version", i32 4}
+  !2 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !3, producer: "", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug)
+  !3 = !DIFile(filename: "sheep.cpp", directory: ".")
+  !4 = distinct !DISubprogram(name: "baaar", scope: !3, file: !3, line: 1, type: !5, spFlags: DISPFlagDefinition, unit: !2, retainedNodes: !8)
+  !5 = !DISubroutineType(types: !6)
+  !6 = !{!7}
+  !7 = !DIBasicType(name: "int", size: 32, align: 32, encoding: DW_ATE_signed)
+  !8 = !{!9}
+  !9 = !DILocalVariable(name: "moo", scope: !4, file: !3, line: 1, type: !7)
+  !10 = !DILocation(line: 1, scope: !4)
+
+...
+---
+name:            baaar
+alignment:       4
+tracksRegLiveness: true
+liveins:
+  - { reg: '$rdi', virtual-reg: '' }
+  - { reg: '$rsi', virtual-reg: '' }
+  - { reg: '$rdx', virtual-reg: '' }
+  - { reg: '$rcx', virtual-reg: '' }
+  - { reg: '$r8', virtual-reg: '' }
+  - { reg: '$r9d', virtual-reg: '' }
+frameInfo:
+  stackSize:       24
+  offsetAdjustment: -24
+  maxAlignment:    8
+  adjustsStack:    true
+  hasCalls:        true
+fixedStack:
+stack:
+  - { id: 0, name: '', type: spill-slot, offset: -16, size: 8, alignment: 8, 
+      stack-id: default, callee-saved-register: '', callee-saved-restored: true, 
+      debug-info-variable: '', debug-info-expression: '', debug-info-location: '' }
+  - { id: 1, name: '', type: spill-slot, offset: -32, size: 8, alignment: 8, 
+      stack-id: default, callee-saved-register: '', callee-saved-restored: true, 
+      debug-info-variable: '', debug-info-expression: '', debug-info-location: '' }
+body:             |
+  bb.0.entry:
+    liveins: $rcx, $rdi, $rdx, $rsi, $r8, $r9d, $rbp, $r15, $r14, $r13, $r12, $rbx
+  
+    ; Setup
+    DBG_VALUE $r9d, $noreg, !9, !DIExpression(), debug-location !10
+    $rsp = frame-setup SUB64ri8 $rsp, 24, implicit-def dead $eflags
+
+    ; Spill
+    MOV32mr $rsp, 1, $noreg, 16, $noreg, $r9d :: (store 4 into %stack.0)
+    DBG_VALUE $rsp, 0, !9, !DIExpression(DW_OP_plus_uconst, 16), debug-location !10
+
+    ; This load from the stack can be misinterpreted as a stack restore of
+    ; any DBG_VALUe pointing at $rsp.
+    $rsi = MOV64rm $rsp, 1, $noreg, 0, $noreg :: (load 8 from %stack.1)
+
+    ; Return faff
+    $eax = MOV32ri 0
+    $rsp = frame-destroy ADD64ri8 $rsp, 24, implicit-def dead $eflags
+    RETQ debug-location !10
+...