[v2] Fortran: error recovery for invalid types in array constructors [PR107000]
Commit Message
Am 07.10.22 um 10:01 schrieb Mikael Morin:
> Le 06/10/2022 ? 23:36, Harald Anlauf a ?crit?:
>>>
>>> For example, for this case:
>>>
>>> [real :: 2] * [real :: +(.true.)]
>>>
>>> First there is a "root" invocation of reduce binary with arguments [real
>>> :: 2] and [real :: +(.true.)]
>>> The root invocation of reduce_binary will call reduce_binary_aa. This is
>>> normal.
>>>
>>> Then reduce_binary_aa calls reduce_binary again with arguments 2 and
>>> +(.true.).? And reduce_binary calls again reduce_binary_aa with those
>>> arguments.? This is weird, reduce_binary_aa is supposed to have arrays
>>> for both arguments.
>>
>> Am I seeing something different from you?? My gdb says
>> that one argument of reduce_binary is EXPR_CONSTANT,
>> the other EXPR_OP and BT_UNKNOWN.? Both rank 0.
>>
> No, I get the same, and the program goes to reduce_binary_aa with those
> arguments; this is the problem.
>
>>> The same goes for the array vs constant case, reduce_binary_ca (or
>>> reduce_binary_ac) is invoked with two scalars, while if you look at
>>> reduce_binary, you would expect that we only get to reduce_binary_ca
>>> with a scalar constant and an array as arguments.
>>>
>>>
>>> I think the checks in the three reduce_binary_* functions should be
>>> moved into their respective loops, so that we detect the invalid type
>>> just before these weird recursive calls instead of just after entering
>>> into them.
>>
>> I think I tried that before, and it didn't work.
>> There was always one weird case that lead to a bad or
>> invalid constructor for one of the arrays you want to
>> look at in the respective loop,? and this is why the
>> testcase tries to cover everything that I hit then and
>> there... (hopefully).? So I ended up with the check
>> before the loop.
>>
> I see, I'll have a look.
>
>> What do we actually gain with your suggested change?
>> Moving the check into the loop does not really make
>> the code more readable to me.? And the recursion is
>> needed anyway.
>>
> I think we gain clarity, consistency.
>
> I try to rephrase again.
> From a high level point of view, to evaluate a binary operator you need
> a specific (one for each operator) function to evaluate the scalar vs
> scalar case, and three generic (they are common to all the operators)
> functions to handle respectively:
> ? the scalar vs array case,
> ? the array vs scalar case,
> ? the array vs array case,
> by calling in a loop the scalar specific function.
> Here we are only dealing with constants, arrays of constants, arrays of
> arrays, etc, all valid cases.
>
> Your patch introduces support for invalid cases, that is invalid values
> that can't be reduced to a constant.? This is fine, and it works.
> What is weird is that the scalar vs invalid scalar case is caught in the
> array vs array function.
OK, that is because reduce_binary dispatches the reduce_binary_*.
We could move the check from reduce_binary_aa to the beginning of
reduce_binary, as with the following change on top of the patch:
d = gfc_constructor_first (op2>value.constructor);
@@ 1467,6 +1463,10 @@ static arith
reduce_binary (arith (*eval) (gfc_expr *, gfc_expr *, gfc_expr **),
gfc_expr *op1, gfc_expr *op2, gfc_expr **result)
{
+ if ((op1>expr_type == EXPR_OP && op1>ts.type == BT_UNKNOWN)
+  (op2>expr_type == EXPR_OP && op2>ts.type == BT_UNKNOWN))
+ return ARITH_INVALID_TYPE;
+
if (op1>expr_type == EXPR_CONSTANT && op2>expr_type == EXPR_CONSTANT)
return eval (op1, op2, result);
However, we cannot remove the checks from reduce_binary_ac
or reduce_binary_ca, as the lengthy testcase proves...
Do you like the above better?
Cheers,
Harald
Comments
Le 07/10/2022 ? 20:46, Harald Anlauf a ?crit?:
>
> OK, that is because reduce_binary dispatches the reduce_binary_*.
> We could move the check from reduce_binary_aa to the beginning of
> reduce_binary, as with the following change on top of the patch:
>
> diff git a/gcc/fortran/arith.cc b/gcc/fortran/arith.cc
> index 2c57c796270..91e70655ad3 100644
>  a/gcc/fortran/arith.cc
> +++ b/gcc/fortran/arith.cc
> @@ 1426,10 +1426,6 @@ reduce_binary_aa (arith (*eval) (gfc_expr *,
> gfc_expr *, gfc_expr **),
> ?? if (!gfc_check_conformance (op1, op2, _("elemental binary operation")))
> ???? return ARITH_INCOMMENSURATE;
>
> ? if ((op1>expr_type == EXPR_OP && op1>ts.type == BT_UNKNOWN)
> ?????  (op2>expr_type == EXPR_OP && op2>ts.type == BT_UNKNOWN))
> ??? return ARITH_INVALID_TYPE;
> 
> ?? head = gfc_constructor_copy (op1>value.constructor);
> ?? for (c = gfc_constructor_first (head),
> ??????? d = gfc_constructor_first (op2>value.constructor);
> @@ 1467,6 +1463,10 @@ static arith
> ?reduce_binary (arith (*eval) (gfc_expr *, gfc_expr *, gfc_expr **),
> ?????????????? gfc_expr *op1, gfc_expr *op2, gfc_expr **result)
> ?{
> +? if ((op1>expr_type == EXPR_OP && op1>ts.type == BT_UNKNOWN)
> +?????  (op2>expr_type == EXPR_OP && op2>ts.type == BT_UNKNOWN))
> +??? return ARITH_INVALID_TYPE;
> +
> ?? if (op1>expr_type == EXPR_CONSTANT && op2>expr_type == EXPR_CONSTANT)
> ???? return eval (op1, op2, result);
>
> However, we cannot remove the checks from reduce_binary_ac
> or reduce_binary_ca, as the lengthy testcase proves...
>
> Do you like the above better?
>
Yes, definitely, but some less important weirdness remains;
the scalar vs array function catches scalar vs invalid scalar cases.
Let me have a look.
@@ 1426,10 +1426,6 @@ reduce_binary_aa (arith (*eval) (gfc_expr *,
gfc_expr *, gfc_expr **),
if (!gfc_check_conformance (op1, op2, _("elemental binary operation")))
return ARITH_INCOMMENSURATE;
 if ((op1>expr_type == EXPR_OP && op1>ts.type == BT_UNKNOWN)
  (op2>expr_type == EXPR_OP && op2>ts.type == BT_UNKNOWN))
 return ARITH_INVALID_TYPE;

head = gfc_constructor_copy (op1>value.constructor);
for (c = gfc_constructor_first (head),