Globalopt pass produces invalid debug info

[ykhatav]

This patch fixes an issue in the GlobalOpt pass where deleting a global variable fails to update the corresponding dbg.value and it references an empty metadata entry. The SalvageDebugInfo() function has been updated to handle dbg.value intrinsic when globals are deleted.

[llvmbot]

@llvm/pr-subscribers-debuginfo
@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-llvm-transforms

Author: ykhatav (ykhatav)

Changes

This patch fixes an issue in the GlobalOpt pass where deleting a global variable fails to update the corresponding dbg.value and it references an empty metadata entry. The SalvageDebugInfo() function has been updated to handle dbg.value intrinsic when globals are deleted.

---

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

3 Files Affected:

• (modified) llvm/lib/IR/Metadata.cpp (+6)
• (modified) llvm/lib/Transforms/IPO/GlobalOpt.cpp (+1)
• (added) llvm/test/DebugInfo/X86/undef-dbg-val.ll (+35)

diff --git a/llvm/lib/IR/Metadata.cpp b/llvm/lib/IR/Metadata.cpp
index ae5f5de142328..a91b073f17348 100644
--- a/llvm/lib/IR/Metadata.cpp
+++ b/llvm/lib/IR/Metadata.cpp
@@ -346,6 +346,12 @@ void ReplaceableMetadataImpl::SalvageDebugInfo(const Constant &C) {
     MetadataTracking::OwnerTy Owner = Pair.second.first;
     if (!Owner)
       continue;
+    // Check for MetadataAsValue.
+    if (isa<MetadataAsValue *>(Owner)) {
+      cast<MetadataAsValue *>(Owner)->handleChangedMetadata(
+          ValueAsMetadata::get(UndefValue::get(C.getType())));
+      continue;
+    }
     if (!isa<Metadata *>(Owner))
       continue;
     auto *OwnerMD = dyn_cast_if_present<MDNode>(cast<Metadata *>(Owner));
diff --git a/llvm/lib/Transforms/IPO/GlobalOpt.cpp b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
index ab1e41ebf9a9d..5293a777496bc 100644
--- a/llvm/lib/Transforms/IPO/GlobalOpt.cpp
+++ b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
@@ -1338,6 +1338,7 @@ deleteIfDead(GlobalValue &GV,
     if (DeleteFnCallback)
       DeleteFnCallback(*F);
   }
+  ReplaceableMetadataImpl::SalvageDebugInfo(GV);
   GV.eraseFromParent();
   ++NumDeleted;
   return true;
diff --git a/llvm/test/DebugInfo/X86/undef-dbg-val.ll b/llvm/test/DebugInfo/X86/undef-dbg-val.ll
new file mode 100644
index 0000000000000..be386cb14c85f
--- /dev/null
+++ b/llvm/test/DebugInfo/X86/undef-dbg-val.ll
@@ -0,0 +1,35 @@
+; RUN:  opt -S -passes=globalopt <%s | FileCheck %s
+; CHECK: #dbg_value(ptr poison, 
+; CHECK-SAME:    [[VAR:![0-9]+]],
+; CHECK-SAME:    !DIExpression()
+; CHECK: [[VAR]] = !DILocalVariable(name: "_format"
+
+
+; ModuleID = '<stdin>'
+source_filename = "test.cpp"
+
+@_ZZZZ4mainENKUlRN4sycl3_V17handlerEE_clES2_ENKUlvE_clEvE7_format = internal constant [24 x i8] c"Result1: Hello, World!\0A\00", align 16, !dbg !9
+
+; Function Attrs: convergent mustprogress norecurse
+define void  @foo() align 2 !dbg !5 {
+entry:
+  call void @llvm.dbg.value(metadata ptr @_ZZZZ4mainENKUlRN4sycl3_V17handlerEE_clES2_ENKUlvE_clEvE7_format, metadata !11, metadata !DIExpression()), !dbg !12
+  ret void
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang", isOptimized: true, emissionKind: FullDebug)
+!1 = !DIFile(filename: "test.cpp", directory: "/path/to")
+!2 = !{}
+!3 = !{i32 7, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 27, type: !6, scopeLine: 27, spFlags: DISPFlagDefinition, unit: !0)
+!6 = !DISubroutineType(types: !7)
+!7 = !{null}
+!8 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!9 = !DIGlobalVariableExpression(var: !10, expr: !DIExpression())
+!10 = distinct !DIGlobalVariable(name: "_format", scope: !5, file: !1, line: 49, type: !8, isLocal: true, isDefinition: true)
+!11 = !DILocalVariable(name: "_format", arg: 1, scope: !5, file: !1, line: 79, type: !8)
+!12 = !DILocation(line: 0, scope: !5)

[jmorse]

Hi -- using a recent opt (built from 1ebfc81) the test case already produces a #dbg_value with "poison" operand, which suggests this patch isn't having any effect. Perhaps it was recently fixed somewhere else?

[ykhatav]

Hi -- using a recent opt (built from 1ebfc81) the test case already produces a #dbg_value with "poison" operand, which suggests this patch isn't having any effect. Perhaps it was recently fixed somewhere else?

Sorry for the confusion. The problem only reproduces with the --experimental-debuginfo-iterators=false switch. Looks like the new debug format does not cause this issue. I have updated the test to use this switch. I think we should still keep the code change for backward compatibility.

[jmorse]

Interesting; we were aiming for identical behaviour between the two formats to avoid any uncertainty about it's correctness. I believe we're not aiming to keep support for intrinsic-format in the future, but if there's changing behaviour then this area wants testing and more examination (will examine it more on Monday).

[SLTozer]

Is this line necessary? I believe GV.removeDeadConstantUsers(); should already call this if GV is dead, as part of constantIsDead(..., /*RemoveDeadUsers=*/true).

[ykhatav]

The loop responsible for removing dead users and potentially calling constantIsDead() does not execute in this case. It looks like both the iterators I and E are null indicating it does not count debug intrinsic as users.

[SLTozer]

It might be more appropriate to put this into removeDeadConstantUsers then, after the loop - I think we'd want to perform the salvage step for any constant that dies with debug users regardless of the pass that does it. However, that might end up affecting other tests; if so it can easily be left for another patch.

[ykhatav]

Yes, I can investigate more and follow up on a different patch if it's a viable approach.

[ykhatav]

friendly ping!

[SLTozer]

Apologies for the delay, been on holiday for a bit. Patch LGTM, with some minor nits and a non-blocking comment/suggestion.

[SLTozer]

It might be more appropriate to put this into removeDeadConstantUsers then, after the loop - I think we'd want to perform the salvage step for any constant that dies with debug users regardless of the pass that does it. However, that might end up affecting other tests; if so it can easily be left for another patch.