[compiler-rt] Fix race in the implementation of __tsan_acquire() (PR #84923)

[via llvm-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: Dave Clausen (daveclausen)

<details>
<summary>Changes</summary>

`__tsan::Acquire()`, which is called by `__tsan_acquire()`, has a performance optimization which attempts to avoid acquiring the atomic variable's mutex if the variable has no associated memory model state.

However, if the atomic variable was recently written to by a `compare_exchange_weak/strong` on another thread, the memory model state may be created *after* the atomic variable is updated. This is a data race, and can cause the thread calling `Acquire()` to not realize that the atomic variable was previously written to by another thread.

Specifically, if you have code that writes to an atomic variable using `compare_exchange_weak/strong`, and then in another thread you read the value using a relaxed load, followed by an `atomic_thread_fence(memory_order_acquire)`, followed by a call to `__tsan_acquire()`, TSAN may not realize that the store happened before the fence, and so it will complain about any other variables you access from both threads if the thread-safety of those accesses depended on the happens-before relationship between the store and the fence.

This change eliminates the unsafe optimization in `Acquire()`. Now, `Acquire()` acquires the mutex before checking for the existence of the memory model state.

---
Full diff: https://github.com/llvm/llvm-project/pull/84923.diff


2 Files Affected:

- (modified) compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp (+1-1) 
- (added) compiler-rt/test/tsan/compare_exchange_acquire_fence.cpp (+46) 


``````````diff
diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp b/compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp
index 2e978852ea7d37..2a8aa1915c9aeb 100644
--- a/compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp
+++ b/compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp
@@ -446,9 +446,9 @@ void Acquire(ThreadState *thr, uptr pc, uptr addr) {
   if (!s)
     return;
   SlotLocker locker(thr);
+  ReadLock lock(&s->mtx);
   if (!s->clock)
     return;
-  ReadLock lock(&s->mtx);
   thr->clock.Acquire(s->clock);
 }
 
diff --git a/compiler-rt/test/tsan/compare_exchange_acquire_fence.cpp b/compiler-rt/test/tsan/compare_exchange_acquire_fence.cpp
new file mode 100644
index 00000000000000..12bab80f14bfbb
--- /dev/null
+++ b/compiler-rt/test/tsan/compare_exchange_acquire_fence.cpp
@@ -0,0 +1,46 @@
+// RUN: %clangxx_tsan -O1 %s -o %t && %run %t 2>&1 | FileCheck %s
+// This is a correct program and tsan should not report a race.
+//
+// Verify that there is a happens-before relationship between a
+// memory_order_release store that happens as part of a successful
+// compare_exchange_strong(), and an atomic_thread_fence(memory_order_acquire)
+// that happens after a relaxed load.
+
+#include <atomic>
+#include <sanitizer/tsan_interface.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <thread>
+
+std::atomic<bool> a;
+unsigned int b;
+constexpr int loops = 100000;
+
+void Thread1() {
+  for (int i = 0; i < loops; ++i) {
+    while (a.load(std::memory_order_acquire)) {
+    }
+    b = i;
+    bool expected = false;
+    a.compare_exchange_strong(expected, true, std::memory_order_acq_rel);
+  }
+}
+
+int main() {
+  std::thread t(Thread1);
+  unsigned int sum = 0;
+  for (int i = 0; i < loops; ++i) {
+    while (!a.load(std::memory_order_relaxed)) {
+    }
+    std::atomic_thread_fence(std::memory_order_acquire);
+    __tsan_acquire(&a);
+    sum += b;
+    a.store(false, std::memory_order_release);
+  }
+  t.join();
+  fprintf(stderr, "DONE: %u\n", sum);
+  return 0;
+}
+
+// CHECK: DONE
+// CHECK-NOT: ThreadSanitizer: data race

``````````

</details>


https://github.com/llvm/llvm-project/pull/84923