* [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).