This is an archive of the discontinued LLVM Phabricator instance.

[TSan][MIPS] Adding support for MIPS64
ClosedPublic

Authored by slthakur on Nov 16 2014, 10:00 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

slthakur retitled this revision from to [TSan][MIPS] Adding support for MIPS64.Nov 16 2014, 10:00 PM
slthakur updated this object.
slthakur edited the test plan for this revision. (Show Details)
slthakur added reviewers: kcc, dvyukov, dsanders, petarj.
slthakur added subscribers: sdkie, mohit.bhakkad, farazs, Unknown Object (MLST).
slthakur updated this revision to Diff 16280.Nov 16 2014, 10:00 PM
slthakur set the repository for this revision to rL LLVM.
slthakur added a subscriber: Anand.Takale.
kcc edited edge metadata.Nov 17 2014, 12:10 PM

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

slthakur updated this revision to Diff 16379.Nov 19 2014, 5:18 AM
slthakur updated this object.
slthakur edited edge metadata.
slthakur set the repository for this revision to rL LLVM.

Checked if SIZEOF_INT128 is defined for 128 bit atomic operations in tsan_interface_atomic.cc

samsonov added inline comments.Nov 19 2014, 1:32 PM
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
#if !defined(TSAN_GO) && defined(SIZEOF_INT128)

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).

slthakur added inline comments.Nov 20 2014, 1:17 AM
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().
Initialize() creates background thread.
But in mips64
interceptor_memset gets called before cxa_guard_aquire() interceptor. So Initialize() is called from within intercerptor_memset and REAL(pthread_create) emits a SIGIOT.

@dvyukov I don't know why such behaviour in mips, can you help us with this?

samsonov added inline comments.Nov 20 2014, 12:40 PM
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.

slthakur added inline comments.Nov 25 2014, 5:16 AM
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.

samsonov added inline comments.Nov 25 2014, 4:35 PM
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.

slthakur added inline comments.Dec 11 2014, 10:22 PM
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.
On x86, __pthread_initialize_minimal_internal () does not call memset and thus can complete the minimal initialization.
For MIPS, we need to avoid calling pthread_create () if memset is called during pthread initialization.

samsonov added inline comments.Dec 12 2014, 1:59 PM
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.

slthakur added inline comments.Dec 17 2014, 4:38 AM
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.

samsonov added inline comments.Dec 17 2014, 5:59 PM
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.

slthakur added inline comments.Dec 18 2014, 12:46 AM
lib/tsan/rtl/tsan_interceptors.cc
661 ↗(On Diff #16379)

Who calls __pthread_initialize_minimal_internal at all in that case?

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

Is it called from library constructor?

As per my observation, call to __pthread_initialize_minimal_internal () occurs even before preinit functions execute.
And as fas as I know library constructors execute after preinit functions execute.

slthakur updated this revision to Diff 17435.Dec 18 2014, 5:15 AM
slthakur updated this object.
slthakur edited the test plan for this revision. (Show Details)
slthakur set the repository for this revision to rL LLVM.

Updated patch as per suggestions of Samsonov.

slthakur updated this revision to Diff 17438.Dec 18 2014, 5:25 AM

Removed extra empty line in tsan_rtl.cc

samsonov edited edge metadata.Dec 22 2014, 6:12 PM

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.

slthakur updated this revision to Diff 17590.Dec 23 2014, 4:29 AM
slthakur edited edge metadata.

Updated patch as per suggestions by Samsonov

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.

slthakur added a comment.EditedDec 24 2014, 5:57 AM

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.

slthakur updated this revision to Diff 17625.Dec 24 2014, 6:03 AM

Updated patch as per suggestions by Samsonov

samsonov accepted this revision.Dec 29 2014, 11:16 AM
samsonov edited edge metadata.

LGTM. However, please let Dmitry sign this off.

lib/tsan/rtl/tsan_platform_linux.cc
217 ↗(On Diff #17625)

Mark these as const

This revision is now accepted and ready to land.Dec 29 2014, 11:16 AM
slthakur updated this revision to Diff 17756.Jan 1 2015, 11:37 PM
slthakur edited edge metadata.

Marked kMadviseRangeSize and kMadviseRangeBeg as const

dvyukov accepted this revision.Jan 14 2015, 5:31 AM
dvyukov edited edge metadata.

LGTM with nits

lib/tsan/rtl/tsan_interceptors.cc
43 ↗(On Diff #17756)

In mips linux kernel:

#define SIGRTMAX _NSIG
#define _NSIG 128

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)
/*
C/C++ on linux/x86_64 and freebsd/x86_64
0000 0000 1000 - 0100 0000 0000: main binary and/or MAP_32BIT mappings
...
*/
const uptr kMetaShadowBeg = 0x300000000000ull;
...
#elif defined(__mips64)
/*
C/C++ on linux/mips64
0100 0000 00 - 0200 0000 00: main binary
...
*/
const uptr kMetaShadowBeg = 0x3000000000ull;
...
#endif

slthakur updated this revision to Diff 18221.Jan 15 2015, 5:03 AM
slthakur edited edge metadata.
  • Addressed review comments by Dmitry.
  • Added changes to enable unit tests.
This comment was removed by slthakur.
slthakur added inline comments.Jan 15 2015, 5:18 AM
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.
I have added this change to enable unit tests for all architectures in this patch.

Does this change look good ?

LGTM. Thank you for working on this!

dvyukov added inline comments.Jan 18 2015, 11:46 PM
lib/tsan/rtl/tsan_rtl.h
702 ↗(On Diff #18221)

please change !defined(mips) to defined(x86_64)
people working on aarch64 has a similar change that disables HACKY_CALL for aarch64

slthakur updated this revision to Diff 18418.Jan 20 2015, 2:18 AM

Changed !defined(mips) to defined(x86_64) for HACKY_CALL() in tsan_rtl.h

slthakur updated this object.Jan 20 2015, 2:19 AM
slthakur updated this revision to Diff 20143.Feb 17 2015, 9:52 PM
  • Enabled GotsanRuntimeCheck for x86_64 only.
  • Added #ifdef directive to compile HeapEnd() function in C/C++ mode only.
samsonov requested changes to this revision.Feb 18 2015, 11:11 AM
samsonov edited edge metadata.
samsonov added inline comments.
lib/tsan/CMakeLists.txt
114 ↗(On Diff #20143)

Move this target definition inside foreach() loop, and put it under if(arch STREQUAL "x86_64")

This revision now requires changes to proceed.Feb 18 2015, 11:11 AM
slthakur updated this revision to Diff 20260.Feb 18 2015, 11:08 PM
slthakur edited edge metadata.

Addressed review comments by @samsonov

slthakur updated this revision to Diff 20262.Feb 18 2015, 11:29 PM
slthakur edited edge metadata.
slthakur removed subscribers: farazs, sdkie.

Sorry I missed one empty line. Added missing empty line.

samsonov accepted this revision.Feb 19 2015, 6:01 PM
samsonov edited edge metadata.

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}.

This revision is now accepted and ready to land.Feb 19 2015, 6:01 PM
slthakur updated this revision to Diff 20376.Feb 19 2015, 10:30 PM
slthakur edited edge metadata.

Moved target GotsanRuntimeCheck into foreach() loop as suggested by @samsonov

This revision was automatically updated to reflect the committed changes.