Fix hashtable memory leak

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Fix hashtable memory leak

François Dumont-2
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


hashtable_mem_leak.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Fix hashtable memory leak

François Dumont-2
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
>


hashtable_mem_leak.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Fix hashtable memory leak

Jonathan Wakely-3
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).



Reply | Threaded
Open this post in threaded view
|

Re: Fix hashtable memory leak

Jonathan Wakely-3
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


Reply | Threaded
Open this post in threaded view
|

Re: Fix hashtable memory leak

François Dumont-2
In reply to this post by François Dumont-2
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  <[hidden email]>

     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


hashtable_mem_leak.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Fix hashtable memory leak

Jonathan Wakely-3
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!