PR fortran/103411 - ICE in gfc_conv_array_initializer, at fortran/trans-array.c:6377

Message ID trinity-4c07fd5c-8afc-4a43-ba7a-bdd5893eebf6-1637789577911@3c-app-gmx-bs39
State New
Headers show
Series
  • PR fortran/103411 - ICE in gfc_conv_array_initializer, at fortran/trans-array.c:6377
Related show

Commit Message

Petter Tomner via Gcc-patches Nov. 24, 2021, 9:32 p.m.
Dear all,

when checking the SOURCE and SHAPE arguments to the RESHAPE
intrinsic, for absent PAD argument we failed to handle the case
when SHAPE was a parameter.

Fortunately, the proper check was already there, and the code
just needs some tweaking, as well as one of the testcases.

Regtested on x86_64-pc-linux-gnu.  OK for mainline?

Thanks,
Harald

Comments

Mikael Morin Nov. 25, 2021, 4:46 p.m. | #1
Hello,

Le 24/11/2021 à 22:32, Harald Anlauf via Fortran a écrit :
> diff --git a/gcc/fortran/check.c b/gcc/fortran/check.c

> index 5a5aca10ebe..837eb0912c0 100644

> --- a/gcc/fortran/check.c

> +++ b/gcc/fortran/check.c

> @@ -4866,10 +4868,17 @@ gfc_check_reshape (gfc_expr *source, gfc_expr *shape,

>  	{

>  	  gfc_constructor *c;

>  	  bool test;

> +	  gfc_constructor_base b;

> 

> +	  if (shape->expr_type == EXPR_ARRAY)

> +	    b = shape->value.constructor;

> +	  else if (shape->expr_type == EXPR_VARIABLE)

> +	    b = shape->symtree->n.sym->value->value.constructor;


This misses a check that shape->symtree->n.sym->value is an array, so 
that it makes sense to access its constructor.

Actually, this only supports the case where the parameter value is 
defined by an array; but it could be an intrinsic call, a sum of 
parameters, a reference to an other parameter, etc.

The usual way to handle this is to call gfc_reduce_init_expr which (pray 
for it) will make an array out of whatever the shape expression is.

The rest looks good.
In the test, can you add a comment telling what it is testing?
Something like: "This tests that constant shape expressions passed to 
the reshape intrinsic are properly simplified before being used to 
diagnose invalid values"
We also used to put a comment mentioning the person who submitted the 
test, but not everybody seems to do it these days.

Mikael
Petter Tomner via Gcc-patches Nov. 25, 2021, 8:03 p.m. | #2
Hi Mikael,

Am 25.11.21 um 17:46 schrieb Mikael Morin:
> Hello,

>

> Le 24/11/2021 à 22:32, Harald Anlauf via Fortran a écrit :

>> diff --git a/gcc/fortran/check.c b/gcc/fortran/check.c

>> index 5a5aca10ebe..837eb0912c0 100644

>> --- a/gcc/fortran/check.c

>> +++ b/gcc/fortran/check.c

>> @@ -4866,10 +4868,17 @@ gfc_check_reshape (gfc_expr *source, gfc_expr

>> *shape,

>>      {

>>        gfc_constructor *c;

>>        bool test;

>> +      gfc_constructor_base b;

>>

>> +      if (shape->expr_type == EXPR_ARRAY)

>> +        b = shape->value.constructor;

>> +      else if (shape->expr_type == EXPR_VARIABLE)

>> +        b = shape->symtree->n.sym->value->value.constructor;

>

> This misses a check that shape->symtree->n.sym->value is an array, so

> that it makes sense to access its constructor.


there are checks further above for the cases
   shape->expr_type == EXPR_ARRAY
and for
   shape->expr_type == EXPR_VARIABLE
which look at the elements of array shape to see if they are
non-negative.

Only in those cases where the full "if ()'s" pass we set
shape_is_const = true; and proceed.  The purpose of the auxiliary
bool shape_is_const is to avoid repeating the lengthy if's again.
Only then the above cited code segment should get executed.

For shape->expr_type == EXPR_ARRAY there is really no change in logic.
For shape->expr_type == EXPR_VARIABLE the above snipped is now executed,
but then we already had

   else if (shape->expr_type == EXPR_VARIABLE && shape->ref
	   && shape->ref->u.ar.type == AR_FULL && shape->ref->u.ar.dimen == 1
	   && shape->ref->u.ar.as
	   && shape->ref->u.ar.as->lower[0]->expr_type == EXPR_CONSTANT
	   && shape->ref->u.ar.as->lower[0]->ts.type == BT_INTEGER
	   && shape->ref->u.ar.as->upper[0]->expr_type == EXPR_CONSTANT
	   && shape->ref->u.ar.as->upper[0]->ts.type == BT_INTEGER
	   && shape->symtree->n.sym->attr.flavor == FL_PARAMETER
	   && shape->symtree->n.sym->value)

In which situations do I miss anything new?

> Actually, this only supports the case where the parameter value is

> defined by an array; but it could be an intrinsic call, a sum of

> parameters, a reference to an other parameter, etc.


E.g. the following (still) does get rejected:

   print *, reshape([1,2,3,4,5], a+1)
   print *, reshape([1,2,3,4,5], a+a)
   print *, reshape([1,2,3,4,5], 2*a)
   print *, reshape([1,2,3,4,5], [3,3])
   print *, reshape([1,2,3,4,5], spread(3,dim=1,ncopies=2))

and has been rejected before.

> The usual way to handle this is to call gfc_reduce_init_expr which (pray

> for it) will make an array out of whatever the shape expression is.


Can you give an example where it fails?

I think the current code would almost certainly fail, too.

> The rest looks good.

> In the test, can you add a comment telling what it is testing?

> Something like: "This tests that constant shape expressions passed to

> the reshape intrinsic are properly simplified before being used to

> diagnose invalid values"


Can do.

> We also used to put a comment mentioning the person who submitted the

> test, but not everybody seems to do it these days.


Can do.

> Mikael

>


Harald
Mikael Morin Nov. 25, 2021, 9:02 p.m. | #3
Le 25/11/2021 à 21:03, Harald Anlauf a écrit :
> Hi Mikael,

> 

> Am 25.11.21 um 17:46 schrieb Mikael Morin:

>> Hello,

>>

>> Le 24/11/2021 à 22:32, Harald Anlauf via Fortran a écrit :

>>> diff --git a/gcc/fortran/check.c b/gcc/fortran/check.c

>>> index 5a5aca10ebe..837eb0912c0 100644

>>> --- a/gcc/fortran/check.c

>>> +++ b/gcc/fortran/check.c

>>> @@ -4866,10 +4868,17 @@ gfc_check_reshape (gfc_expr *source, gfc_expr

>>> *shape,

>>>      {

>>>        gfc_constructor *c;

>>>        bool test;

>>> +      gfc_constructor_base b;

>>>

>>> +      if (shape->expr_type == EXPR_ARRAY)

>>> +        b = shape->value.constructor;

>>> +      else if (shape->expr_type == EXPR_VARIABLE)

>>> +        b = shape->symtree->n.sym->value->value.constructor;

>>

>> This misses a check that shape->symtree->n.sym->value is an array, so

>> that it makes sense to access its constructor.

> 

> there are checks further above for the cases

>    shape->expr_type == EXPR_ARRAY

> and for

>    shape->expr_type == EXPR_VARIABLE

> which look at the elements of array shape to see if they are

> non-negative.

> 

> Only in those cases where the full "if ()'s" pass we set

> shape_is_const = true; and proceed.  The purpose of the auxiliary

> bool shape_is_const is to avoid repeating the lengthy if's again.

> Only then the above cited code segment should get executed.

> 

> For shape->expr_type == EXPR_ARRAY there is really no change in logic.

> For shape->expr_type == EXPR_VARIABLE the above snipped is now executed,

> but then we already had

> 

>    else if (shape->expr_type == EXPR_VARIABLE && shape->ref

>         && shape->ref->u.ar.type == AR_FULL && shape->ref->u.ar.dimen == 1

>         && shape->ref->u.ar.as

>         && shape->ref->u.ar.as->lower[0]->expr_type == EXPR_CONSTANT

>         && shape->ref->u.ar.as->lower[0]->ts.type == BT_INTEGER

>         && shape->ref->u.ar.as->upper[0]->expr_type == EXPR_CONSTANT

>         && shape->ref->u.ar.as->upper[0]->ts.type == BT_INTEGER

>         && shape->symtree->n.sym->attr.flavor == FL_PARAMETER

>         && shape->symtree->n.sym->value)

> 

> In which situations do I miss anything new?

> 

Yes, I agree with all of this.
My comment wasn’t about a check on shape->expr_type, but on 
shape->value->expr_type if shape->expr_type is a (parameter) variable.

>> Actually, this only supports the case where the parameter value is

>> defined by an array; but it could be an intrinsic call, a sum of

>> parameters, a reference to an other parameter, etc.

> 

> E.g. the following (still) does get rejected:

> 

>    print *, reshape([1,2,3,4,5], a+1)

>    print *, reshape([1,2,3,4,5], a+a)

>    print *, reshape([1,2,3,4,5], 2*a)

>    print *, reshape([1,2,3,4,5], [3,3])

>    print *, reshape([1,2,3,4,5], spread(3,dim=1,ncopies=2))

> 

> and has been rejected before.

> 


>> The usual way to handle this is to call gfc_reduce_init_expr which (pray

>> for it) will make an array out of whatever the shape expression is.

> 

> Can you give an example where it fails?

> 

> I think the current code would almost certainly fail, too.

> 

Probably, I was just trying to avoid followup bugs. ;-)

I have checked the following:

   integer, parameter :: a(2) = [1,1]
   integer, parameter :: b(2) = a + 1
   print *, reshape([1,2,3,4], b)
end

and it doesn’t fail as I thought it would.
So yes, I was wrong; b has been expanded to an array before.

Can you add an assert or a comment saying that the parameter value has 
been expanded to a constant array?

Ok with that change.

Patch

From d6af2a33bad852bcea39b8c5b2e7c27976bde2a1 Mon Sep 17 00:00:00 2001
From: Harald Anlauf <anlauf@gmx.de>
Date: Wed, 24 Nov 2021 22:22:24 +0100
Subject: [PATCH] Fortran: improve check of arguments to the RESHAPE intrinsic

gcc/fortran/ChangeLog:

	PR fortran/103411
	* check.c (gfc_check_reshape): Improve check of size of source
	array for the RESHAPE intrinsic against the given shape when pad
	is not given, and shape is a parameter.

gcc/testsuite/ChangeLog:

	PR fortran/103411
	* gfortran.dg/reshape_7.f90: Adjust test to improved check.
	* gfortran.dg/reshape_9.f90: New test.
---
 gcc/fortran/check.c                     | 17 +++++++++++++----
 gcc/testsuite/gfortran.dg/reshape_7.f90 |  2 +-
 gcc/testsuite/gfortran.dg/reshape_9.f90 | 14 ++++++++++++++
 3 files changed, 28 insertions(+), 5 deletions(-)
 create mode 100644 gcc/testsuite/gfortran.dg/reshape_9.f90

diff --git a/gcc/fortran/check.c b/gcc/fortran/check.c
index 5a5aca10ebe..837eb0912c0 100644
--- a/gcc/fortran/check.c
+++ b/gcc/fortran/check.c
@@ -4699,6 +4699,7 @@  gfc_check_reshape (gfc_expr *source, gfc_expr *shape,
   mpz_t size;
   mpz_t nelems;
   int shape_size;
+  bool shape_is_const = false;

   if (!array_check (source, 0))
     return false;
@@ -4736,6 +4737,7 @@  gfc_check_reshape (gfc_expr *source, gfc_expr *shape,
     {
       gfc_expr *e;
       int i, extent;
+      shape_is_const = true;
       for (i = 0; i < shape_size; ++i)
 	{
 	  e = gfc_constructor_lookup_expr (shape->value.constructor, i);
@@ -4748,7 +4750,7 @@  gfc_check_reshape (gfc_expr *source, gfc_expr *shape,
 	      gfc_error ("%qs argument of %qs intrinsic at %L has "
 			 "negative element (%d)",
 			 gfc_current_intrinsic_arg[1]->name,
-			 gfc_current_intrinsic, &e->where, extent);
+			 gfc_current_intrinsic, &shape->where, extent);
 	      return false;
 	    }
 	}
@@ -4766,6 +4768,7 @@  gfc_check_reshape (gfc_expr *source, gfc_expr *shape,
       int i, extent;
       gfc_expr *e, *v;

+      shape_is_const = true;
       v = shape->symtree->n.sym->value;

       for (i = 0; i < shape_size; i++)
@@ -4856,8 +4859,7 @@  gfc_check_reshape (gfc_expr *source, gfc_expr *shape,
 	}
     }

-  if (pad == NULL && shape->expr_type == EXPR_ARRAY
-      && gfc_is_constant_expr (shape)
+  if (pad == NULL && shape_is_const
       && !(source->expr_type == EXPR_VARIABLE && source->symtree->n.sym->as
 	   && source->symtree->n.sym->as->type == AS_ASSUMED_SIZE))
     {
@@ -4866,10 +4868,17 @@  gfc_check_reshape (gfc_expr *source, gfc_expr *shape,
 	{
 	  gfc_constructor *c;
 	  bool test;
+	  gfc_constructor_base b;

+	  if (shape->expr_type == EXPR_ARRAY)
+	    b = shape->value.constructor;
+	  else if (shape->expr_type == EXPR_VARIABLE)
+	    b = shape->symtree->n.sym->value->value.constructor;
+	  else
+	    gcc_unreachable ();

 	  mpz_init_set_ui (size, 1);
-	  for (c = gfc_constructor_first (shape->value.constructor);
+	  for (c = gfc_constructor_first (b);
 	       c; c = gfc_constructor_next (c))
 	    mpz_mul (size, size, c->expr->value.integer);

diff --git a/gcc/testsuite/gfortran.dg/reshape_7.f90 b/gcc/testsuite/gfortran.dg/reshape_7.f90
index d752650aa4e..4216cb60cbb 100644
--- a/gcc/testsuite/gfortran.dg/reshape_7.f90
+++ b/gcc/testsuite/gfortran.dg/reshape_7.f90
@@ -4,7 +4,7 @@ 
 subroutine p0
    integer, parameter :: sh(2) = [2, 3]
    integer, parameter :: &
-   & a(2,2) = reshape([1, 2, 3, 4], sh)   ! { dg-error "Different shape" }
+   & a(2,2) = reshape([1, 2, 3, 4], sh)   ! { dg-error "not enough elements" }
    if (a(1,1) /= 0) STOP 1
 end subroutine p0

diff --git a/gcc/testsuite/gfortran.dg/reshape_9.f90 b/gcc/testsuite/gfortran.dg/reshape_9.f90
new file mode 100644
index 00000000000..c46e211b47e
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/reshape_9.f90
@@ -0,0 +1,14 @@ 
+! { dg-do compile }
+! PR fortran/103411 - ICE in gfc_conv_array_initializer
+
+program p
+  integer, parameter :: a(2) = [2,2]
+  integer, parameter :: d(2,2) = reshape([1,2,3,4,5], a)
+  integer, parameter :: c(2,2) = reshape([1,2,3,4], a)
+  integer, parameter :: b(2,2) = &
+           reshape([1,2,3], a) ! { dg-error "not enough elements" }
+  print *, reshape([1,2,3], a) ! { dg-error "not enough elements" }
+  print *, reshape([1,2,3,4], a)
+  print *, reshape([1,2,3,4,5], a)
+  print *, b, c, d
+end
--
2.26.2