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

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

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 400 bytes --]

Hi,


the test case g++.dg/tsan/aligned_vs_unaligned_race.C
still fails sporadically (around 1 out of 100 times).
That is apparently a race condition in the tsan runtime itself.
To make the test reproducible pass, I need to add a sleep(1)
in one of the two threads.


Tested really often with:
make check-gcc-c++ RUNTESTFLAGS="tsan.exp=*"

OK for trunk?


Thanks
Bernd.
 		 	   		  

[-- Attachment #2: patch-tsan-race.diff --]
[-- Type: application/octet-stream, Size: 610 bytes --]

2015-01-03  Bernd Edlinger  <bernd.edlinger@hotmail.de>

        * g++.dg/tsan/aligned_vs_unaligned_race.C: Fixed race condition.

Index: gcc/testsuite/g++.dg/tsan/aligned_vs_unaligned_race.C
===================================================================
--- gcc/testsuite/g++.dg/tsan/aligned_vs_unaligned_race.C	(revision 219154)
+++ gcc/testsuite/g++.dg/tsan/aligned_vs_unaligned_race.C	(working copy)
@@ -2,10 +2,12 @@
 #include <pthread.h>
 #include <stdio.h>
 #include <stdint.h>
+#include <unistd.h>
 
 uint64_t Global[2];
 
 void *Thread1(void *x) {
+  sleep(1);
   Global[1]++;
   return NULL;
 }

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

[Mike Stump]

On Jan 3, 2015, at 1:01 AM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote:
> the test case g++.dg/tsan/aligned_vs_unaligned_race.C
> still fails sporadically (around 1 out of 100 times).
> That is apparently a race condition in the tsan runtime itself.
> To make the test reproducible pass, I need to add a sleep(1)
> in one of the two threads.
> 
> 
> Tested really often with:
> make check-gcc-c++ RUNTESTFLAGS="tsan.exp=*"
> 
> OK for trunk?

No.  sleep can never fix race conditions.  The only time sleep can fix a race condition would be in a hard real time system, and, that in general doesn’t apply to anything in the gcc test suite.

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

[Bernd Edlinger]

On Sat, 3 Jan 2015 01:51:34, Mike Strump wrote:
>
> On Jan 3, 2015, at 1:01 AM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote:
>> the test case g++.dg/tsan/aligned_vs_unaligned_race.C
>> still fails sporadically (around 1 out of 100 times).
>> That is apparently a race condition in the tsan runtime itself.
>> To make the test reproducible pass, I need to add a sleep(1)
>> in one of the two threads.
>>
>>
>> Tested really often with:
>> make check-gcc-c++ RUNTESTFLAGS="tsan.exp=*"
>>
>> OK for trunk?
>
> No. sleep can never fix race conditions. The only time sleep can fix a race condition would be in a hard real time system, and, that in general doesn’t apply to anything in the gcc test suite.

Yes, but all other tsan test cases call sleep(1) too.
The sleep does not really "fix" the race condition but it changes
the likelihood from 1 / 100 to 1 / 10^20.

That is just to avoid noise from sporadic test failures.


Bernd.
 		 	   		  

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

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 1038 bytes --]

Aehm, sorry,

that's the sporadic failure, I mentioned:

https://gcc.gnu.org/ml/gcc-regression/2015-01/msg00041.html
New failures:
FAIL: g++.dg/tsan/aligned_vs_unaligned_race.C   -O2  execution test

New passes:

https://gcc.gnu.org/ml/gcc-regression/2015-01/msg00044.html
New failures:

New passes:
FAIL: g++.dg/tsan/aligned_vs_unaligned_race.C   -O2  execution test

Really I did run this test often, before I checked it in, but...

It is due to a race condition in tsan itself, it cannot decide which access was the
previous one and which was the second one, but our tsan tests are not meant as
a functional test of the tsan runtime library, they are only meant to test the GCC
instrumentation.

For the purpose of finding race conditions in an application a detection likelihood of 98%
is absolutely perfect, just for our test suite that causes unnecessary failures.


So, I still think I should fix that test case, maybe with a comment, why I have to
sleep(1), what do you think?


Bernd.
 		 	   		  

[-- Attachment #2: patch-tsan-race.diff --]
[-- Type: application/octet-stream, Size: 718 bytes --]

2015-01-03  Bernd Edlinger  <bernd.edlinger@hotmail.de>

        * g++.dg/tsan/aligned_vs_unaligned_race.C: Fixed sporadic failure.


Index: gcc/testsuite/g++.dg/tsan/aligned_vs_unaligned_race.C
===================================================================
--- gcc/testsuite/g++.dg/tsan/aligned_vs_unaligned_race.C	(revision 219160)
+++ gcc/testsuite/g++.dg/tsan/aligned_vs_unaligned_race.C	(working copy)
@@ -2,10 +2,14 @@
 #include <pthread.h>
 #include <stdio.h>
 #include <stdint.h>
+#include <unistd.h>
 
 uint64_t Global[2];
 
 void *Thread1(void *x) {
+  /* We have to sleep here, to make it somewhat easier for tsan to
+     detect the race condition.  */
+  sleep(1);
   Global[1]++;
   return NULL;
 }

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

[Mike Stump]

On Jan 4, 2015, at 9:00 AM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote:
> It is due to a race condition in tsan itself, it cannot decide which access was the
> previous one and which was the second one, but our tsan tests are not meant as
> a functional test of the tsan runtime library, they are only meant to test the GCC
> instrumentation.

Well, at least one test case that _is_ a functional test of the tsan runtime library isn’t a bad idea.

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

[Jakub Jelinek]

On Sun, Jan 04, 2015 at 11:07:31AM -0800, Mike Stump wrote:
> On Jan 4, 2015, at 9:00 AM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote:
> > It is due to a race condition in tsan itself, it cannot decide which access was the
> > previous one and which was the second one, but our tsan tests are not meant as
> > a functional test of the tsan runtime library, they are only meant to test the GCC
> > instrumentation.
> 
> Well, at least one test case that _is_ a functional test of the tsan runtime library isn’t a bad idea.

The GCC instrumentation can be tested just by scanning the *.optimized dumps
or assembly.  And perhaps the runtime test just be hidden by some special
environment variable that the user acks he doesn't main spurious FAILs.

	Jakub

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

[Bernd Edlinger]

Hi,

On Sun, 4 Jan 2015 20:16:58, Jakub Jelinek wrote:
>
> On Sun, Jan 04, 2015 at 11:07:31AM -0800, Mike Stump wrote:
>> On Jan 4, 2015, at 9:00 AM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote:
>>> It is due to a race condition in tsan itself, it cannot decide which access was the
>>> previous one and which was the second one, but our tsan tests are not meant as
>>> a functional test of the tsan runtime library, they are only meant to test the GCC
>>> instrumentation.
>>
>> Well, at least one test case that _is_ a functional test of the tsan runtime library isn’t a bad idea.

There are of course numerous functional tests, but they are all in LLVM's tree 
at /compiler-rt/trunk/test/tsan

>
> The GCC instrumentation can be tested just by scanning the *.optimized dumps
> or assembly. And perhaps the runtime test just be hidden by some special
> environment variable that the user acks he doesn't main spurious FAILs.
>

I never had suspected the sleep calls to play such an important role, If I did, I would
have smuggled one in before checking in....

That's surely due to that there was no comment anywhere mentioning that race
condition.  I wonder if there is a test for that in the LLVM tree?  Probably they
wouldn't consider that a BUG.  I've looked deep in the implementation and I
saw, for every 8-byte word, there are 4 shadow words, each containing previous
accesses with call stack, and the __tsan_write8 functions just dont lock
a mutex because of performance reasons, if the application does not have
a race conditions tsan does not have a race either, if the application has a
race condition, there is a certain chance that both threads call __tsan_write8,
both look at the same shadow word, see nothing, and both write their
callstack in the same shadow cell. But tsan still gets at least 90% chance
to spot that.  As a matter of fact, the tsan runtime is just _incredibly_ fast,
and catches errors, with a very high reliability.  But it is racy by design.


Bernd.


> Jakub


 		 	   		  

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

[Mike Stump]

On Jan 4, 2015, at 11:44 AM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote:
> On Sun, 4 Jan 2015 20:16:58, Jakub Jelinek wrote:
>> 
>> On Sun, Jan 04, 2015 at 11:07:31AM -0800, Mike Stump wrote:
>>> On Jan 4, 2015, at 9:00 AM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote:
>>>> It is due to a race condition in tsan itself, it cannot decide which access was the
>>>> previous one and which was the second one, but our tsan tests are not meant as
>>>> a functional test of the tsan runtime library, they are only meant to test the GCC
>>>> instrumentation.
>>> 
>>> Well, at least one test case that _is_ a functional test of the tsan runtime library isn’t a bad idea.
> 
> There are of course numerous functional tests, but they are all in LLVM's tree 
> at /compiler-rt/trunk/test/tsan
> 
>> 
>> The GCC instrumentation can be tested just by scanning the *.optimized dumps
>> or assembly. And perhaps the runtime test just be hidden by some special
>> environment variable that the user acks he doesn't main spurious FAILs.
>> 
> 
> I never had suspected the sleep calls to play such an important role, If I did, I would
> have smuggled one in before checking in….

We prefer a nice designed where each element can stand on its own and is defensible if reviewed.  :-)

> That's surely due to that there was no comment anywhere mentioning that race
> condition.

A comment or a bug report someplace that describe the race would be nice.

> I wonder if there is a test for that in the LLVM tree?  Probably they
> wouldn't consider that a BUG.

I would not assume that.  If given the choice between 5% slower and 100% reliable, and 5% faster and 90% reliable, it may be reasonable to pick the 100% reliable option. Also, in the fullness of time, maybe the choice between the two should be exposed to the user.  With 1 user, no option is fine, with 10,000 users, maybe 100 of them would like the other choice.

> I've looked deep in the implementation and I
> saw, for every 8-byte word, there are 4 shadow words, each containing previous
> accesses with call stack, and the __tsan_write8 functions just dont lock
> a mutex because of performance reasons, if the application does not have
> a race conditions tsan does not have a race either, if the application has a
> race condition, there is a certain chance that both threads call __tsan_write8,
> both look at the same shadow word, see nothing, and both write their
> callstack in the same shadow cell.

So, this sounds like programming 101 type stuff.  I’d like to think that once could atomic increment it from 0 and notice if they were first, and if not, know reliably they were not first (and could fix the value by subtracting the value back out).  I don’t think this would hurt performance much, if any.  I’m assuming they only read towards the end, and at the end, they could ensure that all the code finished running (that the atomic subtract above would have finished).

> But tsan still gets at least 90% chance to spot that.  As a matter of fact, the tsan runtime is just _incredibly_ fast,
> and catches errors, with a very high reliability.  But it is racy by design.

You say by design.  I’m skeptical of that.  Can you quote the email or the code were they said I wanted to design a system that wasn’t 100% reliable, because the best they could do was 2% slower, and they didn’t want it to be 2% slower?  I tend to think someone didn’t figure out they could atomically, locklessly do the update, or they didn’t understand the performance hit on the lockless code was small, or worse, they had no clue it was racy.  I’d be curious to know.  Maybe, there are other complications that prevent it from being non-racy.

Using sleep goes back decades, and I think that style should have mostly died around 1987 or so.  It’s 2015, and I’d rather consider it an obsolete style, exclusive of hard real-time.  I bring it up, as I’ve been burned by sleep and people that think sleep is a nice solution.

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

[Bernd Edlinger]

Hi,

On Sun, 4 Jan 2015 14:18:59, Mike Stump wrote:
>
>> But tsan still gets at least 90% chance to spot that. As a matter of fact, the tsan runtime is just _incredibly_ fast,
>> and catches errors, with a very high reliability. But it is racy by design.
>
> You say by design. I’m skeptical of that. Can you quote the email or the code were they said I wanted to design a system that wasn’t 100% reliable, because the best they could do was 2% slower, and they didn’t want it to be 2% slower? I tend to think someone didn’t figure out they could atomically, locklessly do the update, or they didn’t understand the performance hit on the lockless code was small, or worse, they had no clue it was racy. I’d be curious to know. Maybe, there are other complications that prevent it from being non-racy.
>

see for instance this thread "Is TSan an exact tool?":
https://groups.google.com/forum/#!topic/thread-sanitizer/mB73m6Nltaw

When I say "by design" I did not mean to say something negative on the code quality.

It is an excellent design.  One of the design principles is speed.
We are able to use TSan to successfully test complex communication
protocols.  If it would slow down the execution more than absolutely necessary
it would be useless.

> Using sleep goes back decades, and I think that style should have mostly died around 1987 or so. It’s 2015, and I’d rather consider it an obsolete style, exclusive of hard real-time. I bring it up, as I’ve been burned by sleep and people that think sleep is a nice solution.

The sleep(1) is only in the positive test cases. That is obviously not the best possible solution.

... And it makes the test suite run at least 10 seconds longer... :-)

Nevertheless I would prefer to silence the test now with sleep(1),

And come back later with a follow-up patch to use a un-instrumented and
non-interceptable synchronization primitive in all tsan tests. As it is at least
not obvious how to do that in the test suite.  But it must be possible.


Bernd.
 		 	   		  

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

[Mike Stump]

On Jan 5, 2015, at 12:49 AM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote:
> On Sun, 4 Jan 2015 14:18:59, Mike Stump wrote:
>> 
>>> But tsan still gets at least 90% chance to spot that. As a matter of fact, the tsan runtime is just _incredibly_ fast,
>>> and catches errors, with a very high reliability. But it is racy by design.
>> 
>> You say by design. I’m skeptical of that. Can you quote the email or the code were they said I wanted to design a system that wasn’t 100% reliable, because the best they could do was 2% slower, and they didn’t want it to be 2% slower? I tend to think someone didn’t figure out they could atomically, locklessly do the update, or they didn’t understand the performance hit on the lockless code was small, or worse, they had no clue it was racy. I’d be curious to know. Maybe, there are other complications that prevent it from being non-racy.
> 
> see for instance this thread "Is TSan an exact tool?":
> https://groups.google.com/forum/#!topic/thread-sanitizer/mB73m6Nltaw

Yeah, that’s not the issue.  The issue is, in this one narrow case, was is possible to use a lockless method of updating the data so that the tool would be slightly better.  That post doesn’t address this question.  For example, if a patch were submitted to locklessly update so both can notice who is first, was it rejected and why?  Anyway, that’s a side issue, and we can safely ignore it.  I’m fine with making testing reliable with how it works today.

> Nevertheless I would prefer to silence the test now with sleep(1),

> And come back later with a follow-up patch to use a un-instrumented and
> non-interceptable synchronization primitive in all tsan tests. As it is at least
> not obvious how to do that in the test suite.  But it must be possible.

I thought the code was posted, I thought how to put it in a file was posted.  I think it is a 2 minute endeavor to put all those pieces together and run a test case?  You do know how to run just 1 tsan test case with make RUNTESTFLAGS=tsan.exp=aligned_vs_unaligned_race.C check-g++ sort of thing?  This can shorten the testing time from hours per iteration to seconds per iteration.  Slightly non-obvious, but very handy when developing some types of test cases.

So, to help you out, I tried my hand at wiring up the extra library code:

gcc/gcc/testsuite/gcc.dg/tsan$ cat test1.c
/* { dg-additional-options "-O2" } */
/* { dg-additional-sources "lib.c" } */

void sync(int);

int main(int argc, char**argv) {
  sync(1);
  sync(2);
  sync(3);
}
gcc/gcc/testsuite/gcc.dg/tsan$ cat lib.c
/* { dg-do compile } */
#include <stdlib.h>

volatile int serial;

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

and it ran just fine.  The options are for both files.  I read some of the tsan documentation, and they seem to claim you can have tsan not process a function by merely asking.  I did that.  I don’t know if that is enough to hide all the semantics that need to be hidden.

In the above code, the ++ and -- should be an atomic increment/decrement.

The idea is that can can introduce a deterministic ordering to the execution of the code by adding sync(n), where n is the step number, whose range starts at 1.

So, for example, here is the test case, with the bits added, so you can see how I transformed what would have been the sleep into the use of the synchronizing primitive from the library.

/* { dg-additional-sources "lib.c" } */
/* { dg-shouldfail "tsan" } */

#include <pthread.h>
#include <stdio.h>
#include <stdint.h>
#include <unistd.h>

void sync(int);

uint64_t Global[2];

void *Thread1(void *x) {
  sync (2);
  Global[1]++;
  return NULL;
}

void *Thread2(void *x) {
  char *p1 = reinterpret_cast<char *>(&Global[0]);
  struct __attribute__((packed, aligned(1))) u_uint64_t { uint64_t val; };
  u_uint64_t *p4 = reinterpret_cast<u_uint64_t *>(p1 + 1);
  (*p4).val++;
  sync (1);
  return NULL;
}

int main() {
  pthread_t t[2];
  pthread_create(&t[0], NULL, Thread1, NULL);
  pthread_create(&t[1], NULL, Thread2, NULL);
  pthread_join(t[0], NULL);
  pthread_join(t[1], NULL);
  printf("Pass\n");
  /* { dg-output "WARNING: ThreadSanitizer: data race.*(\n|\r\n|\r)" } */
  /* { dg-output "Pass.*" } */
  return 0;
}

The question is, does this 100% reliably solve the problem?

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

[Mike Stump]

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

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

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 5165 bytes --]

Hi Mike,


On Mon, 5 Jan 2015 14:01:42, Mike Stump wrote:
>
> 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);
> }
> }


I tried to elaborate your idea a bit, and used proper atomics.
It is necessary that the logic of the step function is completely invisible to tsan.
Therefore it should not call sleep, because that can be intercepted, even
if the code is not instrumented.  And if this is the right solution for 
aligned_vs_unaligned.C it should be the used in all other tests as well.

Unfortunately there is no __attribute__((no_sanitize_thread)) just
no_sanitize_address, and no_sanitize_undefined.  So I need to call
the gcc compiler driver a second time with different options to compile
lib.c and link that together with the test case.  If done by hand, the tests
work very reliable, even without any sleep.

I did not know about the dg-additional-sources, that was new to me.
But it does not quite do what I need.  Unfortunately it just adds lib.c
to the gcc command line, and both sources use the same options.
But that instruments the atomics in lib.c and thus tsan figures out,
that there is really no race condition at all.
So some tests start to fail.


make check-gcc-c++ RUNTESTFLAGS="tsan.exp=*"
...
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/atomic_free.C   -O0  execution test
FAIL: g++.dg/tsan/atomic_free.C   -O2  execution test


That is of course only a technical problem, but one that is beyond my scope.
And OTOH I doubt that these race-free tests look very realistic.


Bernd.
 		 	   		  

[-- Attachment #2: tsan-racefree.diff --]
[-- Type: application/octet-stream, Size: 3374 bytes --]

Index: gcc/testsuite/g++.dg/tsan/aligned_vs_unaligned_race.C
===================================================================
--- gcc/testsuite/g++.dg/tsan/aligned_vs_unaligned_race.C	(revision 219198)
+++ gcc/testsuite/g++.dg/tsan/aligned_vs_unaligned_race.C	(working copy)
@@ -1,11 +1,15 @@
+/* { dg-additional-sources "lib.c" } */
 /* { dg-shouldfail "tsan" } */
 #include <pthread.h>
 #include <stdio.h>
 #include <stdint.h>
 
+void step(int);
+
 uint64_t Global[2];
 
 void *Thread1(void *x) {
+  step (2);
   Global[1]++;
   return NULL;
 }
@@ -15,6 +19,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;
 }
 
Index: gcc/testsuite/g++.dg/tsan/atomic_free.C
===================================================================
--- gcc/testsuite/g++.dg/tsan/atomic_free.C	(revision 219198)
+++ gcc/testsuite/g++.dg/tsan/atomic_free.C	(working copy)
@@ -1,10 +1,13 @@
+/* { dg-additional-sources "lib.c" } */
 /* { dg-shouldfail "tsan" } */
 
 #include <pthread.h>
-#include <unistd.h>
 
+void step(int);
+
 void *Thread(void *a) {
   __atomic_fetch_add((int*)a, 1, __ATOMIC_SEQ_CST);
+  step (1);
   return 0;
 }
 
@@ -12,7 +15,7 @@ int main() {
   int *a = new int(0);
   pthread_t t;
   pthread_create(&t, 0, Thread, a);
-  sleep(1);
+  step (2);
   delete a;
   pthread_join(t, 0);
 }
Index: gcc/testsuite/g++.dg/tsan/atomic_free2.C
===================================================================
--- gcc/testsuite/g++.dg/tsan/atomic_free2.C	(revision 219198)
+++ gcc/testsuite/g++.dg/tsan/atomic_free2.C	(working copy)
@@ -1,10 +1,12 @@
+/* { dg-additional-sources "lib.c" } */
 /* { dg-shouldfail "tsan" } */
 
 #include <pthread.h>
-#include <unistd.h>
 
+void step(int);
+
 void *Thread(void *a) {
-  sleep(1);
+  step (2);
   __atomic_fetch_add((int*)a, 1, __ATOMIC_SEQ_CST);
   return 0;
 }
@@ -14,6 +16,7 @@ int main() {
   pthread_t t;
   pthread_create(&t, 0, Thread, a);
   delete a;
+  step (1);
   pthread_join(t, 0);
 }
 
Index: gcc/testsuite/g++.dg/tsan/benign_race.C
===================================================================
--- gcc/testsuite/g++.dg/tsan/benign_race.C	(revision 219198)
+++ gcc/testsuite/g++.dg/tsan/benign_race.C	(working copy)
@@ -1,7 +1,9 @@
+/* { dg-additional-sources "lib.c" } */
 #include <pthread.h>
 #include <stdio.h>
-#include <unistd.h>
 
+void step(int);
+
 int Global;
 int WTFGlobal;
 
@@ -17,6 +19,7 @@ void WTFAnnotateBenignRaceSized(const char *f, int
 void *Thread(void *x) {
   Global = 42;
   WTFGlobal = 142;
+  step (1);
   return 0;
 }
 
@@ -28,7 +31,7 @@ int main() {
                              "Race on WTFGlobal");
   pthread_t t;
   pthread_create(&t, 0, Thread, 0);
-  sleep(1);
+  step (2);
   Global = 43;
   WTFGlobal = 143;
   pthread_join(t, 0);
Index: gcc/testsuite/g++.dg/tsan/lib.c
===================================================================
--- gcc/testsuite/g++.dg/tsan/lib.c	(revision 0)
+++ gcc/testsuite/g++.dg/tsan/lib.c	(working copy)
@@ -0,0 +1,10 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fno-sanitize=all" } */
+ 
+static volatile int serial = 0;
+ 
+void step (int i)
+{
+  while (__atomic_load_n (&serial, __ATOMIC_ACQUIRE) != i - 1);
+  __atomic_store_n (&serial, i, __ATOMIC_RELEASE);
+}

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

[Bernd Edlinger]

Hi Mike,

after some hours of sleep I realized that your step function can do something very interesting,
(which you already requested previously):

That is: create a race condition that is _always_ at 100% missed by tsan:

cat lib.c
/* { dg-do compile } */
/* { dg-options "-O2 -fno-sanitize=all" } */
 
static volatile int serial = 0;
 
void step (int i)
{
  while (__atomic_load_n (&serial, __ATOMIC_ACQUIRE) != i - 1);
  __atomic_store_n (&serial, i, __ATOMIC_RELEASE);
}

cat tiny_race.c 
/* { dg-shouldfail "tsan" } */

#include <pthread.h>

void step(int);

int Global;

void *Thread1(void *x) {
  step (1);
  Global = 42;
  step (3);
  return x;
}

int main() {
  pthread_t t;
  pthread_create(&t, 0, Thread1, 0);
  step (2);
  Global = 43;
  step (4);
  pthread_join(t, 0);
  return Global;
}

/* { dg-output "WARNING: ThreadSanitizer: data race.*(\n|\r\n|\r)" } */



Bernd.
 		 	   		  

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

[Jakub Jelinek]

On Tue, Jan 06, 2015 at 10:08:22AM +0100, Bernd Edlinger wrote:
> Hi Mike,
> 
> after some hours of sleep I realized that your step function can do something very interesting,
> (which you already requested previously):
> 
> That is: create a race condition that is _always_ at 100% missed by tsan:
> 
> cat lib.c
> /* { dg-do compile } */
> /* { dg-options "-O2 -fno-sanitize=all" } */
>  
> static volatile int serial = 0;
>  
> void step (int i)
> {
>   while (__atomic_load_n (&serial, __ATOMIC_ACQUIRE) != i - 1);
>   __atomic_store_n (&serial, i, __ATOMIC_RELEASE);
> }

Such busy waiting is not very CPU time friendly in the testsuite, especially
if you have just a single HW thread.
If libtsan is not fixed not to be so racy, perhaps instead of all the sleeps
we could arrange (on x86_64-linux, which is the only target supporting tsan
right now) to make sure the thread runs on the same core/hw thread as the
initial thread using pthread_[gs]etaffinity_np/pthread_attr_setaffinity_np ?
Does tsan then always report the races when the threads can't run
concurrently?

	Jakub

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

[Bernd Edlinger]

On Tue, 6 Jan 2015 10:16:33, Jakub Jelinek wrote:
>
> On Tue, Jan 06, 2015 at 10:08:22AM +0100, Bernd Edlinger wrote:
>> Hi Mike,
>>
>> after some hours of sleep I realized that your step function can do something very interesting,
>> (which you already requested previously):
>>
>> That is: create a race condition that is _always_ at 100% missed by tsan:
>>
>> cat lib.c
>> /* { dg-do compile } */
>> /* { dg-options "-O2 -fno-sanitize=all" } */
>>
>> static volatile int serial = 0;
>>
>> void step (int i)
>> {
>>   while (__atomic_load_n (&serial, __ATOMIC_ACQUIRE) != i - 1);
>>   __atomic_store_n (&serial, i, __ATOMIC_RELEASE);
>> }
>
> Such busy waiting is not very CPU time friendly in the testsuite, especially
> if you have just a single HW thread.

and in real-time environment, without time-slicing it is even a deadlock...

plus, I need to compile this helper with different options, that can be solved,
but currently I see no example where something like that was ever done.

> If libtsan is not fixed not to be so racy, perhaps instead of all the sleeps
> we could arrange (on x86_64-linux, which is the only target supporting tsan
> right now) to make sure the thread runs on the same core/hw thread as the
> initial thread using pthread_[gs]etaffinity_np/pthread_attr_setaffinity_np ?
> Does tsan then always report the races when the threads can't run
> concurrently?
>

Probably yes, but there can be no guarantees.  Even a single core can
be interrupted.

I think we can live with sleep(1) in aligned_vs_unaligned.C,
failure of that test will be very very unlikely as well.

.. and if we find a way how to do this custom buiild step in the test suite,
it would be good as a XFAIL test, that reminds us of the racy nature of libtsan.


Bernd.

> Jakub
 		 	   		  

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

[Bernd Edlinger]

----------------------------------------
> Date: Tue, 6 Jan 2015 10:16:33 +0100
> From: jakub@redhat.com
> To: bernd.edlinger@hotmail.de
> CC: mikestump@comcast.net; hjl.tools@gmail.com; gcc-patches@gcc.gnu.org; dvyukov@google.com
> Subject: Re: [PATCH] Fix sporadic failure in g++.dg/tsan/aligned_vs_unaligned_race.C
>
> On Tue, Jan 06, 2015 at 10:08:22AM +0100, Bernd Edlinger wrote:
>> Hi Mike,
>>
>> after some hours of sleep I realized that your step function can do something very interesting,
>> (which you already requested previously):
>>
>> That is: create a race condition that is _always_ at 100% missed by tsan:
>>
>> cat lib.c
>> /* { dg-do compile } */
>> /* { dg-options "-O2 -fno-sanitize=all" } */
>>
>> static volatile int serial = 0;
>>
>> void step (int i)
>> {
>>   while (__atomic_load_n (&serial, __ATOMIC_ACQUIRE) != i - 1);
>>   __atomic_store_n (&serial, i, __ATOMIC_RELEASE);
>> }
>
> Such busy waiting is not very CPU time friendly in the testsuite, especially
> if you have just a single HW thread.
> If libtsan is not fixed not to be so racy, perhaps instead of all the sleeps
> we could arrange (on x86_64-linux, which is the only target supporting tsan
> right now) to make sure the thread runs on the same core/hw thread as the
> initial thread using pthread_[gs]etaffinity_np/pthread_attr_setaffinity_np ?
> Does tsan then always report the races when the threads can't run
> concurrently?
>
> Jakub

I tried your suggestion now, and it seems to work. (on a  4-way core AMD64 laptop)

Would you prefer this over adding a sleep in Thread1, which I posted previously?




2015-01-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>

        * g++.dg/tsan/aligned_vs_unaligned_race.C: Fixed sporadic failure.


Index: aligned_vs_unaligned_race.C
===================================================================
--- aligned_vs_unaligned_race.C    (revision 219198)
+++ aligned_vs_unaligned_race.C    (working copy)
@@ -1,5 +1,7 @@
 /* { dg-shouldfail "tsan" } */
+/* { dg-additional-options "-pthread" } */
 #include <pthread.h>
+#include <sched.h>
 #include <stdio.h>
 #include <stdint.h>
 
@@ -19,6 +21,12 @@ void *Thread2(void *x) {
 }
 
 int main() {
+  /* Run this test on a single CPU, to make it somewhat easier for tsan to
+     detect the race condition.  */
+  cpu_set_t s;
+  CPU_ZERO(&s);
+  CPU_SET(0, &s);
+  pthread_setaffinity_np(pthread_self(), sizeof(s), &s);
   pthread_t t[2];
   pthread_create(&t[0], NULL, Thread1, NULL);
   pthread_create(&t[1], NULL, Thread2, NULL);



Thanks,
Bernd.
 		 	   		  

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

[Mike Stump]

On Jan 6, 2015, at 9:45 AM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote:
> I tried your suggestion now, and it seems to work. (on a  4-way core AMD64 laptop)
> 
> Would you prefer this over adding a sleep in Thread1, which I posted previously?

The problem with the patch is there is nothing in the patch that makes it work.  You merely lower the odds of it failing.  The point of addressing the problem, and the problem is, this test case randomly fails, is to change the test case, so that it is impossible by design for the test case to ever fail.  In my version of the fix, I think it can't fail.

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

[Bernd Edlinger]

On Tue, 6 Jan 2015 11:47:30, Mike Stump wrote:
>
> On Jan 6, 2015, at 9:45 AM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote:
>> I tried your suggestion now, and it seems to work. (on a 4-way core AMD64 laptop)
>>
>> Would you prefer this over adding a sleep in Thread1, which I posted previously?
>
> The problem with the patch is there is nothing in the patch that makes it work. You merely lower the odds of it failing. The point of addressing the problem, and the problem is, this test case randomly fails, is to change the test case, so that it is impossible by design for the test case to ever fail. In my version of the fix, I think it can't fail.

Yes, I think too that it can't fail under these conditions. Maybe that is just a philosophical problem.

If we prepare the test in this way it does not really contain any race condition.
   
(*p4).val++;
   step (1);

happens first, and then:

   step (2);
   Global[1]++;

but we consider the test passed, when we see a diagnostic?


>> Therefore it should not call sleep,
> Again, sleep can’t fix race conditions, so therefore tsan can never use it as an indication that no race exists.  If it ever did, that would be a bug.  The only case where that would not be true, would be a hard real time analysis verifier, and, when we get one, sleep can be so modified.
>> because that can be intercepted, even if the code is not instrumented.
> I don’t see the relevance.


I mention that only, because sleep, usleep and nanosleep are intercepted by tsan_interceptors.cc
and calling them alters the printed diagnostics, some tests need to see "as if synchronized by sleep",
some don't.

As far as I can see, there is no interceptor for sched_yield, so using that in the step function is OK.

However the other use of step is a valid test, although one that always fails.
Here we see a real race condition with no tsan diagnostic, just because both
threads alter the same variable at the same nanosecond, that is remarkable.
Just as if tsan had a blind spot here.

Once again, with a 3-way handshake in case the Tread1 is scheduled before
pthread_create returns:

cat tiny-race.c
/* { dg-shouldfail "tsan" } */

#include <pthread.h>

void step(int);

int Global;

void *Thread1(void *x) {
  step (2);
  Global = 42;
  return x;
}

int main() {
  pthread_t t;
  pthread_create(&t, 0, Thread1, 0);
  step (1);
  step (3);
  Global = 43;
  pthread_join(t, 0);
  return 0;
}

/* { dg-output "WARNING: ThreadSanitizer: data race.*(\n|\r\n|\r)" } */

>> Unfortunately there is no __attribute__((no_sanitize_thread))
> So, is that any harder to add then:

If a new attribute is easier to implement than teaching the tsan.exp tcl script
that lib.c is something special, maybe...


Bernd.
 		 	   		  

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

[Mike Stump]

On Jan 6, 2015, at 3:22 PM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote:
> Yes, I think too that it can't fail under these conditions.

If you mean your version…  A lot has been written on how to to make racy code non-racy…  I’d refer you to the literature on all the various solutions people have found to date.  I don’t think I’ve ever seen anyone claim that affinity can be used to solve race conditions like this.  Do you have a pointer?

> If we prepare the test in this way it does not really contain any race condition.
> 
> (*p4).val++;
>   step (1);
> 
> happens first, and then:
> 
>   step (2);
>   Global[1]++;
> 
> but we consider the test passed, when we see a diagnostic?

If tsan doesn’t work in the face of a race, then trivially, the code to test it cannot have a race.  The difference is letting tsan know there is a race, versus hiding the fact there is no race from it, so that it still thinks there is a race.  For test cases to diagnose a race, those cases must have the code that ensures there is not a race, hidden from view.

> As far as I can see, there is no interceptor for sched_yield, so using that in the step function is OK.

Sounds good.

I’m running a test suite run now with my attribute patch.  I’ll ask the tsan people if it can go in, if it passes.

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

[Bernd Edlinger]

On Tue, 6 Jan 2015 16:32:35, Mike Stump wrote:
>
> On Jan 6, 2015, at 3:22 PM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote:
>> Yes, I think too that it can't fail under these conditions.
>
> If you mean your version… A lot has been written on how to to make racy code non-racy… I’d refer you to the literature on all the various solutions people have found to date. I don’t think I’ve ever seen anyone claim that affinity can be used to solve race conditions like this. Do you have a pointer?
>

no, I agree, the affinity would just lower the likelihood, as sleep does.
The version with affinity is just disgusting, sorry to have posted it ;-).

I meant your version, with step(1)/step(2) between the race. It is probably bullet-proof.
But the version with sleep is more realistic.  That is however just a matter of taste...

If we can agree to use your version, then all tests in g++.dg/tsan and
c-c++-common/tsan that currently use sleep(1) should use your step function instead.
sleep_sync.c can use step but needs still to call sleep because it triggers on the
"As if synchronized via sleep" diagnostic.

I don't know what to make of simple_race.c, which uses usleep, but uses
a loop to repeat the race often. Maybe leave that one as a "realistic" test.


Bernd.
 		 	   		  

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

[Jakub Jelinek]

On Wed, Jan 07, 2015 at 08:17:34AM +0100, Bernd Edlinger wrote:
> 
> On Tue, 6 Jan 2015 16:32:35, Mike Stump wrote:
> >
> > On Jan 6, 2015, at 3:22 PM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote:
> >> Yes, I think too that it can't fail under these conditions.
> >
> > If you mean your version… A lot has been written on how to to make racy code non-racy… I’d refer you to the literature on all the various solutions people have found to date. I don’t think I’ve ever seen anyone claim that affinity can be used to solve race conditions like this. Do you have a pointer?
> >
> 
> no, I agree, the affinity would just lower the likelihood, as sleep does.
> The version with affinity is just disgusting, sorry to have posted it ;-).
> 
> I meant your version, with step(1)/step(2) between the race. It is probably bullet-proof.
> But the version with sleep is more realistic.  That is however just a matter of taste...
> 
> If we can agree to use your version, then all tests in g++.dg/tsan and
> c-c++-common/tsan that currently use sleep(1) should use your step function instead.
> sleep_sync.c can use step but needs still to call sleep because it triggers on the
> "As if synchronized via sleep" diagnostic.
> 
> I don't know what to make of simple_race.c, which uses usleep, but uses
> a loop to repeat the race often. Maybe leave that one as a "realistic" test.

I'm fine with adding the no_sanitize_thread attribute, the patch LGTM, I
think we might even have a PR for that.

But I really don't like the busy waiting.  You essentially want something
like pthread_barrier_wait that libtsan isn't aware of, right?

As tsan is only supported on x86_64-linux (and in the future could be on
some other 64-bit linuxes), I wonder if we couldn't just simplify libgomp's
config/linux/bar* for that purpose - libtsan doesn't intercept syscall(3)
and doesn't intercept futex in particular, so we could have a busy waiting
free synchronization primitive.  Or another option is to just dlopen
libpthread.so.0 and dlsym pthread_barrier_wait in it and use that, that way
libtsan won't intercept it either.

	Jakub

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

[Bernd Edlinger]

On Wed, 7 Jan 2015 09:23:39, Jakub Jelinek wrote:
>
> But I really don't like the busy waiting. You essentially want something
> like pthread_barrier_wait that libtsan isn't aware of, right?
>

Yes.


> As tsan is only supported on x86_64-linux (and in the future could be on
> some other 64-bit linuxes), I wonder if we couldn't just simplify libgomp's
> config/linux/bar* for that purpose - libtsan doesn't intercept syscall(3)
> and doesn't intercept futex in particular, so we could have a busy waiting
> free synchronization primitive. Or another option is to just dlopen
> libpthread.so.0 and dlsym pthread_barrier_wait in it and use that, that way
> libtsan won't intercept it either.
>


That would be the honey pot.  But unfortunately I can't reach it.


I tried this,

cat barrier.h
#include <dlfcn.h>

static pthread_barrier_t barrier;

static int (*barrier_wait)(pthread_barrier_t *);

__attribute__((constructor(101)))
void barrier_init()
{
  pthread_barrier_init (&barrier, NULL, 2);
  barrier_wait = (int (*)(pthread_barrier_t *))
                 dlsym (dlopen ("pthread.so.0", RTLD_NOW), "pthread_barrier_wait");
}


But dlsym gives me only a pointer to the tsan-interceptor.


Bernd.

 		 	   		  

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

[Jakub Jelinek]

On Wed, Jan 07, 2015 at 03:55:11PM +0100, Bernd Edlinger wrote:
> cat barrier.h
> #include <dlfcn.h>
> 
> static pthread_barrier_t barrier;
> 
> static int (*barrier_wait)(pthread_barrier_t *);

Better
static __typeof (pthread_barrier_wait) *barrier_wait;
?
> 
> __attribute__((constructor(101)))

I wouldn't use a constructor for it, instead simply call it from the
testcase at the start of main.

> void barrier_init()
> {
>   pthread_barrier_init (&barrier, NULL, 2);
>   barrier_wait = (int (*)(pthread_barrier_t *))
>                  dlsym (dlopen ("pthread.so.0", RTLD_NOW), "pthread_barrier_wait");

"libpthread.so.0" instead.

I'd use:
#ifdef RTLD_NOLOAD
  void *h = dlopen ("libpthread.so.0", RTLD_NOLOAD);
#else
  void *h = dlopen ("libpthread.so.0", RTLD_NOW);
#endif
  barrier_wait = (__typeof (pthread_barrier_wait) *) dlsym (h, "pthread_barrier_wait");

	Jakub

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

[Mike Stump]

On Jan 7, 2015, at 12:23 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> But I really don't like the busy waiting.

We’ve already determined that sched_sleep isn’t intercepted and can be used to non-busy wait.  Any reason not to use it?

> As tsan is only supported on x86_64-linux

So, I hate hardening the code to be overly non-portable when it doesn’t have to be that.  There is something enticing to me about the simplicity of sched_sleep.

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

[Jakub Jelinek]

On Wed, Jan 07, 2015 at 08:58:04AM -0800, Mike Stump wrote:
> On Jan 7, 2015, at 12:23 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> > But I really don't like the busy waiting.
> 
> We’ve already determined that sched_sleep isn’t intercepted and can be used to non-busy wait.  Any reason not to use it?
> 
> > As tsan is only supported on x86_64-linux
> 
> So, I hate hardening the code to be overly non-portable when it doesn’t have to be that.  There is something enticing to me about the simplicity of sched_sleep.

Well, pthread_barrier_wait and dlopen/dlsym are already used by libtsan and
therefore have to be supported on all the architectures that support tsan.
So that method is as portable as libtsan itself.

	Jakub

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

[Bernd Edlinger]

On Wed, 7 Jan 2015 18:00:27, Jakub Jelinek wrote:
>
> On Wed, Jan 07, 2015 at 08:58:04AM -0800, Mike Stump wrote:
>> On Jan 7, 2015, at 12:23 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>> But I really don't like the busy waiting.
>>
>> We’ve already determined that sched_sleep isn’t intercepted and can be used to non-busy wait. Any reason not to use it?
>>
>>> As tsan is only supported on x86_64-linux
>>
>> So, I hate hardening the code to be overly non-portable when it doesn’t have to be that. There is something enticing to me about the simplicity of sched_sleep.
>
> Well, pthread_barrier_wait and dlopen/dlsym are already used by libtsan and
> therefore have to be supported on all the architectures that support tsan.
> So that method is as portable as libtsan itself.
>
> Jakub


Ok, just for completeness, I've got the dlopen finally working:
RTLD_NOLOAD is not working, but RTLD_LAZY or RTLD_NOW does.

The test case is passing reliably with this method too.

I am however not sure if I can always use -ldl or have to use some target-dependencies here.



Index: aligned_vs_unaligned_race.C
===================================================================
--- aligned_vs_unaligned_race.C    (revision 219198)
+++ aligned_vs_unaligned_race.C    (working copy)
@@ -1,11 +1,17 @@
 /* { dg-shouldfail "tsan" } */
+/* { dg-additional-options "-ldl" } */
 #include <pthread.h>
+#include <sched.h>
 #include <stdio.h>
 #include <stdint.h>
+#include "barrier.h"
 
+static pthread_barrier_t barrier;
+
 uint64_t Global[2];
 
 void *Thread1(void *x) {
+  barrier_wait(&barrier);
   Global[1]++;
   return NULL;
 }
@@ -15,10 +21,12 @@
   struct __attribute__((packed, aligned(1))) u_uint64_t { uint64_t val; };
   u_uint64_t *p4 = reinterpret_cast<u_uint64_t *>(p1 + 1);
   (*p4).val++;
+  barrier_wait(&barrier);
   return NULL;
 }
 
 int main() {
+  barrier_init(&barrier);
   pthread_t t[2];
   pthread_create(&t[0], NULL, Thread1, NULL);
   pthread_create(&t[1], NULL, Thread2, NULL);
Index: barrier.h
===================================================================
--- barrier.h    (revision 0)
+++ barrier.h    (working copy)
@@ -0,0 +1,12 @@
+#include <dlfcn.h>
+
+static __typeof(pthread_barrier_wait) *barrier_wait;
+
+static
+void barrier_init (pthread_barrier_t *barrier)
+{
+  void *h = dlopen ("libpthread.so.0", RTLD_LAZY);
+  barrier_wait = (__typeof (pthread_barrier_wait) *)
+          dlsym (h, "pthread_barrier_wait");
+  pthread_barrier_init (barrier, NULL, 2);
+}


Thanks
Bernd.
 		 	   		  

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

[Jakub Jelinek]

On Wed, Jan 07, 2015 at 07:21:40PM +0100, Bernd Edlinger wrote:
> Ok, just for completeness, I've got the dlopen finally working:
> RTLD_NOLOAD is not working, but RTLD_LAZY or RTLD_NOW does.

No big deal I guess.

> I am however not sure if I can always use -ldl or have to use some target-dependencies here.

libsanitizer always puts
-lpthread -ldl -lm
into libsanitizer.spec, so I'd say it should be safe and you shouldn't even
need -ldl in dg-additional-options, as merely linking with -fsanitize=thread
should link -ldl in.

> --- barrier.h    (revision 0)
> +++ barrier.h    (working copy)

I think better to name it tsan_barrier.h or otherwise make it
clear what this is for.

> @@ -0,0 +1,12 @@
> +#include <dlfcn.h>
> +
> +static __typeof(pthread_barrier_wait) *barrier_wait;
> +
> +static
> +void barrier_init (pthread_barrier_t *barrier)

And, I'd add unsigned count argument here, and pass it through
pthread_barrier_init, just in case you need more than 2 threads
in some test.

Also, what do you need <sched.h> for?

	Jakub

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

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 1695 bytes --]


On Wed, 7 Jan 2015 19:32:16, Jakub Jelinek wrote:
>> I am however not sure if I can always use -ldl or have to use some target-dependencies here.
>
> libsanitizer always puts
> -lpthread -ldl -lm
> into libsanitizer.spec, so I'd say it should be safe and you shouldn't even
> need -ldl in dg-additional-options, as merely linking with -fsanitize=thread
> should link -ldl in.
>

I need -ldl otherwise this happens:

FAIL: g++.dg/tsan/aligned_vs_unaligned_race.C   -O0  (test for excess errors)
Excess errors:
/home/ed/gnu/install/x86_64-unknown-linux-gnu/bin/ld: /tmp/ccW6IHbj.o: undefined reference to symbol 'dlsym@@GLIBC_2.2.5'
/lib/x86_64-linux-gnu/libdl.so.2: error adding symbols: DSO missing from command line


>> --- barrier.h    (revision 0)
>> +++ barrier.h    (working copy)
>
> I think better to name it tsan_barrier.h or otherwise make it
> clear what this is for.
>

Yes. I'll call it tsan_barrier.h and add a comment.

>> @@ -0,0 +1,12 @@
>> +#include <dlfcn.h>
>> +
>> +static __typeof(pthread_barrier_wait) *barrier_wait;
>> +
>> +static
>> +void barrier_init (pthread_barrier_t *barrier)
>
> And, I'd add unsigned count argument here, and pass it through
> pthread_barrier_init, just in case you need more than 2 threads
> in some test.
>

done.

> Also, what do you need <sched.h> for?
>

Oops ... Thanks for your help.

Here is a new patch, that uses this method on all tsan tests,
which seem to depend on sleep(1), and which have unstable results
because of that.

Successfully tested multiple times with:

make check-gcc-c check-gcc-c++ RUNTESTFLAGS="tsan.exp=*"


OK for trunk?


Thanks
Bernd.

 		 	   		  

[-- Attachment #2: changelog-tsan-tests.txt --]
[-- Type: text/plain, Size: 993 bytes --]

testsuite/ChangeLog:
2015-01-07  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c-c++-common/tsan/tsan_barrier.h: New.
	* c-c++-common/tsan/atomic_stack.c: Reworked to not depend on sleep.
	* c-c++-common/tsan/bitfield_race.c: Likewise.
	* c-c++-common/tsan/fd_pipe_race.c: Likewise.
	* c-c++-common/tsan/mutexset1.c: Likewise.
	* c-c++-common/tsan/race_on_barrier.c: Likewise.
	* c-c++-common/tsan/race_on_mutex.c: Likewise.
	* c-c++-common/tsan/race_on_mutex2.c: Likewise.
	* c-c++-common/tsan/simple_race.c: Likewise.
	* c-c++-common/tsan/simple_stack.c: Likewise.
	* c-c++-common/tsan/sleep_sync.c: Likewise.
	* c-c++-common/tsan/tiny_race.c: Likewise.
	* c-c++-common/tsan/tls_race.c: Likewise.
	* c-c++-common/tsan/write_in_reader_lock.c: Likewise.
	* g++.dg/tsan/aligned_vs_unaligned_race.C: Likewise.
	* g++.dg/tsan/atomic_free.C: Likewise.
	* g++.dg/tsan/atomic_free2.C: Likewise.
	* g++.dg/tsan/cond_race.C: Likewise.
	* g++.dg/tsan/tsan_barrier.h: Copied from c-c++-common/tsan. 


[-- Attachment #3: patch-tsan-tests.diff --]
[-- Type: application/octet-stream, Size: 19200 bytes --]

Index: gcc/testsuite/c-c++-common/tsan/atomic_stack.c
===================================================================
--- gcc/testsuite/c-c++-common/tsan/atomic_stack.c	(revision 219198)
+++ gcc/testsuite/c-c++-common/tsan/atomic_stack.c	(working copy)
@@ -1,12 +1,14 @@
 /* { dg-shouldfail "tsan" } */
+/* { dg-additional-options "-ldl" } */
 
 #include <pthread.h>
-#include <unistd.h>
+#include "tsan_barrier.h"
 
+static pthread_barrier_t barrier;
 int Global;
 
 void *Thread1(void *x) {
-  sleep(1);
+  barrier_wait(&barrier);
   __atomic_fetch_add(&Global, 1, __ATOMIC_RELAXED);
   return NULL;
 }
@@ -13,10 +15,12 @@
 
 void *Thread2(void *x) {
   Global++;
+  barrier_wait(&barrier);
   return NULL;
 }
 
 int main() {
+  barrier_init(&barrier, 2);
   pthread_t t[2];
   pthread_create(&t[0], NULL, Thread1, NULL);
   pthread_create(&t[1], NULL, Thread2, NULL);
Index: gcc/testsuite/c-c++-common/tsan/bitfield_race.c
===================================================================
--- gcc/testsuite/c-c++-common/tsan/bitfield_race.c	(revision 219198)
+++ gcc/testsuite/c-c++-common/tsan/bitfield_race.c	(working copy)
@@ -1,8 +1,10 @@
 /* { dg-shouldfail "tsan" } */
+/* { dg-additional-options "-ldl" } */
 
 #include <pthread.h>
-#include <unistd.h>
+#include "tsan_barrier.h"
 
+static pthread_barrier_t barrier;
 struct bitfield
 {
   int a:10;
@@ -10,15 +12,17 @@
 } Global;
 
 void *Thread1(void *x) {
-  sleep(1);
+  barrier_wait(&barrier);
   Global.a = 42;
   return x;
 }
 
 int main() {
+  barrier_init(&barrier, 2);
   pthread_t t;
   pthread_create(&t, 0, Thread1, 0);
   Global.b = 43;
+  barrier_wait(&barrier);
   pthread_join(t, 0);
   return Global.a;
 }
Index: gcc/testsuite/c-c++-common/tsan/fd_pipe_race.c
===================================================================
--- gcc/testsuite/c-c++-common/tsan/fd_pipe_race.c	(revision 219198)
+++ gcc/testsuite/c-c++-common/tsan/fd_pipe_race.c	(working copy)
@@ -1,18 +1,21 @@
 /* { dg-shouldfail "tsan" } */
+/* { dg-additional-options "-ldl" } */
 
 #include <pthread.h>
-#include <stdio.h>
 #include <unistd.h>
+#include "tsan_barrier.h"
 
+static pthread_barrier_t barrier;
 int fds[2];
 
 void *Thread1(void *x) {
   write(fds[1], "a", 1);
+  barrier_wait(&barrier);
   return NULL;
 }
 
 void *Thread2(void *x) {
-  sleep(1);
+  barrier_wait(&barrier);
   close(fds[0]);
   close(fds[1]);
   return NULL;
@@ -19,6 +22,7 @@
 }
 
 int main() {
+  barrier_init(&barrier, 2);
   pipe(fds);
   pthread_t t[2];
   pthread_create(&t[0], NULL, Thread1, NULL);
@@ -25,6 +29,7 @@
   pthread_create(&t[1], NULL, Thread2, NULL);
   pthread_join(t[0], NULL);
   pthread_join(t[1], NULL);
+  return 0;
 }
 
 /* { dg-output "WARNING: ThreadSanitizer: data race.*\n" } */
Index: gcc/testsuite/c-c++-common/tsan/mutexset1.c
===================================================================
--- gcc/testsuite/c-c++-common/tsan/mutexset1.c	(revision 219198)
+++ gcc/testsuite/c-c++-common/tsan/mutexset1.c	(working copy)
@@ -1,14 +1,15 @@
 /* { dg-shouldfail "tsan" } */
+/* { dg-additional-options "-ldl" } */
 
 #include <pthread.h>
-#include <stdio.h>
-#include <unistd.h>
+#include "tsan_barrier.h"
 
+static pthread_barrier_t barrier;
 int Global;
 pthread_mutex_t mtx;
 
 void *Thread1(void *x) {
-  sleep(1);
+  barrier_wait(&barrier);
   pthread_mutex_lock(&mtx);
   Global++;
   pthread_mutex_unlock(&mtx);
@@ -17,11 +18,13 @@
 
 void *Thread2(void *x) {
   Global--;
+  barrier_wait(&barrier);
   return NULL;/* { dg-output ".*" } */
 
 }
 
 int main() {
+  barrier_init(&barrier, 2);
   pthread_mutex_init(&mtx, 0);
   pthread_t t[2];
   pthread_create(&t[0], NULL, Thread1, NULL);
Index: gcc/testsuite/c-c++-common/tsan/race_on_barrier.c
===================================================================
--- gcc/testsuite/c-c++-common/tsan/race_on_barrier.c	(revision 219198)
+++ gcc/testsuite/c-c++-common/tsan/race_on_barrier.c	(working copy)
@@ -1,26 +1,28 @@
 /* { dg-shouldfail "tsan" } */
+/* { dg-additional-options "-ldl" } */
 
 #include <pthread.h>
-#include <stdio.h>
-#include <stddef.h>
-#include <unistd.h>
+#include "tsan_barrier.h"
 
+static pthread_barrier_t barrier;
 pthread_barrier_t B;
 int Global;
 
 void *Thread1(void *x) {
   pthread_barrier_init(&B, 0, 2);
+  barrier_wait(&barrier);
   pthread_barrier_wait(&B);
   return NULL;
 }
 
 void *Thread2(void *x) {
-  sleep(1);
+  barrier_wait(&barrier);
   pthread_barrier_wait(&B);
   return NULL;
 }
 
 int main() {
+  barrier_init(&barrier, 2);
   pthread_t t;
   pthread_create(&t, NULL, Thread1, NULL);
   Thread2(0);
Index: gcc/testsuite/c-c++-common/tsan/race_on_mutex.c
===================================================================
--- gcc/testsuite/c-c++-common/tsan/race_on_mutex.c	(revision 219198)
+++ gcc/testsuite/c-c++-common/tsan/race_on_mutex.c	(working copy)
@@ -1,10 +1,10 @@
 /* { dg-shouldfail "tsan" } */
+/* { dg-additional-options "-ldl" } */
 
 #include <pthread.h>
-#include <stdio.h>
-#include <stddef.h>
-#include <unistd.h>
+#include "tsan_barrier.h"
 
+static pthread_barrier_t barrier;
 pthread_mutex_t Mtx;
 int Global;
 
@@ -13,11 +13,12 @@
   pthread_mutex_lock(&Mtx);
   Global = 42;
   pthread_mutex_unlock(&Mtx);
+  barrier_wait(&barrier);
   return NULL;
 }
 
 void *Thread2(void *x) {
-  sleep(1);
+  barrier_wait(&barrier);
   pthread_mutex_lock(&Mtx);
   Global = 43;
   pthread_mutex_unlock(&Mtx);
@@ -25,6 +26,7 @@
 }
 
 int main() {
+  barrier_init(&barrier, 2);
   pthread_t t[2];
   pthread_create(&t[0], NULL, Thread1, NULL);
   pthread_create(&t[1], NULL, Thread2, NULL);
@@ -37,7 +39,7 @@
 /* { dg-output "WARNING: ThreadSanitizer: data race.*(\n|\r\n|\r)" } */
 /* { dg-output "  Atomic read of size 1 at .* by thread T2:(\n|\r\n|\r)" } */
 /* { dg-output "    #0 pthread_mutex_lock.*" } */
-/* { dg-output "    #1 Thread2.* .*(race_on_mutex.c:21|\\?{2}:0) (.*)" } */
+/* { dg-output "    #1 Thread2.* .*(race_on_mutex.c:22|\\?{2}:0) (.*)" } */
 /* { dg-output "  Previous write of size 1 at .* by thread T1:(\n|\r\n|\r)" } */
 /* { dg-output "    #0 pthread_mutex_init .* (.)*" } */
 /* { dg-output "    #1 Thread1.* .*(race_on_mutex.c:12|\\?{2}:0) .*" } */
Index: gcc/testsuite/c-c++-common/tsan/race_on_mutex2.c
===================================================================
--- gcc/testsuite/c-c++-common/tsan/race_on_mutex2.c	(revision 219198)
+++ gcc/testsuite/c-c++-common/tsan/race_on_mutex2.c	(working copy)
@@ -1,22 +1,25 @@
 /* { dg-shouldfail "tsan" } */
+/* { dg-additional-options "-ldl" } */
 
 #include <pthread.h>
-#include <stdio.h>
-#include <stddef.h>
-#include <unistd.h>
+#include "tsan_barrier.h"
 
+static pthread_barrier_t barrier;
+
 void *Thread(void *x) {
   pthread_mutex_lock((pthread_mutex_t*)x);
   pthread_mutex_unlock((pthread_mutex_t*)x);
+  barrier_wait(&barrier);
   return 0;
 }
 
 int main() {
+  barrier_init(&barrier, 2);
   pthread_mutex_t Mtx;
   pthread_mutex_init(&Mtx, 0);
   pthread_t t;
   pthread_create(&t, 0, Thread, &Mtx);
-  sleep(1);
+  barrier_wait(&barrier);
   pthread_mutex_destroy(&Mtx);
   pthread_join(t, 0);
   return 0;
Index: gcc/testsuite/c-c++-common/tsan/simple_race.c
===================================================================
--- gcc/testsuite/c-c++-common/tsan/simple_race.c	(revision 219198)
+++ gcc/testsuite/c-c++-common/tsan/simple_race.c	(working copy)
@@ -1,13 +1,15 @@
 /* { dg-set-target-env-var TSAN_OPTIONS "halt_on_error=1" } */
 /* { dg-shouldfail "tsan" } */
+/* { dg-additional-options "-ldl" } */
 
 #include <pthread.h>
-#include <stdio.h>
 #include <unistd.h>
+#include "tsan_barrier.h"
 
-#define MAX_ITERATIONS_NUMBER 100
-#define SLEEP_STEP 128000 
+#define MAX_ITERATIONS_NUMBER 1
+#define SLEEP_STEP 128000
 
+static pthread_barrier_t barrier;
 unsigned int delay_time = 1000;
 
 static inline void delay () {
@@ -17,6 +19,7 @@
 extern int main_1();
 
 int main() {
+  barrier_init(&barrier, 2);
   int i;
   for (i = 0; i < MAX_ITERATIONS_NUMBER; i++) {
     main_1();
@@ -28,6 +31,7 @@
 int Global;
 
 void *Thread1(void *x) {
+  barrier_wait(&barrier);
   delay();
   Global = 42;
   return NULL;
@@ -35,6 +39,7 @@
 
 void *Thread2(void *x) {
   Global = 43;
+  barrier_wait(&barrier);
   return NULL;
 }
 
Index: gcc/testsuite/c-c++-common/tsan/simple_stack.c
===================================================================
--- gcc/testsuite/c-c++-common/tsan/simple_stack.c	(revision 219198)
+++ gcc/testsuite/c-c++-common/tsan/simple_stack.c	(working copy)
@@ -1,9 +1,10 @@
 /* { dg-shouldfail "tsan" } */
+/* { dg-additional-options "-ldl" } */
 
 #include <pthread.h>
-#include <stdio.h>
-#include <unistd.h>
+#include "tsan_barrier.h"
 
+static pthread_barrier_t barrier;
 int Global;
 
 void __attribute__((noinline)) foo1() {
@@ -25,7 +26,7 @@
 }
 
 void *Thread1(void *x) {
-  sleep(1);
+  barrier_wait(&barrier);
   bar1();
   return NULL;
 }
@@ -32,6 +33,7 @@
 
 void *Thread2(void *x) {
   bar2();
+  barrier_wait(&barrier);
   return NULL;
 }
 
@@ -40,6 +42,7 @@
 }
 
 int main() {
+  barrier_init(&barrier, 2);
   pthread_t t[2];
   StartThread(&t[0], Thread1);
   StartThread(&t[1], Thread2);
@@ -50,16 +53,16 @@
 
 /* { dg-output "WARNING: ThreadSanitizer: data race.*" } */
 /* { dg-output "  Write of size 4 at .* by thread T1:(\n|\r\n|\r)" } */
-/* { dg-output "    #0 foo1.* .*(simple_stack.c:10|\\?{2}:0) (.*)" } */
-/* { dg-output "    #1 bar1.* .*(simple_stack.c:15|\\?{2}:0) (.*)" } */
-/* { dg-output "    #2 Thread1.* .*(simple_stack.c:29|\\?{2}:0) (.*)" } */
+/* { dg-output "    #0 foo1.* .*(simple_stack.c:11|\\?{2}:0) (.*)" } */
+/* { dg-output "    #1 bar1.* .*(simple_stack.c:16|\\?{2}:0) (.*)" } */
+/* { dg-output "    #2 Thread1.* .*(simple_stack.c:30|\\?{2}:0) (.*)" } */
 /* { dg-output "  Previous read of size 4 at .* by thread T2:(\n|\r\n|\r)" } */
-/* { dg-output "    #0 foo2.* .*(simple_stack.c:19|\\?{2}:0) (.*)" } */
-/* { dg-output "    #1 bar2.* .*(simple_stack.c:24|\\?{2}:0) (.*)" } */
-/* { dg-output "    #2 Thread2.* .*(simple_stack.c:34|\\?{2}:0) (.*)" } */
+/* { dg-output "    #0 foo2.* .*(simple_stack.c:20|\\?{2}:0) (.*)" } */
+/* { dg-output "    #1 bar2.* .*(simple_stack.c:25|\\?{2}:0) (.*)" } */
+/* { dg-output "    #2 Thread2.* .*(simple_stack.c:35|\\?{2}:0) (.*)" } */
 /* { dg-output "  Thread T1 \\(tid=.*, running\\) created by main thread at:(\n|\r\n|\r)" } */
 /* { dg-output "    #0 pthread_create .* (.*)" } */
-/* { dg-output "    #1 StartThread.* .*(simple_stack.c:39|\\?{2}:0) (.*)" } */
+/* { dg-output "    #1 StartThread.* .*(simple_stack.c:41|\\?{2}:0) (.*)" } */
 /* { dg-output "  Thread T2 (.*) created by main thread at:(\n|\r\n|\r)" } */
 /* { dg-output "    #0 pthread_create .* (.*)" } */
-/* { dg-output "    #1 StartThread.* .*(simple_stack.c:39|\\?{2}:0) (.*)" } */
+/* { dg-output "    #1 StartThread.* .*(simple_stack.c:41|\\?{2}:0) (.*)" } */
Index: gcc/testsuite/c-c++-common/tsan/sleep_sync.c
===================================================================
--- gcc/testsuite/c-c++-common/tsan/sleep_sync.c	(revision 219198)
+++ gcc/testsuite/c-c++-common/tsan/sleep_sync.c	(working copy)
@@ -1,8 +1,11 @@
 /* { dg-shouldfail "tsan" } */
+/* { dg-additional-options "-ldl" } */
 
 #include <pthread.h>
 #include <unistd.h>
+#include "tsan_barrier.h"
 
+static pthread_barrier_t barrier;
 int X = 0;
 
 void MySleep() {
@@ -10,6 +13,7 @@
 }
 
 void *Thread(void *p) {
+  barrier_wait(&barrier);
   MySleep();  // Assume the main thread has done the write.
   X = 42;
   return 0;
@@ -16,9 +20,11 @@
 }
 
 int main() {
+  barrier_init(&barrier, 2);
   pthread_t t;
   pthread_create(&t, 0, Thread, 0);
   X = 43;
+  barrier_wait(&barrier);
   pthread_join(t, 0);
   return 0;
 }
Index: gcc/testsuite/c-c++-common/tsan/tiny_race.c
===================================================================
--- gcc/testsuite/c-c++-common/tsan/tiny_race.c	(revision 219198)
+++ gcc/testsuite/c-c++-common/tsan/tiny_race.c	(working copy)
@@ -1,20 +1,24 @@
 /* { dg-shouldfail "tsan" } */
+/* { dg-additional-options "-ldl" } */
 
 #include <pthread.h>
-#include <unistd.h>
+#include "tsan_barrier.h"
 
+static pthread_barrier_t barrier;
 int Global;
 
 void *Thread1(void *x) {
-  sleep(1);
+  barrier_wait(&barrier);
   Global = 42;
   return x;
 }
 
 int main() {
+  barrier_init(&barrier, 2);
   pthread_t t;
   pthread_create(&t, 0, Thread1, 0);
   Global = 43;
+  barrier_wait(&barrier);
   pthread_join(t, 0);
   return Global;
 }
Index: gcc/testsuite/c-c++-common/tsan/tls_race.c
===================================================================
--- gcc/testsuite/c-c++-common/tsan/tls_race.c	(revision 219198)
+++ gcc/testsuite/c-c++-common/tsan/tls_race.c	(working copy)
@@ -1,18 +1,24 @@
 /* { dg-shouldfail "tsan" } */
+/* { dg-additional-options "-ldl" } */
 
 #include <pthread.h>
-#include <stddef.h>
+#include "tsan_barrier.h"
 
+static pthread_barrier_t barrier;
+
 void *Thread(void *a) {
+  barrier_wait(&barrier);
   *(int*)a = 43;
   return 0;
 }
 
 int main() {
+  barrier_init(&barrier, 2);
   static __thread int Var = 42;
   pthread_t t;
   pthread_create(&t, 0, Thread, &Var);
   Var = 43;
+  barrier_wait(&barrier);
   pthread_join(t, 0);
 }
 
Index: gcc/testsuite/c-c++-common/tsan/tsan_barrier.h
===================================================================
--- gcc/testsuite/c-c++-common/tsan/tsan_barrier.h	(revision 0)
+++ gcc/testsuite/c-c++-common/tsan/tsan_barrier.h	(working copy)
@@ -0,0 +1,14 @@
+/* TSAN-invisible barriers.  Link with -ldl.  */
+#include <pthread.h>
+#include <dlfcn.h>
+
+static __typeof(pthread_barrier_wait) *barrier_wait;
+
+static
+void barrier_init (pthread_barrier_t *barrier, unsigned count)
+{
+  void *h = dlopen ("libpthread.so.0", RTLD_LAZY);
+  barrier_wait = (__typeof (pthread_barrier_wait) *)
+	 	 dlsym (h, "pthread_barrier_wait");
+  pthread_barrier_init (barrier, NULL, count);
+}
Index: gcc/testsuite/c-c++-common/tsan/write_in_reader_lock.c
===================================================================
--- gcc/testsuite/c-c++-common/tsan/write_in_reader_lock.c	(revision 219198)
+++ gcc/testsuite/c-c++-common/tsan/write_in_reader_lock.c	(working copy)
@@ -1,8 +1,10 @@
 /* { dg-shouldfail "tsan" } */
+/* { dg-additional-options "-ldl" } */
 
 #include <pthread.h>
-#include <unistd.h>
+#include "tsan_barrier.h"
 
+static pthread_barrier_t barrier;
 pthread_rwlock_t rwlock;
 int GLOB;
 
@@ -10,7 +12,7 @@
  (void)p;
   pthread_rwlock_rdlock(&rwlock);
   // Write under reader lock.
-  sleep(1);
+  barrier_wait(&barrier);
   GLOB++;
   pthread_rwlock_unlock(&rwlock);
   return 0;
@@ -17,6 +19,7 @@
 }
 
 int main(int argc, char *argv[]) {
+  barrier_init(&barrier, 2);
   pthread_rwlock_init(&rwlock, NULL);
   pthread_rwlock_rdlock(&rwlock);
   pthread_t t;
@@ -24,6 +27,7 @@
   volatile int x = GLOB;
  (void)x;
   pthread_rwlock_unlock(&rwlock);
+  barrier_wait(&barrier);
   pthread_join(t, 0);
   pthread_rwlock_destroy(&rwlock);
   return 0;
Index: gcc/testsuite/g++.dg/tsan/aligned_vs_unaligned_race.C
===================================================================
--- gcc/testsuite/g++.dg/tsan/aligned_vs_unaligned_race.C	(revision 219198)
+++ gcc/testsuite/g++.dg/tsan/aligned_vs_unaligned_race.C	(working copy)
@@ -1,11 +1,16 @@
 /* { dg-shouldfail "tsan" } */
+/* { dg-additional-options "-ldl" } */
+
 #include <pthread.h>
 #include <stdio.h>
 #include <stdint.h>
+#include "tsan_barrier.h"
 
+static pthread_barrier_t barrier;
 uint64_t Global[2];
 
 void *Thread1(void *x) {
+  barrier_wait(&barrier);
   Global[1]++;
   return NULL;
 }
@@ -15,10 +20,12 @@
   struct __attribute__((packed, aligned(1))) u_uint64_t { uint64_t val; };
   u_uint64_t *p4 = reinterpret_cast<u_uint64_t *>(p1 + 1);
   (*p4).val++;
+  barrier_wait(&barrier);
   return NULL;
 }
 
 int main() {
+  barrier_init(&barrier, 2);
   pthread_t t[2];
   pthread_create(&t[0], NULL, Thread1, NULL);
   pthread_create(&t[1], NULL, Thread2, NULL);
Index: gcc/testsuite/g++.dg/tsan/atomic_free.C
===================================================================
--- gcc/testsuite/g++.dg/tsan/atomic_free.C	(revision 219198)
+++ gcc/testsuite/g++.dg/tsan/atomic_free.C	(working copy)
@@ -1,18 +1,23 @@
 /* { dg-shouldfail "tsan" } */
+/* { dg-additional-options "-ldl" } */
 
 #include <pthread.h>
-#include <unistd.h>
+#include "tsan_barrier.h"
 
+static pthread_barrier_t barrier;
+
 void *Thread(void *a) {
   __atomic_fetch_add((int*)a, 1, __ATOMIC_SEQ_CST);
+  barrier_wait(&barrier);
   return 0;
 }
 
 int main() {
+  barrier_init(&barrier, 2);
   int *a = new int(0);
   pthread_t t;
   pthread_create(&t, 0, Thread, a);
-  sleep(1);
+  barrier_wait(&barrier);
   delete a;
   pthread_join(t, 0);
 }
Index: gcc/testsuite/g++.dg/tsan/atomic_free2.C
===================================================================
--- gcc/testsuite/g++.dg/tsan/atomic_free2.C	(revision 219198)
+++ gcc/testsuite/g++.dg/tsan/atomic_free2.C	(working copy)
@@ -1,19 +1,24 @@
 /* { dg-shouldfail "tsan" } */
+/* { dg-additional-options "-ldl" } */
 
 #include <pthread.h>
-#include <unistd.h>
+#include "tsan_barrier.h"
 
+static pthread_barrier_t barrier;
+
 void *Thread(void *a) {
-  sleep(1);
+  barrier_wait(&barrier);
   __atomic_fetch_add((int*)a, 1, __ATOMIC_SEQ_CST);
   return 0;
 }
 
 int main() {
+  barrier_init(&barrier, 2);
   int *a = new int(0);
   pthread_t t;
   pthread_create(&t, 0, Thread, a);
   delete a;
+  barrier_wait(&barrier);
   pthread_join(t, 0);
 }
 
Index: gcc/testsuite/g++.dg/tsan/cond_race.C
===================================================================
--- gcc/testsuite/g++.dg/tsan/cond_race.C	(revision 219198)
+++ gcc/testsuite/g++.dg/tsan/cond_race.C	(working copy)
@@ -1,11 +1,13 @@
 /* { dg-shouldfail "tsan" } */
+/* { dg-additional-options "-ldl" } */
 /* { dg-output "ThreadSanitizer: data race.*" } */
 /* { dg-output "pthread_cond_signal.*" } */
 
-#include <stdio.h>
-#include <stdlib.h>
 #include <pthread.h>
+#include "tsan_barrier.h"
 
+static pthread_barrier_t barrier;
+
 struct Ctx {
   pthread_mutex_t m;
   pthread_cond_t c;
@@ -18,10 +20,12 @@
   c->done = true;
   pthread_mutex_unlock(&c->m);
   pthread_cond_signal(&c->c);
+  barrier_wait(&barrier);
   return 0;
 }
 
 int main() {
+  barrier_init(&barrier, 2);
   Ctx *c = new Ctx();
   pthread_mutex_init(&c->m, 0);
   pthread_cond_init(&c->c, 0);
@@ -31,6 +35,7 @@
   while (!c->done)
     pthread_cond_wait(&c->c, &c->m);
   pthread_mutex_unlock(&c->m);
+  barrier_wait(&barrier);
   delete c;
   pthread_join(th, 0);
 }
Index: gcc/testsuite/g++.dg/tsan/tsan_barrier.h
===================================================================
--- gcc/testsuite/g++.dg/tsan/tsan_barrier.h	(revision 0)
+++ gcc/testsuite/g++.dg/tsan/tsan_barrier.h	(working copy)
@@ -0,0 +1,14 @@
+/* TSAN-invisible barriers.  Link with -ldl.  */
+#include <pthread.h>
+#include <dlfcn.h>
+
+static __typeof(pthread_barrier_wait) *barrier_wait;
+
+static
+void barrier_init (pthread_barrier_t *barrier, unsigned count)
+{
+  void *h = dlopen ("libpthread.so.0", RTLD_LAZY);
+  barrier_wait = (__typeof (pthread_barrier_wait) *)
+	 	 dlsym (h, "pthread_barrier_wait");
+  pthread_barrier_init (barrier, NULL, count);
+}

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

[Mike Stump]

On Jan 7, 2015, at 2:44 PM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote:
> Here is a new patch, that uses this method on all tsan tests,
> which seem to depend on sleep(1), and which have unstable results
> because of that.

> OK for trunk?

No.  So, I think this is wrong.  The problem is that any synchronizing primitive can trapped by tsan and added to the analysis, and if it resolves the race, then it should change the analysis that tsan produces.

The point of the atomic set, load primitives and volatile, the code-gen is a single instruction that tsan by definition, won’t now, or ever instrument because we tell it explicitly, don’t with no_sanitize_thread.

Since gcc now supports no_sanitize_thread, I don’t know of any reason why the test cases should not now be fixed to use step.

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

[Jakub Jelinek]

On Thu, Jan 08, 2015 at 11:24:21AM -0800, Mike Stump wrote:
> On Jan 7, 2015, at 2:44 PM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote:
> > Here is a new patch, that uses this method on all tsan tests,
> > which seem to depend on sleep(1), and which have unstable results
> > because of that.
> 
> > OK for trunk?
> 
> No.  So, I think this is wrong.  The problem is that any synchronizing primitive can trapped by tsan and added to the analysis, and if it resolves the race, then it should change the analysis that tsan produces.

I disagree.  Busy waiting of this kind is not appropriate for the testsuite,
we burn already way too much time and adding to that is undesirable.
tsan can't intercept the calls that you do through dlsym, because you
explicitly bypass tsan in that case.

> The point of the atomic set, load primitives and volatile, the code-gen is a single instruction that tsan by definition, won’t now, or ever instrument because we tell it explicitly, don’t with no_sanitize_thread.
> 
> Since gcc now supports no_sanitize_thread, I don’t know of any reason why the test cases should not now be fixed to use step.

See above.

	Jakub

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

[Mike Stump]

On Jan 8, 2015, at 11:29 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> I disagree.  Busy waiting of this kind is not appropriate for the test suite,

What busy waiting, there is none in the last version of the patch?

> tsan can't intercept the calls that you do through dlsym, because you
> explicitly bypass tsan in that case.

Ah, yes, right, I had pthread_barrier_wait on the brain, sorry.  The direct use of it would be problematic.  The dlopen use of it, is safe.

So, that removes the objection I had to his patch.  Jakub, since he has a complete solution to the problem submitted with all the test cases fixed, I think it should go in.

Any objections to approving it now?

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

[Jakub Jelinek]

On Thu, Jan 08, 2015 at 01:07:02PM -0800, Mike Stump wrote:
> On Jan 8, 2015, at 11:29 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> > I disagree.  Busy waiting of this kind is not appropriate for the test suite,
> 
> What busy waiting, there is none in the last version of the patch?

It was still while (... != i - 1), wasn't it?
While pthread_barrier_wait/futex would typically only wake you up when
needed.

> > tsan can't intercept the calls that you do through dlsym, because you
> > explicitly bypass tsan in that case.
> 
> Ah, yes, right, I had pthread_barrier_wait on the brain, sorry.  The direct use of it would be problematic.  The dlopen use of it, is safe.
> 
> So, that removes the objection I had to his patch.  Jakub, since he has a complete solution to the problem submitted with all the test cases fixed, I think it should go in.
> 
> Any objections to approving it now?

LGTM.

	Jakub

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

[Mike Stump]

On Jan 8, 2015, at 1:27 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> Any objections to approving it now?
> 
> LGTM.

Patch is Ok.  If you could send the clang folks a heads up, I’ve love to see them adopt the style.

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

[Bernd Edlinger]

On Thu, 8 Jan 2015 14:05:32, Mike Stump wrote:
>
> On Jan 8, 2015, at 1:27 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>>> Any objections to approving it now?
>>
>> LGTM.
>
> Patch is Ok. If you could send the clang folks a heads up, I’ve love to see them adopt the style.


Thanks, I am glad that we finally have a working solution.

Checked in as https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=219367


Dmitry, would you like to use that solution also in the clang tree, instead of sleep(1) and %deflake ?


Thanks
Bernd.
 		 	   		  

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

[Bernd Edlinger]

Hi,

On Thu, 8 Jan 2015 22:27:26, Jakub Jelinek wrote:
>> Any objections to approving it now?
>
> LGTM.
>
> Jakub

would it be OK to apply this patch also to the 4.9 testsuite,
except for c-c++-common/tsan/bitfield_race.c and 
g++.dg/tsan/aligned_vs_unaligned_race.C of course?


Bernd.
 		 	   		  

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

[Jakub Jelinek]

On Fri, Jan 09, 2015 at 04:32:47PM +0100, Bernd Edlinger wrote:
> Hi,
> 
> On Thu, 8 Jan 2015 22:27:26, Jakub Jelinek wrote:
> >> Any objections to approving it now?
> >
> > LGTM.
> >
> > Jakub
> 
> would it be OK to apply this patch also to the 4.9 testsuite,
> except for c-c++-common/tsan/bitfield_race.c and 
> g++.dg/tsan/aligned_vs_unaligned_race.C of course?

Yes, but please give Dmitry some time to respond.

	Jakub

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

[Dmitry Vyukov]

Long story short. Tsan has a logical data race the core of data race
detection algorithm. The race is not a bug, but a deliberate design
decision that makes tsan considerably faster. So ironically, if the
race memory accesses happen almost simultaneously, tsan can miss the
race.
Thus we have sleeps.
Sleeps vs barrier is being discussed in the "Fix parameters of
__tsan_vptr_update" thread.
I would really like to keep llvm and gcc tests in sync as much as possible.



On Fri, Jan 9, 2015 at 6:36 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Jan 09, 2015 at 04:32:47PM +0100, Bernd Edlinger wrote:
>> Hi,
>>
>> On Thu, 8 Jan 2015 22:27:26, Jakub Jelinek wrote:
>> >> Any objections to approving it now?
>> >
>> > LGTM.
>> >
>> > Jakub
>>
>> would it be OK to apply this patch also to the 4.9 testsuite,
>> except for c-c++-common/tsan/bitfield_race.c and
>> g++.dg/tsan/aligned_vs_unaligned_race.C of course?
>
> Yes, but please give Dmitry some time to respond.
>
>         Jakub

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

[Mike Stump]

On Jan 19, 2015, at 12:47 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
> Long story short. Tsan has a logical data race the core of data race
> detection algorithm. The race is not a bug, but a deliberate design
> decision that makes tsan considerably faster.

Could you please quantify that for us?  Also, what lockless update method did you use?  Did you try atomic increment?  On my machine, they are as cheap as stores; can’t imagine they could be slow at all.  If the latency and bandwidth of atomic increment is identical to store, would the costs be any higher than using a store to update the tsan data?  A proper port of tsan to my machine would make use of atomic increment.  I consider it a simple matter to sequence the thread termination and the output routine to ensure that all the updates in the threads happen before the output routine runs.  The output routine strikes me as slow, and thread termination strikes me as slow, so taking a little extra time there seems reasonable.  Was the excessive cost you saw due to the termination costs?

> So ironically, if the race memory accesses happen almost simultaneously, tsan can miss the
> race.
> Thus we have sleeps.

I’ve not seen a reason why the test suite should randomly fail.  The gcc test suite does not.  Could you explain why the llvm test suite does?  Surely you know that sleep is not a synchronization primitive?

> Sleeps vs barrier is being discussed in the "Fix parameters of
> __tsan_vptr_update" thread.

When finished, let us know the outcome.  To date, I’ve not seen any compelling reason to have the core of tsan be other than deterministic and the test suite other than deterministic.  I’d love to see the backing for such a decision.

> I would really like to keep llvm and gcc tests in sync as much as possible.

Excellent, from my perspective, that would mean that you make the llvm test suite deterministic.

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

[Dmitry Vyukov]

Hi Mike,

Yes, I can quantify the cost. Is it very high.

Here is the patch that I used:

--- rtl/tsan_rtl.cc (revision 226644)
+++ rtl/tsan_rtl.cc (working copy)
@@ -709,7 +709,11 @@
 ALWAYS_INLINE USED
 void MemoryAccess(ThreadState *thr, uptr pc, uptr addr,
     int kAccessSizeLog, bool kAccessIsWrite, bool kIsAtomic) {
   u64 *shadow_mem = (u64*)MemToShadow(addr);
+
+  atomic_fetch_add((atomic_uint64_t*)shadow_mem, 0, memory_order_acq_rel);
+

On the standard tsan benchmark that does 8-byte writes:
before:
[       OK ] DISABLED_BENCH.Mop8Write (1161 ms)
after:
[       OK ] DISABLED_BENCH.Mop8Write (5085 ms)

So that's 338% slowdown.





On Mon, Jan 19, 2015 at 6:12 PM, Mike Stump <mikestump@comcast.net> wrote:
> On Jan 19, 2015, at 12:47 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
>> Long story short. Tsan has a logical data race the core of data race
>> detection algorithm. The race is not a bug, but a deliberate design
>> decision that makes tsan considerably faster.
>
> Could you please quantify that for us?  Also, what lockless update method did you use?  Did you try atomic increment?  On my machine, they are as cheap as stores; can’t imagine they could be slow at all.  If the latency and bandwidth of atomic increment is identical to store, would the costs be any higher than using a store to update the tsan data?  A proper port of tsan to my machine would make use of atomic increment.  I consider it a simple matter to sequence the thread termination and the output routine to ensure that all the updates in the threads happen before the output routine runs.  The output routine strikes me as slow, and thread termination strikes me as slow, so taking a little extra time there seems reasonable.  Was the excessive cost you saw due to the termination costs?
>
>> So ironically, if the race memory accesses happen almost simultaneously, tsan can miss the
>> race.
>> Thus we have sleeps.
>
> I’ve not seen a reason why the test suite should randomly fail.  The gcc test suite does not.  Could you explain why the llvm test suite does?  Surely you know that sleep is not a synchronization primitive?
>
>> Sleeps vs barrier is being discussed in the "Fix parameters of
>> __tsan_vptr_update" thread.
>
> When finished, let us know the outcome.  To date, I’ve not seen any compelling reason to have the core of tsan be other than deterministic and the test suite other than deterministic.  I’d love to see the backing for such a decision.
>
>> I would really like to keep llvm and gcc tests in sync as much as possible.
>
> Excellent, from my perspective, that would mean that you make the llvm test suite deterministic.

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

[Jakub Jelinek]

On Wed, Jan 21, 2015 at 12:23:34PM +0400, Dmitry Vyukov wrote:
> Hi Mike,
> 
> Yes, I can quantify the cost. Is it very high.
> 
> Here is the patch that I used:
> 
> --- rtl/tsan_rtl.cc (revision 226644)
> +++ rtl/tsan_rtl.cc (working copy)
> @@ -709,7 +709,11 @@
>  ALWAYS_INLINE USED
>  void MemoryAccess(ThreadState *thr, uptr pc, uptr addr,
>      int kAccessSizeLog, bool kAccessIsWrite, bool kIsAtomic) {
>    u64 *shadow_mem = (u64*)MemToShadow(addr);
> +
> +  atomic_fetch_add((atomic_uint64_t*)shadow_mem, 0, memory_order_acq_rel);

And the cost of adding that atomic_fetch_add guarded by
if (__builtin_expect (someCondition, 0)) ?
If that doesn't slow down the non-deterministic default case too much,
that would allow users to choose what they prefer - much faster unreliable
and slower deterministic.  Then for the gcc testsuite we could opt for the
latter.

> +
> 
> On the standard tsan benchmark that does 8-byte writes:
> before:
> [       OK ] DISABLED_BENCH.Mop8Write (1161 ms)
> after:
> [       OK ] DISABLED_BENCH.Mop8Write (5085 ms)
> 
> So that's 338% slowdown.

	Jakub

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

[Dmitry Vyukov]

On Wed, Jan 21, 2015 at 11:34 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Jan 21, 2015 at 12:23:34PM +0400, Dmitry Vyukov wrote:
>> Hi Mike,
>>
>> Yes, I can quantify the cost. Is it very high.
>>
>> Here is the patch that I used:
>>
>> --- rtl/tsan_rtl.cc (revision 226644)
>> +++ rtl/tsan_rtl.cc (working copy)
>> @@ -709,7 +709,11 @@
>>  ALWAYS_INLINE USED
>>  void MemoryAccess(ThreadState *thr, uptr pc, uptr addr,
>>      int kAccessSizeLog, bool kAccessIsWrite, bool kIsAtomic) {
>>    u64 *shadow_mem = (u64*)MemToShadow(addr);
>> +
>> +  atomic_fetch_add((atomic_uint64_t*)shadow_mem, 0, memory_order_acq_rel);
>
> And the cost of adding that atomic_fetch_add guarded by
> if (__builtin_expect (someCondition, 0)) ?
> If that doesn't slow down the non-deterministic default case too much,
> that would allow users to choose what they prefer - much faster unreliable
> and slower deterministic.  Then for the gcc testsuite we could opt for the
> latter.

It's more complex than this.

1. In reality the cost can be higher. We will need to construct a
spin-mutex in shadow. Thus there will be contention between threads
(both waiting for the mutex and cache-line contention).
Currently threads don't always write to shadow (just read), so the
cache line can actually be shared between several threads accessing
the same shadow.

2. We don't have a spare bit in shadow for the mutex.

3. Asynchronous shadow memory flush can turn arbitrary regions of
shadow memory into zeroes. The shadow is especially designed to
tolerate this. This does not play well with mutexes.

4. That won't make multi-threaded programs deterministic. First, there
is at least one another issue in tsan -- it has only 4 slots to
remember previous memory accesses, they are evicted in effectively
random manner. Then, multi-threaded programs are non-deterministic by
themselves.

So there are lots of technical problems, significant slowdown and no
value for end users. I don't want to solve it for tests. The invisible
barrier is the right solution to make tests deterministic.



>> On the standard tsan benchmark that does 8-byte writes:
>> before:
>> [       OK ] DISABLED_BENCH.Mop8Write (1161 ms)
>> after:
>> [       OK ] DISABLED_BENCH.Mop8Write (5085 ms)
>>
>> So that's 338% slowdown.
>
>         Jakub

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

[Mike Stump]

[-- Attachment #1: Type: text/plain, Size: 202 bytes --]

On Jan 7, 2015, at 12:23 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> 
> I'm fine with adding the no_sanitize_thread attribute, the patch LGTM

Thanks, I checked it in, the doc seemed trivial enough.


[-- Attachment #2: tsan-attr-2.diffs.txt --]
[-- Type: text/plain, Size: 2998 bytes --]

	* tsan.c (pass_tsan::gate): Add no_sanitize_thread support.
	(pass_tsan_O0::gate): Likewise.
	* extend.texi (Function Attributes): Add no_sanitize_thread
	documentation.

c-family:
	* c-common.c (c_common_attribute_table): Add no_sanitize_thread.

Index: doc/extend.texi
===================================================================
--- doc/extend.texi	(revision 219354)
+++ doc/extend.texi	(revision 219355)
@@ -2205,6 +2205,7 @@ attributes are currently defined for fun
 @code{returns_nonnull}, @code{gnu_inline},
 @code{externally_visible}, @code{hot}, @code{cold}, @code{artificial},
 @code{no_sanitize_address}, @code{no_address_safety_analysis},
+@code{no_sanitize_thread},
 @code{no_sanitize_undefined}, @code{no_reorder}, @code{bnd_legacy},
 @code{bnd_instrument},
 @code{error} and @code{warning}.
@@ -3721,6 +3722,12 @@ The @code{no_address_safety_analysis} is
 @code{no_sanitize_address} attribute, new code should use
 @code{no_sanitize_address}.
 
+@item no_sanitize_thread
+@cindex @code{no_sanitize_thread} function attribute
+The @code{no_sanitize_thread} attribute on functions is used
+to inform the compiler that it should not instrument memory accesses
+in the function when compiling with the @option{-fsanitize=thread} option.
+
 @item no_sanitize_undefined
 @cindex @code{no_sanitize_undefined} function attribute
 The @code{no_sanitize_undefined} attribute on functions is used
Index: c-family/c-common.c
===================================================================
--- c-family/c-common.c	(revision 219354)
+++ c-family/c-common.c	(revision 219355)
@@ -764,6 +764,9 @@ const struct attribute_spec c_common_att
   { "no_sanitize_address",    0, 0, true, false, false,
 			      handle_no_sanitize_address_attribute,
 			      false },
+  { "no_sanitize_thread",     0, 0, true, false, false,
+			      handle_no_sanitize_address_attribute,
+			      false },
   { "no_sanitize_undefined",  0, 0, true, false, false,
 			      handle_no_sanitize_undefined_attribute,
 			      false },
Index: tsan.c
===================================================================
--- tsan.c	(revision 219354)
+++ tsan.c	(revision 219355)
@@ -868,7 +868,9 @@ public:
   opt_pass * clone () { return new pass_tsan (m_ctxt); }
   virtual bool gate (function *)
 {
-  return (flag_sanitize & SANITIZE_THREAD) != 0;
+  return ((flag_sanitize & SANITIZE_THREAD) != 0
+	  && !lookup_attribute ("no_sanitize_thread",
+                                DECL_ATTRIBUTES (current_function_decl)));
 }
 
   virtual unsigned int execute (function *) { return tsan_pass (); }
@@ -908,7 +910,9 @@ public:
   /* opt_pass methods: */
   virtual bool gate (function *)
     {
-      return (flag_sanitize & SANITIZE_THREAD) != 0 && !optimize;
+      return ((flag_sanitize & SANITIZE_THREAD) != 0 && !optimize
+	      && !lookup_attribute ("no_sanitize_thread",
+				    DECL_ATTRIBUTES (current_function_decl)));
     }
 
   virtual unsigned int execute (function *) { return tsan_pass (); }

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

[Mike Stump]

On Jan 6, 2015, at 11:17 PM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote:
> 
> On Tue, 6 Jan 2015 16:32:35, Mike Stump wrote:
>> 
>> On Jan 6, 2015, at 3:22 PM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote:
>>> Yes, I think too that it can't fail under these conditions.
>> 
>> If you mean your version… A lot has been written on how to to make racy code non-racy… I’d refer you to the literature on all the various solutions people have found to date. I don’t think I’ve ever seen anyone claim that affinity can be used to solve race conditions like this. Do you have a pointer?
>> 
> 
> no, I agree, the affinity would just lower the likelihood, as sleep does.
> The version with affinity is just disgusting, sorry to have posted it ;-).
> 
> I meant your version, with step(1)/step(2) between the race. It is probably bullet-proof.
> But the version with sleep is more realistic.

> sleep_sync.c can use step but needs still to call sleep because it triggers on the
> "As if synchronized via sleep" diagnostic.

Ah, thanks for the pointer.  Yeah, it is important to not remove that sleep, though, if the test case itself is racy, we can use step to ensure the elements of it happen in a deterministic order.

> I don't know what to make of simple_race.c, which uses usleep, but uses
> a loop to repeat the race often. Maybe leave that one as a "realistic" test.

I looked at that test case to see if it is flaky (llvm term) and found:

http://reviews.llvm.org/D3913

> The tests test that ThreadSanitizer finds the data race in particular conditions. However, ThreadSanitizer core algorithm can miss a data race when the racy memory access attempts happen very close to each other (literally simultaneously). This was done intentionally, fixing this would impose significant slowdown and this is not a problem for programs other than unit tests.

So, clearly your presentation of the base problem is accurate.  I still doubt the costs of a good solution are all that expensive.

Anyway, http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140526/219172.html makes it clear that this test case is flakey (and should be fixed).

I’d fix it by removing all looping and making it deterministic (with step).  We can leave the usleep in there, it doesn’t hurt; though, not sure it adds anything.

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

[Mike Stump]

[-- Attachment #1: Type: text/plain, Size: 784 bytes --]

On Jan 5, 2015, at 5:06 PM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote:
> 
> I tried to elaborate your idea a bit, and used proper atomics.
> It is necessary that the logic of the step function is completely invisible to tsan.

> Therefore it should not call sleep,

Again, sleep can’t fix race conditions, so therefore tsan can never use it as an indication that no race exists.  If it ever did, that would be a bug.  The only case where that would not be true, would be a hard real time analysis verifier, and, when we get one, sleep can be so modified.

> because that can be intercepted, even if the code is not instrumented.

I don’t see the relevance.

> Unfortunately there is no __attribute__((no_sanitize_thread))

So, is that any harder to add then:


[-- Attachment #2: tsan-attr.diffs.txt --]
[-- Type: text/plain, Size: 1540 bytes --]

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index ce141b6..58bdc9e 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -766,6 +766,9 @@ const struct attribute_spec c_common_attribute_table[] =
   { "no_sanitize_address",    0, 0, true, false, false,
 			      handle_no_sanitize_address_attribute,
 			      false },
+  { "no_sanitize_thread",     0, 0, true, false, false,
+			      handle_no_sanitize_address_attribute,
+			      false },
   { "no_sanitize_undefined",  0, 0, true, false, false,
 			      handle_no_sanitize_undefined_attribute,
 			      false },
diff --git a/gcc/tsan.c b/gcc/tsan.c
index 678fcdc..cafb150 100644
--- a/gcc/tsan.c
+++ b/gcc/tsan.c
@@ -758,7 +758,9 @@ public:
   opt_pass * clone () { return new pass_tsan (m_ctxt); }
   virtual bool gate (function *)
 {
-  return (flag_sanitize & SANITIZE_THREAD) != 0;
+  return ((flag_sanitize & SANITIZE_THREAD) != 0
+	  && !lookup_attribute ("no_sanitize_thread",
+                                DECL_ATTRIBUTES (current_function_decl)));
 }
 
   virtual unsigned int execute (function *) { return tsan_pass (); }
@@ -798,7 +800,9 @@ public:
   /* opt_pass methods: */
   virtual bool gate (function *)
     {
-      return (flag_sanitize & SANITIZE_THREAD) != 0 && !optimize;
+      return ((flag_sanitize & SANITIZE_THREAD) != 0 && !optimize
+	      && !lookup_attribute ("no_sanitize_thread",
+				    DECL_ATTRIBUTES (current_function_decl)));
     }
 
   virtual unsigned int execute (function *) { return tsan_pass (); }

[-- Attachment #3: Type: text/plain, Size: 959 bytes --]



?  Doesn’t look to be much an issue, but, I’m not a tsan code-gen person, so they might have to weigh in.

> So some tests start to fail.
> 
> FAIL: g++.dg/tsan/aligned_vs_unaligned_race.C   -O0  execution test
> FAIL: g++.dg/tsan/aligned_vs_unaligned_race.C   -O2  execution test

I don’t see this when I do this:

__attribute__((no_sanitize_thread))
void step (int i)
{
  while (__atomic_load_n (&serial, __ATOMIC_ACQUIRE) != i - 1) ;
  __atomic_store_n (&serial, i, __ATOMIC_RELEASE);
}

to step.

Any issues just adding no_sanitize_thread to let us do what we want to do?

And the last point, should we add a sched_yield or a pthread_yield to the busy loop to be nice:

__attribute__((no_sanitize_thread))
void step (int i)
{
  while (__atomic_load_n (&serial, __ATOMIC_ACQUIRE) != i - 1)
    sched_yield ();
  __atomic_store_n (&serial, i, __ATOMIC_RELEASE);
}

?  One a single core, this seems like it would run faster.

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

[Mike Stump]

On Jan 3, 2015, at 3:20 AM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote:
> Yes, but all other tsan test cases call sleep(1) too.

Ick.  The problem is pushing bugs to obscure them without fixing them, makes the system inherently less good.

If there is already one bug that documents the problem that the sleep cures then it is fine to so obscure the bug.  At least 1 test case should remain that isn’t obscured.  Indeed, if you can enhance the failure rate of the test case that shows the failure, ideally to 1/1, that would be great.  There is no need to have more than one test case that doesn’t have the sleep, but, all the test cases with the sleep should have a comment on sleep that points to the PR that documents the problem.

[ reviewing all the c++ tsan cases ]

Ah, even more curious.  So, for testing, it would be best if we had a way to synchronize the threads so that they reliably can show what you want to show, and reliably pick which thread will run first and which second and so on.  The problem is of course, the synchronization primitives can’t get in the way (be seen by) of normal tsan functioning.  I’m thinking asm() can so obscure arbitrary things, but I’m not sure I can’t think of a way to do that with anything less.  sleep is close, and, at least portable and simple.  What it isn’t is bullet proof.  The tsan test cases would be enhanced if we could find a way to synchronize the threads invisibly to tsan.  I’d like to think that someone can propose a better scheme than sleep.  My thought would be something like:

int order = 0;

void sync(int i) {
  while (++order != i) {  /* atomic inc */
    --order; /* atomic dec */
    sleep(1);  /* or some type of operative yield */
  }
}



thread 1:
  asm (“call sync(1)”);
  action1;
  asm (“call sync(3)”);
  action3;

thread 2:

  asm (“call sync(2)”);
  action2;

where the order executed would be action1, action2, action3.  The asm hides all details of the synchronization from everyone, optimizer, tsan.

Now, why go to all this work?  Simply, determinism in gcc and the test suite, is, well, nice.  I understand that user programs won’t be so nice, and that in the wild, it won’t be 100%.

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

[Jakub Jelinek]

On Sun, Jan 04, 2015 at 11:04:34AM -0800, Mike Stump wrote:
> On Jan 3, 2015, at 3:20 AM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote:
> > Yes, but all other tsan test cases call sleep(1) too.
> 
> Ick.  The problem is pushing bugs to obscure them without fixing them, makes the system inherently less good.
> 
> If there is already one bug that documents the problem that the sleep cures then it is fine to so obscure the bug.  At least 1 test case should remain that isn’t obscured.  Indeed, if you can enhance the failure rate of the test case that shows the failure, ideally to 1/1, that would be great.  There is no need to have more than one test case that doesn’t have the sleep, but, all the test cases with the sleep should have a comment on sleep that points to the PR that documents the problem.
> 
> [ reviewing all the c++ tsan cases ]
> 
> Ah, even more curious.  So, for testing, it would be best if we had a way to synchronize the threads so that they reliably can show what you want to show, and reliably pick which thread will run first and which second and so on.  The problem is of course, the synchronization primitives can’t get in the way (be seen by) of normal tsan functioning.  I’m thinking asm() can so obscure arbitrary things, but I’m not sure I can’t think of a way to do that with anything less.  sleep is close, and, at least portable and simple.  What it isn’t is bullet proof.  The tsan test cases would be enhanced if we could find a way to synchronize the threads invisibly to tsan.  I’d like to think that someone can propose a better scheme than sleep.  My thought would be something like:

Yeah, I think it was just a bad idea to add testsuite for something that is
racy.  So, either we should disable tsan testsuite by default, or change
libsanitize/tsan/, so that it can be built as a perhaps slower, but not racy
variant, and test only that variant.

	Jakub

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

[Bernd Edlinger]

On Sun, 4 Jan 2015 11:04:34, Mike Stump wrote:

>
> Ah, even more curious. So, for testing, it would be best if we had a way to synchronize the threads so that they reliably can show what you want to show, and reliably pick which thread will run first and which second and so on. The problem is of course, the synchronization primitives can’t get in the way (be seen by) of normal tsan functioning. I’m thinking asm() can so obscure arbitrary things, but I’m not sure I can’t think of a way to do that with anything less. sleep is close, and, at least portable and simple. What it isn’t is bullet proof. The tsan test cases would be enhanced if we could find a way to synchronize the threads invisibly to tsan. I’d like to think that someone can propose a better scheme than sleep. My thought would be something like:
>
> int order = 0;
>
> void sync(int i) {
> while (++order != i) { /* atomic inc */
> --order; /* atomic dec */
> sleep(1); /* or some type of operative yield */
> }
> }
>
>
>
> thread 1:
> asm (“call sync(1)”);
> action1;
> asm (“call sync(3)”);
> action3;
>
> thread 2:
>
> asm (“call sync(2)”);
> action2;
>
> where the order executed would be action1, action2, action3. The asm hides all details of the synchronization from everyone, optimizer, tsan.
>
> Now, why go to all this work? Simply, determinism in gcc and the test suite, is, well, nice. I understand that user programs won’t be so nice, and that in the wild, it won’t be 100%.

Hmm,,,,

that could work....

I would need a way to link the test case two helper functions, that are not compiled with -fsanitize=thread

like tsan-helper.C
void set(int * a , int b)
{
  *a = b;
}
int get(int *a)
{
  return *a;
}

uint64_t Global[2];
int z=0;
int get(int*);
void set(int*,int);

void *Thread1(void *x) {
  /* We have to sleep here, to make it somewhat easier for tsan to
     detect the race condition.  */
  while (get(&z) == 0);
  Global[1]++;
  return NULL;
}

void *Thread2(void *x) {
  char *p1 = reinterpret_cast<char *>(&Global[0]);
  struct __attribute__((packed, aligned(1))) u_uint64_t { uint64_t val; };
  u_uint64_t *p4 = reinterpret_cast<u_uint64_t *>(p1 + 1);
  (*p4).val++;
  set(&z,1);
  return NULL;
}


I tried, it and it works 10.000 times without one failure.

But I have no idea how to do that in our test framework.



Bernd.
 		 	   		  

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

[Mike Stump]

On Jan 4, 2015, at 1:48 PM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote:
> I would need a way to link the test case two helper functions, that are not compiled with -fsanitize=thread

/* { dg-additional-sources “tsan-helper.c" } */

might be one step forward to do that.  I don’t know off hand about the options.  I think you may be able to specify unique options per file.

> I tried, it and it works 10.000 times without one failure.

So, statistically knowing it works in practice is nice.  However, the standard is, does is work by design?  I’ll let you comment on that.  The sync approach I designed to work in any case, no matter the complexity, by design.  I’d assume that you’ve engineered it to work by design.

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

[Bernd Edlinger]

On Sun, 4 Jan 2015 13:57:38, Mike Stump wrote:
>
> On Jan 4, 2015, at 1:48 PM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote:
>> I would need a way to link the test case two helper functions, that are not compiled with -fsanitize=thread
>
> /* { dg-additional-sources “tsan-helper.c" } */
>
> might be one step forward to do that. I don’t know off hand about the options. I think you may be able to specify unique options per file.
>
>> I tried, it and it works 10.000 times without one failure.
>
> So, statistically knowing it works in practice is nice. However, the standard is, does is work by design? I’ll let you comment on that. The sync approach I designed to work in any case, no matter the complexity, by design. I’d assume that you’ve engineered it to work by design.

IMO Yes, but Dmitry may know better. Tsan does not acquire a mutex _before_ it detects a race.
And race conditions that happen exactly synchronous may be invisible.

If we use some hidden mechanism to ensure that certain events happen in a certain sequence,
that would make a big difference. It would work much better than sleep(), we only need sleep if
we need to see the "as if synchronized via sleep" diagnostic.

I see now, that our tsan tests are almost verbatim copies of the LLVM test suite.

They _do_ have problems with the stability of positive tests, especially under stress,
I see all positive tests are called by a "deflake" tool, that exercises the test 10 times,
until it eventually succeeds.

I see one other test (which we apparently do not have here),
was changed recently from sleep(2) to sleep(4)

[Tsan] Fix the atomic_race.cc test to pass on systems with high loads
Differential Revision: http://reviews.llvm.org/D6478

--- compiler-rt/trunk/test/tsan/atomic_race.cc  2014/05/30 14:08:51     209898
+++ compiler-rt/trunk/test/tsan/atomic_race.cc  2014/12/02 15:04:39     223122
@@ -36,7 +36,7 @@
   for (int i = 0; i < kTestCount; i++) {
     Test(i, &atomics[i], false);
   }
-  sleep(2);
+  sleep(4);
   for (int i = 0; i < kTestCount; i++) {
     fprintf(stderr, "Test %d reverse\n", i);
     Test(i, &atomics[kTestCount + i], false);



Thanks
Bernd.