Fix build on macOS

Message ID 20191210214717.25680-1-tromey@adacore.com
State New
Headers show
Series
  • Fix build on macOS
Related show

Commit Message

Tom Tromey Dec. 10, 2019, 9:47 p.m.
PR build/25268 points out that the build fails on macOS, because on
macOS the "pthread_setname_np" function takes a single argument.

This patch fixes the problem, by introducing a new template adapter
function that handles both styles of pthread_setname_np.  This is a
technique I learned from Alexandre Oliva, and avoids the need for a
complicated autoconf check.

This change also meant moving the pthread_setname_np call to the
thread function, because macOS only permits setting the name of the
current thread.  This means that there can be a brief window when gdb
will see the wrong name; but I think this is a minor concern.

Tested by rebuilding on x86-64 Fedora 30, and on macOS High Sierra.
On Linux I also debugged gdb to ensure that the thread names are still
set correctly.

gdb/ChangeLog
2019-12-10  Tom Tromey  <tromey@adacore.com>

	PR build/25268:
	* gdbsupport/thread-pool.c (set_thread_name): New template
	function.
	(thread_pool::set_thread_count): Don't call pthread_setname_np.
	(thread_pool::thread_function): Call set_thread_name.

Change-Id: Id7bf28d99ca27a893a9fc87ebb90b15a9c2a9cb4
---
 gdb/ChangeLog                |  8 ++++++++
 gdb/gdbsupport/thread-pool.c | 31 +++++++++++++++++++++++++++----
 2 files changed, 35 insertions(+), 4 deletions(-)

-- 
2.21.0

Comments

Simon Marchi Dec. 11, 2019, 1:37 a.m. | #1
On 2019-12-10 4:47 p.m., Tom Tromey wrote:
> PR build/25268 points out that the build fails on macOS, because on

> macOS the "pthread_setname_np" function takes a single argument.

> 

> This patch fixes the problem, by introducing a new template adapter

> function that handles both styles of pthread_setname_np.  This is a

> technique I learned from Alexandre Oliva, and avoids the need for a

> complicated autoconf check.

> 

> This change also meant moving the pthread_setname_np call to the

> thread function, because macOS only permits setting the name of the

> current thread.  This means that there can be a brief window when gdb

> will see the wrong name; but I think this is a minor concern.

> 

> Tested by rebuilding on x86-64 Fedora 30, and on macOS High Sierra.

> On Linux I also debugged gdb to ensure that the thread names are still

> set correctly.


I think a version without templates would make it a bit simpler to understand:

void
set_thread_name (int (*set_name) (pthread_t, const char *), const char *name)
{
  set_name (pthread_self (), name);
}

void
set_thread_name (int (*set_name) (const char *), const char *name)
{
  set_name (name);
}

Also, they should probably be static, disabling -Wunused-functions, if necessary,
like in:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blobdiff;f=gdb/gdbsupport/safe-strerror.c;h=9973fa678577c3ff1d9188a6679a0c8cecfd5d26;hp=7425af590f789c1625013c9dc51b505a324ad7fd;hb=cb51113052d534b628c635ac7b86b95fe436d60d;hpb=ab7d13f07027e6232a21448ef51f0a52a96738a9

Simon
Tom Tromey Dec. 11, 2019, 1:52 p.m. | #2
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:


Simon> I think a version without templates would make it a bit simpler to understand:

Yeah, duh.  I wonder why I thought that was needed.

Fixing this taught me that the macOS man page for pthread_setname_np is
incorrect, in that it says the function returns void, whereas
<pthread.h> declares it returning int.

Here's v2.

Tom

commit 3e82417785ea486a2b683e7d7a0cbda88bca1e9a
Author: Tom Tromey <tromey@adacore.com>
Date:   Tue Dec 10 14:16:19 2019 -0700

    Fix build on macOS
    
    PR build/25268 points out that the build fails on macOS, because on
    macOS the "pthread_setname_np" function takes a single argument.
    
    This patch fixes the problem, by introducing a new adapter function
    that handles both styles of pthread_setname_np.
    
    This change also meant moving the pthread_setname_np call to the
    thread function, because macOS only permits setting the name of the
    current thread.  This means that there can be a brief window when gdb
    will see the wrong name; but I think this is a minor concern.
    
    Tested by rebuilding on x86-64 Fedora 30, and on macOS High Sierra.
    On Linux I also debugged gdb to ensure that the thread names are still
    set correctly.
    
    gdb/ChangeLog
    2019-12-10  Tom Tromey  <tromey@adacore.com>
    
            PR build/25268:
            * gdbsupport/thread-pool.c (set_thread_name): New function.
            (thread_pool::set_thread_count): Don't call pthread_setname_np.
            (thread_pool::thread_function): Call set_thread_name.
    
    Change-Id: Id7bf28d99ca27a893a9fc87ebb90b15a9c2a9cb4

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index c4e8109b7c5..223a973ccd1 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,10 @@
+2019-12-10  Tom Tromey  <tromey@adacore.com>
+
+	PR build/25268:
+	* gdbsupport/thread-pool.c (set_thread_name): New function.
+	(thread_pool::set_thread_count): Don't call pthread_setname_np.
+	(thread_pool::thread_function): Call set_thread_name.
+
 2019-12-10  Christian Biesinger  <cbiesinger@google.com>
 
 	* gdbsupport/safe-strerror.c: Supress the unused function warning
diff --git a/gdb/gdbsupport/thread-pool.c b/gdb/gdbsupport/thread-pool.c
index d19ae02e3ef..7c63fc6aa02 100644
--- a/gdb/gdbsupport/thread-pool.c
+++ b/gdb/gdbsupport/thread-pool.c
@@ -25,6 +25,7 @@
 #include "gdbsupport/alt-stack.h"
 #include "gdbsupport/block-signals.h"
 #include <algorithm>
+#include "diagnostics.h"
 
 /* On the off chance that we have the pthread library on a Windows
    host, but std::thread is not using it, avoid calling
@@ -36,8 +37,31 @@
 #endif
 
 #ifdef USE_PTHREAD_SETNAME_NP
+
 #include <pthread.h>
-#endif
+
+DIAGNOSTIC_PUSH
+DIAGNOSTIC_IGNORE_UNUSED_FUNCTION
+
+/* Handle platform discrepancies in pthread_setname_np: macOS uses a
+   single-argument form, while Linux uses a two-argument form.  This
+   wrapper template handles the difference.  */
+
+static void
+set_thread_name (int (*set_name) (pthread_t, const char *), const char *name)
+{
+  set_name (pthread_self (), name);
+}
+
+static void
+set_thread_name (void (*set_name) (const char *), const char *name)
+{
+  set_name (name);
+}
+
+DIAGNOSTIC_POP
+
+#endif	/* USE_PTHREAD_SETNAME_NP */
 
 namespace gdb
 {
@@ -75,9 +99,6 @@ thread_pool::set_thread_count (size_t num_threads)
       for (size_t i = m_thread_count; i < num_threads; ++i)
 	{
 	  std::thread thread (&thread_pool::thread_function, this);
-#ifdef USE_PTHREAD_SETNAME_NP
-	  pthread_setname_np (thread.native_handle (), "gdb worker");
-#endif
 	  thread.detach ();
 	}
     }
@@ -115,6 +136,12 @@ thread_pool::post_task (std::function<void ()> func)
 void
 thread_pool::thread_function ()
 {
+#ifdef USE_PTHREAD_SETNAME_NP
+  /* This must be done here, because on macOS one can only set the
+     name of the current thread.  */
+  set_thread_name (pthread_setname_np, "gdb worker");
+#endif
+
   /* Ensure that SIGSEGV is delivered to an alternate signal
      stack.  */
   gdb::alternate_signal_stack signal_stack;
Simon Marchi Dec. 11, 2019, 2:07 p.m. | #3
On 2019-12-11 8:52 a.m., Tom Tromey wrote:
> @@ -36,8 +37,31 @@

>  #endif

>  

>  #ifdef USE_PTHREAD_SETNAME_NP

> +

>  #include <pthread.h>

> -#endif

> +

> +DIAGNOSTIC_PUSH

> +DIAGNOSTIC_IGNORE_UNUSED_FUNCTION

> +

> +/* Handle platform discrepancies in pthread_setname_np: macOS uses a

> +   single-argument form, while Linux uses a two-argument form.  This

> +   wrapper template handles the difference.  */


This comment still mentions "template", you might want to change it?

Otherwise, that LGTM.

Simon
Kevin Buettner Jan. 31, 2020, 2:23 p.m. | #4
On Tue, 10 Dec 2019 14:47:17 -0700
Tom Tromey <tromey@adacore.com> wrote:

> PR build/25268 points out that the build fails on macOS, because on

> macOS the "pthread_setname_np" function takes a single argument.

> 

> This patch fixes the problem, by introducing a new template adapter

> function that handles both styles of pthread_setname_np.  This is a

> technique I learned from Alexandre Oliva, and avoids the need for a

> complicated autoconf check.

> 

> This change also meant moving the pthread_setname_np call to the

> thread function, because macOS only permits setting the name of the

> current thread.  This means that there can be a brief window when gdb

> will see the wrong name; but I think this is a minor concern.

> 

> Tested by rebuilding on x86-64 Fedora 30, and on macOS High Sierra.

> On Linux I also debugged gdb to ensure that the thread names are still

> set correctly.

> 

> gdb/ChangeLog

> 2019-12-10  Tom Tromey  <tromey@adacore.com>

> 

> 	PR build/25268:

> 	* gdbsupport/thread-pool.c (set_thread_name): New template

> 	function.

> 	(thread_pool::set_thread_count): Don't call pthread_setname_np.

> 	(thread_pool::thread_function): Call set_thread_name.


Did this ever go in?

If not, I think it looks reasonable.

Kevin
Tom Tromey Jan. 31, 2020, 2:43 p.m. | #5
>>>>> "Kevin" == Kevin Buettner <kevinb@redhat.com> writes:


>> gdb/ChangeLog

>> 2019-12-10  Tom Tromey  <tromey@adacore.com>

>> 

>> PR build/25268:

>> * gdbsupport/thread-pool.c (set_thread_name): New template

>> function.

>> (thread_pool::set_thread_count): Don't call pthread_setname_np.

>> (thread_pool::thread_function): Call set_thread_name.


Kevin> Did this ever go in?

Kevin> If not, I think it looks reasonable.

Something like it did, and then there was another fix after that.

Tom
Kevin Buettner Jan. 31, 2020, 9:02 p.m. | #6
On Fri, 31 Jan 2020 15:43:29 +0100
Tom Tromey <tromey@adacore.com> wrote:

> >>>>> "Kevin" == Kevin Buettner <kevinb@redhat.com> writes:  

> 

> >> gdb/ChangeLog

> >> 2019-12-10  Tom Tromey  <tromey@adacore.com>

> >> 

> >> PR build/25268:

> >> * gdbsupport/thread-pool.c (set_thread_name): New template

> >> function.

> >> (thread_pool::set_thread_count): Don't call pthread_setname_np.

> >> (thread_pool::thread_function): Call set_thread_name.  

> 

> Kevin> Did this ever go in?  

> 

> Kevin> If not, I think it looks reasonable.  

> 

> Something like it did, and then there was another fix after that.


Yeah, I saw that there was a second fix, but I didn't see the template
stuff, which I thought was pretty cool.

Kevin
Christian Biesinger via Gdb-patches Feb. 5, 2020, 6:36 p.m. | #7
On Fri, Jan 31, 2020 at 4:02 PM Kevin Buettner <kevinb@redhat.com> wrote:
>

> On Fri, 31 Jan 2020 15:43:29 +0100

> Tom Tromey <tromey@adacore.com> wrote:

>

> > >>>>> "Kevin" == Kevin Buettner <kevinb@redhat.com> writes:

> >

> > >> gdb/ChangeLog

> > >> 2019-12-10  Tom Tromey  <tromey@adacore.com>

> > >>

> > >> PR build/25268:

> > >> * gdbsupport/thread-pool.c (set_thread_name): New template

> > >> function.

> > >> (thread_pool::set_thread_count): Don't call pthread_setname_np.

> > >> (thread_pool::thread_function): Call set_thread_name.

> >

> > Kevin> Did this ever go in?

> >

> > Kevin> If not, I think it looks reasonable.

> >

> > Something like it did, and then there was another fix after that.

>

> Yeah, I saw that there was a second fix, but I didn't see the template

> stuff, which I thought was pretty cool.


The ultimate solution used function overloads instead of templates,
which seems simpler.

Christian

Patch

diff --git a/gdb/gdbsupport/thread-pool.c b/gdb/gdbsupport/thread-pool.c
index d19ae02e3ef..8e24570ee96 100644
--- a/gdb/gdbsupport/thread-pool.c
+++ b/gdb/gdbsupport/thread-pool.c
@@ -36,8 +36,28 @@ 
 #endif
 
 #ifdef USE_PTHREAD_SETNAME_NP
+
 #include <pthread.h>
-#endif
+
+/* Handle platform discrepancies in pthread_setname_np: macOS uses a
+   single-argument form, while Linux uses a two-argument form.  This
+   wrapper template handles the difference.  */
+
+template<typename R, typename A1, typename A2>
+void
+set_thread_name (R (*set_name) (A1, A2), const char *name)
+{
+  set_name (pthread_self (), name);
+}
+
+template<typename R, typename A1>
+void
+set_thread_name (R (*set_name) (A1), const char *name)
+{
+  set_name (name);
+}
+
+#endif	/* USE_PTHREAD_SETNAME_NP */
 
 namespace gdb
 {
@@ -75,9 +95,6 @@  thread_pool::set_thread_count (size_t num_threads)
       for (size_t i = m_thread_count; i < num_threads; ++i)
 	{
 	  std::thread thread (&thread_pool::thread_function, this);
-#ifdef USE_PTHREAD_SETNAME_NP
-	  pthread_setname_np (thread.native_handle (), "gdb worker");
-#endif
 	  thread.detach ();
 	}
     }
@@ -115,6 +132,12 @@  thread_pool::post_task (std::function<void ()> func)
 void
 thread_pool::thread_function ()
 {
+#ifdef USE_PTHREAD_SETNAME_NP
+  /* This must be done here, because on macOS one can only set the
+     name of the current thread.  */
+  set_thread_name (pthread_setname_np, "gdb worker");
+#endif
+
   /* Ensure that SIGSEGV is delivered to an alternate signal
      stack.  */
   gdb::alternate_signal_stack signal_stack;