Re: Fix hashtable memory leak

[François Dumont <frs dot dumont at gmail dot com>]

On 11/27/18 11:00 PM, Jonathan Wakely wrote:
> On 27/11/18 22:31 +0100, François Dumont wrote:
> > I eventually called the new method _M_assign_elements.
>
> Perfect.
>
> > And yes, tracker_allocator was enough.
>
> Great.
>
> > Committed on trunk for the moment.
>
> Great, thanks.
>
> Please note that GCC 7.4 RC1 is scheduled for this week:
> https://gcc.gnu.org/ml/gcc/2018-11/msg00118.html
>
> Will you be able to backport the simpler patch (without the
> refactoring to remove code duplication) to the branch before then?
>
> If not I can take care of it.
I just backport to gcc-7/8-branch the same patch.

2018-11-28  François Dumont  <fdumont@gcc.gnu.org>

    PR libstdc++/88199
    * include/bits/hashtable.h
    (_Hashtable<>::_M_move_assign(_Hashtable&&, false_type)): Deallocate
    former buckets after assignment.
    * testsuite/23_containers/unordered_set/allocator/move_assign.cc
    (test03): New.

Bests, François

Index: libstdc++-v3/include/bits/hashtable.h
===================================================================
--- libstdc++-v3/include/bits/hashtable.h	(révision 266538)
+++ libstdc++-v3/include/bits/hashtable.h	(copie de travail)
@@ -1222,6 +1222,9 @@
 	      _M_assign(__ht,
 			[&__roan](__node_type* __n)
 			{ return __roan(std::move_if_noexcept(__n->_M_v())); });
+
+	      if (__former_buckets)
+		_M_deallocate_buckets(__former_buckets, __former_bucket_count);
 	      __ht.clear();
 	    }
 	  __catch(...)
Index: libstdc++-v3/testsuite/23_containers/unordered_set/allocator/move_assign.cc
===================================================================
--- libstdc++-v3/testsuite/23_containers/unordered_set/allocator/move_assign.cc	(révision 266538)
+++ libstdc++-v3/testsuite/23_containers/unordered_set/allocator/move_assign.cc	(copie de travail)
@@ -18,6 +18,7 @@
 // { dg-do run { target c++11 } }
 
 #include <unordered_set>
+
 #include <testsuite_hooks.h>
 #include <testsuite_allocator.h>
 #include <testsuite_counter_type.h>
@@ -24,10 +25,15 @@
 
 using __gnu_test::propagating_allocator;
 using __gnu_test::counter_type;
+using __gnu_test::tracker_allocator;
+using __gnu_test::tracker_allocator_counter;
 
 void test01()
 {
-  typedef propagating_allocator<counter_type, false> alloc_type;
+  tracker_allocator_counter::reset();
+  {
+    typedef propagating_allocator<counter_type, false,
+				  tracker_allocator<counter_type>> alloc_type;
   typedef __gnu_test::counter_type_hasher hash;
   typedef std::unordered_set<counter_type, hash,
 			     std::equal_to<counter_type>,
@@ -50,9 +56,19 @@
   VERIFY( counter_type::destructor_count == 2 );
 }
 
+  // Check there's nothing left allocated or constructed.
+  VERIFY( tracker_allocator_counter::get_construct_count()
+	  == tracker_allocator_counter::get_destruct_count() );
+  VERIFY( tracker_allocator_counter::get_allocation_count()
+	  == tracker_allocator_counter::get_deallocation_count() );
+}
+
 void test02()
 {
-  typedef propagating_allocator<counter_type, true> alloc_type;
+  tracker_allocator_counter::reset();
+  {
+    typedef propagating_allocator<counter_type, true,
+				  tracker_allocator<counter_type>> alloc_type;
   typedef __gnu_test::counter_type_hasher hash;
   typedef std::unordered_set<counter_type, hash,
 			     std::equal_to<counter_type>,
@@ -80,9 +96,55 @@
   VERIFY( it == v2.begin() );
 }
 
+  // Check there's nothing left allocated or constructed.
+  VERIFY( tracker_allocator_counter::get_construct_count()
+	  == tracker_allocator_counter::get_destruct_count() );
+  VERIFY( tracker_allocator_counter::get_allocation_count()
+	  == tracker_allocator_counter::get_deallocation_count() );
+}
+
+void test03()
+{
+  tracker_allocator_counter::reset();
+  {
+    typedef propagating_allocator<counter_type, false,
+				  tracker_allocator<counter_type>> alloc_type;
+    typedef __gnu_test::counter_type_hasher hash;
+    typedef std::unordered_set<counter_type, hash,
+			       std::equal_to<counter_type>,
+			       alloc_type> test_type;
+
+    test_type v1(alloc_type(1));
+    v1.emplace(0);
+
+    test_type v2(alloc_type(2));
+    int i = 0;
+    v2.emplace(i++);
+    for (; v2.bucket_count() == v1.bucket_count(); ++i)
+      v2.emplace(i);
+
+    counter_type::reset();
+
+    v2 = std::move(v1);
+
+    VERIFY( 1 == v1.get_allocator().get_personality() );
+    VERIFY( 2 == v2.get_allocator().get_personality() );
+
+    VERIFY( counter_type::move_count == 1  );
+    VERIFY( counter_type::destructor_count == i + 1 );
+  }
+
+  // Check there's nothing left allocated or constructed.
+  VERIFY( tracker_allocator_counter::get_construct_count()
+	  == tracker_allocator_counter::get_destruct_count() );
+  VERIFY( tracker_allocator_counter::get_allocation_count()
+	  == tracker_allocator_counter::get_deallocation_count() );
+}
+
 int main()
 {
   test01();
   test02();
+  test03();
   return 0;
 }