diff mbox

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

Message ID 52F54545.4090009@sfr.fr
State New
Headers show

Commit Message

Mikael Morin Feb. 7, 2014, 8:42 p.m. UTC 
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

 	    {
 	      fini_elem = fini;

Comments

Mikael Morin Feb. 7, 2014, 8:46 p.m. UTC | #1 
Le 07/02/2014 21:42, Mikael Morin a écrit :
> maybe add gcc_assert to
> make it clear that fini->proc_tree should be set at this point.
Or better: a comment ;-)
diff mbox

Patch

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)