[DebugInfo] Fix crash when emitting an invalidated SDDbgValue

[StephenTozer]

Summary

This patch resolves an issue reported in review https://reviews.llvm.org/D91722 (post-commit). The error in this case is simple: during the lambda function hasUnknownVReg, used to determine whether an SDDbgValue has any unresolved VReg arguments, constructs an SDValue from any SDNode operands to the debug value. The constructor for an SDValue asserts that it is a valid value; this means that when a debug value has an invalid operand, this check will trigger that assertion and crash the compiler.

The solution in this patch is to only perform this check if !SDDbgValue->isInvalidated(); the check is irrelevant for an invalidated debug value in the first place, because we will set all of its operands to undef, so it doesn't matter whether any of its arguments are unresolved at that point. This check is not used to determine the "correct" insert location for a DBG_VALUE, only the first location where it can be valid, so there should be no harm in short-circuiting this way.

[StephenTozer]

Remove git hash and directory path from test case metadata.

[Orlando]

The change makes sense and LGTM.

Is the test relying on a debug info bug happening I wonder? If so, that would make the test slightly fragile (assuming that bugs will eventually get fixed). Looking at the generated MIR there seems to be a 1:1 mapping of IR to MIR instructions, which I think suggests that the SDDbgValue wasn't updated at some point.

I had a play with this and I've manged to further reduce the test. You can replace the function body with this:

define dso_local i32 @intel_pmu_enable_bts() local_unnamed_addr !dbg !16 {
entry:
  %0 = extractvalue { i32, i64 } zeroinitializer, 1
  %1 = load i32, i32* @intel_pmu_enable_bts_config, align 4
  call void @llvm.dbg.value(metadata !DIArgList(i64 %0, i32 %1), metadata !20, metadata !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 1, DW_OP_or, DW_OP_stack_value)), !dbg !23
  ret i32 %1
}

I guess the test case is may not relying on a "bug" - it feels similar to the situation in IR when an instruction is deleted, the pass doesn't call salvageDebugInfo, and Instructions dtor defensively sets metadata uses to undef. The debug info isn't incorrect, it's just worse than it needs to be. I would be happier if we could generate a test for this which doesn't rely on this kind of thing, because if the underlying issue is fixed the test might become hard to regenerate, but maybe this code path is only ever hit that way? I'm not sure.

EDIT: Sorry, my concern about the test was unfounded. I managed to miss that %asmresult1 isn't used anywhere other than in the dbg.value and subsequently and subsequently doesn't make it through to MIR.

llvm/test/DebugInfo/Generic/invalidated-dbg-value-is-undef.ll

13

You could add --implicit-check-not=DBG_VALUE to FileCheck and remove the CHECK-NOTs? No strong opinion on this though.

[StephenTozer]

Update test case with further reduced function body and implicit-check-not.

[Orlando]

LGTM but please wait a little to see if anyone else has comments on the test, cc @jmorse.

[StephenTozer]

This patch caused some failures on buildbots, caused by the fact that the test requires x86_64 but does not have the explicit REQUIRE set. I've pushed up a followup fix that moves the test from DebugInfo/Generic to DebugInfo/X86, which should fix the issue; I welcome any post-commit review on this change.

[dblaikie]

In D101540#2744380, @StephenTozer wrote:

This patch caused some failures on buildbots, caused by the fact that the test requires x86_64 but does not have the explicit REQUIRE set. I've pushed up a followup fix that moves the test from DebugInfo/Generic to DebugInfo/X86, which should fix the issue; I welcome any post-commit review on this change.

Might be handy to include the commit hash so folks can easily follow-up from here to there. (I think there's some way to write the commit hash in Phab such it it turns it into a link, which is nice)

[StephenTozer]

In D101540#2745215, @dblaikie wrote:

Might be handy to include the commit hash so folks can easily follow-up from here to there. (I think there's some way to write the commit hash in Phab such it it turns it into a link, which is nice)

Thanks for the heads up; the commit hash for the fix is 14818a86d044909d8eeb1f39f689e2785a09823b.

llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp

for (auto DV : DAG->GetDbgValues(N)) {
     unsigned DVOrder = DV->getOrder();
     if (Order != 0 && DVOrder != Order)
       continue;
     // If DV has any VReg location operands which haven't been mapped then
     // either that node is no longer available or we just haven't visited the
     // node yet. In the former case we should emit an undef dbg_value, but we
     // can do it later. And for the latter we'll want to wait until all
     // dependent nodes have been visited.
-    if (HasUnknownVReg(DV))
+    if (!DV->isInvalidated() && HasUnknownVReg(DV))
       continue;
     MachineInstr *DbgMI = Emitter.EmitDbgValue(DV, VRBaseMap);
     if (!DbgMI)
       continue;
     Orders.push_back({DVOrder, DbgMI});
     BB->insert(InsertPos, DbgMI);
   }
 }

llvm/test/DebugInfo/Generic/invalidated-dbg-value-is-undef.ll

+; RUN: llc %s --stop-before=finalize-isel -o - | FileCheck %s --implicit-check-not=DBG_VALUE
+
+;; Check that when a debug value is invalidated during Instruction Selection,
+;; we produce an undef DBG_VALUE/DBG_VALUE_LIST.
+
+; CHECK-LABEL: body:
+
+; CHECK: DBG_VALUE_LIST ![[VAR:[0-9]+]]
+; CHECK-SAME: $noreg, $noreg
+
+
+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"
+
+@intel_pmu_enable_bts_config = external dso_local local_unnamed_addr global i32, align 4, !dbg !0
+
+define dso_local i32 @intel_pmu_enable_bts() local_unnamed_addr !dbg !16 {
+entry:
+  %0 = extractvalue { i32, i64 } zeroinitializer, 1
+  %1 = load i32, i32* @intel_pmu_enable_bts_config, align 4
+  call void @llvm.dbg.value(metadata !DIArgList(i64 %0, i32 %1), metadata !20, metadata !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 1, DW_OP_or, DW_OP_stack_value)), !dbg !23
+  ret i32 %1
+}
+
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!2}
+!llvm.module.flags = !{!14, !15}
+
+!0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
+!1 = distinct !DIGlobalVariable(name: "intel_pmu_enable_bts_config", scope: !2, file: !3, line: 1, type: !8, isLocal: false, isDefinition: true)
+!2 = distinct !DICompileUnit(language: DW_LANG_C99, file: !3, producer: "clang version 13.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !4, globals: !5, splitDebugInlining: false, nameTableKind: None)
+!3 = !DIFile(filename: "invalidated-dbg-value-is-undef.ll", directory: "/")
+!4 = !{}
+!5 = !{!0, !6, !9, !11}
+!6 = !DIGlobalVariableExpression(var: !7, expr: !DIExpression())
+!7 = distinct !DIGlobalVariable(name: "intel_pmu_enable_bts___trans_tmp_1", scope: !2, file: !3, line: 1, type: !8, isLocal: false, isDefinition: true)
+!8 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!9 = !DIGlobalVariableExpression(var: !10, expr: !DIExpression())
+!10 = distinct !DIGlobalVariable(name: "intel_pmu_enable_bts___ecx", scope: !2, file: !3, line: 2, type: !8, isLocal: false, isDefinition: true)
+!11 = !DIGlobalVariableExpression(var: !12, expr: !DIExpression())
+!12 = distinct !DIGlobalVariable(name: "intel_pmu_enable_bts___eax", scope: !2, file: !3, line: 3, type: !13, isLocal: false, isDefinition: true)
+!13 = !DIBasicType(name: "long int", size: 64, encoding: DW_ATE_signed)
+!14 = !{i32 2, !"Debug Info Version", i32 3}
+!15 = !{i32 7, !"uwtable", i32 1}
+!16 = distinct !DISubprogram(name: "intel_pmu_enable_bts", scope: !3, file: !3, line: 4, type: !17, scopeLine: 4, flags: DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !2, retainedNodes: !19)
+!17 = !DISubroutineType(types: !18)
+!18 = !{!8}
+!19 = !{!20, !21}
+!20 = !DILocalVariable(name: "debugctlmsr", scope: !16, file: !3, line: 5, type: !13)
+!21 = !DILocalVariable(name: "low", scope: !16, file: !3, line: 17, type: !22)
+!22 = !DIBasicType(name: "unsigned int", size: 32, encoding: DW_ATE_unsigned)
+!23 = !DILocation(line: 0, scope: !16)