Re: [PATCH] Fix sporadic failure in g++.dg/tsan/aligned_vs_unaligned_race.C
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
On Wed, Jan 21, 2015 at 11:34 AM, Jakub Jelinek ja...@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
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 mikest...@comcast.net wrote:
On Jan 19, 2015, at 12:47 AM, Dmitry Vyukov dvyu...@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
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 ja...@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
On Jan 19, 2015, at 12:47 AM, Dmitry Vyukov dvyu...@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
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
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
On Jan 8, 2015, at 1:27 PM, Jakub Jelinek ja...@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
On Thu, 8 Jan 2015 14:05:32, Mike Stump wrote: On Jan 8, 2015, at 1:27 PM, Jakub Jelinek ja...@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=revisionrevision=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
On Jan 7, 2015, at 12:23 AM, Jakub Jelinek ja...@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.
* 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
On Jan 8, 2015, at 11:29 AM, Jakub Jelinek ja...@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
On Thu, Jan 08, 2015 at 01:07:02PM -0800, Mike Stump wrote: On Jan 8, 2015, at 11:29 AM, Jakub Jelinek ja...@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
On Jan 7, 2015, at 2:44 PM, Bernd Edlinger bernd.edlin...@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
On Thu, Jan 08, 2015 at 11:24:21AM -0800, Mike Stump wrote: On Jan 7, 2015, at 2:44 PM, Bernd Edlinger bernd.edlin...@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
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.edlin...@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
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
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. testsuite/ChangeLog: 2015-01-07 Bernd Edlinger bernd.edlin...@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. patch-tsan-tests.diff Description: Binary data
Re: [PATCH] Fix sporadic failure in g++.dg/tsan/aligned_vs_unaligned_race.C
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
On Jan 7, 2015, at 12:23 AM, Jakub Jelinek ja...@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
On Wed, Jan 07, 2015 at 08:58:04AM -0800, Mike Stump wrote: On Jan 7, 2015, at 12:23 AM, Jakub Jelinek ja...@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
On Jan 6, 2015, at 11:17 PM, Bernd Edlinger bernd.edlin...@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.edlin...@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
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
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 ja...@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_castu_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
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
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
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
On Jan 6, 2015, at 9:45 AM, Bernd Edlinger bernd.edlin...@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
On Jan 6, 2015, at 3:22 PM, Bernd Edlinger bernd.edlin...@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
On Tue, 6 Jan 2015 11:47:30, Mike Stump wrote:
On Jan 6, 2015, at 9:45 AM, Bernd Edlinger bernd.edlin...@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
On Jan 5, 2015, at 5:06 PM, Bernd Edlinger bernd.edlin...@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:
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 (); }
? 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
On Tue, 6 Jan 2015 16:32:35, Mike Stump wrote: On Jan 6, 2015, at 3:22 PM, Bernd Edlinger bernd.edlin...@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
Date: Tue, 6 Jan 2015 10:16:33 +0100
From: ja...@redhat.com
To: bernd.edlin...@hotmail.de
CC: mikest...@comcast.net; hjl.to...@gmail.com; gcc-patches@gcc.gnu.org;
dvyu...@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.edlin...@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
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
On Jan 5, 2015, at 12:58 PM, Mike Stump mikest...@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_castu_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
Hi Mike,
On Mon, 5 Jan 2015 14:01:42, Mike Stump wrote:
On Jan 5, 2015, at 12:58 PM, Mike Stump mikest...@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_castu_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.
Re: [PATCH] Fix sporadic failure in g++.dg/tsan/aligned_vs_unaligned_race.C
On Jan 5, 2015, at 12:49 AM, Bernd Edlinger bernd.edlin...@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_castchar *(Global[0]);
struct __attribute__((packed, aligned(1))) u_uint64_t { uint64_t val; };
u_uint64_t *p4 = reinterpret_castu_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
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. patch-tsan-race.diff Description: Binary data
Re: [PATCH] Fix sporadic failure in g++.dg/tsan/aligned_vs_unaligned_race.C
On Jan 3, 2015, at 3:20 AM, Bernd Edlinger bernd.edlin...@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
On Jan 4, 2015, at 9:00 AM, Bernd Edlinger bernd.edlin...@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
On Sun, Jan 04, 2015 at 11:04:34AM -0800, Mike Stump wrote: On Jan 3, 2015, at 3:20 AM, Bernd Edlinger bernd.edlin...@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
On Sun, Jan 04, 2015 at 11:07:31AM -0800, Mike Stump wrote: On Jan 4, 2015, at 9:00 AM, Bernd Edlinger bernd.edlin...@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
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.edlin...@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
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_castchar *(Global[0]);
struct __attribute__((packed, aligned(1))) u_uint64_t { uint64_t val; };
u_uint64_t *p4 = reinterpret_castu_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
On Jan 4, 2015, at 1:48 PM, Bernd Edlinger bernd.edlin...@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
On Jan 4, 2015, at 11:44 AM, Bernd Edlinger bernd.edlin...@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.edlin...@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
On Sun, 4 Jan 2015 13:57:38, Mike Stump wrote:
On Jan 4, 2015, at 1:48 PM, Bernd Edlinger bernd.edlin...@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.
RE: [PATCH] Fix sporadic failure in g++.dg/tsan/aligned_vs_unaligned_race.C
On Sat, 3 Jan 2015 01:51:34, Mike Strump wrote: On Jan 3, 2015, at 1:01 AM, Bernd Edlinger bernd.edlin...@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
On Jan 3, 2015, at 1:01 AM, Bernd Edlinger bernd.edlin...@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.
