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
