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

[Carrot Wei <carrot at google dot com>]

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);
>>  }