[PATCH] PR fortran/69419 - ICE: tree check: expected array_type, have real_type in gfc_conv_array_initializer, at fortran/trans-array.c:5618

[PATCH] PR fortran/69419 - ICE: tree check: expected array_type, have real_type in gfc_conv_array_initializer, at fortran/trans-array.c:5618

[Harald Anlauf]

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

Dear Fortranners,

when debugging the testcase, I noticed that a coarray declaration in
a COMMON statement wrongly set the dimension attribute instead of the
codimension.  As a consequence, subsequent checks that catch this
invalid situation would not trigger.

I see two possible solutions:

- in gfc_match_common, replace

	  /* Deal with an optional array specification after the
	     symbol name.  */
	  m = gfc_match_array_spec (&as, true, true);

  by

  m = gfc_match_array_spec (&as, true, false);

  which in turn would lead to a syntax error.  Interestingly, the Intel
  compiler also takes this route and gives a syntax error.

- check the resulting as->corank and emit an error as in the attached
  patch.

The attached patch regtests fine on x86_64-pc-linux-gnu.  OK for mainline?

Thanks,
Harald


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: pr69419.diff --]
[-- Type: text/x-patch, Size: 1262 bytes --]

Fortran: a symbol in a COMMON cannot be a coarray

gcc/fortran/ChangeLog:

	PR fortran/69419
	* match.c (gfc_match_common): Check array spec of a symbol in a
	COMMON object list and reject it if it is a coarray.

gcc/testsuite/ChangeLog:

	PR fortran/69419
	* gfortran.dg/pr69419.f90: New test.

diff --git a/gcc/fortran/match.c b/gcc/fortran/match.c
index 53a575e616e..df97620634d 100644
--- a/gcc/fortran/match.c
+++ b/gcc/fortran/match.c
@@ -5314,6 +5314,13 @@ gfc_match_common (void)
 		  goto cleanup;
 		}

+	      if (as->corank)
+		{
+		  gfc_error ("Symbol %qs in COMMON at %C cannot be a "
+			     "coarray", sym->name);
+		  goto cleanup;
+		}
+
 	      if (!gfc_add_dimension (&sym->attr, sym->name, NULL))
 		goto cleanup;

diff --git a/gcc/testsuite/gfortran.dg/pr69419.f90 b/gcc/testsuite/gfortran.dg/pr69419.f90
new file mode 100644
index 00000000000..7329808611c
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/pr69419.f90
@@ -0,0 +1,9 @@
+! { dg-do compile }
+! { dg-options "-fcoarray=lib" }
+! PR fortran/69419 - ICE on invalid coarray in common
+
+blockdata b
+   real x           ! { dg-error "must be in COMMON" }
+   common /c/ x[*]  ! { dg-error "cannot be a coarray" }
+   data x /1.0/
+end

*PING* [PATCH] PR fortran/69419 - ICE: tree check: expected array_type, have real_type in gfc_conv_array_initializer, at fortran/trans-array.c:5618

[Harald Anlauf]

*PING*

Am 27.10.21 um 21:09 schrieb Harald Anlauf via Fortran:
> Dear Fortranners,
>
> when debugging the testcase, I noticed that a coarray declaration in
> a COMMON statement wrongly set the dimension attribute instead of the
> codimension.  As a consequence, subsequent checks that catch this
> invalid situation would not trigger.
>
> I see two possible solutions:
>
> - in gfc_match_common, replace
>
> 	  /* Deal with an optional array specification after the
> 	     symbol name.  */
> 	  m = gfc_match_array_spec (&as, true, true);
>
>    by
>
>    m = gfc_match_array_spec (&as, true, false);
>
>    which in turn would lead to a syntax error.  Interestingly, the Intel
>    compiler also takes this route and gives a syntax error.
>
> - check the resulting as->corank and emit an error as in the attached
>    patch.
>
> The attached patch regtests fine on x86_64-pc-linux-gnu.  OK for mainline?
>
> Thanks,
> Harald
>

Re: *PING* [PATCH] PR fortran/69419 - ICE: tree check: expected array_type, have real_type in gfc_conv_array_initializer, at fortran/trans-array.c:5618

[Bernhard Reutner-Fischer]

On Wed, 3 Nov 2021 21:00:41 +0100
Harald Anlauf via Fortran <fortran@gcc.gnu.org> wrote:

> *PING*
> 
> Am 27.10.21 um 21:09 schrieb Harald Anlauf via Fortran:
> > Dear Fortranners,
> > 
> > when debugging the testcase, I noticed that a coarray declaration in
> > a COMMON statement wrongly set the dimension attribute instead of the
> > codimension.  As a consequence, subsequent checks that catch this
> > invalid situation would not trigger.
> > 
> > I see two possible solutions:
> > 
> > - in gfc_match_common, replace
> > 
> > 	  /* Deal with an optional array specification after the
> > 	     symbol name.  */
> > 	  m = gfc_match_array_spec (&as, true, true);

If coarrays are generally forbidden in commons then..
> > 
> >    by
> > 
> >    m = gfc_match_array_spec (&as, true, false);

.. this sounds right to me.

> > 
> >    which in turn would lead to a syntax error.  Interestingly, the Intel
> >    compiler also takes this route and gives a syntax error.
> > 
> > - check the resulting as->corank and emit an error as in the attached
> >    patch.

If we want to be more helpful than a mere syntax error (and we
should be) then yes.
Otherwise we'd have to explicitly
@@ -5275,9 +5275,19 @@ gfc_match_common (void)
 
 	  /* Deal with an optional array specification after the
 	     symbol name.  */
-	  m = gfc_match_array_spec (&as, true, true);
+	  m = gfc_match_array_spec (&as, true, false);
 	  if (m == MATCH_ERROR)
 	    goto cleanup;
+	  if (m == MATCH_NO)
+	    {
+	      /* See if it is a coarray and diagnose it nicely.  */
+	      if (gfc_match_array_spec (&as, false, true) == MATCH_YES)
+		{
+		  gfc_error ("Symbol %qs in COMMON at %C cannot be a "
+			     "coarray", sym->name);
+		  goto cleanup;
+		}
+	    }

where your patch works equally well and is more concise.
Maybe you want to add a test for the double-colon variant too?
   common /c2/ y[:] ! { dg-error "cannot be a coarray" }

A type with a coarray seems to require to be allocatable so is
rejected (properly, but not mentioning the coarray) with
Error: Derived type variable ‘comm_ty1’ in COMMON at (1) has an ultimate component that is allocatable

When reading gfc_match_array_spec i thought that it might have been cleaner
to split the coarray handling out to a separate gfc_match_coarray_spec but
that's what we have.
> > 
> > The attached patch regtests fine on x86_64-pc-linux-gnu.  OK for mainline?

LGTM but i cannot approve it.
Thanks for the patch!

Re: *PING* [PATCH] PR fortran/69419 - ICE: tree check: expected array_type, have real_type in gfc_conv_array_initializer, at fortran/trans-array.c:5618

[Harald Anlauf]

Hi Bernhard,

Am 04.11.21 um 10:06 schrieb Bernhard Reutner-Fischer via Fortran:
> On Wed, 3 Nov 2021 21:00:41 +0100
> Harald Anlauf via Fortran <fortran@gcc.gnu.org> wrote:
>
>> *PING*
>>
>> Am 27.10.21 um 21:09 schrieb Harald Anlauf via Fortran:
>>> Dear Fortranners,
>>>
>>> when debugging the testcase, I noticed that a coarray declaration in
>>> a COMMON statement wrongly set the dimension attribute instead of the
>>> codimension.  As a consequence, subsequent checks that catch this
>>> invalid situation would not trigger.
>>>
>>> I see two possible solutions:
>>>
>>> - in gfc_match_common, replace
>>>
>>> 	  /* Deal with an optional array specification after the
>>> 	     symbol name.  */
>>> 	  m = gfc_match_array_spec (&as, true, true);
>
> If coarrays are generally forbidden in commons then..

F2018:

(R874) A common-block-object shall not be a dummy argument, a function
result, an allocatable variable, a derived-type object with an ultimate
component that is allocatable, a procedure pointer, an automatic data
object, a variable with the BIND attribute, an unlimited polymorphic
pointer, or a coarray.

>>>
>>>     by
>>>
>>>     m = gfc_match_array_spec (&as, true, false);
>
> .. this sounds right to me.
>
>>>
>>>     which in turn would lead to a syntax error.  Interestingly, the Intel
>>>     compiler also takes this route and gives a syntax error.
>>>
>>> - check the resulting as->corank and emit an error as in the attached
>>>     patch.
>
> If we want to be more helpful than a mere syntax error (and we
> should be) then yes.
> Otherwise we'd have to explicitly
> @@ -5275,9 +5275,19 @@ gfc_match_common (void)
>
>   	  /* Deal with an optional array specification after the
>   	     symbol name.  */
> -	  m = gfc_match_array_spec (&as, true, true);
> +	  m = gfc_match_array_spec (&as, true, false);
>   	  if (m == MATCH_ERROR)
>   	    goto cleanup;
> +	  if (m == MATCH_NO)
> +	    {
> +	      /* See if it is a coarray and diagnose it nicely.  */

I think you would need to add

	      gfc_array_spec *as;

to avoid clobbering the correct "as" as it is needed later.

> +	      if (gfc_match_array_spec (&as, false, true) == MATCH_YES)
> +		{
> +		  gfc_error ("Symbol %qs in COMMON at %C cannot be a "
> +			     "coarray", sym->name);
> +		  goto cleanup;
> +		}
> +	    }
>
> where your patch works equally well and is more concise.
> Maybe you want to add a test for the double-colon variant too?
>     common /c2/ y[:] ! { dg-error "cannot be a coarray" }

Well, that one is already rejected, although with a different error
message.

I am not sure whether to add that case.  In fact, there are
issues with not always rejecting things like y[:] in declarations
as the last dimension must be "*" or "lbound:*".  If checking
gets improved in this direction, we would have to adjust the error
message.  So adding this variant now does not buy us much.

> A type with a coarray seems to require to be allocatable so is
> rejected (properly, but not mentioning the coarray) with
> Error: Derived type variable ‘comm_ty1’ in COMMON at (1) has an ultimate component that is allocatable

If multiple errors show up, which ones are most important or
must show up?

If you ask me, it is more important to get correct results from
correct input than optimal error messages on wrong code.

I guess users may have an opinion different from mine...

> When reading gfc_match_array_spec i thought that it might have been cleaner
> to split the coarray handling out to a separate gfc_match_coarray_spec but
> that's what we have.
>>>
>>> The attached patch regtests fine on x86_64-pc-linux-gnu.  OK for mainline?
>
> LGTM but i cannot approve it.
> Thanks for the patch!
>

Thanks for your comments so far.

Let's see what others think.

Harald

Re: *PING* [PATCH] PR fortran/69419 - ICE: tree check: expected array_type, have real_type in gfc_conv_array_initializer, at fortran/trans-array.c:5618

[Mikael Morin]

Le 04/11/2021 à 20:49, Harald Anlauf via Fortran a écrit :
> Let's see what others think.
> 
OK, thanks.

Mikael