Re: [Fortran] PR37336 - FIINAL patch [1/n]: Implement the finalization wrapper subroutine

[Tobias Burnus <burnus at net-b dot de>]

Dear Mikael, dear all,

Mikael Morin wrote:
> the patch mixes deallocation and finalization, which are treated
> separately in the standard.

First, I want to remark that the standard - in many cases - does not 
require memory freeing ("deallocation"), it "merely" makes it possible 
that one does not leak memory with allocatables. The actually freeing of 
the memory is just a matter of the qualify of the implementation.

Secondly, for a polymorphic type, one does not know at compile time 
whether it has allocatable components or not - nor whether it has a 
finalizer or not. Hence, I do not see another possibility to have a 
common _free/_final entry point in the vtable. As there has to be a 
common entry point, I think it makes sense to have a single finalization 
wrapper which handles both. (I had also initially thought, that one 
could handle those two cases separately, but now I don't see it anymore.)

>   1. some weird cases are not correctly covered (polymorphic components,
> multiple level of finalizable and/or non-finalizable components, of
> inheritance, ...)

I do believe that polymorphic components are correctly handled: If they 
are a POINTER, they are untouched but if they are ALLOCATABLE, one calls 
DEALLOCATE for the component, which should handle the 
finalization/deallocation correctly. (And nonallocatble, nonpointer 
components do not exist.)

> I would like to point out that forcing the wrapper's array argument to
> be contiguous will lead to poor code as repacking will be needed with
> inherited types to call the parent's wrapper (and the parent's parent's,
> etc...).

I think that's unavoidable with the current array descriptor, which 
assumes that the stride is always a multiple of the size of the type. I 
concur that with the new array descriptor, one restrict the 
copy-in/copy-out to calling explict-shape/assumed-size finalizers, which 
probably do not occur in practice.


> >+finalize_component (gfc_expr *expr, gfc_symbol *derived, gfc_component *comp,
> >+  e = gfc_copy_expr (expr);
> >+  e->ref = gfc_get_ref ();
> You should walk to the end of the reference chain.  Otherwise you are
> overwriting it here.

I will do this.

> > >+  if (comp->attr.allocatable
> > >+      || (comp->ts.type == BT_CLASS && CLASS_DATA (comp)
> > >+	  && CLASS_DATA (comp)->attr.allocatable))
> > >+    {
> >
> > >+    }
> > >+  else if (comp->ts.type == BT_DERIVED
> > >+	    && comp->ts.u.derived->f2k_derived
> > >+	    && comp->ts.u.derived->f2k_derived->finalizers)
> What about polymorphic components?

I have to admit that the code is a bit implicit: polymorphic components 
are either ALLOCATABLE - and hence handled in the "if" block, or they 
are pointers - in which case this function is not called at all.

> What if only comp's subcomponents are finalizable, the finalization
> wrapper should still be called, shouldn't it?

Well, that's handled in the "else" branch. There, I walk all 
subcomponents. I do not need to walk them in case there is a finalizer 
as the called finalization wrapper will handle them.

> > >+  else
> > >+    {
> > >+      gfc_component *c;
> > >+
> > >+      gcc_assert ((comp->attr.alloc_comp || comp->attr.final_comp)
> > >+		  && comp->ts.type != BT_CLASS);
> > >+      for (c = comp->ts.u.derived->components; c; c = c->next)
> > >+	if ((comp->ts.type != BT_CLASS && !comp->attr.pointer
> > >+	     && (comp->attr.alloc_comp || comp->attr.allocatable
> > >+		 || comp->attr.final_comp))
> > >+	    || ((comp->ts.type == BT_CLASS && CLASS_DATA (comp)
> > >+		 && CLASS_DATA (comp)->attr.allocatable)))
> > >+	  finalize_component (e, comp->ts.u.derived, comp, stat, code);
> > >+    }
> This doesn't work, you use comp instead of c.

I hate copy-and-paste bugs. Thanks.

> If there is a polymorphic component whose declared type is not
> finalizable, but whose actual type is, the finalization wrapper should
> still be called.

But it will, as written above, polymorphic components are allocatable 
(or they are pointers and won't get finalized).

> If comp has finalizable subcomponents, it has a finalization wrapper,
> which is (or should be) caught above, so this branch is (or should be)
> unreachable.

I probably miss something, but I don't see why this branch should be 
unreachable. One has:

if (component is allocatable)
  call DEALLOCATE(comp) ! which might invoke finalizers
else if (component itself has a finalizer)
  call FINAL_WRAPPER
else
   for all nonpointer subcomponents which are allocatables, have 
finalizers or have allocatable/finalizable components, call 
finalize_component.
end if


> > >+  block->symtree = gfc_find_symtree (sub_ns->sym_root, "c_f_pointer");
> This is useless...

I concur.

> > >+  bool alloc_comp = false;
> This is misnamed, it should be final_comp or something.

I concur, its use became extended during the development of the patch.

> > >+      for (comp = vtab->ts.u.derived->components; comp; comp = comp->next)
> > >+	if (comp->name[0] == '_' && comp->name[1] == 'f')
> I have no strong opinion about it, but slightly prefer strcmp (...,
> "_final") with regard to readability, and solidity against future vtab
> extensions with methods starting with "_f".

Maybe. "_" && "f" should be faster and I don't see us adding more vtable 
functions. on the other hand, strcmp is safer and clearer. I also don't 
have a strong opinion about that.


> > >+	  {
> > >+	    ancestor_wrapper = comp->initializer;
> > >+	    break;
> > >+	  }
> > >+    }
> > >+
> > >+  /* No wrapper of the ancestor and no own FINAL subroutines and
> > >+     allocatable components: Return a NULL() expression.  */
> > >+  if ((!ancestor_wrapper || ancestor_wrapper->expr_type == EXPR_NULL)
> > >+      && !derived->attr.alloc_comp
> shouldn't there be `&& !derived->attr.final_comp' also?

I concur; I forgot that line when I retrofitted the case that there is a 
finalizer but no allocatable componet.


> > >+      else if (comp->ts.type == BT_CLASS && CLASS_DATA (comp)
> > >+	       && CLASS_DATA (comp)->attr.allocatable)
> > >+	alloc_comp = true;
> Shouldn't one assume without condition that there are allocatable or
> finalizable subcomponents when there is a polymorphic component?

Well, we do not deallocate/finalize polymorphic POINTER components.

> > >+  if (ancestor_wrapper && ancestor_wrapper->expr_type != EXPR_NULL)
> > >+    {
> > >+      last_code->next = XCNEW (gfc_code);
> > >+      last_code = last_code->next;
> > >+      last_code->op = EXEC_CALL;
> > >+      last_code->loc = gfc_current_locus;
> > >+      last_code->symtree = ancestor_wrapper->symtree;
> > >+      last_code->resolved_sym = ancestor_wrapper->symtree->n.sym;
> > >+
> > >+      last_code->ext.actual = gfc_get_actual_arglist ();
> > >+      last_code->ext.actual->expr = gfc_lval_expr_from_sym (array);
> I think a reference to the parent component is missing.

Actually, for a scalar it does not matter and for nonscalars, I still 
need to write the pack/unpack support. For the latter, I am not yet sure 
how to handle it best. As the Fortran standard doesn't allow 
"assumed_rank%comp", this case has to be handled in some special way.

> > >diff --git a/gcc/fortran/parse.c b/gcc/fortran/parse.c
> > >index 44b1900..4cafefe 100644
> > >--- a/gcc/fortran/parse.c
> > >+++ b/gcc/fortran/parse.c
> > >@@ -2250,6 +2250,16 @@ endType:
> > >  	  sym->attr.lock_comp = 1;
> > >  	}
> > >  
> > >+      /* Look for finalizers.  */
> > >+      if (c->attr.final_comp
> c->attr.final_comp is never set.
>
> I would like to avoid if possible yet another symbol attribute set in
> three different functions in three different files and used all over the
> place.  What about using a function "calculating" the predicate this time?

Maybe, however, one has then to call the function a lot of times: In 
generate_finalization_wrapper for the whole type, then for the new added 
components, and then for each component in finalize_component. With the 
current code, the latter has a complexity of approx. O(n lg n), but one 
might be able to improve it a bit by restructuring the code. (On the 
other hand, "n" is probably not excessively large.)

Tobias