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

[Florian Weimer <fweimer at redhat dot com>]

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