From f5e33cfc8cabd5d66f184471f787a7cb8f8d3c6c Mon Sep 17 00:00:00 2001 From: Clark Barrett Date: Sat, 12 Nov 2016 09:16:33 -0800 Subject: [PATCH] Fixed a bug in cdhashmap in which doubly-linked list was not properly cleaned up on a call to obliterate. Also, removed some experimental code and a unit test from cdmap_black that used it. This test created a CDList *in* context memory which seems like a very bad idea (and it was improperly implemented resulting in a memory leak). --- src/context/cdhashmap.h | 22 +---- src/context/cdlist.h | 17 ---- test/unit/context/cdmap_black.h | 147 -------------------------------- 3 files changed, 3 insertions(+), 183 deletions(-) diff --git a/src/context/cdhashmap.h b/src/context/cdhashmap.h index 6ae74fbde..0a6c400a7 100644 --- a/src/context/cdhashmap.h +++ b/src/context/cdhashmap.h @@ -163,7 +163,7 @@ class CDOhash_map : public ContextObj { d_data = p->d_data; } } - // Explicitly call destructors for the key and the date as they will not + // Explicitly call destructors for the key and the data as they will not // otherwise get called. p->d_key.~Key(); p->d_data.~Data(); @@ -418,21 +418,6 @@ public: } } - // Use this for pointer data d allocated in context memory at this - // level. THIS IS HIGHLY EXPERIMENTAL. It seems to work if ALL - // your data objects are allocated from context memory. - void insertDataFromContextMemory(const Key& k, const Data& d) { - emptyTrash(); - - AlwaysAssert(d_map.find(k) == d_map.end()); - - Element* obj = new(d_context->getCMM()) Element(d_context, this, k, d, - false /* atLevelZero */, - true /* allocatedInCMM */); - - d_map[k] = obj; - } - /** * Version of insert() for CDHashMap<> that inserts data value d at * context level zero. This is a special escape hatch for inserting @@ -494,10 +479,9 @@ public: } else { d_first = elt->d_next; } - } else { - elt->d_prev->d_next = elt->d_next; - elt->d_next->d_prev = elt->d_prev; } + elt->d_prev->d_next = elt->d_next; + elt->d_next->d_prev = elt->d_prev; d_map.erase(j);//FIXME multithreading Debug("gc") << "key " << k << " obliterated zero-scope: " << elt << std::endl; if(!elt->d_noTrash) { diff --git a/src/context/cdlist.h b/src/context/cdlist.h index 4a5ebfd30..47667cc8d 100644 --- a/src/context/cdlist.h +++ b/src/context/cdlist.h @@ -237,23 +237,6 @@ public: d_allocator(alloc) { } - /** - * Main constructor: d_list starts as NULL, size is 0 - */ - CDList(bool allocatedInCMM, - Context* context, - bool callDestructor = true, - const CleanUp& cleanup = CleanUp(), - const Allocator& alloc = Allocator()) : - ContextObj(allocatedInCMM, context), - d_list(NULL), - d_size(0), - d_callDestructor(callDestructor), - d_sizeAlloc(0), - d_cleanUp(cleanup), - d_allocator(alloc) { - } - /** * Destructor: delete the list */ diff --git a/test/unit/context/cdmap_black.h b/test/unit/context/cdmap_black.h index 3da4dc0b0..c00b7c4d4 100644 --- a/test/unit/context/cdmap_black.h +++ b/test/unit/context/cdmap_black.h @@ -914,151 +914,4 @@ public: TS_ASSERT(map.find(23) == map.end()); TS_ASSERT(map[3] == 4); } - - struct int_hasher { - size_t operator()(int i) const { return i; } - }; - - struct myint { - int x; - myint(int i) : x(i) {} - ~myint() { std::cout << "dtor " << x << std::endl; } - myint& operator=(int i) { x = i; return *this; } - bool operator==(int i) const { return x == i; } - }; - - void testMapOfLists() { - //Debug.on("gc"); - //Debug.on("context"); - - CDHashMap*, int_hasher> map(d_context); - - CDList *list1, *list2, *list3, *list4; - - TS_ASSERT(map.find(1) == map.end()); - TS_ASSERT(map.find(2) == map.end()); - TS_ASSERT(map.find(3) == map.end()); - TS_ASSERT(map.find(4) == map.end()); - - { - d_context->push(); - - int* x CVC4_UNUSED = (int*) d_context->getCMM()->newData(sizeof(int)); - - list1 = new(d_context->getCMM()) CDList(true, d_context); - list2 = new(d_context->getCMM()) CDList(true, d_context); - list3 = new(d_context->getCMM()) CDList(true, d_context); - list4 = new(d_context->getCMM()) CDList(true, d_context); - - list1->push_back(1); - list2->push_back(2); - list3->push_back(3); - list4->push_back(4); - - map.insertDataFromContextMemory(1, list1); - map.insertDataFromContextMemory(2, list2); - - { - d_context->push(); - - list1->push_back(10); - list2->push_back(20); - list3->push_back(30); - list4->push_back(40); - - map.insertDataFromContextMemory(3, list3); - map.insertDataFromContextMemory(4, list4); - - x = (int*) d_context->getCMM()->newData(sizeof(int)); - - TS_ASSERT(map.find(1) != map.end()); - TS_ASSERT(map.find(2) != map.end()); - TS_ASSERT(map.find(3) != map.end()); - TS_ASSERT(map.find(4) != map.end()); - - TS_ASSERT(map[1] == list1); - TS_ASSERT(map[2] == list2); - TS_ASSERT(map[3] == list3); - TS_ASSERT(map[4] == list4); - - TS_ASSERT((*list1)[0] == 1); - TS_ASSERT((*list2)[0] == 2); - TS_ASSERT((*list3)[0] == 3); - TS_ASSERT((*list4)[0] == 4); - - TS_ASSERT((*list1)[1] == 10); - TS_ASSERT((*list2)[1] == 20); - TS_ASSERT((*list3)[1] == 30); - TS_ASSERT((*list4)[1] == 40); - - TS_ASSERT(list1->size() == 2); - TS_ASSERT(list2->size() == 2); - TS_ASSERT(list3->size() == 2); - TS_ASSERT(list4->size() == 2); - - d_context->pop(); - } - - TS_ASSERT(map.find(1) != map.end()); - TS_ASSERT(map.find(2) != map.end()); - TS_ASSERT(map.find(3) == map.end()); - TS_ASSERT(map.find(4) == map.end()); - - TS_ASSERT(map[1] == list1); - TS_ASSERT(map[2] == list2); - - TS_ASSERT((*list1)[0] == 1); - TS_ASSERT((*list2)[0] == 2); - TS_ASSERT((*list3)[0] == 3); - TS_ASSERT((*list4)[0] == 4); - - TS_ASSERT(list1->size() == 1); - TS_ASSERT(list2->size() == 1); - TS_ASSERT(list3->size() == 1); - TS_ASSERT(list4->size() == 1); - - d_context->pop(); - } - - { - d_context->push(); - - // This re-uses context memory used above. the map.clear() - // triggers an emptyTrash() which fails if the CDOhash_maps are put - // in the trash. (We use insertDataFromContextMemory() above to - // keep them out of the trash.) - cout << "allocating\n"; - int* x = (int*) d_context->getCMM()->newData(sizeof(int)); - cout << "x == " << x << std::endl; - for(int i = 0; i < 1000; ++i) { - *(int*)d_context->getCMM()->newData(sizeof(int)) = 512; - } - x = (int*) d_context->getCMM()->newData(sizeof(int)); - cout << "x == " << x << std::endl; - - TS_ASSERT(map.find(1) == map.end()); - TS_ASSERT(map.find(2) == map.end()); - TS_ASSERT(map.find(3) == map.end()); - TS_ASSERT(map.find(4) == map.end()); - - map.clear(); - - TS_ASSERT(map.find(1) == map.end()); - TS_ASSERT(map.find(2) == map.end()); - TS_ASSERT(map.find(3) == map.end()); - TS_ASSERT(map.find(4) == map.end()); - - d_context->pop(); - } - - TS_ASSERT(d_context->getLevel() == 0); - } - - void testCmmElementsAtLevel0() { - // this was crashing - - CDHashMap map(d_context); - int* a = (int*)d_context->getCMM()->newData(sizeof(int)); - map.insertDataFromContextMemory(1, a); - } }; -- 2.30.2