[compiler-rt] r267864 - [tsan] Fix Darwin GCD support after separation of Processor and ThreadState

[Dmitry Vyukov via llvm-commits]

On Mon, May 2, 2016 at 11:52 AM, Kuba Brecka <jbrecka at apple.com> wrote:
>
> On 2 May 2016, at 11:48, Dmitry Vyukov <dvyukov at google.com> wrote:
>
> On Thu, Apr 28, 2016 at 11:26 AM, Kuba Brecka via llvm-commits
> <llvm-commits at lists.llvm.org> wrote:
>
> Author: kuba.brecka
> Date: Thu Apr 28 04:26:30 2016
> New Revision: 267864
>
> URL: http://llvm.org/viewvc/llvm-project?rev=267864&view=rev
> Log:
> [tsan] Fix Darwin GCD support after separation of Processor and ThreadState
>
> Recent TSan changes (r267678) which factor out parts of ThreadState into a
> Processor structure broke worker threads on OS X.  This fixes it by properly
> calling ProcCreate for GCD worker threads and by replacing some CHECKs with
> RAW_CHECK in early process initialization.  CHECK() in TSan calls the
> allocator, which requires a valid Processor.
>
>
> Modified:
>    compiler-rt/trunk/lib/sanitizer_common/sanitizer_mac.cc
>    compiler-rt/trunk/lib/tsan/rtl/tsan_platform_mac.cc
>
> Modified: compiler-rt/trunk/lib/sanitizer_common/sanitizer_mac.cc
> URL:
> http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_mac.cc?rev=267864&r1=267863&r2=267864&view=diff
> ==============================================================================
> --- compiler-rt/trunk/lib/sanitizer_common/sanitizer_mac.cc (original)
> +++ compiler-rt/trunk/lib/sanitizer_common/sanitizer_mac.cc Thu Apr 28
> 04:26:30 2016
> @@ -602,7 +602,7 @@ void MaybeReexec() {
>   // wrappers work. If it is not, set DYLD_INSERT_LIBRARIES and re-exec
>   // ourselves.
>   Dl_info info;
> -  CHECK(dladdr((void*)((uptr)&__sanitizer_report_error_summary), &info));
> +  RAW_CHECK(dladdr((void*)((uptr)&__sanitizer_report_error_summary),
> &info));
>   char *dyld_insert_libraries =
>       const_cast<char*>(GetEnv(kDyldInsertLibraries));
>   uptr old_env_len = dyld_insert_libraries ?
> @@ -647,7 +647,7 @@ void MaybeReexec() {
>            "environment variable and re-execute itself, but execv() failed,
> "
>            "possibly because of sandbox restrictions. Make sure to launch
> the "
>            "executable with:\n%s=%s\n", kDyldInsertLibraries, new_env);
> -    CHECK("execv failed" && 0);
> +    RAW_CHECK("execv failed" && 0);
>   }
>
>   // Verify that interceptors really work.  We'll use dlsym to locate
> @@ -655,14 +655,14 @@ void MaybeReexec() {
>   // "wrap_pthread_create" within our own dylib.
>   Dl_info info_pthread_create;
>   void *dlopen_addr = dlsym(RTLD_DEFAULT, "pthread_create");
> -  CHECK(dladdr(dlopen_addr, &info_pthread_create));
> +  RAW_CHECK(dladdr(dlopen_addr, &info_pthread_create));
>   if (internal_strcmp(info.dli_fname, info_pthread_create.dli_fname) != 0) {
>     Report(
>         "ERROR: Interceptors are not working. This may be because %s is "
>         "loaded too late (e.g. via dlopen). Please launch the executable "
>         "with:\n%s=%s\n",
>         SanitizerToolName, kDyldInsertLibraries, info.dli_fname);
> -    CHECK("interceptors not installed" && 0);
> +    RAW_CHECK("interceptors not installed" && 0);
>   }
>
>   if (!lib_is_in_env)
> @@ -677,7 +677,7 @@ void MaybeReexec() {
>   // sign and the '\0' char.
>   char *new_env = (char*)allocator_for_env.Allocate(
>       old_env_len + 2 + env_name_len);
> -  CHECK(new_env);
> +  RAW_CHECK(new_env);
>   internal_memset(new_env, '\0', old_env_len + 2 + env_name_len);
>   internal_strncpy(new_env, kDyldInsertLibraries, env_name_len);
>   new_env[env_name_len] = '=';
>
> Modified: compiler-rt/trunk/lib/tsan/rtl/tsan_platform_mac.cc
> URL:
> http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/tsan/rtl/tsan_platform_mac.cc?rev=267864&r1=267863&r2=267864&view=diff
> ==============================================================================
> --- compiler-rt/trunk/lib/tsan/rtl/tsan_platform_mac.cc (original)
> +++ compiler-rt/trunk/lib/tsan/rtl/tsan_platform_mac.cc Thu Apr 28 04:26:30
> 2016
> @@ -135,10 +135,12 @@ static void my_pthread_introspection_hoo
>   if (event == PTHREAD_INTROSPECTION_THREAD_CREATE) {
>     if (thread == pthread_self()) {
>       // The current thread is a newly created GCD worker thread.
> +      ThreadState *thr = cur_thread();
> +      Processor *proc = ProcCreate();
>
>
> Any signal that arrives between cur_thread() and ProcWire will be
> delayed, right?
>
>
> I think signals are actually not disabled during early new thread state, and
> this bug was already present before this patch :(

Signals are delayed by default, unless we receive a synchronous signal
or in a blocking function.
Signal handler still needs to look at ThreadState (that's why we have
cur_thread async-signal-safe), but it should not touch
cur_thread->proc1.
So at first glance we are fine here.

> I think you also need to explicitly ProcDestroy the proc.
>
>
> Let me double check, but I think DestroyThreadState is called for both
> regular and worker threads, which will destroy the Processor.
>
> Kuba
>