Index: include/llvm/IR/ValueHandle.h =================================================================== --- include/llvm/IR/ValueHandle.h +++ include/llvm/IR/ValueHandle.h @@ -37,7 +37,6 @@ enum HandleBaseKind { Assert, Callback, - Tracking, Weak }; @@ -165,6 +164,10 @@ operator Value*() const { return getValPtr(); } + + bool pointsToAliveValue() const { + return ValueHandleBase::isValid(getValPtr()); + } }; // Specialize simplify_type to allow WeakVH to participate in @@ -280,39 +283,37 @@ /// 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. -/// /// It is an error to attempt to replace a value with one of a type which is /// incompatible with any of its outstanding TrackingVHs. -template -class TrackingVH : public ValueHandleBase { - void CheckValidity() const { - Value *VP = ValueHandleBase::getValPtr(); - - // Null is always ok. - if (!VP) return; +/// +/// It is an error to read from a TrackingVH that does not point to a valid +/// value. A TrackingVH is said to not point to a valid value if either it +/// hasn't yet been assigned a value yet or because the value it was tracking +/// has since been deleted. +/// +/// Assigning a value to a TrackingVH is always allowed, even if said TrackingVH +/// no longer points to a valid value. +template class TrackingVH { + WeakVH InnerHandle; - // Check that this value is valid (i.e., it hasn't been deleted). We - // explicitly delay this check until access to avoid requiring clients to be - // unnecessarily careful w.r.t. destruction. - assert(ValueHandleBase::isValid(VP) && "Tracked Value was deleted!"); +public: + ValueTy *getValPtr() const { + assert(InnerHandle.pointsToAliveValue() && + "TrackingVH must be non-null and valid on dereference!"); // Check that the value is a member of the correct subclass. We would like // to check this property on assignment for better debugging, but we don't // want to require a virtual interface on this VH. Instead we allow RAUW to // replace this value with a value of an invalid type, and check it here. - assert(isa(VP) && + assert(isa(InnerHandle) && "Tracked Value was replaced by one with an invalid type!"); + return cast(InnerHandle); } - ValueTy *getValPtr() const { - CheckValidity(); - return (ValueTy*)ValueHandleBase::getValPtr(); - } void setValPtr(ValueTy *P) { - CheckValidity(); - ValueHandleBase::operator=(GetAsValue(P)); + // Assigning to non-valid TrackingVH 's are fine so we just unconditionally + // assign here. + InnerHandle = GetAsValue(P); } // Convert a ValueTy*, which may be const, to the type the base @@ -321,8 +322,8 @@ static Value *GetAsValue(const Value *V) { return const_cast(V); } public: - TrackingVH() : ValueHandleBase(Tracking) {} - TrackingVH(ValueTy *P) : ValueHandleBase(Tracking, GetAsValue(P)) {} + TrackingVH() {} + TrackingVH(ValueTy *P) { setValPtr(P); } operator ValueTy*() const { return getValPtr(); Index: lib/IR/Value.cpp =================================================================== --- lib/IR/Value.cpp +++ lib/IR/Value.cpp @@ -820,11 +820,6 @@ switch (Entry->getKind()) { case Assert: break; - case Tracking: - // Mark that this value has been deleted by setting it to an invalid Value - // pointer. - Entry->operator=(DenseMapInfo::getTombstoneKey()); - break; case Weak: // Weak just goes to null, which will unlink it from the list. Entry->operator=(nullptr); @@ -876,13 +871,6 @@ case Assert: // Asserting handle does not follow RAUW implicitly. break; - case Tracking: - // Tracking goes to new value like a WeakVH. Note that this may make it - // something incompatible with its templated type. We don't want to have a - // virtual (or inline) interface to handle this though, so instead we make - // the TrackingVH accessors guarantee that a client never sees this value. - - LLVM_FALLTHROUGH; case Weak: // Weak goes to the new value, which will unlink it from Old's list. Entry->operator=(New); @@ -895,18 +883,17 @@ } #ifndef NDEBUG - // If any new tracking or weak value handles were added while processing the + // If any new weak value handles were added while processing the // list, then complain about it now. if (Old->HasValueHandle) for (Entry = pImpl->ValueHandles[Old]; Entry; Entry = Entry->Next) switch (Entry->getKind()) { - case Tracking: case Weak: dbgs() << "After RAUW from " << *Old->getType() << " %" << Old->getName() << " to " << *New->getType() << " %" << New->getName() << "\n"; - llvm_unreachable("A tracking or weak value handle still pointed to the" - " old value!\n"); + llvm_unreachable( + "A weak value handle still pointed to the old value!\n"); default: break; } Index: unittests/IR/ValueHandleTest.cpp =================================================================== --- unittests/IR/ValueHandleTest.cpp +++ unittests/IR/ValueHandleTest.cpp @@ -482,6 +482,12 @@ #else // !NDEBUG +TEST_F(ValueHandle, TrackingVH_Tracks) { + TrackingVH VH(BitcastV.get()); + BitcastV->replaceAllUsesWith(ConstantV); + EXPECT_EQ(VH, ConstantV); +} + #ifdef GTEST_HAS_DEATH_TEST TEST_F(ValueHandle, PoisoningVH_Asserts) { @@ -502,6 +508,26 @@ // Don't clear anything out here as destroying the handles should be fine. } +TEST_F(ValueHandle, TrackingVH_Asserts) { + { + TrackingVH VH(BitcastV.get()); + + // The tracking 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, + "TrackingVH must be non-null and valid on dereference!"); + } + + { + TrackingVH VH(BitcastV.get()); + + BitcastV->replaceAllUsesWith(ConstantV); + EXPECT_DEATH((void)*VH, + "Tracked Value was replaced by one with an invalid type!"); + } +} + #endif // GTEST_HAS_DEATH_TEST #endif // NDEBUG