patch dependent on http://reviews.llvm.org/D7013
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
I will let Dmitry review this in detail, but my general request is to avoid #ifdefs inside the code as much as possible.
Please try to move asm many #ifdefs as you can into headers and then use plain if() in the code.
For example, you can do if(SANITIZER_CAN_USE_ALLOCATOR64) instead of #ifdef SANITIZER_CAN_USE_ALLOCATOR64
Checked if SIZEOF_INT128 is defined for 128 bit atomic operations in tsan_interface_atomic.cc
lib/tsan/CMakeLists.txt | ||
---|---|---|
103 ↗ | (On Diff #16379) | Please merge this into x86_64 case somehow. |
lib/tsan/rtl/tsan_interceptors.cc | ||
44 ↗ | (On Diff #16379) | acquired (here and below) |
661 ↗ | (On Diff #16379) | I don't understand this comment. Why does REAL(pthread_create) crash? |
lib/tsan/rtl/tsan_interface_atomic.cc | ||
43 ↗ | (On Diff #16379) | use Or, better, fix this macro definition in Clang. |
lib/tsan/rtl/tsan_platform_linux.cc | ||
318 ↗ | (On Diff #16379) | Please reduce the amount of #ifs here (e.g. introduce a helper function). |
lib/tsan/rtl/tsan_interceptors.cc | ||
---|---|---|
661 ↗ | (On Diff #16379) | As per my observation, in x86_64 first cxa_guard_aquired() is called and its interceptor calls Initialize(). @dvyukov I don't know why such behaviour in mips, can you help us with this? |
lib/tsan/rtl/tsan_interceptors.cc | ||
---|---|---|
661 ↗ | (On Diff #16379) | Why does the call to REAL(pthread_create) succeeds if it is issued from __cxa_guard_acquired interceptor and fails if it is issued from memset interceptor? I think you'd have to figure it out. I don't see the immediate problem from your description: memset interceptor calls Initialize(), which calls InitializeIntercepotrs() (that is supposed to initialize all REAL(foo) function pointers), and only then calls internal_start_thread. |
lib/tsan/rtl/tsan_interceptors.cc | ||
---|---|---|
661 ↗ | (On Diff #16379) | While debugging the code, I observed that interceptor_memset and cxa_guard_acquire are being called even before __start. So its reference is probably in LLVM/clang. I have no knowledge from where they are being called. Through strace in x86_64, the sequence of calls observed: rt_sigaction() -> rt_sigprocmask() -> __cxa_guard_acquired() -> Initialize() In case of mips64, the sequence is: __interceptor_memset() -> Initialize() So for mips64 there is no signal handler registered during Initialize() because rt_sigaction syscall is not yet issued. I will be very thankful if someone could advice me on how to solve this issue. |
lib/tsan/rtl/tsan_interceptors.cc | ||
---|---|---|
661 ↗ | (On Diff #16379) | It's hard to predict which TSan interceptor will be called first (and thus will be first to call __tsan::Initialize()). For example, on my x86_64 host the stack trace is: __tsan::Initialize ScopedInterceptor::ScopedInterceptor __cxa_guard_acquire __future_category_instance __static_initialization_and_destruction_0 _GLOBAL__sub_I_compatibility_thread_c__0x.cc call_init _dl_start_user I don't see why signal handler is at all important. Can you just debug your TSan-instrumented program in gdb, reach the place where it fails with SIGIOT/SIGABRT and learn the reason why it's happening? E.g. it calls REAL(foo), but this function pointer is not yet initialized. Or it accesses mprotect()-ed memory. Or something else. Sorry, I can't yet diagnose the issue from your descriptions. |
lib/tsan/rtl/tsan_interceptors.cc | ||
---|---|---|
661 ↗ | (On Diff #16379) | Hi, The intercepted memset is called during pthread initialization function __pthread_initialize_minimal_internal (). Sequence of calls for MIPS: __pthread_initialize_minimal_internal () -> __sigemptyset () -> __interceptor_memset () -> pthread_create () -> __GI_abort () The intercepted memset calls pthread_create() before the minimal initialization required for pthread_create is complete and this results in abort. |
lib/tsan/rtl/tsan_interceptors.cc | ||
---|---|---|
661 ↗ | (On Diff #16379) | So, this has nothing to do with __cxa_guard_acquired or memset. You are just not allowed to call pthread_create() from TSan runtime before __pthread_initialize_minimal_internal exits. For example, you can try to intercept __pthread_initialize_minimal_internal and use ScopedIgnoreInterceptors to disable TSan interceptors for sigemptyset(), memset() or whatever. |
lib/tsan/rtl/tsan_interceptors.cc | ||
---|---|---|
661 ↗ | (On Diff #16379) | Hi, I tried to intercept __pthread_initialize_minimal_internal (), but it does not get intercepted. As far as I know, this is probably because references to __pthread_initialize_minimal_internal () are resolved when glibc is compiled. I also tried to search for calls to functions occurring before call to __pthread_initialize_minimal_internal () which would be resolved at runtime, so that we could intercept them and use ScopedIgnoreInterceptors, but I couldn’t see any such function call. |
lib/tsan/rtl/tsan_interceptors.cc | ||
---|---|---|
661 ↗ | (On Diff #16379) | Who calls __pthread_initialize_minimal_internal at all in that case? Is it called from library constructor? As a quick workaround, you can disable background thread in MIPS port - IIRC it's not critical to TSan functioning. |
lib/tsan/rtl/tsan_interceptors.cc | ||
---|---|---|
661 ↗ | (On Diff #16379) |
Here is the backtrace at __pthread_initialize_minimal_internal () : (gdb) bt #0 __pthread_initialize_minimal_internal () at nptl-init.c:281 #1 0x000000ffed9567e4 in _init () at ../ports/sysdeps/mips/mips64/n64/crti.S:80 #2 0x000000ffed9a0d94 in call_init (l=0xffed9c0930, argc=argc@entry=1, argv=argv@entry=0xffff77a658, env=env@entry=0xffff77a668) at dl-init.c:64 #3 0x000000ffed9a0fc0 in call_init (env=0xffff77a668, argv=0xffff77a658, argc=1, l=<optimized out>) at dl-init.c:36 #4 _dl_init (main_map=0xffed9c3848, argc=<optimized out>, argv=0xffff77a658, env=0xffff77a668) at dl-init.c:93 #5 0x000000ffed9906e0 in _dl_start_user () from /lib64/ld.so.1
As per my observation, call to __pthread_initialize_minimal_internal () occurs even before preinit functions execute. |
What is the status of check-tsan command with these changes applied?
lib/tsan/CMakeLists.txt | ||
---|---|---|
80 ↗ | (On Diff #17438) | set TSAN_ASM_SOURCES to empty list for non-x86_64 architectures. |
lib/tsan/rtl/tsan_interface_atomic.cc | ||
36 ↗ | (On Diff #17438) | This change is wrong - it won't work if you build TSan runtime with GCC. Why do you need it? |
129 ↗ | (On Diff #17438) | Shouldn't this be related to __TSAN_HAS_INT128 ? |
lib/tsan/rtl/tsan_platform.h | ||
30 ↗ | (On Diff #17438) | Care to add a similar comment for MIPS? |
lib/tsan/rtl/tsan_platform_linux.cc | ||
218 ↗ | (On Diff #17438) | Where do these magic numbers come from? Please use a named constant for them. |
295 ↗ | (On Diff #17438) | factor this into a separate constant. |
lib/tsan/rtl/tsan_rtl.cc | ||
1001 ↗ | (On Diff #17438) | Looks like it should be inline function in the same file where you define the memory layout. |
What is the status of check-tsan command with these changes applied?
Expected Passes : 159 Expected Failures : 1 Unsupported Tests : 1 Unexpected Failures: 15
lib/tsan/rtl/tsan_interface_atomic.cc | ||
---|---|---|
36 ↗ | (On Diff #17438) | We have not yet implemented 128-bit operations for mips as of clang version 3.6.0. We need to disable __TSAN_HAS_INT128 because tsan fails to compile with the use of 128-bit atomic operations. I have changed this to disable __TSAN_HAS_INT128 only for mips64. |
lib/tsan/rtl/tsan_platform_linux.cc | ||
218 ↗ | (On Diff #17438) | If I understand correctly this address range is part of high application memory which is used for thread stack and large user mmaps. |
OK, this patch is getting closer to commit-ready state. However, I'd still ask Dmitry to review / sign this off.
What is the main reason of unexpected failures in test?
lib/tsan/CMakeLists.txt | ||
---|---|---|
86 ↗ | (On Diff #17590) | set(TSAN_ASM_SOURCES) |
lib/tsan/rtl/tsan_platform.h | ||
135 ↗ | (On Diff #17590) | No, please don't copy-paste this code around. You can move this function to tsan_rtl.h, where allocator is defined. |
lib/tsan/rtl/tsan_rtl.cc | ||
331 ↗ | (On Diff #17590) | You can shorten the comment and tell that on MIPS TSan initialization is run before __pthread_initialize_minimal_internal() is finished, so we can't spawn new threads. |
What is the main reason of unexpected failures in test?
Failing Tests (15):
ThreadSanitizer :: global_race.cc ThreadSanitizer :: global_race2.cc ThreadSanitizer :: global_race3.cc ThreadSanitizer :: longjmp.cc ThreadSanitizer :: longjmp2.cc ThreadSanitizer :: longjmp3.cc ThreadSanitizer :: longjmp4.cc ThreadSanitizer :: map32bit.cc ThreadSanitizer :: mmap_large.cc ThreadSanitizer :: race_on_mutex.c ThreadSanitizer :: signal_longjmp.cc ThreadSanitizer :: signal_errno.cc ThreadSanitizer :: stack_sync_reuse.cc ThreadSanitizer :: tls_race.cc ThreadSanitizer :: tls_race2.cc
global_race.cc, global_race2.cc, global_race3.cc : File Check expects a 47-bit address in place of ADDR, ADDR2, ADDR3 in the test cases. Where as MIPS64 uses 40-bit addresses.
longjmp.cc, longjmp2.cc, longjmp3.cc, longjmp4.cc, signal_longjmp.cc : We have not implemented longjmp/setjmp assemble for MIPS.
map32bit.cc : MAP_32BIT flag for mmap is supported only for x86_64.
mmap_large.cc : Mmap start address is given according to 47-bit address space. Where as MIPS64 uses 40-bit addresses.
race_on_mutex.c : Stack trace is incorrect because of interception of memset call from nptl/pthread_mutex_init.c:91 in glibc.
signal_errno.cc : Stack trace is incorrect because pc is incremented by 1 at tsan_interceptors.cc:1908 in CallUserSignalHandler (). Here, we need to use StackTrace::GetNextInstructionPC () in place of pc +1.
stack_sync_reuse.cc : There is address mismatch between addr and &s in the test case.
tls_race.cc : Test case passes on application of http://reviews.llvm.org/D5616 patch because this patch provides the definition of ThreadDescriptorSize (), GetTls () and ThreadSelf () for MIPS.
tls_race2.cc : ReportSourceLocation is Stack instead of TLS. This is because there is an overlap between TLS and Stack address range.
LGTM. However, please let Dmitry sign this off.
lib/tsan/rtl/tsan_platform_linux.cc | ||
---|---|---|
217 ↗ | (On Diff #17625) | Mark these as const |
LGTM with nits
lib/tsan/rtl/tsan_interceptors.cc | ||
---|---|---|
43 ↗ | (On Diff #17756) | In mips linux kernel: #define SIGRTMAX _NSIG So signal 128 is a valid signal. kSigCount is size of an array indexed with signo. So this constant must be 129. |
lib/tsan/rtl/tsan_platform.h | ||
57 ↗ | (On Diff #17756) | Please move the x86_64 constants on top of __mips64 constants to be consistent with order of archs in the comment above. Or even better group comment with constants for a single arch: #if defined(x86_64) |
lib/tsan/tests/CMakeLists.txt | ||
---|---|---|
38 ↗ | (On Diff #18221) | Hi @samsonov On X86_64, with the previous patch, standalone compilation was failing with -DCOMPILER_RT_INCLUDE_TESTS=ON because of a cmake error. This was happening because the funtion get_target_flags_for_arch () in add_tsan_unittest macro was revieving incorrect arch value in its first argument. Does this change look good ? |
lib/tsan/rtl/tsan_rtl.h | ||
---|---|---|
702 ↗ | (On Diff #18221) | please change !defined(mips) to defined(x86_64) |
- Enabled GotsanRuntimeCheck for x86_64 only.
- Added #ifdef directive to compile HeapEnd() function in C/C++ mode only.
lib/tsan/CMakeLists.txt | ||
---|---|---|
114 ↗ | (On Diff #20143) | Move this target definition inside foreach() loop, and put it under if(arch STREQUAL "x86_64") |
LGTM, feel free to submit after addressing one nit below.
lib/tsan/CMakeLists.txt | ||
---|---|---|
111 ↗ | (On Diff #20262) | No, I actually meant to put it inside foreach() loop above, where we create a build rule for clang_rt.tsan-${arch}. |