| Message ID | 20120823074548.GW1999@tucnak.redhat.com |
|---|---|
| State | New |
| Headers | show |
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
On Thu, Aug 23, 2012 at 11:24:26AM +0200, Richard Guenther wrote: > > 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. The whole patch passed testing, but in the C++ FE (the cp/decl.c) it is probably just a partial fix or even wrong fix (e.g., even when we don't allocate a new cfun there, we still overwrite cfun->language with newly GCed memory, etc. So I'm going to commit now the tree.c and trans-decl.c hunks and will leave the rest for later. > > 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. Jakub
--- 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 ();