Skip to content

Commit 47625e4

Browse files
authored
Fix race in the implementation of __tsan_acquire() (#84923)
`__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.
1 parent 88bf640 commit 47625e4

File tree

2 files changed

+44
-1
lines changed

2 files changed

+44
-1
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -446,9 +446,9 @@ void Acquire(ThreadState *thr, uptr pc, uptr addr) {
446446
if (!s)
447447
return;
448448
SlotLocker locker(thr);
449+
ReadLock lock(&s->mtx);
449450
if (!s->clock)
450451
return;
451-
ReadLock lock(&s->mtx);
452452
thr->clock.Acquire(s->clock);
453453
}
454454

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
// RUN: %clangxx_tsan -O1 %s -o %t && %run %t 2>&1
2+
// This is a correct program and tsan should not report a race.
3+
//
4+
// Verify that there is a happens-before relationship between a
5+
// memory_order_release store that happens as part of a successful
6+
// compare_exchange_strong(), and an atomic_thread_fence(memory_order_acquire)
7+
// that happens after a relaxed load.
8+
9+
#include <atomic>
10+
#include <sanitizer/tsan_interface.h>
11+
#include <stdbool.h>
12+
#include <stdio.h>
13+
#include <thread>
14+
15+
std::atomic<bool> a;
16+
unsigned int b;
17+
constexpr int loops = 100000;
18+
19+
void Thread1() {
20+
for (int i = 0; i < loops; ++i) {
21+
while (a.load(std::memory_order_acquire)) {
22+
}
23+
b = i;
24+
bool expected = false;
25+
a.compare_exchange_strong(expected, true, std::memory_order_acq_rel);
26+
}
27+
}
28+
29+
int main() {
30+
std::thread t(Thread1);
31+
unsigned int sum = 0;
32+
for (int i = 0; i < loops; ++i) {
33+
while (!a.load(std::memory_order_relaxed)) {
34+
}
35+
std::atomic_thread_fence(std::memory_order_acquire);
36+
__tsan_acquire(&a);
37+
sum += b;
38+
a.store(false, std::memory_order_release);
39+
}
40+
t.join();
41+
fprintf(stderr, "DONE: %u\n", sum);
42+
return 0;
43+
}

0 commit comments

Comments
 (0)