Re: [PATCH v5 05/13] string: Remove old TLS usage on strsignal

[Carlos O'Donell <carlos@redhat.com>]

On 6/19/20 9:43 AM, Adhemerval Zanella wrote:
> The per-thread state is refactored two use two strategies:
> 
>   1. The default one uses a TLS structure, which will be place in the
>      static TLS space (using __thread keyword).

s/place/placed/g

OK.
 
>   2. Linux allocates on struct pthread and access it through THREAD_*
>      macros.

s/on/via/g

OK.

 
> The default strategy has the disadvantage of increasing libc.so static
> TLS consumption and thus descreasing the possible surplus used in
> some scenarios (which might be mitigated by BZ#25051 fix).

s/descreasing/decreasing/g

> 
> It is used only on Hurd, where accessing the thread point in single

s/point in/storage in the/g

> thread case is not straightforward (afaiu, Hurd developers could
> correct me here).
> 
> The fallback static allocation used for allocation failure is also
> removed: defining its size is problematic without synchronize with

s/synchronize/synchronizing/g

> translated messages (to avoid partial translation) and the resulting
> usage is not thread-safe.

OK for master with fixed up commit message.

Framework seems sensible and we might extend it to include
more data if we need to. The existing use of __libc_key_create is a little
heavy for just a string, and the complex shutdown through a stored function
pointer is always difficult to debug. It is easier as you have it to just
free the memory during the thread's freeres.

No regressions on x86_64 and i686.

Tested-by: Carlos O'Donell <carlos@redhat.com>
Reviewed-by: Carlos O'Donell <carlos@redhat.com>
 
> Checked on x86-64-linux-gnu, i686-linux-gnu, powerpc64le-linux-gnu,
> and s390x-linux-gnu.
> ---
>  malloc/thread-freeres.c                |   2 +
>  nptl/allocatestack.c                   |   2 +
>  nptl/descr.h                           |   4 +
>  string/strsignal.c                     | 112 +++++--------------------
>  sysdeps/generic/Makefile               |   1 +
>  sysdeps/generic/tls-internal-struct.h  |  27 ++++++
>  sysdeps/generic/tls-internal.c         |  21 +++++
>  sysdeps/generic/tls-internal.h         |  39 +++++++++
>  sysdeps/unix/sysv/linux/tls-internal.c |   1 +
>  sysdeps/unix/sysv/linux/tls-internal.h |  37 ++++++++
>  10 files changed, 155 insertions(+), 91 deletions(-)
>  create mode 100644 sysdeps/generic/tls-internal-struct.h
>  create mode 100644 sysdeps/generic/tls-internal.c
>  create mode 100644 sysdeps/generic/tls-internal.h
>  create mode 100644 sysdeps/unix/sysv/linux/tls-internal.c
>  create mode 100644 sysdeps/unix/sysv/linux/tls-internal.h
> 
> diff --git a/malloc/thread-freeres.c b/malloc/thread-freeres.c
> index c71ca4fc33..3408bdbefd 100644
> --- a/malloc/thread-freeres.c
> +++ b/malloc/thread-freeres.c
> @@ -21,6 +21,7 @@
>  #include <resolv/resolv-internal.h>
>  #include <rpc/rpc.h>
>  #include <string.h>
> +#include <tls-internal.h>

OK.

>  
>  /* Thread shutdown function.  Note that this function must be called
>     for threads during shutdown for correctness reasons.  Unlike
> @@ -32,6 +33,7 @@ __libc_thread_freeres (void)
>    call_function_static_weak (__rpc_thread_destroy);
>    call_function_static_weak (__res_thread_freeres);
>    call_function_static_weak (__strerror_thread_freeres);
> +  __glibc_tls_internal_free ();

OK. We remove the use of __libc_key_create and instead of the destructor 
used there we use the thread shutdown __libc_thread_freeres to free it.

>  
>    /* This should come last because it shuts down malloc for this
>       thread and the other shutdown functions might well call free.  */
> diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
> index d16f3d71f8..4ae4b5a986 100644
> --- a/nptl/allocatestack.c
> +++ b/nptl/allocatestack.c
> @@ -237,6 +237,8 @@ get_cached_stack (size_t *sizep, void **memp)
>    /* No pending event.  */
>    result->nextevent = NULL;
>  
> +  result->tls_state = (struct tls_internal_t) { 0 };

OK.

> +
>    /* Clear the DTV.  */
>    dtv_t *dtv = GET_DTV (TLS_TPADJ (result));
>    for (size_t cnt = 0; cnt < dtv[-1].counter; ++cnt)
> diff --git a/nptl/descr.h b/nptl/descr.h
> index e1c7db5473..6a509b6725 100644
> --- a/nptl/descr.h
> +++ b/nptl/descr.h
> @@ -34,6 +34,7 @@
>  #include <unwind.h>
>  #include <bits/types/res_state.h>
>  #include <kernel-features.h>
> +#include <tls-internal-struct.h>

OK.

>  
>  #ifndef TCB_ALIGNMENT
>  # define TCB_ALIGNMENT	sizeof (double)
> @@ -398,6 +399,9 @@ struct pthread
>    /* Indicates whether is a C11 thread created by thrd_creat.  */
>    bool c11;
>  
> +  /* Used on strsignal.  */
> +  struct tls_internal_t tls_state;

OK.

> +
>    /* This member must be last.  */
>    char end_padding[];
>  
> diff --git a/string/strsignal.c b/string/strsignal.c
> index 7e3b262c55..701ce20e6e 100644
> --- a/string/strsignal.c
> +++ b/string/strsignal.c
> @@ -20,106 +20,36 @@
>  #include <stdlib.h>
>  #include <string.h>
>  #include <libintl.h>
> -#include <libc-lock.h>
> -
> -static __libc_key_t key;
> -
> -/* If nonzero the key allocation failed and we should better use a
> -   static buffer than fail.  */
> -#define BUFFERSIZ	100
> -static char local_buf[BUFFERSIZ];
> -static char *static_buf;

OK. Good to see this go.

> -
> -/* Destructor for the thread-specific data.  */
> -static void init (void);
> -static void free_key_mem (void *mem);
> -static char *getbuffer (void);
> -
> +#include <tls-internal.h>
> +#include <array_length.h>
>  
>  /* Return a string describing the meaning of the signal number SIGNUM.  */
>  char *
>  strsignal (int signum)
>  {
> -  __libc_once_define (static, once);
> -  const char *desc;
> -
> -  /* If we have not yet initialized the buffer do it now.  */
> -  __libc_once (once, init);
> -
> -  if (
> -#ifdef SIGRTMIN
> -      (signum >= SIGRTMIN && signum <= SIGRTMAX) ||
> -#endif
> -      signum < 0 || signum >= NSIG
> -      || (desc = __sys_siglist[signum]) == NULL)
> -    {
> -      char *buffer = getbuffer ();
> -      int len;
> -#ifdef SIGRTMIN
> -      if (signum >= SIGRTMIN && signum <= SIGRTMAX)
> -	len = __snprintf (buffer, BUFFERSIZ - 1, _("Real-time signal %d"),
> -			  signum - SIGRTMIN);
> -      else
> -#endif
> -	len = __snprintf (buffer, BUFFERSIZ - 1, _("Unknown signal %d"),
> -			  signum);
> -      if (len >= BUFFERSIZ)
> -	buffer = NULL;
> -      else
> -	buffer[len] = '\0';
> -
> -      return buffer;
> -    }
> -
> -  return (char *) _(desc);
> -}
> -
> -
> -/* Initialize buffer.  */
> -static void
> -init (void)
> -{
> -  if (__libc_key_create (&key, free_key_mem))
> -    /* Creating the key failed.  This means something really went
> -       wrong.  In any case use a static buffer which is better than
> -       nothing.  */
> -    static_buf = local_buf;
> -}
> -
> +  const char *desc = NULL;
>  
> -/* Free the thread specific data, this is done if a thread terminates.  */
> -static void
> -free_key_mem (void *mem)
> -{
> -  free (mem);
> -  __libc_setspecific (key, NULL);
> -}
> +  if (signum >= 0 && signum <= NSIG && signum < array_length (__sys_siglist))
> +    desc = __sys_siglist[signum];

OK.

>  
> +  if (desc != NULL)
> +    return (char *) _(desc);

OK.

>  
> -/* Return the buffer to be used.  */
> -static char *
> -getbuffer (void)
> -{
> -  char *result;
> +  struct tls_internal_t *tls_internal = __glibc_tls_internal ();
> +  free (tls_internal->strsignal_buf);
>  
> -  if (static_buf != NULL)
> -    result = static_buf;
> +  int r;
> +#ifdef SIGRTMIN
> +  if (signum >= SIGRTMIN && signum <= SIGRTMAX)
> +    r = __asprintf (&tls_internal->strsignal_buf, _("Real-time signal %d"),
> +		    signum - SIGRTMIN);

OK. Use asprintf which will call malloc.

>    else
> -    {
> -      /* We don't use the static buffer and so we have a key.  Use it
> -	 to get the thread-specific buffer.  */
> -      result = __libc_getspecific (key);
> -      if (result == NULL)
> -	{
> -	  /* No buffer allocated so far.  */
> -	  result = malloc (BUFFERSIZ);
> -	  if (result == NULL)
> -	    /* No more memory available.  We use the static buffer.  */
> -	    result = local_buf;
> -	  else
> -	    __libc_setspecific (key, result);
> -	}
> -    }
> +#endif
> +    r = __asprintf (&tls_internal->strsignal_buf, _("Unknown signal %d"),
> +		    signum);

OK. Use asptrintf which will call malloc.

> +
> +  if (r == -1)
> +    tls_internal->strsignal_buf = NULL;
>  
> -  return result;
> +  return tls_internal->strsignal_buf;

OK.

>  }
> diff --git a/sysdeps/generic/Makefile b/sysdeps/generic/Makefile
> index bd4f9425ca..1240d99436 100644
> --- a/sysdeps/generic/Makefile
> +++ b/sysdeps/generic/Makefile
> @@ -16,6 +16,7 @@
>  # <https://www.gnu.org/licenses/>.
>  
>  ifeq ($(subdir),string)
> +sysdep_routines += tls-internal

OK.

>  CFLAGS-wordcopy.c += -Wno-uninitialized
>  endif
>  
> diff --git a/sysdeps/generic/tls-internal-struct.h b/sysdeps/generic/tls-internal-struct.h
> new file mode 100644
> index 0000000000..33a9079ee9
> --- /dev/null
> +++ b/sysdeps/generic/tls-internal-struct.h
> @@ -0,0 +1,27 @@
> +/* Per-thread state.  Generic version.
> +   Copyright (C) 2020 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/>.  */
> +
> +#ifndef _TLS_INTERNAL_STRUCT_H
> +#define _TLS_INTERNAL_STRUCT_H 1
> +
> +struct tls_internal_t
> +{
> +  char *strsignal_buf;
> +};

OK.

> +
> +#endif
> diff --git a/sysdeps/generic/tls-internal.c b/sysdeps/generic/tls-internal.c
> new file mode 100644
> index 0000000000..14b914e5f3
> --- /dev/null
> +++ b/sysdeps/generic/tls-internal.c
> @@ -0,0 +1,21 @@
> +/* Per-thread state.  Generic version.
> +   Copyright (C) 2020 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 <tls-internal.h>
> +
> +__thread struct tls_internal_t __tls_internal;

OK.

> diff --git a/sysdeps/generic/tls-internal.h b/sysdeps/generic/tls-internal.h
> new file mode 100644
> index 0000000000..1f6a117d76
> --- /dev/null
> +++ b/sysdeps/generic/tls-internal.h
> @@ -0,0 +1,39 @@
> +/* Per-thread state.  Generic version.
> +   Copyright (C) 2020 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/>.  */
> +
> +#ifndef _TLS_INTERNAL_H
> +#define _TLS_INTERNAL_H 1
> +
> +#include <stdlib.h>
> +#include <tls-internal-struct.h>
> +
> +extern __thread struct tls_internal_t __tls_internal attribute_hidden;

OK. Generic implementation uses __thread to store the structure.

> +
> +static inline struct tls_internal_t *
> +__glibc_tls_internal (void)
> +{
> +  return &__tls_internal;
> +}

OK.

> +
> +static inline void
> +__glibc_tls_internal_free (void)
> +{
> +  free (__tls_internal.strsignal_buf);
> +}

OK.

> +
> +#endif
> diff --git a/sysdeps/unix/sysv/linux/tls-internal.c b/sysdeps/unix/sysv/linux/tls-internal.c
> new file mode 100644
> index 0000000000..6e25b021ab
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/tls-internal.c
> @@ -0,0 +1 @@
> +/* Empty.  */
> diff --git a/sysdeps/unix/sysv/linux/tls-internal.h b/sysdeps/unix/sysv/linux/tls-internal.h
> new file mode 100644
> index 0000000000..5d712abd4a
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/tls-internal.h
> @@ -0,0 +1,37 @@
> +/* Per-thread state.  Linux version.
> +   Copyright (C) 2020 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/>.  */
> +
> +#ifndef _TLS_INTERNAL_H
> +#define _TLS_INTERNAL_H 1
> +
> +#include <stdlib.h>
> +#include <nptl/pthreadP.h>
> +
> +static inline struct tls_internal_t *
> +__glibc_tls_internal (void)
> +{
> +  return &THREAD_SELF->tls_state;
> +}
> +
> +static inline void
> +__glibc_tls_internal_free (void)
> +{
> +  free (THREAD_SELF->tls_state.strsignal_buf);
> +}

OK.

> +
> +#endif
> 


-- 
Cheers,
Carlos.