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