Fix hashtable memory leak

Fix hashtable memory leak

[François Dumont]

[-- Attachment #1: Type: text/plain, Size: 699 bytes --]

There a memory leak when move assigning between 2 instances with 
different bucket count and non-propagating and unequal allocator 
instances (see new test03).

I reduced code duplication to fix the issue as 1 of the 2 
implementations was fine.

     * include/bits/hashtable.h (_Hashtable<>::_M_replicate): 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.

I still need to run all tests but new move_assign.cc works fine.

Ok to commit once fully tested ?

François


[-- Attachment #2: hashtable_mem_leak.patch --]
[-- Type: text/x-patch, Size: 6510 bytes --]

diff --git a/libstdc++-v3/include/bits/hashtable.h b/libstdc++-v3/include/bits/hashtable.h
index efc4c4ab94f..61e177abb45 100644
--- a/libstdc++-v3/include/bits/hashtable.h
+++ b/libstdc++-v3/include/bits/hashtable.h
@@ -398,6 +398,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       _M_begin() const
       { return static_cast<__node_type*>(_M_before_begin._M_nxt); }
 
+      template<typename _Ht, typename _NodeGenerator>
+	void
+	_M_replicate(_Ht&&, const _NodeGenerator&);
+
       template<typename _NodeGenerator>
 	void
 	_M_assign(const _Hashtable&, const _NodeGenerator&);
@@ -1058,6 +1062,23 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	}
 
       // Reuse allocated buckets and nodes.
+      __reuse_or_alloc_node_type __roan(_M_begin(), *this);
+      _M_replicate(__ht,
+		   [&__roan](const __node_type* __n)
+		   { return __roan(__n->_M_v()); });
+      return *this;
+    }
+
+  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_replicate(_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();
@@ -1074,14 +1095,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
 	__try
 	  {
-	  __hashtable_base::operator=(__ht);
+	    __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,
-		    [&__roan](const __node_type* __n)
-		    { return __roan(__n->_M_v()); });
+	    _M_assign(__ht, __node_gen);
 	    if (__former_buckets)
 	      _M_deallocate_buckets(__former_buckets, __former_bucket_count);
 	  }
@@ -1099,7 +1117,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 			     _M_bucket_count * sizeof(__bucket_type));
 	    __throw_exception_again;
 	  }
-      return *this;
       }
 
   template<typename _Key, typename _Value,
@@ -1227,46 +1244,12 @@ _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,
+	  _M_replicate(std::move(__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;
-	    }
-	}
     }
 
   template<typename _Key, typename _Value,
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 ef6c0deb1af..59153001d29 100644
--- 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,6 +18,8 @@
 // { dg-do run { target c++11 } }
 
 #include <unordered_set>
+#include <ext/throw_allocator.h>
+
 #include <testsuite_hooks.h>
 #include <testsuite_allocator.h>
 #include <testsuite_counter_type.h>
@@ -27,7 +29,9 @@ using __gnu_test::counter_type;
 
 void test01()
 {
-  typedef propagating_allocator<counter_type, false> alloc_type;
+  {
+    typedef propagating_allocator<counter_type, false,
+		__gnu_cxx::throw_allocator_limit<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 +54,15 @@ void test01()
     VERIFY( counter_type::destructor_count == 2 );
   }
 
+  // Check there's nothing left allocated or constructed.
+  __gnu_cxx::annotate_base::check();
+}
+
 void test02()
 {
-  typedef propagating_allocator<counter_type, true> alloc_type;
+  {
+    typedef propagating_allocator<counter_type, true,
+		__gnu_cxx::throw_allocator_limit<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 +90,48 @@ void test02()
     VERIFY( it == v2.begin() );
   }
 
+  // Check there's nothing left allocated or constructed.
+  __gnu_cxx::annotate_base::check();
+}
+
+void test03()
+{
+  {
+    typedef propagating_allocator<counter_type, false,
+		__gnu_cxx::throw_allocator_limit<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.
+  __gnu_cxx::annotate_base::check();
+}
+
 int main()
 {
   test01();
   test02();
+  test03();
   return 0;
 }

Re: Fix hashtable memory leak

[François Dumont]

[-- Attachment #1: Type: text/plain, Size: 1005 bytes --]

Tests have shown a regression. I was building the _ReuseOrAllocNode 
instance too early, node ownership was transfered but was still owned by 
_Hashtable instance.

This new version pass all tests.

Ok to commit ?

François



On 11/19/18 10:23 PM, François Dumont wrote:
> There a memory leak when move assigning between 2 instances with 
> different bucket count and non-propagating and unequal allocator 
> instances (see new test03).
>
> I reduced code duplication to fix the issue as 1 of the 2 
> implementations was fine.
>
>     * include/bits/hashtable.h (_Hashtable<>::_M_replicate): 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.
>
> I still need to run all tests but new move_assign.cc works fine.
>
> Ok to commit once fully tested ?
>
> François
>


[-- Attachment #2: hashtable_mem_leak.patch --]
[-- Type: text/x-patch, Size: 6598 bytes --]

diff --git a/libstdc++-v3/include/bits/hashtable.h b/libstdc++-v3/include/bits/hashtable.h
index efc4c4ab94f..caa4e8ad20c 100644
--- a/libstdc++-v3/include/bits/hashtable.h
+++ b/libstdc++-v3/include/bits/hashtable.h
@@ -398,6 +398,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       _M_begin() const
       { return static_cast<__node_type*>(_M_before_begin._M_nxt); }
 
+      template<typename _Ht, typename _NodeGenerator>
+	void
+	_M_replicate(_Ht&&, const _NodeGenerator&);
+
       template<typename _NodeGenerator>
 	void
 	_M_assign(const _Hashtable&, const _NodeGenerator&);
@@ -1058,6 +1062,22 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	}
 
       // Reuse allocated buckets and nodes.
+      _M_replicate(__ht,
+	[](const __reuse_or_alloc_node_type& __roan, const __node_type* __n)
+	{ return __roan(__n->_M_v()); });
+      return *this;
+    }
+
+  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_replicate(_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();
@@ -1074,14 +1094,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
 	__try
 	  {
-	  __hashtable_base::operator=(__ht);
+	    __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,
-		    [&__roan](const __node_type* __n)
-		    { return __roan(__n->_M_v()); });
+		      [&__node_gen, &__roan](__node_type* __n)
+		      { return __node_gen(__roan, __n); });
 	    if (__former_buckets)
 	      _M_deallocate_buckets(__former_buckets, __former_bucket_count);
 	  }
@@ -1099,7 +1119,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 			     _M_bucket_count * sizeof(__bucket_type));
 	    __throw_exception_again;
 	  }
-      return *this;
       }
 
   template<typename _Key, typename _Value,
@@ -1227,46 +1246,11 @@ _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)
+	  _M_replicate(std::move(__ht),
+		[](const __reuse_or_alloc_node_type& __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;
-	    }
-	}
     }
 
   template<typename _Key, typename _Value,
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 ef6c0deb1af..59153001d29 100644
--- 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,6 +18,8 @@
 // { dg-do run { target c++11 } }
 
 #include <unordered_set>
+#include <ext/throw_allocator.h>
+
 #include <testsuite_hooks.h>
 #include <testsuite_allocator.h>
 #include <testsuite_counter_type.h>
@@ -27,7 +29,9 @@ using __gnu_test::counter_type;
 
 void test01()
 {
-  typedef propagating_allocator<counter_type, false> alloc_type;
+  {
+    typedef propagating_allocator<counter_type, false,
+		__gnu_cxx::throw_allocator_limit<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 +54,15 @@ void test01()
     VERIFY( counter_type::destructor_count == 2 );
   }
 
+  // Check there's nothing left allocated or constructed.
+  __gnu_cxx::annotate_base::check();
+}
+
 void test02()
 {
-  typedef propagating_allocator<counter_type, true> alloc_type;
+  {
+    typedef propagating_allocator<counter_type, true,
+		__gnu_cxx::throw_allocator_limit<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 +90,48 @@ void test02()
     VERIFY( it == v2.begin() );
   }
 
+  // Check there's nothing left allocated or constructed.
+  __gnu_cxx::annotate_base::check();
+}
+
+void test03()
+{
+  {
+    typedef propagating_allocator<counter_type, false,
+		__gnu_cxx::throw_allocator_limit<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.
+  __gnu_cxx::annotate_base::check();
+}
+
 int main()
 {
   test01();
   test02();
+  test03();
   return 0;
 }

Re: Fix hashtable memory leak

[Jonathan Wakely]

On 24/11/18 22:58 +0100, François Dumont wrote:
>Tests have shown a regression. I was building the _ReuseOrAllocNode 
>instance too early, node ownership was transfered but was still owned 
>by _Hashtable instance.
>
>This new version pass all tests.

This is why it's worth waiting until tests have run before sending a
patch for review.

>Ok to commit ?

OK, but please rename _M_replicate to _M_do_assign or _M_assign_impl
or something that makes it clear this is part of the assignment
operation. The name "replicate" isn't clear.

A comment on the declaration of the new function could be helpful too.

Thanks for finding this leak. I think it's worth backporting the fix,
but for gcc-7-branch and gcc-8-branch it should be just:

--- a/libstdc++-v3/include/bits/hashtable.h
+++ b/libstdc++-v3/include/bits/hashtable.h
@@ -1223,6 +1223,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
                        [&__roan](__node_type* __n)
                        { return __roan(std::move_if_noexcept(__n->_M_v())); });
              __ht.clear();
+             if (__former_buckets)
+               _M_deallocate_buckets(__former_buckets, __former_bucket_count);
            }
          __catch(...)
            {

(Plus the improvements to the tests, of course).

Re: Fix hashtable memory leak

[Jonathan Wakely]

On 26/11/18 12:03 +0000, Jonathan Wakely wrote:
>On 24/11/18 22:58 +0100, François Dumont wrote:
>>Tests have shown a regression. I was building the _ReuseOrAllocNode 
>>instance too early, node ownership was transfered but was still 
>>owned by _Hashtable instance.
>>
>>This new version pass all tests.
>
>This is why it's worth waiting until tests have run before sending a
>patch for review.
>
>>Ok to commit ?
>
>OK, but please rename _M_replicate to _M_do_assign or _M_assign_impl
>or something that makes it clear this is part of the assignment
>operation. The name "replicate" isn't clear.
>
>A comment on the declaration of the new function could be helpful too.
>
>Thanks for finding this leak. I think it's worth backporting the fix,
>but for gcc-7-branch and gcc-8-branch it should be just:
>
>--- a/libstdc++-v3/include/bits/hashtable.h
>+++ b/libstdc++-v3/include/bits/hashtable.h
>@@ -1223,6 +1223,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>                       [&__roan](__node_type* __n)
>                       { return __roan(std::move_if_noexcept(__n->_M_v())); });
>             __ht.clear();
>+             if (__former_buckets)
>+               _M_deallocate_buckets(__former_buckets, __former_bucket_count);
>           }
>         __catch(...)
>           {
>
>(Plus the improvements to the tests, of course).

Because this is a regression, and we want to fix it on the branches,
I've created a bugzilla to track it:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88199

Re: Fix hashtable memory leak

[Jonathan Wakely]

On 24/11/18 22:58 +0100, François Dumont wrote:
>--- 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,6 +18,8 @@
> // { dg-do run { target c++11 } }
> 
> #include <unordered_set>
>+#include <ext/throw_allocator.h>
>+
> #include <testsuite_hooks.h>
> #include <testsuite_allocator.h>
> #include <testsuite_counter_type.h>
>@@ -27,7 +29,9 @@ using __gnu_test::counter_type;
> 
> void test01()
> {
>-  typedef propagating_allocator<counter_type, false> alloc_type;
>+  {
>+    typedef propagating_allocator<counter_type, false,
>+		__gnu_cxx::throw_allocator_limit<counter_type>> alloc_type;

You don't seem to need the throw_allocator here, can't you use
__gnu_test::tracker_allocator from <testsuite_allocator.h> instead?

Re: Fix hashtable memory leak

[François Dumont]

I eventually called the new method _M_assign_elements.

And yes, tracker_allocator was enough.

Committed on trunk for the moment.

François


On 11/26/18 1:12 PM, Jonathan Wakely wrote:
> On 24/11/18 22:58 +0100, François Dumont wrote:
>> --- 
>> 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,6 +18,8 @@
>> // { dg-do run { target c++11 } }
>>
>> #include <unordered_set>
>> +#include <ext/throw_allocator.h>
>> +
>> #include <testsuite_hooks.h>
>> #include <testsuite_allocator.h>
>> #include <testsuite_counter_type.h>
>> @@ -27,7 +29,9 @@ using __gnu_test::counter_type;
>>
>> void test01()
>> {
>> -  typedef propagating_allocator<counter_type, false> alloc_type;
>> +  {
>> +    typedef propagating_allocator<counter_type, false,
>> + __gnu_cxx::throw_allocator_limit<counter_type>> alloc_type;
>
> You don't seem to need the throw_allocator here, can't you use
> __gnu_test::tracker_allocator from <testsuite_allocator.h> instead?
>
> .
>

Re: Fix hashtable memory leak

[Jonathan Wakely]

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.

Re: Fix hashtable memory leak

[François Dumont]

[-- Attachment #1: Type: text/plain, Size: 1013 bytes --]

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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: hashtable_mem_leak.patch --]
[-- Type: text/x-patch; name="hashtable_mem_leak.patch", Size: 3941 bytes --]

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;
 }

Re: Fix hashtable memory leak

[Jonathan Wakely]

On 28/11/18 07:36 +0100, François Dumont wrote:
>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.

Thanks!