ok. David
On Mon, May 6, 2013 at 6:03 PM, Carrot Wei <car...@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 <car...@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 <davi...@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 <car...@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 <car...@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); >>> }