Don't require ThreadState to be contained within tls on all platforms

[fjricci]

Summary

The existing code ran CHECKs to assert that the thread state pointer
was stored in tls. However, the mac implementation of tsan
doesn't store the pointer in tls, so these checks fail once
tls darwin support is added. Move to soft if-condition checks instead.

[fjricci]

Fix if condition equality

[alekseyshl]

If all other platforms expect these conditions to hold, maybe we should relax our requirements for Darwin only?

[fjricci]

Yeah, I wasn't sure whether the preferred method here was to skip the block if Darwin or the current solution. There currently aren't any os-specific references in this file. Hopefully @dvyukov or @kubamracek have insight on the preferred way to do this.

[dvyukov]

Yes, please do this only for Darwin. These things are extremely fragile, it's nice to be able to rely at least on some things. If we change it to if, it will silently break on other platforms and we will not notice.
You can either move some code to platform* files, or merely provide a flag in platform* files and use it here.

Also, if there is tls, but ThreadState is not there, we still need to do:

MemoryRangeImitateWrite(thr, /*pc=*/2, tls_addr, tls_size);

There checks are merely to avoid writing to ThreadState to improve performance.

Also please fix the change description: we don't store thread state pointer in tls, we store whole ThreadState object in tls.

[fjricci]

Only remove checks on Darwin.

Not sure how great the function name is, open to suggestions there. Wasn't
able to determine exactly what the purpose of that code block is,
since it appears to date back to the original tsan check-in commit.

[dvyukov]

Thanks!

[fjricci]

This patch passes tests on its own, which is to be expected because it's currently a NFC (enabling darwin tls is blocked on this, so it hasn't been committed yet). However, it looks like this line causes many test failures, when combined with the patch to enable darwin tls:

MemoryRangeImitateWrite(thr, /*pc=*/ 2, tls_addr, tls_size)

I don't know a lot about how TSan works or why that write is necessary (I'm adding the tls range on darwin to support LSan), but it appears that the resulting failures exclusively come from Darwin-specific unit tests, most of which fail.

They fail with failing CHECK conditions, for example:

FATAL: ThreadSanitizer CHECK failed: /Users/fjricci/Source/llvm/projects/compiler-rt/lib/tsan/rtl/tsan_platform.h:786 "((p)) < ((Mapping::kTraceMemEnd))" (0x72976dc1000000, 0x620000000000)

* thread #2: tid = 0xfac294, 0x000000010013d172 libclang_rt.tsan_osx_dynamic.dylib`__tsan::TsanCheckFailed(file="/Users/fjricci/Source/llvm/projects/compiler-rt/lib/tsan/rtl/tsan_platform.h", line=786, cond="((p)) < ((Mapping::kTraceMemEnd))", v1=32254644990246912, v2=107752139522048) + 34 at tsan_rtl_report.cc:40, stop reason = breakpoint 1.1
  * frame #0: 0x000000010013d172 libclang_rt.tsan_osx_dynamic.dylib`__tsan::TsanCheckFailed(file="/Users/fjricci/Source/llvm/projects/compiler-rt/lib/tsan/rtl/tsan_platform.h", line=786, cond="((p)) < ((Mapping::kTraceMemEnd))", v1=32254644990246912, v2=107752139522048) + 34 at tsan_rtl_report.cc:40
    frame #1: 0x00000001000b09c8 libclang_rt.tsan_osx_dynamic.dylib`__sanitizer::CheckFailed(file="/Users/fjricci/Source/llvm/projects/compiler-rt/lib/tsan/rtl/tsan_platform.h", line=786, cond="((p)) < ((Mapping::kTraceMemEnd))", v1=32254644990246912, v2=107752139522048) + 120 at sanitizer_termination.cc:79
    frame #2: 0x0000000100138015 libclang_rt.tsan_osx_dynamic.dylib`unsigned long __tsan::GetThreadTraceHeaderImpl<__tsan::Mapping>(tid=1783840768) + 133 at tsan_platform.h:786
    frame #3: 0x0000000100112209 libclang_rt.tsan_osx_dynamic.dylib`__tsan::ThreadTrace(int) [inlined] __tsan::GetThreadTraceHeader(tid=1783840768) + 25 at tsan_platform.h:807
    frame #4: 0x0000000100112201 libclang_rt.tsan_osx_dynamic.dylib`__tsan::ThreadTrace(tid=1783840768) + 17 at tsan_rtl.cc:537
    frame #5: 0x000000010011210e libclang_rt.tsan_osx_dynamic.dylib`__tsan::TraceSwitch(thr=0x0000600000000002) + 46 at tsan_rtl.cc:526
    frame #6: 0x0000000100112441 libclang_rt.tsan_osx_dynamic.dylib`::__tsan_trace_switch() + 17 at tsan_rtl.cc:556
    frame #7: 0x000000010014309f libclang_rt.tsan_osx_dynamic.dylib`__tsan::ThreadContext::OnStarted(void*) [inlined] __tsan::TraceAddEvent(thr=0x0000000101f20000, fs=(x_ = 1139094046384128), typ=EventTypeMop, addr=0) + 463 at tsan_rtl.h:833
    frame #8: 0x0000000100142ed0 libclang_rt.tsan_osx_dynamic.dylib`__tsan::ThreadContext::OnStarted(this=0x00000001026063f8, arg=0x00007e8000103dd0) + 544 at tsan_rtl_thread.cc:115
    frame #9: 0x00000001000aef7a libclang_rt.tsan_osx_dynamic.dylib`__sanitizer::ThreadContextBase::SetStarted(this=0x00000001026063f8, _os_id=16433812, _workerthread=true, arg=0x00007e8000103dd0) + 90 at sanitizer_thread_registry.cc:67
    frame #10: 0x00000001000b0504 libclang_rt.tsan_osx_dynamic.dylib`__sanitizer::ThreadRegistry::StartThread(this=0x0000000100e75d40, tid=1, os_id=16433812, workerthread=true, arg=0x00007e8000103dd0) + 436 at sanitizer_thread_registry.cc:279
    frame #11: 0x0000000100143baf libclang_rt.tsan_osx_dynamic.dylib`__tsan::ThreadStart(thr=0x0000000101f20000, tid=1, os_id=16433812, workerthread=true) + 287 at tsan_rtl_thread.cc:258
    frame #12: 0x0000000100151ba1 libclang_rt.tsan_osx_dynamic.dylib`__tsan::my_pthread_introspection_hook(event=1, thread=0x00007e8000104000, addr=0x00007e8000104000, size=8192) + 257 at tsan_platform_mac.cc:210
    frame #13: 0x00007fffbf8453e8 libsystem_pthread.dylib`_pthread_introspection_hook_callout_thread_create + 62
    frame #14: 0x00007fffbf841325 libsystem_pthread.dylib`_pthread_wqthread + 546
    frame #15: 0x00007fffbf8410f1 libsystem_pthread.dylib`start_wqthread + 13

It's not clear to me why this patch (and that line in particular) would cause these failures, since I don't see much in common in the stacks, other than the fact that they both happen during thread start.

[dvyukov]

I think that's because we use _shadow_ memory to store pointer to ThreadState. See cur_thread in tsan_platform_mac.cc. We probably need to skip that slot or something.

[fjricci]

Don't clobber thread state pointer in the tls shadow memory

[dvyukov]

lib/tsan/rtl/tsan_platform_mac.cc

248

This looks quite fragile.
I would suggest to factor out part that computes location of ThreadState pointer from cur_thread(). E.g. introduce ThreadState **cur_thread_location(). And then use it in cur_thread() and here to skip the right word.

[fjricci]

Factor out ThreadState location into helper function

[dvyukov]

LGTM with a nit

lib/tsan/rtl/tsan_platform_mac.cc

253

Please add CHECKs that thr is in fact located within tls range, similar to linux impl.

[fjricci]

Committed as rL303886

lib/tsan/rtl/tsan_platform.h

 void InitializePlatform();
 void InitializePlatformEarly();
 void CheckAndProtect();
 void InitializeShadowMemoryPlatform();
 void FlushShadowMemory();
 void WriteMemoryProfile(char *buf, uptr buf_size, uptr nthread, uptr nlive);
 int ExtractResolvFDs(void *state, int *fds, int nfd);
 int ExtractRecvmsgFDs(void *msg, int *fds, int nfd);
+void ImitateTlsWrite(ThreadState *thr, uptr tls_addr, uptr tls_size);
 
 int call_pthread_cancel_with_cleanup(int(*fn)(void *c, void *m,
     void *abstime), void *c, void *m, void *abstime,
     void(*cleanup)(void *arg), void *arg);
 
 void DestroyThreadState();
 
 }  // namespace __tsan
 
 #endif  // TSAN_PLATFORM_H

lib/tsan/rtl/tsan_platform_linux.cc

for (int i = 0; i < n; i++) {
       fds[res++] = ((int*)CMSG_DATA(cmsg))[i];
       if (res == nfd)
         return res;
     }
   }
   return res;
 }
 
+void ImitateTlsWrite(ThreadState *thr, uptr tls_addr, uptr tls_size) {
+  // Check that the thr object is in tls;
+  const uptr thr_beg = (uptr)thr;
+  const uptr thr_end = (uptr)thr + sizeof(*thr);
+  CHECK_GE(thr_beg, tls_addr);
+  CHECK_LE(thr_beg, tls_addr + tls_size);
+  CHECK_GE(thr_end, tls_addr);
+  CHECK_LE(thr_end, tls_addr + tls_size);
+  // Since the thr object is huge, skip it.
+  MemoryRangeImitateWrite(thr, /*pc=*/ 2, tls_addr, thr_beg - tls_addr);
+  MemoryRangeImitateWrite(thr, /*pc=*/ 2,
+      thr_end, tls_addr + tls_size - thr_end);
+}
+
 // Note: this function runs with async signals enabled,
 // so it must not touch any tsan state.
 int call_pthread_cancel_with_cleanup(int(*fn)(void *c, void *m,
     void *abstime), void *c, void *m, void *abstime,
     void(*cleanup)(void *arg), void *arg) {
   // pthread_cleanup_push/pop are hardcore macros mess.
   // We can't intercept nor call them w/o including pthread.h.
   int res;

lib/tsan/rtl/tsan_platform_mac.cc

#if !SANITIZER_GO
   main_thread_identity = (uptr)pthread_self();
 
   prev_pthread_introspection_hook =
       pthread_introspection_hook_install(&my_pthread_introspection_hook);
 #endif
 }
 
 #if !SANITIZER_GO
+void ImitateTlsWrite(ThreadState *thr, uptr tls_addr, uptr tls_size) {
+  CHECK_EQ(tls_addr, (uptr)pthread_self());
+
+  // We stash a pointer to ThreadState in the tls shadow memory,
+  // make sure we don't clobber it.
+  tls_addr += sizeof(uptr);
+  tls_size -= sizeof(uptr);
+  MemoryRangeImitateWrite(thr, /*pc=*/ 2, tls_addr, tls_size);
+}
+#endif
+
+#if !SANITIZER_GO
 // Note: this function runs with async signals enabled,
 // so it must not touch any tsan state.
 int call_pthread_cancel_with_cleanup(int(*fn)(void *c, void *m,
     void *abstime), void *c, void *m, void *abstime,
     void(*cleanup)(void *arg), void *arg) {
   // pthread_cleanup_push/pop are hardcore macros mess.
   // We can't intercept nor call them w/o including pthread.h.
   int res;

lib/tsan/rtl/tsan_rtl_thread.cc

void ThreadStart(ThreadState *thr, int tid, tid_t os_id, bool workerthread) {
   uptr tls_size = 0;
 #if !SANITIZER_GO
   GetThreadStackAndTls(tid == 0, &stk_addr, &stk_size, &tls_addr, &tls_size);
 
   if (tid) {
     if (stk_addr && stk_size)
       MemoryRangeImitateWrite(thr, /*pc=*/ 1, stk_addr, stk_size);
 
-    if (tls_addr && tls_size) {
-      // Check that the thr object is in tls;
-      const uptr thr_beg = (uptr)thr;
-      const uptr thr_end = (uptr)thr + sizeof(*thr);
-      CHECK_GE(thr_beg, tls_addr);
-      CHECK_LE(thr_beg, tls_addr + tls_size);
-      CHECK_GE(thr_end, tls_addr);
-      CHECK_LE(thr_end, tls_addr + tls_size);
-      // Since the thr object is huge, skip it.
-      MemoryRangeImitateWrite(thr, /*pc=*/ 2, tls_addr, thr_beg - tls_addr);
-      MemoryRangeImitateWrite(thr, /*pc=*/ 2,
-          thr_end, tls_addr + tls_size - thr_end);
-    }
+    if (tls_addr && tls_size)
+      ImitateTlsWrite(thr, tls_addr, tls_size);
   }
 #endif
 
   ThreadRegistry *tr = ctx->thread_registry;
   OnStartedArgs args = { thr, stk_addr, stk_size, tls_addr, tls_size };
   tr->StartThread(tid, os_id, workerthread, &args);
 
   tr->Lock();