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,
> rinThank 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.
signature.asc
Description: OpenPGP digital signature
