| Message ID | 503E734A.3050801@net-b.de |
|---|---|
| State | New |
| Headers | show |
On 29/08/2012 21:53, Tobias Burnus wrote: > Dear all, > > that's the revised version of patch at > http://gcc.gnu.org/ml/fortran/2012-08/msg00095.html, taking the review > comments into account. > > Reminder: This patch only generates the finalization wrapper, which is > in the virtual table. It does not add the required calls; hence, it > still doesn't allow to use finalization. > > > The patch consists of three parts: > > a) The main patch, which implements the wrapper. > I am asking for approval for that patch. A few more nitpicks below. > > b) A patch which removes the gfc_error "not yet implemented" > I suggest to only remove the error after finalization calls have been > added Sensible. By the way some (testsuite) parts of b) should be part of a). > > c) A patch which bumps the .mod version > - or alternatively - > a patch which disables the _final generation in the vtable. > > I have build and regtested (on x86-64-linux) the patch with (a) and > (a)+(b) applied. > > > I would like to include the patch (c) as modifying the vtable changes > the ABI. Bumping the .mod version is a reliable way to force > recompilation. The alternative is to wait until the final FINAL patch > before bumping the .mod version (and disable the "_final" generation). I don't like bumping the module version, for something not module-related (vtypes are output as normal types in the module files), but I guess it is the most user-friendly thing to do. > > One possibility, if deemed useful, is to combine the .mod version bump > with backward compatible reading of .mod files, i.e., only error out > when BT_CLASS is encountered in an old .mod file. Let's keep things simple, let's not do that. > > > Is the patch (a) OK for the trunk? With which version of (c)? > > (I am slightly inclined to do the .mod bump now. As a follow up, one can > also commit Janus' proc-pointer patch, > http://gcc.gnu.org/ml/fortran/2012-04/msg00033.html, though I think > someone has still to review it.) I am inclined to disable finalization completely (thus defer .mod bump), because the new code is hardly tested and doesn't provide (yet) new benefits such as reduced memory leaks as it is disabled. We could do the bump now, but I'm afraid that we discover a bug when finalization gets completely implemented, and we have to bump again to fix it (though I don't see what kind of bug it could be). I think Janus' patch is a much less serious problem in the sense that people trying to link codes compiled with patched and non-patched compiler will get a link error. I don't think it's worth a .mod bump. Unless I miss something. For (a), I noticed a few indenting issues (8+ spaces) and (mostly wording) nits below to be fixed. Then OK. > diff --git a/gcc/fortran/class.c b/gcc/fortran/class.c > index 21a91ba..9d58aab 100644 > --- a/gcc/fortran/class.c > +++ b/gcc/fortran/class.c > @@ -689,6 +694,702 @@ copy_vtab_proc_comps (gfc_symbol *declared, gfc_symbol *vtype) > } > > > +/* Returns true if any of its nonpointer nonallocatable components or > + their nonpointer nonallocatable subcomponents has a finalization > + subroutine. */ > + > +static bool > +has_finalizer_component (gfc_symbol *derived) Rename to has_finalizable_component ? > +{ > + gfc_component *c; > + > + for (c = derived->components; c; c = c->next) > + { > + if (c->ts.type == BT_DERIVED && c->ts.u.derived->f2k_derived > + && c->ts.u.derived->f2k_derived->finalizers) > + return true; > + > + if (c->ts.type == BT_DERIVED > + && !c->attr.pointer && !c->attr.allocatable > + && has_finalizer_component (c->ts.u.derived)) > + return true; > + } > + return false; > +} > + > + > +/* Call DEALLOCATE for the passed component if it is allocatable, if it is > + neither allocatable nor a pointer but has a finalizer, call it. If it > + is a nonpointer component with allocatable or finalizes components, walk s/finalizes/finalizable/ ? > + them. Either of the is required; other nonallocatables and pointers aren't s/the/them/ ? > + handled gracefully. > + Note: The DEALLOCATE handling takes care of finalizers, coarray > + deregistering and allocatable components of the allocatable. */ "coarray deregistering and allocatable components" is confusing. Note: If the component is allocatable, the DEALLOCATE handling takes care of calling the appropriate finalizer(s), of coarray deregistering, and of deallocating allocatable (sub)components. > + > +static void > +finalize_component (gfc_expr *expr, gfc_symbol *derived, gfc_component *comp, > + gfc_expr *stat, gfc_code **code) [...] > + > + > +/* Generate the wrapper finalization/polymorphic freeing subroutine for the Difficult to read. "Generate the finalization/polymorphic freeing wrapper subroutine..." or something ? > + derived type "derived". The function first calls the approriate FINAL > + subroutine, then it DEALLOCATEs (finalizes/frees) the allocatable > + components (but not the inherited ones). Last, it calls the wrapper > + subroutine of the parent. The generated wrapper procedure takes as argument > + an assumed-rank array. > + If neither allocatable components nor FINAL subroutines exists, the vtab > + will contain a NULL pointer. */ > + > +static void > +generate_finalization_wrapper (gfc_symbol *derived, gfc_namespace *ns, > + const char *tname, gfc_component *vtab_final) > +{ > + gfc_symbol *final, *array, *nelem; > + gfc_symbol *ptr = NULL, *idx = NULL; > + gfc_component *comp; > + gfc_namespace *sub_ns; > + gfc_code *last_code; > + char name[GFC_MAX_SYMBOL_LEN+1]; > + bool finalizable_comp = false; > + gfc_expr *ancestor_wrapper = NULL; > + > + /* Search for the ancestor's finalizers. */ > + if (derived->attr.extension && derived->components > + && (!derived->components->ts.u.derived->attr.abstract > + || has_finalizer_component (derived))) > + { > + gfc_symbol *vtab; > + gfc_component *comp; > + > + vtab = gfc_find_derived_vtab (derived->components->ts.u.derived); > + for (comp = vtab->ts.u.derived->components; comp; comp = comp->next) > + if (comp->name[0] == '_' && comp->name[1] == 'f') > + { > + 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 > + && (!derived->f2k_derived || !derived->f2k_derived->finalizers) > + && !has_finalizer_component (derived)) > + { > + vtab_final->initializer = gfc_get_null_expr (NULL); > + return; > + } > + > + /* Check whether there are new allocatable components. */ > + for (comp = derived->components; comp; comp = comp->next) > + { > + if (comp == derived->components && derived->attr.extension > + && ancestor_wrapper && ancestor_wrapper->expr_type != EXPR_NULL) > + continue; > + > + if (comp->ts.type != BT_CLASS && !comp->attr.pointer > + && (comp->attr.alloc_comp || comp->attr.allocatable > + || (comp->ts.type == BT_DERIVED > + && has_finalizer_component (comp->ts.u.derived)))) > + finalizable_comp = true; > + else if (comp->ts.type == BT_CLASS && CLASS_DATA (comp) > + && CLASS_DATA (comp)->attr.allocatable) > + finalizable_comp = true; > + } > + > + /* If there is no new finalizer and no new allocatable, return with > + an expr to the ancestor's one. */ > + if ((!derived->f2k_derived || !derived->f2k_derived->finalizers) > + && !finalizable_comp) > + { > + vtab_final->initializer = gfc_copy_expr (ancestor_wrapper); > + return; > + } > + > + /* We now create a wrapper, which does the following: > + 1. It calls the suitable finalization subroutine for this type s/It calls/Call/ ? (to be in line with the other verbs). > + 2. In a loop over all noninherited allocatable components and noninherited s/In a loop/Loop/ > + components with allocatable components and DEALLOCATE those; this will > + take care of finalizers, coarray deregistering and allocatable > + nested components. > + 3. Call the ancestor's finalizer. */ > + > + /* Declare the wrapper function; it takes an assumed-rank array > + as argument. */ > + Thanks. Mikael
Mikael Morin wrote: > On 29/08/2012 21:53, Tobias Burnus wrote: >> a) The main patch, which implements the wrapper. >> I am asking for approval for that patch. > A few more nitpicks below. > >> I would like to include the patch (c) as modifying the vtable changes >> the ABI. Bumping the .mod version is a reliable way to force >> recompilation. The alternative is to wait until the final FINAL patch >> before bumping the .mod version (and disable the "_final" generation). > I don't like bumping the module version, for something not > module-related (vtypes are output as normal types in the module files), > but I guess it is the most user-friendly thing to do. I also do not like it - but it might otherwise lead to segfaults at run time (or the wrong procedure being called), which is even uglier. >> Is the patch (a) OK for the trunk? With which version of (c)? >> >> (I am slightly inclined to do the .mod bump now. As a follow up, one can >> also commit Janus' proc-pointer patch, >> http://gcc.gnu.org/ml/fortran/2012-04/msg00033.html, though I think >> someone has still to review it.) > I am inclined to disable finalization completely (thus defer .mod bump), > because the new code is hardly tested and doesn't provide (yet) new > benefits such as reduced memory leaks as it is disabled. > We could do the bump now, but I'm afraid that we discover a bug when > finalization gets completely implemented, and we have to bump again to > fix it (though I don't see what kind of bug it could be). I concur. > I think Janus' patch is a much less serious problem in the sense that > people trying to link codes compiled with patched and non-patched > compiler will get a link error. I don't think it's worth a .mod bump. Well, the idea was: If we do bump the .mod for this patch, having around that time Janus' patch (which also breaks the ABI) makes sense. I concur that his ABI breakage is less severe. > For (a), I noticed a few indenting issues (8+ spaces) and (mostly > wording) nits below to be fixed. Then OK. Fixed. >> +/* Returns true if any of its nonpointer nonallocatable components or >> + their nonpointer nonallocatable subcomponents has a finalization >> + subroutine. */ >> + >> +static bool >> +has_finalizer_component (gfc_symbol *derived) > Rename to has_finalizable_component ? I prefer finalizer because also allocatable components are finalizable but they are excluded by this check. >> +/* Call DEALLOCATE for the passed component if it is allocatable, if it is >> + neither allocatable nor a pointer but has a finalizer, call it. If it >> + is a nonpointer component with allocatable or finalizes components, walk > s/finalizes/finalizable/ ? Actually: with allocatable components or has finalizers. >> + them. Either of the is required; other nonallocatables and pointers aren't > s/the/them/ ? done. >> + handled gracefully. >> + Note: The DEALLOCATE handling takes care of finalizers, coarray >> + deregistering and allocatable components of the allocatable. */ > "coarray deregistering and allocatable components" is confusing. > > Note: If the component is allocatable, the DEALLOCATE handling takes > care of calling the appropriate finalizer(s), of coarray deregistering, > and of deallocating allocatable (sub)components. Done. >> +/* Generate the wrapper finalization/polymorphic freeing subroutine for the > Difficult to read. > "Generate the finalization/polymorphic freeing wrapper subroutine..." or > something ? Done. >> + /* We now create a wrapper, which does the following: >> + 1. It calls the suitable finalization subroutine for this type > s/It calls/Call/ ? (to be in line with the other verbs). >> + 2. In a loop over all noninherited allocatable components and noninherited > s/In a loop/Loop/ Done. Thanks for the review. I have no committed it as Rev. 190869 Tobias
-------------------------------------------------------
NOTE: Only one of the two patchlets should be committed
not both!
-------------------------------------------------------
2012-08-19 Alessandro Fanfarillo <fanfarillo.gcc@gmail.com>
Tobias Burnus <burnus@net-b.de>
PR fortran/37336
* module.c (MOD_VERSION): Bump to for recompilation
after the vtable ABI has changed.
* class.c (gfc_find_derived_vtab): Disable the FINAL
wrapper generation.
diff --git a/gcc/fortran/class.c b/gcc/fortran/class.c
index 9d58aab..8c51302 100644
--- a/gcc/fortran/class.c
+++ b/gcc/fortran/class.c
@@ -1624,13 +1624,14 @@ gfc_find_derived_vtab (gfc_symbol *derived)
Note: The actual wrapper function can only be generated
at resolution time. */
+ /* TODO: Enabled when FINAL is implemented. */
if (gfc_add_component (vtype, "_final", &c) == FAILURE)
goto cleanup;
c->attr.proc_pointer = 1;
c->attr.access = ACCESS_PRIVATE;
c->tb = XCNEW (gfc_typebound_proc);
c->tb->ppc = 1;
- generate_finalization_wrapper (derived, ns, tname, c);
+ generate_finalization_wrapper (derived, ns, tname, c);*/
/* Add procedure pointers for type-bound procedures. */
add_procs_to_declared_vtab (derived, vtype);
diff --git a/gcc/fortran/module.c b/gcc/fortran/module.c
index bfd8b01..b8f0b02 100644
--- a/gcc/fortran/module.c
+++ b/gcc/fortran/module.c
@@ -82,7 +82,7 @@ along with GCC; see the file COPYING3. If not see
/* Don't put any single quote (') in MOD_VERSION,
if yout want it to be recognized. */
-#define MOD_VERSION "9"
+#define MOD_VERSION "10"
/* Structure that describes a position within a module file. */
Dear all, that's the revised version of patch at http://gcc.gnu.org/ml/fortran/2012-08/msg00095.html, taking the review comments into account. Reminder: This patch only generates the finalization wrapper, which is in the virtual table. It does not add the required calls; hence, it still doesn't allow to use finalization. The patch consists of three parts: a) The main patch, which implements the wrapper. I am asking for approval for that patch. b) A patch which removes the gfc_error "not yet implemented" I suggest to only remove the error after finalization calls have been added c) A patch which bumps the .mod version - or alternatively - a patch which disables the _final generation in the vtable. I have build and regtested (on x86-64-linux) the patch with (a) and (a)+(b) applied. I would like to include the patch (c) as modifying the vtable changes the ABI. Bumping the .mod version is a reliable way to force recompilation. The alternative is to wait until the final FINAL patch before bumping the .mod version (and disable the "_final" generation). One possibility, if deemed useful, is to combine the .mod version bump with backward compatible reading of .mod files, i.e., only error out when BT_CLASS is encountered in an old .mod file. Is the patch (a) OK for the trunk? With which version of (c)? (I am slightly inclined to do the .mod bump now. As a follow up, one can also commit Janus' proc-pointer patch, http://gcc.gnu.org/ml/fortran/2012-04/msg00033.html, though I think someone has still to review it.) Tobias PS: When doing the ABI change, I am going to document it in the release notes / wiki.