Message ID | CAHqFgjXULBJeA=Gd80sz_+Y4psju2Ka3BZoR-vS5qEqTEro=cA@mail.gmail.com |
---|---|
State | New |
Headers | show |
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
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...)
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
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.
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
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.
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
> 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
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
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)