bug#38632: 27.0.50; Emacs process name is changed permanently upon creating a named thread

["Mattias Engdegård" <mattiase@acm.org>]

[-- Attachment #1: Type: text/plain, Size: 489 bytes --]

7 jan. 2020 kl. 16.45 skrev Eli Zaretskii <eliz@gnu.org>:

> This line cannot be deleted, as it will cause systhread.c fail to
> compile on MS-Windows.

Oops! Reinstated.

However, isn't thread_start_address inherently racy? It doesn't matter much since it will only ever have a single value (run_thread), but it does violate layering. That problem won't be addressed in this patch, however.

> And this comment on thread.h:

Edited.

Thanks for the review! Patch updated.


[-- Attachment #2: 0001-Fix-BSD-and-macOS-builds-w.r.t.-pthread_setname_np-b.patch --]
[-- Type: application/octet-stream, Size: 7545 bytes --]

From f69ecf1c239c3ed90d4265b0b41c9881387c49dd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Tue, 7 Jan 2020 17:08:25 +0100
Subject: [PATCH] Fix BSD and macOS builds w.r.t. pthread_setname_np
 (bug#38632)

pthread_setname_np takes only a single argument on BSD and macOS,
and affects the current thread only.

* configure.ac: Add check for single-argument pthread_setname_np
* src/systhread.c (sys_thread_set_name): New (w32 and pthread versions).
(sys_thread_create): Remove name argument and name-setting.
(w32_beginthread_wrapper): Remove name-setting.
* src/systhread.h (sys_thread_create, sys_thread_set_name):
Update prototypes.
* src/thread.c (run_thread): Call sys_thread_set_name.
(Fmake_thread): Adapt call to sys_thread_create.
* src/thread.h (struct thread_state): Adjust comment.
---
 configure.ac    | 17 +++++++++++++++
 src/systhread.c | 58 +++++++++++++++++++++++++------------------------
 src/systhread.h |  5 +++--
 src/thread.c    |  5 ++++-
 src/thread.h    |  3 +--
 5 files changed, 55 insertions(+), 33 deletions(-)

diff --git a/configure.ac b/configure.ac
index de4710f150..068ef5c20c 100644
--- a/configure.ac
+++ b/configure.ac
@@ -4183,6 +4183,23 @@ AC_DEFUN
 cfmakeraw cfsetspeed __executable_start log2 pthread_setname_np)
 LIBS=$OLD_LIBS
 
+if test "$ac_cv_func_pthread_setname_np" = "yes"; then
+  AC_CACHE_CHECK(
+   [whether pthread_setname_np takes a single argument],
+   [emacs_cv_pthread_setname_np_1arg],
+   [AC_COMPILE_IFELSE(
+     [AC_LANG_PROGRAM(
+       [[#include <pthread.h>]],
+       [[pthread_setname_np ("a");]])],
+     [emacs_cv_pthread_setname_np_1arg=yes],
+     [emacs_cv_pthread_setname_np_1arg=no])])
+  if test "$emacs_cv_pthread_setname_np_1arg" = "yes"; then
+    AC_DEFINE(
+      HAVE_PTHREAD_SETNAME_NP_1ARG, 1,
+      [Define to 1 if pthread_setname_np takes a single argument.])
+  fi
+fi
+
 dnl No need to check for posix_memalign if aligned_alloc works.
 AC_CHECK_FUNCS([aligned_alloc posix_memalign], [break])
 AC_CHECK_DECLS([aligned_alloc], [], [], [[#include <stdlib.h>]])
diff --git a/src/systhread.c b/src/systhread.c
index 1dda036cc2..2c3a060a17 100644
--- a/src/systhread.c
+++ b/src/systhread.c
@@ -200,9 +200,28 @@ sys_thread_equal (sys_thread_t t, sys_thread_t u)
   return pthread_equal (t, u);
 }
 
+void
+sys_thread_set_name (const char *name)
+{
+#ifdef HAVE_PTHREAD_SETNAME_NP
+  /* We need to truncate here otherwise pthread_setname_np
+     fails to set the name.  TASK_COMM_LEN is what the length
+     is called in the Linux kernel headers (Bug#38632).  */
+#define TASK_COMM_LEN 16
+  char p_name[TASK_COMM_LEN];
+  strncpy (p_name, name, TASK_COMM_LEN - 1);
+  p_name[TASK_COMM_LEN - 1] = '\0';
+ #ifdef HAVE_PTHREAD_SETNAME_NP_1ARG
+  pthread_setname_np (p_name);
+ #else
+  pthread_setname_np (pthread_self (), p_name);
+ #endif
+#endif
+}
+
 bool
-sys_thread_create (sys_thread_t *thread_ptr, const char *name,
-		   thread_creation_function *func, void *arg)
+sys_thread_create (sys_thread_t *thread_ptr, thread_creation_function *func,
+                   void *arg)
 {
   pthread_attr_t attr;
   bool result = false;
@@ -221,22 +240,7 @@ sys_thread_create (sys_thread_t *thread_ptr, const char *name,
     }
 
   if (!pthread_attr_setdetachstate (&attr, PTHREAD_CREATE_DETACHED))
-    {
-      result = pthread_create (thread_ptr, &attr, func, arg) == 0;
-#ifdef HAVE_PTHREAD_SETNAME_NP
-      if (result && name != NULL)
-        {
-          /* We need to truncate here otherwise pthread_setname_np
-             fails to set the name.  TASK_COMM_LEN is what the length
-             is called in the Linux kernel headers (Bug#38632).  */
-#define TASK_COMM_LEN 16
-          char p_name[TASK_COMM_LEN];
-          strncpy (p_name, name, TASK_COMM_LEN - 1);
-          p_name[TASK_COMM_LEN - 1] = '\0';
-          pthread_setname_np (*thread_ptr, p_name);
-        }
-#endif
-    }
+    result = pthread_create (thread_ptr, &attr, func, arg) == 0;
 
  out: ;
   int error = pthread_attr_destroy (&attr);
@@ -457,26 +461,24 @@ w32_set_thread_name (DWORD thread_id, const char *name)
 
 static thread_creation_function *thread_start_address;
 
+void
+sys_thread_set_name (const char *name)
+{
+  w32_set_thread_name (GetCurrentThreadId (), name);
+}
+
 /* _beginthread wants a void function, while we are passed a function
    that returns a pointer.  So we use a wrapper.  See the command in
    w32term.h about the need for ALIGN_STACK attribute.  */
 static void ALIGN_STACK
 w32_beginthread_wrapper (void *arg)
 {
-  /* FIXME: This isn't very clean: systhread.c is not supposed to know
-     that ARG is a pointer to a thread_state object, or be familiar
-     with thread_state object's structure in general.  */
-  struct thread_state *this_thread = arg;
-
-  if (this_thread->thread_name)
-    w32_set_thread_name (GetCurrentThreadId (), this_thread->thread_name);
-
   (void)thread_start_address (arg);
 }
 
 bool
-sys_thread_create (sys_thread_t *thread_ptr, const char *name,
-		   thread_creation_function *func, void *arg)
+sys_thread_create (sys_thread_t *thread_ptr, thread_creation_function *func,
+                   void *arg)
 {
   /* FIXME: Do threads that run Lisp require some minimum amount of
      stack?  Zero here means each thread will get the same amount as
diff --git a/src/systhread.h b/src/systhread.h
index 5368acfb52..005388fd5a 100644
--- a/src/systhread.h
+++ b/src/systhread.h
@@ -112,10 +112,11 @@ #define SYSTHREAD_H
 extern bool sys_thread_equal (sys_thread_t, sys_thread_t)
   ATTRIBUTE_WARN_UNUSED_RESULT;
 
-extern bool sys_thread_create (sys_thread_t *, const char *,
-                               thread_creation_function *, void *)
+extern bool sys_thread_create (sys_thread_t *, thread_creation_function *,
+                               void *)
   ATTRIBUTE_WARN_UNUSED_RESULT;
 
 extern void sys_thread_yield (void);
+extern void sys_thread_set_name (const char *);
 
 #endif /* SYSTHREAD_H */
diff --git a/src/thread.c b/src/thread.c
index f7e39dc427..c7fe061426 100644
--- a/src/thread.c
+++ b/src/thread.c
@@ -725,6 +725,9 @@ run_thread (void *state)
   self->m_stack_bottom = self->stack_top = (char *) &stack_pos;
   self->thread_id = sys_thread_self ();
 
+  if (self->thread_name)
+    sys_thread_set_name (self->thread_name);
+
   acquire_global_lock (self);
 
   /* Put a dummy catcher at top-level so that handlerlist is never NULL.
@@ -832,7 +835,7 @@ DEFUN ("make-thread", Fmake_thread, Smake_thread, 1, 2, 0,
   else
     new_thread->thread_name = NULL;
   sys_thread_t thr;
-  if (! sys_thread_create (&thr, c_name, run_thread, new_thread))
+  if (! sys_thread_create (&thr, run_thread, new_thread))
     {
       /* Restore the previous situation.  */
       all_threads = all_threads->next_thread;
diff --git a/src/thread.h b/src/thread.h
index e96a063a10..a09929fa44 100644
--- a/src/thread.h
+++ b/src/thread.h
@@ -169,8 +169,7 @@ #define getcjmp (current_thread->m_getcjmp)
      interrupter should broadcast to this condition.  */
   sys_cond_t *wait_condvar;
 
-  /* Thread's name in the locale encoding.  Actually used only on
-     WINDOWSNT.  */
+  /* Thread's name in the locale encoding.  */
   char *thread_name;
 
   /* This thread might have released the global lock.  If so, this is
-- 
2.21.0 (Apple Git-122.2)