Re: [PATCH 1/4] Cherry-pick fprofile-generate-atomic from google/gcc-4_9 branch

[Nathan Sidwell <nathan at acm dot org>]

On 08/05/16 04:55, Martin Liška wrote:

> Thank you for very intensive brainstorming ;) Well I still believe that following code
> is not thread safe, let's image following situation:

yeah, you're right.

> > 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, ...);

Bah.  but (a) does it matter enough? and (b) if so does changing the delta<0 
handling to store a count of 1 solve it?: (answer: no)

gcov_t expected, val;
A:atomic_load (&counter[0],  &val, ...);
gcov_t delta = val == value ? 1 : -1;
B:atomic_add (&counter[1], delta);

if (delta < 0) {
     /* can we set counter[0]? */
     C:atomic_load (&counter[1], &expected, ...);
     if (expected < 0) {
       D:atomic_store (&counter[0], &value);
       E: atomic_store (&counter[1], 1);
  }
atomic_add (&counter[1], 2, ...);


thread-1: value = 1, reads '1' at A
thread-2: value = 2, reads '1' at A
thread-2: decrements count @ B
thread-2: reads -1 at C
thread-2: write 2 at D
thread-2: stores 1 at E
thread-1: increments count @ B (finally)

So we still can go awry.  But the code's simpler.  Like you said, I don't think 
it's possible to solve without an atomic update to both counter[0] & counter[1].


> Well, I wrote attached test-case which should trigger a data-race, but TSAN is silent:

I'm not too surprised.  The race window is tiny and you put a printf in the 
middle of one path.  I suspect if you put a sleep/printf on the counter[1] 
increment path you'll see it more often.

nathan