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

["Martin Liška" <mliska@suse.cz>]

On 08/05/2016 02:38 PM, Nathan Sidwell wrote:
> 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

Ok, after all the experimenting and inventing "almost" thread-safe code, I incline to not to include __gcov_one_value_profiler_body_atomic
counter. The final solution is cumbersome and probably does not worth doing. Moreover, even having a thread-safe implementation, result of an indirect call counter
is not going to be stable among different runs (due to a single value storage capability).

If you agree, I'll prepare a final version of patch?

Thanks,
Martin