| Message ID | 20121124081119.GH2315@tucnak.redhat.com |
|---|---|
| State | New |
| Headers | show |
On Sat, Nov 24, 2012 at 12:11 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Fri, Nov 23, 2012 at 08:10:39PM +0400, Dmitry Vyukov wrote: >> I've just committed a patch to llvm with failure_memory_order (r168518). > > Shouldn't it be something like this instead? > > --- tsan_interface_atomic.cc.jj 2012-11-23 17:20:49.000000000 +0100 > +++ tsan_interface_atomic.cc 2012-11-23 19:06:05.917147568 +0100 > @@ -199,15 +199,17 @@ static T AtomicFetchXor(ThreadState *thr > template<typename T> > static bool AtomicCAS(ThreadState *thr, uptr pc, > volatile T *a, T *c, T v, morder mo, morder fmo) { > - (void)fmo; > if (IsReleaseOrder(mo)) > Release(thr, pc, (uptr)a); > T cc = *c; > T pr = __sync_val_compare_and_swap(a, cc, v); > - if (IsAcquireOrder(mo)) > - Acquire(thr, pc, (uptr)a); > - if (pr == cc) > + if (pr == cc) { > + if (IsAcquireOrder(mo)) > + Acquire(thr, pc, (uptr)a); > return true; > + } > + if (IsAcquireOrder(fmo)) > + Acquire(thr, pc, (uptr)a); > *c = pr; > return false; > } > > The failure model is for when the CAS fails, success model for when it > succeeds. Or perhaps the IsReleaseOrder/Release lines should be also after > the __sync_val_compare_and_swap (that doesn't matter, right, one thing is > the actual accesses, another thing is tsan events?) and also depend on the > corresponding model? You are right. But llvm does not pass failure_mo now, so I can't actually use it right now. Moreover, the logic before I added fail_mo was incorrect as well. Precise modeling of C++11 MM is impossible anyway. We've found that we have now is enough for all real programs we verify (millions of LOC). ThreadSanitizer is not intended for verification of tricky synchronization algorithms (it can't be done this way anyway). So the plan is to have modestly precise model (at least support basic acquire-release sync) and them improve it as we see real-world failures. However, it's great that gcc will pass it, because such changes that involve 2 compilers and runtime are pain. I must admit that my model of work is based on the fact that I was able to continuously improve and redeploy llvm-based tsan as needed outside of any release cycles. If we are talking about shipping tsan as part of gcc4.8, then perhaps we need to revisit it, and prepare some kind of list of things that must be done before release (there are still tons of work on runtime side).
--- tsan_interface_atomic.cc.jj 2012-11-23 17:20:49.000000000 +0100 +++ tsan_interface_atomic.cc 2012-11-23 19:06:05.917147568 +0100 @@ -199,15 +199,17 @@ static T AtomicFetchXor(ThreadState *thr template<typename T> static bool AtomicCAS(ThreadState *thr, uptr pc, volatile T *a, T *c, T v, morder mo, morder fmo) { - (void)fmo; if (IsReleaseOrder(mo)) Release(thr, pc, (uptr)a); T cc = *c; T pr = __sync_val_compare_and_swap(a, cc, v); - if (IsAcquireOrder(mo)) - Acquire(thr, pc, (uptr)a); - if (pr == cc) + if (pr == cc) { + if (IsAcquireOrder(mo)) + Acquire(thr, pc, (uptr)a); return true; + } + if (IsAcquireOrder(fmo)) + Acquire(thr, pc, (uptr)a); *c = pr; return false; }
On Fri, Nov 23, 2012 at 08:10:39PM +0400, Dmitry Vyukov wrote: > I've just committed a patch to llvm with failure_memory_order (r168518). Shouldn't it be something like this instead? The failure model is for when the CAS fails, success model for when it succeeds. Or perhaps the IsReleaseOrder/Release lines should be also after the __sync_val_compare_and_swap (that doesn't matter, right, one thing is the actual accesses, another thing is tsan events?) and also depend on the corresponding model? Jakub