[PATCH] Add lock_guard to thread_pool::thread_count

[Tom Tromey <tom@tromey.com>]

I think there's a possible race when calling thread_pool::thread_count
from a worker thread, if the main thread changes the number of threads
at the same time.

When this code was initially written, we didn't worry about this,
because this was only accessed from the main thread.  This is no
longer the case, though.

This patch fixes any potential problem by adding a lock_guard to the
method.  This doesn't seem too harmful because this is not called very
much and because I doubt this lock is highly contended.

While doing this I noticed that thread_pool::do_post_task could access
m_sized_at_least_once and m_thread_count without holding the lock.
This patch changes this method as well.
---
 gdbsupport/thread-pool.cc | 47 +++++++++++++++++++++++++++++----------
 gdbsupport/thread-pool.h  |  9 +-------
 2 files changed, 36 insertions(+), 20 deletions(-)

diff --git a/gdbsupport/thread-pool.cc b/gdbsupport/thread-pool.cc
index 6940773d58a..4dc72e56642 100644
--- a/gdbsupport/thread-pool.cc
+++ b/gdbsupport/thread-pool.cc
@@ -148,6 +148,17 @@ thread_pool::~thread_pool ()
      case -- see the comment by the definition of g_thread_pool.  */
 }
 
+size_t
+thread_pool::thread_count ()
+{
+#if CXX_STD_THREAD
+  std::lock_guard<std::mutex> guard (m_tasks_mutex);
+  return m_thread_count;
+#else
+  return 0;
+#endif
+}
+
 void
 thread_pool::set_thread_count (size_t num_threads)
 {
@@ -197,21 +208,33 @@ thread_pool::set_thread_count (size_t num_threads)
 void
 thread_pool::do_post_task (std::packaged_task<void ()> &&func)
 {
-  /* This assert is here to check that no tasks are posted to the pool between
-     its initialization and sizing.  */
-  gdb_assert (m_sized_at_least_once);
-  std::packaged_task<void ()> t (std::move (func));
+  /* This is a bit strange but we don't want to hold the lock when
+     calling FUNC in the case where it must be called immediately.
+     Although that seems pathological, there are some weird cases (a
+     moribund worker thread or FUNC itself submitting a task) and it
+     seemed best to be safe.  */
+  bool call_immediately = false;
 
-  if (m_thread_count != 0)
-    {
-      std::lock_guard<std::mutex> guard (m_tasks_mutex);
-      m_tasks.emplace (std::move (t));
-      m_tasks_cv.notify_one ();
-    }
-  else
+  {
+    std::lock_guard<std::mutex> guard (m_tasks_mutex);
+
+    /* This assert is here to check that no tasks are posted to the
+       pool between its initialization and sizing.  */
+    gdb_assert (m_sized_at_least_once);
+
+    if (m_thread_count != 0)
+      {
+	m_tasks.emplace (std::move (func));
+	m_tasks_cv.notify_one ();
+      }
+    else
+      call_immediately = true;
+  }
+
+  if (call_immediately)
     {
       /* Just execute it now.  */
-      t ();
+      func ();
     }
 }
 
diff --git a/gdbsupport/thread-pool.h b/gdbsupport/thread-pool.h
index 50b7a064d7e..a9188d1e107 100644
--- a/gdbsupport/thread-pool.h
+++ b/gdbsupport/thread-pool.h
@@ -50,14 +50,7 @@ class thread_pool
   void set_thread_count (size_t num_threads);
 
   /* Return the number of executing threads.  */
-  size_t thread_count () const
-  {
-#if CXX_STD_THREAD
-    return m_thread_count;
-#else
-    return 0;
-#endif
-  }
+  size_t thread_count ();
 
   /* Post a task to the thread pool.  A future is returned, which can
      be used to wait for the result.  */
-- 
2.49.0