Re: Question about s390 THREAD_SET_STACK_GUARD

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 02/11/2018 19:40, Gc Frix wrote:
> Hello,
> 
> I was looking through the s390 sources and I noticed something I don't
> quite understand. From sysdeps/s390/nptl/tls.h:
> 
> ```
> /* Set the stack guard field in TCB head.  */
> #define THREAD_SET_STACK_GUARD(value) \
>   do \
>    { \
>      __asm__ __volatile__ ("" : : : "a0", "a1"); \
>      THREAD_SETMEM (THREAD_SELF, header.stack_guard, value); \
>    } \
>   while (0)
> 
> /* For reference, here's THREAD_SETMEM and THREAD_SELF.  */
> #define THREAD_SETMEM(descr, member, value) \
>   descr->member = (value)
> 
> # define THREAD_SELF ((struct pthread *) __builtin_thread_pointer ())
> ```
> 
> I can't figure out what the point of the asm in THREAD_SET_STACK_GUARD
> is. I know that for s390x, the thread pointer is split between a0 and
> a1, and that __builtin_thread_pointer() is a GCC builtin that represents
> them (__builtin_thread_pointer is completely undocumented for s390, by
> the way). What I don't understand is why the asm is necessary at all.
> The most I can figure is that it's meant to force a reload from a0 and
> a1, but why? Shouldn't they only ever get modified by context switches
> after they get set?
> 
> I'm new to glibc, so I apologize if this is a dumb question.
> 
> Regards,
> Giancarlo Frix
> 

For s390x the only symbol which the asm volatile seems to interfere is
security_init from ld.so:

$ diff -u ld-base.disas ld-patched.disas 
--- ld-base.disas       2018-11-05 10:02:30.575798123 -0200
+++ ld-patched.disas    2018-11-05 10:04:58.812381637 -0200
@@ -167,19 +167,19 @@
     1236:      07 07                   nopr    %r7
 
 0000000000001238 <security_init>:
-    1238:      c0 40 00 01 36 20       larl    %r4,27e78 <_dl_random>
-    123e:      e3 30 40 00 00 04       lg      %r3,0(%r4)
-    1244:      b2 4f 00 10             ear     %r1,%a0
-    1248:      eb 11 00 20 00 0d       sllg    %r1,%r1,32
-    124e:      b2 4f 00 11             ear     %r1,%a1
-    1252:      e3 50 30 00 00 04       lg      %r5,0(%r3)
-    1258:      a5 54 00 ff             nihh    %r5,255
-    125c:      c0 20 00 01 32 fe       larl    %r2,27858 <__pointer_chk_guard_local>
-    1262:      a7 09 00 00             lghi    %r0,0
-    1266:      e3 50 10 28 00 24       stg     %r5,40(%r1)
-    126c:      e3 10 30 08 00 04       lg      %r1,8(%r3)
-    1272:      e3 00 40 00 00 24       stg     %r0,0(%r4)
-    1278:      e3 10 20 00 00 24       stg     %r1,0(%r2)
+    1238:      c0 10 00 01 36 20       larl    %r1,27e78 <_dl_random>
+    123e:      b2 4f 00 20             ear     %r2,%a0
+    1242:      eb 22 00 20 00 0d       sllg    %r2,%r2,32
+    1248:      e3 40 10 00 00 04       lg      %r4,0(%r1)
+    124e:      b2 4f 00 21             ear     %r2,%a1
+    1252:      c0 30 00 01 33 03       larl    %r3,27858 <__pointer_chk_guard_local>
+    1258:      e3 50 40 00 00 04       lg      %r5,0(%r4)
+    125e:      a5 54 00 ff             nihh    %r5,255
+    1262:      e3 50 20 28 00 24       stg     %r5,40(%r2)
+    1268:      a7 29 00 00             lghi    %r2,0
+    126c:      e3 20 10 00 00 24       stg     %r2,0(%r1)
+    1272:      e3 10 40 08 00 04       lg      %r1,8(%r4)
+    1278:      e3 10 30 00 00 24       stg     %r1,0(%r3)
     127e:      07 fe                   br      %r14

- ld-base.disas: master glibc
- ld-patched.disas: master glibc with the asm removed.

I can't identify any significant change that indeed justify the asm
requirement neither git log gives any indication why this change
was added. Stefan and Andreas, do you why and if this is required
for s390?