Re: [PATCH] PR fortran/103411 - ICE in gfc_conv_array_initializer, at fortran/trans-array.c:6377
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
Re: [PATCH] PR fortran/103411 - ICE in gfc_conv_array_initializer, at fortran/trans-array.c:6377
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
Re: [PATCH] PR fortran/103411 - ICE in gfc_conv_array_initializer, at fortran/trans-array.c:6377
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, v2] PR fortran/103411 - ICE in gfc_conv_array_initializer, at fortran/trans-array.c:6377
Hi Mikael,
Am 25.11.21 um 22:02 schrieb Mikael Morin:
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.
well, that one is actually better valid, since b=[2,2].
So yes, I was wrong; b has been expanded to an array before.
Motivated by your reasoning I tried gfc_reduce_init_expr. That attempt
failed miserably (many regressions), and I think it is not right.
Then I found that array sections posed a problem that wasn't detected
before. gfc_simplify_expr seemed to be a better choice that makes more
sense for the present situations and seems to work here. And it even
detects many more invalid cases now than e.g. Intel ;-)
I've updated the patch and testcase accordingly.
Can you add an assert or a comment saying that the parameter value has
been expanded to a constant array?
Ok with that change.
Given the above discussion, I'll give you another day or two to have a
further look. Otherwise Gerhard will... ;-)
Cheers,
Harald
From 56fd0d23ac0a5bda802e5cce3024b947e497555a Mon Sep 17 00:00:00 2001
From: Harald Anlauf
Date: Thu, 25 Nov 2021 22:39:44 +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. Try other simplifications
of shape.
gcc/testsuite/ChangeLog:
PR fortran/103411
* gfortran.dg/pr68153.f90: Adjust test to improved check.
* gfortran.dg/reshape_7.f90: Likewise.
* gfortran.dg/reshape_9.f90: New test.
---
gcc/fortran/check.c | 22 +-
gcc/testsuite/gfortran.dg/pr68153.f90 | 2 +-
gcc/testsuite/gfortran.dg/reshape_7.f90 | 2 +-
gcc/testsuite/gfortran.dg/reshape_9.f90 | 24
4 files changed, 43 insertions(+), 7 deletions(-)
create mode 100644 gcc/t
