Re: [PATCH] fix static TLS consumption by TLS descriptors

[Kyle McMartin <kmcmarti at redhat dot com>]

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