[Verifier] Require that dbg.declare variable is a ptr #134355

nikic · 2025-04-04T07:53:15Z

As far as I understand, the first operand of dbg_declare should be a pointer (inside a metadata wrapper). However, using a non-pointer is currently not rejected, and we have some tests that use non-pointer types. As far as I can tell, these tests either meant to use dbg_value or are just incorrect hand-crafted tests.

Ran into this while trying to fix #134008.

As far as I understand, the first operand of dbg_declare should be a pointer (inside a metadata wrapper). However, using a non-pointer is currently not rejected, and we have some tests that use non-pointer types. As far as I can tell, these tests either meant to use dbg_value or are just incorrect hand-crafted tests. Ran into this while trying to fix llvm#134008.

llvmbot · 2025-04-04T07:53:48Z

@llvm/pr-subscribers-llvm-ir
@llvm/pr-subscribers-backend-aarch64

@llvm/pr-subscribers-backend-x86

Author: Nikita Popov (nikic)

Changes

As far as I understand, the first operand of dbg_declare should be a pointer (inside a metadata wrapper). However, using a non-pointer is currently not rejected, and we have some tests that use non-pointer types. As far as I can tell, these tests either meant to use dbg_value or are just incorrect hand-crafted tests.

Ran into this while trying to fix #134008.

Full diff: https://github.com/llvm/llvm-project/pull/134355.diff

8 Files Affected:

(modified) llvm/lib/IR/Verifier.cpp (+5-2)
(modified) llvm/test/CodeGen/AArch64/fast-isel-dbg.ll (+2-2)
(modified) llvm/test/CodeGen/AArch64/selectiondag-order.ll (+2-1)
(modified) llvm/test/CodeGen/MIR/X86/diexpr-win32.mir (+1-1)
(modified) llvm/test/CodeGen/X86/selectiondag-order.ll (+2-1)
(modified) llvm/test/DebugInfo/ARM/lowerbdgdeclare_vla.ll (+2-2)
(modified) llvm/test/Transforms/Coroutines/coro-debug.ll (+9-9)
(modified) llvm/test/Transforms/LoopVectorize/discriminator.ll (+2-2)

diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index 7c6cd414554e3..81ddb62fb9561 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -6666,9 +6666,12 @@ void Verifier::visit(DbgVariableRecord &DVR) {
   CheckDI(MD && (isa<ValueAsMetadata>(MD) || isa<DIArgList>(MD) ||
                  (isa<MDNode>(MD) && !cast<MDNode>(MD)->getNumOperands())),
           "invalid #dbg record address/value", &DVR, MD);
-  if (auto *VAM = dyn_cast<ValueAsMetadata>(MD))
+  if (auto *VAM = dyn_cast<ValueAsMetadata>(MD)) {
     visitValueAsMetadata(*VAM, F);
-  else if (auto *AL = dyn_cast<DIArgList>(MD))
+    if (DVR.isDbgDeclare())
+      CheckDI(VAM->getValue()->getType()->isPointerTy(),
+              "location of #dbg_declare must be a pointer", &DVR, MD);
+  } else if (auto *AL = dyn_cast<DIArgList>(MD))
     visitDIArgList(*AL, F);
 
   CheckDI(isa_and_nonnull<DILocalVariable>(DVR.getRawVariable()),
diff --git a/llvm/test/CodeGen/AArch64/fast-isel-dbg.ll b/llvm/test/CodeGen/AArch64/fast-isel-dbg.ll
index d17c14747b942..ac6fa71e9a9e4 100644
--- a/llvm/test/CodeGen/AArch64/fast-isel-dbg.ll
+++ b/llvm/test/CodeGen/AArch64/fast-isel-dbg.ll
@@ -6,8 +6,8 @@ target triple="aarch64--"
 
 ; CHECK-LABEL: name: func
 ; CHECK: DBG_VALUE
-define void @func(i32 %a) !dbg !4 {
-  call void @llvm.dbg.declare(metadata i32 %a, metadata !5, metadata !DIExpression()), !dbg !7
+define void @func(ptr %a) !dbg !4 {
+  call void @llvm.dbg.declare(metadata ptr %a, metadata !5, metadata !DIExpression()), !dbg !7
   ret void
 }
 
diff --git a/llvm/test/CodeGen/AArch64/selectiondag-order.ll b/llvm/test/CodeGen/AArch64/selectiondag-order.ll
index fb40653723fec..a633fc0785896 100644
--- a/llvm/test/CodeGen/AArch64/selectiondag-order.ll
+++ b/llvm/test/CodeGen/AArch64/selectiondag-order.ll
@@ -55,8 +55,9 @@ end:                                        ; preds = %body
 
 define i64 @simulateWithDbgDeclare(<2 x i32> %a) local_unnamed_addr  {
 entry:
+  %ptr = alloca i32
   %rand = tail call i64 @lrand48() #3
-  tail call void @llvm.dbg.declare(metadata i64 %rand, metadata !6, metadata !7), !dbg !8
+  tail call void @llvm.dbg.declare(metadata ptr %ptr, metadata !6, metadata !7), !dbg !8
   br label %body
 
 body:                                        ; preds = %body, %entry
diff --git a/llvm/test/CodeGen/MIR/X86/diexpr-win32.mir b/llvm/test/CodeGen/MIR/X86/diexpr-win32.mir
index b1bcf24f8c5f4..d8d76758a08a0 100644
--- a/llvm/test/CodeGen/MIR/X86/diexpr-win32.mir
+++ b/llvm/test/CodeGen/MIR/X86/diexpr-win32.mir
@@ -82,7 +82,7 @@
   entry:
     %0 = bitcast ptr %s to ptr
     %bytes = load i32, ptr %0, !dbg !34
-    call void @llvm.dbg.declare(metadata i32 %bytes, metadata !35, metadata !28), !dbg !34
+    call void @llvm.dbg.value(metadata i32 %bytes, metadata !35, metadata !28), !dbg !34
     %1 = add i32 %bytes, %acc, !dbg !36
     ret i32 %1, !dbg !36
   }
diff --git a/llvm/test/CodeGen/X86/selectiondag-order.ll b/llvm/test/CodeGen/X86/selectiondag-order.ll
index 163e2cb90b2fe..924211325c2b0 100644
--- a/llvm/test/CodeGen/X86/selectiondag-order.ll
+++ b/llvm/test/CodeGen/X86/selectiondag-order.ll
@@ -55,8 +55,9 @@ end:                                        ; preds = %body
 
 define i64 @simulateWithDbgDeclare(<2 x i32> %a) local_unnamed_addr  {
 entry:
+  %ptr = alloca i32
   %rand = tail call i64 @lrand48() #3
-  tail call void @llvm.dbg.declare(metadata i64 %rand, metadata !6, metadata !7), !dbg !8
+  tail call void @llvm.dbg.declare(metadata ptr %ptr, metadata !6, metadata !7), !dbg !8
   br label %body
 
 body:                                        ; preds = %body, %entry
diff --git a/llvm/test/DebugInfo/ARM/lowerbdgdeclare_vla.ll b/llvm/test/DebugInfo/ARM/lowerbdgdeclare_vla.ll
index 94b527a445d3a..35b7b044abb55 100644
--- a/llvm/test/DebugInfo/ARM/lowerbdgdeclare_vla.ll
+++ b/llvm/test/DebugInfo/ARM/lowerbdgdeclare_vla.ll
@@ -19,9 +19,9 @@ target triple = "thumbv7-apple-ios8.0.0"
 ; Function Attrs: nounwind optsize readnone
 define void @run(float %r) #0 !dbg !4 {
 entry:
-  tail call void @llvm.dbg.declare(metadata float %r, metadata !11, metadata !DIExpression()), !dbg !22
+  tail call void @llvm.dbg.value(metadata float %r, metadata !11, metadata !DIExpression()), !dbg !22
   %conv = fptosi float %r to i32, !dbg !23
-  tail call void @llvm.dbg.declare(metadata i32 %conv, metadata !12, metadata !DIExpression()), !dbg !23
+  tail call void @llvm.dbg.value(metadata i32 %conv, metadata !12, metadata !DIExpression()), !dbg !23
   %vla = alloca float, i32 %conv, align 4, !dbg !24
   tail call void @llvm.dbg.declare(metadata ptr %vla, metadata !14, metadata !DIExpression(DW_OP_deref)), !dbg !24
 ; The VLA alloca should be described by a dbg.declare:
diff --git a/llvm/test/Transforms/Coroutines/coro-debug.ll b/llvm/test/Transforms/Coroutines/coro-debug.ll
index 3a4eeddbe6198..17a0b80c5b5e5 100644
--- a/llvm/test/Transforms/Coroutines/coro-debug.ll
+++ b/llvm/test/Transforms/Coroutines/coro-debug.ll
@@ -29,8 +29,8 @@ sw.bb:                                            ; preds = %entry
   %direct = load i32, ptr %x.addr, align 4, !dbg !14
   %gep = getelementptr inbounds [16 x i8], ptr undef, i32 %direct, !dbg !14
   call void @llvm.dbg.declare(metadata ptr %gep, metadata !27, metadata !13), !dbg !14
-  call void @llvm.dbg.declare(metadata i32 %conv, metadata !26, metadata !13), !dbg !14
-  call void @llvm.dbg.declare(metadata i32 %direct, metadata !25, metadata !13), !dbg !14
+  call void @llvm.dbg.value(metadata i32 %conv, metadata !26, metadata !13), !dbg !14
+  call void @llvm.dbg.value(metadata i32 %direct, metadata !25, metadata !13), !dbg !14
   call void @llvm.dbg.declare(metadata ptr %x.addr, metadata !12, metadata !13), !dbg !14
   call void @llvm.dbg.declare(metadata ptr %coro_hdl, metadata !15, metadata !13), !dbg !16
   call void @llvm.dbg.declare(metadata ptr %late_local, metadata !29, metadata !13), !dbg !16
@@ -66,7 +66,7 @@ coro_Cleanup:                                     ; preds = %sw.epilog, %sw.bb1
   %5 = load ptr, ptr %coro_hdl, align 8, !dbg !24
   %6 = call ptr @llvm.coro.free(token %0, ptr %5), !dbg !24
   call void @free(ptr %6), !dbg !24
-  call void @llvm.dbg.declare(metadata i32 %asm_res, metadata !32, metadata !13), !dbg !16
+  call void @llvm.dbg.value(metadata i32 %asm_res, metadata !32, metadata !13), !dbg !16
   br label %coro_Suspend, !dbg !24
 
 coro_Suspend:                                     ; preds = %coro_Cleanup, %sw.default
@@ -176,14 +176,14 @@ attributes #7 = { noduplicate }
 ; CHECK: %[[DBG_PTR:.*]] = alloca ptr
 ; CHECK: #dbg_declare(ptr %[[DBG_PTR]], ![[RESUME_COROHDL:[0-9]+]], !DIExpression(DW_OP_deref, DW_OP_plus_uconst,
 ; CHECK: #dbg_declare(ptr %[[DBG_PTR]], ![[RESUME_X:[0-9]+]], !DIExpression(DW_OP_deref, DW_OP_plus_uconst, [[EXPR_TAIL:.*]])
-; CHECK: #dbg_declare(ptr %[[DBG_PTR]], ![[RESUME_DIRECT:[0-9]+]], !DIExpression(DW_OP_deref, DW_OP_plus_uconst, [[EXPR_TAIL]])
 ; CHECK: store ptr {{.*}}, ptr %[[DBG_PTR]]
 ; CHECK-NOT: alloca ptr
-; CHECK: #dbg_declare(i8 0, ![[RESUME_CONST:[0-9]+]], !DIExpression(DW_OP_LLVM_convert, 8, DW_ATE_signed, DW_OP_LLVM_convert, 32, DW_ATE_signed),
+; CHECK: call void @coro.devirt.trigger(ptr null)
+; CHECK: #dbg_value(i8 0, ![[RESUME_CONST:[0-9]+]], !DIExpression(DW_OP_LLVM_convert, 8, DW_ATE_signed, DW_OP_LLVM_convert, 32, DW_ATE_signed),
+; CHECK: #dbg_value(ptr %[[DBG_PTR]], ![[RESUME_DIRECT:[0-9]+]], !DIExpression(DW_OP_deref, DW_OP_plus_uconst, {{[0-9]+}}, DW_OP_deref),
 ; Note that keeping the undef value here could be acceptable, too.
 ; CHECK-NOT: #dbg_declare(ptr undef, !{{[0-9]+}}, !DIExpression(),
-; CHECK: call void @coro.devirt.trigger(ptr null)
-; CHECK: #dbg_value(ptr {{.*}}, ![[RESUME_DIRECT_VALUE:[0-9]+]], !DIExpression(DW_OP_deref, DW_OP_plus_uconst, {{[0-9]+}}, DW_OP_deref),
+; CHECK: #dbg_value(ptr %[[DBG_PTR]], ![[RESUME_DIRECT_VALUE:[0-9]+]], !DIExpression(DW_OP_deref, DW_OP_plus_uconst, {{[0-9]+}}, DW_OP_deref),
 ; Check that the dbg.declare intrinsic of invoke instruction is hanled correctly.
 ; CHECK: %[[ALLOCATED_STORAGE:.+]] = invoke ptr @allocate()
 ; CHECK-NEXT: to label %[[NORMAL_DEST:.+]] unwind
@@ -193,7 +193,7 @@ attributes #7 = { noduplicate }
 ; CHECK-NEXT: to label %[[DEFAULT_DEST:.+]] [label
 ; CHECK: [[DEFAULT_DEST]]:
 ; CHECK-NOT: {{.*}}:
-; CHECK: #dbg_declare(i32 %[[CALLBR_RES]]
+; CHECK: #dbg_value(i32 %[[CALLBR_RES]]
 ; CHECK: define internal fastcc void @f.destroy(ptr noundef nonnull align 8 dereferenceable(40) %0) #0 personality i32 0 !dbg ![[DESTROY:[0-9]+]]
 ; CHECK: define internal fastcc void @f.cleanup(ptr noundef nonnull align 8 dereferenceable(40) %0) #0 personality i32 0 !dbg ![[CLEANUP:[0-9]+]]
 
@@ -202,8 +202,8 @@ attributes #7 = { noduplicate }
 ; CHECK: ![[RESUME]] = distinct !DISubprogram(name: "f", linkageName: "flink"
 ; CHECK: ![[RESUME_COROHDL]] = !DILocalVariable(name: "coro_hdl", scope: ![[RESUME]]
 ; CHECK: ![[RESUME_X]] = !DILocalVariable(name: "x", arg: 1, scope: ![[RESUME]]
-; CHECK: ![[RESUME_DIRECT]] = !DILocalVariable(name: "direct_mem", scope: ![[RESUME]]
 ; CHECK: ![[RESUME_CONST]] = !DILocalVariable(name: "direct_const", scope: ![[RESUME]]
+; CHECK: ![[RESUME_DIRECT]] = !DILocalVariable(name: "direct_mem", scope: ![[RESUME]]
 ; CHECK: ![[RESUME_DIRECT_VALUE]] = !DILocalVariable(name: "direct_value", scope: ![[RESUME]]
 
 ; CHECK: ![[DESTROY]] = distinct !DISubprogram(name: "f", linkageName: "flink"
diff --git a/llvm/test/Transforms/LoopVectorize/discriminator.ll b/llvm/test/Transforms/LoopVectorize/discriminator.ll
index b66a70b9768c4..fe71b6bd9e765 100644
--- a/llvm/test/Transforms/LoopVectorize/discriminator.ll
+++ b/llvm/test/Transforms/LoopVectorize/discriminator.ll
@@ -32,8 +32,8 @@ define void @_Z3foov() local_unnamed_addr #0 !dbg !6 {
   %7 = load i32, ptr %6, align 4, !dbg !17, !tbaa !15
   %8 = add nsw i32 %7, %5, !dbg !17
 ;PSEUDO_PROBE-COUNT-5: call void @llvm.pseudoprobe(i64 6699318081062747564, i64 2, i32 0, i64 -1), !dbg ![[#PROBE:]]
-;DBG_VALUE: #dbg_declare{{.*}} ![[DBG:[0-9]*]]
-  call void @llvm.dbg.declare(metadata i32 %8, metadata !22, metadata !DIExpression()), !dbg !17
+;DBG_VALUE: #dbg_value{{.*}} ![[DBG:[0-9]*]]
+  call void @llvm.dbg.value(metadata i32 %8, metadata !22, metadata !DIExpression()), !dbg !17
   store i32 %8, ptr %6, align 4, !dbg !17, !tbaa !15
   %indvars.iv.next = add nuw nsw i64 %indvars.iv, 1, !dbg !18
   %exitcond = icmp eq i64 %indvars.iv.next, 4096, !dbg !19

OCHyams

SGTM, I can't think of a reason we should allow non-ptrs there. Just an inline question around test changes. @SLTozer do you have any additional comments? (I'll let you hit the approve button)

OCHyams · 2025-04-04T09:16:32Z

llvm/test/CodeGen/X86/selectiondag-order.ll

  %rand = tail call i64 @lrand48() #3
-  tail call void @llvm.dbg.declare(metadata i64 %rand, metadata !6, metadata !7), !dbg !8
+  tail call void @llvm.dbg.declare(metadata ptr %ptr, metadata !6, metadata !7), !dbg !8


I think this slightly changes the tested behaviour, because the new dbg.declare will get put in the MF side-table:

https://godbolt.org/z/bh6oEM55G

The old dbg.declare causes a DBG_INSTR_REF, whereas the new on gets stuffed into stack:. I think this may be the case in a couple of other tests too. If you make %ptr an argument instead of a local alloca it looks like a DBG_VALUE is generated, which is closer to the output of the original test.

Since the test isn't checking for the debug instruction in the output, maybe it doesn't matter, but I'd personally err on the side of minimising the change in output. YMMV.

SLTozer

Principle SGTM, probably calls for a documentation update: this could be extremely minor just to stress the "new" requirement, updating the text for #dbg_declare in SourceLevelDebugging.rst:'

- The first argument is an SSA value corresponding to a variable address, and is
+ The first argument is an SSA `ptr` value corresponding to a variable address, and is

SLTozer · 2025-04-04T08:59:23Z

llvm/lib/IR/Verifier.cpp

+    if (DVR.isDbgDeclare())
+      CheckDI(VAM->getValue()->getType()->isPointerTy(),
+              "location of #dbg_declare must be a pointer", &DVR, MD);
+  } else if (auto *AL = dyn_cast<DIArgList>(MD))


Nit, since the if now uses braces the "else if" should as well.

SLTozer · 2025-04-04T09:32:10Z

llvm/test/CodeGen/AArch64/selectiondag-order.ll

@@ -55,8 +55,9 @@ end:                                        ; preds = %body

 define i64 @simulateWithDbgDeclare(<2 x i32> %a) local_unnamed_addr  {
 entry:
+  %ptr = alloca i32


Slightly awkward case here in that this might degrade the test quality... the intent here seems to be to test that the debug declare does not cause any changes to the code in the CHECK set below, so making it no longer use %rand as an argument might interfere with that.

But otoh the test is non-specific; there's no description of how the debug declare might be expected to affect codegen, so I don't think I have a specific suggested change - you could add a store i64 %rand, ptr %ptr, or expand the check to cover the generated code for %ptr, but I also don't think it's a blocking issue if it stays as-is.

Hm, adding a store like that would affect the existing CHECK lines, so I'm not sure that's better...

Yeah, I'm not convinced there are any solid answers - the test is to check that "nothing bad happens" because of the presence of a dbg.declare, rather than checking for a specific defect, so it's not clear what properties would need to be maintained to keep the test valid. If there aren't any obvious solutions, I'm fine with the latest revision.

When salvaging debug info for #dbg_declare, do not perform salvage that changes the type of the value, i.e. bail out on cast instructions. This ensures that we don't run afoul of the verifier check in llvm#134355.

llvmbot added backend:AArch64 backend:X86 debuginfo coroutines C++20 coroutines llvm:ir llvm:transforms labels Apr 4, 2025

nikic requested a review from SLTozer April 4, 2025 07:53

nikic requested a review from OCHyams April 4, 2025 07:54

OCHyams reviewed Apr 4, 2025

View reviewed changes

SLTozer reviewed Apr 4, 2025

View reviewed changes

Address review feedback

541460c

SLTozer approved these changes Apr 4, 2025

View reviewed changes

nikic merged commit ecd4c08 into llvm:main Apr 4, 2025
12 checks passed

nikic deleted the dbg-declare-ptr branch April 4, 2025 13:34

nikic mentioned this pull request Apr 7, 2025

Assertion `Ty->isPtrOrPtrVectorTy() && "This should only be called with a pointer or pointer vector type"' failed. #134523

Closed

nikic mentioned this pull request Apr 7, 2025

[DebugInfo] Do not change #dbg_declare type during salvage #134601

Closed

stbergmann mentioned this pull request Apr 8, 2025

llvm/lib/IR/DataLayout.cpp:756: unsigned int llvm::DataLayout::getIndexTypeSizeInBits(Type *) const: Assertion `Ty->isPtrOrPtrVectorTy() && "This should only be called with a pointer or pointer vector type"' failed. #134792

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Verifier] Require that dbg.declare variable is a ptr #134355

[Verifier] Require that dbg.declare variable is a ptr #134355

nikic commented Apr 4, 2025

llvmbot commented Apr 4, 2025 •

edited

Loading

OCHyams left a comment

OCHyams Apr 4, 2025 •

edited

Loading

SLTozer left a comment

SLTozer Apr 4, 2025

SLTozer Apr 4, 2025

nikic Apr 4, 2025

SLTozer Apr 4, 2025

[Verifier] Require that dbg.declare variable is a ptr #134355

[Verifier] Require that dbg.declare variable is a ptr #134355

Conversation

nikic commented Apr 4, 2025

llvmbot commented Apr 4, 2025 • edited Loading

OCHyams left a comment

Choose a reason for hiding this comment

OCHyams Apr 4, 2025 • edited Loading

Choose a reason for hiding this comment

SLTozer left a comment

Choose a reason for hiding this comment

SLTozer Apr 4, 2025

Choose a reason for hiding this comment

SLTozer Apr 4, 2025

Choose a reason for hiding this comment

nikic Apr 4, 2025

Choose a reason for hiding this comment

SLTozer Apr 4, 2025

Choose a reason for hiding this comment

llvmbot commented Apr 4, 2025 •

edited

Loading

OCHyams Apr 4, 2025 •

edited

Loading