tsan: Support calling ThreadCreate()/ThreadStart()/ThreadFinish() for non-current thread

[yuri]

Summary

Currently it is not possible to call any of ThreadCreate()/ThreadStart()/ThreadFinish() with thr argument that is not current thread because of HACKY_CALL() implementation. This removes this limitation by calling TraceSwitch() directly on corresponding code paths.

Only generated code for ThreadContext::OnCreated()/ThreadContext::OnStarted()/ThreadContext::OnFinished() has changed (they became few bytes shorter). There is no any changes in performance-critical functions.

[yuri]

I have working implemented of fibers (coroutines) support for thread sanitizer.
Taking in the account that fibers are supported by address sanitizer, support in thread sanitizer may be useful as well.

This is a first patch of the changeset. Please advise, what is the preferred process of reviewing in such cases: submit patches one-by-one or submit the entire changeset?

[dvyukov]

In D54835#1306431, @yuri wrote:

I have working implemented of fibers (coroutines) support for thread sanitizer.
Taking in the account that fibers are supported by address sanitizer, support in thread sanitizer may be useful as well.

This is a first patch of the changeset. Please advise, what is the preferred process of reviewing in such cases: submit patches one-by-one or submit the entire changeset?

Hi Yuri,

Fibers support sounds great.
Regardless of how we proceed with submitting patches, I would like to see the whole change first. Otherwise it's hard to see reasoning behind individual changes and understand if it's right approach or not.
Please upload whole change into a separate review.

Thanks

[yuri]

In D54835#1306574, @dvyukov wrote:

Fibers support sounds great.
Regardless of how we proceed with submitting patches, I would like to see the whole change first. Otherwise it's hard to see reasoning behind individual changes and understand if it's right approach or not.
Please upload whole change into a separate review.

Hi Dmitry,
I have uploaded full code in https://reviews.llvm.org/D54889

[dvyukov]

In D54835#1307684, @yuri wrote:

In D54835#1306574, @dvyukov wrote:

Fibers support sounds great.
Regardless of how we proceed with submitting patches, I would like to see the whole change first. Otherwise it's hard to see reasoning behind individual changes and understand if it's right approach or not.
Please upload whole change into a separate review.

Hi Dmitry,
I have uploaded full code in https://reviews.llvm.org/D54889

Thanks. Just to let you know, I am travelling this whole week, don't know yet when I will be able to look at this.

[yuri]

Problem solved differently in D54889

lib/tsan/rtl/tsan_rtl.h

 void TraceSwitch(ThreadState *thr);
 uptr TraceTopPC(ThreadState *thr);
 uptr TraceSize();
 uptr TraceParts();
 Trace *ThreadTrace(int tid);
 
 extern "C" void __tsan_trace_switch();
 void ALWAYS_INLINE TraceAddEvent(ThreadState *thr, FastState fs,
-                                        EventType typ, u64 addr) {
+                                        EventType typ, u64 addr,
+                                        bool hacky_call = true) {
   if (!kCollectHistory)
     return;
   DCHECK_GE((int)typ, 0);
   DCHECK_LE((int)typ, 7);
   DCHECK_EQ(GetLsb(addr, kEventPCBits), addr);
   StatInc(thr, StatEvents);
   u64 pos = fs.GetTracePos();
   if (UNLIKELY((pos % kTracePartSize) == 0)) {
 #if !SANITIZER_GO
+    if (hacky_call) {
+      DCHECK_EQ(thr, cur_thread());
       HACKY_CALL(__tsan_trace_switch);
-#else
-    TraceSwitch(thr);
+    } else
 #endif
+      TraceSwitch(thr);
   }
   Event *trace = (Event*)GetThreadTrace(fs.tid());
   Event *evp = &trace[pos];
   Event ev = (u64)addr | ((u64)typ << kEventPCBits);
   *evp = ev;
 }
 
 #if !SANITIZER_GO
 uptr ALWAYS_INLINE HeapEnd() {
   return HeapMemEnd() + PrimaryAllocator::AdditionalSize();
 }
 #endif
 
 }  // namespace __tsan
 
 #endif  // TSAN_RTL_H

lib/tsan/rtl/tsan_rtl.cc

 
 ALWAYS_INLINE
 void HandleRace(ThreadState *thr, u64 *shadow_mem,
                               Shadow cur, Shadow old) {
   thr->racy_state[0] = cur.raw();
   thr->racy_state[1] = old.raw();
   thr->racy_shadow_addr = shadow_mem;
 #if !SANITIZER_GO
+  DCHECK_EQ(thr, cur_thread());
   HACKY_CALL(__tsan_report_race);
 #else
   ReportRace(thr);
 #endif
 }
 
 static inline bool HappensBefore(Shadow old, ThreadState *thr) {
   return thr->clock.get(old.TidWithIgnore()) >= old.epoch();

lib/tsan/rtl/tsan_rtl_thread.cc

void ThreadContext::OnCreated(void *arg) {
   thr = 0;
   if (tid == 0)
     return;
   OnCreatedArgs *args = static_cast<OnCreatedArgs *>(arg);
   if (!args->thr)  // GCD workers don't have a parent thread.
     return;
   args->thr->fast_state.IncrementEpoch();
   // Can't increment epoch w/o writing to the trace as well.
-  TraceAddEvent(args->thr, args->thr->fast_state, EventTypeMop, 0);
+  TraceAddEvent(args->thr, args->thr->fast_state, EventTypeMop, 0, false);
   ReleaseImpl(args->thr, 0, &sync);
   creation_stack_id = CurrentStackId(args->thr, args->pc);
   if (reuse_count == 0)
     StatInc(args->thr, StatThreadMaxTid);
 }
 
 void ThreadContext::OnReset() {
   CHECK_EQ(sync.size(), 0);
#else
   thr->shadow_stack_pos = thr->shadow_stack;
   thr->shadow_stack_end = thr->shadow_stack + kInitStackSize;
 #endif
   if (common_flags()->detect_deadlocks)
     thr->dd_lt = ctx->dd->CreateLogicalThread(unique_id);
   thr->fast_state.SetHistorySize(flags()->history_size);
   // Commit switch to the new part of the trace.
   // TraceAddEvent will reset stack0/mset0 in the new part for us.
-  TraceAddEvent(thr, thr->fast_state, EventTypeMop, 0);
+  TraceAddEvent(thr, thr->fast_state, EventTypeMop, 0, false);
 
   thr->fast_synch_epoch = epoch0;
   AcquireImpl(thr, 0, &sync);
   StatInc(thr, StatSyncAcquire);
   sync.Reset(&thr->proc()->clock_cache);
   thr->is_inited = true;
   DPrintf("#%d: ThreadStart epoch=%zu stk_addr=%zx stk_size=%zx "
           "tls_addr=%zx tls_size=%zx\n",
           tid, (uptr)epoch0, args->stk_addr, args->stk_size,
           args->tls_addr, args->tls_size);
 }
 
 void ThreadContext::OnFinished() {
 #if SANITIZER_GO
   internal_free(thr->shadow_stack);
   thr->shadow_stack = nullptr;
   thr->shadow_stack_pos = nullptr;
   thr->shadow_stack_end = nullptr;
 #endif
   if (!detached) {
     thr->fast_state.IncrementEpoch();
     // Can't increment epoch w/o writing to the trace as well.
-    TraceAddEvent(thr, thr->fast_state, EventTypeMop, 0);
+    TraceAddEvent(thr, thr->fast_state, EventTypeMop, 0, false);
     ReleaseImpl(thr, 0, &sync);
   }
   epoch1 = thr->fast_state.epoch();
 
   if (common_flags()->detect_deadlocks)
     ctx->dd->DestroyLogicalThread(thr->dd_lt);
   thr->clock.ResetCached(&thr->proc()->clock_cache);
 #if !SANITIZER_GO

 }
 
 void ThreadStart(ThreadState *thr, int tid, tid_t os_id, bool workerthread) {
   uptr stk_addr = 0;
   uptr stk_size = 0;
   uptr tls_addr = 0;
   uptr tls_size = 0;
 #if !SANITIZER_GO
+  if (os_id)
     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) ImitateTlsWrite(thr, tls_addr, tls_size);
   }
 #endif