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

Dmitry Vyukov via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Apr 28 23:38:23 PDT 2021


dvyukov added inline comments.


================
Comment at: compiler-rt/lib/tsan/rtl/tsan_interface_atomic.cpp:413
     TraceAddEvent(thr, thr->fast_state, EventTypeMop, 0);
     if (IsAcqRelOrder(mo))
       AcquireReleaseImpl(thr, pc, &s->clock);
----------------
I think it's better to avoid doing this if the CAS will fail and fmo == mo_relaxed. It will provide more precise race detection.
I think we could do something along the following lines:
 - if either mo or fmo != relaxed, do GetOrCreateAndLock
 - if either mo or fmo involves release, do a write lock
 - evaluate cas
 - respect mo or fmo based on the cas result


================
Comment at: compiler-rt/lib/tsan/rtl/tsan_interface_atomic.cpp:438
+  // those are used.
+  if (IsAcquireOrder(fmo) && s) {
+    AcquireImpl(thr, pc, &s->clock);
----------------
We did not create/obtain the sync object if mo == mo_relaxed. Is mo == mo_relaxed and fmo == mo_acquire possible? If yes, then we will fail to respect fmo.


================
Comment at: compiler-rt/lib/tsan/rtl/tsan_interface_atomic.cpp:440
+    AcquireImpl(thr, pc, &s->clock);
+    s->mtx.ReadUnlock();
+  }
----------------
We used a different condition to decide if we need to do read lock. Can't this deadlock? I think we need to check write_lock.


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

https://reviews.llvm.org/D99434



More information about the llvm-commits mailing list