Re: [PATCH] Fix a race condition in indirect call value profiling

["Richard Guenther" <richard dot guenther at gmail dot com>]

2008/12/27 Seongbae Park 박성배 朴成培 <seongbae.park@gmail.com>:
> *ping*

Ok.

Thanks,
Richard.

> Seongbae
>
> On Mon, Dec 15, 2008 at 1:56 PM, Seongbae Park 박성배 朴成培
> <seongbae.park@gmail.com> wrote:
>> Hi,
>>
>> ChangeLog:
>>
>> 2008-12-15  Seongbae Park <seongbae.park@gmail.com>
>>
>>        * tree-profile.c (tree_init_ic_make_global_vars): Make static
>> variables TLS.
>>
>> The actual patch:
>>
>> Index: gcc/tree-profile.c
>> ===================================================================
>> --- gcc/tree-profile.c  (revision 142734)
>> +++ gcc/tree-profile.c  (working copy)
>> @@ -82,6 +82,7 @@ tree_init_ic_make_global_vars (void)
>>   TREE_PUBLIC (ic_void_ptr_var) = 0;
>>   DECL_ARTIFICIAL (ic_void_ptr_var) = 1;
>>   DECL_INITIAL (ic_void_ptr_var) = NULL;
>> +  DECL_TLS_MODEL (ic_void_ptr_var) = decl_default_tls_model (ic_void_ptr_var);
>>   assemble_variable (ic_void_ptr_var, 0, 0, 0);
>>
>>   gcov_type_ptr = build_pointer_type (get_gcov_type ());
>> @@ -93,6 +94,7 @@ tree_init_ic_make_global_vars (void)
>>   TREE_PUBLIC (ic_gcov_type_ptr_var) = 0;
>>   DECL_ARTIFICIAL (ic_gcov_type_ptr_var) = 1;
>>   DECL_INITIAL (ic_gcov_type_ptr_var) = NULL;
>> +  DECL_TLS_MODEL (ic_gcov_type_ptr_var) = decl_default_tls_model
>> (ic_gcov_type_ptr_var);
>>   assemble_variable (ic_gcov_type_ptr_var, 0, 0, 0);
>>  }
>>
>>
>> This is to fix a race condition in the value profiling of the indirect
>> call targets in multithreaded programs. The race can happen when
>> __gcov_indirect_call_counters is updated after
>> __gcov_indirect_call_callee is read, or any of the other combinations,
>> essentially leading to wrong callee-counter combination to be read
>> When that happens, the wrong counter can be updated, effectively
>> making the value profiling to report wrong callee for indirect call
>> sites.
>>
>> On the surface, the race seems to be relatively improbable, but a
>> combination of conditions can make this race trigger almost always:
>>
>> 1) If the actual callee of the profiled call site is in a different
>> module, the counter is never updated by the valid path leaving the
>> callee 0, and when the recorded callee is 0 in the counter, the callee
>> is (blindly) recorded - hence it is guaranteed that if the race
>> happens even once, it will get recorded.
>> 2) The module containing a hot function tends to have other hot
>> functions in it as well. If there's another indirect call in the
>> module that's hot, it would very frequently update the same
>> __gcov_indirect_call_counters variable, increasing the chance of
>> triggering the race significantly.
>> 3) Normally, the mismatch between the overall count reported by
>> indirect call value profiler and the basic block count reported by
>> edge counts would trigger the corrupted value profile error. However,
>> under -fprofile-correction, we ignore this error condition because it
>> could happen without this race condition due to missed counter
>> updates.
>>
>> The immediate symptoms can be either
>> 1) code bloat - the wrong callee is speculatively inlined, which
>> increases the code size unnecessarily but otherwise harmless since
>> it's guarded by check.
>> or
>> 2) ICE - if the return type of the callee doesn't match the caller's
>> expectation, various bad things can happen down the line after
>> inlining.
>>
>> Tested on x86-64 - c/c++ bootstrapped clean, and make-check is running
>> (check-c tree-prof.exp result is clean).
>>
>> I'm not completely sure whether it's ok to turn on TLS always - please
>> advise if I'd need #ifdef guard or make this optional under a flag.
>>
>> Seongbae
>>
>