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

[Bruno Cardoso Lopes via Phabricator via llvm-commits]

bruno 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.
----------------
dvyukov wrote:
> 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.
> If I am not missing something, I think this comment still holds:
> 
> > Can it be seq_cst?

Yes, I also added tests to cover that.

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

See my next comment.


================
Comment at: compiler-rt/lib/tsan/rtl/tsan_interface_atomic.cpp:453
+    // to use load style memory order.
+    s->mtx.ReadUnlock();
+  }
----------------
dvyukov wrote:
> 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;
fmo cannot be `mo_release` or `mo_acq_rel`, and when it's `mo_seq_cst` my understanding (which could be wrong) is that it has load semantics. @rjmccall @jfb does my understanding makes sense?

With that in mind, `write_lock` only needs to track `IsReleaseOrder(mo)`, which is what the current patch does. 


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

https://reviews.llvm.org/D99434