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

[Andreas Krebbel <krebbel at linux dot vnet dot ibm dot com>]

Hi Siddhesh,

On 25/11/13 06:50, Siddhesh Poyarekar wrote:
...
> +# ifdef __s390x__
> +#  define LOAD_R12(r12) asm ("lgr %0,%%r12" : "=r" (r12) ::)
> +# elif defined __s390__
> +#  define LOAD_R12(r12) asm ("lr %0,%%r12" : "=r" (r12) ::)
> +# endif
> +
>  # define GET_ADDR_OFFSET \
>    (ti->ti_offset - (unsigned long) __builtin_thread_pointer ())
> 
> +/* Subtract contents of r12 from __TI.  We don't really care if this is the
> +   GOT 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 (); })
> +({									      \
> +  unsigned long int r12;						      \
> +  LOAD_R12 (r12);							      \
> +  ((void *) __tls_get_offset ((unsigned long int) (__ti) - r12)		      \
> +   + (unsigned long) __builtin_thread_pointer ());			      \
> +})

I cannot see why it shouldn't work that way. However, I think it is more readable to just set up the
GOT pointer in r12.  For __tls_get_offset it is a prerequisite to have the GOT pointer set up in r12
so it would appear more logical to me and it should not be any slower. The only user of this is
dlsym correct?

> 
>  #endif
> 
> diff --git a/sysdeps/s390/s390-32/tls-macros.h b/sysdeps/s390/s390-32/tls-macros.h
> index 8a0ad58..0a11998 100644
> --- a/sysdeps/s390/s390-32/tls-macros.h
> +++ b/sysdeps/s390/s390-32/tls-macros.h
> @@ -8,12 +8,17 @@
> 
>  #ifdef PIC
>  # define TLS_IE(x) \
> -  ({ unsigned long __offset;						      \
> +  ({ unsigned long __offset, __save;					      \
>       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:\tlr %1,%%r12\n\t"						      \
> +	  "l %%r12,0(%0)\n\t"                                                 \
> +	  "la %%r12,0(%%r12,%0)\n\t"					      \
> +	  "l %0,4(%0)\n\t"						      \
> +	  "l %0,0(%0,%%r12):tls_load:" #x "\n\t"			      \
> +	  "lr %%r12, %1\n"						      \
> +	  : "=&a" (__offset), "=&a" (__save) : : "cc" );		      \
>       (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..a65047c 100644
> --- a/sysdeps/s390/s390-64/tls-macros.h
> +++ b/sysdeps/s390/s390-64/tls-macros.h
> @@ -8,12 +8,15 @@
> 
>  #ifdef PIC
>  # define TLS_IE(x) \
> -  ({ unsigned long __offset;						      \
> +  ({ unsigned long __offset, __save;					      \
>       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" );				      \
> +	  "1:\tlgr %1,%%r12\n\t"					      \
> +	  "larl %%r12,_GLOBAL_OFFSET_TABLE_\n\t"			      \
> +	  "lg %0,0(%0)\n\t"						      \
> +	  "lg %0,0(%0,%%r12):tls_load:" #x "\n\t"			      \
> +	  "lgr %%r12,%1"						      \
> +	  : "=&a" (__offset), "=&a" (__save) : : "cc" );		      \
>       (int *) (__builtin_thread_pointer() + __offset); })
>  #else
>  # define TLS_IE(x) \
> 

For this code to work the GOT pointer is not required to be in r12. Can't you just set it up in a
compiler chosen reg? Something like this:

diff --git a/sysdeps/s390/s390-64/tls-macros.h b/sysdeps/s390/s390-64/tls-macros.h
index be8aa6c..0e97b61 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"                                                     \
+          ".quad " #x "@gotntpoff\n"                                         \
+         "0:\tlg %0,0(%0)\n\t"                                               \
+          "larl %1,_GLOBAL_OFFSET_TABLE_\n\t"                                \
+         "lg %0,0(%0,%1):tls_load:" #x                                       \
+         : "=&a" (__offset), "=&a" (__got) : : "cc" );                       \
      (int *) (__builtin_thread_pointer() + __offset); })
 #else
 # define TLS_IE(x) \


Thanks for taking care of this!

Bye,

-Andreas-