tsan: model atomic read for failing CAS

See the added test and https://github.com/google/sanitizers/issues/1520
for the description of the problem.
The standard says that failing CAS is a memory load only,
model it as such to avoid false positives.

Reviewed By: melver

Differential Revision: https://reviews.llvm.org/D124507

2 files changed, 56 insertions, 5 deletions

diff --git a/compiler-rt/lib/tsan/rtl/tsan_interface_atomic.cpp b/compiler-rt/lib/tsan/rtl/tsan_interface_atomic.cpp
index f794a2fcdd0d..efa1ad2ecd82 100644
--- a/compiler-rt/lib/tsan/rtl/tsan_interface_atomic.cpp
+++ b/compiler-rt/lib/tsan/rtl/tsan_interface_atomic.cpp
@@ -411,14 +411,39 @@ static bool AtomicCAS(ThreadState *thr, uptr pc, volatile T *a, T *c, T v,
   // (mo_relaxed) when those are used.
   DCHECK(IsLoadOrder(fmo));
 
-  MemoryAccess(thr, pc, (uptr)a, AccessSize<T>(), kAccessWrite | kAccessAtomic);
   if (LIKELY(mo == mo_relaxed && fmo == mo_relaxed)) {
     T cc = *c;
     T pr = func_cas(a, cc, v);
-    if (pr == cc)
-      return true;
-    *c = pr;
-    return false;
+    bool success = pr == cc;
+    if (!success)
+      *c = pr;
+    // We used to always model a write and before the actual CAS.
+    // However, the standard 29.6.5/21 says:
+    // If the operation returns true, these operations are atomic
+    // read-modify-write operations (1.10). Otherwise, these operations
+    // are atomic load operations.
+    //
+    // And this difference can lead to false positive reports when
+    // a program uses __atomic builtins and one thread executes a failing CAS
+    // while another executes a non-atomic load. If we always model a write
+    // these operations will race.
+    //
+    // This leads to another subtle aspect:
+    // Atomics provide destruction-safety and can be used to synchronize
+    // own destruction/freeing of underlying memory.
+    // So one thread can do (a successful) CAS to signal to free the memory,
+    // and another thread do an atomic load, observe the CAS and free the
+    // memory. So modelling the memory access after the actual access is
+    // somewhat risky (can potentially lead to other false positive reports if
+    // the memory is already reused when we model the memory access). However,
+    // destruction signalling must use acquire/release memory ordering
+    // constraints. So we should lock s->mtx below and it should prevent the
+    // reuse race. If/when we support stand-alone memory fences, it's unclear
+    // how to handle this case. We could always lock s->mtx even for relaxed
+    // accesses, but it will have severe performance impact on relaxed loads.
+    MemoryAccess(thr, pc, (uptr)a, AccessSize<T>(),
+                 kAccessAtomic | (success ? kAccessWrite : kAccessRead));
+    return success;
   }
   SlotLocker locker(thr);
   bool release = IsReleaseOrder(mo);
@@ -433,6 +458,9 @@ static bool AtomicCAS(ThreadState *thr, uptr pc, volatile T *a, T *c, T v,
       *c = pr;
       mo = fmo;
     }
+    // See the comment on the previous MemoryAccess.
+    MemoryAccess(thr, pc, (uptr)a, AccessSize<T>(),
+                 kAccessAtomic | (success ? kAccessWrite : kAccessRead));
     if (success && IsAcqRelOrder(mo))
       thr->clock.ReleaseAcquire(&s->clock);
     else if (success && IsReleaseOrder(mo))
diff --git a/compiler-rt/test/tsan/atomic_norace3.cpp b/compiler-rt/test/tsan/atomic_norace3.cpp
new file mode 100644
index 000000000000..6ac2c9ea29d7
--- /dev/null
+++ b/compiler-rt/test/tsan/atomic_norace3.cpp
@@ -0,0 +1,23 @@
+// RUN: %clangxx_tsan -O1 %s %link_libcxx_tsan -o %t && %run %t 2>&1 | FileCheck %s
+
+#include "test.h"
+#include <thread>
+
+int main() {
+  barrier_init(&barrier, 2);
+  volatile int x = 0;
+  std::thread reader([&]() {
+    barrier_wait(&barrier);
+    int l = x;
+    (void)l;
+  });
+  int cmp = 1;
+  __atomic_compare_exchange_n(&x, &cmp, 1, 1, __ATOMIC_RELAXED,
+                              __ATOMIC_RELAXED);
+  barrier_wait(&barrier);
+  reader.join();
+  fprintf(stderr, "DONE\n");
+}
+
+// CHECK-NOT: ThreadSanitizer: data race
+// CHECK: DONE