[llvm] r292925 - [PM] Introduce a PoisoningVH as a (more expensive) alternative to
Philip Reames via llvm-commits
llvm-commits at lists.llvm.org
Tue Jan 24 09:45:42 PST 2017
To me, assert on use seems like the obviously right semantics for a
ValueHandle. I'd be really tempted to make this the default behavior of
AssertingVH. Thanks for working on this!
Philip
On 01/24/2017 04:34 AM, Chandler Carruth via llvm-commits wrote:
> Author: chandlerc
> Date: Tue Jan 24 06:34:47 2017
> New Revision: 292925
>
> URL: http://llvm.org/viewvc/llvm-project?rev=292925&view=rev
> Log:
> [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
>
> Modified:
> llvm/trunk/include/llvm/IR/ValueHandle.h
> llvm/trunk/unittests/IR/ValueHandleTest.cpp
>
> Modified: llvm/trunk/include/llvm/IR/ValueHandle.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/ValueHandle.h?rev=292925&r1=292924&r2=292925&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/IR/ValueHandle.h (original)
> +++ llvm/trunk/include/llvm/IR/ValueHandle.h Tue Jan 24 06:34:47 2017
> @@ -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
>
> Modified: llvm/trunk/unittests/IR/ValueHandleTest.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/IR/ValueHandleTest.cpp?rev=292925&r1=292924&r2=292925&view=diff
> ==============================================================================
> --- llvm/trunk/unittests/IR/ValueHandleTest.cpp (original)
> +++ llvm/trunk/unittests/IR/ValueHandleTest.cpp Tue Jan 24 06:34:47 2017
> @@ -412,4 +412,97 @@ TEST_F(ValueHandle, AssertingVHCheckedLa
> 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
> }
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list