fix static TLS consumption by TLS descriptors

  On Wed, May 14, 2014 at 04:32:51PM -0400, Kyle McMartin wrote:
>

rth points out something more sensible than the if (0) ugliness which I
like better. Thanks!
  

On 14 May 2014 21:54, Kyle McMartin <kmcmarti@redhat.com> wrote:
> On Wed, May 14, 2014 at 04:32:51PM -0400, Kyle McMartin wrote:
>>
>
> rth points out something more sensible than the if (0) ugliness which I
> like better. Thanks!

Yes, I think this version looks nicer. I guess you could try using a
more complicated #ifdef but I'm not sure if it would be any prettier.

It looks like it might be possible to abstract these details out into
two functions, one for REL and one for RELA. Did you investigate that?

It would be nice to make the test a part of the glibc test suite. It
might take a bit of work to translate into the glibc test framework
but would be worth it to make sure this area of the dynamic linker
gets tested.

Is aarch64 the only architecture in this set that uses the gnu2
dialect by default? (btw, typo in your commit message
s/ftls-dialect/mtls-dialect/)

> --- a/sysdeps/aarch64/dl-machine.h
> +++ b/sysdeps/aarch64/dl-machine.h
> @@ -295,7 +295,7 @@ elf_machine_rela (struct link_map *map, const ElfW(Rela) *reloc,
>  # ifndef SHARED
>                 CHECK_STATIC_TLS (map, sym_map);
>  # else
> -               if (!TRY_STATIC_TLS (map, sym_map))
> +               if (1)
>                   {
>                     td->arg = _dl_make_tlsdesc_dynamic
>                       (sym_map, sym->st_value + reloc->r_addend);
> diff --git a/sysdeps/arm/dl-machine.h b/sysdeps/arm/dl-machine.h
> index 899b256..f55a991 100644
> --- a/sysdeps/arm/dl-machine.h
> +++ b/sysdeps/arm/dl-machine.h
> @@ -458,7 +458,7 @@ elf_machine_rel (struct link_map *map, const Elf32_Rel *reloc,
>  #  ifndef SHARED
>                 CHECK_STATIC_TLS (map, sym_map);
>  #  else
> -               if (!TRY_STATIC_TLS (map, sym_map))
> +               if (1)
>                   {
>                     td->argument.pointer
>                       = _dl_make_tlsdesc_dynamic (sym_map, value);

I tested this on ARM and there were no regressions.

> diff --git a/sysdeps/i386/dl-machine.h b/sysdeps/i386/dl-machine.h
> index 368bee2..b6b5802 100644
> --- a/sysdeps/i386/dl-machine.h
> +++ b/sysdeps/i386/dl-machine.h
> @@ -394,7 +394,7 @@ elf_machine_rel (struct link_map *map, const Elf32_Rel *reloc,
>  #  ifndef SHARED
>                 CHECK_STATIC_TLS (map, sym_map);
>  #  else
> -               if (!TRY_STATIC_TLS (map, sym_map))
> +               if (1)
>                   {
>                     td->arg = _dl_make_tlsdesc_dynamic
>                       (sym_map, sym->st_value + (ElfW(Word))td->arg);
> diff --git a/sysdeps/x86_64/dl-machine.h b/sysdeps/x86_64/dl-machine.h
> index 8df04a9..4ec4340 100644
> --- a/sysdeps/x86_64/dl-machine.h
> +++ b/sysdeps/x86_64/dl-machine.h
> @@ -359,7 +359,7 @@ elf_machine_rela (struct link_map *map, const ElfW(Rela) *reloc,
>  #   ifndef SHARED
>                 CHECK_STATIC_TLS (map, sym_map);
>  #   else
> -               if (!TRY_STATIC_TLS (map, sym_map))
> +               if (1)
>                   {
>                     td->arg = _dl_make_tlsdesc_dynamic
>                       (sym_map, sym->st_value + reloc->r_addend);
  

Sorry for the delay, been a busy few weeks.

On Mon, May 19, 2014 at 12:25:55PM +0100, Will Newton wrote:
> On 14 May 2014 21:54, Kyle McMartin <kmcmarti@redhat.com> wrote:
> > On Wed, May 14, 2014 at 04:32:51PM -0400, Kyle McMartin wrote:
> >>
> >
> > rth points out something more sensible than the if (0) ugliness which I
> > like better. Thanks!
> 
> Yes, I think this version looks nicer. I guess you could try using a
> more complicated #ifdef but I'm not sure if it would be any prettier.
> 

The nesting makes it significantly worse, better to just let the
compiler to do the work here, rather than cpp.

> It looks like it might be possible to abstract these details out into
> two functions, one for REL and one for RELA. Did you investigate that?
> 

Not sure I follow you here.

> It would be nice to make the test a part of the glibc test suite. It
> might take a bit of work to translate into the glibc test framework
> but would be worth it to make sure this area of the dynamic linker
> gets tested.
> 

No objection from me, all of this TLS code is very much a
nest of vipers and corner cases.

> Is aarch64 the only architecture in this set that uses the gnu2
> dialect by default? (btw, typo in your commit message
> s/ftls-dialect/mtls-dialect/)
> 

Yes, it's the only one that uses this default in GCC. And, frankly,
given how this rtld code falls apart, I don't think anyone has even
bothered testing it anywhere else, much less used it in production.

--Kyle

> > --- a/sysdeps/aarch64/dl-machine.h
> > +++ b/sysdeps/aarch64/dl-machine.h
> > @@ -295,7 +295,7 @@ elf_machine_rela (struct link_map *map, const ElfW(Rela) *reloc,
> >  # ifndef SHARED
> >                 CHECK_STATIC_TLS (map, sym_map);
> >  # else
> > -               if (!TRY_STATIC_TLS (map, sym_map))
> > +               if (1)
> >                   {
> >                     td->arg = _dl_make_tlsdesc_dynamic
> >                       (sym_map, sym->st_value + reloc->r_addend);
> > diff --git a/sysdeps/arm/dl-machine.h b/sysdeps/arm/dl-machine.h
> > index 899b256..f55a991 100644
> > --- a/sysdeps/arm/dl-machine.h
> > +++ b/sysdeps/arm/dl-machine.h
> > @@ -458,7 +458,7 @@ elf_machine_rel (struct link_map *map, const Elf32_Rel *reloc,
> >  #  ifndef SHARED
> >                 CHECK_STATIC_TLS (map, sym_map);
> >  #  else
> > -               if (!TRY_STATIC_TLS (map, sym_map))
> > +               if (1)
> >                   {
> >                     td->argument.pointer
> >                       = _dl_make_tlsdesc_dynamic (sym_map, value);
> 
> I tested this on ARM and there were no regressions.
> 
> > diff --git a/sysdeps/i386/dl-machine.h b/sysdeps/i386/dl-machine.h
> > index 368bee2..b6b5802 100644
> > --- a/sysdeps/i386/dl-machine.h
> > +++ b/sysdeps/i386/dl-machine.h
> > @@ -394,7 +394,7 @@ elf_machine_rel (struct link_map *map, const Elf32_Rel *reloc,
> >  #  ifndef SHARED
> >                 CHECK_STATIC_TLS (map, sym_map);
> >  #  else
> > -               if (!TRY_STATIC_TLS (map, sym_map))
> > +               if (1)
> >                   {
> >                     td->arg = _dl_make_tlsdesc_dynamic
> >                       (sym_map, sym->st_value + (ElfW(Word))td->arg);
> > diff --git a/sysdeps/x86_64/dl-machine.h b/sysdeps/x86_64/dl-machine.h
> > index 8df04a9..4ec4340 100644
> > --- a/sysdeps/x86_64/dl-machine.h
> > +++ b/sysdeps/x86_64/dl-machine.h
> > @@ -359,7 +359,7 @@ elf_machine_rela (struct link_map *map, const ElfW(Rela) *reloc,
> >  #   ifndef SHARED
> >                 CHECK_STATIC_TLS (map, sym_map);
> >  #   else
> > -               if (!TRY_STATIC_TLS (map, sym_map))
> > +               if (1)
> >                   {
> >                     td->arg = _dl_make_tlsdesc_dynamic
> >                       (sym_map, sym->st_value + reloc->r_addend);
> 
> 
> 
> -- 
> Will Newton
> Toolchain Working Group, Linaro
  

On 3 June 2014 04:05, Kyle McMartin <kmcmarti@redhat.com> wrote:
> Sorry for the delay, been a busy few weeks.
>
> On Mon, May 19, 2014 at 12:25:55PM +0100, Will Newton wrote:
>> On 14 May 2014 21:54, Kyle McMartin <kmcmarti@redhat.com> wrote:
>> > On Wed, May 14, 2014 at 04:32:51PM -0400, Kyle McMartin wrote:
>> >>
>> >
>> > rth points out something more sensible than the if (0) ugliness which I
>> > like better. Thanks!
>>
>> Yes, I think this version looks nicer. I guess you could try using a
>> more complicated #ifdef but I'm not sure if it would be any prettier.
>>
>
> The nesting makes it significantly worse, better to just let the
> compiler to do the work here, rather than cpp.
>
>> It looks like it might be possible to abstract these details out into
>> two functions, one for REL and one for RELA. Did you investigate that?
>>
>
> Not sure I follow you here.

What I meant was that there is a lot of copy and paste here, maybe we
can clean that up. But your patch doesn't make it worse so its fine to
leave that for another day I think.

>> It would be nice to make the test a part of the glibc test suite. It
>> might take a bit of work to translate into the glibc test framework
>> but would be worth it to make sure this area of the dynamic linker
>> gets tested.
>>
>
> No objection from me, all of this TLS code is very much a
> nest of vipers and corner cases.
>
>> Is aarch64 the only architecture in this set that uses the gnu2
>> dialect by default? (btw, typo in your commit message
>> s/ftls-dialect/mtls-dialect/)
>>
>
> Yes, it's the only one that uses this default in GCC. And, frankly,
> given how this rtld code falls apart, I don't think anyone has even
> bothered testing it anywhere else, much less used it in production.

I tested the patch on ARM with -mtls-dialect=gnu2 and found one
unrelated glibc bug and two binutils bugs, so yes, it appears to be
lightly used. ;-)