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

[Mikael Morin <morin-mikael@orange.fr>]

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 <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:
>>>    
>>>> 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.
> 
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".