[llvm-commits] [llvm] r109851 - in /llvm/trunk: include/llvm/ADT/ValueMap.h unittests/ADT/ValueMapTest.cpp

[Duncan Sands]

Author: baldrick
Date: Fri Jul 30 00:49:32 2010
New Revision: 109851

URL: http://llvm.org/viewvc/llvm-project?rev=109851&view=rev
Log:
Fix the ValueMap copy constructor.  The issue is that the map keys are value
handles with a pointer to the containing map.  When a map is copied, these
pointers need to be corrected to point to the new map.  If not, then consider
the case of a map M1 which maps a value V to something.  Create a copy M2 of
M1.  At this point there are two value handles on V, one representing V as a
key in M1, the other representing V as a key in M2.  But both value handles
point to M1 as the containing map.  Now delete V.  The value handles remove
themselves from their containing map (which destroys them), but only the first
value handle is successful: the second one cannot remove itself from M1 as
(once the first one has removed itself) there is nothing there to remove; it
is therefore not destroyed.  This causes an assertion failure "All references
to V were not removed?".

Modified:
    llvm/trunk/include/llvm/ADT/ValueMap.h
    llvm/trunk/unittests/ADT/ValueMapTest.cpp

Modified: llvm/trunk/include/llvm/ADT/ValueMap.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/ValueMap.h?rev=109851&r1=109850&r2=109851&view=diff
==============================================================================
--- llvm/trunk/include/llvm/ADT/ValueMap.h (original)
+++ llvm/trunk/include/llvm/ADT/ValueMap.h Fri Jul 30 00:49:32 2010
@@ -87,7 +87,12 @@
   typedef ValueT mapped_type;
   typedef std::pair<KeyT, ValueT> value_type;
 
-  ValueMap(const ValueMap& Other) : Map(Other.Map), Data(Other.Data) {}
+  ValueMap(const ValueMap& Other) : Map(Other.Map), Data(Other.Data) {
+    // Each ValueMapCVH key contains a pointer to the containing ValueMap.
+    // The keys in the new map need to point to the new map, not Other.
+    for (typename MapT::iterator I = Map.begin(), E = Map.end(); I != E; ++I)
+      I->first.Map = this;
+  }
 
   explicit ValueMap(unsigned NumInitBuckets = 64)
     : Map(NumInitBuckets), Data() {}

Modified: llvm/trunk/unittests/ADT/ValueMapTest.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/ValueMapTest.cpp?rev=109851&r1=109850&r2=109851&view=diff
==============================================================================
--- llvm/trunk/unittests/ADT/ValueMapTest.cpp (original)
+++ llvm/trunk/unittests/ADT/ValueMapTest.cpp Fri Jul 30 00:49:32 2010
@@ -39,6 +39,15 @@
 typedef ::testing::Types<Value, Instruction, const Instruction> KeyTypes;
 TYPED_TEST_CASE(ValueMapTest, KeyTypes);
 
+TYPED_TEST(ValueMapTest, CopyConstructor) {
+  ValueMap<TypeParam*, int> VM1;
+  VM1[this->AddV.get()] = 7;
+  ValueMap<TypeParam*, int> VM2(VM1);
+  this->AddV.reset();
+  EXPECT_TRUE(VM1.empty());
+  EXPECT_TRUE(VM2.empty());
+}
+
 TYPED_TEST(ValueMapTest, Null) {
   ValueMap<TypeParam*, int> VM1;
   VM1[NULL] = 7;