Bug 49648 - ICE(segfault) with MATMUL and function-result actual argument
Summary: ICE(segfault) with MATMUL and function-result actual argument
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: fortran (show other bugs)
Version: 4.7.0
: P3 normal
Target Milestone: ---
Assignee: Mikael Morin
URL:
Keywords: ice-on-valid-code
Depends on:
Blocks: 32834
  Show dependency treegraph
 
Reported: 2011-07-05 17:05 UTC by Tobias Burnus
Modified: 2011-07-18 20:42 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail: 4.1.2, 4.3.4, 4.4.0, 4.5.1, 4.6.0, 4.7.0
Last reconfirmed: 2011-07-07 21:12:19


Attachments
test case (365 bytes, text/plain)
2011-07-05 17:05 UTC, Tobias Burnus
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Tobias Burnus 2011-07-05 17:05:38 UTC
Created attachment 24696 [details]
test case

With gfortran 4.1 to 4.7, I get a segfault for the attached program:

test.f90: In function ‘gf_generateembpot’:
test.f90:31:0: internal compiler error: Segmentation fault


==1006== Invalid read of size 4
==1006==    at 0x57C534: gfc_walk_subexpr (trans-array.c:7509)
==1006==    by 0x57CC10: gfc_walk_expr (trans-array.c:7893)
==1006==    by 0x5A2B32: gfc_conv_intrinsic_size (trans-intrinsic.c:4947)
==1006==    by 0x5B159C: gfc_conv_intrinsic_function (trans-intrinsic.c:6595)
==1006==    by 0x5A0532: gfc_conv_function_expr (trans-expr.c:4064)
==1006==    by 0x59C629: gfc_apply_interface_mapping (trans-expr.c:2313)
==1006==    by 0x57229B: gfc_set_loop_bounds_from_array_spec (trans-array.c:585)
==1006==    by 0x59FC79: gfc_conv_procedure_call (trans-expr.c:3509)
Comment 1 Tobias Burnus 2011-07-05 17:14:31 UTC
At some point, the array spec seems to got lost:

    COMPLEX,SAVE,ALLOCATABLE::P(:)

      COMPLEX:: PM(SIZE(P),SIZE(P))

We have:
(gdb) p expr->expr_type
$1 = EXPR_VARIABLE
(gdb) p expr->symtree->n.sym->name
$2 = 0x2aaaaab42fb8 "p"
(gdb) p expr->ref->type
$4 = REF_ARRAY
(gdb) p expr->ref->u.ar
$5 = {type = AR_FULL, dimen = 0, [...]  as = 0x0, [...]}

And in gfc_walk_variable_expr's switch (ar->type):

        case AR_FULL:
          [...]
          newss->data.info.dimen = ar->as->rank;

which is a NULL pointer access.

The question is: Why is ar->as == NULL?
Comment 2 Tobias Burnus 2011-07-05 17:29:45 UTC
The crucial point about the bug seems to be that "getPhaseMatrix" is in a (different) module than the call in matmul.

Paul, do you have an idea where to look?


module m2
  COMPLEX, SAVE, ALLOCATABLE :: P(:)
contains
  FUNCTION getPhaseMatrix() RESULT(PM)
    COMPLEX:: PM(SIZE(P),3)
    PM=0.0
  END FUNCTION
end module m2

module m
  use m2
contains
   SUBROUTINE gf_generateEmbPot()
      COMPLEX :: sigma2(3,3)
      sigma2 = MATMUL(getPhaseMatrix(), sigma2)
   END SUBROUTINE
end module m
Comment 3 Tobias Burnus 2011-07-05 19:00:51 UTC
Draft patch, which seems to fix some ref->u.ar->as issues for "p" - but not the one, I am looking for. At least the added assert now triggers.

 * * *

Debugging shows that when resolving MAXMUL, one calls resolve_actual_arglist for SIZE's p. At that point "p's e->ref->u.ar.as == NULL, which is fixed. One then resolves MAXMUL in resolve_actual_arglist for getPhaseMatrix; if one now looks at getPhaseMatrix's e->symtree->n.sym->as->upper[0], one finds:

(gdb) p arg->expr->symtree->n.sym->as->upper[0]->value.function.isym->id
$11 = GFC_ISYM_SIZE
(gdb) p arg->expr->symtree->n.sym->as->upper[0]->value.function.actual->expr->symtree->n.sym->name
$12 = 0x2aaaaab42fa0 "p"
(gdb) p arg->expr->symtree->n.sym->as->upper[0]->value.function.actual->expr->ref->u.ar.as.
A syntax error in expression, near `'.
(gdb) p arg->expr->symtree->n.sym->as->upper[0]->value.function.actual->expr->ref->u.ar.as
$13 = (gfc_array_spec *) 0x159f830

Which also looks fine.



Seemingly, the problem is really the result variable - which seems to be carried through:

(gdb) p arg->expr->symtree->n.sym->result->name
$17 = 0x2aaaaab42fb8 "pm"

And as both "as" are different:

(gdb) p arg->expr->symtree->n.sym->result->as
$18 = (gfc_array_spec *) 0x159ef70
(gdb) p arg->expr->symtree->n.sym->as
$19 = (gfc_array_spec *) 0x159cd40

it is not surprising that one get's at the end:

(gdb) p arg->expr->symtree->n.sym->result->as->upper[0]->value.function.actual->expr->ref->u.ar.as
$14 = (gfc_array_spec *) 0x0

If one removes the RESULT(), it works - even without the patch.


--- a/gcc/fortran/trans-array.c
+++ b/gcc/fortran/trans-array.c
@@ -7506,6 +7521,7 @@ gfc_walk_variable_expr (gfc_ss * ss, gfc_expr * expr)

       ar = &ref->u.ar;

+      gcc_assert (ar->as);
       if (ar->as->rank == 0 && ref->next != NULL)
        {
          /* Scalar coarray.  */
--- a/gcc/fortran/resolve.c
+++ b/gcc/fortran/resolve.c
@@ -82,6 +82,11 @@ static bitmap_obstack labels_obstack;
 /* True when simplifying a EXPR_VARIABLE argument to an inquiry function.  */
 static bool inquiry_argument = false;

+
+/* Prototypes of static functions.  */
+static gfc_try resolve_ref (gfc_expr *);
+
+
 int
 gfc_is_formal_arg (void)
 {
@@ -1577,6 +1582,9 @@ resolve_actual_arglist (gfc_actual_arglist *arg, procedure_type ptype,
            && count_specific_procs (e) != 1)
        return FAILURE;

+      if (e->ref && e->symtree->n.sym->attr.use_assoc)
+       resolve_ref (e);
+
       if (e->ts.type != BT_PROCEDURE)
        {
          save_need_full_assumed_size = need_full_assumed_size;
Comment 4 Tobias Burnus 2011-07-05 19:46:45 UTC
Draft patch, which should also save some memory, .mod file-size and should slightly speed the program.

It fixes the test case in comment 0 and comment 2, but I have not yet regtested it (currently bootstrapping). A modification would be to not write it at all - and simply add "sym->result = sym" if sym->attr.function. (This will break .mod compatibility, which was last done 2011-03-29 - though I am not 100% sure that 4.6 and 4.7 are indeed still compatible.)

--- a/gcc/fortran/module.c
+++ b/gcc/fortran/module.c
@@ -3631,7 +3637,10 @@ mio_symbol (gfc_symbol *sym)
 
   mio_array_spec (&sym->as);
 
-  mio_symbol_ref (&sym->result);
+  if (iomode == IO_OUTPUT && sym->result != sym)
+    mio_symbol_ref (&sym);
+  else
+    mio_symbol_ref (&sym->result);
 
   if (sym->attr.cray_pointee)
     mio_symbol_ref (&sym->cp_pointer);
Comment 5 Tobias Burnus 2011-07-05 22:52:01 UTC
(In reply to comment #4)
> Draft patch, which should also save some memory, .mod file-size and should
> slightly speed the program.

The patch fails for procedure pointers - as the function->attr.proc_pointer == 0 and only function->result->attr.proc_pointer != 0, cf. add_hidden_procptr_result. For instance: gfortran.dg/proc_ptr_result_1.f90 and gfortran.dg/proc_ptr_result_3.f90.
Comment 6 Tobias Burnus 2011-07-06 06:43:37 UTC
I tried something fancier by setting the proc_pointer/pointer attribute of sym->attr based on sym->result->attr, but that fails for proc_ptr_result_3.f90 (cf. failing patch below). -- In that case, "get_sub()" is a function, which returns a subroutine procedure pointer, i.e. "sym->result" is a subroutine while "sym" itself is a function.

One solution would be, to save only the result attribute - in particular the attr.subroutine/attr.function - and construct a new result symbol, re-using as much as possible from "sym" itself. One just needs to make sure that one does not double free components.

Alternatively, one should somehow make sure that also expressions below sym->result - in particular: sym->result->as->{lbound,ubound} - get fixed via a call to resolve_ref.
Comment 7 Mikael Morin 2011-07-06 14:25:10 UTC
Here is a "fix".
The important part I suppose is that one has to apply resolve_symbol on
sym->result. It is the case before the patch, only if the symbol has unknown type.

diff --git a/resolve.c b/resolve.c
index f484a22..908e72b 100644
--- a/resolve.c
+++ b/resolve.c
@@ -12181,21 +12181,22 @@ resolve_symbol (gfc_symbol *sym)
 	     in the case that there is no implicit type.  */
 	  if (!mp_flag)
 	    gfc_set_default_type (sym, sym->attr.external, NULL);
-	  else
-	    {
-	      /* Result may be in another namespace.  */
-	      resolve_symbol (sym->result);
+	}
+    }
 
-	      if (!sym->result->attr.proc_pointer)
-		{
-		  sym->ts = sym->result->ts;
-		  sym->as = gfc_copy_array_spec (sym->result->as);
-		  sym->attr.dimension = sym->result->attr.dimension;
-		  sym->attr.pointer = sym->result->attr.pointer;
-		  sym->attr.allocatable = sym->result->attr.allocatable;
-		  sym->attr.contiguous = sym->result->attr.contiguous;
-		}
-	    }
+  if (mp_flag && sym->attr.flavor == FL_PROCEDURE && sym->attr.function)
+    {
+      /* Result may be in another namespace.  */
+      resolve_symbol (sym->result);
+    
+      if (!sym->result->attr.proc_pointer)
+	{
+	  sym->ts = sym->result->ts;
+	  sym->as = gfc_copy_array_spec (sym->result->as);
+	  sym->attr.dimension = sym->result->attr.dimension;
+	  sym->attr.pointer = sym->result->attr.pointer;
+	  sym->attr.allocatable = sym->result->attr.allocatable;
+	  sym->attr.contiguous = sym->result->attr.contiguous;
 	}
     }
Comment 8 Mikael Morin 2011-07-06 15:00:56 UTC
(In reply to comment #7)
> Here is a "fix".
It breaks class_20.f03 and extends_4.f03 at least.
There is another attempt below.

(In reply to comment #6)
> Alternatively, one should somehow make sure that also expressions below
> sym->result - in particular: sym->result->as->{lbound,ubound} - get fixed via a
> call to resolve_ref.
That is a better way (than your other suggestions) IMO.


diff --git a/resolve.c b/resolve.c
index f484a22..cbf403c 100644
--- a/resolve.c
+++ b/resolve.c
@@ -12198,6 +12198,8 @@ resolve_symbol (gfc_symbol *sym)
 	    }
 	}
     }
+  else if (mp_flag && sym->attr.flavor == FL_PROCEDURE && sym->attr.function)
+    gfc_resolve_array_spec (sym->result->as, false);
 
   /* Assumed size arrays and assumed shape arrays must be dummy
      arguments.  Array-spec's of implied-shape should have been resolved to
Comment 9 Tobias Burnus 2011-07-06 16:53:32 UTC
(In reply to comment #8)
> +  else if (mp_flag && sym->attr.flavor == FL_PROCEDURE && sym->attr.function)
> +    gfc_resolve_array_spec (sym->result->as, false);

I think one could add somewhere a condition "if (sym != sym->result)"; and I was wondering whether also other items like length-type parameters are effected. (I think, usually character lengths are shared, but I am not sure whether it also happens here.)
Comment 10 Mikael Morin 2011-07-06 17:28:14 UTC
(In reply to comment #9)
> (In reply to comment #8)
> > +  else if (mp_flag && sym->attr.flavor == FL_PROCEDURE && sym->attr.function)
> > +    gfc_resolve_array_spec (sym->result->as, false);
> 
> I think one could add somewhere a condition "if (sym != sym->result)";
This is contained in mp_flag :-)

> and I was wondering whether also other items like length-type parameters are
> effected. (I think, usually character lengths are shared, but I am not sure
> whether it also happens here.)
Well, it's difficult to tell by quickly (or even slowly) browsing through the function.
I was supposing resolve_symbol would handle all other items in my previous patch, but it fails unfortunately (for reasons that I have admittedly not investigated); hence this less ambitious one, which doesn't regress at least. 
For what it's worth the following variant using character lengths doesn't ICE. I can't tell in the general case though.


module m2
  COMPLEX, SAVE, ALLOCATABLE :: P(:)
contains
  FUNCTION getPhaseMatrix() RESULT(PM)
    CHARACTER(len=SIZE(P)) :: PM
    PM="foo   "
  END FUNCTION
end module m2

module m
  use m2
contains
   SUBROUTINE gf_generateEmbPot()
      CHARACTER(3) :: sigma2
      sigma2 = TRIM(getPhaseMatrix())
   END SUBROUTINE
end module m
Comment 11 Dominique d'Humieres 2011-07-07 08:53:24 UTC
The patch in comment #8 fixes the ICEs for the test cases without disturbing my pet bugs and without regression.
Comment 12 Mikael Morin 2011-07-07 20:58:19 UTC
Author: mikael
Date: Thu Jul  7 20:58:16 2011
New Revision: 176011

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=176011
Log:
2011-07-07  Mikael Morin  <mikael.morin@sfr.fr>

	PR fortran/49648
	* resolve.c (resolve_symbol): Force resolution of function result's
	array specification.


Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/resolve.c
Comment 13 Mikael Morin 2011-07-07 21:03:27 UTC
Author: mikael
Date: Thu Jul  7 21:03:25 2011
New Revision: 176012

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=176012
Log:
2011-07-07  Mikael Morin  <mikael.morin@sfr.fr>

	PR fortran/49648
	* gfortran.dg/result_in_spec_4.f90: New test.


Added:
    trunk/gcc/testsuite/gfortran.dg/result_in_spec_4.f90
Modified:
    trunk/gcc/testsuite/ChangeLog
Comment 14 Mikael Morin 2011-07-18 20:31:05 UTC
Author: mikael
Date: Mon Jul 18 20:31:02 2011
New Revision: 176419

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=176419
Log:
2011-07-18  Mikael Morin  <mikael.morin@sfr.fr>

	PR fortran/49648
	* resolve.c (resolve_symbol): Force resolution of function result's
	array specification.


iiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiii

M    ChangeLog
M    resolve.c

Modified:
    branches/gcc-4_6-branch/gcc/fortran/ChangeLog
    branches/gcc-4_6-branch/gcc/fortran/resolve.c
Comment 15 Mikael Morin 2011-07-18 20:35:22 UTC
Author: mikael
Date: Mon Jul 18 20:35:18 2011
New Revision: 176422

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=176422
Log:
2011-07-18  Mikael Morin  <mikael.morin@sfr.fr>

	PR fortran/49648
	* gfortran.dg/result_in_spec_4.f90: New test.


Added:
    branches/gcc-4_6-branch/gcc/testsuite/gfortran.dg/result_in_spec_4.f90
Modified:
    branches/gcc-4_6-branch/gcc/testsuite/ChangeLog
Comment 16 Mikael Morin 2011-07-18 20:42:41 UTC
(In reply to comment #14)
> iiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiii
> 
> M    ChangeLog
> M    resolve.c

I couldn't help messing up the svn log somehow.

Anyway, fixed on trunk(4.7.0) and 4.6 branch (4.6.2).