Re: [PATCH] Fix sporadic failure in g++.dg/tsan/aligned_vs_unaligned_race.C

[Mike Stump <mikestump at comcast dot net>]

On Jan 5, 2015, at 12:58 PM, Mike Stump <mikestump@comcast.net> wrote:
> So, to help you out, I tried my hand at wiring up the extra library code:

So, my tsan build finally finished, and I could try this with a real test case.  I ran it 20 times, and got 11 fails with:

$ i=20; while let i--; do make RUNTESTFLAGS=tsan.exp=aligned_vs_unaligned_race.C check-g++ | grep FAIL; done
FAIL: g++.dg/tsan/aligned_vs_unaligned_race.C   -O2  execution test
FAIL: g++.dg/tsan/aligned_vs_unaligned_race.C   -O0  execution test
FAIL: g++.dg/tsan/aligned_vs_unaligned_race.C   -O2  execution test
FAIL: g++.dg/tsan/aligned_vs_unaligned_race.C   -O2  execution test
FAIL: g++.dg/tsan/aligned_vs_unaligned_race.C   -O2  execution test
FAIL: g++.dg/tsan/aligned_vs_unaligned_race.C   -O0  execution test
FAIL: g++.dg/tsan/aligned_vs_unaligned_race.C   -O0  execution test
FAIL: g++.dg/tsan/aligned_vs_unaligned_race.C   -O2  execution test
FAIL: g++.dg/tsan/aligned_vs_unaligned_race.C   -O2  execution test
FAIL: g++.dg/tsan/aligned_vs_unaligned_race.C   -O2  execution test
FAIL: g++.dg/tsan/aligned_vs_unaligned_race.C   -O0  execution test
$ 

When I fixed the problem, I ran it 20 times:

$ i=20; while let i--; do make RUNTESTFLAGS=tsan.exp=aligned_vs_unaligned_race.C check-g++ | grep FAIL; done
$ 

and got 0 failures.

So, it seems to work.  I’d like a tsan person to review it and we can go from there.

The only thing I would add to it would be a huge comment that explains that the tsan run time isn’t thread safe and they should use a lock free update to the shadow bits, but, they don’t.  We introduce the step primitive to work around that bug.  Ideally, the the problem should be filed into a bug database for the tsan code gen and when closed as not to be fixed, we can then change the word bug to design, but leave the bug reference so that others that want to completely understand the issue can go read up on it.  If they actually fix the codegen to be thread safe, then we can simply remove all the step code.

To make this clang friendly, if one turns off inlining and obscures the semantics with weak from the optimizer and puts it into a header files and then #includes that header files, I think it would work.  I’ll leave this to someone that might want to do that.  If not, I’m fine with #ifdef clang/gcc and have the gcc test cases use step and the clang test cases, well, be unreliable.

$ svn diff
Index: g++.dg/tsan/aligned_vs_unaligned_race.C
===================================================================
--- g++.dg/tsan/aligned_vs_unaligned_race.C     (revision 219198)
+++ g++.dg/tsan/aligned_vs_unaligned_race.C     (working copy)
@@ -1,11 +1,17 @@
+/* { dg-additional-sources "lib.c" } */
 /* { dg-shouldfail "tsan" } */
+
 #include <pthread.h>
 #include <stdio.h>
 #include <stdint.h>
+#include <unistd.h>
+
+void step(int);
 
 uint64_t Global[2];
 
 void *Thread1(void *x) {
+  step (2);
   Global[1]++;
   return NULL;
 }
@@ -15,6 +21,7 @@ void *Thread2(void *x) {
   struct __attribute__((packed, aligned(1))) u_uint64_t { uint64_t val; };
   u_uint64_t *p4 = reinterpret_cast<u_uint64_t *>(p1 + 1);
   (*p4).val++;
+  step (1);
   return NULL;
 }
 
$ cat g++.dg/tsan/lib.c 
/* { dg-do compile } */
#include <unistd.h>

volatile int serial;

__attribute__((no_sanitize_address))
void step(int i) {
  while (serial != i-1) ;
  while (1) {
    if (++serial == i)
      return;
    --serial;
    sleep (1);
  }
}