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

[Yabin Cui via llvm-commits]

yabinc added inline comments.

================
Comment at: lib/tsan/rtl/tsan_interceptors.cc:252
@@ +251,3 @@
+#if SANITIZER_ANDROID
+  internal_sigfillset(sigset);
+  return 0;
----------------
dvyukov wrote:
> fix internal_sigfillset to work on android instead
> the existing layer of indirection is exactly to hide such differences
I use internal_sigfillset() for android because sigfillset() is not intercepted on android (maybe because of asan). And I think we can't use internal_sigfillset() for mac or windows.

================
Comment at: lib/tsan/rtl/tsan_interceptors.h:35
@@ -34,3 +34,3 @@
     }                                                    \
-    if (thr->ignore_interceptors || thr->in_ignored_lib) \
+    if (!thr->is_inited || thr->ignore_interceptors || thr->in_ignored_lib) \
       return REAL(func)(__VA_ARGS__); \
----------------
danalbert wrote:
> I can't say much for the rest of the change, but the `is_inited` checks can easily be a separate, smaller patch, and I can pretty much guarantee that someone else would say so if I didn't :)
I will split the patch into 3 parts as in the summary, if no one complains that it should be smaller.

================
Comment at: lib/tsan/rtl/tsan_platform_linux.cc:361
@@ +360,3 @@
+    __sanitizer_sigset_t emptyset;
+    internal_sigfillset(&emptyset);
+    __sanitizer_sigset_t oldset;
----------------
dvyukov wrote:
> 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
why it is not signal-safe? There is another check of thr in L365. And I think L364-L365 will not be moved above L363 because L364 is inlined assemble language.

================
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;
----------------
dvyukov wrote:
> 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.
> 
yes, it's a good idea.

================
Comment at: lib/tsan/rtl/tsan_platform_posix.cc:112
@@ -111,2 +111,3 @@
+#if !SANITIZER_ANDROID
   ProtectRange(kLoAppMemEnd, kShadowBeg);
   ProtectRange(kShadowEnd, kMetaShadowBeg);
----------------
dvyukov wrote:
> 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
surely we can. I commented this out just because the address map for android aarch64 doesn't conform the assumption here. I found that by commenting these out, the core dump becomes reasonable small, which helps 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++) {
----------------
dvyukov wrote:
> why is this change made?
Because previously I made kHeapMemBeg == kHeapMemEnd on android/aarch64. I am not sure how I should set kHeapMemBeg and kHeapMemEnd. The brk() space goes after main binary. But android uses jemalloc which uses common mmap() to allocate heap space. This makes real heap space mixed with kHiAppMem. As the correctness of kHeapMem is not very important, I changed it to follow linux/aarch64.
By the way, I am a little curious whether kxxxEnd should be accessible. From tsan_platform.h, it gives me a sense that kxxEnd should not be accessible, but kHiAppMemEnd should be accessible when it is 0x7fffffffffull.


http://reviews.llvm.org/D14855