Re: [PATCH 6/7] Delay initialization of CPU features struct in static binaries

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 11/05/2017 11:51, Siddhesh Poyarekar wrote:
> Allow the CPU features structure set up to be overridden by tunables
> by delaying it to until after tunables are initialized.  The
> initialization is already delayed in dynamically linked glibc, it is
> only in static binaries that the initialization is set early to allow
> it to influence IFUNC relocations that happen in libc-start.  It is a
> bit too early however and there is a good place between tunables
> initialization and IFUNC relocations where this can be done.
> 
> Verified that this does not regress the testsuite.
> 
> 	* csu/libc-start.c [!ARCH_INIT_CPU_FEATURES]: Define
> 	ARCH_INIT_CPU_FEATURES.
> 	(LIBC_START_MAIN): Call it.
> 	* sysdeps/unix/sysv/linux/aarch64/libc-start.c
> 	(__libc_start_main): Remove.
> 	(ARCH_INIT_CPU_FEATURES): New macro.
> 	* sysdeps/x86/libc-start.c (__libc_start_main): Remove.
> 	(ARCH_INIT_CPU_FEATURES): New macro.

LGTM with some nits about indentation.

> ---
>  csu/libc-start.c                             |  6 ++++++
>  sysdeps/unix/sysv/linux/aarch64/libc-start.c | 23 +++++------------------
>  sysdeps/x86/libc-start.c                     | 23 +++++------------------
>  3 files changed, 16 insertions(+), 36 deletions(-)
> 
> diff --git a/csu/libc-start.c b/csu/libc-start.c
> index 9a56dcb..c2dd159 100644
> --- a/csu/libc-start.c
> +++ b/csu/libc-start.c
> @@ -104,6 +104,10 @@ apply_irel (void)
>  # define MAIN_AUXVEC_PARAM
>  #endif
>  
> +#ifndef ARCH_INIT_CPU_FEATURES
> +# define ARCH_INIT_CPU_FEATURES()
> +#endif
> +
>  STATIC int LIBC_START_MAIN (int (*main) (int, char **, char **
>  					 MAIN_AUXVEC_DECL),
>  			    int argc,
> @@ -182,6 +186,8 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
>  
>    __tunables_init (__environ);
>  
> +  ARCH_INIT_CPU_FEATURES ();
> +
>    /* Perform IREL{,A} relocations.  */
>    apply_irel ();
>  
> diff --git a/sysdeps/unix/sysv/linux/aarch64/libc-start.c b/sysdeps/unix/sysv/linux/aarch64/libc-start.c
> index a5babd4..46041fe 100644
> --- a/sysdeps/unix/sysv/linux/aarch64/libc-start.c
> +++ b/sysdeps/unix/sysv/linux/aarch64/libc-start.c
> @@ -16,26 +16,13 @@
>     License along with the GNU C Library; if not, see
>     <http://www.gnu.org/licenses/>.  */
>  
> -#ifdef SHARED
> -# include <csu/libc-start.c>
> -# else
> -/* The main work is done in the generic function.  */
> -# define LIBC_START_DISABLE_INLINE
> -# define LIBC_START_MAIN generic_start_main
> -# include <csu/libc-start.c>
> +#ifndef SHARED
> +#include <ldsodefs.h>
>  # include <cpu-features.c>
>  
>  extern struct cpu_features _dl_aarch64_cpu_features;
>  
> -int
> -__libc_start_main (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
> -		   int argc, char **argv,
> -		   __typeof (main) init,
> -		   void (*fini) (void),
> -		   void (*rtld_fini) (void), void *stack_end)
> -{
> -  init_cpu_features (&_dl_aarch64_cpu_features);
> -  return generic_start_main (main, argc, argv, init, fini, rtld_fini,
> -			     stack_end);
> -}
> +#define ARCH_INIT_CPU_FEATURES() init_cpu_features (&_dl_aarch64_cpu_features)
> +
>  #endif
> +# include <csu/libc-start.c>

Resulting indentation seems odd, I think it should be:

#ifndef SHARED
# include <ldsodefs.h>
# include <cpu-features.c>

extern struct cpu_features _dl_aarch64_cpu_features;

# define ARCH_INIT_CPU_FEATURES() init_cpu_features (&_dl_aarch64_cpu_features)

#endif
#include <csu/libc-start.c>

Same for x86.

> diff --git a/sysdeps/x86/libc-start.c b/sysdeps/x86/libc-start.c
> index 9a56adc..e11b490 100644
> --- a/sysdeps/x86/libc-start.c
> +++ b/sysdeps/x86/libc-start.c
> @@ -15,27 +15,14 @@
>     License along with the GNU C Library; if not, see
>     <http://www.gnu.org/licenses/>.  */
>  
> -#ifdef SHARED
> -# include <csu/libc-start.c>
> -# else
> -/* The main work is done in the generic function.  */
> -# define LIBC_START_DISABLE_INLINE
> -# define LIBC_START_MAIN generic_start_main
> -# include <csu/libc-start.c>
> +#ifndef SHARED
> +#include <ldsodefs.h>
>  # include <cpu-features.h>
>  # include <cpu-features.c>
>  
>  extern struct cpu_features _dl_x86_cpu_features;
>  
> -int
> -__libc_start_main (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
> -		   int argc, char **argv,
> -		   __typeof (main) init,
> -		   void (*fini) (void),
> -		   void (*rtld_fini) (void), void *stack_end)
> -{
> -  init_cpu_features (&_dl_x86_cpu_features);
> -  return generic_start_main (main, argc, argv, init, fini, rtld_fini,
> -			     stack_end);
> -}
> +#define ARCH_INIT_CPU_FEATURES() init_cpu_features (&_dl_x86_cpu_features)
> +
>  #endif
> +# include <csu/libc-start.c>
>