[review v4] Set names of worker threads

["Pedro Alves (Code Review)" <gerrit@gnutoolchain-gerrit.osci.io>]

Pedro Alves has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/176
......................................................................


Patch Set 4:

(3 comments)

Some comments below.  Nothing major.

| --- gdb/gdbsupport/thread-pool.c
| +++ gdb/gdbsupport/thread-pool.c
| @@ -22,15 +22,24 @@ #include "common-defs.h"
|  #if CXX_STD_THREAD
|  
|  #include "gdbsupport/thread-pool.h"
|  #include "gdbsupport/alt-stack.h"
|  #include "gdbsupport/block-signals.h"
|  #include <algorithm>
|  
| +/* On the off chance that we have the pthread library on a Windows
| +   host, but std::thread is not using it, avoid calling
| +   pthread_setname_np on Windows.  */

PS4, Line 31:

Meh.  Judging from
<https://stackoverflow.com/questions/19250008/mingw-stdthread-with-
windows-api>, I think this could be solved.  But yeah, we really don't
have to bend backwards in this initial patch.

| +#ifndef _WIN32
| +#ifdef HAVE_PTHREAD_SETNAME_NP
| +#include <pthread.h>
| +#endif
| +#endif

PS4, Line 36:

I'd rather this was:

#ifndef _WIN32
# ifdef HAVE_PTHREAD_SETNAME_NP
#  define USE_PTHREAD_SETNAME_NP
# endif
#endif

#ifdef USE_PTHREAD_SETNAME_NP
# include <pthread.h>
#endif

so that below we don't duplicate the checks, and we'd have a single
place to edit in the future if the checks evolve.  See below...

| +
|  namespace gdb
|  {
|  
|  /* The thread pool detach()s its threads, so that the threads will not
|     prevent the process from exiting.  However, it was discovered that
|     if any detached threads were still waiting on a condition variable,
|     then the condition variable's destructor would wait for the threads
|     to exit -- defeating the purpose.

 ...

| @@ -57,14 +66,19 @@ thread_pool::set_thread_count (size_t num_threads)
|    if (m_thread_count < num_threads)
|      {
|        /* Ensure that signals used by gdb are blocked in the new
|  	 threads.  */
|        block_signals blocker;
|        for (size_t i = m_thread_count; i < num_threads; ++i)
|  	{
|  	  std::thread thread (&thread_pool::thread_function, this);
| +#ifndef _WIN32 /* See the comment at the top of the file.  */
| +#ifdef HAVE_PTHREAD_SETNAME_NP

PS4, Line 75:

... here, we'd do:

#ifdef USE_PTHREAD_SETNAME_NP

| +	  pthread_setname_np (thread.native_handle (), "gdb worker");
| +#endif
| +#endif
|  	  thread.detach ();
|  	}
|      }
|    /* If the new size is smaller, terminate some existing threads.  */
|    if (num_threads < m_thread_count)
|      {

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I60473d65ae9ae14d8c56ddde39684240c16aaf35
Gerrit-Change-Number: 176
Gerrit-PatchSet: 4
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-CC: Pedro Alves <palves@redhat.com>
Gerrit-Comment-Date: Tue, 26 Nov 2019 19:38:06 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment