tsan: fixes to ThreadClock::releaseStoreAcquire

[dfava]

Summary

Fixes:

1. Setting the number of entries in a thread's clock to max between the thread and the SyncClock the thread is acquiring from
2. Setting last_acquire_

Unit- and stress-test for releaseStoreAcquire added to tests/unit/tsan_clock_test.cpp

[dvyukov]

Please add a unit test for these bugs to tsan/tests/unit/tsan_clock_test.cpp and releaseStoreAcquire to the stress test in that file.

[dvyukov]

You can now land this yourself, right?

[dfava]

Committed in https://github.com/llvm/llvm-project/commit/3b222ef246ef9298e57d00b7c8c1ffd92d0fb44d

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

void ThreadClock::releaseStoreAcquire(ClockCache *c, SyncClock *sc) {
 
   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.
+  nclk_ = max(nclk_, (uptr) sc->size_);
+
+  // Check if we need to resize sc.
   if (sc->size_ < nclk_)
     sc->Resize(c, nclk_);
 
+  bool acquired = false;
+
   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]);
+    if (clk_[i] < ce.epoch) {
+      clk_[i] = ce.epoch;
+      acquired = true;
+    }
     ce.epoch = tmp;
     ce.reused = 0;
     i++;
   }
   sc->release_store_tid_ = kInvalidTid;
   sc->release_store_reused_ = 0;
+
+  if (acquired) {
+    CPP_STAT_INC(StatClockAcquiredSomething);
+    last_acquire_ = clk_[tid_];
+    ResetCached(c);
+  }
 }
 
 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_,

compiler-rt/lib/tsan/tests/unit/tsan_clock_test.cpp

TEST(Clock, RepeatedAcquire) {
   thr1.ReleaseStore(&cache, &sync);
 
   thr2.acquire(&cache, &sync);
   thr2.acquire(&cache, &sync);
 
   sync.Reset(&cache);
 }
 
+TEST(Clock, releaseStoreAcquire) {
+  ThreadClock thr0(0);
+  thr0.tick();
+  ThreadClock thr1(1);
+  thr1.tick();
+  SyncClock syncA;
+  SyncClock syncB;
+  ASSERT_EQ(syncA.size(), 0U);
+  ASSERT_EQ(syncB.size(), 0U);
+  thr1.releaseStoreAcquire(&cache, &syncB);
+  ASSERT_EQ(syncB.size(), 2U); // T0 and T1
+  // releaseStoreAcquire to an empty SyncClock
+  thr0.releaseStoreAcquire(&cache, &syncA);
+  ASSERT_EQ(syncA.size(), 1U);
+  // releaseStoreAcquire from a non-empty SyncClock
+  // T0 learns about T1
+  thr0.releaseStoreAcquire(&cache, &syncB);
+  // releaseStoreAcquire to the originally empty SyncClock
+  // T0 deposits info about T1 into syncA
+  thr0.releaseStoreAcquire(&cache, &syncA);
+  ASSERT_EQ(syncA.size(), 2U);
+  syncA.Reset(&cache);
+  syncB.Reset(&cache);
+}
+
 TEST(Clock, ManyThreads) {
   SyncClock chunked;
   for (unsigned i = 0; i < 200; i++) {
     ThreadClock vector(0);
     vector.tick();
     vector.set(&cache, i, i + 1);
     vector.release(&cache, &chunked);
     ASSERT_EQ(i + 1, chunked.size());
struct SimpleThreadClock {
 
   void release(SimpleSyncClock *dst) const {
     if (dst->size < size)
       dst->size = size;
     for (uptr i = 0; i < kThreads; i++)
       dst->clock[i] = max(dst->clock[i], clock[i]);
   }
 
+  void releaseStoreAcquire(SimpleSyncClock *sc) {
+    if (sc->size < size)
+      sc->size = size;
+    else
+      size = sc->size;
+    for (uptr i = 0; i < kThreads; i++) {
+      uptr tmp = clock[i];
+      clock[i] = max(sc->clock[i], clock[i]);
+      sc->clock[i] = tmp;
+    }
+  }
+
   void acq_rel(SimpleSyncClock *dst) {
     acquire(dst);
     release(dst);
   }
 
   void ReleaseStore(SimpleSyncClock *dst) const {
     if (dst->size < size)
       dst->size = size;
static bool ClockFuzzer(bool printing) {
   // Do N random operations (acquire, release, etc) and compare results
   // for SimpleThread/SyncClock and real Thread/SyncClock.
   for (int i = 0; i < 10000; i++) {
     unsigned tid = rand() % kThreads;
     unsigned cid = rand() % kClocks;
     thr0[tid]->tick();
     thr1[tid]->tick();
 
-    switch (rand() % 6) {
+    switch (rand() % 7) {
     case 0:
       if (printing)
         printf("acquire thr%d <- clk%d\n", tid, cid);
       thr0[tid]->acquire(sync0[cid]);
       thr1[tid]->acquire(&cache, sync1[cid]);
       break;
     case 1:
       if (printing)
for (int i = 0; i < 10000; i++) {
     case 4:
       if (printing)
         printf("reset clk%d\n", cid);
       sync0[cid]->Reset();
       sync1[cid]->Reset(&cache);
       break;
     case 5:
       if (printing)
+        printf("releaseStoreAcquire thr%d -> clk%d\n", tid, cid);
+      thr0[tid]->releaseStoreAcquire(sync0[cid]);
+      thr1[tid]->releaseStoreAcquire(&cache, sync1[cid]);
+      break;
+    case 6:
+      if (printing)
         printf("reset thr%d\n", tid);
       u64 epoch = thr0[tid]->clock[tid] + 1;
       reused[tid]++;
       delete thr0[tid];
       thr0[tid] = new SimpleThreadClock(tid);
       thr0[tid]->clock[tid] = epoch;
       delete thr1[tid];
       thr1[tid] = new ThreadClock(tid, reused[tid]);