[PATCH] D14855: [tsan] Support thread sanitizer on Android.

[Dmitry Vyukov via llvm-commits]

dvyukov added a comment.

This needs to be split into smaller changes. But otherwise I think we can get something like this in.


================
Comment at: lib/tsan/rtl/tsan_interceptors.cc:252
@@ +251,3 @@
+#if SANITIZER_ANDROID
+  internal_sigfillset(sigset);
+  return 0;
----------------
fix internal_sigfillset to work on android instead
the existing layer of indirection is exactly to hide such differences

================
Comment at: lib/tsan/rtl/tsan_interceptors.cc:279
@@ +278,3 @@
+  Initialize(thr);
+  if (!thr_->is_inited) {
+    return;
----------------
drop {} for if with single-statement/single-line body
whole tsan codebase does it that way

================
Comment at: lib/tsan/rtl/tsan_interceptors.cc:282
@@ -245,3 +281,3 @@
+  }
   if (!thr_->ignore_interceptors) {
     FuncEntry(thr, pc);
----------------
ditto

================
Comment at: lib/tsan/rtl/tsan_interceptors.cc:294
@@ -257,1 +293,3 @@
 ScopedInterceptor::~ScopedInterceptor() {
+  if (!thr_->is_inited) {
+    return;
----------------
ditto

================
Comment at: lib/tsan/rtl/tsan_platform.h:203
@@ +202,3 @@
+#if SANITIZER_ANDROID && defined(__aarch64__)
+  if (x <= kLoAppMemEnd) {
+    return ((x & ~(kLoAppMemMsk | (kShadowCell - 1))) + kLoAppMemAdd) * kShadowCnt;
----------------
drop {}

================
Comment at: lib/tsan/rtl/tsan_platform.h:235
@@ +234,3 @@
+#if SANITIZER_ANDROID && defined(__aarch64__)
+  if (s <= MemToShadow(kLoAppMemEnd)) {
+    return ((s / kShadowCnt) - kLoAppMemAdd) | kLoAppMemMsk;
----------------
drop {}

================
Comment at: lib/tsan/rtl/tsan_platform_linux.cc:354
@@ +353,3 @@
+// in one TLS slot.
+#define TLS_SLOT_TSAN 8
+static ThreadState *dead_thread_state = nullptr;
----------------
please use a const here, it does not need to be a macro

    const int kTsanTlsSlot = 8;  // allocated by bionic for tsan

================
Comment at: lib/tsan/rtl/tsan_platform_linux.cc:355
@@ +354,3 @@
+#define TLS_SLOT_TSAN 8
+static ThreadState *dead_thread_state = nullptr;
+
----------------
add a comment explaining what it is for

================
Comment at: lib/tsan/rtl/tsan_platform_linux.cc:361
@@ +360,3 @@
+    __sanitizer_sigset_t emptyset;
+    internal_sigfillset(&emptyset);
+    __sanitizer_sigset_t oldset;
----------------
this is still not completely async-signal-safe
consider that a thread is preempted by a signal handler on this line: now signal handler will allocate thread context and work for some time with it, then thread will allocate another context and lead the first one
see how mac does this, a similar change for mac recently went it

================
Comment at: lib/tsan/rtl/tsan_platform_linux.cc:366
@@ +365,3 @@
+    if (thr == nullptr) {
+      thr = reinterpret_cast<ThreadState*>(internal_mmap(nullptr, sizeof(ThreadState),
+                                                         PROT_READ | PROT_WRITE,
----------------
use MmapOrDie, it does not have zillion of uninteresting arguments and does not return nullptr

================
Comment at: lib/tsan/rtl/tsan_platform_linux.cc:375
@@ +374,3 @@
+        CHECK(dead_thread_state);
+        dead_thread_state->fast_state.SetIgnoreBit();
+        dead_thread_state->ignore_interceptors = 1;
----------------
I wonder if there will be some races on dead_thread_state since it is shared between threads. I think it is a good idea to mprotect(PROT_READ) it after initialization. It can save us lots of debugging in future.


================
Comment at: lib/tsan/rtl/tsan_platform_posix.cc:112
@@ -111,2 +111,3 @@
+#if !SANITIZER_ANDROID
   ProtectRange(kLoAppMemEnd, kShadowBeg);
   ProtectRange(kShadowEnd, kMetaShadowBeg);
----------------
can't we keep these ProtectRange's on android?

applications do weird things like mmaps on strange fixed addresses, or mmaps at completely random addresses (a-la home-grown ASLR), such things cause unpleasant debugging

================
Comment at: lib/tsan/rtl/tsan_rtl.cc:292
@@ -291,3 +291,3 @@
     VPrintf(3, "checking shadow region %p-%p\n", beg, end);
-    for (uptr p0 = beg; p0 <= end; p0 += (end - beg) / 4) {
+    for (uptr p0 = beg; p0 < end; p0 += (end - beg) / 4) {
       for (int x = -1; x <= 1; x++) {
----------------
why is this change made?


http://reviews.llvm.org/D14855