[RemoveDIs][DebugInfo] Handle RAUW from dead constants

[SLTozer]

Currently when a constant value dies, all ValueAsMetadata uses of that constant will be replaced with nullptr. For debug info intrinsics, this RAUW operation ends up being passed to MetadataAsValue, which canonicalizes the nullptr argument, replacing it with an empty MDNode, !{} (this is one of the only ways that an MDNode can be the first argument of a debug intrinsic). Since DPValues don't use MetadataAsValue, this is skipped and we end up with just a nullptr; currently this doesn't cause any problems because for most operations we use a wrapper that translates nullness, but will cause problems when we start printing DPValues, and should be considered an invalid state.

There are no tests for this change because currently any nullptr argument in a DPValue will be replaced with !{} when we convert it back to a debug intrinsic before printing, meaning this change is "NFC" for now. Instead, this patch adds an assert before converting back to debug intrinsics, which will cause the following tests to fail without the functional change:

llvm/test/Transforms/Inline/delete-function-with-metadata-use.ll
llvm/test/DebugInfo/X86/array2.ll

Hopefully this also paves the way to eliminating instances of empty MDNodes appearing in DPValues at any time as well, but there may be other cases that introduce them as well (@OCHyams may be familiar?).

[llvmbot]

@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-debuginfo

Author: Stephen Tozer (SLTozer)

Changes

Currently when a constant value dies, all ValueAsMetadata uses of that constant will be replaced with nullptr. For debug info intrinsics, this RAUW operation ends up being passed to MetadataAsValue, which canonicalizes the nullptr argument, replacing it with an empty MDNode, !{} (this is one of the only ways that an MDNode can be the first argument of a debug intrinsic). Since DPValues don't use MetadataAsValue, this is skipped and we end up with just a nullptr; currently this doesn't cause any problems because for most operations we use a wrapper that translates nullness, but will cause problems when we start printing DPValues, and should be considered an invalid state.

There are no tests for this change because currently any nullptr argument in a DPValue will be replaced with !{} when we convert it back to a debug intrinsic before printing, meaning this change is "NFC" for now. Instead, this patch adds an assert before converting back to debug intrinsics, which will cause the following tests to fail without the functional change:

llvm/test/Transforms/Inline/delete-function-with-metadata-use.ll
llvm/test/DebugInfo/X86/array2.ll

Hopefully this also paves the way to eliminating instances of empty MDNodes appearing in DPValues at any time as well, but there may be other cases that introduce them as well (@OCHyams may be familiar?).

---

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

2 Files Affected:

• (modified) llvm/lib/IR/DebugProgramInstruction.cpp (+1)
• (modified) llvm/lib/IR/Metadata.cpp (+6)

diff --git a/llvm/lib/IR/DebugProgramInstruction.cpp b/llvm/lib/IR/DebugProgramInstruction.cpp
index 03085b3bfd1279..5b9382fec88fdd 100644
--- a/llvm/lib/IR/DebugProgramInstruction.cpp
+++ b/llvm/lib/IR/DebugProgramInstruction.cpp
@@ -286,6 +286,7 @@ DPValue::createDebugIntrinsic(Module *M, Instruction *InsertBefore) const {
   // Create the intrinsic from this DPValue's information, optionally insert
   // into the target location.
   DbgVariableIntrinsic *DVI;
+  assert(getRawLocation() && "DPValue's RawLocation should be non-null.");
   if (isDbgAssign()) {
     Value *AssignArgs[] = {
         MetadataAsValue::get(Context, getRawLocation()),
diff --git a/llvm/lib/IR/Metadata.cpp b/llvm/lib/IR/Metadata.cpp
index 37017a222d4853..06db91fe4f8e74 100644
--- a/llvm/lib/IR/Metadata.cpp
+++ b/llvm/lib/IR/Metadata.cpp
@@ -158,6 +158,12 @@ void DebugValueUser::handleChangedValue(void *Old, Metadata *New) {
   // getOwner, if needed.
   auto OldMD = static_cast<Metadata **>(Old);
   ptrdiff_t Idx = std::distance(&*DebugValues.begin(), OldMD);
+  // If replacing a ValueAsMetadata with a nullptr, replace it with a
+  // PoisonValue instead.
+  if (OldMD && isa<ValueAsMetadata>(*OldMD) && !New) {
+    auto *OldVAM = cast<ValueAsMetadata>(*OldMD);
+    New = ValueAsMetadata::get(PoisonValue::get(OldVAM->getValue()->getType()));
+  }
   resetDebugValue(Idx, New);
 }
 

[jmorse]

LGTM -- there are a few nullptrs leaking in from abnormal freeing-of-values paths, it'll be good to have this catch-all.

[OCHyams]

I think my summary here https://discourse.llvm.org/t/auto-undef-debug-uses-of-a-deleted-value/65019/5 is still relevant -- the natural path in llvm is to introduce nullptrs in some places (which become !{}), and undef/poison in others. Both should be handled as "kill locations". Since we deleted the troublesome bit of code that was treating nullptr-ed dbg intrinsics as "safe to clean up" that is true; that is to say they're semantically equivalent today.

Hopefully this also paves the way to eliminating instances of empty MDNodes appearing in DPValues at any time as well, but there may be other cases that introduce them as well (@OCHyams may be familiar?).

Since we now have the ability to catch these nullptrs in the debug record itself rather than in the more general MetadataAsValue wrapper used in dbg intrinsics, it makes sense to canonicalize things.

We could remove the "empty metadata tuple" bit in the docs https://llvm.org/docs/SourceLevelDebugging.html once we eliminate debug intrinscs entirely.

LGTM -- there are a few nullptrs leaking in from abnormal freeing-of-values paths, it'll be good to have this catch-all.

^ I agree.