Re: [RFC PATCH glibc 24/34] hurd: Only check for TLS initialization inside rtld or in static builds

[Samuel Thibault <samuel.thibault@gnu.org>]

Applied, thanks!

Sergey Bugaev, le dim. 19 mars 2023 18:10:07 +0300, a ecrit:
> When glibc is built as a shared library, TLS is always initialized by
> the call of TLS_INIT_TP () macro made inside the dynamic loader, prior
> to running the main program (see dl-call_tls_init_tp.h). We can take
> advantage of this: we know for sure that __LIBC_NO_TLS () will evaluate
> to 0 in all other cases, so let the compiler know that explicitly too.
> 
> Also, only define _hurd_tls_init () and TLS_INIT_TP () under the same
> conditions (either !SHARED or inside rtld), to statically assert that
> this is the case.
> 
> Other than a microoptimization, this also helps with avoiding awkward
> sharing of the __libc_tls_initialized variable between ld.so and libc.so
> that we would have to do otherwise -- we know for sure that no sharing
> is required, simply because __libc_tls_initialized would always be set
> to true inside libc.so.
> 
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
> ---
>  sysdeps/mach/hurd/Makefile                    |  4 ++
>  sysdeps/mach/hurd/i386/dl-tls-initialized.c   | 21 +++++++++
>  sysdeps/mach/hurd/i386/tls.h                  | 43 +++++++++++--------
>  sysdeps/mach/hurd/x86/init-first.c            | 11 +----
>  sysdeps/mach/hurd/x86_64/dl-tls-initialized.c | 21 +++++++++
>  sysdeps/mach/hurd/x86_64/tls.h                | 19 +++++---
>  6 files changed, 85 insertions(+), 34 deletions(-)
>  create mode 100644 sysdeps/mach/hurd/i386/dl-tls-initialized.c
>  create mode 100644 sysdeps/mach/hurd/x86_64/dl-tls-initialized.c
> 
> diff --git a/sysdeps/mach/hurd/Makefile b/sysdeps/mach/hurd/Makefile
> index d5584930..f43e92ba 100644
> --- a/sysdeps/mach/hurd/Makefile
> +++ b/sysdeps/mach/hurd/Makefile
> @@ -197,6 +197,10 @@ ifeq (hurd, $(subdir))
>  sysdep_routines += cthreads
>  endif
>  
> +ifeq (elf, $(subdir))
> +sysdep-dl-routines += dl-tls-initialized
> +endif
> +
>  ifeq (io, $(subdir))
>  sysdep_routines += f_setlk close_nocancel close_nocancel_nostatus \
>  		   fcntl_nocancel open_nocancel openat_nocancel read_nocancel \
> diff --git a/sysdeps/mach/hurd/i386/dl-tls-initialized.c b/sysdeps/mach/hurd/i386/dl-tls-initialized.c
> new file mode 100644
> index 00000000..493ec239
> --- /dev/null
> +++ b/sysdeps/mach/hurd/i386/dl-tls-initialized.c
> @@ -0,0 +1,21 @@
> +/* Determine whether TLS is initialized, for i386/Hurd.
> +   Copyright (C) 1995-2023 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 SHARED
> +unsigned short __init1_desc;
> +#endif
> diff --git a/sysdeps/mach/hurd/i386/tls.h b/sysdeps/mach/hurd/i386/tls.h
> index 0f8dd241..ee7b8004 100644
> --- a/sysdeps/mach/hurd/i386/tls.h
> +++ b/sysdeps/mach/hurd/i386/tls.h
> @@ -69,18 +69,6 @@ _Static_assert (offsetof (tcbhead_t, __private_ss) == 0x30,
>                    |  (desc->high_word & 0xff000000));			      \
>    })
>  
> -/* Return 1 if TLS is not initialized yet.  */
> -#ifndef SHARED
> -extern unsigned short __init1_desc;
> -#define __HURD_DESC_INITIAL(gs, ds) ((gs) == (ds) || (gs) == __init1_desc)
> -#else
> -#define __HURD_DESC_INITIAL(gs, ds) ((gs) == (ds))
> -#endif
> -
> -#define __LIBC_NO_TLS()							      \
> -  ({ unsigned short ds, gs;						      \
> -     asm ("movw %%ds,%w0; movw %%gs,%w1" : "=q" (ds), "=q" (gs));	      \
> -     __builtin_expect(__HURD_DESC_INITIAL(gs, ds), 0); })
>  #endif
>  
>  /* The TCB can have any size and the memory following the address the
> @@ -125,6 +113,28 @@ extern unsigned short __init1_desc;
>  
>  # define HURD_SEL_LDT(sel) (__builtin_expect ((sel) & 4, 0))
>  
> +#ifndef SHARED
> +extern unsigned short __init1_desc;
> +# define __HURD_DESC_INITIAL(gs, ds) ((gs) == (ds) || (gs) == __init1_desc)
> +#else
> +# define __HURD_DESC_INITIAL(gs, ds) ((gs) == (ds))
> +#endif
> +
> +#if !defined (SHARED) || IS_IN (rtld)
> +/* Return 1 if TLS is not initialized yet.  */
> +extern inline bool __attribute__ ((unused))
> +__LIBC_NO_TLS (void)
> +{
> +  unsigned short ds, gs;
> +  asm ("movw %%ds, %w0\n"
> +       "movw %%gs, %w1"
> +       : "=q" (ds), "=q" (gs));
> +  return __glibc_unlikely (__HURD_DESC_INITIAL (gs, ds));
> +}
> +
> +/* Code to initially initialize the thread pointer.  This might need
> +   special attention since 'errno' is not yet available and if the
> +   operation can cause a failure 'errno' must not be touched.  */
>  static inline bool __attribute__ ((unused))
>  _hurd_tls_init (tcbhead_t *tcb)
>  {
> @@ -168,11 +178,10 @@ out:
>    return success;
>  }
>  
> -/* Code to initially initialize the thread pointer.  This might need
> -   special attention since 'errno' is not yet available and if the
> -   operation can cause a failure 'errno' must not be touched.  */
> -# define TLS_INIT_TP(descr) \
> -    _hurd_tls_init ((tcbhead_t *) (descr))
> +# define TLS_INIT_TP(descr) _hurd_tls_init ((tcbhead_t *) (descr))
> +#else /* defined (SHARED) && !IS_IN (rtld) */
> +# define __LIBC_NO_TLS() 0
> +#endif
>  
>  # if __GNUC_PREREQ (6, 0)
>  
> diff --git a/sysdeps/mach/hurd/x86/init-first.c b/sysdeps/mach/hurd/x86/init-first.c
> index 48c330ec..89a5f44c 100644
> --- a/sysdeps/mach/hurd/x86/init-first.c
> +++ b/sysdeps/mach/hurd/x86/init-first.c
> @@ -40,13 +40,6 @@ extern char **_dl_argv;
>  
>  #ifndef SHARED
>  static tcbhead_t __init1_tcbhead;
> -# ifndef __x86_64__
> -unsigned short __init1_desc;
> -# endif
> -#endif
> -
> -#ifdef __x86_64__
> -unsigned char __libc_tls_initialized;
>  #endif
>  
>  /* Things that want to be run before _hurd_init or much anything else.
> @@ -166,9 +159,7 @@ first_init (void)
>    _hurd_tls_init (&__init1_tcbhead);
>  
>    /* Make sure __LIBC_NO_TLS () keeps evaluating to 1.  */
> -# ifdef __x86_64__
> -  __libc_tls_initialized = 0;
> -# else
> +# ifndef __x86_64__
>    asm ("movw %%gs,%w0" : "=m" (__init1_desc));
>  # endif
>  #endif
> diff --git a/sysdeps/mach/hurd/x86_64/dl-tls-initialized.c b/sysdeps/mach/hurd/x86_64/dl-tls-initialized.c
> new file mode 100644
> index 00000000..d0766f95
> --- /dev/null
> +++ b/sysdeps/mach/hurd/x86_64/dl-tls-initialized.c
> @@ -0,0 +1,21 @@
> +/* Determine whether TLS is initialized, for x86_64/Hurd.
> +   Copyright (C) 1995-2023 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/>.  */
> +
> +#if !defined (SHARED) || IS_IN (rtld)
> +unsigned short __libc_tls_initialized;
> +#endif
> diff --git a/sysdeps/mach/hurd/x86_64/tls.h b/sysdeps/mach/hurd/x86_64/tls.h
> index cf74e1f4..da504d9c 100644
> --- a/sysdeps/mach/hurd/x86_64/tls.h
> +++ b/sysdeps/mach/hurd/x86_64/tls.h
> @@ -68,10 +68,6 @@ _Static_assert (offsetof (tcbhead_t, stack_guard) == 0x28,
>  _Static_assert (offsetof (tcbhead_t, __private_ss) == 0x70,
>                  "split stack pointer offset");
>  
> -extern unsigned char __libc_tls_initialized;
> -
> -# define __LIBC_NO_TLS() __builtin_expect (!__libc_tls_initialized, 0)
> -
>  /* The TCB can have any size and the memory following the address the
>     thread pointer points to is unspecified.  Allocate the TCB there.  */
>  # define TLS_TCB_AT_TP	1
> @@ -87,8 +83,6 @@ extern unsigned char __libc_tls_initialized;
>  # define TCB_ALIGNMENT	64
>  
>  
> -# define TLS_INIT_TP(descr) _hurd_tls_init ((tcbhead_t *) (descr))
> -
>  # define THREAD_SELF							\
>    (*(tcbhead_t * __seg_fs *) offsetof (tcbhead_t, tcb))
>  /* Read member of the thread descriptor directly.  */
> @@ -174,6 +168,10 @@ _hurd_tls_new (thread_t child, tcbhead_t *tcb)
>                               i386_FSGS_BASE_STATE_COUNT);
>  }
>  
> +# if !defined (SHARED) || IS_IN (rtld)
> +extern unsigned char __libc_tls_initialized;
> +#  define __LIBC_NO_TLS() __builtin_expect (!__libc_tls_initialized, 0)
> +
>  static inline bool __attribute__ ((unused))
>  _hurd_tls_init (tcbhead_t *tcb)
>  {
> @@ -184,11 +182,18 @@ _hurd_tls_init (tcbhead_t *tcb)
>    tcb->multiple_threads = 1;
>  
>    err = _hurd_tls_new (self, tcb);
> +  if (err == 0)
> +    __libc_tls_initialized = 1;
>    __mach_port_deallocate (__mach_task_self (), self);
> -  __libc_tls_initialized = 1;
>    return err == 0;
>  }
>  
> +#  define TLS_INIT_TP(descr) _hurd_tls_init ((tcbhead_t *) (descr))
> +# else /* defined (SHARED) && !IS_IN (rtld) */
> +#  define __LIBC_NO_TLS() 0
> +# endif
> +
> +
>  
>  /* Global scope switch support.  */
>  # define THREAD_GSCOPE_FLAG_UNUSED 0
> -- 
> 2.39.2
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.