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

[Dmitry Vyukov via llvm-commits]

dvyukov added a comment.

+Kuba for sigfillset Mac question.


================
Comment at: lib/tsan/rtl/tsan_interceptors.cc:252
@@ +251,3 @@
+#if SANITIZER_ANDROID
+  internal_sigfillset(sigset);
+  return 0;
----------------
This file calls REAL(sigfillset) just because it was written before internal_sigfillset appeared. The right thing to do is to replace all calls of REAL(sigfillset) to internal_sigfillset in this file, and then make internal_sigfillset work on all platforms. Windows is not an issue, because tsan does not work on windows yet. Mac calls just sigfillset inside of internal_sigfillset, not sure whether it can lead to infinite recursion or not... +Kuba for this. Anyway we need to fix internal_sigfillset.

================
Comment at: lib/tsan/rtl/tsan_platform_linux.cc:361
@@ +360,3 @@
+  ThreadState* thr = (ThreadState*)__get_tls()[TLS_SLOT_TSAN];
+  if (thr == nullptr) {
+    __sanitizer_sigset_t emptyset;
----------------
I missed the second check. Yes, this looks signal-safe. Sorry.

================
Comment at: lib/tsan/rtl/tsan_platform_posix.cc:111-112
@@ -110,3 +110,4 @@
 
-  ProtectRange(kLoAppMemEnd, kShadowBeg);
+  if (kLoAppMemEnd < kShadowBeg)
+    ProtectRange(kLoAppMemEnd, kShadowBeg);
   ProtectRange(kShadowEnd, kMetaShadowBeg);
----------------
I think we need let each platform define what regions need to be protected (see GetUserRegion function for similar functionality). Then you can support Android mapping.

I don't understand how it helps with core dumps. These regions should not be in core dump as they are all zeros. And anyway you still protect huge regions of memory. And we have huge shadow that we need to map anyway.

================
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++) {
----------------
yabinc wrote:
> 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.
There is a check below:
    if (p < beg || p >= end)
which ensures that we don't actually end address. So correct constant for kHiAppMemEnd is 0x8000000000 rather than 0x7fffffffff.

All sanitizers replace default malloc, so jemalloc behavior is irrelevant here.
Tsan allocator is defined in tsan_rtl.h, search for "SizeClassAllocator". For mips64 and arm64 it actually does not use kHeapMemBeg/kHeapMemEnd, it just uses whatever mmap returns (this again makes it important to mprotect all unused holes in address space so that mmap does not return that addresses). So it should be irrelevant how you define it for arm64 (not sure why it defined these constants at all).





http://reviews.llvm.org/D14855