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

[Bruno Cardoso Lopes via Phabricator via llvm-commits]

bruno 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);
----------------
dvyukov wrote:
> 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
Sounds good, minor correction on the fact that fmo cannot be release.


================
Comment at: compiler-rt/lib/tsan/rtl/tsan_interface_atomic.cpp:438
+  // those are used.
+  if (IsAcquireOrder(fmo) && s) {
+    AcquireImpl(thr, pc, &s->clock);
----------------
dvyukov wrote:
> 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.
Right, will fix.


================
Comment at: compiler-rt/lib/tsan/rtl/tsan_interface_atomic.cpp:440
+    AcquireImpl(thr, pc, &s->clock);
+    s->mtx.ReadUnlock();
+  }
----------------
dvyukov wrote:
> 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.
Yea, somehow it slipped, thanks!


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

https://reviews.llvm.org/D99434