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

["Tulio Magno Quites Machado Filho" <tuliom at linux dot vnet dot ibm dot com>]

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.

-- 
Tulio Magno