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

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

[Ihor Radchenko]

Observed behaviour:

When I try to create a named thread like

(make-thread (lambda () (+ 1 2)) "test-emacs-async")

the emacs process gets renamed to "test-emacs-async" even after the
thread returns. Making another thread with different name renames emacs
process again.

Expected behaviour:

Emacs process name remains unchanged.

Note: by process name, I mean "COMMAND" field in top output. 

Best,
Ihor


In GNU Emacs 27.0.50 (build 1, x86_64-pc-linux-gnu, X toolkit, cairo version 1.16.0)
 of 2019-12-16 built on yantar92-laptop
Repository revision: 7254b6346229a7b71b69f71e2d8eee113f02585a
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12005000
System Description: Gentoo/Linux

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

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@gmail.com>
> Date: Mon, 16 Dec 2019 14:42:38 +0800
> 
> When I try to create a named thread like
> 
> (make-thread (lambda () (+ 1 2)) "test-emacs-async")
> 
> the emacs process gets renamed to "test-emacs-async" even after the
> thread returns. Making another thread with different name renames emacs
> process again.

I think, instead of calling prctl in systhread.c, we should call
pthread_set_name_np, and the configure-time test for prctl should be
replaced with a test for pthread_set_name_np.

Patches welcome.

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

[Robert Pluim]

>>>>> On Tue, 17 Dec 2019 22:05:52 +0200, Eli Zaretskii <eliz@gnu.org> said:

    >> From: Ihor Radchenko <yantar92@gmail.com>
    >> Date: Mon, 16 Dec 2019 14:42:38 +0800
    >> 
    >> When I try to create a named thread like
    >> 
    >> (make-thread (lambda () (+ 1 2)) "test-emacs-async")
    >> 
    >> the emacs process gets renamed to "test-emacs-async" even after the
    >> thread returns. Making another thread with different name renames emacs
    >> process again.

    Eli> I think, instead of calling prctl in systhread.c, we should call
    Eli> pthread_set_name_np, and the configure-time test for prctl should be
    Eli> replaced with a test for pthread_set_name_np.

Would it not be easier to call prctl in the context of the created
thread? That way it deals with the name length issues for us:
pthread_setname_np fails if strlen(name) >= 16, wherease prctl
truncates.

Robert

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

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: Ihor Radchenko <yantar92@gmail.com>,  38632@debbugs.gnu.org
> Date: Wed, 18 Dec 2019 10:05:58 +0100
> 
>     Eli> I think, instead of calling prctl in systhread.c, we should call
>     Eli> pthread_set_name_np, and the configure-time test for prctl should be
>     Eli> replaced with a test for pthread_set_name_np.
> 
> Would it not be easier to call prctl in the context of the created
> thread? That way it deals with the name length issues for us:
> pthread_setname_np fails if strlen(name) >= 16, wherease prctl
> truncates.

AFAIU, prctl is Linux-specific, whereas pthread_setname_np is
supported on other Posix platforms that provide pthreads.  Also, prctl
has another disadvantage, in that it requires you to pass the name to
the thread being created, or put it in some global.  OTOH, truncating
a string is not exactly rocket science, we can do that ourselves
before calling the API.

(Btw, the limitation is 16 bytes, including the terminating null, so
truncation needs to be clever about non-ASCII characters, and I wonder
what does prctl do when 15 bytes end in the middle of a multibyte
sequence.)

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

[Robert Pluim]

>>>>> On Wed, 18 Dec 2019 17:53:49 +0200, Eli Zaretskii <eliz@gnu.org> said:

    Eli> AFAIU, prctl is Linux-specific, whereas pthread_setname_np is
    Eli> supported on other Posix platforms that provide pthreads.  Also, prctl
    Eli> has another disadvantage, in that it requires you to pass the name to
    Eli> the thread being created, or put it in some global.  OTOH, truncating
    Eli> a string is not exactly rocket science, we can do that ourselves
    Eli> before calling the API.

We already use a wrapper function to call the user-supplied function,
so prctl could be added there, but the wider availability of
pthread_setname_np weighs in its favour.

    Eli> (Btw, the limitation is 16 bytes, including the terminating null, so
    Eli> truncation needs to be clever about non-ASCII characters, and I wonder
    Eli> what does prctl do when 15 bytes end in the middle of a multibyte
    Eli> sequence.)

It does exactly what you'd expect, it drops the extraneous bytes, so
putting eg ü on the boundary results in a name ending in à (#xc3).

In any case, if emacs or prctl truncates, then the name as reported
by 'list-threads' will be out of sync with pthread_getname_np, unless
you'd want to adjust that too.

Robert

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

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: yantar92@gmail.com,  38632@debbugs.gnu.org
> Date: Wed, 18 Dec 2019 18:04:53 +0100
> 
>     Eli> (Btw, the limitation is 16 bytes, including the terminating null, so
>     Eli> truncation needs to be clever about non-ASCII characters, and I wonder
>     Eli> what does prctl do when 15 bytes end in the middle of a multibyte
>     Eli> sequence.)
> 
> It does exactly what you'd expect, it drops the extraneous bytes, so
> putting eg ü on the boundary results in a name ending in à (#xc3).

So it is one more reason to do or own truncation, so we do it right.

> In any case, if emacs or prctl truncates, then the name as reported
> by 'list-threads' will be out of sync with pthread_getname_np, unless
> you'd want to adjust that too.

We need to truncate the names we store in the thread object (and
document that).

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

[Robert Pluim]

>>>>> On Wed, 18 Dec 2019 19:18:57 +0200, Eli Zaretskii <eliz@gnu.org> said:

    >> From: Robert Pluim <rpluim@gmail.com>
    >> Cc: yantar92@gmail.com,  38632@debbugs.gnu.org
    >> Date: Wed, 18 Dec 2019 18:04:53 +0100
    >> 
    Eli> (Btw, the limitation is 16 bytes, including the terminating null, so
    Eli> truncation needs to be clever about non-ASCII characters, and I wonder
    Eli> what does prctl do when 15 bytes end in the middle of a multibyte
    Eli> sequence.)
    >> 
    >> It does exactly what you'd expect, it drops the extraneous bytes, so
    >> putting eg ü on the boundary results in a name ending in à (#xc3).

    Eli> So it is one more reason to do or own truncation, so we do it right.

OK. Is there a useful function that would help for that? I can cook up
something based on NEXT_CHAR_BOUNDARY and/or BYTES_BY_CHAR_HEAD, but Iʼd
expect there to be something already.

    >> In any case, if emacs or prctl truncates, then the name as reported
    >> by 'list-threads' will be out of sync with pthread_getname_np, unless
    >> you'd want to adjust that too.

    Eli> We need to truncate the names we store in the thread object (and
    Eli> document that).

Thatʼs the easy part. Iʼm assuming mswindows doesnʼt truncate.

Robert

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

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: 38632@debbugs.gnu.org,  yantar92@gmail.com
> Date: Wed, 18 Dec 2019 22:30:14 +0100
> 
>     >> It does exactly what you'd expect, it drops the extraneous bytes, so
>     >> putting eg ü on the boundary results in a name ending in à (#xc3).
> 
>     Eli> So it is one more reason to do or own truncation, so we do it right.
> 
> OK. Is there a useful function that would help for that? I can cook up
> something based on NEXT_CHAR_BOUNDARY and/or BYTES_BY_CHAR_HEAD, but Iʼd
> expect there to be something already.

No, the above two and their ilk (PREV_CHAR_BOUNDARY etc.) are what we
use.

>     >> In any case, if emacs or prctl truncates, then the name as reported
>     >> by 'list-threads' will be out of sync with pthread_getname_np, unless
>     >> you'd want to adjust that too.
> 
>     Eli> We need to truncate the names we store in the thread object (and
>     Eli> document that).

I think I'm going to change my mind on that.  The complication here is
that the name should be encoded by ENCODE_SYSTEM before we pass it to
pthread_setname_np etc., and if the locale-coding-system is not UTF-8
and not single-byte, we don't really know where a character will end
after encoding.  And encoding one character at a time sounds too
gross.

So perhaps we should just truncate the bytes, and leave this to the
application to make sure the result makes sense, and also disregard
the difference between list-threads and the thread name as the OS and
debuggers see it.  Doing that will also avoid the complication of
having the thread name return to the caller different from what the
caller used.

WDYT?

> Iʼm assuming mswindows doesnʼt truncate.

MS-Windows currently doesn't support setting thread names at all;
maybe I'll write something soon to fix that.  But yes, I cannot find
any documentation stating any limits there.  GDB on Windows reads the
first 1024 bytes of the thread name, so maybe we should document that
this is the recommended maximum on any platform.

Thanks.

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

[Robert Pluim]

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

>>>>> On Thu, 19 Dec 2019 17:11:24 +0200, Eli Zaretskii <eliz@gnu.org> said:
    Eli> We need to truncate the names we store in the thread object (and
    Eli> document that).

    Eli> I think I'm going to change my mind on that.  The complication here is
    Eli> that the name should be encoded by ENCODE_SYSTEM before we pass it to
    Eli> pthread_setname_np etc., and if the locale-coding-system is not UTF-8
    Eli> and not single-byte, we don't really know where a character will end
    Eli> after encoding.  And encoding one character at a time sounds too
    Eli> gross.

    Eli> So perhaps we should just truncate the bytes, and leave this to the
    Eli> application to make sure the result makes sense, and also disregard
    Eli> the difference between list-threads and the thread name as the OS and
    Eli> debuggers see it.  Doing that will also avoid the complication of
    Eli> having the thread name return to the caller different from what the
    Eli> caller used.

Hereʼs what it looks like.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Use-pthread_setname_np-to-set-thread-name.patch --]
[-- Type: text/x-patch, Size: 2496 bytes --]

From bbb7e7837fcad3bac10aef69a1b331243c2b1c4c Mon Sep 17 00:00:00 2001
From: Robert Pluim <rpluim@gmail.com>
Date: Thu, 19 Dec 2019 17:33:16 +0100
Subject: [PATCH] Use pthread_setname_np to set thread name

* configure.ac: Remove check for sys/prctl.h and prctl, check for
pthread_setname_np instead.

* systhread.c: Remove sys/prctl.h include.
(sys_thread_create) [HAVE_PTHREAD_SETNAME_NP]: Use pthread_setname_np
to set the name of the newly created thread (Bug#38632).
---
 configure.ac    |  4 ++--
 src/systhread.c | 17 +++++++++++------
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/configure.ac b/configure.ac
index 3b6a2a6d16..34a2654494 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1767,7 +1767,7 @@ AC_DEFUN
   sys/sysinfo.h
   coff.h pty.h
   sys/resource.h
-  sys/utsname.h pwd.h utmp.h util.h sys/prctl.h)
+  sys/utsname.h pwd.h utmp.h util.h)
 
 AC_CACHE_CHECK([for ADDR_NO_RANDOMIZE],
   [emacs_cv_personality_addr_no_randomize],
@@ -4180,7 +4180,7 @@ AC_DEFUN
 sendto recvfrom getsockname getifaddrs freeifaddrs \
 gai_strerror sync \
 getpwent endpwent getgrent endgrent \
-cfmakeraw cfsetspeed __executable_start log2 prctl)
+cfmakeraw cfsetspeed __executable_start log2 pthread_setname_np)
 LIBS=$OLD_LIBS
 
 dnl No need to check for posix_memalign if aligned_alloc works.
diff --git a/src/systhread.c b/src/systhread.c
index 6f4de536fb..55bde837e3 100644
--- a/src/systhread.c
+++ b/src/systhread.c
@@ -98,10 +98,6 @@ sys_thread_yield (void)
 
 #include <sched.h>
 
-#ifdef HAVE_SYS_PRCTL_H
-#include <sys/prctl.h>
-#endif
-
 void
 sys_mutex_init (sys_mutex_t *mutex)
 {
@@ -227,9 +223,18 @@ 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;
-#if defined (HAVE_SYS_PRCTL_H) && defined (HAVE_PRCTL) && defined (PR_SET_NAME)
+#ifdef HAVE_PTHREAD_SETNAME_NP
       if (result && name != NULL)
-	prctl (PR_SET_NAME, name);
+        {
+          /* 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
     }
 
-- 
2.23.0

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

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: 38632@debbugs.gnu.org,  yantar92@gmail.com
> Date: Thu, 19 Dec 2019 17:42:41 +0100
> 
> Hereʼs what it looks like.

Thanks, but I think we need to change thread.c to use ENCODE_SYSTEM
instead of ENCODE_UTF_8, since the name should be passed encoded in
the locale's codeset.

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

[Eli Zaretskii]

> Date: Thu, 19 Dec 2019 17:11:24 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 38632@debbugs.gnu.org, yantar92@gmail.com
> 
> MS-Windows currently doesn't support setting thread names at all;
> maybe I'll write something soon to fix that.

Now done.

I'd appreciate testing on 64-bit Windows and in particular on Windows
10.

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

[Robert Pluim]

>>>>> On Fri, 20 Dec 2019 21:13:59 +0200, Eli Zaretskii <eliz@gnu.org> said:

    >> Date: Thu, 19 Dec 2019 17:11:24 +0200
    >> From: Eli Zaretskii <eliz@gnu.org>
    >> Cc: 38632@debbugs.gnu.org, yantar92@gmail.com
    >> 
    >> MS-Windows currently doesn't support setting thread names at all;
    >> maybe I'll write something soon to fix that.

    Eli> Now done.

    Eli> I'd appreciate testing on 64-bit Windows and in particular on Windows
    Eli> 10.

It builds fine on Windows 10 using mingw64, and I can create a
thread with a name, which 'info threads' in gdb then displays
correctly.

GNU/Linux version pushed to emacs-27.

Robert

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

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: 38632@debbugs.gnu.org,  yantar92@gmail.com
> Date: Mon, 06 Jan 2020 15:43:39 +0100
> 
>     Eli> I'd appreciate testing on 64-bit Windows and in particular on Windows
>     Eli> 10.
> 
> It builds fine on Windows 10 using mingw64, and I can create a
> thread with a name, which 'info threads' in gdb then displays
> correctly.
> 
> GNU/Linux version pushed to emacs-27.

Great, thanks (on both counts).

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

[Mattias Engdegård]

reopen 38632
stop

The committed patch fails to compile on macOS (10.14). Here, and probably BSD in general, pthread_setname_np takes a single argument (the name) and applies to the current thread.

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

[Mattias Engdegård]

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

Here is a quick-and-dirty patch that repairs the macOS/BSD builds. Not tested on anything else.


[-- Attachment #2: pthread-setname.diff --]
[-- Type: application/octet-stream, Size: 5769 bytes --]

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..19fc9e1596 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);
@@ -455,7 +459,11 @@ w32_set_thread_name (DWORD thread_id, const char *name)
 		  (ULONG_PTR *) &tninfo);
 }
 
-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
@@ -463,20 +471,12 @@ w32_set_thread_name (DWORD thread_id, const char *name)
 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;

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

[Robert Pluim]

>>>>> On Mon, 6 Jan 2020 22:58:25 +0100, Mattias Engdegård <mattiase@acm.org> said:

    Mattias> Here is a quick-and-dirty patch that repairs the macOS/BSD builds. Not tested on anything else.

That builds and works fine for me on Fedora. Thanks for fixing that.

Robert

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

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Mon, 6 Jan 2020 22:58:25 +0100
> Cc: Eli Zaretskii <eliz@gnu.org>, Robert Pluim <rpluim@gmail.com>
> 
> Here is a quick-and-dirty patch that repairs the macOS/BSD builds. Not tested on anything else.

Thanks.  This is fine for the emacs-27 branch, but it needs some minor
cleanup:

> -static thread_creation_function *thread_start_address;

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

And this comment on thread.h:

  /* Thread's name in the locale encoding.  Actually used only on
     WINDOWSNT.  */
  char *thread_name;

is no longer accurate, so it needs to be updated to reflect the
modified code.

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

[Mattias Engdegård]

[-- 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)

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

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Tue, 7 Jan 2020 17:46:49 +0100
> Cc: 38632@debbugs.gnu.org, rpluim@gmail.com
> 
> Thanks for the review! Patch updated.

LGTM, thanks.

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

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Tue, 7 Jan 2020 17:46:49 +0100
> Cc: 38632@debbugs.gnu.org, rpluim@gmail.com
> 
> 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.

Yes, that should be a separate cleanup (and yes, we always set it to
the same function, so the race shouldn't matter in practice).

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

[Mattias Engdegård]

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

> Yes, that should be a separate cleanup (and yes, we always set it to
> the same function, so the race shouldn't matter in practice).

Right. Just to be clear, I'm not going to do that since I don't have a Windows development setup.

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

[Glenn Morris]

73fd8a4 fails to build on some GNU/Linux hosts, ref eg

https://hydra.nixos.org/build/109904668
  
  systhread.c:86:1: error: conflicting types for 'sys_thread_create'
   sys_thread_create (sys_thread_t *t, const char *name,
   ^~~~~~~~~~~~~~~~~
  In file included from thread.h:33:0,
                   from lisp.h:2163,
                   from systhread.c:23:
  systhread.h:115:13: note: previous declaration of 'sys_thread_create' was here
   extern bool sys_thread_create (sys_thread_t *, thread_creation_function *,
               ^~~~~~~~~~~~~~~~~
  make[1]: *** [Makefile:402: systhread.o] Error 1

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

[Eli Zaretskii]

> From: Glenn Morris <rgm@gnu.org>
> Cc: Eli Zaretskii <eliz@gnu.org>,  38632@debbugs.gnu.org,  rpluim@gmail.com
> Date: Wed, 08 Jan 2020 13:26:16 -0500
> 
> 
> 73fd8a4 fails to build on some GNU/Linux hosts, ref eg
> 
> https://hydra.nixos.org/build/109904668
>   
>   systhread.c:86:1: error: conflicting types for 'sys_thread_create'
>    sys_thread_create (sys_thread_t *t, const char *name,
>    ^~~~~~~~~~~~~~~~~
>   In file included from thread.h:33:0,
>                    from lisp.h:2163,
>                    from systhread.c:23:
>   systhread.h:115:13: note: previous declaration of 'sys_thread_create' was here
>    extern bool sys_thread_create (sys_thread_t *, thread_creation_function *,
>                ^~~~~~~~~~~~~~~~~
>   make[1]: *** [Makefile:402: systhread.o] Error 1

Thanks, should be fixed now.

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

[Glenn Morris]

Now a different problem:

thread.o: In function `run_thread':
/build/1rjrmrn0sv17hak6ql2igch5f221shf2-source/src/thread.c:729:
undefined reference to `sys_thread_set_name'

Ref: https://hydra.nixos.org/build/109906136

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

[Eli Zaretskii]

> From: Glenn Morris <rgm@gnu.org>
> Cc: mattiase@acm.org,  38632@debbugs.gnu.org,  rpluim@gmail.com
> Date: Wed, 08 Jan 2020 14:34:48 -0500
> 
> Now a different problem:
> 
> thread.o: In function `run_thread':
> /build/1rjrmrn0sv17hak6ql2igch5f221shf2-source/src/thread.c:729:
> undefined reference to `sys_thread_set_name'

Fixed.

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

[Robert Pluim]

>>>>> On Mon, 6 Jan 2020 20:50:44 +0100, Mattias Engdegård <mattiase@acm.org> said:

    Mattias> reopen 38632
    Mattias> stop

    Mattias> The committed patch fails to compile on macOS (10.14). Here, and
    Mattias> probably BSD in general, pthread_setname_np takes a single argument
    Mattias> (the name) and applies to the current thread.

Bother.

Eli, what do you want to do here? I can rework the configure test to
check for the 2-arg version of pthread_setname_np, or we could revert
the change.

Robert