Re: [PATCHv4] powerpc: Fix float128 IFUNC relocations [BZ #21707]

[Carlos O'Donell <carlos at redhat dot com>]

On 07/17/2017 10:05 AM, Tulio Magno Quites Machado Filho wrote:
> Changes since version 3:
> 
>  - Moved definition of ARCH_APPLY_IREL and ARCH_SETUP_IREL to
>    libc-start.h.
>  - Added a comment to elf/rtld.c explaining when thread-local variables
>    are available.
>  - Improved the test to read a TCB variable.

This very is good. No ifdef's in the generic libc main startup code, which
is the way it should be. Tests are good too.

OK to checkin with additional comments added.

Thanks for working through this.

In Fedora Rawhide / Fedora 27 I went with your v1 patch to get our mass
rebuild working. However, before we release F27 I'll update to this version
and drop the intermediate v1 patch.
 
> Changes since version 2:
> 
>  - Fixed a typo in the commit message.
>  - Fixed coding style in the test.
>  - Adapted csu/libc-start.c to let IREL relocation happen before or
>    after TLS initialization, depending on the target architecture.
>  - Removed comment from csu/libc-tls.c added in version 1.
>  - Limited the execution of the tests to multi-arch builds.
>  - Moved the tests to sysdeps/powerpc.
> 
> Changes since version 1:
> 
>  - Added a testcase.  This is now validating both statically and
>    dynamically linked executables.
>  - Fixed an issue in the $(foreach ..) in sysdeps/powerpc/powerpc64le/Makefile.
>  - Added a comment to csu/libc-start.c
>  - Added a comment to csu/libc-tls.c
> 
> -- 8< --
> 
> The patch proposed by Peter Bergner [1] to libgcc in order to fix
> [BZ #21707] adds a dependency on a symbol provided by the loader,
> forcing the loader to be linked to tests after libgcc was linked.
> 
> It also requires to read the thread pointer during IRELA relocations.
> 
> Tested on powerpc, powerpc64, powerpc64le, s390x and x86_64.
> 
> [1] https://sourceware.org/ml/libc-alpha/2017-06/msg01383.html
> 
> 2017-07-17  Tulio Magno Quites Machado Filho  <tuliom@linux.vnet.ibm.com>
> 
> 	[BZ #21707]
> 	* csu/libc-start.c (LIBC_START_MAIN): Perform IREL{,A}
> 	relocations before or after initializing the TCB on statically
> 	linked executables.  That's a per-architecture definition.
> 	* elf/rtld.c (dl_main): Add a comment about thread-local
> 	variables initialization.
> 	* sysdeps/generic/libc-start.h: New file.  Define
> 	ARCH_APPLY_IREL and ARCH_SETUP_IREL.
> 	* sysdeps/powerpc/Makefile:
> 	[$(subdir) = elf && $(multi-arch) != no] (tests-static-internal): Add tst-tlsifunc-static.
> 	[$(subdir) = elf && $(multi-arch) != no && $(build-shared) == yes]
> 	(tests-internal): Add tst-tlsifunc.
> 	* sysdeps/powerpc/tst-tlsifunc.c: New file.
> 	* sysdeps/powerpc/tst-tlsifunc-static.c: Likewise.
> 	* sysdeps/powerpc/powerpc64le/Makefile (f128-loader-link): New
> 	variable.
> 	[$(subdir) = math] (test-float128% test-ifloat128%): Force
> 	linking to the loader after linking to libgcc.
> 	[$(subdir) = wcsmbs || $(subdir) = stdlib] (bug-strtod bug-strtod2)
> 	(bug-strtod2 tst-strtod-round tst-wcstod-round tst-strtod6 tst-strrom)
> 	(tst-strfrom-locale strfrom-skeleton): Likewise.
> 	* sysdeps/unix/sysv/linux/powerpc/libc-start.h: New file.  Define
> 	ARCH_APPLY_IREL and ARCH_SETUP_IREL.
> ---
>  csu/libc-start.c                             |  11 ++-
>  elf/rtld.c                                   |   4 +-
>  sysdeps/generic/libc-start.h                 |  26 ++++++
>  sysdeps/powerpc/Makefile                     |  10 ++-
>  sysdeps/powerpc/powerpc64le/Makefile         |  10 +++
>  sysdeps/powerpc/tst-tlsifunc-static.c        |  19 ++++
>  sysdeps/powerpc/tst-tlsifunc.c               | 129 +++++++++++++++++++++++++++
>  sysdeps/unix/sysv/linux/powerpc/libc-start.h |  28 ++++++
>  8 files changed, 233 insertions(+), 4 deletions(-)
>  create mode 100644 sysdeps/generic/libc-start.h
>  create mode 100644 sysdeps/powerpc/tst-tlsifunc-static.c
>  create mode 100644 sysdeps/powerpc/tst-tlsifunc.c
>  create mode 100644 sysdeps/unix/sysv/linux/powerpc/libc-start.h
> 
> diff --git a/csu/libc-start.c b/csu/libc-start.c
> index c2dd159..6720617 100644
> --- a/csu/libc-start.c
> +++ b/csu/libc-start.c
> @@ -108,6 +108,8 @@ apply_irel (void)
>  # define ARCH_INIT_CPU_FEATURES()
>  #endif
>  
> +#include <libc-start.h>

OK.

> +
>  STATIC int LIBC_START_MAIN (int (*main) (int, char **, char **
>  					 MAIN_AUXVEC_DECL),
>  			    int argc,
> @@ -189,11 +191,16 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
>    ARCH_INIT_CPU_FEATURES ();
>  
>    /* Perform IREL{,A} relocations.  */
> -  apply_irel ();
> +  ARCH_SETUP_IREL ();

OK.

>  
>    /* The stack guard goes into the TCB, so initialize it early.  */
>    __libc_setup_tls ();
>  
> +  /* In some architectures, IREL{,A} relocations happen after TLS setup in
> +     order to let IFUNC resolvers benefit from TCB information, e.g. powerpc's
> +     hwcap and platform fields available in the TCB.  */
> +  ARCH_APPLY_IREL ();

OK. Nice comment.

> +
>    /* Set up the stack checker's canary.  */
>    uintptr_t stack_chk_guard = _dl_setup_stack_chk_guard (_dl_random);
>  # ifdef THREAD_SET_STACK_GUARD
> @@ -224,7 +231,7 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
>    __pointer_chk_guard_local = pointer_chk_guard;
>  # endif
>  
> -#endif
> +#endif /* !SHARED  */

OK.

>  
>    /* Register the destructor of the dynamic linker if there is any.  */
>    if (__glibc_likely (rtld_fini != NULL))
> diff --git a/elf/rtld.c b/elf/rtld.c
> index 65647fb..1772f89 100644
> --- a/elf/rtld.c
> +++ b/elf/rtld.c
> @@ -2208,7 +2208,9 @@ ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n",
>  
>    /* Now that we have completed relocation, the initializer data
>       for the TLS blocks has its final values and we can copy them
> -     into the main thread's TLS area, which we allocated above.  */
> +     into the main thread's TLS area, which we allocated above.
> +     Note: thread-local variables must only be accessed after completing
> +     the next step.  */

OK.

>    _dl_allocate_tls_init (tcbp);
>  
>    /* And finally install it for the main thread.  */
> diff --git a/sysdeps/generic/libc-start.h b/sysdeps/generic/libc-start.h
> new file mode 100644
> index 0000000..dfb1c75
> --- /dev/null
> +++ b/sysdeps/generic/libc-start.h
> @@ -0,0 +1,26 @@

Needs a one line header comment explaining what this file is.

"Generic definitions for libc main startup."

> +/* Copyright (C) 2017 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
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef _LIBC_START_H
> +#define _LIBC_START_H
> +
> +#ifndef SHARED

Should have a comment:

/* By default we perform STT_GNU_IFUNC resolution *before* TLS
   initialization, and this means you cannot, without machine
   knowledge, access TLS from a IFUNC resolver.  */

> +#define ARCH_SETUP_IREL() apply_irel ()
> +#define ARCH_APPLY_IREL()
> +#endif /* ! SHARED  */
> +
> +#endif /* _LIBC_START_H  */
> diff --git a/sysdeps/powerpc/Makefile b/sysdeps/powerpc/Makefile
> index e03a202..0d9206b 100644
> --- a/sysdeps/powerpc/Makefile
> +++ b/sysdeps/powerpc/Makefile
> @@ -11,7 +11,15 @@ sysdep-rtld-routines += dl-machine hwcapinfo
>  # Don't optimize GD tls sequence to LE.
>  LDFLAGS-tst-tlsopt-powerpc += -Wl,--no-tls-optimize
>  tests += tst-tlsopt-powerpc
> -endif
> +
> +ifneq (no,$(multi-arch))
> +tests-static += tst-tlsifunc-static
> +tests-internal += tst-tlsifunc-static
> +ifeq (yes,$(build-shared))
> +tests-internal += tst-tlsifunc
> +endif # build-shared
> +endif # multi-arch
> +endif # subdir = elf

OK.

>  
>  ifeq ($(subdir),setjmp)
>  ifeq (yes,$(build-shared))
> diff --git a/sysdeps/powerpc/powerpc64le/Makefile b/sysdeps/powerpc/powerpc64le/Makefile
> index 2c34f38..77617b6 100644
> --- a/sysdeps/powerpc/powerpc64le/Makefile
> +++ b/sysdeps/powerpc/powerpc64le/Makefile
> @@ -1,6 +1,11 @@
>  # When building float128 we need to ensure -mfloat128 is
>  # passed to all such object files.
>  
> +# libgcc requires __tcb_parse_hwcap_and_convert_at_platform when built with
> +# a binary128 type.  That symbol is provided by the loader on dynamically
> +# linked executables, forcing to link the loader after libgcc link.
> +f128-loader-link = $(as-needed) $(elf-objpfx)ld.so $(no-as-needed)
> +
>  ifeq ($(subdir),math)
>  # sqrtf128 requires emulation before POWER9.
>  CPPFLAGS += -I../soft-fp
> @@ -11,6 +16,8 @@ $(foreach suf,$(all-object-suffixes),%f128_r$(suf)): CFLAGS += -mfloat128
>  $(foreach suf,$(all-object-suffixes),$(objpfx)test-float128%$(suf)): CFLAGS += -mfloat128
>  $(foreach suf,$(all-object-suffixes),$(objpfx)test-ifloat128%$(suf)): CFLAGS += -mfloat128
>  CFLAGS-libm-test-support-float128.c += -mfloat128
> +$(objpfx)test-float128% $(objpfx)test-ifloat128%: \
> +  gnulib-tests += $(f128-loader-link)
>  endif
>  
>  # Append flags to string <-> _Float128 routines.
> @@ -28,6 +35,9 @@ CFLAGS-tst-strtod6.c += -mfloat128
>  CFLAGS-tst-strfrom.c += -mfloat128
>  CFLAGS-tst-strfrom-locale.c += -mfloat128
>  CFLAGS-strfrom-skeleton.c += -mfloat128
> +$(foreach test,bug-strtod bug-strtod2 bug-strtod2 tst-strtod-round \
> +tst-wcstod-round tst-strtod6 tst-strrom tst-strfrom-locale \
> +strfrom-skeleton,$(objpfx)$(test)): gnulib-tests += $(f128-loader-link)

OK.

>  
>  # When building glibc with support for _Float128, the powers of ten tables in
>  # fpioconst.c and in the string conversion functions must be extended.  Some
> diff --git a/sysdeps/powerpc/tst-tlsifunc-static.c b/sysdeps/powerpc/tst-tlsifunc-static.c
> new file mode 100644
> index 0000000..e5313af
> --- /dev/null
> +++ b/sysdeps/powerpc/tst-tlsifunc-static.c
> @@ -0,0 +1,19 @@
> +/* Test if an executable can read from the TLS from an STT_GNU_IFUNC resolver.
> +   Copyright (C) 2017 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
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include "tst-tlsifunc.c"

OK.

> diff --git a/sysdeps/powerpc/tst-tlsifunc.c b/sysdeps/powerpc/tst-tlsifunc.c
> new file mode 100644
> index 0000000..0a8bdbf
> --- /dev/null
> +++ b/sysdeps/powerpc/tst-tlsifunc.c
> @@ -0,0 +1,129 @@
> +/* Test if an executable can read from the TLS from an STT_GNU_IFUNC resolver.
> +   Copyright (C) 2017 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
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <stdint.h>
> +#include <inttypes.h>
> +#include <libc-symbols.h>
> +#include <tls-macros.h>
> +
> +__thread int bar;
> +static int *bar_ptr = NULL;
> +
> +static uint32_t resolver_platform = 0;
> +
> +int foo (void);
> +
> +int tcb_test (void);
> +
> +/* Offsets copied from tcb-offsets.h.  */
> +#ifdef __powerpc64__
> +# define __TPREG     "r13"
> +# define __ATPLATOFF -28764
> +#else
> +# define __TPREG     "r2"
> +# define __ATPLATOFF -28724
> +#endif
> +
> +uint32_t
> +get_platform (void)
> +{
> +  register unsigned long tp __asm__ (__TPREG);
> +  uint32_t tmp;
> +
> +  __asm__  ("lwz %0,%1(%2)\n"
> +	    : "=r" (tmp)
> +	    : "i" (__ATPLATOFF), "b" (tp));
> +
> +  return tmp;
> +}
> +
> +void
> +init_foo (void)
> +{
> +  bar_ptr = TLS_GD (bar);

OK.

> +}
> +
> +int
> +my_foo (void)
> +{
> +  printf ("&bar = %p and bar_ptr = %p.\n", &bar, bar_ptr);
> +  return bar_ptr != NULL;
> +}
> +
> +__ifunc (foo, foo, my_foo, void, init_foo);
> +
> +void
> +init_tcb_test (void)
> +{
> +  resolver_platform = get_platform ();
> +}
> +
> +int
> +my_tcb_test (void)
> +{
> +  printf ("resolver_platform = 0x%"PRIx32
> +	  " and current platform = 0x%"PRIx32".\n",
> +	  resolver_platform, get_platform ());
> +  return resolver_platform != 0;

OK. Ensuring this is setup early enough.

> +}
> +
> +__ifunc (tcb_test, tcb_test, my_tcb_test, void, init_tcb_test);
> +
> +static int
> +do_test (void)
> +{
> +  int ret = 0;
> +
> +  if (foo ())
> +    printf ("PASS: foo IFUNC resolver called once.\n");
> +  else
> +    {
> +      printf ("FAIL: foo IFUNC resolver not called once.\n");
> +      ret = 1;
> +    }
> +
> +  if (&bar == bar_ptr)
> +    printf ("PASS: bar address read from IFUNC resolver is correct.\n");
> +  else
> +    {
> +      printf ("FAIL: bar address read from IFUNC resolver is incorrect.\n");
> +      ret = 1;
> +    }
> +
> +  if (tcb_test ())
> +    printf ("PASS: tcb_test IFUNC resolver called once.\n");
> +  else
> +    {
> +      printf ("FAIL: tcb_test IFUNC resolver not called once.\n");
> +      ret = 1;
> +    }
> +
> +  if (resolver_platform == get_platform ())

OK. Results match calling again at a later time in the startup (after main).

> +    printf ("PASS: platform read from IFUNC resolver is correct.\n");
> +  else
> +    {
> +      printf ("FAIL: platform read from IFUNC resolver is incorrect.\n");
> +      ret = 1;
> +    }
> +
> +  return ret;
> +}
> +
> +#include <support/test-driver.c>
> diff --git a/sysdeps/unix/sysv/linux/powerpc/libc-start.h b/sysdeps/unix/sysv/linux/powerpc/libc-start.h
> new file mode 100644
> index 0000000..c8af068
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/powerpc/libc-start.h
> @@ -0,0 +1,28 @@

Needs a one line description.

> +/* Copyright (C) 2017 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
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef _LIBC_START_H
> +#define _LIBC_START_H
> +
> +#ifndef SHARED
> +/* IREL{,A} must happen after TCB initialization in order to allow IFUNC
> +   resolvers to read TCB fields, e.g. hwcap and at_platform.  */

OK. Good comment.

> +#define ARCH_SETUP_IREL()
> +#define ARCH_APPLY_IREL() apply_irel ()
> +#endif /* ! SHARED  */
> +
> +#endif /* _LIBC_START_H  */
>