[PATCH] D14199: [compiler-rt] [tsan] Unify aarch64 mapping

[Adhemerval Zanella via llvm-commits]

zatrazz added inline comments.

================
Comment at: lib/tsan/rtl/tsan_platform.h:224
@@ +223,3 @@
+# if !defined(SANITIZER_GO)
+static USED uptr UserRegions[] = {
+  Mapping::kLoAppMemBeg, Mapping::kLoAppMemEnd,
----------------
dvyukov wrote:
> UserRegions are used only once during startup, there is no point in having both const and dynamic versions of it. Also you initialize the dynamic variable only on linux, but it actually needs to be initialized when TSAN_RUNTIME_VMA (e.g. on mac/aarch64 in future).
> 
> Please leave only one version and define it in this header.
> Something along the lines of:
> 
> bool GetUserRegion(int i, uptr *start, uptr *end) {
>   switch (i) {
>   default:
>     return false;
>   case 0:
>     *start = LoAppMemBeg();
>     *end = LoAppMemEnd();
>   case 1:
>     ...
>   case 2:
>     ...
>   }
> }
> 
> And loop in CheckShadowMapping until this function returns false.
I will change that.

================
Comment at: lib/tsan/rtl/tsan_platform.h:272
@@ +271,3 @@
+    case MAPPING_VDSO_BEG: return Mapping::kVdsoBeg;
+#elif defined(SANITIZER_GO) && !SANITIZER_WINDOWS
+    case MAPPING_APP_BEG: return Mapping::kAppMemBeg;
----------------
dvyukov wrote:
> Why !SANITIZER_WINDOWS?
> There is also no windows-Go-specific code, so these definitions must be necessary either on all Go platforms or on no Go platforms.
> 
This is wrong, I will fix it.

================
Comment at: lib/tsan/rtl/tsan_platform_posix.cc:50
@@ -48,3 +49,3 @@
 #elif defined(__aarch64__)
-  const uptr kMadviseRangeBeg  = 0x7e00000000ull;
-  const uptr kMadviseRangeSize = 0x0100000000ull;
+  unsigned runtimeVMA =
+    (MostSignificantSetBitIndex(GET_CURRENT_FRAME()) + 1);
----------------
dvyukov wrote:
> vmaSize should be already set by now, so use it.
> Add
> 
>   } else {
>     CHECK(0);
>   }
> 
> branch below. It will catch both case when vmaSize is somehow not initialized by now and case when support for a new vma size is added.
I will change that.


http://reviews.llvm.org/D14199