[Patch, Fortran, OOP] PR 56266: ICE on invalid in gfc_match_varspec

[Patch, Fortran, OOP] PR 56266: ICE on invalid in gfc_match_varspec

[Janus Weil]

[-- Attachment #1: Type: text/plain, Size: 900 bytes --]

Hi all,

here is a small patch which fixes an ICE-on-invalid problem by simply
turning a "gcc_assert" into a MATCH_ERROR. I am aware that the
resulting error message ("Unclassifiable statement") is not the most
helpful one, but I couldn't come up with something better.

The problem is that "c(i)" is wrongly identified as a function call,
which is the best thing one can do at parsing time. But giving an
error message based on this (wrong) assumption will not be any more
helpful, therefore I prefer the "Unclassifiable statement".

Unless someone has a better idea how to treat this, I will commit the
attached patch as obvious.

Cheers,
Janus


2013-04-12  Janus Weil  <janus@gcc.gnu.org>

    PR fortran/56266
    * primary.c (gfc_match_varspec): Turn gcc_assert into MATCH_ERROR.


2013-04-12  Janus Weil  <janus@gcc.gnu.org>

    PR fortran/56261
    * gfortran.dg/typebound_proc_28.f03: New.

[-- Attachment #2: pr56266.diff --]
[-- Type: application/octet-stream, Size: 814 bytes --]

Index: gcc/fortran/primary.c
===================================================================
--- gcc/fortran/primary.c	(revision 197920)
+++ gcc/fortran/primary.c	(working copy)
@@ -1953,11 +1953,13 @@ gfc_match_varspec (gfc_expr *primary, int equiv_fl
 	    return MATCH_ERROR;
 
 	  gcc_assert (!tail || !tail->next);
-	  gcc_assert (primary->expr_type == EXPR_VARIABLE
-		      || (primary->expr_type == EXPR_STRUCTURE
-			  && primary->symtree && primary->symtree->n.sym
-			  && primary->symtree->n.sym->attr.flavor));
 
+	  if (!(primary->expr_type == EXPR_VARIABLE
+		|| (primary->expr_type == EXPR_STRUCTURE
+		    && primary->symtree && primary->symtree->n.sym
+		    && primary->symtree->n.sym->attr.flavor)))
+	    return MATCH_ERROR;
+
 	  if (tbp->n.tb->is_generic)
 	    tbp_sym = NULL;
 	  else

[-- Attachment #3: typebound_proc_28.f03 --]
[-- Type: application/octet-stream, Size: 504 bytes --]

! { dg-do compile }
!
! PR 56266: [OOP] ICE on invalid in gfc_match_varspec
!
! Contributed by Andrew Benson <abensonca@gmail.com>

module t

  implicit none

  type nc
   contains
     procedure :: encM => em
  end type nc

contains

  double precision function em(self)
    class(nc) :: self
    em=0.
  end function

  double precision function cem(c)
    type(nc) :: c
    cem=c(i)%encM()   ! { dg-error "Unclassifiable statement" }
  end function

end module

! { dg-final { cleanup-modules "t" } }

Re: [Patch, Fortran, OOP] PR 56266: ICE on invalid in gfc_match_varspec

[Mikael Morin]

Hello,

Le 12/04/2013 20:38, Janus Weil a écrit :
> Unless someone has a better idea how to treat this, I will commit the
> attached patch as obvious.
> 
Not really a better idea, but it seems to me that function calls can
have trailing sub-references, so that gfc_match_varspec could be called
on them.

gfc_match_rvalue has:

[...]
switch (sym->attr.flavor)
 {
    [...]

    case FL_UNKNOWN:
      [... try to match a variable ...]
      /* Give up, assume we have a function.  */
      [...]
      e->expr_type = EXPR_FUNCTION;
      [...]
      gfc_match_actual_arglist (...);
      [...]
      /* If our new function returns a character, array or structure
	 type, it might have subsequent references.  */
      m = gfc_match_varspec (e, ...);


So, it seems that EXPR_FUNCTION is acceptable in gfc_match_varspec.
And then, there is nothing preventing 'c(i)' in 'c(i)%encM()' from being
parsed as a function.  Is this supported?

Mikael

Re: [Patch, Fortran, OOP] PR 56266: ICE on invalid in gfc_match_varspec

[Janus Weil]

Hi Mikael,

>> Unless someone has a better idea how to treat this, I will commit the
>> attached patch as obvious.
>>
> Not really a better idea, but it seems to me that function calls can
> have trailing sub-references, so that gfc_match_varspec could be called
> on them.
>
> gfc_match_rvalue has:
>
> [...]
> switch (sym->attr.flavor)
>  {
>     [...]
>
>     case FL_UNKNOWN:
>       [... try to match a variable ...]
>       /* Give up, assume we have a function.  */
>       [...]
>       e->expr_type = EXPR_FUNCTION;
>       [...]
>       gfc_match_actual_arglist (...);
>       [...]
>       /* If our new function returns a character, array or structure
>          type, it might have subsequent references.  */
>       m = gfc_match_varspec (e, ...);
>
>
> So, it seems that EXPR_FUNCTION is acceptable in gfc_match_varspec.
> And then, there is nothing preventing 'c(i)' in 'c(i)%encM()' from being
> parsed as a function.  Is this supported?

I think this is forbidden by the Fortran standard, cf. e.g.

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42188

Actually I'm not sure in which context a function call with sub-refs
would be valid. One should re-check the standard on this ...

(Btw, I have already committed the patch as r197936.)

Cheers,
Janus

Re: [Patch, Fortran, OOP] PR 56266: ICE on invalid in gfc_match_varspec

[Mikael Morin]

Le 13/04/2013 16:02, Janus Weil a écrit :
> Hi Mikael,
> 
>> So, it seems that EXPR_FUNCTION is acceptable in gfc_match_varspec.
>> And then, there is nothing preventing 'c(i)' in 'c(i)%encM()' from being
>> parsed as a function.  Is this supported?
> 
> I think this is forbidden by the Fortran standard, cf. e.g.
> 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42188
> 
> Actually I'm not sure in which context a function call with sub-refs
> would be valid. One should re-check the standard on this ...
> 
Indeed, that's invalid:

structure-component is data-ref
data-ref is part-ref [ % part-ref ] ...
part-ref is part-name [ ( section-subscript-list ) ] [ image-selector ]

(R611) The leftmost part-name shall be the name of a data object.


I thought they were allowed for pointer-returning functions.

Mikael

Re: [Patch, Fortran, OOP] PR 56266: ICE on invalid in gfc_match_varspec

[Mikael Morin]

[-- Attachment #1: Type: text/plain, Size: 191 bytes --]

Le 13/04/2013 17:44, Mikael Morin a écrit :
> Indeed, that's invalid:
> 
And then, the call to gfc_match_varspec shouldn't be there in the first
place.  I'll test the following later.





[-- Attachment #2: remove_match_varspec_call.diff --]
[-- Type: text/plain, Size: 1242 bytes --]

Index: primary.c
===================================================================
--- primary.c	(révision 197949)
+++ primary.c	(copie de travail)
@@ -1954,11 +1954,10 @@ gfc_match_varspec (gfc_expr *primary, int equiv_fl
 
 	  gcc_assert (!tail || !tail->next);
 
-	  if (!(primary->expr_type == EXPR_VARIABLE
-		|| (primary->expr_type == EXPR_STRUCTURE
-		    && primary->symtree && primary->symtree->n.sym
-		    && primary->symtree->n.sym->attr.flavor)))
-	    return MATCH_ERROR;
+	  gcc_assert (primary->expr_type == EXPR_VARIABLE
+		      || (primary->expr_type == EXPR_STRUCTURE
+		          && primary->symtree && primary->symtree->n.sym
+		          && primary->symtree->n.sym->attr.flavor));
 
 	  if (tbp->n.tb->is_generic)
 	    tbp_sym = NULL;
@@ -3102,18 +3101,8 @@ gfc_match_rvalue (gfc_expr **result)
 	gfc_error ("Missing argument list in function '%s' at %C", sym->name);
 
       if (m != MATCH_YES)
-	{
-	  m = MATCH_ERROR;
-	  break;
-	}
+	m = MATCH_ERROR;
 
-      /* If our new function returns a character, array or structure
-	 type, it might have subsequent references.  */
-
-      m = gfc_match_varspec (e, 0, false, true);
-      if (m == MATCH_NO)
-	m = MATCH_YES;
-
       break;
 
     generic_function:

Re: [Patch, Fortran, OOP] PR 56266: ICE on invalid in gfc_match_varspec

[Janus Weil]

>> Indeed, that's invalid:
>>
> And then, the call to gfc_match_varspec shouldn't be there in the first
> place.  I'll test the following later.

It seems like the parts you're removing have essentially been there
since day zero. Would be interesting to know if (and where) your patch
fails.

Cheers,
Janus

Re: [Patch, Fortran, OOP] PR 56266: ICE on invalid in gfc_match_varspec

[Janus Weil]

Hi Mikael,

>> And then, the call to gfc_match_varspec shouldn't be there in the first
>> place.  I'll test the following later.
>
> It seems like the parts you're removing have essentially been there
> since day zero. Would be interesting to know if (and where) your patch
> fails.

actually I just tried it myself, and I did not see any failures in the
testsuite. So in fact it seems like it might be ok to remove it.
However, one should carefully check the standard, in order to make
sure that this is really invalid in all cases, and that the
"gfc_match_varspec" is not needed for some corner case, which is not
covered by the testsuite.

As the comment says which the patch is removing, the gfc_match_varspec
should be relevant for cases like this:

print *,char_func()(1:3)
print *,array_func()(2)
print *,derived_type_func()%comp

Are we sure that all of these are actually invalid? (At least they are
rejected by gfortran.) Or are there other cases which would be valid?

Cheers,
Janus

Re: [Patch, Fortran, OOP] PR 56266: ICE on invalid in gfc_match_varspec

[Tobias Burnus]

Janus Weil wrote:
> As the comment says which the patch is removing, the gfc_match_varspec
> should be relevant for cases like this:
>
> print *,char_func()(1:3)
> print *,array_func()(2)
> print *,derived_type_func()%comp
>
> Are we sure that all of these are actually invalid? (At least they are
> rejected by gfortran.) Or are there other cases which would be valid?

They are all invalid. The only exception is that a function itself is 
regarded as variable (if it returns a pointer):

    f() = 5

That's a new Fortran 2008 feature, which caused quite some trouble with 
user-defined operators, e.g. something like:

   123 .foo. 5 = ...

where "123" could be an argument (binary operator) or a label (+plus 
unary operator). (When we implement it, we should do some careful 
interpretation-request reading.)

However, I think that doesn't affect the code you are removing. 
(Disclaimer: I haven't looked at the actual patch.)

Tobias