[PATCH] D99434: [TSAN] Honor failure memory orders in AtomicCAS

[Dmitry Vyukov via Phabricator via llvm-commits]

dvyukov added inline comments.


================
Comment at: compiler-rt/lib/tsan/rtl/tsan_interface_atomic.cpp:406
+  // 31.7.2.18: "The failure argument shall not be memory_order_release
+  // nor memory_order_acq_rel". LLVM (2021-05) fallbacks to Monotonic (relaxed)
+  // when those are used.
----------------
If I am not missing something, I think this comment still holds:

> Can it be seq_cst?
> If yes, then I think write_lock below is incorrect and checking only IsAcquireOrder(fmo) is incorrect as well.


================
Comment at: compiler-rt/lib/tsan/rtl/tsan_interface_atomic.cpp:453
+    // to use load style memory order.
+    s->mtx.ReadUnlock();
+  }
----------------
There is some duplication here between handling of success case and failure case: adding to trace, acquire, unlock. And taking into account that fmo can be seq_cst, we will need even more duplication to fix this, e.g. we may need to do Release and WriteUnlock here.
I would structure it along the following lines (it should both handle all cases and avoid duplication at the same time):

  SyncVar *s = 0;
  bool write_lock = IsReleaseOrder(mo) || IsReleaseOrder(fmo);
  if (mo != mo_relaxed || fmo != mo_relaxed)
    s = ctx->metamap.GetOrCreateAndLock(thr, pc, (uptr)a, write_lock);

  T cc = *c;
  T pr = func_cas(a, cc, v);
  bool success = pr == cc;
  if (!success) {
    *c = pr;
    mo = fmo;
  }
  if (s) {
    thr->fast_state.IncrementEpoch();
    // Can't increment epoch w/o writing to the trace as well.
    TraceAddEvent(thr, thr->fast_state, EventTypeMop, 0);
    if (IsAcqRelOrder(mo))
      AcquireReleaseImpl(thr, pc, &s->clock);
    else if (IsReleaseOrder(mo))
      ReleaseImpl(thr, pc, &s->clock);
    else if (IsAcquireOrder(mo))
      AcquireImpl(thr, pc, &s->clock);

    if (write_lock)
      s->mtx.Unlock();
    else
      s->mtx.ReadUnlock();
  }
  return success;


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

https://reviews.llvm.org/D99434