[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:436
   *c = pr;
+  CHECK(IsLoadOrder(fmo));
+  if (!s)
----------------
Better to do in the beginning of the function.


================
Comment at: compiler-rt/lib/tsan/rtl/tsan_interface_atomic.cpp:443
+  // when those are used.
+  if (IsAcquireOrder(fmo))
+    AcquireImpl(thr, pc, &s->clock);
----------------
With the current code it makes sense to do "IsAcquireOrder(fmo) && !IsAcquireOrder(mo)", because otherwise we already acquired.
But even better what's discussed above: first evaluate CAS, then decide what order to use.


================
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);
----------------
bruno wrote:
> 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.
Do I wait for this in this change? I see we still acquire/release before evaluating CAS. Or you want to do it later with other improvements you mentioned?


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

https://reviews.llvm.org/D99434