Re: [PATCH,FORTRAN] Fix memory leak in finalization wrappers
On Sun, 7 Nov 2021 13:32:34 +0100
Mikael Morin wrote:
> > btw.. Just because it's vagely related.
> > I think f8add009ce300f24b75e9c2e2cc5dd944a020c28 for
> > PR fortran/88009 (ICE in find_intrinsic_vtab, at fortran/class.c:2761)
> > is incomplete in that i think all the internal class helpers should be
> > flagged artificial. All these symbols built in gfc_build_class_symbol,
> > generate_finalization_wrapper, gfc_find_derived_vtab etc.
> > Looking at the history it seems the artificial bit often was forgotten.
>
> I guess so, yes...
>
> > And most importantly i think it is not correct to ignore artificial in
> > gfc_check_conflict!
> >
> Well, it’s not correct to throw errors at users for things they haven’t
> written and that they don’t control.
oops, i forgot to add the hunk to the patch to drain complaints to
the user 1).
Of course we don't want the error to be user-visible, but i think we do
want to check_conflicts (e.g. gfortran.dg/pr95587.f90 regresses via an
unspecific Unclassifiable statement; I assume we should copy all or at
least some sym attribs to the corresponding CLASS_DATA attribs which i
think makes sense for consistency anyway).
1)
diff --git a/gcc/fortran/symbol.c b/gcc/fortran/symbol.c
index 1a1e4551355..9df23f314df 100644
--- a/gcc/fortran/symbol.c
+++ b/gcc/fortran/symbol.c
@@ -898,6 +898,10 @@ gfc_check_conflict (symbol_attribute *attr, const char
*name, locus *where)
return true;
conflict:
+ /* It would be wrong to complain about artificial code. */
+ if (attr->artificial)
+return false;
+
if (name == NULL)
gfc_error ("%s attribute conflicts with %s attribute at %L",
a1, a2, where);
Re: [PATCH,FORTRAN] Fix memory leak in finalization wrappers
Le 07/11/2021 à 00:56, Bernhard Reutner-Fischer a écrit : On Sat, 6 Nov 2021 13:04:07 +0100 Mikael Morin wrote: Le 05/11/2021 à 23:08, Bernhard Reutner-Fischer a écrit : On Fri, 5 Nov 2021 19:46:16 +0100 Mikael Morin wrote: I’m a bit concerned by the loss of the null_expr’s type interface. I can’t convince myself that it’s either absolutely necessary or completely useless. It's a delicate spot, yes, but i do think they are completely useless. If we do NOT need a finalization, the initializer can (and has to be AFAIU) be a null_expr and AFAICS then does not need an interface. Well, the null pointer itself doesn’t need a type, but I think it’s better if the pointer it’s assigned to has a type different from void*. It will (hopefully) help the middle-end optimizers downstream. I would not expect this to help all that much or at all TBH. So i compiled for i in $(grep -li final $(grep -L dg-error /scratch/src/gcc-12.mine/gcc/testsuite/gfortran.dg/*.f*)); do gfortran -O2 -fcoarray=single -c $i -g -g3 -ggdb3 -fdump-tree-original -fdump-tree-optimized;done and diffed all .original and .optimized dumps against pristine trunk and they are identical. I inspected and ran the binary from finalize_14 and there is no change in the leaks compared to pristine trunk. The 3 shape_w in p leak as they used to. I do remember that finalize_14 was a good testcase, in sum i glared at it for quite some time ;) In fact, the interface is not used. the type is built in gfc_get_ppc_type which has the following. /* Explicit interface. */ if (c->attr.if_source != IFSRC_UNKNOWN && c->ts.interface) return build_pointer_type (gfc_get_function_type (c->ts.interface)); As components have no if_source attribute set, the type is not built here and a default function type is built further down without interface information. This is probably unintended as the components’ initializers carefully set an if_source attribute. The problem has been identified before ; see vaguely related posts from FX in september 2020. Anyway, I don’t think your changes will have negative impact then, and it makes things more readable, so I’m fine with it after all; OK. I will see if I can manage to create a testcase where it makes a difference (don’t hold your breath, I don’t even have a bootstrapped compiler ready yet). That'd be great, TIA! [] I’ve given up eventually. btw.. Just because it's vagely related. I think f8add009ce300f24b75e9c2e2cc5dd944a020c28 for PR fortran/88009 (ICE in find_intrinsic_vtab, at fortran/class.c:2761) is incomplete in that i think all the internal class helpers should be flagged artificial. All these symbols built in gfc_build_class_symbol, generate_finalization_wrapper, gfc_find_derived_vtab etc. Looking at the history it seems the artificial bit often was forgotten. I guess so, yes... And most importantly i think it is not correct to ignore artificial in gfc_check_conflict! Well, it’s not correct to throw errors at users for things they haven’t written and that they don’t control.
Re: [PATCH,FORTRAN] Fix memory leak in finalization wrappers
On Sat, 6 Nov 2021 13:04:07 +0100 Mikael Morin wrote: > Le 05/11/2021 à 23:08, Bernhard Reutner-Fischer a écrit : > > On Fri, 5 Nov 2021 19:46:16 +0100 > > Mikael Morin wrote: > > > >> Le 29/10/2021 à 01:58, Bernhard Reutner-Fischer via Fortran a écrit : > >>> On Wed, 27 Oct 2021 23:39:43 +0200 > >>> Bernhard Reutner-Fischer wrote: > >>> > On Mon, 15 Oct 2018 10:23:06 +0200 > Bernhard Reutner-Fischer wrote: > > > If a finalization is not required we created a namespace containing > > formal arguments for an internal interface definition but never used > > any of these. So the whole sub_ns namespace was not wired up to the > > program and consequently was never freed. The fix is to simply not > > generate any finalization wrappers if we know that it will be unused. > > Note that this reverts back to the original r190869 > > (8a96d64282ac534cb597f446f02ac5d0b13249cc) handling for this case > > by reverting this specific part of r194075 > > (f1ee56b4be7cc3892e6ccc75d73033c129098e87) for PR fortran/37336. > > > >> I’m a bit concerned by the loss of the null_expr’s type interface. > >> I can’t convince myself that it’s either absolutely necessary or > >> completely useless. > > > > It's a delicate spot, yes, but i do think they are completely useless. > > If we do NOT need a finalization, the initializer can (and has to be > > AFAIU) be a null_expr and AFAICS then does not need an interface. > > > Well, the null pointer itself doesn’t need a type, but I think it’s > better if the pointer it’s assigned to has a type different from void*. > It will (hopefully) help the middle-end optimizers downstream. I would not expect this to help all that much or at all TBH. So i compiled for i in $(grep -li final $(grep -L dg-error /scratch/src/gcc-12.mine/gcc/testsuite/gfortran.dg/*.f*)); do gfortran -O2 -fcoarray=single -c $i -g -g3 -ggdb3 -fdump-tree-original -fdump-tree-optimized;done and diffed all .original and .optimized dumps against pristine trunk and they are identical. I inspected and ran the binary from finalize_14 and there is no change in the leaks compared to pristine trunk. The 3 shape_w in p leak as they used to. I do remember that finalize_14 was a good testcase, in sum i glared at it for quite some time ;) > > I will see if I can manage to create a testcase where it makes a > difference (don’t hold your breath, I don’t even have a bootstrapped > compiler ready yet). > That'd be great, TIA! [] btw.. Just because it's vagely related. I think f8add009ce300f24b75e9c2e2cc5dd944a020c28 for PR fortran/88009 (ICE in find_intrinsic_vtab, at fortran/class.c:2761) is incomplete in that i think all the internal class helpers should be flagged artificial. All these symbols built in gfc_build_class_symbol, generate_finalization_wrapper, gfc_find_derived_vtab etc. Looking at the history it seems the artificial bit often was forgotten. And most importantly i think it is not correct to ignore artificial in gfc_check_conflict! I'm attaching my notes on this to illustrate what i mean. Not a patch, even if it regtests cleanly.. The hunk in gfc_match_derived_decl() plugs another leak by first checking if the max extension level is reached before adding the component. Maybe i should split that hunk out. Similar to the removal of *head in gfc_match_derived_decl, there's another spot in gfc_match_decl_type_spec which should get rid of the *head and just wire the interface up as usual. Just cosmetics. Several tests do exercise this code: alloc_comp_class_1.f90, class_19.f03 and 62, unlimited_polymorphic_8.f90 and others. > >> The rest of the changes (appart from class.c) are mostly OK with the nit > >> below and should be put in their own commit. > >> > >> >>> @@ -3826,10 +3828,8 @@ free_tb_tree (gfc_symtree *t) > >> >>> > >> >>> free_tb_tree (t->left); > >> >>> free_tb_tree (t->right); > >> >>> - > >> >>> - /* TODO: Free type-bound procedure structs themselves; probably > >> needs some > >> >>> - sort of ref-counting mechanism. */ > >> >>> free (t->n.tb); > >> > >> Please keep a comment; it remains somehow valid but could be updated > >> maybe: gfc_typebound_proc’s u.generic field for example is nowhere freed > >> as far as I know. > > > > Well that's a valid point, not sure where they are freed indeed. > > Do you have a specific testcase in mind that leaks tbp's u.generic (or > > specific for that matter) for me to look at? > > > Any testcase with generic typebound procedures, I guess. > typebound_generic_3.f03 for example seems like a good candidate. I'll have a look at these later, thanks for the pointer. > > > I'm happy to change the comment to > > TODO: Free type-bound procedure u.generic and u.specific fields > > to reflect the current state. Ok? > > > I don’t think specific leaks because it’s one of gfc_namespace’s > sym_root sub-nodes, and it’s freed with gfc_namespace
Re: [PATCH,FORTRAN] Fix memory leak in finalization wrappers
Sorry, I hadn’t seen your message. Le 05/11/2021 à 23:08, Bernhard Reutner-Fischer a écrit : On Fri, 5 Nov 2021 19:46:16 +0100 Mikael Morin wrote: Le 29/10/2021 à 01:58, Bernhard Reutner-Fischer via Fortran a écrit : On Wed, 27 Oct 2021 23:39:43 +0200 Bernhard Reutner-Fischer wrote: On Mon, 15 Oct 2018 10:23:06 +0200 Bernhard Reutner-Fischer wrote: If a finalization is not required we created a namespace containing formal arguments for an internal interface definition but never used any of these. So the whole sub_ns namespace was not wired up to the program and consequently was never freed. The fix is to simply not generate any finalization wrappers if we know that it will be unused. Note that this reverts back to the original r190869 (8a96d64282ac534cb597f446f02ac5d0b13249cc) handling for this case by reverting this specific part of r194075 (f1ee56b4be7cc3892e6ccc75d73033c129098e87) for PR fortran/37336. I’m a bit concerned by the loss of the null_expr’s type interface. I can’t convince myself that it’s either absolutely necessary or completely useless. It's a delicate spot, yes, but i do think they are completely useless. If we do NOT need a finalization, the initializer can (and has to be AFAIU) be a null_expr and AFAICS then does not need an interface. Well, the null pointer itself doesn’t need a type, but I think it’s better if the pointer it’s assigned to has a type different from void*. It will (hopefully) help the middle-end optimizers downstream. I will see if I can manage to create a testcase where it makes a difference (don’t hold your breath, I don’t even have a bootstrapped compiler ready yet). Don’t you get the same effect on the memory leaks if you keep just the following hunk? No, i don't think emitting the finalization-wrappers unconditionally is correct. > (... lengthy explaination ...) > Agreed, it was a poor suggestion. The rest of the changes (appart from class.c) are mostly OK with the nit below and should be put in their own commit. >>> @@ -3826,10 +3828,8 @@ free_tb_tree (gfc_symtree *t) >>> >>> free_tb_tree (t->left); >>> free_tb_tree (t->right); >>> - >>> - /* TODO: Free type-bound procedure structs themselves; probably needs some >>> - sort of ref-counting mechanism. */ >>> free (t->n.tb); Please keep a comment; it remains somehow valid but could be updated maybe: gfc_typebound_proc’s u.generic field for example is nowhere freed as far as I know. Well that's a valid point, not sure where they are freed indeed. Do you have a specific testcase in mind that leaks tbp's u.generic (or specific for that matter) for me to look at? Any testcase with generic typebound procedures, I guess. typebound_generic_3.f03 for example seems like a good candidate. I'm happy to change the comment to TODO: Free type-bound procedure u.generic and u.specific fields to reflect the current state. Ok? I don’t think specific leaks because it’s one of gfc_namespace’s sym_root sub-nodes, and it’s freed with gfc_namespace. OK without "and u.specific".
Re: [PATCH,FORTRAN] Fix memory leak in finalization wrappers
Le 05/11/2021 à 19:46, Mikael Morin a écrit : Don’t you get the same effect on the memory leaks if you keep just the following hunk? >>> @@ -1605,8 +1608,7 @@ generate_finalization_wrapper (gfc_symbol *derived, gfc_namespace *ns, >>> /* Set up the namespace. */ >>> sub_ns = gfc_get_namespace (ns, 0); >>> sub_ns->sibling = ns->contained; >>> - if (!expr_null_wrapper) >>> - ns->contained = sub_ns; >>> + ns->contained = sub_ns; >>> sub_ns->resolved = 1; >>> >>> /* Set up the procedure symbol. */ That’s probably not a good idea on second thought; it’s preferable to leak memory and not generate an empty finalization procedure.
Re: [PATCH,FORTRAN] Fix memory leak in finalization wrappers
On Fri, 5 Nov 2021 19:46:16 +0100 Mikael Morin wrote: > Le 29/10/2021 à 01:58, Bernhard Reutner-Fischer via Fortran a écrit : > > On Wed, 27 Oct 2021 23:39:43 +0200 > > Bernhard Reutner-Fischer wrote: > > > >> Ping > >> [hmz. it's been a while, I'll rebase and retest this one. > >> Ok if it passes?] > > Testing passed without any new regressions. > > Ok for trunk? > > thanks, > >> > >> On Mon, 15 Oct 2018 10:23:06 +0200 > >> Bernhard Reutner-Fischer wrote: > >> > >>> If a finalization is not required we created a namespace containing > >>> formal arguments for an internal interface definition but never used > >>> any of these. So the whole sub_ns namespace was not wired up to the > >>> program and consequently was never freed. The fix is to simply not > >>> generate any finalization wrappers if we know that it will be unused. > >>> Note that this reverts back to the original r190869 > >>> (8a96d64282ac534cb597f446f02ac5d0b13249cc) handling for this case > >>> by reverting this specific part of r194075 > >>> (f1ee56b4be7cc3892e6ccc75d73033c129098e87) for PR fortran/37336. > >>> > I’m a bit concerned by the loss of the null_expr’s type interface. > I can’t convince myself that it’s either absolutely necessary or > completely useless. It's a delicate spot, yes, but i do think they are completely useless. If we do NOT need a finalization, the initializer can (and has to be AFAIU) be a null_expr and AFAICS then does not need an interface. > Tobias didn’t include a test in his commit unfortunately, but I bet he > did the change on purpose. > Don’t you get the same effect on the memory leaks if you keep just the > following hunk? No, i don't think emitting the finalization-wrappers unconditionally is correct. In https://gcc.gnu.org/pipermail/gcc-patches/2021-October/582894.html i noted: ---8<--- We were generating (and emitting to modules) finalization wrapper needlessly, i.e. even when they were not called for. This 1) leaked like shown in the initial submission and 2) polluted module files with unwarranted (wrong) mention of finalization wrappers even when compiling without any coarray stuff. E.g. a modified udr10.f90 (from libgomp) has the following diff in the module which illustrates the positive side-effect of the fix: -26 'array' '' '' 25 ((VARIABLE INOUT UNKNOWN-PROC UNKNOWN UNKNOWN 0 0 -ARTIFICIAL DIMENSION CONTIGUOUS DUMMY) () (DERIVED 3 0 0 0 DERIVED ()) 0 -0 () (0 0 ASSUMED_RANK) 0 () () () 0 0) -27 'byte_stride' '' '' 25 ((VARIABLE UNKNOWN-INTENT UNKNOWN-PROC UNKNOWN -UNKNOWN 0 0 ARTIFICIAL VALUE DUMMY) () (INTEGER 8 0 0 0 INTEGER ()) 0 0 -() () 0 () () () 0 0) -28 'fini_coarray' '' '' 25 ((VARIABLE UNKNOWN-INTENT UNKNOWN-PROC -UNKNOWN UNKNOWN 0 0 ARTIFICIAL VALUE DUMMY) () (LOGICAL 1 0 0 0 LOGICAL -()) 0 0 () () 0 () () () 0 0) ---8<--- [Should be visible with the original udr10.f90 too.] If something in a module would trigger finalization to be emitted legitimately then this will continue to work as before. But IMHO it is not proper to emit them in an undue manner. Hence it does not help to just wire the sub_ns up in the program when it should not be wired up (and not generated in the first place) I'd say. > > >>> @@ -1605,8 +1608,7 @@ generate_finalization_wrapper (gfc_symbol > *derived, gfc_namespace *ns, > >>> /* Set up the namespace. */ > >>> sub_ns = gfc_get_namespace (ns, 0); > >>> sub_ns->sibling = ns->contained; > >>> - if (!expr_null_wrapper) > >>> -ns->contained = sub_ns; > >>> + ns->contained = sub_ns; > >>> sub_ns->resolved = 1; > >>> > >>> /* Set up the procedure symbol. */ > > > The rest of the changes (appart from class.c) are mostly OK with the nit > below and should be put in their own commit. > > >>> @@ -3826,10 +3828,8 @@ free_tb_tree (gfc_symtree *t) > >>> > >>> free_tb_tree (t->left); > >>> free_tb_tree (t->right); > >>> - > >>> - /* TODO: Free type-bound procedure structs themselves; probably > needs some > >>> - sort of ref-counting mechanism. */ > >>> free (t->n.tb); > > Please keep a comment; it remains somehow valid but could be updated > maybe: gfc_typebound_proc’s u.generic field for example is nowhere freed > as far as I know. Well that's a valid point, not sure where they are freed indeed. Do you have a specific testcase in mind that leaks tbp's u.generic (or specific for that matter) for me to look at? I'm happy to change the comment to TODO: Free type-bound procedure u.generic and u.specific fields to reflect the current state. Ok? > > Thanks. Many thanks for looking at the patch! > > Mikael
Re: [PATCH,FORTRAN] Fix memory leak in finalization wrappers
Le 29/10/2021 à 01:58, Bernhard Reutner-Fischer via Fortran a écrit : On Wed, 27 Oct 2021 23:39:43 +0200 Bernhard Reutner-Fischer wrote: Ping [hmz. it's been a while, I'll rebase and retest this one. Ok if it passes?] Testing passed without any new regressions. Ok for trunk? thanks, On Mon, 15 Oct 2018 10:23:06 +0200 Bernhard Reutner-Fischer wrote: If a finalization is not required we created a namespace containing formal arguments for an internal interface definition but never used any of these. So the whole sub_ns namespace was not wired up to the program and consequently was never freed. The fix is to simply not generate any finalization wrappers if we know that it will be unused. Note that this reverts back to the original r190869 (8a96d64282ac534cb597f446f02ac5d0b13249cc) handling for this case by reverting this specific part of r194075 (f1ee56b4be7cc3892e6ccc75d73033c129098e87) for PR fortran/37336. I’m a bit concerned by the loss of the null_expr’s type interface. I can’t convince myself that it’s either absolutely necessary or completely useless. Tobias didn’t include a test in his commit unfortunately, but I bet he did the change on purpose. Don’t you get the same effect on the memory leaks if you keep just the following hunk? >>> @@ -1605,8 +1608,7 @@ generate_finalization_wrapper (gfc_symbol *derived, gfc_namespace *ns, >>> /* Set up the namespace. */ >>> sub_ns = gfc_get_namespace (ns, 0); >>> sub_ns->sibling = ns->contained; >>> - if (!expr_null_wrapper) >>> -ns->contained = sub_ns; >>> + ns->contained = sub_ns; >>> sub_ns->resolved = 1; >>> >>> /* Set up the procedure symbol. */ The rest of the changes (appart from class.c) are mostly OK with the nit below and should be put in their own commit. >>> @@ -3826,10 +3828,8 @@ free_tb_tree (gfc_symtree *t) >>> >>> free_tb_tree (t->left); >>> free_tb_tree (t->right); >>> - >>> - /* TODO: Free type-bound procedure structs themselves; probably needs some >>> - sort of ref-counting mechanism. */ >>> free (t->n.tb); Please keep a comment; it remains somehow valid but could be updated maybe: gfc_typebound_proc’s u.generic field for example is nowhere freed as far as I know. Thanks. Mikael
Re: [PATCH,FORTRAN] Fix memory leak in finalization wrappers
On Wed, 27 Oct 2021 23:39:43 +0200 Bernhard Reutner-Fischer wrote: > Ping > [hmz. it's been a while, I'll rebase and retest this one. > Ok if it passes?] Testing passed without any new regressions. Ok for trunk? thanks, > > On Mon, 15 Oct 2018 10:23:06 +0200 > Bernhard Reutner-Fischer wrote: > > > If a finalization is not required we created a namespace containing > > formal arguments for an internal interface definition but never used > > any of these. So the whole sub_ns namespace was not wired up to the > > program and consequently was never freed. The fix is to simply not > > generate any finalization wrappers if we know that it will be unused. > > Note that this reverts back to the original r190869 > > (8a96d64282ac534cb597f446f02ac5d0b13249cc) handling for this case > > by reverting this specific part of r194075 > > (f1ee56b4be7cc3892e6ccc75d73033c129098e87) for PR fortran/37336. > > > > Regtests cleanly, installed to the fortran-fe-stringpool branch, sent > > here for reference and later inclusion. > > I might plug a few more leaks in preparation of switching to hash-maps. > > I fear that the leaks around interfaces are another candidate ;) > > > > Should probably add a tag for the compile-time leak PR68800 shouldn't i. > > > > valgrind summary for e.g. > > gfortran.dg/abstract_type_3.f03 and gfortran.dg/abstract_type_4.f03 > > where ".orig" is pristine trunk and ".mine" contains this fix: > > > > at3.orig.vg:LEAK SUMMARY: > > at3.orig.vg- definitely lost: 8,460 bytes in 11 blocks > > at3.orig.vg- indirectly lost: 13,288 bytes in 55 blocks > > at3.orig.vg- possibly lost: 0 bytes in 0 blocks > > at3.orig.vg- still reachable: 572,278 bytes in 2,142 blocks > > at3.orig.vg-suppressed: 0 bytes in 0 blocks > > at3.orig.vg- > > at3.orig.vg-Use --track-origins=yes to see where uninitialised values come > > from > > at3.orig.vg-ERROR SUMMARY: 38 errors from 33 contexts (suppressed: 0 from 0) > > -- > > at3.mine.vg:LEAK SUMMARY: > > at3.mine.vg- definitely lost: 344 bytes in 1 blocks > > at3.mine.vg- indirectly lost: 7,192 bytes in 18 blocks > > at3.mine.vg- possibly lost: 0 bytes in 0 blocks > > at3.mine.vg- still reachable: 572,278 bytes in 2,142 blocks > > at3.mine.vg-suppressed: 0 bytes in 0 blocks > > at3.mine.vg- > > at3.mine.vg-ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0) > > at3.mine.vg-ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0) > > at4.orig.vg:LEAK SUMMARY: > > at4.orig.vg- definitely lost: 13,751 bytes in 12 blocks > > at4.orig.vg- indirectly lost: 11,976 bytes in 60 blocks > > at4.orig.vg- possibly lost: 0 bytes in 0 blocks > > at4.orig.vg- still reachable: 572,278 bytes in 2,142 blocks > > at4.orig.vg-suppressed: 0 bytes in 0 blocks > > at4.orig.vg- > > at4.orig.vg-Use --track-origins=yes to see where uninitialised values come > > from > > at4.orig.vg-ERROR SUMMARY: 18 errors from 16 contexts (suppressed: 0 from 0) > > -- > > at4.mine.vg:LEAK SUMMARY: > > at4.mine.vg- definitely lost: 3,008 bytes in 3 blocks > > at4.mine.vg- indirectly lost: 4,056 bytes in 11 blocks > > at4.mine.vg- possibly lost: 0 bytes in 0 blocks > > at4.mine.vg- still reachable: 572,278 bytes in 2,142 blocks > > at4.mine.vg-suppressed: 0 bytes in 0 blocks > > at4.mine.vg- > > at4.mine.vg-ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 0 from 0) > > at4.mine.vg-ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 0 from 0) > > > > gcc/fortran/ChangeLog: > > > > 2018-10-12 Bernhard Reutner-Fischer > > > > * class.c (generate_finalization_wrapper): Do leak finalization > > wrappers if they will not be used. > > * expr.c (gfc_free_actual_arglist): Formatting fix. > > * gfortran.h (gfc_free_symbol): Pass argument by reference. > > (gfc_release_symbol): Likewise. > > (gfc_free_namespace): Likewise. > > * symbol.c (gfc_release_symbol): Adjust acordingly. > > (free_components): Set procedure pointer components > > of derived types to NULL after freeing. > > (free_tb_tree): Likewise. > > (gfc_free_symbol): Set sym to NULL after freeing. > > (gfc_free_namespace): Set namespace to NULL after freeing. > > --- > > gcc/fortran/class.c| 25 + > > gcc/fortran/expr.c | 2 +- > > gcc/fortran/gfortran.h | 6 +++--- > > gcc/fortran/symbol.c | 19 ++- > > 4 files changed, 23 insertions(+), 29 deletions(-) > > > > diff --git a/gcc/fortran/class.c b/gcc/fortran/class.c > > index 69c95fc5dfa..e0bb381a55f 100644 > > --- a/gcc/fortran/class.c > > +++ b/gcc/fortran/class.c > > @@ -1533,7 +1533,6 @@ generate_finalization_wrapper (gfc_symbol *derived, > > gfc_namespace *ns, > >gfc_code *last_code, *block; > >const char *name; > >bool finalizable_comp = false; > > - bool expr_null_wrapper = false; > >gfc_expr *ancestor_wrapper = NULL, *rank; > >gfc_iterator *iter; > > > > @@ -1561,13 +1560,17 @@ genera
Re: [PATCH,FORTRAN] Fix memory leak in finalization wrappers
Ping [hmz. it's been a while, I'll rebase and retest this one. Ok if it passes?] On Mon, 15 Oct 2018 10:23:06 +0200 Bernhard Reutner-Fischer wrote: > If a finalization is not required we created a namespace containing > formal arguments for an internal interface definition but never used > any of these. So the whole sub_ns namespace was not wired up to the > program and consequently was never freed. The fix is to simply not > generate any finalization wrappers if we know that it will be unused. > Note that this reverts back to the original r190869 > (8a96d64282ac534cb597f446f02ac5d0b13249cc) handling for this case > by reverting this specific part of r194075 > (f1ee56b4be7cc3892e6ccc75d73033c129098e87) for PR fortran/37336. > > Regtests cleanly, installed to the fortran-fe-stringpool branch, sent > here for reference and later inclusion. > I might plug a few more leaks in preparation of switching to hash-maps. > I fear that the leaks around interfaces are another candidate ;) > > Should probably add a tag for the compile-time leak PR68800 shouldn't i. > > valgrind summary for e.g. > gfortran.dg/abstract_type_3.f03 and gfortran.dg/abstract_type_4.f03 > where ".orig" is pristine trunk and ".mine" contains this fix: > > at3.orig.vg:LEAK SUMMARY: > at3.orig.vg- definitely lost: 8,460 bytes in 11 blocks > at3.orig.vg- indirectly lost: 13,288 bytes in 55 blocks > at3.orig.vg- possibly lost: 0 bytes in 0 blocks > at3.orig.vg- still reachable: 572,278 bytes in 2,142 blocks > at3.orig.vg-suppressed: 0 bytes in 0 blocks > at3.orig.vg- > at3.orig.vg-Use --track-origins=yes to see where uninitialised values come > from > at3.orig.vg-ERROR SUMMARY: 38 errors from 33 contexts (suppressed: 0 from 0) > -- > at3.mine.vg:LEAK SUMMARY: > at3.mine.vg- definitely lost: 344 bytes in 1 blocks > at3.mine.vg- indirectly lost: 7,192 bytes in 18 blocks > at3.mine.vg- possibly lost: 0 bytes in 0 blocks > at3.mine.vg- still reachable: 572,278 bytes in 2,142 blocks > at3.mine.vg-suppressed: 0 bytes in 0 blocks > at3.mine.vg- > at3.mine.vg-ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0) > at3.mine.vg-ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0) > at4.orig.vg:LEAK SUMMARY: > at4.orig.vg- definitely lost: 13,751 bytes in 12 blocks > at4.orig.vg- indirectly lost: 11,976 bytes in 60 blocks > at4.orig.vg- possibly lost: 0 bytes in 0 blocks > at4.orig.vg- still reachable: 572,278 bytes in 2,142 blocks > at4.orig.vg-suppressed: 0 bytes in 0 blocks > at4.orig.vg- > at4.orig.vg-Use --track-origins=yes to see where uninitialised values come > from > at4.orig.vg-ERROR SUMMARY: 18 errors from 16 contexts (suppressed: 0 from 0) > -- > at4.mine.vg:LEAK SUMMARY: > at4.mine.vg- definitely lost: 3,008 bytes in 3 blocks > at4.mine.vg- indirectly lost: 4,056 bytes in 11 blocks > at4.mine.vg- possibly lost: 0 bytes in 0 blocks > at4.mine.vg- still reachable: 572,278 bytes in 2,142 blocks > at4.mine.vg-suppressed: 0 bytes in 0 blocks > at4.mine.vg- > at4.mine.vg-ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 0 from 0) > at4.mine.vg-ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 0 from 0) > > gcc/fortran/ChangeLog: > > 2018-10-12 Bernhard Reutner-Fischer > > * class.c (generate_finalization_wrapper): Do leak finalization > wrappers if they will not be used. > * expr.c (gfc_free_actual_arglist): Formatting fix. > * gfortran.h (gfc_free_symbol): Pass argument by reference. > (gfc_release_symbol): Likewise. > (gfc_free_namespace): Likewise. > * symbol.c (gfc_release_symbol): Adjust acordingly. > (free_components): Set procedure pointer components > of derived types to NULL after freeing. > (free_tb_tree): Likewise. > (gfc_free_symbol): Set sym to NULL after freeing. > (gfc_free_namespace): Set namespace to NULL after freeing. > --- > gcc/fortran/class.c| 25 + > gcc/fortran/expr.c | 2 +- > gcc/fortran/gfortran.h | 6 +++--- > gcc/fortran/symbol.c | 19 ++- > 4 files changed, 23 insertions(+), 29 deletions(-) > > diff --git a/gcc/fortran/class.c b/gcc/fortran/class.c > index 69c95fc5dfa..e0bb381a55f 100644 > --- a/gcc/fortran/class.c > +++ b/gcc/fortran/class.c > @@ -1533,7 +1533,6 @@ generate_finalization_wrapper (gfc_symbol *derived, > gfc_namespace *ns, >gfc_code *last_code, *block; >const char *name; >bool finalizable_comp = false; > - bool expr_null_wrapper = false; >gfc_expr *ancestor_wrapper = NULL, *rank; >gfc_iterator *iter; > > @@ -1561,13 +1560,17 @@ generate_finalization_wrapper (gfc_symbol *derived, > gfc_namespace *ns, > } > >/* No wrapper of the ancestor and no own FINAL subroutines and allocatable > - components: Return a NULL() expression; we defer this a bit to have have > + components: Return a NULL() expression; we defer this a bit to have >
