Patchwork [tsan] Instrument atomics

login
register
mail settings
Submitter Jakub Jelinek
Date Nov. 24, 2012, 8:11 a.m.
Message ID <20121124081119.GH2315@tucnak.redhat.com>
Download mbox | patch
Permalink /patch/201443/
State New
Headers show

Comments

Jakub Jelinek - Nov. 24, 2012, 8:11 a.m.
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
Dmitriy Vyukov - Nov. 24, 2012, 8:55 a.m.
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).

Patch

--- 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;
 }