On 11.09.2020 07:13, Rin Okuyama wrote:
> Hi again,
>
> On 2020/09/10 21:53, Kamil Rytarowski wrote:
>> Module Name: src
>> Committed By: kamil
>> Date: Thu Sep 10 12:53:06 UTC 2020
>>
>> Modified Files:
>> src/external/gpl3/gcc/dist/libsanitizer/sanitizer_common:
>> sanitizer_linux_libcdep.cc
>> src/external/gpl3/gcc/lib: Makefile.sanitizer
>>
>> Log Message:
>> Avoid using internal RTLD/libpthread/libc symbol in sanitizers
>>
> ...
>> Index:
>> src/external/gpl3/gcc/dist/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc
>>
>> diff -u
>> src/external/gpl3/gcc/dist/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc:1.15
>> src/external/gpl3/gcc/dist/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc:1.16
>>
>> ---
>> src/external/gpl3/gcc/dist/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc:1.15
>> Mon Sep 7 07:10:43 2020
>> +++
>> src/external/gpl3/gcc/dist/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc
>> Thu Sep 10 12:53:05 2020
>> @@ -47,6 +47,7 @@
>> #if SANITIZER_NETBSD
>> #include <sys/sysctl.h>
>> #include <sys/tls.h>
>> +#include <lwp.h>
>> #endif
>> #if SANITIZER_SOLARIS
>> @@ -417,13 +418,7 @@ uptr ThreadSelf() {
>> #if SANITIZER_NETBSD
>> static struct tls_tcb * ThreadSelfTlsTcb() {
>> - struct tls_tcb * tcb = NULL;
>> -# ifdef __HAVE___LWP_GETTCB_FAST
>> - tcb = (struct tls_tcb *)__lwp_gettcb_fast();
>> -# elif defined(__HAVE___LWP_GETPRIVATE_FAST)
>> - tcb = (struct tls_tcb *)__lwp_getprivate_fast();
>> -# endif
>> - return tcb;
>> + return (struct tls_tcb *)_lwp_getprivate();
>> }
>> uptr ThreadSelf() {
>>
>
> This change breaks at least mips and powerpc, in which the return value of
> __lwp_getprivate(2), i.e., curlwp->l_private is not tcb address itself, but
> biased one. On the other hand, the return value of __lwp_gettcb_fast() is
> unbiased address; see sys/arch/{mips,powerpc}/include/mcontext.h.
>
> For powerpc, I recently attempted to change l_private to store tcb address
> itself:
>
> http://www.nerv.org/netbsd/?q=id:20200621T004000Z.95c1a18070b53713ce2c763df7f40743bf74172c
>
>
> But I reverted it soon as requested by joerg:
>
> http://www.nerv.org/netbsd/?q=id:20200622T053457Z.05db3be87b5ad499f5d1adba755bc573fd241c87
>
>
> His reasoning was that kernel must not know the ABI details in userland.
> I fully agree with this. See above links for more details.
>
> Thanks,
> rin
Thank you for noting it!
This is strange as I assumed that _lwp_getprivate() returns always the
correct private pointer and it is abstraction over fast ABI specific
calls . Also the usage of _lwp_getprivate() was suggested by Joerg back
then in sanitizers.
So we want exported to userland functionality to get the tls_tcb
pointer, something without using the internal RTLS/LIBPTHREAD/LIBC
namespaces.
The current code is confusing, as it attempts to use unimplemented
_PTHREAD_GETTCB_EXT() and in one place uses _lwp_getprivate_fast() in
other _lwp_getprivate(). This caused my confusion... as I assumed that
_lwp_getprivate_fast() is internal and _lwp_getprivate() for public
consumption.
https://nxr.netbsd.org/xref/src/lib/libpthread/pthread_int.h#266
263 static inline pthread_t __constfunc
264 pthread__self(void)
265 {
266 #if defined(_PTHREAD_GETTCB_EXT)
267 struct tls_tcb * const tcb = _PTHREAD_GETTCB_EXT();
268 #elif defined(__HAVE___LWP_GETTCB_FAST)
269 struct tls_tcb * const tcb = __lwp_gettcb_fast();
270 #else
271 struct tls_tcb * const tcb = __lwp_getprivate_fast();
272 #endif
273 return (pthread_t)tcb->tcb_pthread;
274 }
https://nxr.netbsd.org/xref/src/lib/libpthread/pthread.c#1268
1268 #if defined(_PTHREAD_GETTCB_EXT)
1269 pthread__main->pt_tls = _PTHREAD_GETTCB_EXT();
1270 #elif defined(__HAVE___LWP_GETTCB_FAST)
1271 pthread__main->pt_tls = __lwp_gettcb_fast();
1272 #else
1273 pthread__main->pt_tls = _lwp_getprivate();
1274 #endif
1275 pthread__main->pt_tls->tcb_pthread = pthread__main;
https://nxr.netbsd.org/xref/src/libexec/ld.elf_so/tls.c#294
293 #ifdef __HAVE___LWP_GETTCB_FAST
294 struct tls_tcb * const tcb = __lwp_gettcb_fast();
295 #else
296 struct tls_tcb * const tcb = __lwp_getprivate_fast();
297 #endif
1. Could we please synchronize above three code chunks, avoiding the
situation of having each of them implemented differently?
2. Could we please export _rtld_tls_self() or something similar and
register in <sys/tls.h> ?
Does this patch look good?
https://www.netbsd.org/~kamil/patch-00278-_rtld_tls_self.txt
In the worst case I will need to reexpose internal APIs in sanitizers
and pick one of the above tls_tcb retrieval implementations and use in
LLVM/GCC sanitizers.
PS. There is an ongoing GCC and Linux kernel discussion on a related
topic in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96200 "Implement
__builtin_thread_pointer() and __builtin_set_thread_pointer() if TLS is
supported" and they implemented this already for x86. GCC is now growing
__builtin_thread_pointer() for i386/amd64.
Attachment:
signature.asc
Description: OpenPGP digital signature