Bug 50640 - [4.7 Regression] [OOP] FAIL: gfortran.dg/select_type_12.f03 -O (internal compiler error)
[4.7 Regression] [OOP] FAIL: gfortran.dg/select_type_12.f03 -O (internal co...
Status: RESOLVED FIXED
Product: gcc
Classification: Unclassified
Component: fortran
4.7.0
: P1 normal
: 4.7.0
Assigned To: Not yet assigned to anyone
: ice-on-valid-code
: 50657 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-06 21:21 UTC by Dominique d'Humieres
Modified: 2012-09-11 13:44 UTC (History)
10 users (show)

See Also:
Host:
Target: i686-pc-linux-gnu
Build:
Known to work:
Known to fail:
Last reconfirmed: 2011-11-06 00:00:00


Attachments
Possible patch which pushes the vtab to the toplevel (1.66 KB, text/plain)
2011-11-06 19:42 UTC, Tobias Burnus
Details
Updated draft patch (1.55 KB, patch)
2011-11-09 10:07 UTC, Tobias Burnus
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominique d'Humieres 2011-10-06 21:21:33 UTC
Between revisions 179600 (OK) and 179626 (ICE) compiling gfortran.dg/select_type_12.f03 with -O gives

/opt/gcc/work/gcc/testsuite/gfortran.dg/select_type_12.f03: In function 'MAIN__':
/opt/gcc/work/gcc/testsuite/gfortran.dg/select_type_12.f03:28:0: internal compiler error: Segmentation fault

(see http://gcc.gnu.org/ml/gcc-testresults/2011-10/msg00714.html).
Comment 1 Dominique d'Humieres 2011-10-06 21:38:36 UTC
Backtrace

Program received signal SIGSEGV, Segmentation fault.
mark_all_vars_used_1 (tp=<value optimized out>, walk_subtrees=<value optimized out>, data=<value optimized out>)
    at ../../work/gcc/tree-flow-inline.h:562
562	  ann->used = true;
(gdb) bt
#0  mark_all_vars_used_1 (tp=<value optimized out>, walk_subtrees=<value optimized out>, data=<value optimized out>)
    at ../../work/gcc/tree-flow-inline.h:562
#1  0x00000001009ca496 in walk_tree_1 (tp=<value optimized out>, func=<value optimized out>, data=<value optimized out>, pset=<value optimized out>, 
    lh=<value optimized out>) at ../../work/gcc/tree.c:10448
#2  0x00000001009cab23 in walk_tree_1 (tp=<value optimized out>, func=<value optimized out>, data=<value optimized out>, pset=<value optimized out>, 
    lh=<value optimized out>) at ../../work/gcc/tree.c:10526
#3  0x000000010089c1f5 in remove_unused_locals () at ../../work/gcc/tree-ssa-live.c:595
#4  0x00000001006d4afe in execute_function_todo (data=<value optimized out>) at ../../work/gcc/passes.c:1695
#5  0x00000001006d540e in execute_todo (flags=49156) at ../../work/gcc/passes.c:1741
#6  0x00000001006d898a in execute_one_pass (pass=0x100ddb660) at ../../work/gcc/passes.c:2087
#7  0x00000001006d8cdd in execute_pass_list (pass=0x100ddb660) at ../../work/gcc/passes.c:2119
#8  0x00000001006d7d59 in do_per_function_toporder (callback=0x1006d8cc0 <execute_pass_list(opt_pass*)>, data=0x100ddb960)
    at ../../work/gcc/passes.c:1606
#9  0x00000001006d91cd in execute_ipa_pass_list (pass=0x100ddba80) at ../../work/gcc/passes.c:2437
#10 0x00000001003dfba5 in cgraph_optimize () at ../../work/gcc/cgraphunit.c:2011
#11 0x00000001003e015a in cgraph_finalize_compilation_unit () at ../../work/gcc/cgraphunit.c:1312
#12 0x0000000100668b5d in write_global_declarations () at ../../work/gcc/langhooks.c:303
#13 0x0000000100799a20 in toplev_main (argc=3, argv=0x7fff5fbfd998) at ../../work/gcc/toplev.c:581
#14 0x0000000100001a54 in start ()
Comment 2 Dominique d'Humieres 2011-10-07 16:15:28 UTC
*** Bug 50657 has been marked as a duplicate of this bug. ***
Comment 3 Michael Matz 2011-10-07 16:45:55 UTC
Hmm, this is not as trivial as PR50638.  fortran frontend generates this
static variable local to MAIN:

static struct __vtype_MAIN___T1 __vtab_MAIN___T1 = {._hash=41707971, ._size=4, ._extends=0B, ._def_init=&__def_init_MAIN___T1, ._copy=__copy_MAIN___T1};

(the reference to &__def_init_MAIN___T1 will later cause the segfault).
__vtab_MAIN___T1  will be referenced from __vtab_MAIN___T2 (also static local
in MAIN).  But __vtab_MAIN___T2 will _not_ be referenced from MAIN.

Instead references to __vtab_MAIN___T2 and __vtab_MAIN___T1 are emitted from
function 'fun'.  So far so good.  The problem lies in walking DECL_INITIAL
for these statics.  add_referenced_var will only walk it if DECL_CONTEXT==current_function_decl (sensible).  Nobody calls add_rerefenced_var(__vtab_MAIN___T1) for function MAIN (sensible, as there
are no references).

We will call add_rerefenced_var(__vtab_MAIN___T1) from function 'fun' (also
sane), which then won't walk DECL_INITIAL, i.e. nobody makes __def_init_MAIN___T1 referenced.

The the code in tree-ssa-live comes and walks initializers of local variables
that are used.  It does so for function MAIN and variable __vtab_MAIN___T
(which is in local_decls, just not in referenced_vars), because it is marked
used (it's a non-local decl, therefore var_ann is still active from the calls
from function 'fun').  That one then walks the initializer and now notices
that nothing in it is marked referenced.

The non-triviality here lies in deciding who's wrong: add_referenced_var
(i.e. should it always mark referenced all initializers, even though they
might come from other function contexts), the tree-ssa-live code (should
it perhaps not look into initializers or at least expect them not necessarily
marked), or in the fortran frontend (should it perhaps call add_referenced_var
also for the local static in MAIN).
Comment 4 Michael Matz 2011-10-07 16:50:10 UTC
Or another solution: should the fortran frontend perhaps put all these
variables not in MAINs scope (where they aren't referenced anyway), but
rather into file scope?  That last solution would probably be the sanest.
Comment 5 Tobias Burnus 2011-10-07 16:57:08 UTC
(In reply to comment #4)
> Or another solution: should the fortran frontend perhaps put all these
> variables not in MAINs scope (where they aren't referenced anyway), but
> rather into file scope?  That last solution would probably be the sanest.

Looks sensible. Janus & Paul, What do you think?

(One can use "pushdecl_top_level (decl)" to push variable decls to file level.)
Comment 6 Richard Biener 2011-10-07 18:53:44 UTC
(In reply to comment #3)
> Hmm, this is not as trivial as PR50638.  fortran frontend generates this
> static variable local to MAIN:
> 
> static struct __vtype_MAIN___T1 __vtab_MAIN___T1 = {._hash=41707971, ._size=4,
> ._extends=0B, ._def_init=&__def_init_MAIN___T1, ._copy=__copy_MAIN___T1};
> 
> (the reference to &__def_init_MAIN___T1 will later cause the segfault).
> __vtab_MAIN___T1  will be referenced from __vtab_MAIN___T2 (also static local
> in MAIN).  But __vtab_MAIN___T2 will _not_ be referenced from MAIN.
> 
> Instead references to __vtab_MAIN___T2 and __vtab_MAIN___T1 are emitted from
> function 'fun'.  So far so good.  The problem lies in walking DECL_INITIAL
> for these statics.  add_referenced_var will only walk it if
> DECL_CONTEXT==current_function_decl (sensible).  Nobody calls
> add_rerefenced_var(__vtab_MAIN___T1) for function MAIN (sensible, as there
> are no references).
> 
> We will call add_rerefenced_var(__vtab_MAIN___T1) from function 'fun' (also
> sane), which then won't walk DECL_INITIAL, i.e. nobody makes
> __def_init_MAIN___T1 referenced.
> 
> The the code in tree-ssa-live comes and walks initializers of local variables
> that are used.  It does so for function MAIN and variable __vtab_MAIN___T
> (which is in local_decls, just not in referenced_vars), because it is marked
> used (it's a non-local decl, therefore var_ann is still active from the calls
> from function 'fun').  That one then walks the initializer and now notices
> that nothing in it is marked referenced.
> 
> The non-triviality here lies in deciding who's wrong: add_referenced_var
> (i.e. should it always mark referenced all initializers, even though they
> might come from other function contexts), the tree-ssa-live code (should
> it perhaps not look into initializers or at least expect them not necessarily
> marked), or in the fortran frontend (should it perhaps call add_referenced_var
> also for the local static in MAIN).

Huh, I don't see why tree-ssa-live would need to walk variable initializers
(which generally don't exist unless for static storage duration vars).
Comment 7 Richard Biener 2011-10-07 18:59:26 UTC
(In reply to comment #6)
> (In reply to comment #3)
> > Hmm, this is not as trivial as PR50638.  fortran frontend generates this
> > static variable local to MAIN:
> > 
> > static struct __vtype_MAIN___T1 __vtab_MAIN___T1 = {._hash=41707971, ._size=4,
> > ._extends=0B, ._def_init=&__def_init_MAIN___T1, ._copy=__copy_MAIN___T1};
> > 
> > (the reference to &__def_init_MAIN___T1 will later cause the segfault).
> > __vtab_MAIN___T1  will be referenced from __vtab_MAIN___T2 (also static local
> > in MAIN).  But __vtab_MAIN___T2 will _not_ be referenced from MAIN.
> > 
> > Instead references to __vtab_MAIN___T2 and __vtab_MAIN___T1 are emitted from
> > function 'fun'.  So far so good.  The problem lies in walking DECL_INITIAL
> > for these statics.  add_referenced_var will only walk it if
> > DECL_CONTEXT==current_function_decl (sensible).  Nobody calls
> > add_rerefenced_var(__vtab_MAIN___T1) for function MAIN (sensible, as there
> > are no references).
> > 
> > We will call add_rerefenced_var(__vtab_MAIN___T1) from function 'fun' (also
> > sane), which then won't walk DECL_INITIAL, i.e. nobody makes
> > __def_init_MAIN___T1 referenced.
> > 
> > The the code in tree-ssa-live comes and walks initializers of local variables
> > that are used.  It does so for function MAIN and variable __vtab_MAIN___T
> > (which is in local_decls, just not in referenced_vars), because it is marked
> > used (it's a non-local decl, therefore var_ann is still active from the calls
> > from function 'fun').  That one then walks the initializer and now notices
> > that nothing in it is marked referenced.
> > 
> > The non-triviality here lies in deciding who's wrong: add_referenced_var
> > (i.e. should it always mark referenced all initializers, even though they
> > might come from other function contexts), the tree-ssa-live code (should
> > it perhaps not look into initializers or at least expect them not necessarily
> > marked), or in the fortran frontend (should it perhaps call add_referenced_var
> > also for the local static in MAIN).
> 
> Huh, I don't see why tree-ssa-live would need to walk variable initializers
> (which generally don't exist unless for static storage duration vars).

Possibly for

void foo (void)
{
  static int i;
  static int *p = &i;
  return *p;
}

to make i used and thus retained in BLOCK_VARS and thus debuginfo.
Comment 8 Hans-Peter Nilsson 2011-10-15 08:47:07 UTC
Still there, so I'll confirm it; cris-elf at r180014 too, introduced r179598:179611.
Comment 9 janus 2011-10-15 09:13:02 UTC
(In reply to comment #5)
> (In reply to comment #4)
> > Or another solution: should the fortran frontend perhaps put all these
> > variables not in MAINs scope (where they aren't referenced anyway), but
> > rather into file scope?  That last solution would probably be the sanest.
> 
> Looks sensible. Janus & Paul, What do you think?

Yes, I think that would be ok. The 'vtab' symbols are really meant to be global.

(Sorry for the late answer. I almost forgot about this one.)
Comment 10 Hans-Peter Nilsson 2011-11-06 14:12:55 UTC
This regression is still present at r181026.
It seemed there was work in progress last I looked.
If not, it should be xfailed.
Let me know and I'll do the honors.
Comment 11 Thomas Koenig 2011-11-06 17:07:26 UTC
(In reply to comment #10)
> This regression is still present at r181026.
> It seemed there was work in progress last I looked.
> If not, it should be xfailed.
> Let me know and I'll do the honors.

Wouln't standard procedure be to revert the offending patch?
Comment 12 Dominique d'Humieres 2011-11-06 17:20:08 UTC
(In reply to comment #10)
> It seemed there was work in progress last I looked.
> If not, it should be xfailed.

Please don't.

(In reply to comment #11)
> Wouldn't standard procedure be to revert the offending patch?

Well, first, I am not sure that the "offending patch" is identified yet, second I would prefer to see the bug fixed rather than hidden by any of the above.
Comment 13 Hans-Peter Nilsson 2011-11-06 17:47:16 UTC
(In reply to comment #12)
> second
> I would prefer to see the bug fixed

Certainly.

> rather than hidden by any of the above.

It wouldn't be hidden by xfailing it, as the xfail would refer to this PR.

And this _is_ standard procedure if the fix is more important than errors it uncovers (which of course isn't known until the offending change is known, but if the regression is unimportant sometimes it doesn't matter - probably not the case here as it's an ICE).

But the implied question is unanswered: is there work in progress on this regression?

I thought the change introducing the regression was known, but apparently not; would it help to triage it?
Comment 14 janus 2011-11-06 18:16:00 UTC
(In reply to comment #12)
> > Wouldn't standard procedure be to revert the offending patch?
> 
> Well, first, I am not sure that the "offending patch" is identified yet

According to comment 0 and comment 8, it should be between 179600 and 179611. I have to admit that I don't see any obvious candidate at first sight, but maybe someone else does? (Half of the commits in this range is not even on the trunk.)
Comment 15 Dominique d'Humieres 2011-11-06 19:20:45 UTC
(In reply to comment #14)
> According to comment 0 and comment 8, it should be between 179600 and 179611. I
> have to admit that I don't see any obvious candidate at first sight, but maybe
> someone else does? (Half of the commits in this range is not even on the
> trunk.)

From comment #3 I infered that the usual suspect is r179618. Hans-Peter are you sure about you range in comment #8?
Comment 16 Tobias Burnus 2011-11-06 19:42:22 UTC
Created attachment 25730 [details]
Possible patch which pushes the vtab to the toplevel

Based on Michael's suggestion (comment 4), this patch pushes the vtable to the top level. I am not sure whether one needs to fix other occurrences as well or whether it regtests, but it fixes select_type_12.f03.
(Additionally, I set TREE_READONLY to fix PR 50960.)
Comment 17 Hans-Peter Nilsson 2011-11-06 20:02:49 UTC
(In reply to comment #15)
> From comment #3 I infered that the usual suspect is r179618. Hans-Peter are you
> sure about you range in comment #8?

You're right, my mistake, sorry.  My autotester logged the observed worked:failed range as 179611:179625; I somehow quoted the range before that.
Comment 18 Tobias Burnus 2011-11-07 08:11:34 UTC
(In reply to comment #16)
> Created attachment 25730 [details]
> Possible patch which pushes the vtab to the toplevel

At least that approach fails for the following file (of PSBLAS 3's configure), which yields an
  internal compiler error: in cgraph_analyze_functions, at cgraphunit.c:1193

which is at
    fprintf (cgraph_dump_file, "\nReclaiming functions:");
  for (node = cgraph_nodes; node != first_analyzed; node = next)
    {
       ...
       gcc_assert (node->analyzed == node->local.finalized);

program xtt
  type foo
    integer :: i
  end type foo
  type, extends(foo) :: new_foo
    integer :: j
  end type new_foo
  class(foo), allocatable  :: fooab
  type(new_foo) :: nfv
  integer :: info
  allocate(fooab, source=nfv, stat=info)
end program xtt
Comment 19 paul.richard.thomas@gmail.com 2011-11-07 11:00:47 UTC
Dear Tobias,

I have been keeping out of this for the time being because I want to
get some of the final OOP array issues out of the way.  I figured
that, as a regression, it could wait for a little while :-(


>> Possible patch which pushes the vtab to the toplevel

This looks like a perfectly good way to do this.

>
> At least that approach fails for the following file (of PSBLAS 3's configure),
> which yields an
>  internal compiler error: in cgraph_analyze_functions, at cgraphunit.c:1193
>
> which is at
>    fprintf (cgraph_dump_file, "\nReclaiming functions:");
>  for (node = cgraph_nodes; node != first_analyzed; node = next)
>    {
>       ...
>       gcc_assert (node->analyzed == node->local.finalized);
>
> program xtt
>  type foo
>    integer :: i
>  end type foo
>  type, extends(foo) :: new_foo
>    integer :: j
>  end type new_foo
>  class(foo), allocatable  :: fooab
>  type(new_foo) :: nfv
>  integer :: info
>  allocate(fooab, source=nfv, stat=info)
> end program xtt

That it should fail on this is utterly bizarre.  I presume that it is
the allocate statement that wipes it out?

Paul
Comment 20 Tobias Burnus 2011-11-07 16:17:34 UTC
(In reply to comment #18)
> (In reply to comment #16)
> > Created attachment 25730 [details]
> > Possible patch which pushes the vtab to the toplevel

The reason for the failure is clear: For the derived type/class, the function __copy_xtt_Foo is created. The created function is in the scope of the derived type - which is declared in program xtt. If one now places the "vtab" at the top level, the copy function is hidden in another function as nested function.

Hence, one also needs to create the function at top level; I wonder whether this can be already done at decl time in gfc_find_derived_vtab.
Comment 21 Tobias Burnus 2011-11-07 16:27:24 UTC
(In reply to comment #20)
> The reason for the failure is clear: For the derived type/class, the function
> __copy_xtt_Foo is created. The created function is in the scope of the derived
> type - which is declared in program xtt. If one now places the "vtab" at the
> top level, the copy function is hidden in another function as nested function.
> 
> Hence, one also needs to create the function at top level; I wonder whether
> this can be already done at decl time in gfc_find_derived_vtab.

Post script: I was wondering whether it is possible to move the vtab to the top level as one might get in trouble with other member functions; however, as gfortran told me, a method (type-bound procedure) "must be a module procedure or an external procedure with an explicit interface". Hence, all methods are already visible at the toplevel.

Thus, it only affects _copy which /can/ be moved to the toplevel.
Comment 22 Tobias Burnus 2011-11-09 10:07:54 UTC
Created attachment 25766 [details]
Updated draft patch

I am not sure whether it is the nicest patch, but pushing __copy to the top based by the name should work. (A Fortran name may not start with an underscore.) An alternative patch would be to handle this on the Fortran AST level.
This patch was only lightly tested by compiling select_type_12.f03 and the test of comment 18.
Comment 23 paul.richard.thomas@gmail.com 2011-11-10 11:09:02 UTC
Dear Tobias,

I tried this, in the hope of fixing a problem that I have with the
last big part of the class array patch.  To my surprise, it caused
errors like:
test1.f90:24:0: internal compiler error: in cgraph_analyze_functions,
at cgraphunit.c:1193
Please submit a full bug report,
with preprocessed source if appropriate.
See <http://gcc.gnu.org/bugs.html> for instructions.

Cheers

Paul

On Wed, Nov 9, 2011 at 11:07 AM, burnus at gcc dot gnu.org
<gcc-bugzilla@gcc.gnu.org> wrote:
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50640
>
> Tobias Burnus <burnus at gcc dot gnu.org> changed:
>
>           What    |Removed                     |Added
> ----------------------------------------------------------------------------
>  Attachment #25730 [details]|0                           |1
>        is obsolete|                            |
>
> --- Comment #22 from Tobias Burnus <burnus at gcc dot gnu.org> 2011-11-09 10:07:54 UTC ---
> Created attachment 25766 [details]
>  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=25766
> Updated draft patch
>
> I am not sure whether it is the nicest patch, but pushing __copy to the top
> based by the name should work. (A Fortran name may not start with an
> underscore.) An alternative patch would be to handle this on the Fortran AST
> level.
> This patch was only lightly tested by compiling select_type_12.f03 and the test
> of comment 18.
>
> --
> Configure bugmail: http://gcc.gnu.org/bugzilla/userprefs.cgi?tab=email
> ------- You are receiving this mail because: -------
> You are on the CC list for the bug.
>
Comment 24 Tobias Burnus 2011-11-19 09:26:37 UTC
Author: burnus
Date: Sat Nov 19 09:26:33 2011
New Revision: 181505

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=181505
Log:
2011-11-19  Tobias Burnus  <burnus@net-b.de>

        PR fortran/51207
        * class.c (gfc_find_derived_vtab): Mark __def_init as PARAMETER
        and hence as TREE_READONLY; add subroutine attribute to
        __copy_ procedure.

        PR fortran/50640
        * trans.h (GFC_DECL_PUSH_TOPLEVEL): New DECL_LANG_FLAG_7.
        * trans-decl.c (gfc_get_symbol_decl): Mark __def_init and vtab
        as GFC_DECL_PUSH_TOPLEVEL.
        (gfc_generate_function_code): If GFC_DECL_PUSH_TOPLEVEL, push it there.
        (build_function_decl): Push __copy_ procedure to the toplevel.


Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/class.c
    trunk/gcc/fortran/trans-decl.c
    trunk/gcc/fortran/trans.h
Comment 25 Tobias Burnus 2011-11-19 09:31:31 UTC
Should be FIXED. Please reopen if it isn't.

The solution was as proposed by Michael in comment 4 - and it is a slightly extended version of the patch in comment 22.
Comment 26 Tobias Burnus 2012-09-11 13:44:44 UTC
The solution of comment 3, fixed by comment 24 seems to break the test case of PR fortran/53718.

Reverting the patch (comment 24, except for unrelated class.c part) fixes the issue of PR 53718. For some reason, reverting the patch no longer triggers the issue of this PR, i.e. gfortran.dg/select_type_12.f03 gives no ICE.

Hence, it seems as if comment 2 no longer applies.


To recap (rough version): gfortran generates in MAIN__ the nested function __copy_MAIN___T1 and assigns it (function pointer) to a field of the static struct __vtab_MAIN___T1. But __copy_MAIN___T1 does not get called in MAIN__ but only in "foo" which is also a nested function of MAIN__ - thus, __vtab_MAIN___T1 didn't get marked as referenced, causing the ICE.

The patch in comment 24 hoisted the "__copy_MAIN___T1" out of MAIN__ into the TU space.


Does anyone see a reason why the patch shouldn't be revered? (I assume one of Richard's patches in May fixed the issue. That probably means that one has to find another solution for 4.7. Suggestions?)

Comments - especially from the middle-end side?