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

[Dmitry Vyukov via Phabricator via llvm-commits]

dvyukov accepted this revision.
dvyukov added a comment.

I don't see any issues now. Thanks for bearing with me.



================
Comment at: compiler-rt/lib/tsan/rtl/tsan_interface_atomic.cpp:437
+
+    if (success && write_lock)
       s->mtx.Unlock();
----------------
"success &&" is unnecessary here right?
I see it's not harmful, but it also makes me a but nervous because we used just the "write_lock" condition when locking, but we use a different one when unlocking. This is not symmetric and have some potential to get out of sync, so I would drop "success &&" part here.



================
Comment at: compiler-rt/lib/tsan/rtl/tsan_interface_atomic.cpp:453
+    // to use load style memory order.
+    s->mtx.ReadUnlock();
+  }
----------------
bruno wrote:
> 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. 
I think you are right. This makes sense.


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

https://reviews.llvm.org/D99434