On 08/04/16 11:34, Martin Liška wrote:
On 08/04/2016 04:48 PM, Nathan Sidwell wrote:
diff --git a/libgcc/libgcov-profiler.c b/libgcc/libgcov-profiler.c
+static inline void
+__gcov_one_value_profiler_body_atomic (gcov_type *counters, gcov_type value)
+{
...
The body looks to have data races. Some kind of cmp_store needed on
counters[1]? Maybe it can't be completely race free?
nathan
You are right, as we would need to atomically change 2 values (counters[0] and
counters[1]),
it's impossible IMHO. It's question what to do with that:
1) atomically update just counters[2] and live with data racing for the first 2
values
2) add (probably conditionally) a spin lock
3) do not handle thread-safety of indirect call counters at all
Thanks for confirming my thoughts.
For this case there are three 'counters'
1) a value we're checking. Set when the delta is zero.
2) a count of number of uses
3) a count on the delta of the uses that matched and the uses that did not.
Notice that the recorded value can change, whenever the delta returns to zero.
That's intentional. This has the side effect of preventing the delta ever going
negative.
The tricky case is resetting the value when the delta is zero. We can't
simultaneously set the delta and the value. We could use a 2 step process
though -- set delta to 'updating', set value, set delta to 1. That will put a
compare_exchange in the hot path though ... and still turns out to be tricky.
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, ...);
This does have a race condition -- two threads could get into the inner if body.
But I think that's harmless. One of them will win the store of value, and
both of them will restore the delta counter. We'll end up with delta being 1
too high.
wdyt?
nathan
