tsan: Adding releaseAcquire() to ThreadClock

[dfava]

Summary

realeaseAcquire() is a new function added to TSan in support of the Go data-race detector. It's semantics is:

    
void ThreadClock::releaseAcquire(SyncClock *sc) const {
  for (int i = 0; i < kMaxThreads; i++) {
    tmp = clock[i];
    clock[i] = max(clock[i], sc->clock[i]);
    sc->clock[i] = tmp;
  }
}

[dfava]

Added changes to tsan_go.cpp which I had forgotten to include in the first diff.

[dvyukov]

Overall looks good, which some nits.

Please also add this function to compiler-rt/lib/tsan/go/test.c. It's not a super comprehensive test, but it ensures compilation and absence of stupid crashes. Which is still very useful in the situation when tsan/Go parts are split across 2 repos and are updated episodically.

compiler-rt/lib/tsan/rtl/tsan_clock.cpp

37

So this is effectively releaseStore.
There is a somewhat mismatching terminology we use in tsan runtime vs Go api.
If Go api these are called releaseMerge and release, which corresponds to release and releaseStore terminology in tsan runtime. So more complete name would be something like releaseStoreAcquire.

205

This acquired is not used (other than for stats).
We either need to remove this, or do just release and return if already acquired. I think that the expected case is that this is not acquired yet, so probably we drop this part?

[dvyukov]

Perfect. Thanks.

[dvyukov]

Committed in https://github.com/llvm/llvm-project/commit/180d211770a0a1f82d483433fab007cd4d14cbd9

[pcc]

It looks like this change broke the sanitizer-x86_64-linux-autoconf buildbot.
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-autoconf/builds/48824

/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/tsan/rtl/tsan_clock.cpp:190:13: error: use of undeclared identifier 'dst'
  DCHECK_LE(dst->size_, kMaxTid);
            ^
1 error generated.

Can you please take a look?

[dfava]

In D76322#1946849, @pcc wrote:

It looks like this change broke the sanitizer-x86_64-linux-autoconf buildbot.
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-autoconf/builds/48824

/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/tsan/rtl/tsan_clock.cpp:190:13: error: use of undeclared identifier 'dst'
  DCHECK_LE(dst->size_, kMaxTid);
            ^
1 error generated.

Can you please take a look?

Indeed a bug. It looks like the DCHECK is #defined to nothing unless SANITIZER_DEBUG is set, which is why we didn't catch the issue earlier.

I mentioned in an email just now that I would fix it, but since this is a one line fix and I don't have commit access, would it be easier if your or Dmitry makes the change?

The change is from:

DCHECK_LE(dst->size_, kMaxTid);

to:

DCHECK_LE(sc->size_, kMaxTid);

Let me know.

Sorry for the regression and thanks again for pointing it out.

[dvyukov]

In D76322#1947063, @dfava wrote:

In D76322#1946849, @pcc wrote:

It looks like this change broke the sanitizer-x86_64-linux-autoconf buildbot.
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-autoconf/builds/48824

/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/tsan/rtl/tsan_clock.cpp:190:13: error: use of undeclared identifier 'dst'
  DCHECK_LE(dst->size_, kMaxTid);
            ^
1 error generated.

Can you please take a look?

Indeed a bug. It looks like the DCHECK is #defined to nothing unless SANITIZER_DEBUG is set, which is why we didn't catch the issue earlier.

I mentioned in an email just now that I would fix it, but since this is a one line fix and I don't have commit access, would it be easier if your or Dmitry makes the change?

The change is from:

DCHECK_LE(dst->size_, kMaxTid);

to:

DCHECK_LE(sc->size_, kMaxTid);

Let me know.

Sorry for the regression and thanks again for pointing it out.

Done in https://github.com/llvm/llvm-project/commit/65b4695375c6518c0b51a2a4c8392ab9d461e317

compiler-rt/lib/tsan/go/test.c

 void __tsan_read(void *thr, void *addr, void *pc);
 void __tsan_write(void *thr, void *addr, void *pc);
 void __tsan_func_enter(void *thr, void *pc);
 void __tsan_func_exit(void *thr);
 void __tsan_malloc(void *thr, void *pc, void *p, unsigned long sz);
 void __tsan_free(void *p, unsigned long sz);
 void __tsan_acquire(void *thr, void *addr);
 void __tsan_release(void *thr, void *addr);
+void __tsan_release_acquire(void *thr, void *addr);
 void __tsan_release_merge(void *thr, void *addr);
 
 void *current_proc;
 
 void symbolize_cb(long cmd, void *ctx) {
   switch (cmd) {
   case 0:
     if (current_proc == 0)
int main(void) {
   }
   char *buf = (char*)((unsigned long)buf0 + (64<<10) - 1 & ~((64<<10) - 1));
   __tsan_map_shadow(buf, 4096);
   __tsan_malloc(thr0, (char*)&barfoo + 1, buf, 10);
   __tsan_free(buf, 10);
   __tsan_func_enter(thr0, (char*)&main + 1);
   __tsan_malloc(thr0, (char*)&barfoo + 1, buf, 10);
   __tsan_release(thr0, buf);
+  __tsan_release_acquire(thr0, buf);
   __tsan_release_merge(thr0, buf);
   void *thr1 = 0;
   __tsan_go_start(thr0, &thr1, (char*)&barfoo + 1);
   void *thr2 = 0;
   __tsan_go_start(thr0, &thr2, (char*)&barfoo + 1);
   __tsan_func_exit(thr0);
   __tsan_func_enter(thr1, (char*)&foobar + 1);
   __tsan_func_enter(thr1, (char*)&foobar + 1);

compiler-rt/lib/tsan/go/tsan_go.cpp

 void __tsan_proc_destroy(Processor *proc) {
   ProcDestroy(proc);
 }
 
 void __tsan_acquire(ThreadState *thr, void *addr) {
   Acquire(thr, 0, (uptr)addr);
 }
 
+void __tsan_release_acquire(ThreadState *thr, void *addr) {
+  ReleaseStoreAcquire(thr, 0, (uptr)addr);
+}
+
 void __tsan_release(ThreadState *thr, void *addr) {
   ReleaseStore(thr, 0, (uptr)addr);
 }
 
 void __tsan_release_merge(ThreadState *thr, void *addr) {
   Release(thr, 0, (uptr)addr);
 }
 

compiler-rt/lib/tsan/rtl/tsan_clock.h

public:
 
   u64 get(unsigned tid) const;
   void set(ClockCache *c, unsigned tid, u64 v);
   void set(u64 v);
   void tick();
   uptr size() const;
 
   void acquire(ClockCache *c, SyncClock *src);
+  void releaseStoreAcquire(ClockCache *c, SyncClock *src);
   void release(ClockCache *c, SyncClock *dst);
   void acq_rel(ClockCache *c, SyncClock *dst);
   void ReleaseStore(ClockCache *c, SyncClock *dst);
   void ResetCached(ClockCache *c);
 
   void DebugReset();
   void DebugDump(int(*printf)(const char *s, ...));
 

compiler-rt/lib/tsan/rtl/tsan_clock.cpp

 //       clock[i] = max(clock[i], src->clock[i]);
 //   }
 //
 //   void ThreadClock::release(SyncClock *dst) const {
 //     for (int i = 0; i < kMaxThreads; i++)
 //       dst->clock[i] = max(dst->clock[i], clock[i]);
 //   }
 //
+//   void ThreadClock::releaseStoreAcquire(SyncClock *sc) const {
+//     for (int i = 0; i < kMaxThreads; i++) {
+//       tmp = clock[i];
+//       clock[i] = max(clock[i], sc->clock[i]);
+//       sc->clock[i] = tmp;
+//     }
+//   }
+//
 //   void ThreadClock::ReleaseStore(SyncClock *dst) const {
 //     for (int i = 0; i < kMaxThreads; i++)
 //       dst->clock[i] = clock[i];
 //   }
 //
 //   void ThreadClock::acq_rel(SyncClock *dst) {
 //     acquire(dst);
 //     release(dst);
void ThreadClock::acquire(ClockCache *c, SyncClock *src) {
 
   if (acquired) {
     CPP_STAT_INC(StatClockAcquiredSomething);
     last_acquire_ = clk_[tid_];
     ResetCached(c);
   }
 }
 
+void ThreadClock::releaseStoreAcquire(ClockCache *c, SyncClock *sc) {
+  DCHECK_LE(nclk_, kMaxTid);
+  DCHECK_LE(dst->size_, kMaxTid);
+
+  if (sc->size_ == 0) {
+    // ReleaseStore will correctly set release_store_tid_,
+    // which can be important for future operations.
+    ReleaseStore(c, sc);
+    return;
+  }
+
+  // Check if we need to resize dst.
+  if (sc->size_ < nclk_)
+    sc->Resize(c, nclk_);
+
+  sc->Unshare(c);
+  // Update sc->clk_.
+  sc->FlushDirty();
+  uptr i = 0;
+  for (ClockElem &ce : *sc) {
+    u64 tmp = clk_[i];
+    clk_[i] = max(ce.epoch, clk_[i]);
+    ce.epoch = tmp;
+    ce.reused = 0;
+    i++;
+  }
+  sc->release_store_tid_ = kInvalidTid;
+  sc->release_store_reused_ = 0;
+}
+
 void ThreadClock::release(ClockCache *c, SyncClock *dst) {
   DCHECK_LE(nclk_, kMaxTid);
   DCHECK_LE(dst->size_, kMaxTid);
 
   if (dst->size_ == 0) {
     // ReleaseStore will correctly set release_store_tid_,
     // which can be important for future operations.
     ReleaseStore(c, dst);

compiler-rt/lib/tsan/rtl/tsan_rtl.h

 // AcquireGlobal synchronizes the current thread with all other threads.
 // In terms of happens-before relation, it draws a HB edge from all threads
 // (where they happen to execute right now) to the current thread. We use it to
 // handle Go finalizers. Namely, finalizer goroutine executes AcquireGlobal
 // right before executing finalizers. This provides a coarse, but simple
 // approximation of the actual required synchronization.
 void AcquireGlobal(ThreadState *thr, uptr pc);
 void Release(ThreadState *thr, uptr pc, uptr addr);
+void ReleaseStoreAcquire(ThreadState *thr, uptr pc, uptr addr);
 void ReleaseStore(ThreadState *thr, uptr pc, uptr addr);
 void AfterSleep(ThreadState *thr, uptr pc);
 void AcquireImpl(ThreadState *thr, uptr pc, SyncClock *c);
 void ReleaseImpl(ThreadState *thr, uptr pc, SyncClock *c);
+void ReleaseStoreAcquireImpl(ThreadState *thr, uptr pc, SyncClock *c);
 void ReleaseStoreImpl(ThreadState *thr, uptr pc, SyncClock *c);
 void AcquireReleaseImpl(ThreadState *thr, uptr pc, SyncClock *c);
 
 // The hacky call uses custom calling convention and an assembly thunk.
 // It is considerably faster that a normal call for the caller
 // if it is not executed (it is intended for slow paths from hot functions).
 // The trick is that the call preserves all registers and the compiler
 // does not treat it as a call.

compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp

void AcquireGlobal(ThreadState *thr, uptr pc) {
   DPrintf("#%d: AcquireGlobal\n", thr->tid);
   if (thr->ignore_sync)
     return;
   ThreadRegistryLock l(ctx->thread_registry);
   ctx->thread_registry->RunCallbackForEachThreadLocked(
       UpdateClockCallback, thr);
 }
 
+void ReleaseStoreAcquire(ThreadState *thr, uptr pc, uptr addr) {
+  DPrintf("#%d: ReleaseStoreAcquire %zx\n", thr->tid, addr);
+  if (thr->ignore_sync)
+    return;
+  SyncVar *s = ctx->metamap.GetOrCreateAndLock(thr, pc, addr, true);
+  thr->fast_state.IncrementEpoch();
+  // Can't increment epoch w/o writing to the trace as well.
+  TraceAddEvent(thr, thr->fast_state, EventTypeMop, 0);
+  ReleaseStoreAcquireImpl(thr, pc, &s->clock);
+  s->mtx.Unlock();
+}
+
 void Release(ThreadState *thr, uptr pc, uptr addr) {
   DPrintf("#%d: Release %zx\n", thr->tid, addr);
   if (thr->ignore_sync)
     return;
   SyncVar *s = ctx->metamap.GetOrCreateAndLock(thr, pc, addr, true);
   thr->fast_state.IncrementEpoch();
   // Can't increment epoch w/o writing to the trace as well.
   TraceAddEvent(thr, thr->fast_state, EventTypeMop, 0);

 void AcquireImpl(ThreadState *thr, uptr pc, SyncClock *c) {
   if (thr->ignore_sync)
     return;
   thr->clock.set(thr->fast_state.epoch());
   thr->clock.acquire(&thr->proc()->clock_cache, c);
   StatInc(thr, StatSyncAcquire);
 }
 
+void ReleaseStoreAcquireImpl(ThreadState *thr, uptr pc, SyncClock *c) {
+  if (thr->ignore_sync)
+    return;
+  thr->clock.set(thr->fast_state.epoch());
+  thr->fast_synch_epoch = thr->fast_state.epoch();
+  thr->clock.releaseStoreAcquire(&thr->proc()->clock_cache, c);
+  StatInc(thr, StatSyncReleaseStoreAcquire);
+}
+
 void ReleaseImpl(ThreadState *thr, uptr pc, SyncClock *c) {
   if (thr->ignore_sync)
     return;
   thr->clock.set(thr->fast_state.epoch());
   thr->fast_synch_epoch = thr->fast_state.epoch();
   thr->clock.release(&thr->proc()->clock_cache, c);
   StatInc(thr, StatSyncRelease);
 }

compiler-rt/lib/tsan/rtl/tsan_stat.h

enum StatType {
   StatMutexReadLock,
   StatMutexReadUnlock,
 
   // Synchronization.
   StatSyncCreated,
   StatSyncDestroyed,
   StatSyncAcquire,
   StatSyncRelease,
+  StatSyncReleaseStoreAcquire,
 
   // Clocks - acquire.
   StatClockAcquire,
   StatClockAcquireEmpty,
   StatClockAcquireFastRelease,
   StatClockAcquireFull,
   StatClockAcquiredSomething,
   // Clocks - release.