* [PATCH] Fix build on macOS
@ 2019-12-10 21:47 Tom Tromey
2019-12-11 1:37 ` Simon Marchi
2020-01-31 14:43 ` Kevin Buettner
0 siblings, 2 replies; 8+ messages in thread
From: Tom Tromey @ 2019-12-10 21:47 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
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(-)
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;
--
2.21.0
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix build on macOS
2019-12-10 21:47 [PATCH] Fix build on macOS Tom Tromey
@ 2019-12-11 1:37 ` Simon Marchi
2019-12-11 13:52 ` Tom Tromey
2020-01-31 14:43 ` Kevin Buettner
1 sibling, 1 reply; 8+ messages in thread
From: Simon Marchi @ 2019-12-11 1:37 UTC (permalink / raw)
To: Tom Tromey, gdb-patches
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix build on macOS
2019-12-11 1:37 ` Simon Marchi
@ 2019-12-11 13:52 ` Tom Tromey
2019-12-11 14:07 ` Simon Marchi
0 siblings, 1 reply; 8+ messages in thread
From: Tom Tromey @ 2019-12-11 13:52 UTC (permalink / raw)
To: Simon Marchi; +Cc: Tom Tromey, gdb-patches
>>>>> "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;
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix build on macOS
2019-12-11 13:52 ` Tom Tromey
@ 2019-12-11 14:07 ` Simon Marchi
0 siblings, 0 replies; 8+ messages in thread
From: Simon Marchi @ 2019-12-11 14:07 UTC (permalink / raw)
To: Tom Tromey; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix build on macOS
2019-12-10 21:47 [PATCH] Fix build on macOS Tom Tromey
2019-12-11 1:37 ` Simon Marchi
@ 2020-01-31 14:43 ` Kevin Buettner
2020-01-31 16:52 ` Tom Tromey
1 sibling, 1 reply; 8+ messages in thread
From: Kevin Buettner @ 2020-01-31 14:43 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix build on macOS
2020-01-31 14:43 ` Kevin Buettner
@ 2020-01-31 16:52 ` Tom Tromey
2020-01-31 21:40 ` Kevin Buettner
0 siblings, 1 reply; 8+ messages in thread
From: Tom Tromey @ 2020-01-31 16:52 UTC (permalink / raw)
To: Kevin Buettner; +Cc: gdb-patches, Tom Tromey
>>>>> "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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix build on macOS
2020-01-31 16:52 ` Tom Tromey
@ 2020-01-31 21:40 ` Kevin Buettner
2020-02-05 18:37 ` Christian Biesinger via gdb-patches
0 siblings, 1 reply; 8+ messages in thread
From: Kevin Buettner @ 2020-01-31 21:40 UTC (permalink / raw)
To: Tom Tromey; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix build on macOS
2020-01-31 21:40 ` Kevin Buettner
@ 2020-02-05 18:37 ` Christian Biesinger via gdb-patches
0 siblings, 0 replies; 8+ messages in thread
From: Christian Biesinger via gdb-patches @ 2020-02-05 18:37 UTC (permalink / raw)
To: Kevin Buettner; +Cc: Tom Tromey, gdb-patches
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-02-05 18:37 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-10 21:47 [PATCH] Fix build on macOS Tom Tromey
2019-12-11 1:37 ` Simon Marchi
2019-12-11 13:52 ` Tom Tromey
2019-12-11 14:07 ` Simon Marchi
2020-01-31 14:43 ` Kevin Buettner
2020-01-31 16:52 ` Tom Tromey
2020-01-31 21:40 ` Kevin Buettner
2020-02-05 18:37 ` Christian Biesinger via gdb-patches
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).