Re: [PATCH,FORTRAN] Fix memory leak in finalization wrappers

[Bernhard Reutner-Fischer <rep.dot.nop@gmail.com>]

On Fri, 5 Nov 2021 19:46:16 +0100
Mikael Morin <morin-mikael@orange.fr> 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 <rep.dot.nop@gmail.com> 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 <rep.dot.nop@gmail.com> 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