re PR libstdc++/88199 (memory leak on unordered container move assignment)

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

	PR libstdc++/88199
	* include/bits/hashtable.h (_Hashtable<>::_M_assign_elements): New.
	(_Hashtable<>::operator=(const _Hashtable&)): Use latter.
	(_Hashtable<>::_M_move_assign(_Hashtable&&, false_type)): Likewise.
	* testsuite/23_containers/unordered_set/allocator/move_assign.cc
	(test03): New.

From-SVN: r266528

diff --git a/libstdc++-v3/ChangeLog b/libstdc++-v3/ChangeLog
index 11505c0f31f56496b7ba7ff3b0026a68fda0995b..4de7cc8517496cfd9a69c519167448d492e01925 100644 (file)
--- a/libstdc++-v3/ChangeLog
+++ b/libstdc++-v3/ChangeLog
@@ -1,3 +1,12 @@
+2018-11-27  François Dumont  <fdumont@gcc.gnu.org>
+
+       PR libstdc++/88199
+       * include/bits/hashtable.h (_Hashtable<>::_M_assign_elements): New.
+       (_Hashtable<>::operator=(const _Hashtable&)): Use latter.
+       (_Hashtable<>::_M_move_assign(_Hashtable&&, false_type)): Likewise.
+       * testsuite/23_containers/unordered_set/allocator/move_assign.cc
+       (test03): New.
+
 2018-11-26  Jonathan Wakely  <jwakely@redhat.com>
 
        * testsuite/26_numerics/complex/requirements/more_constexpr.cc: Fix
@@ -20,7 +29,7 @@
        more_constexpr.cc: New test.
        * testsuite/26_numerics/headers/complex/synopsis.cc:
        Add _GLIBCXX20_CONSTEXPR to applicable operators; Add missing proj().
-       * testsuite/26_numerics/headers/complex/synopsis.cc: 
+       * testsuite/26_numerics/headers/complex/synopsis.cc:
        Add _GLIBCXX20_CONSTEXPR to relevant decls.
 
 2018-11-23  Martin Sebor  <msebor@redhat.com>

diff --git a/libstdc++-v3/include/bits/hashtable.h b/libstdc++-v3/include/bits/hashtable.h
index af16e2c03cb6fbb7d4542b3f8767856c34cd2056..f6912db3e69277b8d3d2292d4e3fd6e7ff2b93bc 100644 (file)
--- a/libstdc++-v3/include/bits/hashtable.h
+++ b/libstdc++-v3/include/bits/hashtable.h
@@ -388,6 +388,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       _M_begin() const
       { return static_cast<__node_type*>(_M_before_begin._M_nxt); }
 
+      // Assign *this using another _Hashtable instance. Either elements
+      // are copy or move depends on the _NodeGenerator.
+      template<typename _Ht, typename _NodeGenerator>
+       void
+       _M_assign_elements(_Ht&&, const _NodeGenerator&);
+
       template<typename _NodeGenerator>
        void
        _M_assign(const _Hashtable&, const _NodeGenerator&);
@@ -1042,49 +1048,64 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
        }
 
       // Reuse allocated buckets and nodes.
-      __bucket_type* __former_buckets = nullptr;
-      std::size_t __former_bucket_count = _M_bucket_count;
-      const __rehash_state& __former_state = _M_rehash_policy._M_state();
+      _M_assign_elements(__ht,
+       [](const __reuse_or_alloc_node_type& __roan, const __node_type* __n)
+       { return __roan(__n->_M_v()); });
+      return *this;
+    }
 
-      if (_M_bucket_count != __ht._M_bucket_count)
-       {
-         __former_buckets = _M_buckets;
-         _M_buckets = _M_allocate_buckets(__ht._M_bucket_count);
-         _M_bucket_count = __ht._M_bucket_count;
-       }
-      else
-       __builtin_memset(_M_buckets, 0,
-                        _M_bucket_count * sizeof(__bucket_type));
+  template<typename _Key, typename _Value,
+          typename _Alloc, typename _ExtractKey, typename _Equal,
+          typename _H1, typename _H2, typename _Hash, typename _RehashPolicy,
+          typename _Traits>
+    template<typename _Ht, typename _NodeGenerator>
+      void
+      _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal,
+                _H1, _H2, _Hash, _RehashPolicy, _Traits>::
+      _M_assign_elements(_Ht&& __ht, const _NodeGenerator& __node_gen)
+      {
+       __bucket_type* __former_buckets = nullptr;
+       std::size_t __former_bucket_count = _M_bucket_count;
+       const __rehash_state& __former_state = _M_rehash_policy._M_state();
 
-      __try
-       {
-         __hashtable_base::operator=(__ht);
-         _M_element_count = __ht._M_element_count;
-         _M_rehash_policy = __ht._M_rehash_policy;
-         __reuse_or_alloc_node_type __roan(_M_begin(), *this);
-         _M_before_begin._M_nxt = nullptr;
-         _M_assign(__ht,
-                   [&__roan](const __node_type* __n)
-                   { return __roan(__n->_M_v()); });
-         if (__former_buckets)
-           _M_deallocate_buckets(__former_buckets, __former_bucket_count);
-       }
-      __catch(...)
-       {
-         if (__former_buckets)
-           {
-             // Restore previous buckets.
-             _M_deallocate_buckets();
-             _M_rehash_policy._M_reset(__former_state);
-             _M_buckets = __former_buckets;
-             _M_bucket_count = __former_bucket_count;
-           }
+       if (_M_bucket_count != __ht._M_bucket_count)
+         {
+           __former_buckets = _M_buckets;
+           _M_buckets = _M_allocate_buckets(__ht._M_bucket_count);
+           _M_bucket_count = __ht._M_bucket_count;
+         }
+       else
          __builtin_memset(_M_buckets, 0,
                           _M_bucket_count * sizeof(__bucket_type));
-         __throw_exception_again;
-       }
-      return *this;
-    }
+
+       __try
+         {
+           __hashtable_base::operator=(std::forward<_Ht>(__ht));
+           _M_element_count = __ht._M_element_count;
+           _M_rehash_policy = __ht._M_rehash_policy;
+           __reuse_or_alloc_node_type __roan(_M_begin(), *this);
+           _M_before_begin._M_nxt = nullptr;
+           _M_assign(__ht,
+                     [&__node_gen, &__roan](__node_type* __n)
+                     { return __node_gen(__roan, __n); });
+           if (__former_buckets)
+             _M_deallocate_buckets(__former_buckets, __former_bucket_count);
+         }
+       __catch(...)
+         {
+           if (__former_buckets)
+             {
+               // Restore previous buckets.
+               _M_deallocate_buckets();
+               _M_rehash_policy._M_reset(__former_state);
+               _M_buckets = __former_buckets;
+               _M_bucket_count = __former_bucket_count;
+             }
+           __builtin_memset(_M_buckets, 0,
+                            _M_bucket_count * sizeof(__bucket_type));
+           __throw_exception_again;
+         }
+      }
 
   template<typename _Key, typename _Value,
           typename _Alloc, typename _ExtractKey, typename _Equal,
@@ -1198,45 +1219,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       else
        {
          // Can't move memory, move elements then.
-         __bucket_type* __former_buckets = nullptr;
-         size_type __former_bucket_count = _M_bucket_count;
-         const __rehash_state& __former_state = _M_rehash_policy._M_state();
-
-         if (_M_bucket_count != __ht._M_bucket_count)
-           {
-             __former_buckets = _M_buckets;
-             _M_buckets = _M_allocate_buckets(__ht._M_bucket_count);
-             _M_bucket_count = __ht._M_bucket_count;
-           }
-         else
-           __builtin_memset(_M_buckets, 0,
-                            _M_bucket_count * sizeof(__bucket_type));
-
-         __try
-           {
-             __hashtable_base::operator=(std::move(__ht));
-             _M_element_count = __ht._M_element_count;
-             _M_rehash_policy = __ht._M_rehash_policy;
-             __reuse_or_alloc_node_type __roan(_M_begin(), *this);
-             _M_before_begin._M_nxt = nullptr;
-             _M_assign(__ht,
-                       [&__roan](__node_type* __n)
-                       { return __roan(std::move_if_noexcept(__n->_M_v())); });
-             __ht.clear();
-           }
-         __catch(...)
-           {
-             if (__former_buckets)
-               {
-                 _M_deallocate_buckets();
-                 _M_rehash_policy._M_reset(__former_state);
-                 _M_buckets = __former_buckets;
-                 _M_bucket_count = __former_bucket_count;
-               }
-             __builtin_memset(_M_buckets, 0,
-                              _M_bucket_count * sizeof(__bucket_type));
-             __throw_exception_again;
-           }
+         _M_assign_elements(std::move(__ht),
+               [](const __reuse_or_alloc_node_type& __roan, __node_type* __n)
+               { return __roan(std::move_if_noexcept(__n->_M_v())); });
+         __ht.clear();
        }
     }
 

diff --git a/libstdc++-v3/testsuite/23_containers/unordered_set/allocator/move_assign.cc b/libstdc++-v3/testsuite/23_containers/unordered_set/allocator/move_assign.cc
index ef6c0deb1af934c9ba752a82d9c002b7f2c2551c..3a5538975812e853abe78f5d6080907eab362520 100644 (file)
--- a/libstdc++-v3/testsuite/23_containers/unordered_set/allocator/move_assign.cc
+++ b/libstdc++-v3/testsuite/23_containers/unordered_set/allocator/move_assign.cc
@@ -18,71 +18,133 @@
 // { dg-do run { target c++11 } }
 
 #include <unordered_set>
+
 #include <testsuite_hooks.h>
 #include <testsuite_allocator.h>
 #include <testsuite_counter_type.h>
 
 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;
-  typedef __gnu_test::counter_type_hasher hash;
-  typedef std::unordered_set<counter_type, hash,
-                            std::equal_to<counter_type>,
-                            alloc_type> test_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>,
+                              alloc_type> test_type;
+
+    test_type v1(alloc_type(1));
+    v1.emplace(0);
 
-  test_type v1(alloc_type(1));
-  v1.emplace(0);
+    test_type v2(alloc_type(2));
+    v2.emplace(1);
 
-  test_type v2(alloc_type(2));
-  v2.emplace(1);
+    counter_type::reset();
 
-  counter_type::reset();
+    v2 = std::move(v1);
 
-  v2 = std::move(v1);
+    VERIFY( 1 == v1.get_allocator().get_personality() );
+    VERIFY( 2 == v2.get_allocator().get_personality() );
 
-  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 == 2 );
+  }
 
-  VERIFY( counter_type::move_count == 1  );
-  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;
-  typedef __gnu_test::counter_type_hasher hash;
-  typedef std::unordered_set<counter_type, hash,
-                            std::equal_to<counter_type>,
-                            alloc_type> test_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>,
+                              alloc_type> test_type;
+
+    test_type v1(alloc_type(1));
+    v1.emplace(0);
 
-  test_type v1(alloc_type(1));
-  v1.emplace(0);
+    auto it = v1.begin();
 
-  auto it = v1.begin();
+    test_type v2(alloc_type(2));
+    v2.emplace(0);
 
-  test_type v2(alloc_type(2));
-  v2.emplace(0);
+    counter_type::reset();
 
-  counter_type::reset();
+    v2 = std::move(v1);
 
-  v2 = std::move(v1);
+    VERIFY(0 == v1.get_allocator().get_personality());
+    VERIFY(1 == v2.get_allocator().get_personality());
 
-  VERIFY(0 == v1.get_allocator().get_personality());
-  VERIFY(1 == v2.get_allocator().get_personality());
+    VERIFY( counter_type::move_count == 0 );
+    VERIFY( counter_type::copy_count == 0 );
+    VERIFY( counter_type::destructor_count == 1 );
 
-  VERIFY( counter_type::move_count == 0 );
-  VERIFY( counter_type::copy_count == 0 );
-  VERIFY( counter_type::destructor_count == 1 );
+    VERIFY( it == v2.begin() );
+  }
 
-  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;
 }