[llvm-commits] [llvm] r83861 - in /llvm/trunk: include/llvm/Support/ValueHandle.h lib/VMCore/Value.cpp unittests/Support/ValueHandleTest.cpp
Jeffrey Yasskin
jyasskin at google.com
Mon Oct 12 10:43:32 PDT 2009
Author: jyasskin
Date: Mon Oct 12 12:43:32 2009
New Revision: 83861
URL: http://llvm.org/viewvc/llvm-project?rev=83861&view=rev
Log:
Fix http://llvm.org/PR5160, to let CallbackVHs modify other ValueHandles on the
same Value without breaking things.
Modified:
llvm/trunk/include/llvm/Support/ValueHandle.h
llvm/trunk/lib/VMCore/Value.cpp
llvm/trunk/unittests/Support/ValueHandleTest.cpp
Modified: llvm/trunk/include/llvm/Support/ValueHandle.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/ValueHandle.h?rev=83861&r1=83860&r2=83861&view=diff
==============================================================================
--- llvm/trunk/include/llvm/Support/ValueHandle.h (original)
+++ llvm/trunk/include/llvm/Support/ValueHandle.h Mon Oct 12 12:43:32 2009
@@ -111,10 +111,15 @@
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.
Modified: llvm/trunk/lib/VMCore/Value.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/VMCore/Value.cpp?rev=83861&r1=83860&r2=83861&view=diff
==============================================================================
--- llvm/trunk/lib/VMCore/Value.cpp (original)
+++ llvm/trunk/lib/VMCore/Value.cpp Mon Oct 12 12:43:32 2009
@@ -411,6 +411,16 @@
}
}
+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 @@
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<Value *>::getTombstoneKey());
+ Entry->operator=(DenseMapInfo<Value *>::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<CallbackVH*>(ThisNode)->deleted();
+ static_cast<CallbackVH*>(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 @@
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 @@
// 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<CallbackVH*>(ThisNode)->allUsesReplacedWith(New);
+ static_cast<CallbackVH*>(Entry)->allUsesReplacedWith(New);
break;
}
}
Modified: llvm/trunk/unittests/Support/ValueHandleTest.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/ValueHandleTest.cpp?rev=83861&r1=83860&r2=83861&view=diff
==============================================================================
--- llvm/trunk/unittests/Support/ValueHandleTest.cpp (original)
+++ llvm/trunk/unittests/Support/ValueHandleTest.cpp Mon Oct 12 12:43:32 2009
@@ -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 @@
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<WeakVH> 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<Value*>(ShouldBeVisited1));
+ EXPECT_EQ(ConstantV, static_cast<Value*>(ShouldBeVisited2));
+ }
+
+ {
+ WeakVH ShouldBeVisited1(BitcastV.get());
+ DestroyingVH C(BitcastV.get());
+ WeakVH ShouldBeVisited2(BitcastV.get());
+
+ BitcastV.reset();
+ EXPECT_EQ(NULL, static_cast<Value*>(ShouldBeVisited1));
+ EXPECT_EQ(NULL, static_cast<Value*>(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<Value> *ToClear[2];
+ ClearingVH(Value *V,
+ AssertingVH<Value> &A0, AssertingVH<Value> &A1)
+ : CallbackVH(V) {
+ ToClear[0] = &A0;
+ ToClear[1] = &A1;
+ }
+
+ virtual void deleted() {
+ *ToClear[0] = 0;
+ *ToClear[1] = 0;
+ CallbackVH::deleted();
+ }
+ };
+
+ AssertingVH<Value> 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();
+}
+
}
More information about the llvm-commits
mailing list