* [PATCH] PR fortran/103411 - ICE in gfc_conv_array_initializer, at fortran/trans-array.c:6377
@ 2021-11-24 21:32 Harald Anlauf
2021-11-25 16:46 ` Mikael Morin
0 siblings, 1 reply; 9+ messages in thread
From: Harald Anlauf @ 2021-11-24 21:32 UTC (permalink / raw)
To: fortran, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 355 bytes --]
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
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fortran-improve-check-of-arguments-to-the-RESHAPE-in.patch --]
[-- Type: text/x-patch, Size: 4439 bytes --]
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] PR fortran/103411 - ICE in gfc_conv_array_initializer, at fortran/trans-array.c:6377
2021-11-24 21:32 [PATCH] PR fortran/103411 - ICE in gfc_conv_array_initializer, at fortran/trans-array.c:6377 Harald Anlauf
@ 2021-11-25 16:46 ` Mikael Morin
2021-11-25 20:03 ` Harald Anlauf
0 siblings, 1 reply; 9+ messages in thread
From: Mikael Morin @ 2021-11-25 16:46 UTC (permalink / raw)
To: Harald Anlauf, fortran, gcc-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] PR fortran/103411 - ICE in gfc_conv_array_initializer, at fortran/trans-array.c:6377
2021-11-25 16:46 ` Mikael Morin
@ 2021-11-25 20:03 ` Harald Anlauf
2021-11-25 20:03 ` Harald Anlauf
2021-11-25 21:02 ` Mikael Morin
0 siblings, 2 replies; 9+ messages in thread
From: Harald Anlauf @ 2021-11-25 20:03 UTC (permalink / raw)
To: Mikael Morin, fortran, gcc-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] PR fortran/103411 - ICE in gfc_conv_array_initializer, at fortran/trans-array.c:6377
2021-11-25 20:03 ` Harald Anlauf
@ 2021-11-25 20:03 ` Harald Anlauf
2021-11-25 21:02 ` Mikael Morin
1 sibling, 0 replies; 9+ messages in thread
From: Harald Anlauf @ 2021-11-25 20:03 UTC (permalink / raw)
To: gcc-patches; +Cc: fortran
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] PR fortran/103411 - ICE in gfc_conv_array_initializer, at fortran/trans-array.c:6377
2021-11-25 20:03 ` Harald Anlauf
2021-11-25 20:03 ` Harald Anlauf
@ 2021-11-25 21:02 ` Mikael Morin
2021-11-25 21:52 ` [PATCH, v2] " Harald Anlauf
1 sibling, 1 reply; 9+ messages in thread
From: Mikael Morin @ 2021-11-25 21:02 UTC (permalink / raw)
To: Harald Anlauf, fortran, gcc-patches
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH, v2] PR fortran/103411 - ICE in gfc_conv_array_initializer, at fortran/trans-array.c:6377
2021-11-25 21:02 ` Mikael Morin
@ 2021-11-25 21:52 ` Harald Anlauf
2021-11-26 14:45 ` Mikael Morin
0 siblings, 1 reply; 9+ messages in thread
From: Harald Anlauf @ 2021-11-25 21:52 UTC (permalink / raw)
To: Mikael Morin, fortran, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 4553 bytes --]
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
[-- Attachment #2: 0001-Fortran-improve-check-of-arguments-to-the-RESHAPE-in.patch --]
[-- Type: text/x-patch, Size: 5909 bytes --]
From 56fd0d23ac0a5bda802e5cce3024b947e497555a Mon Sep 17 00:00:00 2001
From: Harald Anlauf <anlauf@gmx.de>
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/testsuite/gfortran.dg/reshape_9.f90
diff --git a/gcc/fortran/check.c b/gcc/fortran/check.c
index 5a5aca10ebe..29c8554911f 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;
@@ -4732,10 +4733,14 @@ gfc_check_reshape (gfc_expr *source, gfc_expr *shape,
"than %d elements", &shape->where, GFC_MAX_DIMENSIONS);
return false;
}
- else if (shape->expr_type == EXPR_ARRAY && gfc_is_constant_expr (shape))
+
+ gfc_simplify_expr (shape, 0);
+
+ if (shape->expr_type == EXPR_ARRAY && gfc_is_constant_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 +4753,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 +4771,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 +4862,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 +4871,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/pr68153.f90 b/gcc/testsuite/gfortran.dg/pr68153.f90
index 1a360f80cd6..46a3bc029d7 100644
--- a/gcc/testsuite/gfortran.dg/pr68153.f90
+++ b/gcc/testsuite/gfortran.dg/pr68153.f90
@@ -5,5 +5,5 @@
!
program foo
integer, parameter :: a(2) = [2, -2]
- integer, parameter :: b(2,2) = reshape([1, 2, 3, 4], a) ! { dg-error "cannot be negative" }
+ integer, parameter :: b(2,2) = reshape([1, 2, 3, 4], a) ! { dg-error "negative" }
end program foo
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..b12ecee399b
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/reshape_9.f90
@@ -0,0 +1,24 @@
+! { dg-do compile }
+! PR fortran/103411 - ICE in gfc_conv_array_initializer
+! Based on testcase by G. Steinmetz
+! Test simplifications for checks of shape argument to reshape intrinsic
+
+program p
+ integer :: i
+ integer, parameter :: a(2) = [2,2]
+ integer, parameter :: u(5) = [1,2,2,42,2]
+ integer, parameter :: v(1,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
+ print *, reshape([1,2,3], [(u(i),i=1,2)])
+ print *, reshape([1,2,3], [(u(i),i=2,3)]) ! { dg-error "not enough elements" }
+ print *, reshape([1,2,3,4], u(5:3:-2))
+ print *, reshape([1,2,3], u(5:3:-2)) ! { dg-error "not enough elements" }
+ print *, reshape([1,2,3], v(1,:)) ! { dg-error "not enough elements" }
+end
--
2.26.2
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH, v2] PR fortran/103411 - ICE in gfc_conv_array_initializer, at fortran/trans-array.c:6377
2021-11-25 21:52 ` [PATCH, v2] " Harald Anlauf
@ 2021-11-26 14:45 ` Mikael Morin
2021-11-26 20:07 ` [PATCH, v3] " Harald Anlauf
0 siblings, 1 reply; 9+ messages in thread
From: Mikael Morin @ 2021-11-26 14:45 UTC (permalink / raw)
To: Harald Anlauf, fortran; +Cc: gcc-patches
Le 25/11/2021 à 22:52, Harald Anlauf a écrit :
>
> 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 ;-)
>
Great let’s go with that.
Can you set shape_is_constant just after the simplification?
That is
gfc_simplify_expr (shape, 0);
if (gfc_is_constant_expr (shape))
shape_is_const = true;
if (shape->expr_type == EXPR_ARRAY && shape_is_const)
...
This removes the need for multiple case initialization of shape_is_const
which I overlooked in my previous review.
And the EXPR_ARRAY vs EXPR_VARIABLE change becomes unneeded because the
simplification should produce an EXPR_ARRAY.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH, v3] PR fortran/103411 - ICE in gfc_conv_array_initializer, at fortran/trans-array.c:6377
2021-11-26 14:45 ` Mikael Morin
@ 2021-11-26 20:07 ` Harald Anlauf
2021-11-26 21:45 ` Mikael Morin
0 siblings, 1 reply; 9+ messages in thread
From: Harald Anlauf @ 2021-11-26 20:07 UTC (permalink / raw)
To: Mikael Morin; +Cc: fortran, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 1702 bytes --]
Hi Mikael,
> Gesendet: Freitag, 26. November 2021 um 15:45 Uhr
> Von: "Mikael Morin" <morin-mikael@orange.fr>
> An: "Harald Anlauf" <anlauf@gmx.de>, fortran@gcc.gnu.org
> Cc: gcc-patches@gcc.gnu.org
> Betreff: Re: [PATCH, v2] PR fortran/103411 - ICE in gfc_conv_array_initializer, at fortran/trans-array.c:6377
>
> Le 25/11/2021 à 22:52, Harald Anlauf a écrit :
> >
> > 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 ;-)
> >
> Great let’s go with that.
> Can you set shape_is_constant just after the simplification?
> That is
>
> gfc_simplify_expr (shape, 0);
> if (gfc_is_constant_expr (shape))
> shape_is_const = true;
>
> if (shape->expr_type == EXPR_ARRAY && shape_is_const)
> ...
>
>
> This removes the need for multiple case initialization of shape_is_const
> which I overlooked in my previous review.
>
> And the EXPR_ARRAY vs EXPR_VARIABLE change becomes unneeded because the
> simplification should produce an EXPR_ARRAY.
ah, I did not expect that. And indeed it seems to do the job! Furthermore
it turns out that the new patch (v3) removes more code than it adds. :-)
I extended the testcase slightly and regtested again.
That should hopefully be the final version...
Thanks for the really constructive comments!
Harald
[-- Attachment #2: 0001-Fortran-improve-check-of-arguments-to-the-RESHAPE-in.patch.v3 --]
[-- Type: application/octet-stream, Size: 6390 bytes --]
From 4d540c7a4a7fb87b04d06e1ee7f9b004116279a4 Mon Sep 17 00:00:00 2001
From: Harald Anlauf <anlauf@gmx.de>
Date: Fri, 26 Nov 2021 21:00:35 +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 | 43 +++++--------------------
gcc/testsuite/gfortran.dg/pr68153.f90 | 2 +-
gcc/testsuite/gfortran.dg/reshape_7.f90 | 2 +-
gcc/testsuite/gfortran.dg/reshape_9.f90 | 31 ++++++++++++++++++
4 files changed, 41 insertions(+), 37 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..3e65f3d8b1f 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;
if (!array_check (source, 0))
return false;
@@ -4732,7 +4733,11 @@ gfc_check_reshape (gfc_expr *source, gfc_expr *shape,
"than %d elements", &shape->where, GFC_MAX_DIMENSIONS);
return false;
}
- else if (shape->expr_type == EXPR_ARRAY && gfc_is_constant_expr (shape))
+
+ gfc_simplify_expr (shape, 0);
+ shape_is_const = gfc_is_constant_expr (shape);
+
+ if (shape->expr_type == EXPR_ARRAY && shape_is_const)
{
gfc_expr *e;
int i, extent;
@@ -4748,38 +4753,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);
- return false;
- }
- }
- }
- 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)
- {
- int i, extent;
- gfc_expr *e, *v;
-
- v = shape->symtree->n.sym->value;
-
- for (i = 0; i < shape_size; i++)
- {
- e = gfc_constructor_lookup_expr (v->value.constructor, i);
- if (e == NULL)
- break;
-
- gfc_extract_int (e, &extent);
-
- if (extent < 0)
- {
- gfc_error ("Element %d of actual argument of RESHAPE at %L "
- "cannot be negative", i + 1, &shape->where);
+ gfc_current_intrinsic, &shape->where, extent);
return false;
}
}
@@ -4856,8 +4830,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->expr_type == EXPR_ARRAY && shape_is_const
&& !(source->expr_type == EXPR_VARIABLE && source->symtree->n.sym->as
&& source->symtree->n.sym->as->type == AS_ASSUMED_SIZE))
{
diff --git a/gcc/testsuite/gfortran.dg/pr68153.f90 b/gcc/testsuite/gfortran.dg/pr68153.f90
index 1a360f80cd6..46a3bc029d7 100644
--- a/gcc/testsuite/gfortran.dg/pr68153.f90
+++ b/gcc/testsuite/gfortran.dg/pr68153.f90
@@ -5,5 +5,5 @@
!
program foo
integer, parameter :: a(2) = [2, -2]
- integer, parameter :: b(2,2) = reshape([1, 2, 3, 4], a) ! { dg-error "cannot be negative" }
+ integer, parameter :: b(2,2) = reshape([1, 2, 3, 4], a) ! { dg-error "negative" }
end program foo
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..dc52e26cc86
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/reshape_9.f90
@@ -0,0 +1,31 @@
+! { dg-do compile }
+! PR fortran/103411 - ICE in gfc_conv_array_initializer
+! Based on testcase by G. Steinmetz
+! Test simplifications for checks of shape argument to reshape intrinsic
+
+program p
+ integer :: i
+ integer, parameter :: a(2) = [2,2]
+ integer, parameter :: u(5) = [1,2,2,42,2]
+ integer, parameter :: v(1,3) = 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
+ print *, reshape([1,2,3], [(u(i),i=1,2)])
+ print *, reshape([1,2,3], [(u(i),i=2,3)]) ! { dg-error "not enough elements" }
+ print *, reshape([1,2,3], &
+ [(u(i)*(-1)**i,i=2,3)]) ! { dg-error "has negative element" }
+ print *, reshape([1,2,3,4], u(5:3:-2))
+ print *, reshape([1,2,3], u(5:3:-2)) ! { dg-error "not enough elements" }
+ print *, reshape([1,2,3,4], u([5,3]))
+ print *, reshape([1,2,3] , u([5,3])) ! { dg-error "not enough elements" }
+ print *, reshape([1,2,3,4], v(1,2:))
+ print *, reshape([1,2,3], v(1,2:)) ! { dg-error "not enough elements" }
+ print *, reshape([1,2,3,4], v(1,[2,1]))
+ print *, reshape([1,2,3] , v(1,[2,1])) ! { dg-error "not enough elements" }
+end
--
2.26.2
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH, v3] PR fortran/103411 - ICE in gfc_conv_array_initializer, at fortran/trans-array.c:6377
2021-11-26 20:07 ` [PATCH, v3] " Harald Anlauf
@ 2021-11-26 21:45 ` Mikael Morin
0 siblings, 0 replies; 9+ messages in thread
From: Mikael Morin @ 2021-11-26 21:45 UTC (permalink / raw)
To: Harald Anlauf; +Cc: fortran, gcc-patches
Le 26/11/2021 à 21:07, Harald Anlauf a écrit :
>
> That should hopefully be the final version...
>
Yes it is OK. Thanks for your patience.
Mikael
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-11-26 21:45 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-24 21:32 [PATCH] PR fortran/103411 - ICE in gfc_conv_array_initializer, at fortran/trans-array.c:6377 Harald Anlauf
2021-11-25 16:46 ` Mikael Morin
2021-11-25 20:03 ` Harald Anlauf
2021-11-25 20:03 ` Harald Anlauf
2021-11-25 21:02 ` Mikael Morin
2021-11-25 21:52 ` [PATCH, v2] " Harald Anlauf
2021-11-26 14:45 ` Mikael Morin
2021-11-26 20:07 ` [PATCH, v3] " Harald Anlauf
2021-11-26 21:45 ` Mikael Morin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).