diff mbox

[Fortran,DRAFT] PR 46321 - [OOP] Polymorphic deallocation

Message ID CAHqFgjXULBJeA=Gd80sz_+Y4psju2Ka3BZoR-vS5qEqTEro=cA@mail.gmail.com
State New
Headers show

Commit Message

Alessandro Fanfarillo June 2, 2012, 12:58 p.m. UTC
Dear all,

I have realized a draft patch for the PR 46321, currently it works
only with the explicit DEALLOCATE.

Running the regression tests it doesn't pass the following:

- gfortran.dg/class_19.f03 (too much "__builtin_free")
- gfortran.dg/auto_dealloc_2.f90 (too much "__builtin_free")
- gfortran.dg/dynamic_dispatch_4.f03 (free on invalid pointer)
- gfortran.dg/typebound_operator_9.f03 (fails during the execution test)

The first two tests fail due to the introduction of "__builtin_free"
in the freeing functions, so it is not a problem.

The gfortran.dg/dynamic_dispatch_4.f03 had this problem in the past
(http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43986); currently it
calls the __free_s_bar_mod_S_bar function instead of the proper
doit().

Regarding typebound_operator_9.f03, I don't know how to fix the patch...

The patch is written in a "raw" way due to my newbieness, so any
suggestion is well accepted.

Regards.

Alessandro

Comments

Janus Weil June 2, 2012, 4:12 p.m. UTC | #1
Hi Alessandro,

> I have realized a draft patch for the PR 46321, currently it works
> only with the explicit DEALLOCATE.

thanks for the patch! Some first comments without actually looking at
the patch in detail ...


> Running the regression tests it doesn't pass the following:
>
> - gfortran.dg/class_19.f03 (too much "__builtin_free")
> - gfortran.dg/auto_dealloc_2.f90 (too much "__builtin_free")
> - gfortran.dg/dynamic_dispatch_4.f03 (free on invalid pointer)
> - gfortran.dg/typebound_operator_9.f03 (fails during the execution test)
>
> The first two tests fail due to the introduction of "__builtin_free"
> in the freeing functions, so it is not a problem.

Right. You should certainly fix the "scan-tree-dump-times" checks (by
adjusting the numbers properly, and making sure that they are actually
what one would expect), in order to make them pass.


> The gfortran.dg/dynamic_dispatch_4.f03 had this problem in the past
> (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43986); currently it
> calls the __free_s_bar_mod_S_bar function instead of the proper
> doit().

Sorry, I don't understand the last sentence. Why should it call some
"__free..." instead of "doit"? And why is that test case even affected
by your patch (you said it would only work with explicit DEALLOCATE,
which does not appear in that test case)?


> Regarding typebound_operator_9.f03, I don't know how to fix the patch...

Unfortunately that test case is rather large, so maybe you should
reduce it a bit to find the error (or just do some debugging in order
to find out where exactly it fails). Another possibility: Compare the
dump (using -fdump-tree-original) with and without the patch.


> The patch is written in a "raw" way due to my newbieness, so any
> suggestion is well accepted.

The patch actually gives a few warnings:

/home/jweil/gcc48/trunk/gcc/fortran/class.c: In function
‘gfc_find_derived_vtab’:
/home/jweil/gcc48/trunk/gcc/fortran/class.c:912:8: warning: ISO C90
forbids mixed declarations and code [-pedantic]
/home/jweil/gcc48/trunk/gcc/fortran/class.c:932:7: warning: C++ style
comments are not allowed in ISO C90 [enabled by default]
/home/jweil/gcc48/trunk/gcc/fortran/class.c:932:7: warning: (this will
be reported only once per input file) [enabled by default]

(and similar things in trans-stmt.c). You should definitely fix those.
Although a non-buildstrap build still works with those warnings, a
full bootstrap will fail.

Cheers,
Janus
Alessandro Fanfarillo June 2, 2012, 4:48 p.m. UTC | #2
Hi Janus,

> Sorry, I don't understand the last sentence. Why should it call some
> "__free..." instead of "doit"? And why is that test case even affected
> by your patch (you said it would only work with explicit DEALLOCATE,
> which does not appear in that test case)?

Yes, it is as I said... In
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43986#c4 the doit() call
produces a segfault because r26 is 0 instead of the address of
__s_bar_mod_MOD_doit. With my patched version, the doit call is in
reality a _free __free_s_bar_mod_S_bar call. To better understand I
report a little portion (only the MAIN__) of the fdump-tree-original
and the testcase execution (hoping that it will be understandable...)

MAIN__ ()
{
  struct __class_foo_mod_Foo_p a;
  struct foo b;
  static struct s_bar c = {};
  static struct a_bar d = {};

  try
    {
      c.a = 0B;
      d.a.data = 0B;
      (struct __vtype_foo_mod_Foo *) a._vptr = &__vtab_foo_mod_Foo;
      a._data = &b;
      a._vptr->doit (&a);
      if (a._vptr->getit (&a) != 1)
        {
          _gfortran_abort ();
        }
      L.1:;
      (struct __vtype_foo_mod_Foo *) a._vptr = (struct
__vtype_foo_mod_Foo *) &__vtab_s_bar_mod_S_bar;
      a._data = (struct foo *) &c;
      a._vptr->doit (&a); IT REALLY WANTS TO CALL THE DOIT FUNCTION!
      if (a._vptr->getit (&a) != 2)
        {
          _gfortran_abort ();
        }
      L.2:;
      (struct __vtype_foo_mod_Foo *) a._vptr = (struct
__vtype_foo_mod_Foo *) &__vtab_a_bar_mod_A_bar;
      a._data = (struct foo *) &d;
      a._vptr->doit (&a);
      if (a._vptr->getit (&a) != 3)
        {
          _gfortran_abort ();
        }
      L.3:;
    }
  finally
    {
      if (d.a.data != 0B)
        {
          __builtin_free ((void *) d.a.data);
        }
      d.a.data = 0B;
      if (c.a != 0B)
        {
          __builtin_free ((void *) c.a);
        }
      c.a = 0B;
    }
}

An now the testcase execution with gdb:

Breakpoint 1, MAIN__ () at dynamic_dispatch_4.f03:82
82	  type(s_bar), target :: c
(gdb) next
83	  type(a_bar), target :: d
(gdb)
85	  a => b
(gdb)
86	  call a%doit
(gdb)
87	  if (a%getit () .ne. 1) call abort
(gdb)
88	  a => c
(gdb) step
89	  call a%doit
(gdb)
s_bar_mod::__free_s_bar_mod_S_bar (tofree=...) at dynamic_dispatch_4.f03:43
43	    class(s_bar) :: a

I don't know if I got it across...

> The patch actually gives a few warnings:

Ok, thanks. I always use bootstrap and it works but I never look at
the compile result (unless it doesn't compile...)
Janus Weil June 2, 2012, 5:16 p.m. UTC | #3
Hi,

>> Sorry, I don't understand the last sentence. Why should it call some
>> "__free..." instead of "doit"? And why is that test case even affected
>> by your patch (you said it would only work with explicit DEALLOCATE,
>> which does not appear in that test case)?
>
> Yes, it is as I said... In
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43986#c4 the doit() call
> produces a segfault because r26 is 0 instead of the address of
> __s_bar_mod_MOD_doit. With my patched version, the doit call is in
> reality a _free __free_s_bar_mod_S_bar call.

huh, this is strange, indeed. I guess it means that something is
messed up in the vtable (some sort of offset?). We try to call one
virtual function, but we get another. Since this problem was already
seen in PR43986, it is probably a case of your patch uncovering an
existing bug. I'll try to look into this problem soon ...

Cheers,
Janus
Tobias Burnus June 2, 2012, 5:34 p.m. UTC | #4
Janus Weil wrote:
> The patch actually gives a few warnings:

Looking at those warnings, they seem to be valid C++ but invalid C89. As 
Stages 2 and 3 are, by default, compiled by C++, I assume that 
Alessandro does not see those.

By contrast, I assume that you (Janus) build GCC with the C compiler, 
i.e. you configure with --disable-build-poststage1-with-cxx.

Thus, a default boot strap,  shouldn't fail. Nonetheless, it is useful 
to keep compatibility with C and bootstrapping with 
--disable-build-poststage1-with-cxx.Hence, the warnings should be fixed. 
(Bootstrapping implies -Werror.)

Tobias

> /home/jweil/gcc48/trunk/gcc/fortran/class.c: In function
> ‘gfc_find_derived_vtab’:
> /home/jweil/gcc48/trunk/gcc/fortran/class.c:912:8: warning: ISO C90
> forbids mixed declarations and code [-pedantic]
> /home/jweil/gcc48/trunk/gcc/fortran/class.c:932:7: warning: C++ style
> comments are not allowed in ISO C90 [enabled by default]
> /home/jweil/gcc48/trunk/gcc/fortran/class.c:932:7: warning: (this will
> be reported only once per input file) [enabled by default]
>
> (and similar things in trans-stmt.c). You should definitely fix those.
> Although a non-buildstrap build still works with those warnings, a
> full bootstrap will fail.
Tobias Burnus June 2, 2012, 6:29 p.m. UTC | #5
Alessandro Fanfarillo wrote:
> The gfortran.dg/dynamic_dispatch_4.f03 had this problem in the past
> (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43986); currently it
> calls the __free_s_bar_mod_S_bar function instead of the proper
> doit().

That kind of makes sense: "foo" has no allocatable components while 
"s_bar" has. Seemingly, "foo" has no "_free" component - and thus, the 
first entry in "vtab" after _hash, _size, _extends, _def_init and _copy 
is "doit". However, s_bar has at that position not "doit" but "_free".

My impression is that you do not add a
   "_free => null()"  (EXPR_NULL)
in the case that there are no allocatable components in the type or its 
parents.

Side note: In class.c, please update the comment at the top by 
mentioning _free after the description of "_copy".

Tobias
Janus Weil June 2, 2012, 7:38 p.m. UTC | #6
Hi,

> By contrast, I assume that you (Janus) build GCC with the C compiler, i.e.
> you configure with --disable-build-poststage1-with-cxx.

actually I used --disable-bootstrap, which also has the effect that
gfortran is built with the C compiler.


> Thus, a default boot strap,  shouldn't fail. Nonetheless, it is useful to
> keep compatibility with C and bootstrapping with
> --disable-build-poststage1-with-cxx.Hence, the warnings should be fixed.
> (Bootstrapping implies -Werror.)

Right. I'm not sure what the further plans are for GCC regarding the C
vs C++ issue, but as long as GCC can still be built with a C compiler,
one should probably avoid unnecessary C++isms.

Cheers,
Janus



>> /home/jweil/gcc48/trunk/gcc/fortran/class.c: In function
>> ‘gfc_find_derived_vtab’:
>> /home/jweil/gcc48/trunk/gcc/fortran/class.c:912:8: warning: ISO C90
>> forbids mixed declarations and code [-pedantic]
>> /home/jweil/gcc48/trunk/gcc/fortran/class.c:932:7: warning: C++ style
>> comments are not allowed in ISO C90 [enabled by default]
>> /home/jweil/gcc48/trunk/gcc/fortran/class.c:932:7: warning: (this will
>> be reported only once per input file) [enabled by default]
>>
>> (and similar things in trans-stmt.c). You should definitely fix those.
>> Although a non-buildstrap build still works with those warnings, a
>> full bootstrap will fail.
Janus Weil June 2, 2012, 8:38 p.m. UTC | #7
2012/6/2 Tobias Burnus <burnus@net-b.de>:
> Alessandro Fanfarillo wrote:
>>
>> The gfortran.dg/dynamic_dispatch_4.f03 had this problem in the past
>> (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43986); currently it
>> calls the __free_s_bar_mod_S_bar function instead of the proper
>> doit().
>
>
> That kind of makes sense: "foo" has no allocatable components while "s_bar"
> has. Seemingly, "foo" has no "_free" component - and thus, the first entry
> in "vtab" after _hash, _size, _extends, _def_init and _copy is "doit".
> However, s_bar has at that position not "doit" but "_free".

Right, the problem is that the _free component is missing. Just as the
_copy component, _free should be present for *every* vtype, no matter
if there are allocatable components or not. If the _free component is
not needed, it should be initialized to EXPR_NULL.

Cheers,
Janus
Alessandro Fanfarillo June 3, 2012, 10:15 a.m. UTC | #8
> Right, the problem is that the _free component is missing. Just as the
> _copy component, _free should be present for *every* vtype, no matter
> if there are allocatable components or not. If the _free component is
> not needed, it should be initialized to EXPR_NULL.

With an "empty" _free function for every type which does not have
allocatable components the problem with dynamic_dispatch_4.f03
disappears :), thank you very much. In the afternoon I'll reorganize
the code.

Bye.

Alessandro
Paul Richard Thomas June 5, 2012, 9:58 a.m. UTC | #9
Hi Alessandro,

I am glad to see that Janus is giving you a helping hand, in addition
to Tobias.  I am so tied up with every aspect of life that gfortran is
not figuring much at all.

When you clean up the patch, you might consider making this into a
separate function:

+	  if (free_proc)
+	    {
+	      ppc = gfc_copy_expr(free_proc->initializer);
+	      ppc_code = gfc_get_code ();
+	      ppc_code->resolved_sym = ppc->symtree->n.sym;
+	      ppc_code->resolved_sym->attr.elemental = 1;
+	      ppc_code->ext.actual = actual;
+	      ppc_code->expr1 = ppc;
+	      ppc_code->op = EXEC_CALL;
+	      tmp = gfc_trans_call (ppc_code, true, NULL, NULL, false);
+	      gfc_free_statements (ppc_code);
+	      gfc_add_expr_to_block (&block, tmp);
+	    }

... and using the function call to replace the corresponding call to
_copy in trans_allocate.

I suspect that we are going to do this some more :-)

Once we have the separate function, we could at later stage replace it
by a TREE_SSA version.

Cheers

Paul

On 3 June 2012 12:15, Alessandro Fanfarillo <fanfarillo.gcc@gmail.com> wrote:
>> Right, the problem is that the _free component is missing. Just as the
>> _copy component, _free should be present for *every* vtype, no matter
>> if there are allocatable components or not. If the _free component is
>> not needed, it should be initialized to EXPR_NULL.
>
> With an "empty" _free function for every type which does not have
> allocatable components the problem with dynamic_dispatch_4.f03
> disappears :), thank you very much. In the afternoon I'll reorganize
> the code.
>
> Bye.
>
> Alessandro
diff mbox

Patch

Index: gcc/fortran/class.c
===================================================================
--- gcc/fortran/class.c	(revisione 188002)
+++ gcc/fortran/class.c	(copia locale)
@@ -717,6 +717,7 @@ 
   gfc_namespace *ns;
   gfc_symbol *vtab = NULL, *vtype = NULL, *found_sym = NULL, *def_init = NULL;
   gfc_symbol *copy = NULL, *src = NULL, *dst = NULL;
+  gfc_symbol *free = NULL, *tofree = NULL;
 
   /* Find the top-level namespace (MODULE or PROGRAM).  */
   for (ns = gfc_current_ns; ns; ns = ns->parent)
@@ -907,6 +908,119 @@ 
 		  c->ts.interface = copy;
 		}
 
+	      /* Add component _free.  */
+	      gfc_component *temp = NULL;
+	      bool der_comp_alloc = false, comp_alloc = false;
+	      bool  class_comp_alloc = false;
+	      for (temp = derived->components; temp; temp = temp->next)
+		{
+		  if (temp == derived->components && derived->attr.extension)
+		    continue;
+
+		  if (temp->ts.type == BT_DERIVED
+		      && !temp->attr.pointer
+		      && (temp->attr.alloc_comp || temp->attr.allocatable))
+		    der_comp_alloc = true;
+		  else if (temp->ts.type != BT_DERIVED
+			   && !temp->attr.pointer
+			   && (temp->attr.alloc_comp
+			       || temp->attr.allocatable))
+		    comp_alloc = true;
+		  else if (temp->ts.u.derived
+			   && temp->ts.type == BT_CLASS
+			   && CLASS_DATA (temp)
+			   //&& (CLASS_DATA (temp)->attr.class_pointer
+			   //    || CLASS_DATA (temp)->attr.allocatable))
+			   && CLASS_DATA (temp)->attr.allocatable)
+		    class_comp_alloc = true;
+		}
+	      if (derived->attr.extension
+		  && (!der_comp_alloc && !comp_alloc && !class_comp_alloc))
+		{
+		  gfc_component *parent = derived->components;
+		  gfc_component *free_proc = NULL;
+		  gfc_symbol *vtab2 = NULL;
+		  gfc_expr *tmp1 = NULL, *tmp2 = NULL;
+		  vtab2 = gfc_find_derived_vtab (parent->ts.u.derived);
+
+		  for (free_proc = vtab2->ts.u.derived->components;
+		       free_proc; free_proc = free_proc->next)
+		    if (free_proc->name[0] == '_'
+			&& free_proc->name[1] == 'f')
+		      break;
+
+		  if (!free_proc)
+		    goto end_vtab;
+
+		  if (gfc_add_component (vtype, "_free", &c) == FAILURE)
+		    goto cleanup;
+		  c->attr.proc_pointer = 1;
+		  c->attr.access = ACCESS_PRIVATE;
+		  c->tb = XCNEW (gfc_typebound_proc);
+		  c->tb->ppc = 1;
+		  /* Not sure about this part */
+		  tmp1 = gfc_lval_expr_from_sym (free_proc->ts.interface);
+		  tmp2 = gfc_copy_expr (tmp1);
+		  c->initializer = tmp2;
+		  c->ts.interface = tmp2->symtree->n.sym;
+		  goto end_vtab;
+
+		}
+
+	      if (derived->attr.alloc_comp || der_comp_alloc
+		  || class_comp_alloc)
+		{
+		  gfc_alloc *head = NULL;
+		  if (gfc_add_component (vtype, "_free", &c) == FAILURE)
+		    goto cleanup;
+		  c->attr.proc_pointer = 1;
+		  c->attr.access = ACCESS_PRIVATE;
+		  c->tb = XCNEW (gfc_typebound_proc);
+		  c->tb->ppc = 1;
+		  if (derived->attr.abstract)
+		    c->initializer = gfc_get_null_expr (NULL);
+		  else
+		    {
+		      /* Set up namespace.  */
+		      gfc_namespace *sub_ns2 = gfc_get_namespace (ns, 0);
+		      sub_ns2->sibling = ns->contained;
+		      ns->contained = sub_ns2;
+		      sub_ns2->resolved = 1;
+		      /* Set up procedure symbol.  */
+		      sprintf (name, "__free_%s", tname);
+		      gfc_get_symbol (name, sub_ns2, &free);
+		      sub_ns2->proc_name = free;
+		      free->attr.flavor = FL_PROCEDURE;
+		      free->attr.subroutine = 1;
+		      free->attr.if_source = IFSRC_DECL;
+		      /* This is elemental so that arrays are automatically
+		      treated correctly by the scalarizer.  */
+		      free->attr.elemental = 1;
+		      if (ns->proc_name->attr.flavor == FL_MODULE)
+			free->module = ns->proc_name->name;
+		      gfc_set_sym_referenced (free);
+		      /* Set up formal arguments.  */
+		      gfc_get_symbol ("tofree", sub_ns2, &tofree);
+		      tofree->ts.type = BT_DERIVED;
+		      tofree->ts.u.derived = derived;
+		      tofree->attr.flavor = FL_VARIABLE;
+		      tofree->attr.dummy = 1;
+		      tofree->attr.intent = INTENT_OUT;
+		      gfc_set_sym_referenced (tofree);
+		      free->formal = gfc_get_formal_arglist ();
+		      free->formal->sym = tofree;
+		      /* Set up code.  */
+		      sub_ns2->code = gfc_get_code ();
+		      sub_ns2->code->op = EXEC_NOP;
+		      head = gfc_get_alloc ();
+		      head->expr = gfc_lval_expr_from_sym (tofree);
+		      sub_ns2->code->ext.alloc.list = head;
+		      /* Set initializer.  */
+		      c->initializer = gfc_lval_expr_from_sym (free);
+		      c->ts.interface = free;
+		    }
+		}
+end_vtab:
 	      /* Add procedure pointers for type-bound procedures.  */
 	      add_procs_to_declared_vtab (derived, vtype);
 	    }
@@ -935,6 +1049,10 @@ 
 	gfc_commit_symbol (src);
       if (dst)
 	gfc_commit_symbol (dst);
+      if (free)
+	gfc_commit_symbol (free);
+      if (tofree)
+	gfc_commit_symbol (tofree);
     }
   else
     gfc_undo_symbols ();
Index: gcc/fortran/trans-stmt.c
===================================================================
--- gcc/fortran/trans-stmt.c	(revisione 188002)
+++ gcc/fortran/trans-stmt.c	(copia locale)
@@ -5343,6 +5343,11 @@ 
     {
       gfc_expr *expr = gfc_copy_expr (al->expr);
       gcc_assert (expr->expr_type == EXPR_VARIABLE);
+      gfc_expr *ppc;
+      gfc_code *ppc_code;
+      gfc_actual_arglist *actual;
+      gfc_component *free_proc = NULL;
+      gfc_symbol *vtab2 = NULL, *tmp_sym = NULL;
 
       if (expr->ts.type == BT_CLASS)
 	gfc_add_data_component (expr);
@@ -5354,6 +5359,43 @@ 
       se.descriptor_only = 1;
       gfc_conv_expr (&se, expr);
 
+      actual = gfc_get_actual_arglist ();
+      actual->expr = gfc_copy_expr (expr);
+      if (expr->symtree->n.sym->ts.type == BT_CLASS
+	  && expr->symtree->n.sym->tlink
+	  && expr->symtree->n.sym->tlink->ts.u.derived)
+	{
+	  if (expr->ref && expr->ref->u.c.component->ts.type == BT_CLASS)
+	    {
+	      tmp_sym = expr->ref->u.c.component->ts.u.derived;
+	      tmp_sym = tmp_sym->components->ts.u.derived;
+	    }
+	  else
+	    {
+	      tmp_sym = expr->symtree->n.sym->tlink->ts.u.derived;
+	    }
+	  vtab2 = gfc_find_derived_vtab (tmp_sym);
+	  vtab2 = vtab2->ts.u.derived;
+	  for (free_proc = vtab2->components;
+	       free_proc; free_proc = free_proc->next)
+	    if (free_proc->name[0] == '_'
+		&& free_proc->name[1] == 'f')
+	      break;
+	  if (free_proc)
+	    {
+	      ppc = gfc_copy_expr(free_proc->initializer);
+	      ppc_code = gfc_get_code ();
+	      ppc_code->resolved_sym = ppc->symtree->n.sym;
+	      ppc_code->resolved_sym->attr.elemental = 1;
+	      ppc_code->ext.actual = actual;
+	      ppc_code->expr1 = ppc;
+	      ppc_code->op = EXEC_CALL;
+	      tmp = gfc_trans_call (ppc_code, true, NULL, NULL, false);
+	      gfc_free_statements (ppc_code);
+	      gfc_add_expr_to_block (&block, tmp);
+	    }
+	}
+
       if (expr->rank || gfc_is_coarray (expr))
 	{
 	  if (expr->ts.type == BT_DERIVED && expr->ts.u.derived->attr.alloc_comp)