[PATCH] D99434: [TSAN] Honor acquire failure mode on AtomicCAS
Dmitry Vyukov via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Apr 28 23:38:23 PDT 2021
dvyukov 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);
----------------
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
================
Comment at: compiler-rt/lib/tsan/rtl/tsan_interface_atomic.cpp:438
+ // those are used.
+ if (IsAcquireOrder(fmo) && s) {
+ AcquireImpl(thr, pc, &s->clock);
----------------
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.
================
Comment at: compiler-rt/lib/tsan/rtl/tsan_interface_atomic.cpp:440
+ AcquireImpl(thr, pc, &s->clock);
+ s->mtx.ReadUnlock();
+ }
----------------
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.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D99434/new/
https://reviews.llvm.org/D99434
More information about the llvm-commits
mailing list