[v2,5/5] Linux: Consolidate auxiliary vector parsing

  And optimize it slightly.

The large switch statement in _dl_sysdep_start can be replaced with
a large array.  This reduces source code and binary size.  On
i686-linux-gnu:

Before:

   text	   data	    bss	    dec	    hex	filename
   7791	     12	      0	   7803	   1e7b	elf/dl-sysdep.os

After:

   text	   data	    bss	    dec	    hex	filename
   7135	     12	      0	   7147	   1beb	elf/dl-sysdep.os
---
 elf/dl-support.c                             |  80 +------------
 sysdeps/unix/sysv/linux/alpha/dl-auxv.h      |  18 +--
 sysdeps/unix/sysv/linux/dl-parse_auxv.h      |  61 ++++++++++
 sysdeps/unix/sysv/linux/dl-sysdep.c          | 111 ++++++-------------
 sysdeps/unix/sysv/linux/powerpc/dl-auxv.h    |  14 +--
 sysdeps/unix/sysv/linux/powerpc/dl-support.c |   4 +
 6 files changed, 107 insertions(+), 181 deletions(-)
 create mode 100644 sysdeps/unix/sysv/linux/dl-parse_auxv.h
 create mode 100644 sysdeps/unix/sysv/linux/powerpc/dl-support.c
  

On 03/02/2022 08:08, Florian Weimer via Libc-alpha wrote:
> And optimize it slightly.
> 
> The large switch statement in _dl_sysdep_start can be replaced with
> a large array.  This reduces source code and binary size.  On
> i686-linux-gnu:
> 
> Before:
> 
>    text	   data	    bss	    dec	    hex	filename
>    7791	     12	      0	   7803	   1e7b	elf/dl-sysdep.os
> 
> After:
> 
>    text	   data	    bss	    dec	    hex	filename
>    7135	     12	      0	   7147	   1beb	elf/dl-sysdep.os

LGTM with a couple comments below.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  elf/dl-support.c                             |  80 +------------
>  sysdeps/unix/sysv/linux/alpha/dl-auxv.h      |  18 +--
>  sysdeps/unix/sysv/linux/dl-parse_auxv.h      |  61 ++++++++++
>  sysdeps/unix/sysv/linux/dl-sysdep.c          | 111 ++++++-------------
>  sysdeps/unix/sysv/linux/powerpc/dl-auxv.h    |  14 +--
>  sysdeps/unix/sysv/linux/powerpc/dl-support.c |   4 +
>  6 files changed, 107 insertions(+), 181 deletions(-)
>  create mode 100644 sysdeps/unix/sysv/linux/dl-parse_auxv.h
>  create mode 100644 sysdeps/unix/sysv/linux/powerpc/dl-support.c
> 
> diff --git a/elf/dl-support.c b/elf/dl-support.c
> index fb64765537..0fff62064a 100644
> --- a/elf/dl-support.c
> +++ b/elf/dl-support.c
> @@ -241,93 +241,21 @@ __rtld_lock_define_initialized_recursive (, _dl_load_tls_lock)
>  
>  
>  #ifdef HAVE_AUX_VECTOR
> +#include <dl-parse_auxv.h>
> +
>  int _dl_clktck;
>  
>  void
>  _dl_aux_init (ElfW(auxv_t) *av)
>  {
> -  int seen = 0;
> -  uid_t uid = 0;
> -  gid_t gid = 0;
> -
>  #ifdef NEED_DL_SYSINFO
>    /* NB: Avoid RELATIVE relocation in static PIE.  */
>    GL(dl_sysinfo) = DL_SYSINFO_DEFAULT;
>  #endif
>  
>    _dl_auxv = av;
> -  for (; av->a_type != AT_NULL; ++av)
> -    switch (av->a_type)
> -      {
> -      case AT_PAGESZ:
> -	if (av->a_un.a_val != 0)
> -	  GLRO(dl_pagesize) = av->a_un.a_val;
> -	break;
> -      case AT_CLKTCK:
> -	GLRO(dl_clktck) = av->a_un.a_val;
> -	break;
> -      case AT_PHDR:
> -	GL(dl_phdr) = (const void *) av->a_un.a_val;
> -	break;
> -      case AT_PHNUM:
> -	GL(dl_phnum) = av->a_un.a_val;
> -	break;
> -      case AT_PLATFORM:
> -	GLRO(dl_platform) = (void *) av->a_un.a_val;
> -	break;
> -      case AT_HWCAP:
> -	GLRO(dl_hwcap) = (unsigned long int) av->a_un.a_val;
> -	break;
> -      case AT_HWCAP2:
> -	GLRO(dl_hwcap2) = (unsigned long int) av->a_un.a_val;
> -	break;
> -      case AT_FPUCW:
> -	GLRO(dl_fpu_control) = av->a_un.a_val;
> -	break;
> -#ifdef NEED_DL_SYSINFO
> -      case AT_SYSINFO:
> -	GL(dl_sysinfo) = av->a_un.a_val;
> -	break;
> -#endif
> -#ifdef NEED_DL_SYSINFO_DSO
> -      case AT_SYSINFO_EHDR:
> -	GL(dl_sysinfo_dso) = (void *) av->a_un.a_val;
> -	break;
> -#endif
> -      case AT_UID:
> -	uid ^= av->a_un.a_val;
> -	seen |= 1;
> -	break;
> -      case AT_EUID:
> -	uid ^= av->a_un.a_val;
> -	seen |= 2;
> -	break;
> -      case AT_GID:
> -	gid ^= av->a_un.a_val;
> -	seen |= 4;
> -	break;
> -      case AT_EGID:
> -	gid ^= av->a_un.a_val;
> -	seen |= 8;
> -	break;
> -      case AT_SECURE:
> -	seen = -1;
> -	__libc_enable_secure = av->a_un.a_val;
> -	__libc_enable_secure_decided = 1;
> -	break;
> -      case AT_RANDOM:
> -	_dl_random = (void *) av->a_un.a_val;
> -	break;
> -      case AT_MINSIGSTKSZ:
> -	_dl_minsigstacksize = av->a_un.a_val;
> -	break;
> -      DL_PLATFORM_AUXV
> -      }
> -  if (seen == 0xf)
> -    {
> -      __libc_enable_secure = uid != 0 || gid != 0;
> -      __libc_enable_secure_decided = 1;
> -    }
> +  dl_parse_auxv_t auxv_values = { 0, };
> +  _dl_parse_auxv (av, auxv_values);
>  }
>  #endif
>  
> diff --git a/sysdeps/unix/sysv/linux/alpha/dl-auxv.h b/sysdeps/unix/sysv/linux/alpha/dl-auxv.h
> index 81d90da095..fcec743239 100644
> --- a/sysdeps/unix/sysv/linux/alpha/dl-auxv.h
> +++ b/sysdeps/unix/sysv/linux/alpha/dl-auxv.h
> @@ -20,16 +20,8 @@
>  
>  extern long __libc_alpha_cache_shape[4];
>  
> -#define DL_PLATFORM_AUXV				\
> -      case AT_L1I_CACHESHAPE:				\
> -	__libc_alpha_cache_shape[0] = av->a_un.a_val;	\
> -	break;						\
> -      case AT_L1D_CACHESHAPE:				\
> -	__libc_alpha_cache_shape[1] = av->a_un.a_val;	\
> -	break;						\
> -      case AT_L2_CACHESHAPE:				\
> -	__libc_alpha_cache_shape[2] = av->a_un.a_val;	\
> -	break;						\
> -      case AT_L3_CACHESHAPE:				\
> -	__libc_alpha_cache_shape[3] = av->a_un.a_val;	\
> -	break;
> +#define DL_PLATFORM_AUXV					\
> +  __libc_alpha_cache_shape[0] = auxv_values[AT_L1I_CACHESHAPE]; \
> +  __libc_alpha_cache_shape[1] = auxv_values[AT_L1D_CACHESHAPE]; \
> +  __libc_alpha_cache_shape[2] = auxv_values[AT_L2_CACHESHAPE];	\
> +  __libc_alpha_cache_shape[3] = auxv_values[AT_L3_CACHESHAPE];
> diff --git a/sysdeps/unix/sysv/linux/dl-parse_auxv.h b/sysdeps/unix/sysv/linux/dl-parse_auxv.h
> new file mode 100644
> index 0000000000..f450c6c5ce
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/dl-parse_auxv.h
> @@ -0,0 +1,61 @@
> +/* Parse the Linux auxiliary vector.
> +   Copyright (C) 1995-2022 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 <elf.h>
> +#include <entry.h>
> +#include <fpu_control.h>
> +#include <ldsodefs.h>
> +#include <link.h>
> +
> +typedef ElfW(Addr) dl_parse_auxv_t[AT_MINSIGSTKSZ + 1];
> +
> +/* Copy the auxiliary vector into AUX_VALUES and set up GLRO
> +   variables.  */
> +static inline
> +void _dl_parse_auxv (ElfW(auxv_t) *av, dl_parse_auxv_t auxv_values)
> +{
> +  auxv_values[AT_ENTRY] =  (ElfW(Addr)) ENTRY_POINT;

Extra whitespace.

> +  auxv_values[AT_PAGESZ] = EXEC_PAGESIZE;
> +  auxv_values[AT_FPUCW] = _FPU_DEFAULT;
> +
> +  /* NB: Default to a constant CONSTANT_MINSIGSTKSZ.  */
> +  _Static_assert (__builtin_constant_p (CONSTANT_MINSIGSTKSZ),
> +                  "CONSTANT_MINSIGSTKSZ is constant");

Shouldn't it warn that CONSTANT_MINSIGSTKSZ is not a constant?

> +  auxv_values[AT_MINSIGSTKSZ] = CONSTANT_MINSIGSTKSZ;
> +
> +  for (; av->a_type != AT_NULL; av++)
> +    if (av->a_type <= AT_MINSIGSTKSZ)
> +      auxv_values[av->a_type] = av->a_un.a_val;
> +
> +  GLRO(dl_pagesize) = auxv_values[AT_PAGESZ];
> +  __libc_enable_secure = auxv_values[AT_SECURE];
> +  GLRO(dl_platform) = (void *) auxv_values[AT_PLATFORM];
> +  GLRO(dl_hwcap) = auxv_values[AT_HWCAP];
> +  GLRO(dl_hwcap2) = auxv_values[AT_HWCAP2];
> +  GLRO(dl_clktck) = auxv_values[AT_CLKTCK];
> +  GLRO(dl_fpu_control) = auxv_values[AT_FPUCW];
> +  _dl_random = (void *) auxv_values[AT_RANDOM];
> +  GLRO(dl_minsigstacksize) = auxv_values[AT_MINSIGSTKSZ];
> +  GLRO(dl_sysinfo_dso) = (void *) auxv_values[AT_SYSINFO_EHDR];
> +#ifdef NEED_DL_SYSINFO

I think you can now assume NEED_DL_SYSINFO is always defined.

> +  if (GLRO(dl_sysinfo_dso) != NULL)
> +    GLRO(dl_sysinfo) = auxv_values[AT_SYSINFO];
> +#endif
> +
> +  DL_PLATFORM_AUXV
> +}
> diff --git a/sysdeps/unix/sysv/linux/dl-sysdep.c b/sysdeps/unix/sysv/linux/dl-sysdep.c
> index 2bda76b820..71cf1968b5 100644
> --- a/sysdeps/unix/sysv/linux/dl-sysdep.c
> +++ b/sysdeps/unix/sysv/linux/dl-sysdep.c
> @@ -21,13 +21,12 @@
>  #include <dl-auxv.h>
>  #include <dl-hwcap-check.h>
>  #include <dl-osinfo.h>
> +#include <dl-parse_auxv.h>
>  #include <dl-procinfo.h>
>  #include <dl-tunables.h>
>  #include <elf.h>
> -#include <entry.h>
>  #include <errno.h>
>  #include <fcntl.h>
> -#include <fpu_control.h>
>  #include <ldsodefs.h>
>  #include <libc-internal.h>
>  #include <libintl.h>
> @@ -63,24 +62,24 @@ void *_dl_random attribute_relro = NULL;
>  # define DL_STACK_END(cookie) ((void *) (cookie))
>  #endif
>  
> -ElfW(Addr)
> -_dl_sysdep_start (void **start_argptr,
> -		  void (*dl_main) (const ElfW(Phdr) *phdr, ElfW(Word) phnum,
> -				   ElfW(Addr) *user_entry, ElfW(auxv_t) *auxv))
> +/* Arguments passed to dl_main.  */
> +struct dl_main_arguments
>  {
> -  const ElfW(Phdr) *phdr = NULL;
> -  ElfW(Word) phnum = 0;
> +  const ElfW(Phdr) *phdr;
> +  ElfW(Word) phnum;
>    ElfW(Addr) user_entry;
> -  ElfW(auxv_t) *av;
> -#ifdef NEED_DL_SYSINFO
> -  uintptr_t new_sysinfo = 0;
> -#endif
> +};
>  
> -  __libc_stack_end = DL_STACK_END (start_argptr);
> +/* Separate function, so that dl_main can be called without the large
> +   array on the stack.  */
> +static void
> +_dl_sysdep_parse_arguments (void **start_argptr,
> +			    struct dl_main_arguments *args)
> +{
>    _dl_argc = (intptr_t) *start_argptr;
>    _dl_argv = (char **) (start_argptr + 1); /* Necessary aliasing violation.  */
>    _environ = _dl_argv + _dl_argc + 1;
> -  for (char **tmp = _environ + 1; ; ++tmp)
> +  for (char **tmp = _environ; ; ++tmp)
>      if (*tmp == NULL)
>        {
>  	/* Another necessary aliasing violation.  */
> @@ -88,74 +87,25 @@ _dl_sysdep_start (void **start_argptr,
>  	break;
>        }
>  
> -  user_entry = (ElfW(Addr)) ENTRY_POINT;
> -  GLRO(dl_platform) = NULL; /* Default to nothing known about the platform.  */
> +  dl_parse_auxv_t auxv_values = { 0, };
> +  _dl_parse_auxv (GLRO(dl_auxv), auxv_values);
>  
> -  /* NB: Default to a constant CONSTANT_MINSIGSTKSZ.  */
> -  _Static_assert (__builtin_constant_p (CONSTANT_MINSIGSTKSZ),
> -		  "CONSTANT_MINSIGSTKSZ is constant");
> -  GLRO(dl_minsigstacksize) = CONSTANT_MINSIGSTKSZ;
> +  args->phdr = (const ElfW(Phdr) *) auxv_values[AT_PHDR];
> +  args->phnum = auxv_values[AT_PHNUM];
> +  args->user_entry = auxv_values[AT_ENTRY];
> +}
>  
> -  for (av = GLRO(dl_auxv); av->a_type != AT_NULL; av++)
> -    switch (av->a_type)
> -      {
> -      case AT_PHDR:
> -	phdr = (void *) av->a_un.a_val;
> -	break;
> -      case AT_PHNUM:
> -	phnum = av->a_un.a_val;
> -	break;
> -      case AT_PAGESZ:
> -	GLRO(dl_pagesize) = av->a_un.a_val;
> -	break;
> -      case AT_ENTRY:
> -	user_entry = av->a_un.a_val;
> -	break;
> -      case AT_SECURE:
> -	__libc_enable_secure = av->a_un.a_val;
> -	break;
> -      case AT_PLATFORM:
> -	GLRO(dl_platform) = (void *) av->a_un.a_val;
> -	break;
> -      case AT_HWCAP:
> -	GLRO(dl_hwcap) = (unsigned long int) av->a_un.a_val;
> -	break;
> -      case AT_HWCAP2:
> -	GLRO(dl_hwcap2) = (unsigned long int) av->a_un.a_val;
> -	break;
> -      case AT_CLKTCK:
> -	GLRO(dl_clktck) = av->a_un.a_val;
> -	break;
> -      case AT_FPUCW:
> -	GLRO(dl_fpu_control) = av->a_un.a_val;
> -	break;
> -#ifdef NEED_DL_SYSINFO
> -      case AT_SYSINFO:
> -	new_sysinfo = av->a_un.a_val;
> -	break;
> -#endif
> -      case AT_SYSINFO_EHDR:
> -	GLRO(dl_sysinfo_dso) = (void *) av->a_un.a_val;
> -	break;
> -      case AT_RANDOM:
> -	_dl_random = (void *) av->a_un.a_val;
> -	break;
> -      case AT_MINSIGSTKSZ:
> -	GLRO(dl_minsigstacksize) = av->a_un.a_val;
> -	break;
> -      DL_PLATFORM_AUXV
> -      }
> +ElfW(Addr)
> +_dl_sysdep_start (void **start_argptr,
> +		  void (*dl_main) (const ElfW(Phdr) *phdr, ElfW(Word) phnum,
> +				   ElfW(Addr) *user_entry, ElfW(auxv_t) *auxv))
> +{
> +  __libc_stack_end = DL_STACK_END (start_argptr);
>  
> -  dl_hwcap_check ();
> +  struct dl_main_arguments dl_main_args;
> +  _dl_sysdep_parse_arguments (start_argptr, &dl_main_args);
>  
> -#ifdef NEED_DL_SYSINFO
> -  if (new_sysinfo != 0)
> -    {
> -      /* Only set the sysinfo value if we also have the vsyscall DSO.  */
> -      if (GLRO(dl_sysinfo_dso) != 0)
> -        GLRO(dl_sysinfo) = new_sysinfo;
> -    }
> -#endif
> +  dl_hwcap_check ();
>  
>    __tunables_init (_environ);
>  
> @@ -187,8 +137,9 @@ _dl_sysdep_start (void **start_argptr,
>    if (__builtin_expect (__libc_enable_secure, 0))
>      __libc_check_standard_fds ();
>  
> -  (*dl_main) (phdr, phnum, &user_entry, GLRO(dl_auxv));
> -  return user_entry;
> +  (*dl_main) (dl_main_args.phdr, dl_main_args.phnum,
> +              &dl_main_args.user_entry, GLRO(dl_auxv));
> +  return dl_main_args.user_entry;
>  }
>  
>  void
> diff --git a/sysdeps/unix/sysv/linux/powerpc/dl-auxv.h b/sysdeps/unix/sysv/linux/powerpc/dl-auxv.h
> index 594371940a..ce2281cf11 100644
> --- a/sysdeps/unix/sysv/linux/powerpc/dl-auxv.h
> +++ b/sysdeps/unix/sysv/linux/powerpc/dl-auxv.h
> @@ -16,15 +16,5 @@
>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>  
> -#include <ldsodefs.h>
> -
> -#if IS_IN (libc) && !defined SHARED
> -int GLRO(dl_cache_line_size);
> -#endif
> -
> -/* Scan the Aux Vector for the "Data Cache Block Size" entry and assign it
> -   to dl_cache_line_size.  */
> -#define DL_PLATFORM_AUXV						      \
> -      case AT_DCACHEBSIZE:						      \
> -	GLRO(dl_cache_line_size) = av->a_un.a_val;			      \
> -	break;
> +#define DL_PLATFORM_AUXV \
> +  GLRO(dl_cache_line_size) = auxv_values[AT_DCACHEBSIZE];
> diff --git a/sysdeps/unix/sysv/linux/powerpc/dl-support.c b/sysdeps/unix/sysv/linux/powerpc/dl-support.c
> new file mode 100644
> index 0000000000..abe68a7049
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/powerpc/dl-support.c
> @@ -0,0 +1,4 @@
> +#include <elf/dl-support.c>
> +
> +/* Populated from the auxiliary vector.  */
> +int _dl_cache_line_size;
  

* Adhemerval Zanella:

>> diff --git a/sysdeps/unix/sysv/linux/dl-parse_auxv.h b/sysdeps/unix/sysv/linux/dl-parse_auxv.h
>> new file mode 100644
>> index 0000000000..f450c6c5ce
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/dl-parse_auxv.h

>> +typedef ElfW(Addr) dl_parse_auxv_t[AT_MINSIGSTKSZ + 1];
>> +
>> +/* Copy the auxiliary vector into AUX_VALUES and set up GLRO
>> +   variables.  */
>> +static inline
>> +void _dl_parse_auxv (ElfW(auxv_t) *av, dl_parse_auxv_t auxv_values)
>> +{
>> +  auxv_values[AT_ENTRY] =  (ElfW(Addr)) ENTRY_POINT;
>
> Extra whitespace.

Fxied.

>> +  auxv_values[AT_PAGESZ] = EXEC_PAGESIZE;
>> +  auxv_values[AT_FPUCW] = _FPU_DEFAULT;
>> +
>> +  /* NB: Default to a constant CONSTANT_MINSIGSTKSZ.  */
>> +  _Static_assert (__builtin_constant_p (CONSTANT_MINSIGSTKSZ),
>> +                  "CONSTANT_MINSIGSTKSZ is constant");
>
> Shouldn't it warn that CONSTANT_MINSIGSTKSZ is not a constant?

Sorry, would please rephrase?  Should I change this to
""CONSTANT_MINSIGSTKSZ is not constant"?

>> +  auxv_values[AT_MINSIGSTKSZ] = CONSTANT_MINSIGSTKSZ;
>> +
>> +  for (; av->a_type != AT_NULL; av++)
>> +    if (av->a_type <= AT_MINSIGSTKSZ)
>> +      auxv_values[av->a_type] = av->a_un.a_val;
>> +
>> +  GLRO(dl_pagesize) = auxv_values[AT_PAGESZ];
>> +  __libc_enable_secure = auxv_values[AT_SECURE];
>> +  GLRO(dl_platform) = (void *) auxv_values[AT_PLATFORM];
>> +  GLRO(dl_hwcap) = auxv_values[AT_HWCAP];
>> +  GLRO(dl_hwcap2) = auxv_values[AT_HWCAP2];
>> +  GLRO(dl_clktck) = auxv_values[AT_CLKTCK];
>> +  GLRO(dl_fpu_control) = auxv_values[AT_FPUCW];
>> +  _dl_random = (void *) auxv_values[AT_RANDOM];
>> +  GLRO(dl_minsigstacksize) = auxv_values[AT_MINSIGSTKSZ];
>> +  GLRO(dl_sysinfo_dso) = (void *) auxv_values[AT_SYSINFO_EHDR];
>> +#ifdef NEED_DL_SYSINFO
>
> I think you can now assume NEED_DL_SYSINFO is always defined.

NEED_DL_SYSINFO is only defined by i386 and ia64, only
NEED_DL_SYSINFO_DSO is always defined on Linux.

Thanks,
Florian
  

On 08/02/2022 17:34, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>>> diff --git a/sysdeps/unix/sysv/linux/dl-parse_auxv.h b/sysdeps/unix/sysv/linux/dl-parse_auxv.h
>>> new file mode 100644
>>> index 0000000000..f450c6c5ce
>>> --- /dev/null
>>> +++ b/sysdeps/unix/sysv/linux/dl-parse_auxv.h
> 
>>> +typedef ElfW(Addr) dl_parse_auxv_t[AT_MINSIGSTKSZ + 1];
>>> +
>>> +/* Copy the auxiliary vector into AUX_VALUES and set up GLRO
>>> +   variables.  */
>>> +static inline
>>> +void _dl_parse_auxv (ElfW(auxv_t) *av, dl_parse_auxv_t auxv_values)
>>> +{
>>> +  auxv_values[AT_ENTRY] =  (ElfW(Addr)) ENTRY_POINT;
>>
>> Extra whitespace.
> 
> Fxied.
> 
>>> +  auxv_values[AT_PAGESZ] = EXEC_PAGESIZE;
>>> +  auxv_values[AT_FPUCW] = _FPU_DEFAULT;
>>> +
>>> +  /* NB: Default to a constant CONSTANT_MINSIGSTKSZ.  */
>>> +  _Static_assert (__builtin_constant_p (CONSTANT_MINSIGSTKSZ),
>>> +                  "CONSTANT_MINSIGSTKSZ is constant");
>>
>> Shouldn't it warn that CONSTANT_MINSIGSTKSZ is not a constant?
> 
> Sorry, would please rephrase?  Should I change this to
> ""CONSTANT_MINSIGSTKSZ is not constant"?

Yeah because on a the _Static_assert failure the resulting message seems
to the opposite of what triggered it.

> 
>>> +  auxv_values[AT_MINSIGSTKSZ] = CONSTANT_MINSIGSTKSZ;
>>> +
>>> +  for (; av->a_type != AT_NULL; av++)
>>> +    if (av->a_type <= AT_MINSIGSTKSZ)
>>> +      auxv_values[av->a_type] = av->a_un.a_val;
>>> +
>>> +  GLRO(dl_pagesize) = auxv_values[AT_PAGESZ];
>>> +  __libc_enable_secure = auxv_values[AT_SECURE];
>>> +  GLRO(dl_platform) = (void *) auxv_values[AT_PLATFORM];
>>> +  GLRO(dl_hwcap) = auxv_values[AT_HWCAP];
>>> +  GLRO(dl_hwcap2) = auxv_values[AT_HWCAP2];
>>> +  GLRO(dl_clktck) = auxv_values[AT_CLKTCK];
>>> +  GLRO(dl_fpu_control) = auxv_values[AT_FPUCW];
>>> +  _dl_random = (void *) auxv_values[AT_RANDOM];
>>> +  GLRO(dl_minsigstacksize) = auxv_values[AT_MINSIGSTKSZ];
>>> +  GLRO(dl_sysinfo_dso) = (void *) auxv_values[AT_SYSINFO_EHDR];
>>> +#ifdef NEED_DL_SYSINFO
>>
>> I think you can now assume NEED_DL_SYSINFO is always defined.
> 
> NEED_DL_SYSINFO is only defined by i386 and ia64, only
> NEED_DL_SYSINFO_DSO is always defined on Linux.

Indeed, my confusion here.

> 
> Thanks,
> Florian
>
  

* Adhemerval Zanella:

>>>> +  auxv_values[AT_PAGESZ] = EXEC_PAGESIZE;
>>>> +  auxv_values[AT_FPUCW] = _FPU_DEFAULT;
>>>> +
>>>> +  /* NB: Default to a constant CONSTANT_MINSIGSTKSZ.  */
>>>> +  _Static_assert (__builtin_constant_p (CONSTANT_MINSIGSTKSZ),
>>>> +                  "CONSTANT_MINSIGSTKSZ is constant");
>>>
>>> Shouldn't it warn that CONSTANT_MINSIGSTKSZ is not a constant?
>> 
>> Sorry, would please rephrase?  Should I change this to
>> ""CONSTANT_MINSIGSTKSZ is not constant"?
>
> Yeah because on a the _Static_assert failure the resulting message seems
> to the opposite of what triggered it.

The GCC error message doesn't suggest a strong preference either way:

/tmp/t.c:1:1: error: static assertion failed: "failed"
    1 | _Static_assert (0, "failed");
      | ^~~~~~~~~~~~~~

I think in the existing sources, the message is sometimes indicating the
failure, or what is being asserted (as is the case here).

Thanks,
Florian
  

On 10/02/2022 07:54, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>>>>> +  auxv_values[AT_PAGESZ] = EXEC_PAGESIZE;
>>>>> +  auxv_values[AT_FPUCW] = _FPU_DEFAULT;
>>>>> +
>>>>> +  /* NB: Default to a constant CONSTANT_MINSIGSTKSZ.  */
>>>>> +  _Static_assert (__builtin_constant_p (CONSTANT_MINSIGSTKSZ),
>>>>> +                  "CONSTANT_MINSIGSTKSZ is constant");
>>>>
>>>> Shouldn't it warn that CONSTANT_MINSIGSTKSZ is not a constant?
>>>
>>> Sorry, would please rephrase?  Should I change this to
>>> ""CONSTANT_MINSIGSTKSZ is not constant"?
>>
>> Yeah because on a the _Static_assert failure the resulting message seems
>> to the opposite of what triggered it.
> 
> The GCC error message doesn't suggest a strong preference either way:
> 
> /tmp/t.c:1:1: error: static assertion failed: "failed"
>     1 | _Static_assert (0, "failed");
>       | ^~~~~~~~~~~~~~
> 
> I think in the existing sources, the message is sometimes indicating the
> failure, or what is being asserted (as is the case here).

Compiler is will just emit the message if condition is met, my point is
emitting 'CONSTANT_MINSIGSTKSZ is constant' on an error is the opposite
of what triggered it (CONSTANT_MINSIGSTKSZ is really *not* a constant).
  

* Adhemerval Zanella:

>> The GCC error message doesn't suggest a strong preference either way:
>> 
>> /tmp/t.c:1:1: error: static assertion failed: "failed"
>>     1 | _Static_assert (0, "failed");
>>       | ^~~~~~~~~~~~~~
>> 
>> I think in the existing sources, the message is sometimes indicating the
>> failure, or what is being asserted (as is the case here).
>
> Compiler is will just emit the message if condition is met, my point is
> emitting 'CONSTANT_MINSIGSTKSZ is constant' on an error is the opposite
> of what triggered it (CONSTANT_MINSIGSTKSZ is really *not* a constant).

But the check asserts that CONSTANT_MINSIGSTKSZ is constant, so the
message is still correct.

Thanks,
Florian
  

On 10/02/2022 08:31, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>>> The GCC error message doesn't suggest a strong preference either way:
>>>
>>> /tmp/t.c:1:1: error: static assertion failed: "failed"
>>>     1 | _Static_assert (0, "failed");
>>>       | ^~~~~~~~~~~~~~
>>>
>>> I think in the existing sources, the message is sometimes indicating the
>>> failure, or what is being asserted (as is the case here).
>>
>> Compiler is will just emit the message if condition is met, my point is
>> emitting 'CONSTANT_MINSIGSTKSZ is constant' on an error is the opposite
>> of what triggered it (CONSTANT_MINSIGSTKSZ is really *not* a constant).
> 
> But the check asserts that CONSTANT_MINSIGSTKSZ is constant, so the
> message is still correct.
In fact the _Static_assert does not really matter, __builtin_constant_p
will already trigger a valid warning message:

In file included from ../include/features.h:490,
                 from ../posix/sys/types.h:25,
                 from ../include/sys/types.h:1,
                 from ../include/string.h:6,
                 from dl-support.c:22:
../sysdeps/unix/sysv/linux/dl-parse_auxv.h: In function ‘_dl_parse_auxv’:
../sysdeps/unix/sysv/linux/dl-parse_auxv.h:40:19: error: expression in static assertion is not constant
   40 |   _Static_assert (__builtin_constant_p (CONSTANT_MINSIGSTKSZ),
      |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../include/sys/cdefs.h:7:59: note: in definition of macro ‘_Static_assert’
    7 | # define _Static_assert(expr, diagnostic) _Static_assert (expr, diagnostic)
  

On 10/02/2022 08:40, Adhemerval Zanella wrote:
> 
> 
> On 10/02/2022 08:31, Florian Weimer wrote:
>> * Adhemerval Zanella:
>>
>>>> The GCC error message doesn't suggest a strong preference either way:
>>>>
>>>> /tmp/t.c:1:1: error: static assertion failed: "failed"
>>>>     1 | _Static_assert (0, "failed");
>>>>       | ^~~~~~~~~~~~~~
>>>>
>>>> I think in the existing sources, the message is sometimes indicating the
>>>> failure, or what is being asserted (as is the case here).
>>>
>>> Compiler is will just emit the message if condition is met, my point is
>>> emitting 'CONSTANT_MINSIGSTKSZ is constant' on an error is the opposite
>>> of what triggered it (CONSTANT_MINSIGSTKSZ is really *not* a constant).
>>
>> But the check asserts that CONSTANT_MINSIGSTKSZ is constant, so the
>> message is still correct.
> In fact the _Static_assert does not really matter, __builtin_constant_p

_Static_assert message string*

> will already trigger a valid warning message:
> 
> In file included from ../include/features.h:490,
>                  from ../posix/sys/types.h:25,
>                  from ../include/sys/types.h:1,
>                  from ../include/string.h:6,
>                  from dl-support.c:22:
> ../sysdeps/unix/sysv/linux/dl-parse_auxv.h: In function ‘_dl_parse_auxv’:
> ../sysdeps/unix/sysv/linux/dl-parse_auxv.h:40:19: error: expression in static assertion is not constant
>    40 |   _Static_assert (__builtin_constant_p (CONSTANT_MINSIGSTKSZ),
>       |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../include/sys/cdefs.h:7:59: note: in definition of macro ‘_Static_assert’
>     7 | # define _Static_assert(expr, diagnostic) _Static_assert (expr, diagnostic)
  

this broke aarch64 static binaries:

The 02/03/2022 12:08, Florian Weimer via Libc-alpha wrote
>  void
>  _dl_aux_init (ElfW(auxv_t) *av)
>  {
> -  int seen = 0;
> -  uid_t uid = 0;
> -  gid_t gid = 0;
> -
>  #ifdef NEED_DL_SYSINFO
>    /* NB: Avoid RELATIVE relocation in static PIE.  */
>    GL(dl_sysinfo) = DL_SYSINFO_DEFAULT;
>  #endif
>  
>    _dl_auxv = av;
> -  for (; av->a_type != AT_NULL; ++av)
> -    switch (av->a_type)
> -      {
> -      case AT_PAGESZ:
> -	if (av->a_un.a_val != 0)
> -	  GLRO(dl_pagesize) = av->a_un.a_val;
> -	break;
> -      case AT_CLKTCK:
> -	GLRO(dl_clktck) = av->a_un.a_val;
> -	break;
> -      case AT_PHDR:
> -	GL(dl_phdr) = (const void *) av->a_un.a_val;
> -	break;
> -      case AT_PHNUM:
> -	GL(dl_phnum) = av->a_un.a_val;
> -	break;
> -      case AT_PLATFORM:
> -	GLRO(dl_platform) = (void *) av->a_un.a_val;
> -	break;
> -      case AT_HWCAP:
> -	GLRO(dl_hwcap) = (unsigned long int) av->a_un.a_val;
> -	break;
> -      case AT_HWCAP2:
> -	GLRO(dl_hwcap2) = (unsigned long int) av->a_un.a_val;
> -	break;
> -      case AT_FPUCW:
> -	GLRO(dl_fpu_control) = av->a_un.a_val;
> -	break;
> -#ifdef NEED_DL_SYSINFO
> -      case AT_SYSINFO:
> -	GL(dl_sysinfo) = av->a_un.a_val;
> -	break;
> -#endif
> -#ifdef NEED_DL_SYSINFO_DSO
> -      case AT_SYSINFO_EHDR:
> -	GL(dl_sysinfo_dso) = (void *) av->a_un.a_val;
> -	break;
> -#endif
> -      case AT_UID:
> -	uid ^= av->a_un.a_val;
> -	seen |= 1;
> -	break;
> -      case AT_EUID:
> -	uid ^= av->a_un.a_val;
> -	seen |= 2;
> -	break;
> -      case AT_GID:
> -	gid ^= av->a_un.a_val;
> -	seen |= 4;
> -	break;
> -      case AT_EGID:
> -	gid ^= av->a_un.a_val;
> -	seen |= 8;
> -	break;
> -      case AT_SECURE:
> -	seen = -1;
> -	__libc_enable_secure = av->a_un.a_val;
> -	__libc_enable_secure_decided = 1;
> -	break;
> -      case AT_RANDOM:
> -	_dl_random = (void *) av->a_un.a_val;
> -	break;
> -      case AT_MINSIGSTKSZ:
> -	_dl_minsigstacksize = av->a_un.a_val;
> -	break;
> -      DL_PLATFORM_AUXV
> -      }
> -  if (seen == 0xf)
> -    {
> -      __libc_enable_secure = uid != 0 || gid != 0;
> -      __libc_enable_secure_decided = 1;
> -    }
> +  dl_parse_auxv_t auxv_values = { 0, };
> +  _dl_parse_auxv (av, auxv_values);

this seems to use memset now on aarch64 before irelative
relocs are resolved in static binaries.

which causes infinite loops in the iplt (i've also seen
segfaults in the build log).

i wonder what is a clean fix...
  

On 11/02/2022 09:31, Szabolcs Nagy via Libc-alpha wrote:
> this broke aarch64 static binaries:
> 
> The 02/03/2022 12:08, Florian Weimer via Libc-alpha wrote
>>  void
>>  _dl_aux_init (ElfW(auxv_t) *av)
>>  {
>> -  int seen = 0;
>> -  uid_t uid = 0;
>> -  gid_t gid = 0;
>> -
>>  #ifdef NEED_DL_SYSINFO
>>    /* NB: Avoid RELATIVE relocation in static PIE.  */
>>    GL(dl_sysinfo) = DL_SYSINFO_DEFAULT;
>>  #endif
>>  
>>    _dl_auxv = av;
>> -  for (; av->a_type != AT_NULL; ++av)
>> -    switch (av->a_type)
>> -      {
>> -      case AT_PAGESZ:
>> -	if (av->a_un.a_val != 0)
>> -	  GLRO(dl_pagesize) = av->a_un.a_val;
>> -	break;
>> -      case AT_CLKTCK:
>> -	GLRO(dl_clktck) = av->a_un.a_val;
>> -	break;
>> -      case AT_PHDR:
>> -	GL(dl_phdr) = (const void *) av->a_un.a_val;
>> -	break;
>> -      case AT_PHNUM:
>> -	GL(dl_phnum) = av->a_un.a_val;
>> -	break;
>> -      case AT_PLATFORM:
>> -	GLRO(dl_platform) = (void *) av->a_un.a_val;
>> -	break;
>> -      case AT_HWCAP:
>> -	GLRO(dl_hwcap) = (unsigned long int) av->a_un.a_val;
>> -	break;
>> -      case AT_HWCAP2:
>> -	GLRO(dl_hwcap2) = (unsigned long int) av->a_un.a_val;
>> -	break;
>> -      case AT_FPUCW:
>> -	GLRO(dl_fpu_control) = av->a_un.a_val;
>> -	break;
>> -#ifdef NEED_DL_SYSINFO
>> -      case AT_SYSINFO:
>> -	GL(dl_sysinfo) = av->a_un.a_val;
>> -	break;
>> -#endif
>> -#ifdef NEED_DL_SYSINFO_DSO
>> -      case AT_SYSINFO_EHDR:
>> -	GL(dl_sysinfo_dso) = (void *) av->a_un.a_val;
>> -	break;
>> -#endif
>> -      case AT_UID:
>> -	uid ^= av->a_un.a_val;
>> -	seen |= 1;
>> -	break;
>> -      case AT_EUID:
>> -	uid ^= av->a_un.a_val;
>> -	seen |= 2;
>> -	break;
>> -      case AT_GID:
>> -	gid ^= av->a_un.a_val;
>> -	seen |= 4;
>> -	break;
>> -      case AT_EGID:
>> -	gid ^= av->a_un.a_val;
>> -	seen |= 8;
>> -	break;
>> -      case AT_SECURE:
>> -	seen = -1;
>> -	__libc_enable_secure = av->a_un.a_val;
>> -	__libc_enable_secure_decided = 1;
>> -	break;
>> -      case AT_RANDOM:
>> -	_dl_random = (void *) av->a_un.a_val;
>> -	break;
>> -      case AT_MINSIGSTKSZ:
>> -	_dl_minsigstacksize = av->a_un.a_val;
>> -	break;
>> -      DL_PLATFORM_AUXV
>> -      }
>> -  if (seen == 0xf)
>> -    {
>> -      __libc_enable_secure = uid != 0 || gid != 0;
>> -      __libc_enable_secure_decided = 1;
>> -    }
>> +  dl_parse_auxv_t auxv_values = { 0, };
>> +  _dl_parse_auxv (av, auxv_values);
> 
> this seems to use memset now on aarch64 before irelative
> relocs are resolved in static binaries.
> 
> which causes infinite loops in the iplt (i've also seen
> segfaults in the build log).
> 
> i wonder what is a clean fix...
> 

Maybe add inhibit_loop_to_libcall to avoid the memset call.
  

The 02/11/2022 09:47, Adhemerval Zanella wrote:
> On 11/02/2022 09:31, Szabolcs Nagy via Libc-alpha wrote:
> > The 02/03/2022 12:08, Florian Weimer via Libc-alpha wrote
> >> +  dl_parse_auxv_t auxv_values = { 0, };
> >> +  _dl_parse_auxv (av, auxv_values);
> > 
> > this seems to use memset now on aarch64 before irelative
> > relocs are resolved in static binaries.
> > 
> > which causes infinite loops in the iplt (i've also seen
> > segfaults in the build log).
> > 
> > i wonder what is a clean fix...
> > 
> 
> Maybe add inhibit_loop_to_libcall to avoid the memset call.

does not work for me..
happens at -O0 too
https://godbolt.org/z/W9r3nffYd
  

On 11/02/2022 10:24, Szabolcs Nagy wrote:
> The 02/11/2022 09:47, Adhemerval Zanella wrote:
>> On 11/02/2022 09:31, Szabolcs Nagy via Libc-alpha wrote:
>>> The 02/03/2022 12:08, Florian Weimer via Libc-alpha wrote
>>>> +  dl_parse_auxv_t auxv_values = { 0, };
>>>> +  _dl_parse_auxv (av, auxv_values);
>>>
>>> this seems to use memset now on aarch64 before irelative
>>> relocs are resolved in static binaries.
>>>
>>> which causes infinite loops in the iplt (i've also seen
>>> segfaults in the build log).
>>>
>>> i wonder what is a clean fix...
>>>
>>
>> Maybe add inhibit_loop_to_libcall to avoid the memset call.
> 
> does not work for me..
> happens at -O0 too
> https://godbolt.org/z/W9r3nffYd

Yeah, this won't help much this situation.  Unless we can make gcc *not*
emit this memset call, we will need something like:

diff --git a/elf/dl-support.c b/elf/dl-support.c
index 1977a2be76..49f3fd2a61 100644
--- a/elf/dl-support.c
+++ b/elf/dl-support.c
@@ -19,6 +19,10 @@
 /* This file defines some things that for the dynamic linker are defined in
    rtld.c and dl-sysdep.c in ways appropriate to bootstrap dynamic linking.  */
 
+#ifndef SHARED
+asm ("memset = __memset_generic");
+#endif
+
 #include <string.h>
 /* Mark symbols hidden in static PIE for early self relocation to work.
    Note: string.h may have ifuncs which cannot be hidden on i686.  */


$ make -j24 && make test t=elf/tst-tls1-static-non-pie
[...]
PASS: elf/tst-tls1-static-non-pie
original exit status 0
set bar to 1 (LE)
get sum of foo and bar (IE) = 1
get sum of foo and bar (LD or TLSDESC) = 1
get sum of foo and bar (GD or TLSDESC) = 1


I think the best approach would to add a __memset_generic, __memcpy_generic,
and __memmove_generic on all targets and add a header similar to
sysdeps/generic/symbol-hacks.h to activate the redirection when required
(to not pessimize code that would work with the IPTL generated by the
iFUNC).
  

* Szabolcs Nagy:

> The 02/11/2022 09:47, Adhemerval Zanella wrote:
>> On 11/02/2022 09:31, Szabolcs Nagy via Libc-alpha wrote:
>> > The 02/03/2022 12:08, Florian Weimer via Libc-alpha wrote
>> >> +  dl_parse_auxv_t auxv_values = { 0, };
>> >> +  _dl_parse_auxv (av, auxv_values);
>> > 
>> > this seems to use memset now on aarch64 before irelative
>> > relocs are resolved in static binaries.
>> > 
>> > which causes infinite loops in the iplt (i've also seen
>> > segfaults in the build log).
>> > 
>> > i wonder what is a clean fix...
>> > 
>> 
>> Maybe add inhibit_loop_to_libcall to avoid the memset call.
>
> does not work for me..
> happens at -O0 too
> https://godbolt.org/z/W9r3nffYd

Seems to happen with -fno-builtin or -ffreestanding as well.  It's a GCC
bug, but I'll investigating working around it.

Thanks,
Florian
  

On 11/02/2022 10:49, Florian Weimer wrote:
> * Szabolcs Nagy:
> 
>> The 02/11/2022 09:47, Adhemerval Zanella wrote:
>>> On 11/02/2022 09:31, Szabolcs Nagy via Libc-alpha wrote:
>>>> The 02/03/2022 12:08, Florian Weimer via Libc-alpha wrote
>>>>> +  dl_parse_auxv_t auxv_values = { 0, };
>>>>> +  _dl_parse_auxv (av, auxv_values);
>>>>
>>>> this seems to use memset now on aarch64 before irelative
>>>> relocs are resolved in static binaries.
>>>>
>>>> which causes infinite loops in the iplt (i've also seen
>>>> segfaults in the build log).
>>>>
>>>> i wonder what is a clean fix...
>>>>
>>>
>>> Maybe add inhibit_loop_to_libcall to avoid the memset call.
>>
>> does not work for me..
>> happens at -O0 too
>> https://godbolt.org/z/W9r3nffYd
> 
> Seems to happen with -fno-builtin or -ffreestanding as well.  It's a GCC
> bug, but I'll investigating working around it.

I don't think it is gcc bug, even for -ffreestanding compiler can assume
the C environment will always provide memset/memcpy.  The problem is we
need a gcc option to avoid such optimization, which afaik there is none.

And without such option I think we will need to either revert this change
or use the symbol alias strategy to avoid using IFUNC on loader code.
  

The 02/11/2022 11:17, Adhemerval Zanella wrote:
> On 11/02/2022 10:49, Florian Weimer wrote:
> > * Szabolcs Nagy:
> >> The 02/11/2022 09:47, Adhemerval Zanella wrote:
> >>> On 11/02/2022 09:31, Szabolcs Nagy via Libc-alpha wrote:
> >>>> The 02/03/2022 12:08, Florian Weimer via Libc-alpha wrote
> >>>>> +  dl_parse_auxv_t auxv_values = { 0, };
> >>>>> +  _dl_parse_auxv (av, auxv_values);
> >>>>
> >>>> this seems to use memset now on aarch64 before irelative
> >>>> relocs are resolved in static binaries.
> >>>>
> >>>> which causes infinite loops in the iplt (i've also seen
> >>>> segfaults in the build log).
> >>>>
> >>>> i wonder what is a clean fix...
> >>>>
> >>>
> >>> Maybe add inhibit_loop_to_libcall to avoid the memset call.
> >>
> >> does not work for me..
> >> happens at -O0 too
> >> https://godbolt.org/z/W9r3nffYd
> > 
> > Seems to happen with -fno-builtin or -ffreestanding as well.  It's a GCC
> > bug, but I'll investigating working around it.
> 
> I don't think it is gcc bug, even for -ffreestanding compiler can assume
> the C environment will always provide memset/memcpy.  The problem is we
> need a gcc option to avoid such optimization, which afaik there is none.
> 
> And without such option I think we will need to either revert this change
> or use the symbol alias strategy to avoid using IFUNC on loader code.

if we can rewrite the code not to depend on zeroing a large array
(where large is about >200bytes) that should work too.

i dont see a clean way, but e.g. a

char seen_auxval[AT_MINSIGSTKSZ+1] = {0};

would not trigger memset (on aarch64).

as a bonus with such an array we can distinguish auxv unset and auxv 0.
  

On Fri, Feb 11, 2022 at 5:49 AM Adhemerval Zanella via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
>
>
> On 11/02/2022 10:24, Szabolcs Nagy wrote:
> > The 02/11/2022 09:47, Adhemerval Zanella wrote:
> >> On 11/02/2022 09:31, Szabolcs Nagy via Libc-alpha wrote:
> >>> The 02/03/2022 12:08, Florian Weimer via Libc-alpha wrote
> >>>> +  dl_parse_auxv_t auxv_values = { 0, };
> >>>> +  _dl_parse_auxv (av, auxv_values);
> >>>
> >>> this seems to use memset now on aarch64 before irelative
> >>> relocs are resolved in static binaries.
> >>>
> >>> which causes infinite loops in the iplt (i've also seen
> >>> segfaults in the build log).
> >>>
> >>> i wonder what is a clean fix...
> >>>
> >>
> >> Maybe add inhibit_loop_to_libcall to avoid the memset call.
> >
> > does not work for me..
> > happens at -O0 too
> > https://godbolt.org/z/W9r3nffYd
>
> Yeah, this won't help much this situation.  Unless we can make gcc *not*
> emit this memset call, we will need something like:
>
> diff --git a/elf/dl-support.c b/elf/dl-support.c
> index 1977a2be76..49f3fd2a61 100644
> --- a/elf/dl-support.c
> +++ b/elf/dl-support.c
> @@ -19,6 +19,10 @@
>  /* This file defines some things that for the dynamic linker are defined in
>     rtld.c and dl-sysdep.c in ways appropriate to bootstrap dynamic linking.  */
>
> +#ifndef SHARED
> +asm ("memset = __memset_generic");
> +#endif
> +
>  #include <string.h>
>  /* Mark symbols hidden in static PIE for early self relocation to work.
>     Note: string.h may have ifuncs which cannot be hidden on i686.  */
>
>
> $ make -j24 && make test t=elf/tst-tls1-static-non-pie
> [...]
> PASS: elf/tst-tls1-static-non-pie
> original exit status 0
> set bar to 1 (LE)
> get sum of foo and bar (IE) = 1
> get sum of foo and bar (LD or TLSDESC) = 1
> get sum of foo and bar (GD or TLSDESC) = 1
>
>
> I think the best approach would to add a __memset_generic, __memcpy_generic,
> and __memmove_generic on all targets and add a header similar to
> sysdeps/generic/symbol-hacks.h to activate the redirection when required
> (to not pessimize code that would work with the IPTL generated by the
> iFUNC).

I ran into a similar problem on my CET branch.  In my case, it is memcmp.
I added a new header file, <dl-start.h>, to define functions which can be safely
used during ld.so startup:

https://gitlab.com/x86-glibc/glibc/-/commit/2bfa258b114e556ab0be68d6465b5e4fa5504e2e


--
H.J.
  

* H. J. Lu:

> I ran into a similar problem on my CET branch.  In my case, it is memcmp.
> I added a new header file, <dl-start.h>, to define functions which can be safely
> used during ld.so startup:
>
> https://gitlab.com/x86-glibc/glibc/-/commit/2bfa258b114e556ab0be68d6465b5e4fa5504e2e

I think we would need something using assembler-level redirects because
we need to change where the compiler-generated calls go.

We probably need an adjustment to sysdeps/generic/symbol-hacks.h, and
teach the stack protector controls to affect the symbol redirects
(I think the requirements around TCB initialization are identical).

Thanks,
Florian
  

* Szabolcs Nagy:

> this seems to use memset now on aarch64 before irelative
> relocs are resolved in static binaries.
>
> which causes infinite loops in the iplt (i've also seen
> segfaults in the build log).
>
> i wonder what is a clean fix...

I've reverted this for now.  I missed the “static” part before; I think
our build environment is not really set up for such redirects.  Maybe
the easiest/cleanest way for now is to use an explicit initialization
loop for !SHARED, plus -fno-tree-loop-distribute-patterns.

> char seen_auxval[AT_MINSIGSTKSZ+1] = {0};
> 
> would not trigger memset (on aarch64).
> 
> as a bonus with such an array we can distinguish auxv unset and auxv 0.

The current code does not need that because it writes the default
values first:

  auxv_values[AT_ENTRY] = (ElfW(Addr)) ENTRY_POINT;
  auxv_values[AT_PAGESZ] = EXEC_PAGESIZE;
  auxv_values[AT_FPUCW] = _FPU_DEFAULT;

On the reader side, we would need to write

  if (seen_auxval[AT_CLKTCK])
    GLRO(dl_clktck) = auxv_values[AT_CLKTCK];

and

  if (seen_auxval[AT_FPUCW])
    auxv_values[AT_FPUCW] = auxv_values[AT_FPUCW];
  else
    auxv_values[AT_FPUCW] = _FPU_DEFAULT;

, which does not look like an improvement to me (neither
maintenance-wise nor code-size-wise).

Thanks,
Florian
  

The 02/11/2022 18:32, Florian Weimer wrote:
> * Szabolcs Nagy:
> 
> > this seems to use memset now on aarch64 before irelative
> > relocs are resolved in static binaries.
> >
> > which causes infinite loops in the iplt (i've also seen
> > segfaults in the build log).
> >
> > i wonder what is a clean fix...
> 
> I've reverted this for now.  I missed the “static” part before; I think
> our build environment is not really set up for such redirects.  Maybe
> the easiest/cleanest way for now is to use an explicit initialization
> loop for !SHARED, plus -fno-tree-loop-distribute-patterns.

yeah, loop with

__attribute__((optimize("no-tree-loop-distribute-patterns")))

on the function works.
  

On 11/02/2022 11:29, Szabolcs Nagy wrote:
> The 02/11/2022 11:17, Adhemerval Zanella wrote:
>> On 11/02/2022 10:49, Florian Weimer wrote:
>>> * Szabolcs Nagy:
>>>> The 02/11/2022 09:47, Adhemerval Zanella wrote:
>>>>> On 11/02/2022 09:31, Szabolcs Nagy via Libc-alpha wrote:
>>>>>> The 02/03/2022 12:08, Florian Weimer via Libc-alpha wrote
>>>>>>> +  dl_parse_auxv_t auxv_values = { 0, };
>>>>>>> +  _dl_parse_auxv (av, auxv_values);
>>>>>>
>>>>>> this seems to use memset now on aarch64 before irelative
>>>>>> relocs are resolved in static binaries.
>>>>>>
>>>>>> which causes infinite loops in the iplt (i've also seen
>>>>>> segfaults in the build log).
>>>>>>
>>>>>> i wonder what is a clean fix...
>>>>>>
>>>>>
>>>>> Maybe add inhibit_loop_to_libcall to avoid the memset call.
>>>>
>>>> does not work for me..
>>>> happens at -O0 too
>>>> https://godbolt.org/z/W9r3nffYd
>>>
>>> Seems to happen with -fno-builtin or -ffreestanding as well.  It's a GCC
>>> bug, but I'll investigating working around it.
>>
>> I don't think it is gcc bug, even for -ffreestanding compiler can assume
>> the C environment will always provide memset/memcpy.  The problem is we
>> need a gcc option to avoid such optimization, which afaik there is none.
>>
>> And without such option I think we will need to either revert this change
>> or use the symbol alias strategy to avoid using IFUNC on loader code.
> 
> if we can rewrite the code not to depend on zeroing a large array
> (where large is about >200bytes) that should work too.
> 
> i dont see a clean way, but e.g. a
> 
> char seen_auxval[AT_MINSIGSTKSZ+1] = {0};
> 
> would not trigger memset (on aarch64).
> 
> as a bonus with such an array we can distinguish auxv unset and auxv 0.

I think this kind of solution is fragile, if compiler decides to change
the optimization threshold we will see a regression.