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

[Harald Anlauf]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Fortran-improve-check-of-arguments-to-the-RESHAPE-in.patch
Type: text/x-patch
Size: 5751 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/gcc-patches/attachments/20211125/fa846d19/attachment-0001.bin>