This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH][BZ #16214] Fix TLS access on S390 with -march=z10


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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]