Fix race in the implementation of __tsan_acquire() #84923
Conversation
…lse positive data race warnings when the atomic variable was previously written to using compare_exchange_strong/weak.
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
compiler-rt
compiler-rt:tsan
|
@llvm/pr-subscribers-compiler-rt-sanitizer Author: Dave Clausen (daveclausen) Changes
However, if the atomic variable was recently written to by a Specifically, if you have code that writes to an atomic variable using This change eliminates the unsafe optimization in Full diff: https://github.com/llvm/llvm-project/pull/84923.diff 2 Files Affected:
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
|
| @@ -0,0 +1,46 @@ | |||
| // RUN: %clangxx_tsan -O1 %s -o %t && %run %t 2>&1 | FileCheck %s | |||
There was a problem hiding this comment.
Exit code 0 should be enough
we probably do not need | FileCheck here
There was a problem hiding this comment.
Done.
I also removed the CHECK and CHECK-NOT comment lines at the end of the file.
There was a problem hiding this comment.
It doesn't show up in the list of slowest tests:
$ LIT_OPTS="--time-tests" ninja check-tsan
...
Slowest Tests:
--------------------------------------------------------------------------
45.32s: ThreadSanitizer-x86_64 :: restore_stack.cpp
33.56s: ThreadSanitizer-x86_64 :: Linux/check_memcpy.c
21.56s: ThreadSanitizer-x86_64 :: bench_threads.cpp
14.70s: ThreadSanitizer-x86_64 :: deadlock_detector_stress_test.cpp
14.65s: ThreadSanitizer-x86_64 :: signal_thread.cpp
12.60s: ThreadSanitizer-x86_64 :: stress.cpp
10.81s: ThreadSanitizer-x86_64 :: Linux/fork_multithreaded4.cpp
9.79s: ThreadSanitizer-x86_64 :: force_background_thread.cpp
9.53s: ThreadSanitizer-x86_64 :: lots_of_threads.c
7.14s: ThreadSanitizer-x86_64 :: deep_stack1.cpp
5.73s: ThreadSanitizer-x86_64 :: fd_close_norace3.cpp
5.40s: ThreadSanitizer-x86_64 :: signal_reset.cpp
5.19s: ThreadSanitizer-x86_64 :: mmap_stress.cpp
4.84s: ThreadSanitizer-x86_64 :: signal_sync2.cpp
4.48s: ThreadSanitizer-x86_64 :: global_race.cpp
4.41s: ThreadSanitizer-x86_64 :: ignore_lib6.cpp
3.65s: ThreadSanitizer-x86_64 :: mmap_stress2.cpp
3.51s: ThreadSanitizer-x86_64 :: must_deadlock.cpp
3.51s: ThreadSanitizer-x86_64 :: simple_stack.c
3.50s: ThreadSanitizer-x86_64 :: setuid2.c
Tests Times:
--------------------------------------------------------------------------
[ Range ] :: [ Percentage ] :: [ Count ]
--------------------------------------------------------------------------
[44.0s,46.0s) :: [ ] :: [ 1/409]
[42.0s,44.0s) :: [ ] :: [ 0/409]
[40.0s,42.0s) :: [ ] :: [ 0/409]
[38.0s,40.0s) :: [ ] :: [ 0/409]
[36.0s,38.0s) :: [ ] :: [ 0/409]
[34.0s,36.0s) :: [ ] :: [ 1/409]
[32.0s,34.0s) :: [ ] :: [ 0/409]
[30.0s,32.0s) :: [ ] :: [ 0/409]
[28.0s,30.0s) :: [ ] :: [ 0/409]
[26.0s,28.0s) :: [ ] :: [ 0/409]
[24.0s,26.0s) :: [ ] :: [ 0/409]
[22.0s,24.0s) :: [ ] :: [ 0/409]
[20.0s,22.0s) :: [ ] :: [ 1/409]
[18.0s,20.0s) :: [ ] :: [ 0/409]
[16.0s,18.0s) :: [ ] :: [ 0/409]
[14.0s,16.0s) :: [ ] :: [ 2/409]
[12.0s,14.0s) :: [ ] :: [ 1/409]
[10.0s,12.0s) :: [ ] :: [ 1/409]
[ 8.0s,10.0s) :: [ ] :: [ 2/409]
[ 6.0s, 8.0s) :: [ ] :: [ 1/409]
[ 4.0s, 6.0s) :: [ ] :: [ 6/409]
[ 2.0s, 4.0s) :: [* ] :: [ 16/409]
[ 0.0s, 2.0s) :: [************************************ ] :: [377/409]
--------------------------------------------------------------------------
Testing Time: 45.37s
Total Discovered Tests: 451
Unsupported : 88 (19.51%)
Passed : 362 (80.27%)
Expectedly Failed: 1 (0.22%)
|
Merging having that @dvyukov LGTMed code in internal conversation. |
|
@daveclausen Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested Please check whether problems have been caused by your change specifically, as How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
|
It needs %link_libcxx_tsan, like in llvm-project/compiler-rt/test/tsan/compare_exchange.cpp |
__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/strongon 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 callingAcquire()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 anatomic_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.