[PATCH] PR fortran/101536 - ICE in gfc_conv_expr_descriptor, at fortran/trans-array.c:7324

[PATCH] PR fortran/101536 - ICE in gfc_conv_expr_descriptor, at fortran/trans-array.c:7324

[Harald Anlauf]

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

Another one of Gerhard's infamous testcases.  We did not properly detect
and reject array elements of type CLASS as argument to an intrinsic when
it should be an array.

Regtested on x86_64-pc-linux-gnu.  OK for mainline / 11-branch when it
reopens?

Thanks,
Harald


Fortran: extend check for array arguments and reject CLASS array elements.

gcc/fortran/ChangeLog:

	PR fortran/101536
	* check.c (array_check): Array elements of CLASS type are not
	arrays.

gcc/testsuite/ChangeLog:

	PR fortran/101536
	* gfortran.dg/pr101536.f90: New test.


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

diff --git a/gcc/fortran/check.c b/gcc/fortran/check.c
index 27bf3a7eafe..6d2d9fe4007 100644
--- a/gcc/fortran/check.c
+++ b/gcc/fortran/check.c
@@ -735,6 +735,10 @@ array_check (gfc_expr *e, int n)
 	&& CLASS_DATA (e)->attr.dimension
 	&& CLASS_DATA (e)->as->rank)
     {
+      if (e->ref && e->ref->type == REF_ARRAY
+	  && e->ref->u.ar.type == AR_ELEMENT)
+	goto error;
+
       gfc_add_class_array_ref (e);
       return true;
     }
@@ -742,6 +746,7 @@ array_check (gfc_expr *e, int n)
   if (e->rank != 0 && e->ts.type != BT_PROCEDURE)
     return true;

+error:
   gfc_error ("%qs argument of %qs intrinsic at %L must be an array",
 	     gfc_current_intrinsic_arg[n]->name, gfc_current_intrinsic,
 	     &e->where);
diff --git a/gcc/testsuite/gfortran.dg/pr101536.f90 b/gcc/testsuite/gfortran.dg/pr101536.f90
new file mode 100644
index 00000000000..14cb4100bd6
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/pr101536.f90
@@ -0,0 +1,12 @@
+! { dg-do compile }
+! PR fortran/101536 - ICE in gfc_conv_expr_descriptor
+
+program p
+  type t
+  end type
+contains
+  integer function f(x)
+    class(t), allocatable :: x(:)
+    f = size (x(1)) ! { dg-error "must be an array" }
+  end
+end

Re: [PATCH] PR fortran/101536 - ICE in gfc_conv_expr_descriptor, at fortran/trans-array.c:7324

[Tobias Burnus]

Hi Harald,

On 21.07.21 22:22, Harald Anlauf via Fortran wrote:
> Another one of Gerhard's infamous testcases.  We did not properly detect
> and reject array elements of type CLASS as argument to an intrinsic when
> it should be an array.
>
> Regtested on x86_64-pc-linux-gnu.  OK for mainline / 11-branch when it
> reopens?
...
> +    class(t), allocatable :: x(:)
> +    f = size (x(1)) ! { dg-error "must be an array" }
...
>    if (e->ts.type == BT_CLASS && gfc_expr_attr (e).class_ok
>          && CLASS_DATA (e)->attr.dimension
>          && CLASS_DATA (e)->as->rank)
>      {
> +      if (e->ref && e->ref->type == REF_ARRAY
> +       && e->ref->u.ar.type == AR_ELEMENT)
> +     goto error;

I think that one is wrong. While CLASS_DATA (e) accesses e->ts.u.derived->components,
which always works, your code assumes that there is only 'c' and not 'x%c' where
'c' is of type BT_CLASS and 'x' is of type BT_DERIVED.

I wonder whether it works if you simply remove 'return true;'
as gfc_add_class_array_ref sets 'e->rank = CLASS(e)->rank (and
adds an AR_FULL ref, if needed). In the nonerror case, the
'return true' is obtained via:
    if (e->rank != 0 && e->ts.type != BT_PROCEDURE)
      return true;
And, otherwise, it falls through to the error.

OK if that works – but please also add a test like

type t
   class(*), allocatable :: c(:)
end type t
type(t) :: x
x%c = [1,2,3,4]
print *, size(x%c)
print *, size(x%c(1)) ! { dg-error ... }
end

Thanks,

Tobias

-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

Re: [PATCH] PR fortran/101536 - ICE in gfc_conv_expr_descriptor, at fortran/trans-array.c:7324

[Harald Anlauf]

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

Hi Tobias,

you are right in that I was barking up the wrong tree.
I was focussed too much on the testcase in the PR.

> I think that one is wrong. While CLASS_DATA (e) accesses e->ts.u.derived->components,
> which always works, your code assumes that there is only 'c' and not 'x%c' where
> 'c' is of type BT_CLASS and 'x' is of type BT_DERIVED.
>
> I wonder whether it works if you simply remove 'return true;'
> as gfc_add_class_array_ref sets 'e->rank = CLASS(e)->rank (and
> adds an AR_FULL ref, if needed). In the nonerror case, the
> 'return true' is obtained via:
>     if (e->rank != 0 && e->ts.type != BT_PROCEDURE)
>       return true;
> And, otherwise, it falls through to the error.
>
> OK if that works

Well, I tried and this does not work.

However, an additional plain check on e->rank != 0 also in the
CLASS cases fixes the original issue as well as your example:

> type t
>    class(*), allocatable :: c(:)
> end type t
> type(t) :: x
> x%c = [1,2,3,4]
> print *, size(x%c)
> print *, size(x%c(1)) ! { dg-error ... }
> end

And regtests ok. :-)

See attached updated patch.

Anything else I am missing?

Thanks for the constructive review!

Harald

[-- Attachment #2: pr101536.patch-v2 --]
[-- Type: application/octet-stream, Size: 1324 bytes --]

diff --git a/gcc/fortran/check.c b/gcc/fortran/check.c
index 27bf3a7eafe..b03d322ad9c 100644
--- a/gcc/fortran/check.c
+++ b/gcc/fortran/check.c
@@ -731,7 +731,7 @@ logical_array_check (gfc_expr *array, int n)
 static bool
 array_check (gfc_expr *e, int n)
 {
-  if (e->ts.type == BT_CLASS && gfc_expr_attr (e).class_ok
+  if (e->rank != 0 && e->ts.type == BT_CLASS && gfc_expr_attr (e).class_ok
 	&& CLASS_DATA (e)->attr.dimension
 	&& CLASS_DATA (e)->as->rank)
     {
diff --git a/gcc/testsuite/gfortran.dg/pr101536.f90 b/gcc/testsuite/gfortran.dg/pr101536.f90
new file mode 100644
index 00000000000..f8fa1dee639
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/pr101536.f90
@@ -0,0 +1,26 @@
+! { dg-do compile }
+! PR fortran/101536 - ICE in gfc_conv_expr_descriptor
+
+program p
+  type t
+     class(*), allocatable :: c(:)
+  end type t
+  type u
+     integer :: c(2)
+  end type
+  type(t) :: x
+  x%c = [1,2,3,4]
+  print *, size (x%c)
+  print *, size (x%c(1)) ! { dg-error "must be an array" }
+contains
+  integer function f(x, y)
+    class(t), allocatable :: x(:)
+    class(u)              :: y(:)
+    f = size (x)
+    f = size (x(1))      ! { dg-error "must be an array" }
+    f = size (y)
+    f = size (y%c(1))
+    f = size (y(2)%c)
+    f = size (y(2)%c(1)) ! { dg-error "must be an array" }
+  end
+end

Re: [PATCH] PR fortran/101536 - ICE in gfc_conv_expr_descriptor, at fortran/trans-array.c:7324

[Tobias Burnus]

Hi Harald,

On 22.07.21 21:03, Harald Anlauf wrote:
> you are right in that I was barking up the wrong tree.
> I was focussed too much on the testcase in the PR.
> [...]
> Well, I tried and this does not work.

Which makes sense if one thinks about it:

When using 'a(5,:)', the parser already sets e->rank = 1.

while for 'a', the 'a' is the class wrapper with rank == 0 and
then overriding the e->rank by CLASS_DATA(e)->as.rank
+ adding AR_FULL makes sense.

> However, an additional plain check on e->rank != 0 also in the
> CLASS cases fixes the original issue as well as your example:
[...]
> And regtests ok. :-)
> See attached updated patch.

I think you still need to remove the 'return true;' from
the 'if (e->rank != 0 && e->ts.type == BT_CLASS' block – to
fall through to the e->rank check after the block.
(When 'return true;' is gone, the '{' and '}' can also be removed.)

Reason: Assume 'CLASS(...) x'. In this case, 'x' is a scalar.
And even after calling gfc_add_class_array_ref it remains
a scalar and e->rank == 0.

Or in other words: I think with your current patch,
     class(u)              :: z
     f = size (z)
is wrongly accepted without an error.

Thus: OK with a scalar CLASS entry added which gives an error,
which I believe requires the removal of the 'return true;' line.

Thanks for the patch – and I find it surprising how many
combinations exist which all can go wrong.

Tobias

-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

Re: [PATCH] PR fortran/101536 - ICE in gfc_conv_expr_descriptor, at fortran/trans-array.c:7324

[Harald Anlauf]

Hi Tobias,

> > However, an additional plain check on e->rank != 0 also in the
> > CLASS cases fixes the original issue as well as your example:
> [...]
> > And regtests ok. :-)
> > See attached updated patch.
> 
> I think you still need to remove the 'return true;' from
> the 'if (e->rank != 0 && e->ts.type == BT_CLASS' block – to
> fall through to the e->rank check after the block.
> (When 'return true;' is gone, the '{' and '}' can also be removed.)
> 
> Reason: Assume 'CLASS(...) x'. In this case, 'x' is a scalar.
> And even after calling gfc_add_class_array_ref it remains
> a scalar and e->rank == 0.
> 
> Or in other words: I think with your current patch,
>      class(u)              :: z
>      f = size (z)
> is wrongly accepted without an error.

did you really check that?  My related testing succeeded without
and with the return (which was in the original commit by Paul).

I have nevertheless followed your advice to remove the return
statement, extended the testcase and regtested again.

Committed as https://gcc.gnu.org/g:e314cfc371d8b2405a1d81e51b90f9fb24b9061f

Thanks,
Harald