[PATCH 5/N] Add new *_atomic counter update function, (-fprofile-update=atomic)

Nathan Sidwell nathan@acm.org
Mon Aug 8 15:24:00 GMT 2016


On 08/08/16 09:59, Martin Liška wrote:
> Hello.
>
> This patch is follow-up of the series where I introduce a set of counter update
> function that are thread-safe. I originally thought that majority of profile corruptions are
> caused by non-atomic updated of CFG (-fprofile-arc). But there are multiple counters that compare
> it's count to a number of execution of a basic block:
>
> blake2s.cpp:150:40: error: corrupted value profile: value profile counter (11301120 out of 11314388) inconsistent with basic-block count (11555117)
>        memcpy( S->buf + left, in, fill ); // Fill buffer
>
> This can be seen for unrar binary: PR58306. I'm also adding a simple test-case which reveals the inconsistency: val-profiler-threads-1.c.
>
> I've been running regression tests, ready to install after it finishes?


+      fn_name = concat ("__gcov_interval_profiler", fn_suffix, NULL);
+      tree_interval_profiler_fn = build_fn_decl (fn_name,
+						 interval_profiler_fn_type);

I like this idiom, but doesn't 'concat' call for a following 'free'?

+__gcov_pow2_profiler_atomic (gcov_type *counters, gcov_type value)
+{
+  if (value & (value - 1))
+    __atomic_fetch_add (&counters[0], 1, MEMMODEL_RELAXED);

This seems to think '0' is  a power of 2.  (I suspect a bug in the existing 
code, not  something you've  introduced)

-__gcov_one_value_profiler_body (gcov_type *counters, gcov_type value)
+__gcov_one_value_profiler_body (gcov_type *counters, gcov_type value,
+				int use_atomic)
  {
    if (value == counters[0])

This function should be commented along the lines of the email discussion we had 
last week.  the 'atomic' param doesn't make it completely thread safe.

  /* Counter for first visit of each function.  */
-static gcov_type function_counter;
+gcov_type function_counter;

why is this no longer static?  If  it must be globally visible, it'll need a 
suitable rename.  (perhaps it's simpler to put the 2(?) fns that access it into 
a single object file?)

nathan



More information about the Gcc-patches mailing list