Le 06/02/2014 23:40, Janus Weil a écrit : > Hi Mikael, > > thanks for your comments ... > >>> attached is a small patch which fixes an ICE-on-invalid regression >>> with finalization. In the PR, Dominique objected to the patch, but I >>> think it's the correct thing to do after all. The line that I'm >>> removing was added in a patch authored by Tobias and myself. I suspect >>> it was added to work around some other problem in the finalization >>> implementation, and there is no evidence it's actually needed. >>> >>> The patch regtests cleanly on x86_64-unknown-linux-gnu. Ok for trunk? >>> >> Wait a bit; let's try to understand the problem. >> >> Normally I would say calling gfc_is_finalizable here in >> resolve_fl_derived0 is harmless because gfc_resolve_finalizers has been >> called before in resolve_fl_derived. >> BUT: >> resolve_fl_derived0 recurses on its parent type, while >> gfc_resolve_finalizers doesn't; and in this case we end up recursing on >> type "cfml" whose finalizers haven't been resolved yet. > > Yes, that's more or less what happens. > > And the real problem is that gfc_is_finalzable already generates the > finalization wrapper (via gfc_find_derived_vtab -> > generate_finalizaton_wrapper) before we have checked that the > finalizer is actually valid (which is what gfc_resolve_finalizers > does). Once we get into gfc_resolve_finalizers, it is fooled to > believe that the finalizer has already been resolved and therefore > skips the checks and produces no error message. > > >> Now whether your patch is the right thing to do... I'm a bit skeptical >> about removing the one use of gfc_is_finalizable in resolve.c. > > Well, all others occurrences of 'gfc_is_finalizable' are in trans*, so > this is the only one that comes too early. > Yeah OK. gfc_is_finalizable is almost a no-op anyway (assuming the vtab has been generated at resolution stage). I suggest the following additional patch to make sure that the finalization wrapper is never generated without prior resolution (in this case, it replaces one ICE with another); maybe add gcc_assert to make it clear that fini->proc_tree should be set at this point. Patch is OK anyway. Thanks.
Mikael diff --git a/gcc/fortran/class.c b/gcc/fortran/class.c index d3569fd..20488c0 100644 --- a/gcc/fortran/class.c +++ b/gcc/fortran/class.c @@ -1880,8 +1880,6 @@ generate_finalization_wrapper (gfc_symbol *derived, gfc_namespace *ns, for (fini = derived->f2k_derived->finalizers; fini; fini = fini->next) { - if (!fini->proc_tree) - fini->proc_tree = gfc_find_sym_in_symtree (fini->proc_sym); if (fini->proc_tree->n.sym->attr.elemental) { fini_elem = fini;