Reimplement Thread Static Data ASan routines with TLS

[krytarowski]

Summary

Thread Static Data cannot be used in early init on NetBSD
and FreeBSD. Reuse the ASan TSD API for compatibility with
existing code with an alternative implementation using Thread
Local Storage.

New version uses Thread Local Storage to store a pointer
with thread specific data. The destructor from TSD has been
replaced with a TLS destrucutor that is called upon thread
exit.

[krytarowski]

@vitalybuka this code can became the only one as it shall be portable to others and it might be needed for them too.

I recall that FreeBSD had problems too.

[devnexen]

In D55596#1328181, @krytarowski wrote:

I recall that FreeBSD had problems too.

True.

[krytarowski]

• add missing chunk of patch

[krytarowski]

I will follow up with analogous code in MSan and HWASan.

[krytarowski]

I made a mistake, the destructor must be called for all threads, not just the main one.

[krytarowski]

• new version

[krytarowski]

• replace __weak with a compiler-rt style attribute

[krytarowski]

I would like to make this version as the mainline for all POSIX OSes.

[krytarowski]

@devnexen does it work on FreeBSD?

[devnexen]

In D55596#1332415, @krytarowski wrote:

@devnexen does it work on FreeBSD?

i get more warning messages with the patch than without

__cxa_thread_call_dtors: dtr <address> from unloaded so, skipping

[krytarowski]

In D55596#1332421, @devnexen wrote:

In D55596#1332415, @krytarowski wrote:

@devnexen does it work on FreeBSD?

i get more warning messages with the patch than without

__cxa_thread_call_dtors: dtr <address> from unloaded so, skipping

I can try to rework it into a static TLS object with a destructor that will call the cleanup thread routine... but I will wait for feedback from @vitalybuka.

[krytarowski]

In D55596#1332421, @devnexen wrote:

In D55596#1332415, @krytarowski wrote:

@devnexen does it work on FreeBSD?

i get more warning messages with the patch than without

__cxa_thread_call_dtors: dtr <address> from unloaded so, skipping

How about this version: http://netbsd.org/~kamil/llvm/asan-posix-tls.txt
Does it work on FreeBSD? If so, how about Linux and Darwin.

[devnexen]

How about this version: http://netbsd.org/~kamil/llvm/asan-posix-tls.txt
Does it work on FreeBSD? If so, how about Linux and Darwin.

Seems to work fine with this version.

[krytarowski]

In D55596#1334282, @devnexen wrote:

How about this version: http://netbsd.org/~kamil/llvm/asan-posix-tls.txt
Does it work on FreeBSD? If so, how about Linux and Darwin.

Seems to work fine with this version.

On what OSes?

I'm going to set is as the only version but I need confirmation from Darwin and Linux.

[devnexen]

Sorry I meant FreeBSD here.

[krytarowski]

@ro could you please test this patch for Solaris? I need to know whether it works with TLS destructor on SunOS.

[krytarowski]

• add new implementation
• include FreeBSD

[krytarowski]

In D55596#1334311, @devnexen wrote:

Sorry I meant FreeBSD here.

Please test Linux and Darwin too.

[devnexen]

In D55596#1334347, @krytarowski wrote:

In D55596#1334311, @devnexen wrote:

Sorry I meant FreeBSD here.

Please test Linux and Darwin too.

Ok throwing a Linux's test but Darwin will have to wait couple of hours unless another Apple's folk is faster :-)

So for Linux, seems it is a more an issue get bunch of undefined reference as

undefined reference to `__cxa_thread_atexit'

[krytarowski]

In D55596#1334610, @devnexen wrote:

In D55596#1334347, @krytarowski wrote:

In D55596#1334311, @devnexen wrote:

Sorry I meant FreeBSD here.

Please test Linux and Darwin too.

Ok throwing a Linux's test but Darwin will have to wait couple of hours unless another Apple's folk is faster :-)

So for Linux, seems it is a more an issue get bunch of undefined reference as

undefined reference to `__cxa_thread_atexit'

Hmm, maybe c++ abi handling difference. Keeping there just NetBSD and FreeBSD is fine too.

[devnexen]

Please test Linux and Darwin too.

Ok throwing a Linux's test but Darwin will have to wait couple of hours unless another Apple's folk is faster :-)

Hmm, maybe c++ abi handling difference. Keeping there just NetBSD and FreeBSD is fine too.

Sorry I redid a full build and finally there is an issue
thread-local storage is not supported for the current target changing to THREADLOCAL won t fix it.

[vitalybuka]

lib/asan/asan_posix.cc

49

tsd_key_inited is redundant here?

[devnexen]

Hmm, maybe c++ abi handling difference. Keeping there just NetBSD and FreeBSD is fine too.

Might be good as is indeed. Maybe you want to hear from @dim first though ?

[krytarowski]

In D55596#1335345, @devnexen wrote:

Please test Linux and Darwin too.

Ok throwing a Linux's test but Darwin will have to wait couple of hours unless another Apple's folk is faster :-)

Hmm, maybe c++ abi handling difference. Keeping there just NetBSD and FreeBSD is fine too.

Sorry I redid a full build and finally there is an issue
thread-local storage is not supported for the current target changing to THREADLOCAL won t fix it.

THREADLOCAL evaluates to __thread and it does not support TLS destructors.

It must use C++11 thread_local.

thread-local storage is not supported for the current target

C++11 thread_local not supported for FreeBSD?

lib/asan/asan_posix.cc

49

It serves just as an additional assert, I can drop it.

[devnexen]

thread-local storage is not supported for the current target

C++11 thread_local not supported for FreeBSD?

Nope that was Darwin's test.

[krytarowski]

In D55596#1335938, @devnexen wrote:

thread-local storage is not supported for the current target

C++11 thread_local not supported for FreeBSD?

Nope that was Darwin's test.

OK, so let's restrict this patch to FreeBSD and NetBSD only.

[krytarowski]

• simplify

[vitalybuka]

should this cause any changes in tests? maybe something is going to pass now?

lib/asan/asan_posix.cc

49

I'd prefer if you drop, for simplicity.

[krytarowski]

In D55596#1336058, @vitalybuka wrote:

should this cause any changes in tests? maybe something is going to pass now?

With this patch, ASan is functional at all, no longer with local modifications.

I'm going to update equivalent code for HWASan and MSan soon.

[krytarowski]

In D55596#1336105, @krytarowski wrote:

In D55596#1336058, @vitalybuka wrote:

should this cause any changes in tests? maybe something is going to pass now?

With this patch, ASan is functional at all, no longer with local modifications.

I'm going to update equivalent code for HWASan and MSan soon.

Actually, I've noted that HWASan has been refactored.. and it would help to test it. How to get hardware with HWASan? Is there a simulator?

[eugenis]

HWASAN tests should run on any ARMv8 Android device.
Generally, a patched kernel is required to run HWASAN code, but tests are written to avoid passing tagged pointers to system calls, and they don't have this requirement.

[ro]

In D55596#1334312, @krytarowski wrote:

@ro could you please test this patch for Solaris? I need to know whether it works with TLS destructor on SunOS.

I ran into two compilation failures on unmodified trunk first and got sidetracked (also by the Christmas holidays). However, after
fixing those, applying your patch locally and enabling it on PLATFORM_SOLARIS, it seemed to work fine.

[krytarowski]

In D55596#1343564, @ro wrote:

In D55596#1334312, @krytarowski wrote:

@ro could you please test this patch for Solaris? I need to know whether it works with TLS destructor on SunOS.

I ran into two compilation failures on unmodified trunk first and got sidetracked (also by the Christmas holidays). However, after
fixing those, applying your patch locally and enabling it on PLATFORM_SOLARIS, it seemed to work fine.

As we cannot switch Linux and get a single implementation for everybody, if you need this patch feel free to switch Solaris to it.

[arichardson]

This broke ASAN on FreeBSD (same for the MSAN change). When loading static thread_local struct tsd_key key this is done using __tls_get_addr. The interceptor for __tls_get_addr then calls GetCurrentThread which calls AsanTSDGet which again calls __tls_get_addr.
If I remove the || SANITIZER_FREEBSD it works fine (at least on FreeBSD 11.2).

[dim]

In D55596#1599244, @arichardson wrote:

This broke ASAN on FreeBSD (same for the MSAN change). When loading static thread_local struct tsd_key key this is done using __tls_get_addr. The interceptor for __tls_get_addr then calls GetCurrentThread which calls AsanTSDGet which again calls __tls_get_addr.
If I remove the || SANITIZER_FREEBSD it works fine (at least on FreeBSD 11.2).

Yes indeed, this is https://bugs.llvm.org/show_bug.cgi?id=40761. I'm unsure what a good solution direction is.

[arichardson]

In D55596#1599456, @dim wrote:

In D55596#1599244, @arichardson wrote:

This broke ASAN on FreeBSD (same for the MSAN change). When loading static thread_local struct tsd_key key this is done using __tls_get_addr. The interceptor for __tls_get_addr then calls GetCurrentThread which calls AsanTSDGet which again calls __tls_get_addr.
If I remove the || SANITIZER_FREEBSD it works fine (at least on FreeBSD 11.2).

Yes indeed, this is https://bugs.llvm.org/show_bug.cgi?id=40761. I'm unsure what a good solution direction is.

I just spent some time debugging and it seems like https://reviews.llvm.org/D55596 works.

[dim]

In D55596#1599461, @arichardson wrote:

In D55596#1599456, @dim wrote:

In D55596#1599244, @arichardson wrote:

This broke ASAN on FreeBSD (same for the MSAN change). When loading static thread_local struct tsd_key key this is done using __tls_get_addr. The interceptor for __tls_get_addr then calls GetCurrentThread which calls AsanTSDGet which again calls __tls_get_addr.
If I remove the || SANITIZER_FREEBSD it works fine (at least on FreeBSD 11.2).

Yes indeed, this is https://bugs.llvm.org/show_bug.cgi?id=40761. I'm unsure what a good solution direction is.

I just spent some time debugging and it seems like https://reviews.llvm.org/D55596 works.

Ehm, that is exactly *this* review? Maybe you meant to paste another? :) (Btw, you can just post the abbreviation D55596 and Phabricator will pick it up, like so: D55596.)

[arichardson]

In D55596#1599656, @dim wrote:

In D55596#1599461, @arichardson wrote:

In D55596#1599456, @dim wrote:

In D55596#1599244, @arichardson wrote:

This broke ASAN on FreeBSD (same for the MSAN change). When loading static thread_local struct tsd_key key this is done using __tls_get_addr. The interceptor for __tls_get_addr then calls GetCurrentThread which calls AsanTSDGet which again calls __tls_get_addr.
If I remove the || SANITIZER_FREEBSD it works fine (at least on FreeBSD 11.2).

Yes indeed, this is https://bugs.llvm.org/show_bug.cgi?id=40761. I'm unsure what a good solution direction is.

I just spent some time debugging and it seems like https://reviews.llvm.org/D55596 works.

Ehm, that is exactly *this* review? Maybe you meant to paste another? :) (Btw, you can just post the abbreviation D55596 and Phabricator will pick it up, like so: D55596.)

Yes I actually mean D65221

lib/asan/asan_posix.cc

 void AsanOnDeadlySignal(int signo, void *siginfo, void *context) {
   StartReportDeadlySignal();
   SignalContext sig(siginfo, context);
   ReportDeadlySignal(sig);
 }
 
 // ---------------------- TSD ---------------- {{{1
 
+#if SANITIZER_NETBSD || SANITIZER_FREEBSD
+// Thread Static Data cannot be used in early init on NetBSD
+// Reuse the Asan TSD API for compatibility with existing code
+// with an alternative implementation.
+
+static bool tsd_key_inited = false;
+static void (*tsd_destructor)(void *tsd) = nullptr;
+
+struct tsd_key {
+  tsd_key() : key(nullptr) {}
+  ~tsd_key() {
+    CHECK(tsd_destructor);
+    if (key)
+      (*tsd_destructor)(key);
+  }
+  void *key;
+};
+
+static thread_local struct tsd_key key;
+
+void AsanTSDInit(void (*destructor)(void *tsd)) {
+  CHECK(!tsd_key_inited);
+  tsd_key_inited = true;
+  tsd_destructor = destructor;
+}
+
+void *AsanTSDGet() {
+  CHECK(tsd_key_inited);
+  return key.key;
+}
+
+void AsanTSDSet(void *tsd) {
+  CHECK(tsd_key_inited);
+  CHECK(tsd);
+  CHECK(!key.key);
+  key.key = tsd;
+}
+
+void PlatformTSDDtor(void *tsd) {
+  CHECK(tsd_key_inited);
+  CHECK(key.key);
+  key.key = nullptr;
+  // Make sure that signal handler can not see a stale current thread pointer.
+  atomic_signal_fence(memory_order_seq_cst);
+  AsanThread::TSDDtor(tsd);
+}
+#else
 static pthread_key_t tsd_key;
 static bool tsd_key_inited = false;
 void AsanTSDInit(void (*destructor)(void *tsd)) {
   CHECK(!tsd_key_inited);
   tsd_key_inited = true;
   CHECK_EQ(0, pthread_key_create(&tsd_key, destructor));
 }
 
void PlatformTSDDtor(void *tsd) {
   AsanThreadContext *context = (AsanThreadContext*)tsd;
   if (context->destructor_iterations > 1) {
     context->destructor_iterations--;
     CHECK_EQ(0, pthread_setspecific(tsd_key, tsd));
     return;
   }
   AsanThread::TSDDtor(tsd);
 }
+#endif
 }  // namespace __asan
 
 #endif  // SANITIZER_POSIX