diff mbox series

Fix hashtable node deallocation

Message ID ad43e63d-3626-116e-d34e-dde6d4e708c9@gmail.com
State New
Headers show
Series Fix hashtable node deallocation | expand

Commit Message

François Dumont Nov. 10, 2018, 9:40 p.m. UTC
While working on a hashtable enhancement I noticed that we are not using 
the correct method to deallocate node if the constructor throws in 
_ReuseOrAllocNode operator(). I had to introduce a new 
_M_deallocate_node_ptr for that as node value shall not be destroy again.

I also check other places and noticed that a __node_type destructor call 
was missing.

     * include/bits/hashtable_policy.h
(_Hashtable_alloc<>::_M_deallocate_node_ptr(__node_type*)): New.
     (_Hashtable_alloc<>::_M_deallocate_node(__node_type*)): Use latter.
(_ReuseOrAllocNode<>::operator<_Arg>()(_Arg&&)): Likewise.
     (_Hashtable_alloc<>::_M_allocate_node): Add ~__node_type call.

Tested under Linux x86_64.

Ok to commit ?

François

Comments

François Dumont Nov. 17, 2018, 9:01 p.m. UTC | #1
Here is the same patch but this time with a test change which is 
supposed to show the problem.

However it doesn't because of:
       _Pointer_adapter(element_type* __arg = 0)
       { _Storage_policy::set(__arg); }

which is not explicit.

So is this patch really necessary ? If it isn't, is usage of 
pointer_traits<>::pointer_to really necessary ?

Note that I also found a bug in the 
__gnu_test::CustomPointerAlloc::allocate, the signature with hint is wrong.

     * include/ext/throw_allocator.h
     (annotate_base::insert(void*, size_t)): Use insert result to check for
     double insert attempt.
     (annotate_base::insert_construct(void*)): Likewise.
     (annotate_base::check_allocated(void*, size_t)): Return found iterator.
     (annotate_base::erase(void*, size_t)): Use latter method returned
     iterator.
     (annotate_base::check_constructed(void*, size_t)): Return found 
iterator.
     (annotate_base::erase_construct(void*)): Use latter method returned
     iterator.
     * libstdc++-v3/testsuite/util/testsuite_allocator.h
     (CustomPointerAlloc<>::allocate(size_t, pointer)): Replace by...
     (CustomPointerAlloc<>::allocate(size_t, const_void_pointer)): ...this.
     * testsuite/23_containers/unordered_set/allocator/ext_ptr.cc: Add
     check.

François


On 11/10/18 10:40 PM, François Dumont wrote:
> While working on a hashtable enhancement I noticed that we are not 
> using the correct method to deallocate node if the constructor throws 
> in _ReuseOrAllocNode operator(). I had to introduce a new 
> _M_deallocate_node_ptr for that as node value shall not be destroy again.
>
> I also check other places and noticed that a __node_type destructor 
> call was missing.
>
>     * include/bits/hashtable_policy.h
> (_Hashtable_alloc<>::_M_deallocate_node_ptr(__node_type*)): New.
>     (_Hashtable_alloc<>::_M_deallocate_node(__node_type*)): Use latter.
> (_ReuseOrAllocNode<>::operator<_Arg>()(_Arg&&)): Likewise.
>     (_Hashtable_alloc<>::_M_allocate_node): Add ~__node_type call.
>
> Tested under Linux x86_64.
>
> Ok to commit ?
>
> François
>
diff --git a/libstdc++-v3/include/bits/hashtable_policy.h b/libstdc++-v3/include/bits/hashtable_policy.h
index d8e8c13858d..c87c65fd9f7 100644
--- a/libstdc++-v3/include/bits/hashtable_policy.h
+++ b/libstdc++-v3/include/bits/hashtable_policy.h
@@ -148,8 +148,7 @@ namespace __detail
 		}
 	      __catch(...)
 		{
-		  __node->~__node_type();
-		  __node_alloc_traits::deallocate(__a, __node, 1);
+		  _M_h._M_deallocate_node_ptr(__node);
 		  __throw_exception_again;
 		}
 	      return __node;
@@ -2116,6 +2115,9 @@ namespace __detail
       void
       _M_deallocate_node(__node_type* __n);
 
+      void
+      _M_deallocate_node_ptr(__node_type* __n);
+
       // Deallocate the linked list of nodes pointed to by __n
       void
       _M_deallocate_nodes(__node_type* __n);
@@ -2146,6 +2148,7 @@ namespace __detail
 	  }
 	__catch(...)
 	  {
+	    __n->~__node_type();
 	    __node_alloc_traits::deallocate(_M_node_allocator(), __nptr, 1);
 	    __throw_exception_again;
 	  }
@@ -2154,10 +2157,17 @@ namespace __detail
   template<typename _NodeAlloc>
     void
     _Hashtable_alloc<_NodeAlloc>::_M_deallocate_node(__node_type* __n)
+    {
+      __node_alloc_traits::destroy(_M_node_allocator(), __n->_M_valptr());
+      _M_deallocate_node_ptr(__n);
+    }
+
+  template<typename _NodeAlloc>
+    void
+    _Hashtable_alloc<_NodeAlloc>::_M_deallocate_node_ptr(__node_type* __n)
     {
       typedef typename __node_alloc_traits::pointer _Ptr;
       auto __ptr = std::pointer_traits<_Ptr>::pointer_to(*__n);
-      __node_alloc_traits::destroy(_M_node_allocator(), __n->_M_valptr());
       __n->~__node_type();
       __node_alloc_traits::deallocate(_M_node_allocator(), __ptr, 1);
     }
diff --git a/libstdc++-v3/testsuite/23_containers/unordered_set/allocator/ext_ptr.cc b/libstdc++-v3/testsuite/23_containers/unordered_set/allocator/ext_ptr.cc
index 7e4a6e02900..3b2a9a1ab35 100644
--- a/libstdc++-v3/testsuite/23_containers/unordered_set/allocator/ext_ptr.cc
+++ b/libstdc++-v3/testsuite/23_containers/unordered_set/allocator/ext_ptr.cc
@@ -41,6 +41,9 @@ void test01()
   test_type v;
   v.insert(T());
   VERIFY( ++v.begin() == v.end() );
+
+  v = { { 1 }, { 2 }, { 3 }};
+  VERIFY( v.size() == 3 );
 }
 
 int main()
diff --git a/libstdc++-v3/testsuite/util/testsuite_allocator.h b/libstdc++-v3/testsuite/util/testsuite_allocator.h
index b0fecfb59a3..c18223475c9 100644
--- a/libstdc++-v3/testsuite/util/testsuite_allocator.h
+++ b/libstdc++-v3/testsuite/util/testsuite_allocator.h
@@ -582,7 +582,7 @@ namespace __gnu_test
       typedef Ptr<void>		void_pointer;
       typedef Ptr<const void>	const_void_pointer;
 
-      pointer allocate(std::size_t n, pointer = {})
+      pointer allocate(std::size_t n, const_void_pointer = {})
       { return pointer(std::allocator<Tp>::allocate(n)); }
 
       void deallocate(pointer p, std::size_t n)
Jonathan Wakely Nov. 19, 2018, 12:33 p.m. UTC | #2
On 17/11/18 22:01 +0100, François Dumont wrote:
>Here is the same patch but this time with a test change which is 
>supposed to show the problem.
>
>However it doesn't because of:
>      _Pointer_adapter(element_type* __arg = 0)
>      { _Storage_policy::set(__arg); }
>
>which is not explicit.
>
>So is this patch really necessary ? If it isn't, is usage of 
>pointer_traits<>::pointer_to really necessary ?

Yes. Just because our _Pointer_adapter allows implicit conversions
from raw pointers doesn't mean all fancy pointers allow that.

>Note that I also found a bug in the 
>__gnu_test::CustomPointerAlloc::allocate, the signature with hint is 
>wrong.

Yes, that's a bug, thanks.

>    * include/ext/throw_allocator.h
>    (annotate_base::insert(void*, size_t)): Use insert result to check for
>    double insert attempt.
>    (annotate_base::insert_construct(void*)): Likewise.
>    (annotate_base::check_allocated(void*, size_t)): Return found iterator.
>    (annotate_base::erase(void*, size_t)): Use latter method returned
>    iterator.
>    (annotate_base::check_constructed(void*, size_t)): Return found 
>iterator.
>    (annotate_base::erase_construct(void*)): Use latter method returned
>    iterator.

This looks like the wrong ChangeLog.


>    * libstdc++-v3/testsuite/util/testsuite_allocator.h
>    (CustomPointerAlloc<>::allocate(size_t, pointer)): Replace by...
>    (CustomPointerAlloc<>::allocate(size_t, const_void_pointer)): ...this.
>    * testsuite/23_containers/unordered_set/allocator/ext_ptr.cc: Add
>    check.
Jonathan Wakely Nov. 19, 2018, 12:34 p.m. UTC | #3
On 10/11/18 22:40 +0100, François Dumont wrote:
>While working on a hashtable enhancement I noticed that we are not 
>using the correct method to deallocate node if the constructor throws 
>in _ReuseOrAllocNode operator(). I had to introduce a new 
>_M_deallocate_node_ptr for that as node value shall not be destroy 
>again.
>
>I also check other places and noticed that a __node_type destructor 
>call was missing.

That's intentional. The type has a trivial destructor, so its storage
can just be reused, we don't need to destroy it.
François Dumont Nov. 19, 2018, 9:19 p.m. UTC | #4
On 11/19/18 1:34 PM, Jonathan Wakely wrote:
> On 10/11/18 22:40 +0100, François Dumont wrote:
>> While working on a hashtable enhancement I noticed that we are not 
>> using the correct method to deallocate node if the constructor throws 
>> in _ReuseOrAllocNode operator(). I had to introduce a new 
>> _M_deallocate_node_ptr for that as node value shall not be destroy 
>> again.
>>
>> I also check other places and noticed that a __node_type destructor 
>> call was missing.
>
> That's intentional. The type has a trivial destructor, so its storage
> can just be reused, we don't need to destroy it.
>
>
Ok, do you want to also remove the other call to ~__node_type() then ?

Here is the updated patch and the right ChangeLog entry:

     * include/bits/hashtable_policy.h
(_Hashtable_alloc<>::_M_deallocate_node_ptr(__node_type*)): New.
     (_Hashtable_alloc<>::_M_deallocate_node(__node_type*)): Use latter.
(_ReuseOrAllocNode<>::operator<_Arg>()(_Arg&&)): Likewise.
     (_Hashtable_alloc<>::_M_allocate_node): Add ~__node_type call.
     * libstdc++-v3/testsuite/util/testsuite_allocator.h
     (CustomPointerAlloc<>::allocate(size_t, pointer)): Replace by...
     (CustomPointerAlloc<>::allocate(size_t, const_void_pointer)): ...this.
     * testsuite/23_containers/unordered_set/allocator/ext_ptr.cc: Add
     check.

Ok to commit ?

François
diff --git a/libstdc++-v3/include/bits/hashtable_policy.h b/libstdc++-v3/include/bits/hashtable_policy.h
index d8e8c13858d..acc6f41a0ed 100644
--- a/libstdc++-v3/include/bits/hashtable_policy.h
+++ b/libstdc++-v3/include/bits/hashtable_policy.h
@@ -148,8 +148,7 @@ namespace __detail
 		}
 	      __catch(...)
 		{
-		  __node->~__node_type();
-		  __node_alloc_traits::deallocate(__a, __node, 1);
+		  _M_h._M_deallocate_node_ptr(__node);
 		  __throw_exception_again;
 		}
 	      return __node;
@@ -2116,6 +2115,9 @@ namespace __detail
       void
       _M_deallocate_node(__node_type* __n);
 
+      void
+      _M_deallocate_node_ptr(__node_type* __n);
+
       // Deallocate the linked list of nodes pointed to by __n
       void
       _M_deallocate_nodes(__node_type* __n);
@@ -2154,10 +2156,17 @@ namespace __detail
   template<typename _NodeAlloc>
     void
     _Hashtable_alloc<_NodeAlloc>::_M_deallocate_node(__node_type* __n)
+    {
+      __node_alloc_traits::destroy(_M_node_allocator(), __n->_M_valptr());
+      _M_deallocate_node_ptr(__n);
+    }
+
+  template<typename _NodeAlloc>
+    void
+    _Hashtable_alloc<_NodeAlloc>::_M_deallocate_node_ptr(__node_type* __n)
     {
       typedef typename __node_alloc_traits::pointer _Ptr;
       auto __ptr = std::pointer_traits<_Ptr>::pointer_to(*__n);
-      __node_alloc_traits::destroy(_M_node_allocator(), __n->_M_valptr());
       __n->~__node_type();
       __node_alloc_traits::deallocate(_M_node_allocator(), __ptr, 1);
     }
diff --git a/libstdc++-v3/testsuite/23_containers/unordered_set/allocator/ext_ptr.cc b/libstdc++-v3/testsuite/23_containers/unordered_set/allocator/ext_ptr.cc
index 7e4a6e02900..3b2a9a1ab35 100644
--- a/libstdc++-v3/testsuite/23_containers/unordered_set/allocator/ext_ptr.cc
+++ b/libstdc++-v3/testsuite/23_containers/unordered_set/allocator/ext_ptr.cc
@@ -41,6 +41,9 @@ void test01()
   test_type v;
   v.insert(T());
   VERIFY( ++v.begin() == v.end() );
+
+  v = { { 1 }, { 2 }, { 3 }};
+  VERIFY( v.size() == 3 );
 }
 
 int main()
diff --git a/libstdc++-v3/testsuite/util/testsuite_allocator.h b/libstdc++-v3/testsuite/util/testsuite_allocator.h
index b0fecfb59a3..c18223475c9 100644
--- a/libstdc++-v3/testsuite/util/testsuite_allocator.h
+++ b/libstdc++-v3/testsuite/util/testsuite_allocator.h
@@ -582,7 +582,7 @@ namespace __gnu_test
       typedef Ptr<void>		void_pointer;
       typedef Ptr<const void>	const_void_pointer;
 
-      pointer allocate(std::size_t n, pointer = {})
+      pointer allocate(std::size_t n, const_void_pointer = {})
       { return pointer(std::allocator<Tp>::allocate(n)); }
 
       void deallocate(pointer p, std::size_t n)
François Dumont Nov. 29, 2018, 6:08 a.m. UTC | #5
I am unclear about this patch, is it accepted ?


On 11/19/18 10:19 PM, François Dumont wrote:
> On 11/19/18 1:34 PM, Jonathan Wakely wrote:
>> On 10/11/18 22:40 +0100, François Dumont wrote:
>>> While working on a hashtable enhancement I noticed that we are not 
>>> using the correct method to deallocate node if the constructor 
>>> throws in _ReuseOrAllocNode operator(). I had to introduce a new 
>>> _M_deallocate_node_ptr for that as node value shall not be destroy 
>>> again.
>>>
>>> I also check other places and noticed that a __node_type destructor 
>>> call was missing.
>>
>> That's intentional. The type has a trivial destructor, so its storage
>> can just be reused, we don't need to destroy it.
>>
>>
> Ok, do you want to also remove the other call to ~__node_type() then ?
>
> Here is the updated patch and the right ChangeLog entry:
>
>     * include/bits/hashtable_policy.h
> (_Hashtable_alloc<>::_M_deallocate_node_ptr(__node_type*)): New.
>     (_Hashtable_alloc<>::_M_deallocate_node(__node_type*)): Use latter.
> (_ReuseOrAllocNode<>::operator<_Arg>()(_Arg&&)): Likewise.
>     (_Hashtable_alloc<>::_M_allocate_node): Add ~__node_type call.
>     * libstdc++-v3/testsuite/util/testsuite_allocator.h
>     (CustomPointerAlloc<>::allocate(size_t, pointer)): Replace by...
>     (CustomPointerAlloc<>::allocate(size_t, const_void_pointer)): 
> ...this.
>     * testsuite/23_containers/unordered_set/allocator/ext_ptr.cc: Add
>     check.
>
> Ok to commit ?
>
> François
>
François Dumont Dec. 16, 2018, 1:16 p.m. UTC | #6
Gentle reminder, we still have this issue pending.

     * include/bits/hashtable_policy.h
(_Hashtable_alloc<>::_M_deallocate_node_ptr(__node_type*)): New.
     (_Hashtable_alloc<>::_M_deallocate_node(__node_type*)): Use latter.
(_ReuseOrAllocNode<>::operator<_Arg>()(_Arg&&)): Likewise.
     * libstdc++-v3/testsuite/util/testsuite_allocator.h
     (CustomPointerAlloc<>::allocate(size_t, pointer)): Replace by...
     (CustomPointerAlloc<>::allocate(size_t, const_void_pointer)): ...this.

François

On 11/29/18 7:08 AM, François Dumont wrote:
> I am unclear about this patch, is it accepted ?
>
>
> On 11/19/18 10:19 PM, François Dumont wrote:
>> On 11/19/18 1:34 PM, Jonathan Wakely wrote:
>>> On 10/11/18 22:40 +0100, François Dumont wrote:
>>>> While working on a hashtable enhancement I noticed that we are not 
>>>> using the correct method to deallocate node if the constructor 
>>>> throws in _ReuseOrAllocNode operator(). I had to introduce a new 
>>>> _M_deallocate_node_ptr for that as node value shall not be destroy 
>>>> again.
>>>>
>>>> I also check other places and noticed that a __node_type destructor 
>>>> call was missing.
>>>
>>> That's intentional. The type has a trivial destructor, so its storage
>>> can just be reused, we don't need to destroy it.
>>>
>>>
>> Ok, do you want to also remove the other call to ~__node_type() then ?
>>
>> Here is the updated patch and the right ChangeLog entry:
>>
>>     * include/bits/hashtable_policy.h
>> (_Hashtable_alloc<>::_M_deallocate_node_ptr(__node_type*)): New.
>> (_Hashtable_alloc<>::_M_deallocate_node(__node_type*)): Use latter.
>> (_ReuseOrAllocNode<>::operator<_Arg>()(_Arg&&)): Likewise.
>>     (_Hashtable_alloc<>::_M_allocate_node): Add ~__node_type call.
>>     * libstdc++-v3/testsuite/util/testsuite_allocator.h
>>     (CustomPointerAlloc<>::allocate(size_t, pointer)): Replace by...
>>     (CustomPointerAlloc<>::allocate(size_t, const_void_pointer)): 
>> ...this.
>>     * testsuite/23_containers/unordered_set/allocator/ext_ptr.cc: Add
>>     check.
>>
>> Ok to commit ?
>>
>> François
>>
>
diff --git a/libstdc++-v3/include/bits/hashtable_policy.h b/libstdc++-v3/include/bits/hashtable_policy.h
index b843c955797..aa4808eab31 100644
--- a/libstdc++-v3/include/bits/hashtable_policy.h
+++ b/libstdc++-v3/include/bits/hashtable_policy.h
@@ -148,8 +148,7 @@ namespace __detail
 		}
 	      __catch(...)
 		{
-		  __node->~__node_type();
-		  __node_alloc_traits::deallocate(__a, __node, 1);
+		  _M_h._M_deallocate_node_ptr(__node);
 		  __throw_exception_again;
 		}
 	      return __node;
@@ -2116,6 +2115,9 @@ namespace __detail
       void
       _M_deallocate_node(__node_type* __n);
 
+      void
+      _M_deallocate_node_ptr(__node_type* __n);
+
       // Deallocate the linked list of nodes pointed to by __n
       void
       _M_deallocate_nodes(__node_type* __n);
@@ -2154,10 +2156,17 @@ namespace __detail
   template<typename _NodeAlloc>
     void
     _Hashtable_alloc<_NodeAlloc>::_M_deallocate_node(__node_type* __n)
+    {
+      __node_alloc_traits::destroy(_M_node_allocator(), __n->_M_valptr());
+      _M_deallocate_node_ptr(__n);
+    }
+
+  template<typename _NodeAlloc>
+    void
+    _Hashtable_alloc<_NodeAlloc>::_M_deallocate_node_ptr(__node_type* __n)
     {
       typedef typename __node_alloc_traits::pointer _Ptr;
       auto __ptr = std::pointer_traits<_Ptr>::pointer_to(*__n);
-      __node_alloc_traits::destroy(_M_node_allocator(), __n->_M_valptr());
       __n->~__node_type();
       __node_alloc_traits::deallocate(_M_node_allocator(), __ptr, 1);
     }
diff --git a/libstdc++-v3/testsuite/util/testsuite_allocator.h b/libstdc++-v3/testsuite/util/testsuite_allocator.h
index b0fecfb59a3..c18223475c9 100644
--- a/libstdc++-v3/testsuite/util/testsuite_allocator.h
+++ b/libstdc++-v3/testsuite/util/testsuite_allocator.h
@@ -582,7 +582,7 @@ namespace __gnu_test
       typedef Ptr<void>		void_pointer;
       typedef Ptr<const void>	const_void_pointer;
 
-      pointer allocate(std::size_t n, pointer = {})
+      pointer allocate(std::size_t n, const_void_pointer = {})
       { return pointer(std::allocator<Tp>::allocate(n)); }
 
       void deallocate(pointer p, std::size_t n)
François Dumont Dec. 21, 2018, 5:41 p.m. UTC | #7
Still waiting for this (I hope) last fix before gcc 9 release...

We could perhaps make:
       _Pointer_adapter(element_type* __arg = 0)

explicit to find out if there are other places where we miss to properly 
call pointer_traits<>::pointer_to. But I aren't sure we can do it in 
this extension and I haven't tried yet.

François

On 12/16/18 2:16 PM, François Dumont wrote:
> Gentle reminder, we still have this issue pending.
>
>     * include/bits/hashtable_policy.h
> (_Hashtable_alloc<>::_M_deallocate_node_ptr(__node_type*)): New.
>     (_Hashtable_alloc<>::_M_deallocate_node(__node_type*)): Use latter.
> (_ReuseOrAllocNode<>::operator<_Arg>()(_Arg&&)): Likewise.
>     * libstdc++-v3/testsuite/util/testsuite_allocator.h
>     (CustomPointerAlloc<>::allocate(size_t, pointer)): Replace by...
>     (CustomPointerAlloc<>::allocate(size_t, const_void_pointer)): 
> ...this.
>
> François
>
> On 11/29/18 7:08 AM, François Dumont wrote:
>> I am unclear about this patch, is it accepted ?
>>
>>
>> On 11/19/18 10:19 PM, François Dumont wrote:
>>> On 11/19/18 1:34 PM, Jonathan Wakely wrote:
>>>> On 10/11/18 22:40 +0100, François Dumont wrote:
>>>>> While working on a hashtable enhancement I noticed that we are not 
>>>>> using the correct method to deallocate node if the constructor 
>>>>> throws in _ReuseOrAllocNode operator(). I had to introduce a new 
>>>>> _M_deallocate_node_ptr for that as node value shall not be destroy 
>>>>> again.
>>>>>
>>>>> I also check other places and noticed that a __node_type 
>>>>> destructor call was missing.
>>>>
>>>> That's intentional. The type has a trivial destructor, so its storage
>>>> can just be reused, we don't need to destroy it.
>>>>
>>>>
>>> Ok, do you want to also remove the other call to ~__node_type() then ?
>>>
>>> Here is the updated patch and the right ChangeLog entry:
>>>
>>>     * include/bits/hashtable_policy.h
>>> (_Hashtable_alloc<>::_M_deallocate_node_ptr(__node_type*)): New.
>>> (_Hashtable_alloc<>::_M_deallocate_node(__node_type*)): Use latter.
>>> (_ReuseOrAllocNode<>::operator<_Arg>()(_Arg&&)): Likewise.
>>>     (_Hashtable_alloc<>::_M_allocate_node): Add ~__node_type call.
>>>     * libstdc++-v3/testsuite/util/testsuite_allocator.h
>>>     (CustomPointerAlloc<>::allocate(size_t, pointer)): Replace by...
>>>     (CustomPointerAlloc<>::allocate(size_t, const_void_pointer)): 
>>> ...this.
>>>     * testsuite/23_containers/unordered_set/allocator/ext_ptr.cc: Add
>>>     check.
>>>
>>> Ok to commit ?
>>>
>>> François
>>>
>>
>
Jonathan Wakely Dec. 21, 2018, 8:29 p.m. UTC | #8
On 16/12/18 14:16 +0100, François Dumont wrote:
>Gentle reminder, we still have this issue pending.
>
>    * include/bits/hashtable_policy.h
>(_Hashtable_alloc<>::_M_deallocate_node_ptr(__node_type*)): New.
>    (_Hashtable_alloc<>::_M_deallocate_node(__node_type*)): Use latter.
>(_ReuseOrAllocNode<>::operator<_Arg>()(_Arg&&)): Likewise.

Please add more detail to the commit message explaining the problem.
Either as a paragraph of text in the commit message before the
changelog (e.g. see https://gcc.gnu.org/r267236 or
https://gcc.gnu.org/r267276 for commits with additional text in the
commit message), or in the changelog itself, e.g.

        (_ReuseOrAllocNode<>::operator<_Arg>()(_Arg&&)): Likewise, so
        that the argument to __node_alloc_traits::deallocate is the
        correct pointer type.

>    * libstdc++-v3/testsuite/util/testsuite_allocator.h
>    (CustomPointerAlloc<>::allocate(size_t, pointer)): Replace by...
>    (CustomPointerAlloc<>::allocate(size_t, const_void_pointer)): ...this.

This should have been a separate commit really.

OK for trunk with a better commit message that explains what the
change does.
François Dumont Dec. 23, 2018, 6:17 p.m. UTC | #9
On 12/21/18 9:29 PM, Jonathan Wakely wrote:
> On 16/12/18 14:16 +0100, François Dumont wrote:
>> Gentle reminder, we still have this issue pending.
>>
>>     * include/bits/hashtable_policy.h
>> (_Hashtable_alloc<>::_M_deallocate_node_ptr(__node_type*)): New.
>> (_Hashtable_alloc<>::_M_deallocate_node(__node_type*)): Use latter.
>> (_ReuseOrAllocNode<>::operator<_Arg>()(_Arg&&)): Likewise.
>
> Please add more detail to the commit message explaining the problem.
> Either as a paragraph of text in the commit message before the
> changelog (e.g. see https://gcc.gnu.org/r267236 or
> https://gcc.gnu.org/r267276 for commits with additional text in the
> commit message), or in the changelog itself, e.g.
>
> (_ReuseOrAllocNode<>::operator<_Arg>()(_Arg&&)): Likewise, so
>        that the argument to __node_alloc_traits::deallocate is the
>        correct pointer type.
>
>>     * libstdc++-v3/testsuite/util/testsuite_allocator.h
>>     (CustomPointerAlloc<>::allocate(size_t, pointer)): Replace by...
>>     (CustomPointerAlloc<>::allocate(size_t, const_void_pointer)): 
>> ...this.
>
> This should have been a separate commit really.
>
> OK for trunk with a better commit message that explains what the
> change does.
>
>
Committed in 2 different commits.

I hope you will appreciate my additional message, I didn't notice yours 
before writting mine.

François
diff mbox series

Patch

diff --git a/libstdc++-v3/include/bits/hashtable_policy.h b/libstdc++-v3/include/bits/hashtable_policy.h
index d8e8c13858d..c87c65fd9f7 100644
--- a/libstdc++-v3/include/bits/hashtable_policy.h
+++ b/libstdc++-v3/include/bits/hashtable_policy.h
@@ -148,8 +148,7 @@  namespace __detail
 		}
 	      __catch(...)
 		{
-		  __node->~__node_type();
-		  __node_alloc_traits::deallocate(__a, __node, 1);
+		  _M_h._M_deallocate_node_ptr(__node);
 		  __throw_exception_again;
 		}
 	      return __node;
@@ -2116,6 +2115,9 @@  namespace __detail
       void
       _M_deallocate_node(__node_type* __n);
 
+      void
+      _M_deallocate_node_ptr(__node_type* __n);
+
       // Deallocate the linked list of nodes pointed to by __n
       void
       _M_deallocate_nodes(__node_type* __n);
@@ -2146,6 +2148,7 @@  namespace __detail
 	  }
 	__catch(...)
 	  {
+	    __n->~__node_type();
 	    __node_alloc_traits::deallocate(_M_node_allocator(), __nptr, 1);
 	    __throw_exception_again;
 	  }
@@ -2154,10 +2157,17 @@  namespace __detail
   template<typename _NodeAlloc>
     void
     _Hashtable_alloc<_NodeAlloc>::_M_deallocate_node(__node_type* __n)
+    {
+      __node_alloc_traits::destroy(_M_node_allocator(), __n->_M_valptr());
+      _M_deallocate_node_ptr(__n);
+    }
+
+  template<typename _NodeAlloc>
+    void
+    _Hashtable_alloc<_NodeAlloc>::_M_deallocate_node_ptr(__node_type* __n)
     {
       typedef typename __node_alloc_traits::pointer _Ptr;
       auto __ptr = std::pointer_traits<_Ptr>::pointer_to(*__n);
-      __node_alloc_traits::destroy(_M_node_allocator(), __n->_M_valptr());
       __n->~__node_type();
       __node_alloc_traits::deallocate(_M_node_allocator(), __ptr, 1);
     }