From 6a9291ad55cf3b3731d3512eb5aa72ac7cdf02f9 Mon Sep 17 00:00:00 2001 From: Jeffrey Yasskin Date: Mon, 12 Oct 2009 17:43:32 +0000 Subject: [PATCH] Fix http://llvm.org/PR5160, to let CallbackVHs modify other ValueHandles on the same Value without breaking things. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@83861 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/llvm/Support/ValueHandle.h | 9 ++- lib/VMCore/Value.cpp | 69 +++++++++++++++-------- unittests/Support/ValueHandleTest.cpp | 81 +++++++++++++++++++++++++++ 3 files changed, 134 insertions(+), 25 deletions(-) diff --git a/include/llvm/Support/ValueHandle.h b/include/llvm/Support/ValueHandle.h index a8f2bdba552..e6363ffea98 100644 --- a/include/llvm/Support/ValueHandle.h +++ b/include/llvm/Support/ValueHandle.h @@ -111,10 +111,15 @@ private: HandleBaseKind getKind() const { return PrevPair.getInt(); } void setPrevPtr(ValueHandleBase **Ptr) { PrevPair.setPointer(Ptr); } - /// AddToExistingUseList - Add this ValueHandle to the use list for VP, - /// where List is known to point into the existing use list. + /// AddToExistingUseList - Add this ValueHandle to the use list for VP, where + /// List is the address of either the head of the list or a Next node within + /// the existing use list. void AddToExistingUseList(ValueHandleBase **List); + /// AddToExistingUseListAfter - Add this ValueHandle to the use list after + /// Node. + void AddToExistingUseListAfter(ValueHandleBase *Node); + /// AddToUseList - Add this ValueHandle to the use list for VP. void AddToUseList(); /// RemoveFromUseList - Remove this ValueHandle from its current use list. diff --git a/lib/VMCore/Value.cpp b/lib/VMCore/Value.cpp index 9fce24ab114..03b0e6f172e 100644 --- a/lib/VMCore/Value.cpp +++ b/lib/VMCore/Value.cpp @@ -411,6 +411,16 @@ void ValueHandleBase::AddToExistingUseList(ValueHandleBase **List) { } } +void ValueHandleBase::AddToExistingUseListAfter(ValueHandleBase *List) { + assert(List && "Must insert after existing node"); + + Next = List->Next; + setPrevPtr(&List->Next); + List->Next = this; + if (Next) + Next->setPrevPtr(&Next); +} + /// AddToUseList - Add this ValueHandle to the use list for VP. void ValueHandleBase::AddToUseList() { assert(VP && "Null pointer doesn't have a use list!"); @@ -490,37 +500,46 @@ void ValueHandleBase::ValueIsDeleted(Value *V) { ValueHandleBase *Entry = pImpl->ValueHandles[V]; assert(Entry && "Value bit set but no entries exist"); - while (Entry) { - // Advance pointer to avoid invalidation. - ValueHandleBase *ThisNode = Entry; - Entry = Entry->Next; + // We use a local ValueHandleBase as an iterator so that + // ValueHandles can add and remove themselves from the list without + // breaking our iteration. This is not really an AssertingVH; we + // just have to give ValueHandleBase some kind. + for (ValueHandleBase Iterator(Assert, *Entry); Entry; Entry = Iterator.Next) { + Iterator.RemoveFromUseList(); + Iterator.AddToExistingUseListAfter(Entry); + assert(Entry->Next == &Iterator && "Loop invariant broken."); - switch (ThisNode->getKind()) { + switch (Entry->getKind()) { case Assert: -#ifndef NDEBUG // Only in -g mode... - errs() << "While deleting: " << *V->getType() << " %" << V->getNameStr() - << "\n"; -#endif - llvm_unreachable("An asserting value handle still pointed to this" - " value!"); + break; case Tracking: // Mark that this value has been deleted by setting it to an invalid Value // pointer. - ThisNode->operator=(DenseMapInfo::getTombstoneKey()); + Entry->operator=(DenseMapInfo::getTombstoneKey()); break; case Weak: // Weak just goes to null, which will unlink it from the list. - ThisNode->operator=(0); + Entry->operator=(0); break; case Callback: // Forward to the subclass's implementation. - static_cast(ThisNode)->deleted(); + static_cast(Entry)->deleted(); break; } } - // All callbacks and weak references should be dropped by now. - assert(!V->HasValueHandle && "All references to V were not removed?"); + // All callbacks, weak references, and assertingVHs should be dropped by now. + if (V->HasValueHandle) { +#ifndef NDEBUG // Only in +Asserts mode... + errs() << "While deleting: " << *V->getType() << " %" << V->getNameStr() + << "\n"; + if (pImpl->ValueHandles[V]->getKind() == Assert) + llvm_unreachable("An asserting value handle still pointed to this" + " value!"); + +#endif + llvm_unreachable("All references to V were not removed?"); + } } @@ -535,12 +554,16 @@ void ValueHandleBase::ValueIsRAUWd(Value *Old, Value *New) { assert(Entry && "Value bit set but no entries exist"); - while (Entry) { - // Advance pointer to avoid invalidation. - ValueHandleBase *ThisNode = Entry; - Entry = Entry->Next; + // We use a local ValueHandleBase as an iterator so that + // ValueHandles can add and remove themselves from the list without + // breaking our iteration. This is not really an AssertingVH; we + // just have to give ValueHandleBase some kind. + for (ValueHandleBase Iterator(Assert, *Entry); Entry; Entry = Iterator.Next) { + Iterator.RemoveFromUseList(); + Iterator.AddToExistingUseListAfter(Entry); + assert(Entry->Next == &Iterator && "Loop invariant broken."); - switch (ThisNode->getKind()) { + switch (Entry->getKind()) { case Assert: // Asserting handle does not follow RAUW implicitly. break; @@ -553,11 +576,11 @@ void ValueHandleBase::ValueIsRAUWd(Value *Old, Value *New) { // FALLTHROUGH case Weak: // Weak goes to the new value, which will unlink it from Old's list. - ThisNode->operator=(New); + Entry->operator=(New); break; case Callback: // Forward to the subclass's implementation. - static_cast(ThisNode)->allUsesReplacedWith(New); + static_cast(Entry)->allUsesReplacedWith(New); break; } } diff --git a/unittests/Support/ValueHandleTest.cpp b/unittests/Support/ValueHandleTest.cpp index c6b53561d97..c89a7af6fef 100644 --- a/unittests/Support/ValueHandleTest.cpp +++ b/unittests/Support/ValueHandleTest.cpp @@ -11,6 +11,7 @@ #include "llvm/Constants.h" #include "llvm/Instructions.h" +#include "llvm/ADT/OwningPtr.h" #include "gtest/gtest.h" @@ -327,4 +328,84 @@ TEST_F(ValueHandle, CallbackVH_DeletionCanRAUW) { BitcastUser->getOperand(0)); } +TEST_F(ValueHandle, DestroyingOtherVHOnSameValueDoesntBreakIteration) { + // When a CallbackVH modifies other ValueHandles in its callbacks, + // that shouldn't interfere with non-modified ValueHandles receiving + // their appropriate callbacks. + // + // We create the active CallbackVH in the middle of a palindromic + // arrangement of other VHs so that the bad behavior would be + // triggered in whichever order callbacks run. + + class DestroyingVH : public CallbackVH { + public: + OwningPtr ToClear[2]; + DestroyingVH(Value *V) { + ToClear[0].reset(new WeakVH(V)); + setValPtr(V); + ToClear[1].reset(new WeakVH(V)); + } + virtual void deleted() { + ToClear[0].reset(); + ToClear[1].reset(); + CallbackVH::deleted(); + } + virtual void allUsesReplacedWith(Value *) { + ToClear[0].reset(); + ToClear[1].reset(); + } + }; + + { + WeakVH ShouldBeVisited1(BitcastV.get()); + DestroyingVH C(BitcastV.get()); + WeakVH ShouldBeVisited2(BitcastV.get()); + + BitcastV->replaceAllUsesWith(ConstantV); + EXPECT_EQ(ConstantV, static_cast(ShouldBeVisited1)); + EXPECT_EQ(ConstantV, static_cast(ShouldBeVisited2)); + } + + { + WeakVH ShouldBeVisited1(BitcastV.get()); + DestroyingVH C(BitcastV.get()); + WeakVH ShouldBeVisited2(BitcastV.get()); + + BitcastV.reset(); + EXPECT_EQ(NULL, static_cast(ShouldBeVisited1)); + EXPECT_EQ(NULL, static_cast(ShouldBeVisited2)); + } +} + +TEST_F(ValueHandle, AssertingVHCheckedLast) { + // If a CallbackVH exists to clear out a group of AssertingVHs on + // Value deletion, the CallbackVH should get a chance to do so + // before the AssertingVHs assert. + + class ClearingVH : public CallbackVH { + public: + AssertingVH *ToClear[2]; + ClearingVH(Value *V, + AssertingVH &A0, AssertingVH &A1) + : CallbackVH(V) { + ToClear[0] = &A0; + ToClear[1] = &A1; + } + + virtual void deleted() { + *ToClear[0] = 0; + *ToClear[1] = 0; + CallbackVH::deleted(); + } + }; + + AssertingVH A1, A2; + 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(); +} + } -- 2.34.1