On 08/08/2016 06:50 PM, Martin Liška wrote:
> On 08/08/2016 05:24 PM, Nathan Sidwell wrote:
>> 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'?
> 
> Fixed in the second version of patch.
> 
> 
>>
>> +__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)
> 
> I'll send separate email for that issue.
> 
>>
>> -__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.
> 
> Done, with a link to this mailing list thread.
> 
>>
>>  /* 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?)
> 
> Yeah, I'm putting these 2 functions to the same object.
> 
> Martin
> 
>>
>> nathan
> 

v3: fixed wrong defines in libgcc/Makefine.in

Martin