[PATCH][BZ #16214] Fix TLS access on S390 with -march=z10
Carlos O'Donell
carlos@redhat.com
Fri Nov 29 16:36:00 GMT 2013
On 11/28/2013 11:40 AM, Andreas Krebbel wrote:
> On Thu, Nov 28, 2013 at 05:53:10PM +0530, Siddhesh Poyarekar wrote:
>> How about just something like:
>>
>> strong_alias (__tls_get_addr, __tls_get_addr_internal);
>>
>> and then adding __tls_get_addr_internal as GLIBC_PRIVATE in Versions?
>> I believe that should keep __tls_get_addr private and export
>> __tls_get_addr_internal@@GLIBC_PRIVATE. I don't have a test box yet
>> to verify my claim, so this is again a suggestion based on just
>> reading the code.
>
> You are right it is really that simple. An alias removes the hidden
> attribute.
>
> Attached is an updated version of the patch addressing your comments.
>
> Bye,
>
> -Andreas-
Thanks for working this out with Siddhesh.
I'm also not a fan of the triple underscore, it's a terrible thing
that can waste precious time debugging if you don't notice the extra
underscore. I'm in favour of 0-2 underscores, and verbose names.
The x86 use of triple underscore for the regparm version of
__tls_get_addr is just gratuitously difficult to follow in all of
the redefinition games that we play for that symbol.
> 2013-11-28 Siddhesh Poyarekar <siddhesh@redhat.com>
> Andreas Krebbel <Andreas.Krebbel@de.ibm.com>
>
> [BZ #16214]
> * sysdeps/s390/dl-tls.h (__TLS_GET_ADDR): Invoke
> __tls_get_addr_internal instead of __tls_get_offset in order to
> avoid GOT pointer dependency. Make rtld export
> __tls_get_addr_internal@@GLIBC_PRIVATE while still hiding
> __tls_get_addr since we are a __tls_get_offset platform.
> * sysdeps/s390/s390-64/tls-macros.h (TLS_IE PIC): Don't rely on
> GOT pointer being set up before.
> * sysdeps/s390/s390-32/tls-macros.h (TLS_IE PIC): Likewise.
>
>
> commit e648fd8cb80f6ee97730cae5133250a38146dd1d
> Author: Andreas Krebbel <krebbel@linux.vnet.ibm.com>
> Date: Wed Nov 27 18:17:09 2013 +0100
>
> [BZ #16214] S/390: Fix TLS GOT pointer setup.
>
> diff --git a/sysdeps/s390/Versions b/sysdeps/s390/Versions
> index e18617c..baf9842 100644
> --- a/sysdeps/s390/Versions
> +++ b/sysdeps/s390/Versions
> @@ -3,4 +3,8 @@ ld {
> # runtime interface to TLS
> __tls_get_offset;
> }
> + GLIBC_PRIVATE {
> + # Exported by ld used by libc.
> + __tls_get_addr_internal;
> + }
OK.
> }
> diff --git a/sysdeps/s390/dl-tls.h b/sysdeps/s390/dl-tls.h
> index 68a5af4..3eb1c6b 100644
> --- a/sysdeps/s390/dl-tls.h
> +++ b/sysdeps/s390/dl-tls.h
> @@ -26,11 +26,21 @@ typedef struct
>
>
> #ifdef SHARED
> -/* This is the prototype for the GNU version. */
> -extern void *__tls_get_addr (tls_index *ti) attribute_hidden;
> +
OK.
> extern unsigned long __tls_get_offset (unsigned long got_offset);
>
> # ifdef IS_IN_rtld
> +
> +# include <shlib-compat.h>
> +
> +extern void *__tls_get_addr (tls_index *ti) attribute_hidden;
> +/* Make an temporary alias to get rid of the hidden attribute. The
> + hidden attribute is required to prevent __tls_get_addr from being
> + exported. */
OK if you expand this a bit:
/* Make a temporary alias of __tls_get_addr to remove the hidden attribute.
Then export __tls_get_addr as __tls_get_addr_internal for use from libc.
We do not want to export __tls_get_addr, but we do need to use it from
libc when looking up the address of a TLS variable. We don't use
__tls_get_offset because it requires r12 to be setup and that might
not always be true. Either way it's more optimal to use __tls_get_addr
directly (that's what __tls_get_offset does anyways). */
> +strong_alias (__tls_get_addr, __tls_get_addr_internal1);
> +versioned_symbol (ld, __tls_get_addr_internal1,
> + __tls_get_addr_internal, GLIBC_PRIVATE);
Call it __tls_get_addr_internal_tmp to indicate the temporary nature?
OK if __tls_get_addr_internal_tmp is a local symbol.
OK if __tls_get_addr_internal is the non-hidden global symbol with
default version GLIBC_PRIVATE e.g. __tls_get_addr_internal@@GLIBC_PRIVATE.
It seems a bit obtuse, but if you find a better way in the future we
can always clean it up. I assume you *need* the tmp symbol otherwise
it doesn't work correctly.
> +
> /* The special thing about the s390 TLS ABI is that we do not have the
> standard __tls_get_addr function but the __tls_get_offset function
> which differs in two important aspects:
> @@ -63,15 +73,21 @@ __tls_get_offset:\n\
> 1: .long __tls_get_addr - 0b\n\
> ");
> # endif
> -# endif
> +# else /* IS_IN_rtld */
> +extern void *__tls_get_addr_internal (tls_index *ti);
> +# endif /* !IS_IN_rtld */
>
> # define GET_ADDR_OFFSET \
> (ti->ti_offset - (unsigned long) __builtin_thread_pointer ())
>
> -# define __TLS_GET_ADDR(__ti) \
> - ({ extern char _GLOBAL_OFFSET_TABLE_[] attribute_hidden; \
> - (void *) __tls_get_offset ((char *) (__ti) - _GLOBAL_OFFSET_TABLE_) \
> - + (unsigned long) __builtin_thread_pointer (); })
> +/* Use the privately exported __tls_get_addr_internal instead of
> + __tls_get_offset in order to avoid the __tls_get_offset special
> + linkage requiring the GOT pointer to be set up in r12. The
> + compiler will take care of setting up r12 only if itself issued the
> + __tls_get_offset call. */
> +# define __TLS_GET_ADDR(__ti) \
> + ({ (void *) __tls_get_addr_internal ((char *) (__ti)) \
> + + (unsigned long) __builtin_thread_pointer (); })
>
> #endif
>
> diff --git a/sysdeps/s390/s390-32/tls-macros.h b/sysdeps/s390/s390-32/tls-macros.h
> index 8a0ad58..a592d81 100644
> --- a/sysdeps/s390/s390-32/tls-macros.h
> +++ b/sysdeps/s390/s390-32/tls-macros.h
> @@ -8,12 +8,15 @@
>
> #ifdef PIC
> # define TLS_IE(x) \
> - ({ unsigned long __offset; \
> + ({ unsigned long __offset, __got; \
> asm ("bras %0,1f\n" \
> - "0:\t.long " #x "@gotntpoff\n" \
> - "1:\tl %0,0(%0)\n\t" \
> - "l %0,0(%0,%%r12):tls_load:" #x \
> - : "=&a" (__offset) : : "cc" ); \
> + "0:\t.long _GLOBAL_OFFSET_TABLE_-0b\n\t" \
> + ".long " #x "@gotntpoff\n" \
> + "1:\tl %1,0(%0)\n\t" \
> + "la %1,0(%1,%0)\n\t" \
> + "l %0,4(%0)\n\t" \
> + "l %0,0(%0,%1):tls_load:" #x "\n" \
> + : "=&a" (__offset), "=&a" (__got) : : "cc" ); \
OK.
> (int *) (__builtin_thread_pointer() + __offset); })
> #else
> # define TLS_IE(x) \
> diff --git a/sysdeps/s390/s390-64/tls-macros.h b/sysdeps/s390/s390-64/tls-macros.h
> index be8aa6c..3c59436 100644
> --- a/sysdeps/s390/s390-64/tls-macros.h
> +++ b/sysdeps/s390/s390-64/tls-macros.h
> @@ -8,12 +8,13 @@
>
> #ifdef PIC
> # define TLS_IE(x) \
> - ({ unsigned long __offset; \
> - asm ("bras %0,1f\n" \
> - "0:\t.quad " #x "@gotntpoff\n" \
> - "1:\tlg %0,0(%0)\n\t" \
> - "lg %0,0(%0,%%r12):tls_load:" #x \
> - : "=&a" (__offset) : : "cc" ); \
> + ({ unsigned long __offset, __got; \
> + asm ("bras %0,0f\n\t" \
> + ".quad " #x "@gotntpoff\n" \
> + "0:\tlarl %1,_GLOBAL_OFFSET_TABLE_\n\t" \
> + "lg %0,0(%0)\n\t" \
> + "lg %0,0(%0,%1):tls_load:" #x "\n" \
> + : "=&a" (__offset), "=&a" (__got) : : "cc" ); \
OK.
> (int *) (__builtin_thread_pointer() + __offset); })
> #else
> # define TLS_IE(x) \
My stated preference for the tls-macros.h file still stands, but it's
not important. What's important is that we fix this bug before it starts
impacting s390 users.
OK with the above suggestions.
Cheers,
Carlos.
More information about the Libc-alpha
mailing list