Re: [PATCH] Fix memory leak in Fortran FE with init_emit (take 2)

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

On Thu, Aug 23, 2012 at 9:45 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Aug 22, 2012 at 03:29:28PM +0200, Richard Guenther wrote:
>> On Wed, Aug 22, 2012 at 3:03 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> > init_function_start is these days supposed to be called only at the start of
>> > RTL expansion, it shouldn't be called much earlier, because then we leak
>> > e.g. the memory allocated by init_emit.
>> >
>> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>>
>> Looks ok, but please move the assert into allocate_struct_function instead.
>
> I've done that, but unfortunately that first revealed in a problem that
> many functions would do copy_node followed by tree_function_versioning which
> called in the end allocate_struct_function (solved after IRC discussion
> by tweaking copy_node).  Another issue is in the C++ FE, where
> g++.dg/ext/gnu-inline-template-func.C testcase failed.  Hacked around
> by not calling allocate_struct_function in that case, but am not 100% sure
> what exactly we should be doing in that case and why it only triggers during
> template processing - first start_preparsed_function is called with one decl
> (the GNU extern inline decl), next it is called with another decl, but
>   /* We must call push_template_decl after current_class_type is set
>      up.  (If we are processing inline definitions after exiting a
>      class scope, current_class_type will be NULL_TREE until set above
>      by push_nested_class.)  */
>   if (processing_template_decl)
>     {
>       /* FIXME: Handle error_mark_node more gracefully.  */
>       tree newdecl1 = push_template_decl (decl1);
>       if (newdecl1 != error_mark_node)
>         decl1 = newdecl1;
>     }
> sets decl1 to the GNU extern inline decl on which start_preparsed_function
> has been already called.  Jason, any ideas?
>
> Perhaps it would make sense to commit the fortran/ change separately to fix
> the leak in there, and resolve the rest in a follow-up when it is clear what
> to do in the C++ FE.

Yes.  Does the copy_node change pass testing?  If so then installing it
would be nice as well.

Thanks,
Richard.

> 2012-08-23  Jakub Jelinek  <jakub@redhat.com>
>
>         * function.c (allocate_struct_function): Assert that
>         DECL_STRUCT_FUNCTION (fndecl) is NULL.
>         * tree.c (copy_node_stat): Clear DECL_STRUCT_FUNCTION.
> fortran/
>         * trans-decl.c (trans_function_start, generate_coarray_init,
>         create_main_function, gfc_generate_constructors): Call
>         allocate_struct_function instead of init_function_start.
> cp/
>         * decl.c (start_preparsed_function): Don't call
>         allocate_struct_function if already allocated.
>
> --- gcc/cp/decl.c.jj    2012-08-22 00:12:55.000000000 +0200
> +++ gcc/cp/decl.c       2012-08-22 22:35:24.040391935 +0200
> @@ -12882,7 +12882,10 @@ start_preparsed_function (tree decl1, tr
>       CFUN set up, and our per-function variables initialized.
>       FIXME factor out the non-RTL stuff.  */
>    bl = current_binding_level;
> -  allocate_struct_function (decl1, processing_template_decl);
> +  if (DECL_STRUCT_FUNCTION (decl1) == NULL)
> +    allocate_struct_function (decl1, processing_template_decl);
> +  else
> +    set_cfun (DECL_STRUCT_FUNCTION (decl1));
>
>    /* Initialize the language data structures.  Whenever we start
>       a new function, we destroy temporaries in the usual way.  */
> --- gcc/function.c.jj   2012-08-22 22:16:07.199445894 +0200
> +++ gcc/function.c      2012-08-22 22:37:15.449893843 +0200
> @@ -4479,6 +4479,7 @@ allocate_struct_function (tree fndecl, b
>
>    if (fndecl != NULL_TREE)
>      {
> +      gcc_checking_assert (DECL_STRUCT_FUNCTION (fndecl) == NULL);
>        DECL_STRUCT_FUNCTION (fndecl) = cfun;
>        cfun->decl = fndecl;
>        current_function_funcdef_no = get_next_funcdef_no ();
> --- gcc/tree.c.jj       2012-08-22 22:16:07.175445954 +0200
> +++ gcc/tree.c  2012-08-22 22:37:15.451893834 +0200
> @@ -983,6 +983,8 @@ copy_node_stat (tree node MEM_STAT_DECL)
>           SET_DECL_INIT_PRIORITY (t, DECL_INIT_PRIORITY (node));
>           DECL_HAS_INIT_PRIORITY_P (t) = 1;
>         }
> +      if (TREE_CODE (node) == FUNCTION_DECL)
> +       DECL_STRUCT_FUNCTION (t) = NULL;
>      }
>    else if (TREE_CODE_CLASS (code) == tcc_type)
>      {
> --- gcc/fortran/trans-decl.c.jj 2012-08-22 14:55:21.358138432 +0200
> +++ gcc/fortran/trans-decl.c    2012-08-22 22:37:15.476893717 +0200
> @@ -2265,7 +2265,7 @@ trans_function_start (gfc_symbol * sym)
>    /* Create RTL for function definition.  */
>    make_decl_rtl (fndecl);
>
> -  init_function_start (fndecl);
> +  allocate_struct_function (fndecl, false);
>
>    /* function.c requires a push at the start of the function.  */
>    pushlevel ();
> @@ -4408,7 +4408,7 @@ generate_coarray_init (gfc_namespace * n
>
>    rest_of_decl_compilation (fndecl, 0, 0);
>    make_decl_rtl (fndecl);
> -  init_function_start (fndecl);
> +  allocate_struct_function (fndecl, false);
>
>    pushlevel ();
>    gfc_init_block (&caf_init_block);
> @@ -4982,7 +4982,7 @@ create_main_function (tree fndecl)
>
>    rest_of_decl_compilation (ftn_main, 1, 0);
>    make_decl_rtl (ftn_main);
> -  init_function_start (ftn_main);
> +  allocate_struct_function (ftn_main, false);
>    pushlevel ();
>
>    gfc_init_block (&body);
> @@ -5537,7 +5537,7 @@ gfc_generate_constructors (void)
>
>    make_decl_rtl (fndecl);
>
> -  init_function_start (fndecl);
> +  allocate_struct_function (fndecl, false);
>
>    pushlevel ();
>
>
>         Jakub