Fix Hashtable node manipulation when custom pointer

I wonder if this fix is correct because if it is you spent more time 
making ext_ptr.cc works than fixing it :-)

     libstdc++ Hashtable: Fix node manipulation when using custom pointer


     Use std::__to_address to get NodeHandle when reinserting nodes.

             * include/bits/hashtable.h 
(_Hashtable<>::_M_reinsert_node): Use
             std::__to_address to access node pointer.
             (_Hashtable<>::_M_reinsert_node_multi): Likewise.
             (_Hashtable<>::_M_merge_unique): Likewise.
             * 
testsuite/23_containers/unordered_set/allocator/ext_ptr.cc: Run in
             C++11 and higher.
             * 
testsuite/23_containers/unordered_multiset/allocator/ext_ptr.cc: New.

It only impacts a feature not released so far so it is not a regression 
but we could perhaps fix it now, no ?

François

On 12/03/20 19:33 +0100, François Dumont via Libstdc++ wrote:
>I wonder if this fix is correct because if it is you spent more time 
>making ext_ptr.cc works than fixing it :-)

I don't remember the details, but I think this is not correct. We need
to store the fancy pointer, not a raw pointer. Simply extracting a raw
pointer from the allocator's pointer type effectively means we don't
work with fancy pointer types.

So this makes it compile, but doesn't make it correct.

The status quo is that we don't support fancy pointers in any of our
node-based containers, but that's a bug that needs to be fixed.


>    libstdc++ Hashtable: Fix node manipulation when using custom pointer
>
>
>    Use std::__to_address to get NodeHandle when reinserting nodes.
>
>            * include/bits/hashtable.h 
>(_Hashtable<>::_M_reinsert_node): Use
>            std::__to_address to access node pointer.
>            (_Hashtable<>::_M_reinsert_node_multi): Likewise.
>            (_Hashtable<>::_M_merge_unique): Likewise.
>            * 
>testsuite/23_containers/unordered_set/allocator/ext_ptr.cc: Run in
>            C++11 and higher.
>            * 
>testsuite/23_containers/unordered_multiset/allocator/ext_ptr.cc: New.
>
>It only impacts a feature not released so far so it is not a 

This code has been released since GCC 7.1.0, three years ago.

>regression but we could perhaps fix it now, no ?
>
>François
>

>diff --git a/libstdc++-v3/include/bits/hashtable.h b/libstdc++-v3/include/bits/hashtable.h
>index b00319a668b..cbcbacef3b6 100644
>--- a/libstdc++-v3/include/bits/hashtable.h
>+++ b/libstdc++-v3/include/bits/hashtable.h
>@@ -847,7 +847,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> 	    else
> 	      {
> 		__ret.position
>-		  = _M_insert_unique_node(__k, __bkt, __code, __nh._M_ptr);
>+		  = _M_insert_unique_node(__k, __bkt, __code,
>+					  std::__to_address(__nh._M_ptr));
> 		__nh._M_ptr = nullptr;
> 		__ret.inserted = true;
> 	      }
>@@ -867,7 +868,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> 	const key_type& __k = __nh._M_key();
> 	auto __code = this->_M_hash_code(__k);
> 	auto __ret
>-	  = _M_insert_multi_node(__hint._M_cur, __k, __code, __nh._M_ptr);
>+	  = _M_insert_multi_node(__hint._M_cur, __k, __code,
>+				 std::__to_address(__nh._M_ptr));
> 	__nh._M_ptr = nullptr;
> 	return __ret;
>       }
>@@ -934,7 +936,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> 	      if (_M_find_node(__bkt, __k, __code) == nullptr)
> 		{
> 		  auto __nh = __src.extract(__pos);
>-		  _M_insert_unique_node(__k, __bkt, __code, __nh._M_ptr,
>+		  _M_insert_unique_node(__k, __bkt, __code,
>+					std::__to_address(__nh._M_ptr),
> 					__n_elt);
> 		  __nh._M_ptr = nullptr;
> 		  __n_elt = 1;
>diff --git a/libstdc++-v3/testsuite/23_containers/unordered_multiset/allocator/ext_ptr.cc b/libstdc++-v3/testsuite/23_containers/unordered_multiset/allocator/ext_ptr.cc
>new file mode 100644
>index 00000000000..3a2538eeb31
>--- /dev/null
>+++ b/libstdc++-v3/testsuite/23_containers/unordered_multiset/allocator/ext_ptr.cc
>@@ -0,0 +1,52 @@
>+// Copyright (C) 2020 Free Software Foundation, Inc.
>+//
>+// This file is part of the GNU ISO C++ Library.  This library is free
>+// software; you can redistribute it and/or modify it under the
>+// terms of the GNU General Public License as published by the
>+// Free Software Foundation; either version 3, or (at your option)
>+// any later version.
>+
>+// This library is distributed in the hope that it will be useful,
>+// but WITHOUT ANY WARRANTY; without even the implied warranty of
>+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>+// GNU General Public License for more details.
>+
>+// You should have received a copy of the GNU General Public License along
>+// with this library; see the file COPYING3.  If not see
>+// <http://www.gnu.org/licenses/>.
>+
>+// { dg-do run { target { c++11 } } }
>+
>+#include <unordered_set>
>+#include <memory>
>+#include <testsuite_hooks.h>
>+#include <testsuite_allocator.h>
>+
>+struct T { int i; };
>+bool operator==(const T& l, const T& r) { return l.i == r.i; }
>+struct H
>+{ std::size_t operator()(const T& t) const noexcept { return t.i; } };
>+struct E : std::equal_to<T> { };
>+
>+using __gnu_test::CustomPointerAlloc;
>+
>+template class std::unordered_multiset<T, H, E, CustomPointerAlloc<T>>;
>+
>+void test01()
>+{
>+  typedef CustomPointerAlloc<T> alloc_type;
>+  typedef std::unordered_set<T, H, E, alloc_type> test_type;
>+  test_type v;
>+  v.insert(T());
>+  VERIFY( ++v.begin() == v.end() );
>+
>+  test_type v2 = v;
>+  v2.insert(T{1});
>+  v.merge(v2);
>+  VERIFY( v.size() == 2 );
>+}
>+
>+int main()
>+{
>+  test01();
>+}
>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 f6b908ac03e..9f1c1b2132c 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
>@@ -15,10 +15,7 @@
> // with this library; see the file COPYING3.  If not see
> // <http://www.gnu.org/licenses/>.
> 
>-// This test fails to compile since C++17 (see xfail-if below) so we can only
>-// do a "run" test for C++11 and C++14, and a "compile" test for C++17 and up.
>-// { dg-do run { target { c++11_only || c++14_only } } }
>-// { dg-do compile { target c++17 } }
>+// { dg-do run { target { c++11 } } }
> 
> #include <unordered_set>
> #include <memory>
>@@ -27,14 +24,12 @@
> 
> struct T { int i; };
> bool operator==(const T& l, const T& r) { return l.i == r.i; }
>-struct H { std::size_t operator()(const T& t) const noexcept { return t.i; }
>-};
>+struct H
>+{ std::size_t operator()(const T& t) const noexcept { return t.i; } };
> struct E : std::equal_to<T> { };
> 
> using __gnu_test::CustomPointerAlloc;
> 
>-// { dg-xfail-if "node reinsertion assumes raw pointers" { c++17 } }
>-// TODO when removing this xfail change the test back to "dg-do run".
> template class std::unordered_set<T, H, E, CustomPointerAlloc<T>>;
> 
> void test01()
>@@ -44,6 +39,11 @@ void test01()
>   test_type v;
>   v.insert(T());
>   VERIFY( ++v.begin() == v.end() );
>+
>+  test_type v2 = v;
>+  v2.insert(T{1});
>+  v.merge(v2);
>+  VERIFY( v.size() == 2 );
> }
> 
> int main()