[PATCHv3] powerpc: Fix float128 IFUNC relocations [BZ #21707]

Message ID 20170712185907.16165-1-tuliom@linux.vnet.ibm.com
State Superseded
Delegated to: Carlos O'Donell
Headers

Commit Message

Tulio Magno Quites Machado Filho July 12, 2017, 6:59 p.m. UTC
  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-12  Tulio Magno Quites Machado Filho  <tuliom@linux.vnet.ibm.com>

	[BZ #21707]
	* csu/libc-start.c (ARCH_APPLY_IREL, ARCH_SETUP_IREL): New macros.
	(LIBC_START_MAIN): Perform IREL{,A} relocations before or after
	initializing the TCB on statically linked executables.  That's a
	per-architecture definition.
	* 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.c [!SHARED]: Define
	ARCH_APPLY_IREL().
---
 csu/libc-start.c                             | 22 ++++++++-
 sysdeps/powerpc/Makefile                     | 10 +++-
 sysdeps/powerpc/powerpc64le/Makefile         | 10 ++++
 sysdeps/powerpc/tst-tlsifunc-static.c        | 19 ++++++++
 sysdeps/powerpc/tst-tlsifunc.c               | 68 ++++++++++++++++++++++++++++
 sysdeps/unix/sysv/linux/powerpc/libc-start.c |  2 +
 6 files changed, 128 insertions(+), 3 deletions(-)
 create mode 100644 sysdeps/powerpc/tst-tlsifunc-static.c
 create mode 100644 sysdeps/powerpc/tst-tlsifunc.c
  

Comments

H.J. Lu July 12, 2017, 7:42 p.m. UTC | #1
On Wed, Jul 12, 2017 at 11:59 AM, Tulio Magno Quites Machado Filho
<tuliom@linux.vnet.ibm.com> wrote:
> 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-12  Tulio Magno Quites Machado Filho  <tuliom@linux.vnet.ibm.com>
>
>         [BZ #21707]
>         * csu/libc-start.c (ARCH_APPLY_IREL, ARCH_SETUP_IREL): New macros.
>         (LIBC_START_MAIN): Perform IREL{,A} relocations before or after
>         initializing the TCB on statically linked executables.  That's a
>         per-architecture definition.
>         * 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.c [!SHARED]: Define
>         ARCH_APPLY_IREL().
>

It works in x86-64.

Thanks.
  
Carlos O'Donell July 12, 2017, 8:58 p.m. UTC | #2
On 07/12/2017 02:59 PM, Tulio Magno Quites Machado Filho wrote:
> 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.

Thanks for making this work for other arches that want IFUNC for use
during TLS setup.

I would like to point out that I think Intel's requirements are going
to be unique here to glibc, I don't want to allow IFUNCs in general
to run before TLS is setup, but because glibc can do so, is going to
be special. My design idea is that IFUNC resolvers need to allow TLS
usage.

> 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-12  Tulio Magno Quites Machado Filho  <tuliom@linux.vnet.ibm.com>
> 
> 	[BZ #21707]
> 	* csu/libc-start.c (ARCH_APPLY_IREL, ARCH_SETUP_IREL): New macros.
> 	(LIBC_START_MAIN): Perform IREL{,A} relocations before or after
> 	initializing the TCB on statically linked executables.  That's a
> 	per-architecture definition.
> 	* 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.c [!SHARED]: Define
> 	ARCH_APPLY_IREL().
> ---
>  csu/libc-start.c                             | 22 ++++++++-
>  sysdeps/powerpc/Makefile                     | 10 +++-
>  sysdeps/powerpc/powerpc64le/Makefile         | 10 ++++
>  sysdeps/powerpc/tst-tlsifunc-static.c        | 19 ++++++++
>  sysdeps/powerpc/tst-tlsifunc.c               | 68 ++++++++++++++++++++++++++++
>  sysdeps/unix/sysv/linux/powerpc/libc-start.c |  2 +
>  6 files changed, 128 insertions(+), 3 deletions(-)
>  create mode 100644 sysdeps/powerpc/tst-tlsifunc-static.c
>  create mode 100644 sysdeps/powerpc/tst-tlsifunc.c
> 
> diff --git a/csu/libc-start.c b/csu/libc-start.c
> index c2dd159..a2d1391 100644
> --- a/csu/libc-start.c
> +++ b/csu/libc-start.c
> @@ -108,6 +108,19 @@ apply_irel (void)
>  # define ARCH_INIT_CPU_FEATURES()
>  #endif
>  
> +#if defined ARCH_APPLY_IREL && defined ARCH_SETUP_IREL> +# error "Defining both ARCH_APPLY_IREL and ARCH_SETUP_IREL is prohibited."
> +#endif
> +
> +#ifdef ARCH_APPLY_IREL
> +# define ARCH_SETUP_IREL()
> +#elif defined ARCH_SETUP_IREL
> +# define ARCH_APPLY_IREL()
> +#else
> +# define ARCH_SETUP_IREL() apply_irel ()
> +# define ARCH_APPLY_IREL()
> +#endif

This is a typo-prone macro API and we want to move away from such default
defining constructs so that our -Wundef warnings apply.

We need:

* generic header:
  #define ARCH_SETUP_IREL() apply_irel ()
  #define ARCH_APPLY_IREL() ({})

* ppc64:
  #define ARCH_SETUP_IREL() ({})
  #define ARCH_APPLY_IREL() apply_irel ()

* libc-start.c includes generic header, which is overidden by ppc64 header.
  Uses ARCH_SETUP_IREL() and ARCH_APPLY_IREL() without chekcing they are
  defined. Then -Wundef provides protection against errors in port setup
  like 'ARCH_SETPU_IRELA() apply_irel ()' which is wrong.

Or something semantically equivalent using source and header files.

> +
>  STATIC int LIBC_START_MAIN (int (*main) (int, char **, char **
>  					 MAIN_AUXVEC_DECL),
>  			    int argc,
> @@ -189,11 +202,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 ();
>  
>    /* 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 ();
> +
>    /* 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 +242,7 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
>    __pointer_chk_guard_local = pointer_chk_guard;
>  # endif
>  
> -#endif
> +#endif /* !SHARED  */
>  
>    /* Register the destructor of the dynamic linker if there is any.  */
>    if (__glibc_likely (rtld_fini != NULL))
> 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 bd8a82d..90a41b1 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)
> +

OK.

>  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
>  $(objpfx)test-float128%.o $(objpfx)test-float128%.os: CFLAGS += -mfloat128
>  $(objpfx)test-ifloat128%.o $(objpfx)test-ifloat128%.os: CFLAGS += -mfloat128
>  CFLAGS-libm-test-support-float128.c += -mfloat128
> +$(objpfx)test-float128% $(objpfx)test-ifloat128%: \
> +  gnulib-tests += $(f128-loader-link)

OK.

>  endif
>  
>  # Append flags to string <-> _Float128 routines.
> @@ -24,6 +31,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.
> 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.

Suggest:

Test if an STT_GNU_IFUNC resolver can read from a TLS variable.

> +   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"
> diff --git a/sysdeps/powerpc/tst-tlsifunc.c b/sysdeps/powerpc/tst-tlsifunc.c
> new file mode 100644
> index 0000000..6d6af8a
> --- /dev/null
> +++ b/sysdeps/powerpc/tst-tlsifunc.c
> @@ -0,0 +1,68 @@
> +/* 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 <libc-symbols.h>
> +#include <tls-macros.h>
> +
> +__thread int bar;
> +static int *bar_ptr = NULL;
> +
> +int foo (void);
> +
> +void
> +init_foo (void)
> +{
> +  bar_ptr = TLS_GD (bar);

Why don't you also test writing to the TLS variable?

> +}
> +
> +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);
> +
> +static int
> +do_test (void)
> +{
> +  int ret = 0;
> +
> +  if (foo ())
> +    printf ("PASS: IFUNC resolver called once.\n");
> +  else
> +    {
> +      printf ("FAIL: IFUNC resolver not called once.\n");
> +      ret = 1;
> +    }
> +
> +  if (&bar == bar_ptr)
> +    printf ("PASS: Address read from IFUNC resolver is correct.\n");
> +  else
> +    {
> +      printf ("FAIL: Address 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.c b/sysdeps/unix/sysv/linux/powerpc/libc-start.c
> index ad036c1..dc2df3c 100644
> --- a/sysdeps/unix/sysv/linux/powerpc/libc-start.c
> +++ b/sysdeps/unix/sysv/linux/powerpc/libc-start.c
> @@ -22,6 +22,8 @@
>  
>  #ifndef SHARED
>  #include <hwcapinfo.h>
> +static void apply_irel (void);
> +# define ARCH_APPLY_IREL() apply_irel ()
>  #endif
>  
>  int __cache_line_size attribute_hidden;
>
  
Tulio Magno Quites Machado Filho July 13, 2017, 2:29 p.m. UTC | #3
Carlos O'Donell <carlos@redhat.com> writes:

> On 07/12/2017 02:59 PM, Tulio Magno Quites Machado Filho wrote:
>> diff --git a/csu/libc-start.c b/csu/libc-start.c
>> index c2dd159..a2d1391 100644
>> --- a/csu/libc-start.c
>> +++ b/csu/libc-start.c
>> @@ -108,6 +108,19 @@ apply_irel (void)
>>  # define ARCH_INIT_CPU_FEATURES()
>>  #endif
>>  
>> +#if defined ARCH_APPLY_IREL && defined ARCH_SETUP_IREL> +# error "Defining both ARCH_APPLY_IREL and ARCH_SETUP_IREL is prohibited."
>> +#endif
>> +
>> +#ifdef ARCH_APPLY_IREL
>> +# define ARCH_SETUP_IREL()
>> +#elif defined ARCH_SETUP_IREL
>> +# define ARCH_APPLY_IREL()
>> +#else
>> +# define ARCH_SETUP_IREL() apply_irel ()
>> +# define ARCH_APPLY_IREL()
>> +#endif
>
> This is a typo-prone macro API and we want to move away from such default
> defining constructs so that our -Wundef warnings apply.
>
> We need:
>
> * generic header:
>   #define ARCH_SETUP_IREL() apply_irel ()
>   #define ARCH_APPLY_IREL() ({})
>
> * ppc64:
>   #define ARCH_SETUP_IREL() ({})
>   #define ARCH_APPLY_IREL() apply_irel ()
>
> * libc-start.c includes generic header, which is overidden by ppc64 header.
>   Uses ARCH_SETUP_IREL() and ARCH_APPLY_IREL() without chekcing they are
>   defined. Then -Wundef provides protection against errors in port setup
>   like 'ARCH_SETPU_IRELA() apply_irel ()' which is wrong.
>
> Or something semantically equivalent using source and header files.

OK.  I'm fixing it.

>>  # When building glibc with support for _Float128, the powers of ten tables in
>>  # fpioconst.c and in the string conversion functions must be extended.
>> 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.
> 
> Suggest:
> 
> Test if an STT_GNU_IFUNC resolver can read from a TLS variable.

I validated my first version of the patch with a test that read from the TCB.
As this test is ppc-specific now, I can incorporate that test.

I explain why not TLS variables in the next block...

>> diff --git a/sysdeps/powerpc/tst-tlsifunc.c b/sysdeps/powerpc/tst-tlsifunc.c
>> new file mode 100644
>> index 0000000..6d6af8a
>> --- /dev/null
>> +++ b/sysdeps/powerpc/tst-tlsifunc.c
>> @@ -0,0 +1,68 @@
>> ...
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <libc-symbols.h>
>> +#include <tls-macros.h>
>> +
>> +__thread int bar;
>> +static int *bar_ptr = NULL;
>> +
>> +int foo (void);
>> +
>> +void
>> +init_foo (void)
>> +{
>> +  bar_ptr = TLS_GD (bar);
>
> Why don't you also test writing to the TLS variable?

Because dynamically-linked applications initialization executes the following
steps:

 1. Initialize TLS (including the TCB)
 2. Load and relocate DSOs (including IREL)
 3. Initialize thread-local variables, i.e. calling memset

Whatever you write in step 2 will be overwritten in step 3.
Writing (or reading) to TLS variables at step 2 require even more changes,
which I don't think are appropriate at this moment and are more than ppc*
need to build with the latest libgcc.
  
Carlos O'Donell July 13, 2017, 2:33 p.m. UTC | #4
On 07/13/2017 10:29 AM, Tulio Magno Quites Machado Filho wrote:
>> Why don't you also test writing to the TLS variable?
> 
> Because dynamically-linked applications initialization executes the following
> steps:
> 
>  1. Initialize TLS (including the TCB)
>  2. Load and relocate DSOs (including IREL)
>  3. Initialize thread-local variables, i.e. calling memset
> 
> Whatever you write in step 2 will be overwritten in step 3.
> Writing (or reading) to TLS variables at step 2 require even more changes,
> which I don't think are appropriate at this moment and are more than ppc*
> need to build with the latest libgcc.

That makes sense. Please say that in a comment, and we can expand the test
later when we fix IFUNC behaviours.
  

Patch

diff --git a/csu/libc-start.c b/csu/libc-start.c
index c2dd159..a2d1391 100644
--- a/csu/libc-start.c
+++ b/csu/libc-start.c
@@ -108,6 +108,19 @@  apply_irel (void)
 # define ARCH_INIT_CPU_FEATURES()
 #endif
 
+#if defined ARCH_APPLY_IREL && defined ARCH_SETUP_IREL
+# error "Defining both ARCH_APPLY_IREL and ARCH_SETUP_IREL is prohibited."
+#endif
+
+#ifdef ARCH_APPLY_IREL
+# define ARCH_SETUP_IREL()
+#elif defined ARCH_SETUP_IREL
+# define ARCH_APPLY_IREL()
+#else
+# define ARCH_SETUP_IREL() apply_irel ()
+# define ARCH_APPLY_IREL()
+#endif
+
 STATIC int LIBC_START_MAIN (int (*main) (int, char **, char **
 					 MAIN_AUXVEC_DECL),
 			    int argc,
@@ -189,11 +202,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 ();
 
   /* 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 ();
+
   /* 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 +242,7 @@  LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
   __pointer_chk_guard_local = pointer_chk_guard;
 # endif
 
-#endif
+#endif /* !SHARED  */
 
   /* Register the destructor of the dynamic linker if there is any.  */
   if (__glibc_likely (rtld_fini != NULL))
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
 
 ifeq ($(subdir),setjmp)
 ifeq (yes,$(build-shared))
diff --git a/sysdeps/powerpc/powerpc64le/Makefile b/sysdeps/powerpc/powerpc64le/Makefile
index bd8a82d..90a41b1 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
 $(objpfx)test-float128%.o $(objpfx)test-float128%.os: CFLAGS += -mfloat128
 $(objpfx)test-ifloat128%.o $(objpfx)test-ifloat128%.os: 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.
@@ -24,6 +31,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)
 
 # When building glibc with support for _Float128, the powers of ten tables in
 # fpioconst.c and in the string conversion functions must be extended.
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"
diff --git a/sysdeps/powerpc/tst-tlsifunc.c b/sysdeps/powerpc/tst-tlsifunc.c
new file mode 100644
index 0000000..6d6af8a
--- /dev/null
+++ b/sysdeps/powerpc/tst-tlsifunc.c
@@ -0,0 +1,68 @@ 
+/* 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 <libc-symbols.h>
+#include <tls-macros.h>
+
+__thread int bar;
+static int *bar_ptr = NULL;
+
+int foo (void);
+
+void
+init_foo (void)
+{
+  bar_ptr = TLS_GD (bar);
+}
+
+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);
+
+static int
+do_test (void)
+{
+  int ret = 0;
+
+  if (foo ())
+    printf ("PASS: IFUNC resolver called once.\n");
+  else
+    {
+      printf ("FAIL: IFUNC resolver not called once.\n");
+      ret = 1;
+    }
+
+  if (&bar == bar_ptr)
+    printf ("PASS: Address read from IFUNC resolver is correct.\n");
+  else
+    {
+      printf ("FAIL: Address 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.c b/sysdeps/unix/sysv/linux/powerpc/libc-start.c
index ad036c1..dc2df3c 100644
--- a/sysdeps/unix/sysv/linux/powerpc/libc-start.c
+++ b/sysdeps/unix/sysv/linux/powerpc/libc-start.c
@@ -22,6 +22,8 @@ 
 
 #ifndef SHARED
 #include <hwcapinfo.h>
+static void apply_irel (void);
+# define ARCH_APPLY_IREL() apply_irel ()
 #endif
 
 int __cache_line_size attribute_hidden;