x86-64: Align the stack in __tls_get_addr [BZ #21609]

  On Mon, Jul 3, 2017 at 9:47 PM, Florian Weimer <fweimer@redhat.com> wrote:
> On 07/03/2017 10:25 PM, H.J. Lu wrote:
>> I prefer to let GCC realign the stack for us.  What do you think?
>
> I tried that first, but it does not work with all the GCC versions we

That is true.

> support.  We would have to add a configure check and hope that older GCC
> versions never generate code which needs an aligned stack.
>
> Furthermore, the code GCC generates for stack realignment is really bad,

GCC generates very good code for stack realignment when
-maccumulate-outgoing-args is used.

> and __tls_get_addr appears to be quite performance-critical because not
> all callers cache the result.
>

Some comments on the patch at

https://sourceware.org/ml/libc-alpha/2017-06/msg00922.html

# define __tls_get_addr __tls_get_addr_default
+# include <elf/dl-tls.c>
+
+# undef __tls_get_addr_default
              ^^^^^^^ Shouldn't it be __tls_get_addr?

-#include <stdint.h>
-
 #ifndef _X86_64_DL_TLSDESC_H
-# define _X86_64_DL_TLSDESC_H 1
+#define _X86_64_DL_TLSDESC_H
+
+#include <stdint.h>
+#include <dl-tls.h>

 /* Type used to represent a TLS descriptor in the GOT.  */
 struct tlsdesc
@@ -39,12 +40,6 @@ struct tlsdesc
     };
 };

-typedef struct dl_tls_index
-{
-  uint64_t ti_module;
-  uint64_t ti_offset;
-} tls_index;
-
 /* Type used as the argument in a TLS descriptor for a symbol that
    needs dynamic TLS offsets.  */
 struct tlsdesc_dynamic_arg
@@ -59,12 +54,12 @@ extern ptrdiff_t attribute_hidden
   _dl_tlsdesc_resolve_rela(struct tlsdesc *on_rax),
   _dl_tlsdesc_resolve_hold(struct tlsdesc *on_rax);

-# ifdef SHARED
+#ifdef SHARED
 extern void *_dl_make_tlsdesc_dynamic (struct link_map *map,
        size_t ti_offset)
   internal_function attribute_hidden;

 extern ptrdiff_t attribute_hidden _dl_tlsdesc_dynamic(struct tlsdesc *);
-# endif
-
 #endif
+
+#endif /* _X86_64_DL_TLSDESC_H */

Is this sysdeps/x86_64/dl-tlsdesc.h change related to this?

__tls_get_addr_compat:
+ .type __tls_get_addr_compat,@function
+ .global __tls_get_addr_compat
+ strong_alias (__tls_get_addr_compat, __tls_get_addr)

We can use ENTRY/END here.  Why do we need __tls_get_addr_compat?
Can we just have __tls_get_addr?

Since we are talking performance here, we should add __tls_get_addr_slow
to only handle slow paths.

Here is the patch which implements those.  It is tested on x86-64
and x32.
  

On 07/04/2017 05:16 PM, H.J. Lu wrote:

>> Furthermore, the code GCC generates for stack realignment is really bad,
> 
> GCC generates very good code for stack realignment when
> -maccumulate-outgoing-args is used.

I wonder why that isn't enabled by the force attribute.

>> # define __tls_get_addr __tls_get_addr_default
>> +# include <elf/dl-tls.c>
>> +
>> +# undef __tls_get_addr_default
>              ^^^^^^^ Shouldn't it be __tls_get_addr?

Looks this way.  I'd have to re-test and see if it makes a difference.

>> -typedef struct dl_tls_index
>> -{
>> -  uint64_t ti_module;
>> -  uint64_t ti_offset;
>> -} tls_index;

> Is this sysdeps/x86_64/dl-tlsdesc.h change related to this?

It is because the patch includes both <dl-tls.h> and <dl-tlsdesc.h> from
the same file, causing a multiple definition error without the removal
of the conflicting definition.

> __tls_get_addr_compat:
> + .type __tls_get_addr_compat,@function
> + .global __tls_get_addr_compat
> + strong_alias (__tls_get_addr_compat, __tls_get_addr)
> 
> We can use ENTRY/END here.  Why do we need __tls_get_addr_compat?
> Can we just have __tls_get_addr?

That confuses the linker once we add the .symver directive.

> Since we are talking performance here, we should add __tls_get_addr_slow
> to only handle slow paths.

I'd prefer something that adds a new symbol version for the non-aligning
implementation, so that we eventually move away from the aligning one.

> Here is the patch which implements those.  It is tested on x86-64
> and x32.

Is this really needed on x32, too?  Did GCC have the same bug?

Thanks,
Florian
  

On Tue, Jul 4, 2017 at 8:47 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 07/04/2017 05:16 PM, H.J. Lu wrote:
>
>>> Furthermore, the code GCC generates for stack realignment is really bad,
>>
>> GCC generates very good code for stack realignment when
>> -maccumulate-outgoing-args is used.
>
> I wonder why that isn't enabled by the force attribute.

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81312
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81313

>>> # define __tls_get_addr __tls_get_addr_default
>>> +# include <elf/dl-tls.c>
>>> +
>>> +# undef __tls_get_addr_default
>>              ^^^^^^^ Shouldn't it be __tls_get_addr?
>
> Looks this way.  I'd have to re-test and see if it makes a difference.
>
>>> -typedef struct dl_tls_index
>>> -{
>>> -  uint64_t ti_module;
>>> -  uint64_t ti_offset;
>>> -} tls_index;
>
>> Is this sysdeps/x86_64/dl-tlsdesc.h change related to this?
>
> It is because the patch includes both <dl-tls.h> and <dl-tlsdesc.h> from
> the same file, causing a multiple definition error without the removal
> of the conflicting definition.

It is a cleanup.  But I don't believe it is required here.

>> __tls_get_addr_compat:
>> + .type __tls_get_addr_compat,@function
>> + .global __tls_get_addr_compat
>> + strong_alias (__tls_get_addr_compat, __tls_get_addr)
>>
>> We can use ENTRY/END here.  Why do we need __tls_get_addr_compat?
>> Can we just have __tls_get_addr?
>
> That confuses the linker once we add the .symver directive.

I don't think it is a case.  We just define a different __tls_get_addr for
x86-64.

>> Since we are talking performance here, we should add __tls_get_addr_slow
>> to only handle slow paths.
>
> I'd prefer something that adds a new symbol version for the non-aligning
> implementation, so that we eventually move away from the aligning one.

I thought about it.  There is no easy way to do it without linker help.  We
can add ___tls_get_addr, similar to i386, which will take an aligned
stack.  Linker must support ___tls_get_addr.   Then we can use weakref
to redirect __tls_get_addr to ___tls_get_addr if linker supports
___tls_get_addr and GCC doesn't.

>> Here is the patch which implements those.  It is tested on x86-64
>> and x32.
>
> Is this really needed on x32, too?  Did GCC have the same bug?
>

Yes, it is needed for x32 since the bug is only fixed on I thiuGCC 4.9 branch
and above.
  

On Tue, Jul 4, 2017 at 8:55 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Tue, Jul 4, 2017 at 8:47 AM, Florian Weimer <fweimer@redhat.com> wrote:
>> On 07/04/2017 05:16 PM, H.J. Lu wrote:
>>
>>>> Furthermore, the code GCC generates for stack realignment is really bad,
>>>
>>> GCC generates very good code for stack realignment when
>>> -maccumulate-outgoing-args is used.
>>
>> I wonder why that isn't enabled by the force attribute.
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81312
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81313
>
>>>> # define __tls_get_addr __tls_get_addr_default
>>>> +# include <elf/dl-tls.c>
>>>> +
>>>> +# undef __tls_get_addr_default
>>>              ^^^^^^^ Shouldn't it be __tls_get_addr?
>>
>> Looks this way.  I'd have to re-test and see if it makes a difference.
>>
>>>> -typedef struct dl_tls_index
>>>> -{
>>>> -  uint64_t ti_module;
>>>> -  uint64_t ti_offset;
>>>> -} tls_index;
>>
>>> Is this sysdeps/x86_64/dl-tlsdesc.h change related to this?
>>
>> It is because the patch includes both <dl-tls.h> and <dl-tlsdesc.h> from
>> the same file, causing a multiple definition error without the removal
>> of the conflicting definition.
>
> It is a cleanup.  But I don't believe it is required here.
>
>>> __tls_get_addr_compat:
>>> + .type __tls_get_addr_compat,@function
>>> + .global __tls_get_addr_compat
>>> + strong_alias (__tls_get_addr_compat, __tls_get_addr)
>>>
>>> We can use ENTRY/END here.  Why do we need __tls_get_addr_compat?
>>> Can we just have __tls_get_addr?
>>
>> That confuses the linker once we add the .symver directive.
>
> I don't think it is a case.  We just define a different __tls_get_addr for
> x86-64.
>
>>> Since we are talking performance here, we should add __tls_get_addr_slow
>>> to only handle slow paths.
>>
>> I'd prefer something that adds a new symbol version for the non-aligning
>> implementation, so that we eventually move away from the aligning one.
>
> I thought about it.  There is no easy way to do it without linker help.  We
> can add ___tls_get_addr, similar to i386, which will take an aligned
> stack.  Linker must support ___tls_get_addr.   Then we can use weakref
> to redirect __tls_get_addr to ___tls_get_addr if linker supports
> ___tls_get_addr and GCC doesn't.
>
>>> Here is the patch which implements those.  It is tested on x86-64
>>> and x32.
>>
>> Is this really needed on x32, too?  Did GCC have the same bug?
>>
>
> Yes, it is needed for x32 since the bug is only fixed on I thiuGCC 4.9 branch
> and above.
>
> --
> H.J.