On 08/04/2016 07:03 PM, Nathan Sidwell wrote:
> On 08/04/16 12:43, Nathan Sidwell wrote:
>
>> How about:
>> gcov_t expected;
>> atomic_load (&counter[0], val, ...);
>> gcov_t delta = val == value ? 1 : -1;
>> atomic_add (&counter[1], delta); <-- or atomic_add_fetch
>> if (delta < 0) {
>> /* can we set counter[0]? */
>> atomic_load (&counter[1], &expected, ...);
>> if (expected < 0) {
>> atomic_store (&counter[0], value, ...);
>> atomic_add (&counter[1], 2, ...);
>> }
>> }
>> atomic_add (&counter[2], 1, ...);
>
Hi.
Thank you for very intensive brainstorming ;) Well I still believe that
following code
is not thread safe, let's image following situation:
> we could do better by using compare_exchange storing value, and detect the
> race I mentioned:
>
> gcov_t expected, val;
> atomic_load (&counter[0], &val, ...);
[thread 1]: value == 1, read val == 1 // scheduled here
> gcov_t delta = val == value ? 1 : -1;
> atomic_add (&counter[1], delta);
> if (delta < 0) {
> retry:
> /* can we set counter[0]? */
> atomic_load (&counter[1], &expected, ...);
> if (expected < 0) {
> bool stored = atomic_compare_exchange (&counter[0], &val, &value, ...);
> if (!stored && val != value)
> goto retry;
[thread 2]: value == 2, just updated counter[0] to 2
// after that [thread 1] continue, but wrongly does counter[1]++, but value !=
counter[0]
> atomic_add (&counter[1], 2, ...);
> }
> }
> atomic_add (&counter[2], 1, ...);
>
> This corrects the off-by one issue.
>
> nathan
Well, I wrote attached test-case which should trigger a data-race, but TSAN is
silent:
$ g++ race.cc -pthread -fprofile-generate -g -fsanitize=thread
-fprofile-update=atomic
$ ./a.out
In main: creating thread 0
In main: creating thread 1
new counter[1] value, N:0
In main: creating thread 2
new counter[1] value, N:1
new counter[1] value, N:2
new counter[1] value, N:3
new counter[1] value, N:4
new counter[1] value, N:5
new counter[1] value, N:6
new counter[1] value, N:7
new counter[1] value, N:8
new counter[1] value, N:9
new counter[1] value, N:10
new counter[1] value, N:11
new counter[1] value, N:12
new counter[1] value, N:12
new counter[1] value, N:13
new counter[1] value, N:14
new counter[1] value, N:15
new counter[1] value, N:16
In main: creating thread 3
In main: creating thread 4
In main: creating thread 5
In main: creating thread 6
In main: creating thread 7
In main: creating thread 8
In main: creating thread 9
In main: creating thread 10
In main: creating thread 11
In main: creating thread 12
In main: creating thread 13
In main: creating thread 14
In main: creating thread 15
However, not updating arc counters with atomic operations causes really many
races:
$ g++ race.cc -pthread -fprofile-generate -g -fsanitize=thread
$ ./a.out 2>&1 | grep 'data race' | wc -l
110
Sample:
WARNING: ThreadSanitizer: data race (pid=11424)
Read of size 8 at 0x000000606718 by thread T4:
#0 A::foo() /tmp/race.cc:10 (a.out+0x000000401e78)
Previous write of size 8 at 0x000000606718 by thread T1:
[failed to restore the stack]
Location is global '__gcov0._ZN1A3fooEv' of size 16 at 0x000000606710
(a.out+0x000000606718)
Thread T4 (tid=11429, running) created by main thread at:
#0 pthread_create ../../../../libsanitizer/tsan/tsan_interceptors.cc:876
(libtsan.so.0+0x00000002ad2d)
#1 main /tmp/race.cc:43 (a.out+0x000000401afb)
Thread T1 (tid=11426, finished) created by main thread at:
#0 pthread_create ../../../../libsanitizer/tsan/tsan_interceptors.cc:876
(libtsan.so.0+0x00000002ad2d)
#1 main /tmp/race.cc:43 (a.out+0x000000401afb)
Maybe I miss something and my tester sample is wrong (please apply attached
patch to use original __gcov_one_value_profiler_body)?
Thanks,
Martin
diff --git a/gcc/tree-profile.c b/gcc/tree-profile.c
index 6e96fa9..42b780f 100644
--- a/gcc/tree-profile.c
+++ b/gcc/tree-profile.c
@@ -188,8 +188,8 @@ gimple_init_edge_profiler (void)
profiler_fn_name = "__gcov_indirect_call_profiler_v2";
if (PARAM_VALUE (PARAM_INDIR_CALL_TOPN_PROFILE))
profiler_fn_name = "__gcov_indirect_call_topn_profiler";
- if (flag_profile_update == PROFILE_UPDATE_ATOMIC)
- profiler_fn_name = "__gcov_indirect_call_profiler_v2_atomic";
+// if (flag_profile_update == PROFILE_UPDATE_ATOMIC)
+// profiler_fn_name = "__gcov_indirect_call_profiler_v2_atomic";
tree_indirect_call_profiler_fn
= build_fn_decl (profiler_fn_name, ic_profiler_fn_type);
diff --git a/libgcc/libgcov-profiler.c b/libgcc/libgcov-profiler.c
index c1e287d..d43932e 100644
--- a/libgcc/libgcov-profiler.c
+++ b/libgcc/libgcov-profiler.c
@@ -70,6 +70,8 @@ __gcov_pow2_profiler (gcov_type *counters, gcov_type value)
In any case, COUNTERS[2] is incremented. */
+static int counter = 0;
+
static inline void
__gcov_one_value_profiler_body (gcov_type *counters, gcov_type value)
{
@@ -77,6 +79,7 @@ __gcov_one_value_profiler_body (gcov_type *counters, gcov_type value)
counters[1]++;
else if (counters[1] == 0)
{
+ fprintf (stderr, "new counter[1] value, N:%d\n", counter++);
counters[1] = 1;
counters[0] = value;
}
#include <pthread.h>
#include <stdio.h>
#include <stdlib.h>
#define NUM_THREADS 16
#define ITERATIONS (1 * 100 * 1000)
struct A
{
virtual void foo() { }
};
struct B: A
{
virtual void foo() { }
};
static int counter = 0;
void *PrintHello(void *p)
{
A *a = new A();
B *b = new B();
for (unsigned i = 0; i < ITERATIONS; i++)
{
A *ptr = i % 2 ? a : b;
ptr->foo();
// uncommenting this produces a data race seen by tsan
// counter++;
}
}
int main(int argc, char *argv[])
{
A *a = new A();
pthread_t threads[NUM_THREADS];
int rc;
long t;
for(t=0;t<NUM_THREADS;t++){
printf("In main: creating thread %ld\n", t);
rc = pthread_create(&threads[t], NULL, PrintHello, a);
if (rc){
printf("ERROR; return code from pthread_create() is %d\n", rc);
exit(-1);
}
}
int retval;
for(t=0;t<NUM_THREADS;t++)
pthread_join (threads[t], (void**)&retval);
}
