[PM] Introduce a PoisoningVH as a (more expensive) alternative to

AssertingVH that delays any reported error until the handle is *used*.

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.

Differential Revision: https://reviews.llvm.org/D29061

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@292925 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/llvm/IR/ValueHandle.h b/include/llvm/IR/ValueHandle.h
index a4d4893a9bc9242d51827f93f32a6750e34169d0..4838bac9e0f75c56985c5a7fc296ce8968529794 100644 (file)
--- a/include/llvm/IR/ValueHandle.h
+++ b/include/llvm/IR/ValueHandle.h
@@ -98,6 +98,15 @@ protected:
            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);
@@ -120,8 +129,6 @@ private:
 
   /// \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.
@@ -259,7 +266,6 @@ struct isPodLike<AssertingVH<T> > {
 #endif
 };
 
-
 /// \brief Value handle that tracks a Value across RAUW.
 ///
 /// TrackingVH is designed for situations where a client needs to hold a handle
@@ -370,6 +376,130 @@ public:
   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

diff --git a/unittests/IR/ValueHandleTest.cpp b/unittests/IR/ValueHandleTest.cpp
index 59cd9d7ba37aee19213b1ec07a4b7de418f1b6c8..1abc87c2fdc7f02bdb4c7ba78fbd474005230f2b 100644 (file)
--- a/unittests/IR/ValueHandleTest.cpp
+++ b/unittests/IR/ValueHandleTest.cpp
@@ -412,4 +412,97 @@ TEST_F(ValueHandle, AssertingVHCheckedLast) {
   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
 }