[PATCH] D12841: [LLVMdev] Compiler-RT - Enabling ThreadSanitizer on PPC64(BE/LE) platforms

[Alexey Samsonov via llvm-commits]

samsonov added a subscriber: samsonov.
samsonov added a comment.

Please update the next version of patch with more context (see http://llvm.org/docs/Phabricator.html).


================
Comment at: cmake/config-ix.cmake:146
@@ -145,1 +145,3 @@
   check_symbol_exists(__mips64__ "" __MIPS64)
+  check_symbol_exists(__powerpc64__ "" __POWERPC64)
+  check_symbol_exists(__powerpc64le__ "" __POWERPC64LE)
----------------
This code seems to be used only on Android, why do you need to modify it?

================
Comment at: cmake/config-ix.cmake:162
@@ -159,1 +161,3 @@
     add_default_target_arch(mips)
+  elseif(__POWERPC64)
+    add_default_target_arch(powerpc64)
----------------
Ditto

================
Comment at: cmake/config-ix.cmake:257
@@ -250,2 +256,3 @@
 set(X86_64 x86_64)
+set(PPC64 powerpc64 powerpc64le)
 if(APPLE)
----------------
Why do you need this replacement? These variables were added to special-case Apple systems, where we want to target several variants of x86_64 architectures, unlike on another platforms. Listing both powerpc64 and powerpc64le below would be more clear IMO.

================
Comment at: lib/sanitizer_common/sanitizer_linux.h:48
@@ -48,1 +47,3 @@
+#if defined(__x86_64__) || defined(__mips__) || defined(__aarch64__) || \
+  defined(__powerpc64__)  || defined(__powerpc64le__)
 uptr internal_clone(int (*fn)(void *), void *child_stack, int flags, void *arg,
----------------
Is `__powerpc64le__` a thing? It doesn't seem to be used anywhere in sanitizer code, and we support PowerPC support in ASan, for instance.

================
Comment at: lib/tsan/rtl/tsan_interceptors.cc:2464
@@ -2463,1 +2463,3 @@
 
+#if defined(__powerpc__) || defined(__powerpc64__) \
+  || defined(__powerpc64le__)
----------------
We only care about powerpc64

================
Comment at: lib/tsan/rtl/tsan_interceptors.cc:2470
@@ +2469,3 @@
+  TSAN_INTERCEPT(pthread_cond_wait);
+  TSAN_INTERCEPT(pthread_cond_timedwait);
+  TSAN_INTERCEPT(pthread_cond_destroy);
----------------
Looks like it's better to fix TSAN_INTERCEPT_VER instead?

================
Comment at: test/tsan/CMakeLists.txt:2
@@ -1,3 +1,3 @@
 set(TSAN_TEST_DEPS ${SANITIZER_COMMON_LIT_TEST_DEPS})
-if(NOT ${COMPILER_RT_DEFAULT_TARGET_ARCH} MATCHES "mips")
+if(NOT ${COMPILER_RT_DEFAULT_TARGET_ARCH} MATCHES "mips|powerpc64|powerpc64le")
   list(APPEND TSAN_TEST_DEPS GotsanRuntimeCheck)
----------------
You should probably invert this check to work on x86 only

================
Comment at: test/tsan/mmap_large.cc:19
@@ -18,1 +18,3 @@
   const size_t kLog2Size = 32;
+#elif defined(__powerpc)
+  const size_t kLog2Size = 39;
----------------
See above - we only care about powerpc64, right?


http://reviews.llvm.org/D12841