This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH][BZ #16214] Fix TLS access on S390 with -march=z10
- From: "Carlos O'Donell" <carlos at redhat dot com>
- To: Andreas Krebbel <krebbel at linux dot vnet dot ibm dot com>, Siddhesh Poyarekar <siddhesh at redhat dot com>
- Cc: libc-alpha at sourceware dot org
- Date: Thu, 28 Nov 2013 12:36:24 -0500
- Subject: Re: [PATCH][BZ #16214] Fix TLS access on S390 with -march=z10
- Authentication-results: sourceware.org; auth=none
- References: <20131125055009 dot GK19834 at spoyarek dot pnq dot redhat dot com> <20131127210146 dot GA22879 at bart> <20131128043516 dot GD20495 at spoyarek dot pnq dot redhat dot com> <52971AFE dot 2070502 at linux dot vnet dot ibm dot com> <20131128122310 dot GG20495 at spoyarek dot pnq dot redhat dot com> <20131128164012 dot GA17885 at bart>
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.