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

[Bruno Cardoso Lopes via Phabricator via llvm-commits]

bruno marked 2 inline comments as done.
bruno added inline comments.


================
Comment at: compiler-rt/lib/tsan/rtl/tsan_interface_atomic.cpp:434
+  CHECK(IsLoadOrder(fmo));
+  if (fmo != mo_acquire)
+    return false;
----------------
dvyukov wrote:
> Can't fmo be consume/acq_rel/seq_cst? If yes, please add a test.
Added several variations. The consume failing order currently fallbacks to monotonic in LLVM, just opened https://reviews.llvm.org/D101501 to be consistent with success and fallback to acquire instead, without this change it leads to false positive, thanks for bringing this up.


================
Comment at: compiler-rt/lib/tsan/rtl/tsan_interface_atomic.cpp:443
+    sf->mtx.ReadUnlock();
+    MemoryReadAtomic(thr, pc, (uptr)a, SizeLog<T>());
+  }
----------------
dvyukov wrote:
> 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.
Looks like I overthought the approach, thanks for pointing out.


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

https://reviews.llvm.org/D99434