[PM] Introduce a PoisoningVH as a (more expensive) alternative to AssertingVH that delays any reported error until the handle is *used*.

[chandlerc]

Summary

This allows data structures to contain handles which become dangling
provided the data structure is cleaned up afterward rather than used for
anything interesting.

The implementation is moderately horrible in part because it works to
leave AssertingVH in place, undisturbed. If at some point there is
consensus that this is simply how AssertingVH should be used, it can be
substantially simplified.

This remains a boring pointer in a non-asserts build as you would
expect. The only place we pay cost is in asserts builds.

I plan to use this as a basis for replacing the asserting VHs that
currently dangle in the new PM until invalidation occurs in both LVI and
SCEV.

[chandlerc]

Fix a minor compile issue.

[sanjoy]

include/llvm/IR/ValueHandle.h

410 ↗

(On Diff #85501)

I know you'll hate me for this, but this needs to be conditional on LLVM_ENABLE_ABI_BREAKING_CHECKS also.

383 ↗

(On Diff #85492)

Reflow comment here.

[chandlerc]

include/llvm/IR/ValueHandle.h

410 ↗

(On Diff #85501)

No more or less than AssertingVH?

I assumed that the design idea of AssertingVH was that *users* must leverage LLVM_ENABLE_ABI_BREAKING_CHECKS if they expose the AssertingVH. Maybe that's wrong or maybe that's a bad model, but I'd rather the two handles follow the same model (and be fixed or changed at the same time).

Similarly, if today's users of AssertingVH are failing to use this macro, they were before my changes, and I'd rather not have to boil that ocean.

[chandlerc]

Reflow comment Sanjoy spotted.

[sanjoy]

lgtm

include/llvm/IR/ValueHandle.h

410 ↗

(On Diff #85501)

SGTM!

As I said on IRC, I was under the impression that AssertingVH was using LLVM_ENABLE_ABI_BREAKING_CHECKS. Given that it does not, this looks fine.

llvm/trunk/include/llvm/IR/ValueHandle.h

protected:
   Value *getValPtr() const { return V; }
 
   static bool isValid(Value *V) {
     return V &&
            V != DenseMapInfo<Value *>::getEmptyKey() &&
            V != DenseMapInfo<Value *>::getTombstoneKey();
   }
 
+  /// \brief Remove this ValueHandle from its current use list.
+  void RemoveFromUseList();
+
+  /// \brief Clear the underlying pointer without clearing the use list.
+  ///
+  /// This should only be used if a derived class has manually removed the
+  /// handle from the use list.
+  void clearValPtr() { V = nullptr; }
+
 public:
   // Callbacks made from Value.
   static void ValueIsDeleted(Value *V);
   static void ValueIsRAUWd(Value *Old, Value *New);
 
 private:
   // Internal implementation details.
   ValueHandleBase **getPrevPtr() const { return PrevPair.getPointer(); }
   HandleBaseKind getKind() const { return PrevPair.getInt(); }
   void setPrevPtr(ValueHandleBase **Ptr) { PrevPair.setPointer(Ptr); }
 
   /// \brief Add this ValueHandle to the use list for V.
   ///
   /// List is the address of either the head of the list or a Next node within
   /// the existing use list.
   void AddToExistingUseList(ValueHandleBase **List);
 
   /// \brief Add this ValueHandle to the use list after Node.
   void AddToExistingUseListAfter(ValueHandleBase *Node);
 
   /// \brief Add this ValueHandle to the use list for V.
   void AddToUseList();
-  /// \brief Remove this ValueHandle from its current use list.
-  void RemoveFromUseList();
 };
 
 /// \brief Value handle that is nullable, but tries to track the Value.
 ///
 /// This is a value handle that tries hard to point to a Value, even across
 /// RAUW operations, but will null itself out if the value is destroyed.  this
 /// is useful for advisory sorts of information, but should not be used as the
 /// key of a map (since the map would have to rearrange itself when the pointer

 struct isPodLike<AssertingVH<T> > {
 #ifdef NDEBUG
   static const bool value = true;
 #else
   static const bool value = false;
 #endif
 };
 
-
 /// \brief Value handle that tracks a Value across RAUW.
 ///
 /// TrackingVH is designed for situations where a client needs to hold a handle
 /// to a Value (or subclass) across some operations which may move that value,
 /// but should never destroy it or replace it with some unacceptable type.
 ///
 /// It is an error to do anything with a TrackingVH whose value has been
 /// destroyed, except to destruct it.
public:
   ///
   /// Called when this->getValPtr()->replaceAllUsesWith(new_value) is called,
   /// _before_ any of the uses have actually been replaced.  If WeakVH were
   /// implemented as a CallbackVH, it would use this method to call
   /// setValPtr(new_value).  AssertingVH would do nothing in this method.
   virtual void allUsesReplacedWith(Value *) {}
 };
 
+/// Value handle that poisons itself if the Value is deleted.
+///
+/// This is a Value Handle that points to a value and poisons itself if the
+/// value is destroyed while the handle is still live.  This is very useful for
+/// catching dangling pointer bugs where an \c AssertingVH cannot be used
+/// because the dangling handle needs to outlive the value without ever being
+/// used.
+///
+/// One particularly useful place to use this is as the Key of a map. Dangling
+/// pointer bugs often lead to really subtle bugs that only occur if another
+/// object happens to get allocated to the same address as the old one. Using
+/// a PoisoningVH ensures that an assert is triggered if looking up a new value
+/// in the map finds a handle from the old value.
+///
+/// Note that a PoisoningVH handle does *not* follow values across RAUW
+/// operations. This means that RAUW's need to explicitly update the
+/// PoisoningVH's as it moves. This is required because in non-assert mode this
+/// class turns into a trivial wrapper around a pointer.
+template <typename ValueTy>
+class PoisoningVH
+#ifndef NDEBUG
+    final : public CallbackVH
+#endif
+{
+  friend struct DenseMapInfo<PoisoningVH<ValueTy>>;
+
+  // Convert a ValueTy*, which may be const, to the raw Value*.
+  static Value *GetAsValue(Value *V) { return V; }
+  static Value *GetAsValue(const Value *V) { return const_cast<Value *>(V); }
+
+#ifndef NDEBUG
+  /// A flag tracking whether this value has been poisoned.
+  ///
+  /// On delete and RAUW, we leave the value pointer alone so that as a raw
+  /// pointer it produces the same value (and we fit into the same key of
+  /// a hash table, etc), but we poison the handle so that any top-level usage
+  /// will fail.
+  bool Poisoned = false;
+
+  Value *getRawValPtr() const { return ValueHandleBase::getValPtr(); }
+  void setRawValPtr(Value *P) { ValueHandleBase::operator=(P); }
+
+  /// Handle deletion by poisoning the handle.
+  void deleted() override {
+    assert(!Poisoned && "Tried to delete an already poisoned handle!");
+    Poisoned = true;
+    RemoveFromUseList();
+  }
+
+  /// Handle RAUW by poisoning the handle.
+  void allUsesReplacedWith(Value *) override {
+    assert(!Poisoned && "Tried to RAUW an already poisoned handle!");
+    Poisoned = true;
+    RemoveFromUseList();
+  }
+#else // NDEBUG
+  Value *ThePtr = nullptr;
+
+  Value *getRawValPtr() const { return ThePtr; }
+  void setRawValPtr(Value *P) { ThePtr = P; }
+#endif
+
+  ValueTy *getValPtr() const {
+    assert(!Poisoned && "Accessed a poisoned value handle!");
+    return static_cast<ValueTy *>(getRawValPtr());
+  }
+  void setValPtr(ValueTy *P) { setRawValPtr(GetAsValue(P)); }
+
+public:
+  PoisoningVH() = default;
+#ifndef NDEBUG
+  PoisoningVH(ValueTy *P) : CallbackVH(GetAsValue(P)) {}
+  PoisoningVH(const PoisoningVH &RHS)
+      : CallbackVH(RHS), Poisoned(RHS.Poisoned) {}
+  ~PoisoningVH() {
+    if (Poisoned)
+      clearValPtr();
+  }
+  PoisoningVH &operator=(const PoisoningVH &RHS) {
+    if (Poisoned)
+      clearValPtr();
+    CallbackVH::operator=(RHS);
+    Poisoned = RHS.Poisoned;
+    return *this;
+  }
+#else
+  PoisoningVH(ValueTy *P) : ThePtr(GetAsValue(P)) {}
+#endif
+
+  operator ValueTy *() const { return getValPtr(); }
+
+  ValueTy *operator->() const { return getValPtr(); }
+  ValueTy &operator*() const { return *getValPtr(); }
+};
+
+// Specialize DenseMapInfo to allow PoisoningVH to participate in DenseMap.
+template <typename T> struct DenseMapInfo<PoisoningVH<T>> {
+  static inline PoisoningVH<T> getEmptyKey() {
+    PoisoningVH<T> Res;
+    Res.setRawValPtr(DenseMapInfo<Value *>::getEmptyKey());
+    return Res;
+  }
+  static inline PoisoningVH<T> getTombstoneKey() {
+    PoisoningVH<T> Res;
+    Res.setRawValPtr(DenseMapInfo<Value *>::getTombstoneKey());
+    return Res;
+  }
+  static unsigned getHashValue(const PoisoningVH<T> &Val) {
+    return DenseMapInfo<Value *>::getHashValue(Val.getRawValPtr());
+  }
+  static bool isEqual(const PoisoningVH<T> &LHS, const PoisoningVH<T> &RHS) {
+    return DenseMapInfo<Value *>::isEqual(LHS.getRawValPtr(),
+                                          RHS.getRawValPtr());
+  }
+};
+
+template <typename T> struct isPodLike<PoisoningVH<T>> {
+#ifdef NDEBUG
+  static const bool value = true;
+#else
+  static const bool value = false;
+#endif
+};
+
 } // End llvm namespace
 
 #endif

llvm/trunk/unittests/IR/ValueHandleTest.cpp

TEST_F(ValueHandle, AssertingVHCheckedLast) {
   A1 = BitcastV.get();
   ClearingVH C(BitcastV.get(), A1, A2);
   A2 = BitcastV.get();
   // C.deleted() should run first, clearing the two AssertingVHs,
   // which should prevent them from asserting.
   BitcastV.reset();
 }
 
+TEST_F(ValueHandle, PoisoningVH_BasicOperation) {
+  PoisoningVH<CastInst> VH(BitcastV.get());
+  CastInst *implicit_to_exact_type = VH;
+  (void)implicit_to_exact_type; // Avoid warning.
+
+  PoisoningVH<Value> GenericVH(BitcastV.get());
+  EXPECT_EQ(BitcastV.get(), GenericVH);
+  GenericVH = ConstantV;
+  EXPECT_EQ(ConstantV, GenericVH);
+
+  // Make sure I can call a method on the underlying CastInst.  It
+  // doesn't matter which method.
+  EXPECT_FALSE(VH->mayWriteToMemory());
+  EXPECT_FALSE((*VH).mayWriteToMemory());
+}
+
+TEST_F(ValueHandle, PoisoningVH_Const) {
+  const CastInst *ConstBitcast = BitcastV.get();
+  PoisoningVH<const CastInst> VH(ConstBitcast);
+  const CastInst *implicit_to_exact_type = VH;
+  (void)implicit_to_exact_type; // Avoid warning.
+}
+
+TEST_F(ValueHandle, PoisoningVH_Comparisons) {
+  PoisoningVH<Value> BitcastVH(BitcastV.get());
+  PoisoningVH<Value> ConstantVH(ConstantV);
+
+  EXPECT_TRUE(BitcastVH == BitcastVH);
+  EXPECT_TRUE(BitcastV.get() == BitcastVH);
+  EXPECT_TRUE(BitcastVH == BitcastV.get());
+  EXPECT_FALSE(BitcastVH == ConstantVH);
+
+  EXPECT_TRUE(BitcastVH != ConstantVH);
+  EXPECT_TRUE(BitcastV.get() != ConstantVH);
+  EXPECT_TRUE(BitcastVH != ConstantV);
+  EXPECT_FALSE(BitcastVH != BitcastVH);
+
+  // Cast to Value* so comparisons work.
+  Value *BV = BitcastV.get();
+  Value *CV = ConstantV;
+  EXPECT_EQ(BV < CV, BitcastVH < ConstantVH);
+  EXPECT_EQ(BV <= CV, BitcastVH <= ConstantVH);
+  EXPECT_EQ(BV > CV, BitcastVH > ConstantVH);
+  EXPECT_EQ(BV >= CV, BitcastVH >= ConstantVH);
+
+  EXPECT_EQ(BV < CV, BitcastV.get() < ConstantVH);
+  EXPECT_EQ(BV <= CV, BitcastV.get() <= ConstantVH);
+  EXPECT_EQ(BV > CV, BitcastV.get() > ConstantVH);
+  EXPECT_EQ(BV >= CV, BitcastV.get() >= ConstantVH);
+
+  EXPECT_EQ(BV < CV, BitcastVH < ConstantV);
+  EXPECT_EQ(BV <= CV, BitcastVH <= ConstantV);
+  EXPECT_EQ(BV > CV, BitcastVH > ConstantV);
+  EXPECT_EQ(BV >= CV, BitcastVH >= ConstantV);
+}
+
+TEST_F(ValueHandle, PoisoningVH_DoesNotFollowRAUW) {
+  PoisoningVH<Value> VH(BitcastV.get());
+  BitcastV->replaceAllUsesWith(ConstantV);
+  EXPECT_TRUE(DenseMapInfo<PoisoningVH<Value>>::isEqual(VH, BitcastV.get()));
+}
+
+#ifdef NDEBUG
+
+TEST_F(ValueHandle, PoisoningVH_ReducesToPointer) {
+  EXPECT_EQ(sizeof(CastInst *), sizeof(PoisoningVH<CastInst>));
+}
+
+#else // !NDEBUG
+
+#ifdef GTEST_HAS_DEATH_TEST
+
+TEST_F(ValueHandle, PoisoningVH_Asserts) {
+  PoisoningVH<Value> VH(BitcastV.get());
+
+  // The poisoned handle shouldn't assert when the value is deleted.
+  BitcastV.reset(new BitCastInst(ConstantV, Type::getInt32Ty(Context)));
+  // But should when we access the handle.
+  EXPECT_DEATH((void)*VH, "Accessed a poisoned value handle!");
+
+  // Now check that poison catches RAUW.
+  VH = BitcastV.get();
+  // The replace doesn't trigger anything immediately.
+  BitcastV->replaceAllUsesWith(ConstantV);
+  // But a use does.
+  EXPECT_DEATH((void)*VH, "Accessed a poisoned value handle!");
+
+  // Don't clear anything out here as destroying the handles should be fine.
+}
+
+#endif // GTEST_HAS_DEATH_TEST
+
+#endif // NDEBUG
 }