Patchwork Refactor coverage.c, outline the construction of gcov constructor

login
register
mail settings
Submitter Carrot Wei
Date May 2, 2013, 8:41 p.m.
Message ID <CAEe8uEDfSVdv6-XrxCKkej0GYh-j9iA3oXOT3Gfwq73bYCOF9g@mail.gmail.com>
Download mbox | patch
Permalink /patch/241080/
State New
Headers show

Comments

Carrot Wei - May 2, 2013, 8:41 p.m.
This patch outline the construction of gcov constructor from coverage_obj_init
as a separate function build_init_ctor.

It passed bootstrap and regression test on x86-64.

OK for trunk and google 4.7 branch?

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.
Richard Guenther - May 3, 2013, 8:03 a.m.
On Thu, May 2, 2013 at 10:41 PM, Carrot Wei <carrot@google.com> wrote:
> This patch outline the construction of gcov constructor from coverage_obj_init
> as a separate function build_init_ctor.
>
> It passed bootstrap and regression test on x86-64.
>
> OK for trunk and google 4.7 branch?

Please pass gcov_info_type as parameter to build_init_ctor to avoid
the new GC root.

Ok with that change.

Thanks,
Richard.

> 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.
>
>
> Index: coverage.c
> ===================================================================
> --- coverage.c (revision 198557)
> +++ coverage.c (working copy)
> @@ -99,6 +99,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;
>
> @@ -967,6 +968,32 @@
>    return build_constructor (info_type, v1);
>  }
>
> +/* 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);
> +}
> +
>  /* Create the gcov_info types and object.  Generate the constructor
>     function to call __gcov_init.  Does not generate the initializer
>     for the object.  Returns TRUE if coverage data is being emitted.  */
> @@ -974,7 +1001,6 @@
>  static bool
>  coverage_obj_init (void)
>  {
> -  tree gcov_info_type, ctor, stmt, init_fn;
>    unsigned n_counters = 0;
>    unsigned ix;
>    struct coverage_data *fn;
> @@ -1020,24 +1046,8 @@
>    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);
> +  build_init_ctor ();
>
> -  /* 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;
>  }
Carrot Wei - May 3, 2013, 6:31 p.m.
On Fri, May 3, 2013 at 1:03 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Thu, May 2, 2013 at 10:41 PM, Carrot Wei <carrot@google.com> wrote:
>> This patch outline the construction of gcov constructor from coverage_obj_init
>> as a separate function build_init_ctor.
>>
>> It passed bootstrap and regression test on x86-64.
>>
>> OK for trunk and google 4.7 branch?
>
> Please pass gcov_info_type as parameter to build_init_ctor to avoid
> the new GC root.

Pass gcov_info_type as parameter to build_init_ctor may look more cleaner,
but it may limit build_init_ctor can only be called from coverage_obj_init.
In some situations build_init_ctor may need to be called by other functions,
such as my patch for lipo
http://gcc.gnu.org/ml/gcc-patches/2013-05/msg00078.html.

On the other hand, gcov_info_type is the only non-global variable in the
family of variables that have similar purpose. So it is not very bad to make
it also global.

thanks
Carrot

>
> Ok with that change.
>
> Thanks,
> Richard.
>
>> 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.
>>
>>
>> Index: coverage.c
>> ===================================================================
>> --- coverage.c (revision 198557)
>> +++ coverage.c (working copy)
>> @@ -99,6 +99,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;
>>
>> @@ -967,6 +968,32 @@
>>    return build_constructor (info_type, v1);
>>  }
>>
>> +/* 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);
>> +}
>> +
>>  /* Create the gcov_info types and object.  Generate the constructor
>>     function to call __gcov_init.  Does not generate the initializer
>>     for the object.  Returns TRUE if coverage data is being emitted.  */
>> @@ -974,7 +1001,6 @@
>>  static bool
>>  coverage_obj_init (void)
>>  {
>> -  tree gcov_info_type, ctor, stmt, init_fn;
>>    unsigned n_counters = 0;
>>    unsigned ix;
>>    struct coverage_data *fn;
>> @@ -1020,24 +1046,8 @@
>>    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);
>> +  build_init_ctor ();
>>
>> -  /* 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;
>>  }
Xinliang David Li - May 3, 2013, 6:51 p.m.
Please do what Richard suggested. gcov_info_type can be obtained from
gcov_info_var decl.

David


On Fri, May 3, 2013 at 11:31 AM, Carrot Wei <carrot@google.com> wrote:
> On Fri, May 3, 2013 at 1:03 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Thu, May 2, 2013 at 10:41 PM, Carrot Wei <carrot@google.com> wrote:
>>> This patch outline the construction of gcov constructor from coverage_obj_init
>>> as a separate function build_init_ctor.
>>>
>>> It passed bootstrap and regression test on x86-64.
>>>
>>> OK for trunk and google 4.7 branch?
>>
>> Please pass gcov_info_type as parameter to build_init_ctor to avoid
>> the new GC root.
>
> Pass gcov_info_type as parameter to build_init_ctor may look more cleaner,
> but it may limit build_init_ctor can only be called from coverage_obj_init.
> In some situations build_init_ctor may need to be called by other functions,
> such as my patch for lipo
> http://gcc.gnu.org/ml/gcc-patches/2013-05/msg00078.html.
>
> On the other hand, gcov_info_type is the only non-global variable in the
> family of variables that have similar purpose. So it is not very bad to make
> it also global.
>
> thanks
> Carrot
>
>>
>> Ok with that change.
>>
>> Thanks,
>> Richard.
>>
>>> 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.
>>>
>>>
>>> Index: coverage.c
>>> ===================================================================
>>> --- coverage.c (revision 198557)
>>> +++ coverage.c (working copy)
>>> @@ -99,6 +99,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;
>>>
>>> @@ -967,6 +968,32 @@
>>>    return build_constructor (info_type, v1);
>>>  }
>>>
>>> +/* 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);
>>> +}
>>> +
>>>  /* Create the gcov_info types and object.  Generate the constructor
>>>     function to call __gcov_init.  Does not generate the initializer
>>>     for the object.  Returns TRUE if coverage data is being emitted.  */
>>> @@ -974,7 +1001,6 @@
>>>  static bool
>>>  coverage_obj_init (void)
>>>  {
>>> -  tree gcov_info_type, ctor, stmt, init_fn;
>>>    unsigned n_counters = 0;
>>>    unsigned ix;
>>>    struct coverage_data *fn;
>>> @@ -1020,24 +1046,8 @@
>>>    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);
>>> +  build_init_ctor ();
>>>
>>> -  /* 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;
>>>  }
Carrot Wei - May 4, 2013, 4:27 a.m.
commited as 198591.

On Fri, May 3, 2013 at 11:51 AM, Xinliang David Li <davidxl@google.com> wrote:
> Please do what Richard suggested. gcov_info_type can be obtained from
> gcov_info_var decl.
>
> David
>
>
> On Fri, May 3, 2013 at 11:31 AM, Carrot Wei <carrot@google.com> wrote:
>> On Fri, May 3, 2013 at 1:03 AM, Richard Biener
>> <richard.guenther@gmail.com> wrote:
>>> On Thu, May 2, 2013 at 10:41 PM, Carrot Wei <carrot@google.com> wrote:
>>>> This patch outline the construction of gcov constructor from coverage_obj_init
>>>> as a separate function build_init_ctor.
>>>>
>>>> It passed bootstrap and regression test on x86-64.
>>>>
>>>> OK for trunk and google 4.7 branch?
>>>
>>> Please pass gcov_info_type as parameter to build_init_ctor to avoid
>>> the new GC root.
>>
>> Pass gcov_info_type as parameter to build_init_ctor may look more cleaner,
>> but it may limit build_init_ctor can only be called from coverage_obj_init.
>> In some situations build_init_ctor may need to be called by other functions,
>> such as my patch for lipo
>> http://gcc.gnu.org/ml/gcc-patches/2013-05/msg00078.html.
>>
>> On the other hand, gcov_info_type is the only non-global variable in the
>> family of variables that have similar purpose. So it is not very bad to make
>> it also global.
>>
>> thanks
>> Carrot
>>
>>>
>>> Ok with that change.
>>>
>>> Thanks,
>>> Richard.
>>>

Patch

Index: coverage.c
===================================================================
--- coverage.c (revision 198557)
+++ coverage.c (working copy)
@@ -99,6 +99,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;

@@ -967,6 +968,32 @@ 
   return build_constructor (info_type, v1);
 }

+/* 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);
+}
+
 /* Create the gcov_info types and object.  Generate the constructor
    function to call __gcov_init.  Does not generate the initializer
    for the object.  Returns TRUE if coverage data is being emitted.  */
@@ -974,7 +1001,6 @@ 
 static bool
 coverage_obj_init (void)
 {
-  tree gcov_info_type, ctor, stmt, init_fn;
   unsigned n_counters = 0;
   unsigned ix;
   struct coverage_data *fn;
@@ -1020,24 +1046,8 @@ 
   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);
+  build_init_ctor ();

-  /* 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;
 }