[Fortran] PR 58470: [4.9 Regression] [OOP] ICE on invalid with FINAL procedure and type extension

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.

Also the fact that its return value is not used here tells you that we
are not actually interested if the type is finalizable at this point.
The call is bogus and should be removed, I think.


> On the
> other hand it is regression free...

Well, finalization is new in 4.9, and Dominique has argued that the
test suite may not have sufficient coverage yet. The absence of
regressions may not be enough to conclude that the patch is correct.

But after all I think that the patch should not hurt. After giving it
some second thoughts, the only alternative I could see is this:



It also gets rid of the ICE, but I haven't regtested it yet. Does this
look better to you than the original patch? (It might give duplicate
error messages in some cases?)

Cheers,
Janus

> But after all I think that the patch should not hurt. After giving it
> some second thoughts, the only alternative I could see is this:
>
> Index: gcc/fortran/resolve.c
> ===================================================================
> --- gcc/fortran/resolve.c    (revision 207485)
> +++ gcc/fortran/resolve.c    (working copy)
> @@ -11224,13 +11224,6 @@ gfc_resolve_finalizers (gfc_symbol* derived)
>        gfc_finalizer* i;
>        int my_rank;
>
> -      /* Skip this finalizer if we already resolved it.  */
> -      if (list->proc_tree)
> -    {
> -      prev_link = &(list->next);
> -      continue;
> -    }
> -
>        /* Check this exists and is a SUBROUTINE.  */
>        if (!list->proc_sym->attr.subroutine)
>      {
>
>
> It also gets rid of the ICE, but I haven't regtested it yet. Does this
> look better to you than the original patch? (It might give duplicate
> error messages in some cases?)

Unfortunately this ICEs on a good number of finalize_* test cases ...

Cheers,
Janus