[SelectionDAG] Fix crash for salvaging with indirect debug values #72645
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is a follow-up to llvm#68981. We may end up in SelectionDAG::salvageDebugInfo() with indirect debug values, and attempting to salvage ADD nodes with non-constant RHS would lead us to try to turn those indirect debug values variadic, which is not allowed. This triggered the following assert in the SDDbgValue constructor: Assertion `!(IsVariadic && IsIndirect)' failed. This also adds a lit test for salvaging when having an indirect debug value and constant RHS, as there seems like there was no such lit test. However, I am not sure if the use of the stack_value operation is correct in that case (which is existing behavior before llvm#68981), but that at least documents the current behavior.
|
@llvm/pr-subscribers-llvm-selectiondag @llvm/pr-subscribers-debuginfo Author: David Stenberg (dstenb) ChangesThis is a follow-up to #68981. We may end up in SelectionDAG::salvageDebugInfo() with indirect debug This triggered the following assert in the SDDbgValue constructor: Assertion `!(IsVariadic && IsIndirect)' failed. This also adds a lit test for salvaging when having an indirect debug Full diff: https://github.com/llvm/llvm-project/pull/72645.diff 2 Files Affected:
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
index 1bcd85417eba260..cb79b7a73cd3c72 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
@@ -10852,6 +10852,10 @@ void SelectionDAG::salvageDebugInfo(SDNode &N) {
uint64_t Offset;
if (RHSConstant)
Offset = N.getConstantOperandVal(1);
+ // We are not allowed to turn indirect debug values variadic, so
+ // don't salvage those.
+ if (!RHSConstant && DV->isIndirect())
+ continue;
// Rewrite an ADD constant node into a DIExpression. Since we are
// performing arithmetic to compute the variable's *value* in the
diff --git a/llvm/test/DebugInfo/X86/salvage-add-node-indirect.ll b/llvm/test/DebugInfo/X86/salvage-add-node-indirect.ll
new file mode 100644
index 000000000000000..cae8a479a5ad9da
--- /dev/null
+++ b/llvm/test/DebugInfo/X86/salvage-add-node-indirect.ll
@@ -0,0 +1,103 @@
+; NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 3
+; RUN: llc -mtriple=x86_64 %s -start-before=x86-isel -o - -stop-after=x86-isel | FileCheck %s
+
+; Verify that we don't crash due to attempting to turn the indirect debug value
+; in @test_non_constant variadic when salvaging the ADD node with non-constant
+; RHS (originating from the GEP). This means that the debug location is
+; dropped.
+;
+; We should salvage the debug information in @test_constant.
+; XXX: Is it actually correct to add a stack_value operation in that case?
+
+%struct.x = type { i64, i64, float, i64, double, ptr }
+
+define i64 @test_constant(ptr %rdata) {
+ ; CHECK-LABEL: name: test_constant
+ ; CHECK: bb.0.entry:
+ ; CHECK-NEXT: successors: %bb.1(0x80000000)
+ ; CHECK-NEXT: liveins: $rdi
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: DBG_PHI $rdi, 1
+ ; CHECK-NEXT: [[COPY:%[0-9]+]]:gr64 = COPY $rdi
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: bb.1.for.body31:
+ ; CHECK-NEXT: successors: %bb.2(0x04000000), %bb.1(0x7c000000)
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: DBG_INSTR_REF !4, !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_plus_uconst, 144, DW_OP_deref, DW_OP_stack_value), dbg-instr-ref(1, 0), debug-location !8
+ ; CHECK-NEXT: CMP64mi32 [[COPY]], 1, $noreg, 144, $noreg, 0, implicit-def $eflags :: (load (s64) from %ir.arrayidx33, align 1)
+ ; CHECK-NEXT: JCC_1 %bb.1, 5, implicit $eflags
+ ; CHECK-NEXT: JMP_1 %bb.2
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: bb.2.land.lhs.true.i:
+ ; CHECK-NEXT: [[MOV32r0_:%[0-9]+]]:gr32 = MOV32r0 implicit-def dead $eflags
+ ; CHECK-NEXT: [[SUBREG_TO_REG:%[0-9]+]]:gr64 = SUBREG_TO_REG 0, killed [[MOV32r0_]], %subreg.sub_32bit
+ ; CHECK-NEXT: $rax = COPY [[SUBREG_TO_REG]]
+ ; CHECK-NEXT: RET 0, $rax
+entry:
+ br label %for.body31
+
+for.body31: ; preds = %for.body31, %entry
+ %arrayidx33 = getelementptr [30 x %struct.x], ptr %rdata, i64 0, i64 3
+ call void @llvm.dbg.declare(metadata ptr %arrayidx33, metadata !9, metadata !DIExpression()), !dbg !11
+ %0 = load i64, ptr %arrayidx33, align 1
+ %cmp.i = icmp eq i64 %0, 0
+ br i1 %cmp.i, label %land.lhs.true.i, label %for.body31
+
+land.lhs.true.i: ; preds = %for.body31
+ ret i64 0
+}
+
+define i64 @test_non_constant(ptr %rdata, i64 %i.194) {
+ ; CHECK-LABEL: name: test_non_constant
+ ; CHECK: bb.0.entry:
+ ; CHECK-NEXT: successors: %bb.1(0x80000000)
+ ; CHECK-NEXT: liveins: $rdi, $rsi
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: [[COPY:%[0-9]+]]:gr64_nosp = COPY $rsi
+ ; CHECK-NEXT: [[COPY1:%[0-9]+]]:gr64 = COPY $rdi
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: bb.1.for.body31:
+ ; CHECK-NEXT: successors: %bb.2(0x04000000), %bb.1(0x7c000000)
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: [[LEA64r:%[0-9]+]]:gr64 = LEA64r [[COPY]], 2, [[COPY]], 0, $noreg
+ ; CHECK-NEXT: [[SHL64ri:%[0-9]+]]:gr64_nosp = SHL64ri [[LEA64r]], 4, implicit-def dead $eflags
+ ; CHECK-NEXT: CMP64mi32 [[COPY1]], 1, killed [[SHL64ri]], 0, $noreg, 0, implicit-def $eflags :: (load (s64) from %ir.arrayidx33, align 1)
+ ; CHECK-NEXT: JCC_1 %bb.1, 5, implicit $eflags
+ ; CHECK-NEXT: JMP_1 %bb.2
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: bb.2.land.lhs.true.i:
+ ; CHECK-NEXT: [[MOV32r0_:%[0-9]+]]:gr32 = MOV32r0 implicit-def dead $eflags
+ ; CHECK-NEXT: [[SUBREG_TO_REG:%[0-9]+]]:gr64 = SUBREG_TO_REG 0, killed [[MOV32r0_]], %subreg.sub_32bit
+ ; CHECK-NEXT: $rax = COPY [[SUBREG_TO_REG]]
+ ; CHECK-NEXT: RET 0, $rax
+entry:
+ br label %for.body31
+
+for.body31: ; preds = %for.body31, %entry
+ %arrayidx33 = getelementptr [30 x %struct.x], ptr %rdata, i64 0, i64 %i.194
+ call void @llvm.dbg.declare(metadata ptr %arrayidx33, metadata !4, metadata !DIExpression()), !dbg !8
+ %0 = load i64, ptr %arrayidx33, align 1
+ %cmp.i = icmp eq i64 %0, 0
+ br i1 %cmp.i, label %land.lhs.true.i, label %for.body31
+
+land.lhs.true.i: ; preds = %for.body31
+ ret i64 0
+}
+
+declare void @llvm.dbg.declare(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C11, file: !1, producer: "clang", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, retainedTypes: !2, globals: !2, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "foo.c", directory: "/tmp")
+!2 = !{}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!4 = !DILocalVariable(name: "a", arg: 1, scope: !5, file: !1, line: 33, type: !7)
+!5 = distinct !DISubprogram(name: "test_non_constant", scope: !1, file: !1, line: 33, type: !6, scopeLine: 34, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagLocalToUnit | DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !2)
+!6 = distinct !DISubroutineType(types: !2)
+!7 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "x", file: !1, line: 17, size: 160, elements: !2)
+!8 = !DILocation(line: 33, column: 16, scope: !5)
+!9 = !DILocalVariable(name: "a", arg: 1, scope: !10, file: !1, line: 33, type: !7)
+!10 = distinct !DISubprogram(name: "test_constant", scope: !1, file: !1, line: 33, type: !6, scopeLine: 34, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagLocalToUnit | DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !2)
+!11 = !DILocation(line: 33, column: 16, scope: !10)
|
adrian-prantl
approved these changes
Nov 17, 2023
|
LG, thanks |
dcci
approved these changes
Nov 17, 2023
KanRobert
approved these changes
Nov 18, 2023
This was referenced Nov 18, 2023
sr-tream
pushed a commit
to sr-tream/llvm-project
that referenced
this pull request
Nov 20, 2023
…vm#72645) This is a follow-up to llvm#68981, and fix for llvm#72630, llvm#72447. We may end up in SelectionDAG::salvageDebugInfo() with indirect debug values, and attempting to salvage ADD nodes with non-constant RHS would lead us to try to turn those indirect debug values variadic, which is not allowed. This triggered the following assert in the SDDbgValue constructor: Assertion `!(IsVariadic && IsIndirect)' failed. This also adds a lit test for salvaging when having an indirect debug value and constant RHS, as there seems like there was no such lit test. However, I am not sure if the use of the stack_value operation is correct in that case (which is existing behavior before llvm#68981), but that at least documents the current behavior.
zahiraam
pushed a commit
to zahiraam/llvm-project
that referenced
this pull request
Nov 20, 2023
…vm#72645) This is a follow-up to llvm#68981, and fix for llvm#72630, llvm#72447. We may end up in SelectionDAG::salvageDebugInfo() with indirect debug values, and attempting to salvage ADD nodes with non-constant RHS would lead us to try to turn those indirect debug values variadic, which is not allowed. This triggered the following assert in the SDDbgValue constructor: Assertion `!(IsVariadic && IsIndirect)' failed. This also adds a lit test for salvaging when having an indirect debug value and constant RHS, as there seems like there was no such lit test. However, I am not sure if the use of the stack_value operation is correct in that case (which is existing behavior before llvm#68981), but that at least documents the current behavior.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a follow-up to #68981, and fix for #72630, #72447.
We may end up in SelectionDAG::salvageDebugInfo() with indirect debug
values, and attempting to salvage ADD nodes with non-constant RHS would
lead us to try to turn those indirect debug values variadic, which is
not allowed.
This triggered the following assert in the SDDbgValue constructor:
Assertion `!(IsVariadic && IsIndirect)' failed.
This also adds a lit test for salvaging when having an indirect debug
value and constant RHS, as there seems like there was no such lit test.
However, I am not sure if the use of the stack_value operation is
correct in that case (which is existing behavior before #68981), but
that at least documents the current behavior.