[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:
> kubabrecka wrote:
> > dvyukov wrote:
> > > 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.
> > Calling just `sigfillset` on OS X will call `REAL(sigfillset)`, so it won't infinitely recurse.  (Actually, sigfillset is a macro that sets the value to `~0`, so nothing is called at all.)
> Great, thanks.
> Then we just need to call internal_sigfillset in this file.
Inorder to use internal_sigfillset() in all cases, I have to move internal_sigfillset() from sanitizer_linux.cc to sanitizer_posix.cc. I included "sanitizer_platform_limits_posix.h" in sanitizer_posix.h, I am not very sure that
it is fine.

================
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);
----------------
dvyukov wrote:
> 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.
If we provide a GetProtectRegion() function, it is much longer (about 50~60 lines) than code here. And personally I think that is less clear. Sadly I can't use address range array. So I improved the code here
a little by using #if, which may make it clearer. But if you really think we should use GetProtectRegion(),
feel free to let me know.
I don't know the detail of core dump, maybe dive into this question later.


http://reviews.llvm.org/D14855