Enabling ThreadSaniitizer on PPC64(BE/LE) platforms. Currently the porting is at about 95%, considering that it's failing only 7 tests out of 257.
It looks like that all the failing tests are related to the system call setjmp.
Details
Diff Detail
Event Timeline
Please update the next version of patch with more context (see http://llvm.org/docs/Phabricator.html).
cmake/config-ix.cmake | ||
---|---|---|
146 | This code seems to be used only on Android, why do you need to modify it? | |
162 | Ditto | |
257 | 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. | |
lib/sanitizer_common/sanitizer_linux.h | ||
48 | Is __powerpc64le__ a thing? It doesn't seem to be used anywhere in sanitizer code, and we support PowerPC support in ASan, for instance. | |
lib/tsan/rtl/tsan_interceptors.cc | ||
2464 | We only care about powerpc64 | |
2470 | Looks like it's better to fix TSAN_INTERCEPT_VER instead? | |
test/tsan/CMakeLists.txt | ||
2 | You should probably invert this check to work on x86 only | |
test/tsan/mmap_large.cc | ||
19 | See above - we only care about powerpc64, right? |
lib/sanitizer_common/sanitizer_linux.cc | ||
---|---|---|
983 | Remove LE version | |
1123 | Remove LE version | |
lib/sanitizer_common/sanitizer_linux.h | ||
48 | Right, this is not a thing. Where endianness matters, there are other macros to be used. __powerpc64le__ is #defined for both BE and LE systems. | |
lib/sanitizer_common/sanitizer_linux_libcdep.cc | ||
182 ↗ | (On Diff #34675) | Remove LE version, and so on to the end |
test/tsan/mmap_large.cc | ||
---|---|---|
19 | Looks like you missed addressing this comment. __powerpc is 32-bit PowerPC, and there's no intent to support TSan for that, that I'm aware of. |
lib/sanitizer_common/sanitizer_linux.cc | ||
---|---|---|
993 | Here is a working clone implementation for powerpc64 if you want to incorporate in your patch. uptr internal_clone(int (*fn)(void *), void *child_stack, int flags, void *arg, int *parent_tidptr, void *newtls, int *child_tidptr) { long long res; /* Stack frame offsets. */ #if _CALL_ELF != 2 #define FRAME_MIN_SIZE 112 #define FRAME_TOC_SAVE 40 #else #define FRAME_MIN_SIZE 32 #define FRAME_TOC_SAVE 24 #endif if (!fn || !child_stack) return -EINVAL; CHECK_EQ(0, (uptr)child_stack % 16); child_stack = (char *)child_stack - 2 * sizeof(unsigned long long); ((unsigned long long *)child_stack)[0] = (uptr)fn; ((unsigned long long *)child_stack)[1] = (uptr)arg; register int (*__fn)(void *) __asm__("r3") = fn; register void *__cstack __asm__("r4") = child_stack; register int __flags __asm__("r5") = flags; register void * __arg __asm__("r6") = arg; register int * __ptidptr __asm__("r7") = parent_tidptr; register void * __newtls __asm__("r8") = newtls; register int * __ctidptr __asm__("r9") = child_tidptr; __asm__ __volatile__( /* fn, arg, child_stack are saved acrVoss the syscall */ "mr 28, %5\n\t" "mr 29, %6\n\t" "mr 27, %8\n\t" /* syscall r3 == flags r4 == child_stack r5 == parent_tidptr r6 == newtls r7 == child_tidptr */ "mr 3, %7\n\t" "mr 5, %9\n\t" "mr 6, %10\n\t" "mr 7, %11\n\t" "li 0, %3\n\t" "sc\n\t" /* Test if syscall was successful */ "cmpdi cr1, 3, 0\n\t" "crandc cr1*4+eq, cr1*4+eq, cr0*4+so\n\t" "bne- cr1, 1f\n\t" /* Do the function call */ "std 2, %13(1)\n\t" #if _CALL_ELF != 2 "ld 0, 0(28)\n\t" "ld 2, 8(28)\n\t" "mtctr 0\n\t" #else "mr 12, 28\n\t" "mtctr 12\n\t" #endif "mr 3, 27\n\t" "bctrl\n\t" "ld 2, %13(1)\n\t" /* Call _exit(r3) */ "li 0, %4\n\t" "sc\n\t" /* Return to parent */ "1:\n\t" "mr %0, 3\n\t" : "=r" (res) : "0" (-1), "i" (EINVAL), "i" (__NR_clone), "i" (__NR_exit), "r" (__fn), "r" (__cstack), "r" (__flags), "r" (__arg), "r" (__ptidptr), "r" (__newtls), "r" (__ctidptr), "i" (FRAME_MIN_SIZE), "i" (FRAME_TOC_SAVE) : "cr0", "cr1", "memory", "ctr", "r0", "r29", "r27", "r28"); return res; } Checked on powerpc64le, no regressions found. |
Updated patch for supporting multiple VMA (44 and 46 bits). To support these multiple VMAS transformations I followed the same mechanism used for AARCH64. By default the system will build for VMA=46, in order to build for VMA=44 will be still necessary to define the macro SANITIZER_PPC64_VMA at build time, for example for CMake:
-D CMAKE_C_FLAGS='-D SANITIZER_PPC64_VMA=44' -D CMAKE_CXX_FLAGS='-D SANITIZER_PPC64_VMA=44'
Applying also the patch D13729, it is failing only a few tests, here a summary:
Failing tests with VMA=44:
cond_cancel.c
java_race_pc.cc
signal_longjmp.cc
signal_errno.cc
Failing tests with VMA=46:
java_race_pc.cc
race_on_mutex.c (this is not failing for VMA=44)
I am already working on these last few problems.
Simone
I added XFAIL in the tests that are currently failing.
I added a comment in each test so the failure can be addressed later more easily.
This patch together with D13729 should be ready to be committed.
Hi,
is ok to commit the patch in this status?
I talked to Bill, and he agreed to commit the patch as it is, since in the future they are more concerned about LE systems (VMA=46).
Currently XFAILing the tests with suspicious problems in libc (for VMA=44 bits) will produce XPASSes on ppc64le (VMA=46 bits).
It looks like FileCheck does not allow to discriminate this two different cases, do you have any suggestion?
Otherwise if we can just XFAIL those tests and have 3 XPASSes for ppc64le, I think the patch is ready to be committed unless you have some other comment.
Simone
LGTM with a nit
lib/tsan/rtl/tsan_platform_linux.cc | ||
---|---|---|
248 | Please split this check per-architecture. Current numbers (39 and 42) are for aarch64. Otherwise we will end up allowing just any vma size. |
Addressed Dmitry's comment, patch ready to land.
Bill, will you take care of the commit?
Thanks!
Simone
Yes, I will do some quick testing and commit this patch, my setjmp/longjmp patch, and your Clang patch to enable things. If all goes well this will all land later today.
This code seems to be used only on Android, why do you need to modify it?