Re: Fix hashtable node deallocation

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

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)