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

["H.J. Lu" <hjl dot tools at gmail dot com>]

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.
>

Something like this.

-- 
H.J.

From 04186257d5d6fd0b6b8c69e86558209c3fa25902 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Tue, 4 Jul 2017 09:24:09 -0700
Subject: [PATCH] Add ___tls_get_addr

---
 sysdeps/unix/sysv/linux/x86_64/64/ld.abilist  | 2 ++
 sysdeps/unix/sysv/linux/x86_64/x32/ld.abilist | 2 ++
 sysdeps/x86_64/Versions                       | 6 ++++++
 sysdeps/x86_64/dl-tls.c                       | 6 +++---
 4 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/x86_64/64/ld.abilist b/sysdeps/unix/sysv/linux/x86_64/64/ld.abilist
index 07cab4b..884e52c 100644
--- a/sysdeps/unix/sysv/linux/x86_64/64/ld.abilist
+++ b/sysdeps/unix/sysv/linux/x86_64/64/ld.abilist
@@ -6,6 +6,8 @@ GLIBC_2.2.5 calloc F
 GLIBC_2.2.5 free F
 GLIBC_2.2.5 malloc F
 GLIBC_2.2.5 realloc F
+GLIBC_2.26 GLIBC_2.26 A
+GLIBC_2.26 ___tls_get_addr F
 GLIBC_2.3 GLIBC_2.3 A
 GLIBC_2.3 __tls_get_addr F
 GLIBC_2.4 GLIBC_2.4 A
diff --git a/sysdeps/unix/sysv/linux/x86_64/x32/ld.abilist b/sysdeps/unix/sysv/linux/x86_64/x32/ld.abilist
index 236357b..d42a7a4 100644
--- a/sysdeps/unix/sysv/linux/x86_64/x32/ld.abilist
+++ b/sysdeps/unix/sysv/linux/x86_64/x32/ld.abilist
@@ -7,3 +7,5 @@ GLIBC_2.16 calloc F
 GLIBC_2.16 free F
 GLIBC_2.16 malloc F
 GLIBC_2.16 realloc F
+GLIBC_2.26 GLIBC_2.26 A
+GLIBC_2.26 ___tls_get_addr F
diff --git a/sysdeps/x86_64/Versions b/sysdeps/x86_64/Versions
index a437f85..b8c25c9 100644
--- a/sysdeps/x86_64/Versions
+++ b/sysdeps/x86_64/Versions
@@ -10,3 +10,9 @@ libm {
     exp2l;
   }
 }
+ld {
+  GLIBC_2.26 {
+    # The alternative x86-64 runtime interface to TLS with aligned stack.
+    ___tls_get_addr;
+  }
+}
diff --git a/sysdeps/x86_64/dl-tls.c b/sysdeps/x86_64/dl-tls.c
index 567224e..3584805 100644
--- a/sysdeps/x86_64/dl-tls.c
+++ b/sysdeps/x86_64/dl-tls.c
@@ -25,13 +25,13 @@
 
 /* Define __tls_get_addr within elf/dl-tls.c under a different
    name.  */
-extern __typeof__ (__tls_get_addr) __tls_get_addr_default;
+extern __typeof__ (__tls_get_addr) ___tls_get_addr;
 
-# define __tls_get_addr __tls_get_addr_default
+# define __tls_get_addr ___tls_get_addr
 # include <elf/dl-tls.c>
 # undef __tls_get_addr
 
-hidden_ver (__tls_get_addr_default, __tls_get_addr)
+hidden_ver (___tls_get_addr, __tls_get_addr)
 
 /* Only handle slow paths for __tls_get_addr.  */
 attribute_hidden
-- 
2.9.4