s390x __tls_get_addr_internal vs. __tls_get_offset

[jakubjelinek]

Summary

Symbol tls_get_addr_internal is a GLIBC_PRIVATE private symbol on s390{,x}, the glibc folks aren't very happy about asan using it.
Additionally, only recent glibc versions have it, older versions just have tls_get_offset and nothing else.
The patch doesn't drop the tls_get_addr_internal interception altogether, but changes it so that it calls real tls_get_offset function instead (and much more importantly,
that tls_get_offset interception calls the real tls_get_offset function).
This way it should work also on glibc 2.18 and earlier. See http://gcc.gnu.org/PR79341 for further details.

[kcc]

LGTM
This seems to be a S390-only change, right?
Is there anyone else who can review S390 code?
(I don't even pretend to understand what's going on here)
LGTM-ing anyway.

(BTW, please upload patches with full context next time)

[jakubjelinek]

In D29735#671362, @kcc wrote:

This seems to be a S390-only change, right?

Yes.

Is there anyone else who can review S390 code?

Maybe the IBM s390 GCC maintainers (I'll cross-link this page to the GCC PR).

(I don't even pretend to understand what's going on here)
LGTM-ing anyway.

[• koriakin]

lib/sanitizer_common/sanitizer_common_interceptors.inc

4668

Should be OK to just br %r3 here and skip the load instructions above.

Otherwise LGTM.

[jakubjelinek]

Updated patch for that.

[• koriakin]

LGTM with a nit.

lib/sanitizer_common/sanitizer_common_interceptors.inc

4631

Nit: the symbol is now hidden.

[jakubjelinek]

Shall I upload a new patch with that? What is the process now, can somebody check it in for me?

[kcc]

check-asan does not pass with this change due to lint"
/tmp/check_lint.XhMkOExnXU/tmp.TSPGZQsNod.sanitizer_common_interceptors.inc.cc:4611: Extra space before ( in function call [whitespace/parens] [4]
/tmp/check_lint.XhMkOExnXU/tmp.TSPGZQsNod.sanitizer_common_interceptors.inc.cc:4611: All parameters should be named in a function [readability/function] [3]
/tmp/check_lint.XhMkOExnXU/tmp.TSPGZQsNod.sanitizer_common_interceptors.inc.cc:4614: Extra space before ( in function call [whitespace/parens] [4]
/tmp/check_lint.XhMkOExnXU/tmp.TSPGZQsNod.sanitizer_common_interceptors.inc.cc:4615: Extra space before ( in function call [whitespace/parens] [4]

I can fix that of course, but I also want to ask one more thing: is this change testable?
Does it deserve an extra test, or the existing tests cover it?
Are there buildbots running these tests?

[jakubjelinek]

In D29735#672682, @kcc wrote:

check-asan does not pass with this change due to lint"
/tmp/check_lint.XhMkOExnXU/tmp.TSPGZQsNod.sanitizer_common_interceptors.inc.cc:4611: Extra space before ( in function call [whitespace/parens] [4]
/tmp/check_lint.XhMkOExnXU/tmp.TSPGZQsNod.sanitizer_common_interceptors.inc.cc:4611: All parameters should be named in a function [readability/function] [3]
/tmp/check_lint.XhMkOExnXU/tmp.TSPGZQsNod.sanitizer_common_interceptors.inc.cc:4614: Extra space before ( in function call [whitespace/parens] [4]
/tmp/check_lint.XhMkOExnXU/tmp.TSPGZQsNod.sanitizer_common_interceptors.inc.cc:4615: Extra space before ( in function call [whitespace/parens] [4]

Changed.

I can fix that of course, but I also want to ask one more thing: is this change testable?
Does it deserve an extra test, or the existing tests cover it?

Many existing tests cover it already (added a Report in there and it got printed many times during testing). So if the above would not work properly, you'd notice immediately.
If you want to see crashes without the patch, you'd need to have glibc 2.18 or older (one that doesn't provide __tls_get_offset_internal@@GLIBC_PRIVATE symbol).

Are there buildbots running these tests?

Yes.

lib/sanitizer_common/sanitizer_common_interceptors.inc

 //   descriptor offset as an argument instead of a pointer.  GOT address
 //   is passed in r12, so it's necessary to write it in assembly.  This is
 //   the function used by the compiler.
-#define INIT_TLS_GET_ADDR COMMON_INTERCEPT_FUNCTION(__tls_get_addr_internal)
+extern "C" uptr __tls_get_offset_wrapper (void *arg, uptr (*fn) (void *));
+#define INIT_TLS_GET_ADDR COMMON_INTERCEPT_FUNCTION(__tls_get_offset)
+DEFINE_REAL(uptr, __tls_get_offset, void *arg)
+extern "C" uptr __tls_get_offset (void *arg);
+extern "C" uptr __interceptor___tls_get_offset (void *arg);
 INTERCEPTOR(uptr, __tls_get_addr_internal, void *arg) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, __tls_get_addr_internal, arg);
-  uptr res = REAL(__tls_get_addr_internal)(arg);
+  uptr res = __tls_get_offset_wrapper(arg, REAL(__tls_get_offset));
   uptr tp = reinterpret_cast<uptr>(__builtin_thread_pointer());
   void *ptr = reinterpret_cast<void *>(res + tp);
   uptr tls_begin, tls_end;

 // We need a protected symbol aliasing the above, so that we can jump
 // directly to it from the assembly below.
 extern "C" __attribute__((alias("__interceptor___tls_get_addr_internal"),
-                          visibility("protected")))
-uptr __interceptor___tls_get_addr_internal_protected(void *arg);
+                          visibility("hidden")))
+uptr __tls_get_addr_hidden(void *arg);
 // Now carefully intercept __tls_get_offset.
 asm(
   ".text\n"
-  ".global __tls_get_offset\n"
-  "__tls_get_offset:\n"
 // The __intercept_ version has to exist, so that gen_dynamic_list.py
 // exports our symbol.
+  ".weak __tls_get_offset\n"
+  ".type __tls_get_offset, @function\n"
+  "__tls_get_offset:\n"
   ".global __interceptor___tls_get_offset\n"
+  ".type __interceptor___tls_get_offset, @function\n"
   "__interceptor___tls_get_offset:\n"
 #ifdef __s390x__
   "la %r2, 0(%r2,%r12)\n"
-  "jg __interceptor___tls_get_addr_internal_protected\n"
+  "jg __tls_get_addr_hidden\n"
 #else
   "basr %r3,0\n"
   "0: la %r2,0(%r2,%r12)\n"
   "l %r4,1f-0b(%r3)\n"
   "b 0(%r4,%r3)\n"
-  "1: .long __interceptor___tls_get_addr_internal_protected - 0b\n"
+  "1: .long __tls_get_addr_hidden - 0b\n"
 #endif
-  ".type __tls_get_offset, @function\n"
-  ".size __tls_get_offset, .-__tls_get_offset\n"
+  ".size __interceptor___tls_get_offset, .-__interceptor___tls_get_offset\n"
+// Assembly wrapper to call REAL(__tls_get_offset)(arg)
+  ".type __tls_get_offset_wrapper, @function\n"
+  "__tls_get_offset_wrapper:\n"
+#ifdef __s390x__
+  "sgr %r2,%r12\n"
+#else
+  "sr %r2,%r12\n"
+#endif
+  "br %r3\n"
+  ".size __tls_get_offset_wrapper, .-__tls_get_offset_wrapper\n"
 );
 #endif // SANITIZER_S390
 #else