[PATCH] D99434: [TSAN] Honor acquire failure mode on AtomicCAS

[Dmitry Vyukov via Phabricator via llvm-commits]

dvyukov added inline comments.


================
Comment at: compiler-rt/lib/tsan/rtl/tsan_interface_atomic.cpp:434
+  CHECK(IsLoadOrder(fmo));
+  if (fmo != mo_acquire)
+    return false;
----------------
Can't fmo be consume/acq_rel/seq_cst? If yes, please add a test.


================
Comment at: compiler-rt/lib/tsan/rtl/tsan_interface_atomic.cpp:436
+    return false;
+  SyncVar *sf = ctx->metamap.GetIfExistsAndLock((uptr)a, false);
+  if (sf) {
----------------
I think we should not search the object and re-acquire the mutex second time for performance reasons and complexity (won't need to re-read the value).
Note that we may have already acquired due to mo. We should not acquire second time in that case.


================
Comment at: compiler-rt/lib/tsan/rtl/tsan_interface_atomic.cpp:443
+    sf->mtx.ReadUnlock();
+    MemoryReadAtomic(thr, pc, (uptr)a, SizeLog<T>());
+  }
----------------
I don't think this is correct. Can't this lead to false positives?
Consider that a thread does CAS-release to hand off the object to another thread, and that thread frees the object. I think this memory access can race with the free. It's generally not OK to touch memory after atomic operations.
If if leads to a false positive, please add a test that catches it.


================
Comment at: compiler-rt/test/tsan/compare_exchange_release_relaxed.cpp:18
+  while (expected == nullptr) {
+    _node.compare_exchange_weak(expected, nullptr, std::memory_order_release,
+                                std::memory_order_relaxed);
----------------
If this test differs only by memory order, it can make sense to use parametrized tests. It would also be good to test other mo combinations (e.g. that the CHECK(IsLoadOrder(fmo)) does not fail).
See e.g. test/tsan/ignore_lib0.cpp for an example.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99434/new/

https://reviews.llvm.org/D99434