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

[Siddhesh Poyarekar <siddhesh at redhat dot com>]

On Wed, Nov 27, 2013 at 10:01:46PM +0100, Andreas Krebbel wrote:
> Hi,
> 
> in that version I'm using __tls_get_addr instead of __tls_get_offset.
> That way we can avoid the special linkage of __tls_get_offset which
> requires the GOT pointer to be set up before.  This makes the
> __TLS_GET_ADDR macro even a bit simpler.  On the downside we now have

Oh yeah, that's even better IMO - I didn't take that path because I
thought you might want __tls_get_offset for consistency.  Maybe we
need a comment that mentions this explicitly?

I've not actually tested the patch yet since I'm waiting for an s390x
box internally, so here's an initial review based on just code
reading.  I'm sure Carlos will have some comments on it too.

> to export __tls_get_addr from rtld.  Since I didn't find a way to
> override the symbol version of __tls_get_addr from GLIBC_2.3 to
> GLIBC_PRIVATE I've added a small wrapper where I could add the
> GLIBC_PRIVATE symbol version for.  The symbol exported from rtld is
> ___tls_get_addr@@GLIBC_PRIVATE now (note the 3 underscores).

Where is __tls_get_addr exported for GLIBC_2.3?  Or are we supposed to
avoid exporting __tls_get_addr if we're exporting __tls_get_offset?

> 
> For the TLS_IE macros I've used slightly modified version from
> Siddhesh's patch.
> 
> Thanks to Siddhesh for tracking this down and the initial patch!
> 
> I've verified with head GCC that it fixes the following regressions on
> s390x:
> 
> <   Failed to remake target file `/home/andreas/glibc/glibc-#-build/elf/tst-tls3.out'.
> <   Failed to remake target file `/home/andreas/glibc/glibc-#-build/elf/tst-tls4.out'.
> <   Failed to remake target file `/home/andreas/glibc/glibc-#-build/elf/tst-tls5.out'.
> <   Failed to remake target file `/home/andreas/glibc/glibc-#-build/elf/tst-tls6.out'.
> <   Failed to remake target file `/home/andreas/glibc/glibc-#-build/elf/tst-tls8.out'.
> <   Failed to remake target file `/home/andreas/glibc/glibc-#-build/elf/tst-tls16.out'.
> <   Failed to remake target file `/home/andreas/glibc/glibc-#-build/elf/tst-tls-dlinfo.out'.
> 
> Comments?
> 
> Bye,
> 
> -Andreas-
> 
> 
> 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
> 	instead of __tls_get_offset in order to avoid GOT pointer
> 	dependency.
> 	Make rtld export ___tls_get_addr@@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 3e376de624840ebe6d4cc22e33e844fc365bbe44
> 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..e349458 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;
> +  }
>  }
> diff --git a/sysdeps/s390/dl-tls.h b/sysdeps/s390/dl-tls.h
> index 68a5af4..870498d 100644
> --- a/sysdeps/s390/dl-tls.h
> +++ b/sysdeps/s390/dl-tls.h
> @@ -26,11 +26,20 @@ typedef struct
>  
>  
>  #ifdef SHARED
> -/* This is the prototype for the GNU version.  */
> -extern void *__tls_get_addr (tls_index *ti) attribute_hidden;
> -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;
> +
> +void *
> +__tls_get_addr_internal (tls_index *ti)
> +{
> +  return __tls_get_addr (ti);
> +}
> +versioned_symbol (ld, __tls_get_addr_internal, ___tls_get_addr, GLIBC_PRIVATE);
> +

I think the canonical way to avoid PLT dereference within ld.so or
libc.so is to have the function with the exported name and have an
rtld_hidden_def?  So in this case the function should be
___tls_get_addr with a rtld_hidden_def() directive.  Also (bikeshed) I
believe __tls_get_addr_internal would be a nicer name instead of the
extra underscore.

Of course, if __tls_get_addr can be exported directly as
GLIBC_PRIVATE, then all we need is a hidden symbol for __tls_get_addr
to avoid PLT dereference within the linker.

>  /* 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 +72,18 @@ __tls_get_offset:\n\
>  1:	.long	__tls_get_addr - 0b\n\
>  ");
>  #  endif
> -# endif
> +# else /* IS_IN_rtld */
> +/* This is the prototype for the GNU version.  */
> +extern void *___tls_get_addr (tls_index *ti);
> +extern unsigned long __tls_get_offset (unsigned long got_offset);
> +# 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 (); })
> +# define __TLS_GET_ADDR(__ti)				\
> +  ({ (void *) ___tls_get_addr ((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" );			      \
>       (int *) (__builtin_thread_pointer() + __offset); })
>  #else
>  # define TLS_IE(x) \

I am ambivalent about the use of r12 or another register for GOT for
these test macros.  While the r12 usage emulates the ABI exactly,
loading GOT in another register also does not break semantics of the
test itself.  Maybe debugging any failures in this test may confuse a
programmer looking at this for the first time since he would be
looking for a specific sequence of instructions for TLS access.  But
of course, it would mean that he didn't look at the code carefully
enough ;)

Thanks!
Siddhesh

> 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" );			      \
>       (int *) (__builtin_thread_pointer() + __offset); })
>  #else
>  # define TLS_IE(x) \
>