[IR] Fix PoisoningVH relocation of a poisoned handle

[MaskRay]

In LLVM_ENABLE_ABI_BREAKING_CHECKS builds, when poisoned (deleted() or
allUsesReplacedWith()), a PoisoningVH is removed from its use list but
keeps its raw value pointer for identity, so its PrevPtr/Next are left
stale.

PoisoningVH has no move constructor, so relocating a value that embeds one
falls back to the copy constructor, where setRawValPtr relinks with
the stale pointers and corrupts the use list.

This is a latent bug for any relocation of a PoisoningVH handle
but becomes load-bearing for #199615 , which relocating erase exercises
it via ScalarEvolution's BackedgeTakenInfo (its ExitNotTakenInfo holds a
PoisoningVH).

Fix by special casing the Poisoned case.
Aided By Claude Opus 4.8

[llvmorg-github-actions]

@llvm/pr-subscribers-llvm-ir

Author: Fangrui Song (MaskRay)

Changes

When poisoned (deleted() or allUsesReplacedWith()), a PoisoningVH is
removed from its use list but keeps its raw value pointer for identity,
so its PrevPtr/Next are left stale.

PoisoningVH has no move constructor, so relocating a value that embeds one
falls back to the copy constructor, where setRawValPtr relinks with
the stale pointers and corrupts the use list.

This is a latent bug for any relocation of a PoisoningVH handle
but becomes load-bearing for #199615 , which relocating erase exercises
it via ScalarEvolution's BackedgeTakenInfo (its ExitNotTakenInfo holds a
PoisoningVH<BasicBlock>).

Fix by special casing the Poisoned case.
Aided By Claude Opus 4.8

---

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

2 Files Affected:

• (modified) llvm/include/llvm/IR/ValueHandle.h (+19-5)
• (modified) llvm/unittests/IR/ValueHandleTest.cpp (+36)

diff --git a/llvm/include/llvm/IR/ValueHandle.h b/llvm/include/llvm/IR/ValueHandle.h
index c6e98d80d54cc..818385052ceb1 100644
--- a/llvm/include/llvm/IR/ValueHandle.h
+++ b/llvm/include/llvm/IR/ValueHandle.h
@@ -55,13 +55,13 @@ class ValueHandleBase {
     }
   }
 
+  void setValPtr(Value *V) { Val = V; }
+
 private:
   PointerIntPair<ValueHandleBase**, 2, HandleBaseKind> PrevPair;
   ValueHandleBase *Next = nullptr;
   Value *Val = nullptr;
 
-  void setValPtr(Value *V) { Val = V; }
-
 public:
   explicit ValueHandleBase(HandleBaseKind Kind)
       : PrevPair(nullptr, Kind) {}
@@ -540,8 +540,15 @@ class PoisoningVH final
   PoisoningVH() = default;
 #if LLVM_ENABLE_ABI_BREAKING_CHECKS
   PoisoningVH(ValueTy *P) : CallbackVH(GetAsValue(P)) {}
-  PoisoningVH(const PoisoningVH &RHS)
-      : CallbackVH(RHS), Poisoned(RHS.Poisoned) {}
+  // A poisoned handle is detached from its use list but keeps its raw value
+  // pointer, so its use-list pointers are stale; a copy must not relink through
+  // them.
+  PoisoningVH(const PoisoningVH &RHS) : CallbackVH(), Poisoned(RHS.Poisoned) {
+    if (Poisoned)
+      ValueHandleBase::setValPtr(RHS.getRawValPtr());
+    else
+      setRawValPtr(RHS.getRawValPtr());
+  }
 
   ~PoisoningVH() {
     if (Poisoned)
@@ -551,7 +558,14 @@ class PoisoningVH final
   PoisoningVH &operator=(const PoisoningVH &RHS) {
     if (Poisoned)
       clearValPtr();
-    CallbackVH::operator=(RHS);
+    if (RHS.Poisoned) {
+      // Detach *this and copy only the raw pointer; see the copy constructor.
+      if (isValid(getRawValPtr()))
+        RemoveFromUseList();
+      ValueHandleBase::setValPtr(RHS.getRawValPtr());
+    } else {
+      CallbackVH::operator=(RHS);
+    }
     Poisoned = RHS.Poisoned;
     return *this;
   }
diff --git a/llvm/unittests/IR/ValueHandleTest.cpp b/llvm/unittests/IR/ValueHandleTest.cpp
index 571991da6122e..27040fc473eb8 100644
--- a/llvm/unittests/IR/ValueHandleTest.cpp
+++ b/llvm/unittests/IR/ValueHandleTest.cpp
@@ -7,6 +7,9 @@
 //===----------------------------------------------------------------------===//
 
 #include "llvm/IR/ValueHandle.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/IR/BasicBlock.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/LLVMContext.h"
@@ -532,6 +535,39 @@ TEST_F(ValueHandle, PoisoningVH_DenseMap) {
   EXPECT_TRUE(Map.find_as(ConstantV) != Map.end());
 }
 
+#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+// A poisoned PoisoningVH is detached from its use list but keeps its raw
+// pointer, so its use-list pointers are stale; relocating it must not relink
+// through them. Reproduce the way ScalarEvolution does: a callback mutates a
+// map embedding such handles while RAUW is still walking the use list. Here the
+// callback grows the map, relocating the just-poisoned handles mid-walk.
+TEST_F(ValueHandle, PoisoningVH_RelocatePoisoned) {
+  using MapTy = DenseMap<int, SmallVector<PoisoningVH<Value>, 1>>;
+
+  struct GrowCB final : CallbackVH {
+    MapTy *Map;
+    GrowCB(Value *V, MapTy *Map) : CallbackVH(V), Map(Map) {}
+    void allUsesReplacedWith(Value *) override {
+      for (unsigned I = 0; I != 256; ++I)
+        (void)(*Map)[1000 + I];
+    }
+  };
+
+  BasicBlock *BB = BasicBlock::Create(Context);
+  BasicBlock *BB2 = BasicBlock::Create(Context);
+  MapTy Map;
+  // Registered first => processed last, after the handles below are poisoned.
+  GrowCB CB(BB, &Map);
+  for (unsigned I = 0; I != 8; ++I)
+    Map[I].emplace_back(BB);
+  BB->replaceAllUsesWith(BB2);
+  EXPECT_EQ(Map.size(), 8u + 256u);
+
+  BB->deleteValue();
+  BB2->deleteValue();
+}
+#endif
+
 #ifdef NDEBUG
 
 TEST_F(ValueHandle, PoisoningVH_ReducesToPointer) {

[aengelke]

LG. It is rather ugly, but PoisoningVH's use of invalid pointers is also ugly, creating this problem in the first place... (the deeper one looks into ValueHandle, the worse it gets..)

(Worth noting, there are just three uses of PoisoningVH in our code base: LazyValueInfo, SCEV, SCEVExpander. The first could, I think, be replaced with block numbers. Might be worth evaluating the removal of PoisongVH.)