Re: [PATCH glibc] Add pthread_getname_np and pthread_setname_np for Hurd

[Sergey Bugaev]

Hi Flavio,

in general, what's this useful for? Debugging, maybe?

Do you expect the process itself to query its threads' names locally,
or do you expect another process (GDB) to issue thread_get_name
remotely?

On Wed, Jul 10, 2024 at 7:04 PM Flavio Cruz <flaviocruz@gmail.com> wrote:
> diff --git a/htl/pt-internal.h b/htl/pt-internal.h
> index 85a7d905..b6ccbe12 100644
> --- a/htl/pt-internal.h
> +++ b/htl/pt-internal.h
> @@ -105,6 +105,10 @@ struct __pthread
>    /* Initial sigset for the thread.  */
>    sigset_t init_sigset;
>
> +  /* Used to store the thread name through pthread_setname_np.  */
> +  pthread_mutex_t name_lock;

I see other members of struct __pthread are also using
pthread_mutex_t, but is there really a reason why we need that over a
simple libc_lock?

> +  char *thread_name;

Does it make sense to store the name locally? Could pthread_getname_np
call thread_get_name also, if we don't expect it to be a frequent
operation?

Not saying you shouldn't store the name, but let's think of the space
vs performance trade-off and document it.

If you do store it, you should make sure to free it in pt-dealloc.

> +
>    /* Thread context.  */
>    struct pthread_mcontext mcontext;
>
> diff --git a/sysdeps/htl/pthread.h b/sysdeps/htl/pthread.h
> index fa626ebc..cf42383f 100644
> --- a/sysdeps/htl/pthread.h
> +++ b/sysdeps/htl/pthread.h
> @@ -891,6 +891,17 @@ extern int pthread_setschedparam (pthread_t __thr, int 
> __policy,
>  /* Set thread THREAD's scheduling priority.  */
>  extern int pthread_setschedprio (pthread_t __thr, int __prio) __THROW;
>
> +#ifdef __USE_GNU
> +/* Get thread name visible in the kernel and its interfaces.  */
> +extern int pthread_getname_np (pthread_t __target_thread, char *__buf,
> +                              size_t __buflen)
> +     __THROW __nonnull ((2));

__attr_access ((__write_only__, 2, 3)) perhaps?

> +
> +/* Set thread name visible in the kernel and its interfaces.  */
> +extern int pthread_setname_np (pthread_t __target_thread, const char *__name)
> +     __THROW __nonnull ((2));

Same here, __attr_access ((__read_only__, 2))

> diff --git a/sysdeps/mach/htl/pt-setname-np.c 
> b/sysdeps/mach/htl/pt-setname-np.c
> new file mode 100644
> index 00000000..abc20bf5
> --- /dev/null
> +++ b/sysdeps/mach/htl/pt-setname-np.c
> @@ -0,0 +1,66 @@
> +/* pthread_setname_np.  Mach version.
> +   Copyright (C) 2024 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library;  if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <ldsodefs.h>
> +#include <pthread.h>
> +#include <pthreadP.h>
> +#include <string.h>
> +
> +#include <pt-internal.h>
> +#include <hurdlock.h>

What's hurdlock.h for?

> +
> +int
> +__pthread_setname_np (pthread_t thread, const char *name)
> +{
> +#ifdef HAVE_MACH_THREAD_SET_NAME
> +/* Same as the Mach kernel's thread name size.  */
> +#define THREAD_NAME_SIZE 64
> +  struct __pthread *pthread;
> +  char *oldname, *cp, newname[THREAD_NAME_SIZE];
> +
> +  /* Lookup the thread structure for THREAD.  */
> +  pthread = __pthread_getid (thread);
> +  if (pthread == NULL)
> +    return ESRCH;
> +
> +  /* Check for overflow and copy the name into a buffer.  */
> +  if (strlen (name) >= THREAD_NAME_SIZE)
> +    return ERANGE;
> +  strncpy (newname, name, THREAD_NAME_SIZE);
> +
> +  oldname = pthread->thread_name;
> +  cp = strdup (newname);
> +  if (cp == NULL)
> +    return ENOMEM;
> +
> +  __pthread_mutex_lock (&pthread->name_lock);
> +  oldname = pthread->thread_name;
> +  pthread->thread_name = cp;
> +  __thread_set_name (pthread->kernel_thread, cp);

No checking the return value?

> +  __pthread_mutex_unlock (&pthread->name_lock);
> +
> +  if (oldname != NULL)
> +    free (oldname);

Nitpick: no need to check for NULL before freeing.

Sergey