[google,4.7] Move the building of gcov constructor function after initialization of gcov_info_var

After the refactoring has been checked in, the bug fixing part is simply
a moving a function call.

Tested by running ./buildit with both x86-64 and power64 targets.
The last time regression of tls-tests.c disappeared. So it is really flaky
in our testing environment.

thanks
Carrot

2013-05-02  Guozhi Wei  <carrot@google.com>

        * coverage.c (coverage_obj_init): Move the call of build_init_ctor to
        (coverage_obj_finish): here.




On Thu, May 2, 2013 at 11:15 AM, Xinliang David Li <davidxl@google.com> wrote:
> I suggest submitting the refactoring part of the changes to GCC trunk first.
>
> thanks,
>
> David
>
> On Thu, May 2, 2013 at 11:06 AM, Carrot Wei <carrot@google.com> wrote:
>> This patch fixes google bug 8397853 and targets google 4.7 branch.
>>
>> In LIPO mode, when coverage_obj_init is called, cgraph_state is
>> CGRAPH_STATE_FINISHED. The variable gcov_info_var is created but not
>> initialized. When cgraph_build_static_cdtor is called, the new function and
>> variables are expanded immediately since cgraph_state is CGRAPH_STATE_FINISHED.
>> It causes gcov_info_var into .bss section. But later in function
>> coverage_obj_finish we initialize gcov_info_var with non zero contents, so it
>> should not be put into .bss section.
>>
>> In FDO mode we don't have this problem because when coverage_obj_init is called,
>> cgraph_state is CGRAPH_STATE_IPA_SSA. When cgraph_build_static_cdtor is called,
>> the new function is not immediately expanded. The variable will have been
>> properly initialized when it is expanded.
>>
>> It can be fixed by moving the construction of gcov constructor after
>> initialization of gcov_info_var.
>>
>> Tested with following testing:
>> x86-64 bootstrap.
>> x86-64 regression test.
>> power64 regression test on qemu.
>>
>> The only regression for power64 is
>> FAIL: gcc.dg/torture/tls/tls-test.c  -O2 -flto -fno-use-linker-plugin
>> -flto-partition=none  execution test
>> It is a flaky test case in our testing environment since all other executions
>> with different compiler options failed. All testing of tls-test.c pass native
>> power64 testing.
>>
>> thanks
>> Carrot
>>
>> 2013-05-02  Guozhi Wei  <carrot@google.com>
>>
>> * coverage.c (gcov_info_type): New global variable.
>> (coverage_obj_init): Move the construction of gcov constructor to
>> (build_init_ctor): here.
>> (coverage_obj_finish): Call build_init_ctor after initialization of
>> gcov_info_var.
>>
>>
>> Index: coverage.c
>> ===================================================================
>> --- coverage.c (revision 198425)
>> +++ coverage.c (working copy)
>> @@ -123,6 +123,7 @@
>>
>>  /* Coverage info VAR_DECL and function info type nodes.  */
>>  static GTY(()) tree gcov_info_var;
>> +static GTY(()) tree gcov_info_type;
>>  static GTY(()) tree gcov_fn_info_type;
>>  static GTY(()) tree gcov_fn_info_ptr_type;
>>
>> @@ -2478,14 +2479,12 @@
>>    return build_constructor (info_type, v1);
>>  }
>>
>> -/* Create the gcov_info types and object.  Generate the constructor
>> -   function to call __gcov_init.  Does not generate the initializer
>> +/* Create the gcov_info types and object. Does not generate the initializer
>>     for the object.  Returns TRUE if coverage data is being emitted.  */
>>
>>  static bool
>>  coverage_obj_init (void)
>>  {
>> -  tree gcov_info_type, ctor, stmt, init_fn;
>>    unsigned n_counters = 0;
>>    unsigned ix;
>>    struct coverage_data *fn;
>> @@ -2531,24 +2530,6 @@
>>    ASM_GENERATE_INTERNAL_LABEL (name_buf, "LPBX", 0);
>>    DECL_NAME (gcov_info_var) = get_identifier (name_buf);
>>
>> -  /* Build a decl for __gcov_init.  */
>> -  init_fn = build_pointer_type (gcov_info_type);
>> -  init_fn = build_function_type_list (void_type_node, init_fn, NULL);
>> -  init_fn = build_decl (BUILTINS_LOCATION, FUNCTION_DECL,
>> - get_identifier ("__gcov_init"), init_fn);
>> -  TREE_PUBLIC (init_fn) = 1;
>> -  DECL_EXTERNAL (init_fn) = 1;
>> -  DECL_ASSEMBLER_NAME (init_fn);
>> -
>> -  /* Generate a call to __gcov_init(&gcov_info).  */
>> -  ctor = NULL;
>> -  stmt = build_fold_addr_expr (gcov_info_var);
>> -  stmt = build_call_expr (init_fn, 1, stmt);
>> -  append_to_statement_list (stmt, &ctor);
>> -
>> -  /* Generate a constructor to run it.  */
>> -  cgraph_build_static_cdtor ('I', ctor, DEFAULT_INIT_PRIORITY);
>> -
>>    return true;
>>  }
>>
>> @@ -2570,6 +2551,32 @@
>>    return ctor;
>>  }
>>
>> +/* Generate the constructor function to call __gcov_init.  */
>> +
>> +static void
>> +build_init_ctor ()
>> +{
>> +  tree ctor, stmt, init_fn;
>> +
>> +  /* Build a decl for __gcov_init.  */
>> +  init_fn = build_pointer_type (gcov_info_type);
>> +  init_fn = build_function_type_list (void_type_node, init_fn, NULL);
>> +  init_fn = build_decl (BUILTINS_LOCATION, FUNCTION_DECL,
>> + get_identifier ("__gcov_init"), init_fn);
>> +  TREE_PUBLIC (init_fn) = 1;
>> +  DECL_EXTERNAL (init_fn) = 1;
>> +  DECL_ASSEMBLER_NAME (init_fn);
>> +
>> +  /* Generate a call to __gcov_init(&gcov_info).  */
>> +  ctor = NULL;
>> +  stmt = build_fold_addr_expr (gcov_info_var);
>> +  stmt = build_call_expr (init_fn, 1, stmt);
>> +  append_to_statement_list (stmt, &ctor);
>> +
>> +  /* Generate a constructor to run it.  */
>> +  cgraph_build_static_cdtor ('I', ctor, DEFAULT_INIT_PRIORITY);
>> +}
>> +
>>  /* Finalize the coverage data.  Generates the array of pointers to
>>     function objects from CTOR.  Generate the gcov_info initializer.  */
>>
>> @@ -2589,9 +2596,12 @@
>>    DECL_NAME (fn_info_ary) = get_identifier (name_buf);
>>    DECL_INITIAL (fn_info_ary) = build_constructor (fn_info_ary_type, ctor);
>>    varpool_finalize_decl (fn_info_ary);
>> -
>> +
>>    DECL_INITIAL (gcov_info_var)
>>      = build_info (TREE_TYPE (gcov_info_var), fn_info_ary);
>> +
>> +  build_init_ctor ();
>> +
>>    varpool_finalize_decl (gcov_info_var);
>>  }

ok.

David

On Mon, May 6, 2013 at 6:03 PM, Carrot Wei <carrot@google.com> wrote:
> After the refactoring has been checked in, the bug fixing part is simply
> a moving a function call.
>
> Tested by running ./buildit with both x86-64 and power64 targets.
> The last time regression of tls-tests.c disappeared. So it is really flaky
> in our testing environment.
>
> thanks
> Carrot
>
> 2013-05-02  Guozhi Wei  <carrot@google.com>
>
>         * coverage.c (coverage_obj_init): Move the call of build_init_ctor to
>         (coverage_obj_finish): here.
>
>
> Index: coverage.c
> ===================================================================
> --- coverage.c (revision 198654)
> +++ coverage.c (working copy)
> @@ -2504,8 +2504,7 @@
>    cgraph_build_static_cdtor ('I', ctor, DEFAULT_INIT_PRIORITY);
>  }
>
> -/* Create the gcov_info types and object.  Generate the constructor
> -   function to call __gcov_init.  Does not generate the initializer
> +/* Create the gcov_info types and object. Does not generate the initializer
>     for the object.  Returns TRUE if coverage data is being emitted.  */
>
>  static bool
> @@ -2557,8 +2556,6 @@
>    ASM_GENERATE_INTERNAL_LABEL (name_buf, "LPBX", 0);
>    DECL_NAME (gcov_info_var) = get_identifier (name_buf);
>
> -  build_init_ctor (gcov_info_type);
> -
>    return true;
>  }
>
> @@ -2581,7 +2578,8 @@
>  }
>
>  /* Finalize the coverage data.  Generates the array of pointers to
> -   function objects from CTOR.  Generate the gcov_info initializer.  */
> +   function objects from CTOR.  Generate the gcov_info initializer.
> +   Generate the constructor function to call __gcov_init.  */
>
>  static void
>  coverage_obj_finish (VEC(constructor_elt,gc) *ctor)
> @@ -2599,9 +2597,12 @@
>    DECL_NAME (fn_info_ary) = get_identifier (name_buf);
>    DECL_INITIAL (fn_info_ary) = build_constructor (fn_info_ary_type, ctor);
>    varpool_finalize_decl (fn_info_ary);
> -
> +
>    DECL_INITIAL (gcov_info_var)
>      = build_info (TREE_TYPE (gcov_info_var), fn_info_ary);
> +
> +  build_init_ctor (TREE_TYPE (gcov_info_var));
> +
>    varpool_finalize_decl (gcov_info_var);
>  }
>
>
>
> On Thu, May 2, 2013 at 11:15 AM, Xinliang David Li <davidxl@google.com> wrote:
>> I suggest submitting the refactoring part of the changes to GCC trunk first.
>>
>> thanks,
>>
>> David
>>
>> On Thu, May 2, 2013 at 11:06 AM, Carrot Wei <carrot@google.com> wrote:
>>> This patch fixes google bug 8397853 and targets google 4.7 branch.
>>>
>>> In LIPO mode, when coverage_obj_init is called, cgraph_state is
>>> CGRAPH_STATE_FINISHED. The variable gcov_info_var is created but not
>>> initialized. When cgraph_build_static_cdtor is called, the new function and
>>> variables are expanded immediately since cgraph_state is CGRAPH_STATE_FINISHED.
>>> It causes gcov_info_var into .bss section. But later in function
>>> coverage_obj_finish we initialize gcov_info_var with non zero contents, so it
>>> should not be put into .bss section.
>>>
>>> In FDO mode we don't have this problem because when coverage_obj_init is called,
>>> cgraph_state is CGRAPH_STATE_IPA_SSA. When cgraph_build_static_cdtor is called,
>>> the new function is not immediately expanded. The variable will have been
>>> properly initialized when it is expanded.
>>>
>>> It can be fixed by moving the construction of gcov constructor after
>>> initialization of gcov_info_var.
>>>
>>> Tested with following testing:
>>> x86-64 bootstrap.
>>> x86-64 regression test.
>>> power64 regression test on qemu.
>>>
>>> The only regression for power64 is
>>> FAIL: gcc.dg/torture/tls/tls-test.c  -O2 -flto -fno-use-linker-plugin
>>> -flto-partition=none  execution test
>>> It is a flaky test case in our testing environment since all other executions
>>> with different compiler options failed. All testing of tls-test.c pass native
>>> power64 testing.
>>>
>>> thanks
>>> Carrot
>>>
>>> 2013-05-02  Guozhi Wei  <carrot@google.com>
>>>
>>> * coverage.c (gcov_info_type): New global variable.
>>> (coverage_obj_init): Move the construction of gcov constructor to
>>> (build_init_ctor): here.
>>> (coverage_obj_finish): Call build_init_ctor after initialization of
>>> gcov_info_var.
>>>
>>>
>>> Index: coverage.c
>>> ===================================================================
>>> --- coverage.c (revision 198425)
>>> +++ coverage.c (working copy)
>>> @@ -123,6 +123,7 @@
>>>
>>>  /* Coverage info VAR_DECL and function info type nodes.  */
>>>  static GTY(()) tree gcov_info_var;
>>> +static GTY(()) tree gcov_info_type;
>>>  static GTY(()) tree gcov_fn_info_type;
>>>  static GTY(()) tree gcov_fn_info_ptr_type;
>>>
>>> @@ -2478,14 +2479,12 @@
>>>    return build_constructor (info_type, v1);
>>>  }
>>>
>>> -/* Create the gcov_info types and object.  Generate the constructor
>>> -   function to call __gcov_init.  Does not generate the initializer
>>> +/* Create the gcov_info types and object. Does not generate the initializer
>>>     for the object.  Returns TRUE if coverage data is being emitted.  */
>>>
>>>  static bool
>>>  coverage_obj_init (void)
>>>  {
>>> -  tree gcov_info_type, ctor, stmt, init_fn;
>>>    unsigned n_counters = 0;
>>>    unsigned ix;
>>>    struct coverage_data *fn;
>>> @@ -2531,24 +2530,6 @@
>>>    ASM_GENERATE_INTERNAL_LABEL (name_buf, "LPBX", 0);
>>>    DECL_NAME (gcov_info_var) = get_identifier (name_buf);
>>>
>>> -  /* Build a decl for __gcov_init.  */
>>> -  init_fn = build_pointer_type (gcov_info_type);
>>> -  init_fn = build_function_type_list (void_type_node, init_fn, NULL);
>>> -  init_fn = build_decl (BUILTINS_LOCATION, FUNCTION_DECL,
>>> - get_identifier ("__gcov_init"), init_fn);
>>> -  TREE_PUBLIC (init_fn) = 1;
>>> -  DECL_EXTERNAL (init_fn) = 1;
>>> -  DECL_ASSEMBLER_NAME (init_fn);
>>> -
>>> -  /* Generate a call to __gcov_init(&gcov_info).  */
>>> -  ctor = NULL;
>>> -  stmt = build_fold_addr_expr (gcov_info_var);
>>> -  stmt = build_call_expr (init_fn, 1, stmt);
>>> -  append_to_statement_list (stmt, &ctor);
>>> -
>>> -  /* Generate a constructor to run it.  */
>>> -  cgraph_build_static_cdtor ('I', ctor, DEFAULT_INIT_PRIORITY);
>>> -
>>>    return true;
>>>  }
>>>
>>> @@ -2570,6 +2551,32 @@
>>>    return ctor;
>>>  }
>>>
>>> +/* Generate the constructor function to call __gcov_init.  */
>>> +
>>> +static void
>>> +build_init_ctor ()
>>> +{
>>> +  tree ctor, stmt, init_fn;
>>> +
>>> +  /* Build a decl for __gcov_init.  */
>>> +  init_fn = build_pointer_type (gcov_info_type);
>>> +  init_fn = build_function_type_list (void_type_node, init_fn, NULL);
>>> +  init_fn = build_decl (BUILTINS_LOCATION, FUNCTION_DECL,
>>> + get_identifier ("__gcov_init"), init_fn);
>>> +  TREE_PUBLIC (init_fn) = 1;
>>> +  DECL_EXTERNAL (init_fn) = 1;
>>> +  DECL_ASSEMBLER_NAME (init_fn);
>>> +
>>> +  /* Generate a call to __gcov_init(&gcov_info).  */
>>> +  ctor = NULL;
>>> +  stmt = build_fold_addr_expr (gcov_info_var);
>>> +  stmt = build_call_expr (init_fn, 1, stmt);
>>> +  append_to_statement_list (stmt, &ctor);
>>> +
>>> +  /* Generate a constructor to run it.  */
>>> +  cgraph_build_static_cdtor ('I', ctor, DEFAULT_INIT_PRIORITY);
>>> +}
>>> +
>>>  /* Finalize the coverage data.  Generates the array of pointers to
>>>     function objects from CTOR.  Generate the gcov_info initializer.  */
>>>
>>> @@ -2589,9 +2596,12 @@
>>>    DECL_NAME (fn_info_ary) = get_identifier (name_buf);
>>>    DECL_INITIAL (fn_info_ary) = build_constructor (fn_info_ary_type, ctor);
>>>    varpool_finalize_decl (fn_info_ary);
>>> -
>>> +
>>>    DECL_INITIAL (gcov_info_var)
>>>      = build_info (TREE_TYPE (gcov_info_var), fn_info_ary);
>>> +
>>> +  build_init_ctor ();
>>> +
>>>    varpool_finalize_decl (gcov_info_var);
>>>  }