diff mbox series

[committed] libstdc++: Make pmr::synchronized_pool_resource work without libpthread (PR 94936)

Message ID 20200504133634.GA730513@redhat.com
State New
Headers show
Series [committed] libstdc++: Make pmr::synchronized_pool_resource work without libpthread (PR 94936) | expand

Commit Message

Jonathan Wakely May 4, 2020, 1:36 p.m. UTC
I implicitly assumed that programs using pmr::synchronized_pool_resource
would also be using multiple threads, and so the weak symbols in
gthr-posix.h would be resolved by linking to libpthread. If that isn't
true then it crashes when trying to use pthread_key_create.

This commit makes the pool resource check __gthread_active_p() before
using thread-specific data, and just use a single set of memory pools
when there's only a single thread.

	PR libstdc++/94936
	* src/c++17/memory_resource.cc (synchronized_pool_resource::_TPools):
	Add comment about single-threaded behaviour.
	(synchronized_pool_resource::_TPools::move_nonempty_chunks()): Hoist
	class member access out of loop.
	(synchronized_pool_resource::synchronized_pool_resource())
	(synchronized_pool_resource::~synchronized_pool_resource())
	(synchronized_pool_resource::release()): Check __gthread_active_p
	before creating and/or deleting the thread-specific data key.
	(synchronized_pool_resource::_M_thread_specific_pools()): Adjust
	assertions.
	(synchronized_pool_resource::do_allocate(size_t, size_t)): Add fast
	path for single-threaded case.
	(synchronized_pool_resource::do_deallocate(void*, size_t, size_t)):
	Likewise. Return if unable to find a pool that owns the allocation.
	* testsuite/20_util/synchronized_pool_resource/allocate_single.cc:
	New test.
	* testsuite/20_util/synchronized_pool_resource/cons_single.cc: New
	test.
	* testsuite/20_util/synchronized_pool_resource/release_single.cc: New
	test.

Tested powerpc64le-linux, committed to master.

I'll backport this, but after 10.1 is released.
commit ec40967f1323069da3a5a45286f71fa4f80926df
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Mon May 4 13:34:23 2020 +0100

    libstdc++: Make pmr::synchronized_pool_resource work without libpthread (PR 94936)
    
    I implicitly assumed that programs using pmr::synchronized_pool_resource
    would also be using multiple threads, and so the weak symbols in
    gthr-posix.h would be resolved by linking to libpthread. If that isn't
    true then it crashes when trying to use pthread_key_create.
    
    This commit makes the pool resource check __gthread_active_p() before
    using thread-specific data, and just use a single set of memory pools
    when there's only a single thread.
    
            PR libstdc++/94936
            * src/c++17/memory_resource.cc (synchronized_pool_resource::_TPools):
            Add comment about single-threaded behaviour.
            (synchronized_pool_resource::_TPools::move_nonempty_chunks()): Hoist
            class member access out of loop.
            (synchronized_pool_resource::synchronized_pool_resource())
            (synchronized_pool_resource::~synchronized_pool_resource())
            (synchronized_pool_resource::release()): Check __gthread_active_p
            before creating and/or deleting the thread-specific data key.
            (synchronized_pool_resource::_M_thread_specific_pools()): Adjust
            assertions.
            (synchronized_pool_resource::do_allocate(size_t, size_t)): Add fast
            path for single-threaded case.
            (synchronized_pool_resource::do_deallocate(void*, size_t, size_t)):
            Likewise. Return if unable to find a pool that owns the allocation.
            * testsuite/20_util/synchronized_pool_resource/allocate_single.cc:
            New test.
            * testsuite/20_util/synchronized_pool_resource/cons_single.cc: New
            test.
            * testsuite/20_util/synchronized_pool_resource/release_single.cc: New
            test.
diff mbox series

Patch

diff --git a/libstdc++-v3/src/c++17/memory_resource.cc b/libstdc++-v3/src/c++17/memory_resource.cc
index 56a87844da0..1acab19e306 100644
--- a/libstdc++-v3/src/c++17/memory_resource.cc
+++ b/libstdc++-v3/src/c++17/memory_resource.cc
@@ -1043,7 +1043,8 @@  namespace pmr
    * exposition as _M_tpools[_M_key]).
    * The first element, _M_tpools[0], contains "orphaned chunks" which were
    * allocated by a thread which has since exited, and so there is no
-   * _M_tpools[_M_key] for that thread.
+   * _M_tpools[_M_key] for that thread. Orphaned chunks are never reused,
+   * they're only held in _M_tpools[0] so they can be deallocated.
    * A thread can access its own thread-specific set of pools via _M_key
    * while holding a shared lock on _M_mx. Accessing _M_impl._M_unpooled
    * or _M_tpools[0] or any other thread's _M_tpools[_M_key] requires an
@@ -1052,6 +1053,10 @@  namespace pmr
    * any dereference of that pointer requires an exclusive lock.
    * The _M_impl._M_opts and _M_impl._M_npools members are immutable,
    * and can safely be accessed concurrently.
+   *
+   * In a single-threaded program (i.e. __gthread_active_p() == false)
+   * the pool resource only needs one set of pools and never has orphaned
+   * chunks, so just uses _M_tpools[0] directly, and _M_tpools->next is null.
    */
 
   extern "C" {
@@ -1092,14 +1097,16 @@  namespace pmr
     void move_nonempty_chunks()
     {
       __glibcxx_assert(pools);
+      __glibcxx_assert(__gthread_active_p());
       if (!pools)
 	return;
-      memory_resource* r = owner.upstream_resource();
+      memory_resource* const r = owner.upstream_resource();
+      auto* const shared = owner._M_tpools->pools;
       // move all non-empty chunks to the shared _TPools
       for (int i = 0; i < owner._M_impl._M_npools; ++i)
 	for (auto& c : pools[i]._M_chunks)
 	  if (!c.empty())
-	    owner._M_tpools->pools[i]._M_chunks.insert(std::move(c), r);
+	    shared[i]._M_chunks.insert(std::move(c), r);
     }
 
     synchronized_pool_resource& owner;
@@ -1133,8 +1140,9 @@  namespace pmr
 			     memory_resource* upstream)
   : _M_impl(opts, upstream)
   {
-    if (int err = __gthread_key_create(&_M_key, destroy_TPools))
-      __throw_system_error(err);
+    if (__gthread_active_p())
+      if (int err = __gthread_key_create(&_M_key, destroy_TPools))
+	__throw_system_error(err);
     exclusive_lock l(_M_mx);
     _M_tpools = _M_alloc_shared_tpools(l);
   }
@@ -1143,7 +1151,8 @@  namespace pmr
   synchronized_pool_resource::~synchronized_pool_resource()
   {
     release();
-    __gthread_key_delete(_M_key); // does not run destroy_TPools
+    if (__gthread_active_p())
+      __gthread_key_delete(_M_key); // does not run destroy_TPools
   }
 
   void
@@ -1152,8 +1161,11 @@  namespace pmr
     exclusive_lock l(_M_mx);
     if (_M_tpools)
       {
-	__gthread_key_delete(_M_key); // does not run destroy_TPools
-	__gthread_key_create(&_M_key, destroy_TPools);
+	if (__gthread_active_p())
+	  {
+	    __gthread_key_delete(_M_key); // does not run destroy_TPools
+	    __gthread_key_create(&_M_key, destroy_TPools);
+	  }
 	polymorphic_allocator<_TPools> a(upstream_resource());
 	// destroy+deallocate each _TPools
 	do
@@ -1175,10 +1187,11 @@  namespace pmr
   synchronized_pool_resource::_M_thread_specific_pools() noexcept
   {
     __pool_resource::_Pool* pools = nullptr;
+    __glibcxx_assert(__gthread_active_p());
     if (auto tp = static_cast<_TPools*>(__gthread_getspecific(_M_key)))
       {
-      pools = tp->pools;
-      __glibcxx_assert(tp->pools);
+	pools = tp->pools;
+	// __glibcxx_assert(tp->pools);
       }
     return pools;
   }
@@ -1189,24 +1202,34 @@  namespace pmr
   do_allocate(size_t bytes, size_t alignment)
   {
     const auto block_size = std::max(bytes, alignment);
-    if (block_size <= _M_impl._M_opts.largest_required_pool_block)
+    const pool_options opts = _M_impl._M_opts;
+    if (block_size <= opts.largest_required_pool_block)
       {
 	const ptrdiff_t index = pool_index(block_size, _M_impl._M_npools);
-	memory_resource* const r = upstream_resource();
-	const pool_options opts = _M_impl._M_opts;
-	{
-	  // Try to allocate from the thread-specific pool
-	  shared_lock l(_M_mx);
-	  if (auto pools = _M_thread_specific_pools()) // [[likely]]
-	    {
-	      // Need exclusive lock to replenish so use try_allocate:
-	      if (void* p = pools[index].try_allocate())
-		return p;
-	      // Need to take exclusive lock and replenish pool.
-	    }
-	  // Need to allocate or replenish thread-specific pools using
-	  // upstream resource, so need to hold exclusive lock.
-	}
+	if (__gthread_active_p())
+	  {
+	    // Try to allocate from the thread-specific pool.
+	    shared_lock l(_M_mx);
+	    if (auto pools = _M_thread_specific_pools()) // [[likely]]
+	      {
+		// Need exclusive lock to replenish so use try_allocate:
+		if (void* p = pools[index].try_allocate())
+		  return p;
+		// Need to take exclusive lock and replenish pool.
+	      }
+	    // Need to allocate or replenish thread-specific pools using
+	    // upstream resource, so need to hold exclusive lock.
+	  }
+	else // single-threaded
+	  {
+	    if (!_M_tpools) // [[unlikely]]
+	      {
+		exclusive_lock dummy(_M_mx);
+		_M_tpools = _M_alloc_shared_tpools(dummy);
+	      }
+	    return _M_tpools->pools[index].allocate(upstream_resource(), opts);
+	  }
+
 	// N.B. Another thread could call release() now lock is not held.
 	exclusive_lock excl(_M_mx);
 	if (!_M_tpools) // [[unlikely]]
@@ -1214,7 +1237,7 @@  namespace pmr
 	auto pools = _M_thread_specific_pools();
 	if (!pools)
 	  pools = _M_alloc_tpools(excl)->pools;
-	return pools[index].allocate(r, opts);
+	return pools[index].allocate(upstream_resource(), opts);
       }
     exclusive_lock l(_M_mx);
     return _M_impl.allocate(bytes, alignment); // unpooled allocation
@@ -1230,30 +1253,45 @@  namespace pmr
       {
 	const ptrdiff_t index = pool_index(block_size, _M_impl._M_npools);
 	__glibcxx_assert(index != -1);
-	{
-	  shared_lock l(_M_mx);
-	  auto pools = _M_thread_specific_pools();
-	  if (pools)
-	    {
-	      // No need to lock here, no other thread is accessing this pool.
-	      if (pools[index].deallocate(upstream_resource(), p))
-		return;
-	    }
-	  // Block might have come from a different thread's pool,
-	  // take exclusive lock and check every pool.
-	}
+	if (__gthread_active_p())
+	  {
+	    shared_lock l(_M_mx);
+	    if (auto pools = _M_thread_specific_pools())
+	      {
+		// No need to lock here, no other thread is accessing this pool.
+		if (pools[index].deallocate(upstream_resource(), p))
+		  return;
+	      }
+	    // Block might have come from a different thread's pool,
+	    // take exclusive lock and check every pool.
+	  }
+	else // single-threaded
+	  {
+	    __glibcxx_assert(_M_tpools != nullptr);
+	    if (_M_tpools) // [[likely]]
+	      _M_tpools->pools[index].deallocate(upstream_resource(), p);
+	    return;
+	  }
+
 	// TODO store {p, bytes, alignment} somewhere and defer returning
 	// the block to the correct thread-specific pool until we next
 	// take the exclusive lock.
+
 	exclusive_lock excl(_M_mx);
+	auto my_pools = _M_thread_specific_pools();
 	for (_TPools* t = _M_tpools; t != nullptr; t = t->next)
 	  {
-	    if (t->pools) // [[likely]]
-	      {
-		if (t->pools[index].deallocate(upstream_resource(), p))
-		  return;
-	      }
+	    if (t->pools != my_pools)
+	      if (t->pools) // [[likely]]
+		{
+		  if (t->pools[index].deallocate(upstream_resource(), p))
+		    return;
+		}
 	  }
+	// Not necessarily an error to reach here, release() could have been
+	// called on another thread between releasing the shared lock and
+	// acquiring the exclusive lock.
+	return;
       }
     exclusive_lock l(_M_mx);
     _M_impl.deallocate(p, bytes, alignment);
@@ -1265,6 +1303,7 @@  namespace pmr
   -> _TPools*
   {
     __glibcxx_assert(_M_tpools != nullptr);
+    __glibcxx_assert(__gthread_active_p());
     // dump_list(_M_tpools);
     polymorphic_allocator<_TPools> a(upstream_resource());
     _TPools* p = a.allocate(1);
diff --git a/libstdc++-v3/testsuite/20_util/synchronized_pool_resource/allocate_single.cc b/libstdc++-v3/testsuite/20_util/synchronized_pool_resource/allocate_single.cc
new file mode 100644
index 00000000000..d5b7b162146
--- /dev/null
+++ b/libstdc++-v3/testsuite/20_util/synchronized_pool_resource/allocate_single.cc
@@ -0,0 +1,24 @@ 
+// Copyright (C) 2018-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 }
+// { dg-options "-std=gnu++17" }
+// { dg-require-effective-target c++17 }
+// { dg-require-gthreads "" }
+
+// This runs the same tests as allocate.cc but without -pthread
+#include "./allocate.cc"
diff --git a/libstdc++-v3/testsuite/20_util/synchronized_pool_resource/cons_single.cc b/libstdc++-v3/testsuite/20_util/synchronized_pool_resource/cons_single.cc
new file mode 100644
index 00000000000..060cd7628e5
--- /dev/null
+++ b/libstdc++-v3/testsuite/20_util/synchronized_pool_resource/cons_single.cc
@@ -0,0 +1,24 @@ 
+// Copyright (C) 2018-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 }
+// { dg-options "-std=gnu++17" }
+// { dg-require-effective-target c++17 }
+// { dg-require-gthreads "" }
+
+// This runs the same tests as cons.cc but without -pthread
+#include "./cons.cc"
diff --git a/libstdc++-v3/testsuite/20_util/synchronized_pool_resource/release_single.cc b/libstdc++-v3/testsuite/20_util/synchronized_pool_resource/release_single.cc
new file mode 100644
index 00000000000..32e80c1be3f
--- /dev/null
+++ b/libstdc++-v3/testsuite/20_util/synchronized_pool_resource/release_single.cc
@@ -0,0 +1,24 @@ 
+// Copyright (C) 2018-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 }
+// { dg-options "-std=gnu++17" }
+// { dg-require-effective-target c++17 }
+// { dg-require-gthreads "" }
+
+// This runs the same tests as release.cc but without -pthread
+#include "./release.cc"