[DebugInfo] Handle undefined values for DBG_PHIs in InstrRef-LiveDebugValues

[StephenTozer]

Summary

Fixes https://github.com/llvm/llvm-project/issues/62725

When running llc on the IR file in the above issue, after ISel there is a DBG_INSTR_REF and a single DBG_PHI that it references; at some point in the MIR pipeline, the block containing the DBG_PHI becomes unreachable. When we reach LiveDebugValues, this results in a crash due to unreachable blocks not being considered: In the initial tracking of DBG_PHIs, we will set the value of the lone DBG_PHI as the live-in value at the phi location, i.e. {4,0,4}. In ExtendRanges we perform a machine value dataflow analysis and apply the result to the collected DBG_PHIs. *Generally* this means each DBG_PHI will be updated to use the resolved value if one is found, and otherwise is unchanged. When the DBG_PHI is in an unreachable block however it will be updated to use the empty value instead, which is inconsistent with other phi values (which use an empty optional instead), resulting in us attempting to use the empty value as an actual value and triggering an assertion.

This patch fixes this error by checking for empty live-in values after the data flow analysis, and continuing to use the existing phi value if no value was found for that phi in the analysis. This prevents crashes from mishandling the empty value internally, and results in the correct value being found for any users of the DBG_PHI.

[jmorse]

This works around the reported crash, which is great. However, there are going to be ValueIDNums with slightly different interpretations as a result of the DebugPHIRecord not being updated. I don't *think* (90% sure) that's going to lead to incorrect variable locations, as the machine-location problem being solved just removes un-necessary PHIs. Thus, not updating the ValueIDNum which will have previously referred to a live-in value (aka a PHI):

• If the PHI is eliminated, the ValueIDNum will refer to a value that's never defined, therefore any variables using this value will be optimized out (which is fine for now),
• If the PHI is retained, then ValueIDNum will refer to it , and it will be correct (I think?).

Would you agree?

Could you stick a FIXME in the code-comment you've added indicating that we should find a better overall solution than just not updating the live-in ValueIDNum.

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp

3720

NB, PHI not Phi

llvm/test/DebugInfo/X86/instr-ref-unreachable.mir

1–8

File-comment indicating what's the intention behind the test is please

[StephenTozer]

In D150707#4349347, @jmorse wrote:

Would you agree?

I think that this change might just be the right thing to do: If the DBG_PHI does not refer to an invalid location, such as a dead stack slot, then the PHI-elimination step should always identify a non-empty value for that location: either the PHI for that location, or some non-empty value if we eliminated that PHI. The only exception to this as far as I can tell is if the block containing the DBG_PHI is unreachable, in which case all incoming values will be the empty value. Hence this code assumes that if we observe an empty value for a DBG_PHI with a valid location, it must be because this block is unreachable.

In that case, leaving the DBG_PHI value unchanged should mean it continues to refer to the live-in value for that location, which I think is correct - we have no way of knowing what the live-in value will be if the block is reached by an indirect jump, so leaving the DBG_PHI referring to the live-in value seems like the sensible approach to me; WDYT?

[jmorse]

That sounds alright; some of the trouble is that I don't like the terminology of "leaving" it as a live-in -- through this step we effectively transform live-in ValueIDNums into PHI ValueIDNums and intend for them to be correct.

With that in mind, could you revise the comment into something like... " If there is no resolved value for this live-in then it is not directly reachable from the entry block -- model it as a PHI on entry to this block, which means we leave the ValueIDNum unchanged" if that sounds alright to you?

[StephenTozer]

SGTM; added and updated comments as suggested.

[jmorse]

LGTM. Interesting that we get a DBG_PHI dominated by another same-numbered DBG_PHI here, I don't think that's a problem for any code we have but a curiosity to be aware of.

[StephenTozer]

In D150707#4352523, @jmorse wrote:

LGTM. Interesting that we get a DBG_PHI dominated by another same-numbered DBG_PHI here, I don't think that's a problem for any code we have but a curiosity to be aware of.

That example is entirely artificial, it just exists in the test to provide an example where one DBG_PHI being unreachable doesn't automatically undef the user. I would have thought it possible that some combination of legal CFG transformations could cause one DBG_PHI to become dominated by another, but if you think it's infeasible and undesirable to include then I can remove that part from the test.

[jmorse]

In D150707#4352764, @StephenTozer wrote:

That example is entirely artificial, it just exists in the test to provide an example where one DBG_PHI being unreachable doesn't automatically undef the user. I would have thought it possible that some combination of legal CFG transformations could cause one DBG_PHI to become dominated by another, but if you think it's infeasible and undesirable to include then I can remove that part from the test.

Best to remove it then IMO, and just exercise the one with instr-ref number 1 one, to ensure that the crash is avoided. Alternately, put something in bb.4 checking that the location can be resolved inside a block where the live-in value is available?

[StephenTozer]

It looks as though when the DBG_INSTR_REF is in the non-directly-reachable block, even when it is in the same block as the DBG_PHI, we give it a $noreg value after LDV. I've left this in the test with a FIXME comment describing the situation; although it's a niche case, I think it's worth keeping it in the test as a potential future fixup, WDYT?

[jmorse]

Seems fine as a FIXME as we're aiming to avoid the crash for now

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp

if (!DBG_PHI.ValueRead)
       continue;
 
     ValueIDNum &Num = *DBG_PHI.ValueRead;
     if (!Num.isPHI())
       continue;
 
     unsigned BlockNo = Num.getBlock();
     LocIdx LocNo = Num.getLoc();
-    Num = MInLocs[BlockNo][LocNo.asU64()];
+    ValueIDNum ResolvedValue = MInLocs[BlockNo][LocNo.asU64()];
+    // If the resolved value for this Phi is the EmptyValue, then there is never
+    // a def for that location that reaches this block (likely due to this block
+    // being unreachable), so this Phi's value should stay as the live-in value.
+    if (ResolvedValue != ValueIDNum::EmptyValue)
+      Num = ResolvedValue;
   }
   // Later, we'll be looking up ranges of instruction numbers.
   llvm::sort(DebugPHINumToValue);
 
   // Walk back through each block / instruction, collecting DBG_VALUE
   // instructions and recording what machine value their operands refer to.
   for (auto &OrderPair : OrderToBB) {
     MachineBasicBlock &MBB = *OrderPair.second;

llvm/test/DebugInfo/X86/instr-ref-unreachable.mir

+# RUN: llc --run-pass=livedebugvalues %s -o - -experimental-debug-variable-locations | FileCheck %s
+
+# CHECK-DAG: ![[UNDEFVAR:[0-9]+]] = !DILocalVariable(name: "undef"
+# CHECK-DAG: ![[DEFVAR:[0-9]+]] = !DILocalVariable(name: "def"
+
+# CHECK-LABEL: bb.1.bb1
+# CHECK: DBG_VALUE_LIST ![[UNDEFVAR]], {{.+}}, $noreg
+# CHECK: DBG_VALUE_LIST ![[DEFVAR]], {{.+}}, $rcx
+
+--- |
+  ; ModuleID = 'llvm/test/DebugInfo/X86/instr-ref-unreachable.mir'
+  source_filename = "/tmp/b.ll"
+  target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+  target triple = "x86_64-unknown-linux-gnu"
+  
+  @global = private constant [2 x ptr] [ptr blockaddress(@foo, %bb3), ptr blockaddress(@foo, %bb1)]
+  
+  define ptr @foo() !dbg !4 {
+  bb:
+    br label %bb3
+  
+  bb1:                                              ; preds = %bb3
+    call void @llvm.dbg.value(metadata i64 %phi, metadata !6, metadata !DIExpression()), !dbg !8
+    call void @llvm.dbg.value(metadata i64 %phi, metadata !9, metadata !DIExpression()), !dbg !8
+    store i32 0, ptr null, align 4, !dbg !10
+    %getelementptr = getelementptr i32, ptr null, i64 %phi
+    store i32 0, ptr %getelementptr, align 4
+    br label %bb2
+  
+  bb2:                                              ; preds = %bb2, %bb1
+    %select = select i1 false, i1 false, i1 false
+    br i1 %select, label %bb3, label %bb2
+  
+  bb3:                                              ; preds = %bb2, %bb
+    %phi = phi i64 [ 1, %bb ], [ 0, %bb2 ]
+    br label %bb1
+  }
+  
+  declare void @llvm.dbg.value(metadata, metadata, metadata)
+  
+  !llvm.dbg.cu = !{!0}
+  !llvm.module.flags = !{!3}
+  
+  !0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, retainedTypes: !2, globals: !2, imports: !2, splitDebugInlining: false, nameTableKind: GNU)
+  !1 = !DIFile(filename: "test.cpp", directory: ".")
+  !2 = !{}
+  !3 = !{i32 2, !"Debug Info Version", i32 3}
+  !4 = distinct !DISubprogram(name: "GetNumericFormat", linkageName: "f", scope: null, file: !1, line: 980, type: !5, scopeLine: 984, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !2)
+  !5 = distinct !DISubroutineType(types: !2)
+  !6 = !DILocalVariable(name: "undef", scope: !4, file: !1, line: 263, type: !7)
+  !7 = !DIBasicType(name: "unsigned long", size: 64, encoding: DW_ATE_unsigned)
+  !8 = !DILocation(line: 0, scope: !4)
+  !9 = !DILocalVariable(name: "def", scope: !4, file: !1, line: 263, type: !7)
+  !10 = !DILocation(line: 151, column: 41, scope: !4)
+
+...
+---
+name:            foo
+alignment:       16
+tracksRegLiveness: true
+debugInstrRef:   true
+tracksDebugUserValues: true
+frameInfo:
+  maxAlignment:    1
+body:             |
+  bb.0.bb:
+    successors: %bb.1(0x80000000)
+  
+    $ecx = MOV32ri 1, implicit-def $rcx
+    renamable $al = MOV8ri 1
+  
+  bb.1.bb1 (ir-block-address-taken %ir-block.bb1, align 16):
+    successors: %bb.2(0x80000000)
+    liveins: $al, $rcx
+  
+    DBG_PHI $rcx, 2
+    DBG_INSTR_REF !6, !DIExpression(DW_OP_LLVM_arg, 0), dbg-instr-ref(1, 0), debug-location !8
+    DBG_INSTR_REF !9, !DIExpression(DW_OP_LLVM_arg, 0), dbg-instr-ref(2, 0), debug-location !8
+    MOV32mi $noreg, 1, $noreg, 0, $noreg, 0, debug-location !10 :: (store (s32) into `ptr null`)
+    MOV32mi $noreg, 4, killed renamable $rcx, 0, $noreg, 0 :: (store (s32) into %ir.getelementptr)
+  
+  bb.2.bb2 (align 16):
+    successors: %bb.3(0x04000000), %bb.2(0x7c000000)
+    liveins: $al
+  
+    TEST8rr renamable $al, renamable $al, implicit-def $eflags
+    JCC_1 %bb.2, 5, implicit killed $eflags
+  
+  bb.3:
+    successors: %bb.1(0x80000000)
+    liveins: $al
+  
+    renamable $ecx = XOR32rr undef $ecx, undef $ecx, implicit-def dead $eflags, implicit-def $rcx
+    JMP_1 %bb.1
+  
+  bb.4.bb3 (ir-block-address-taken %ir-block.bb3):
+    successors: %bb.1(0x80000000)
+    liveins: $al, $rcx
+  
+    DBG_PHI $rcx, 1
+    DBG_PHI $rcx, 2
+    JMP_1 %bb.1
+
+...