Re: [PATCH, v2] Fortran: error recovery for invalid types in array constructors [PR107000]

[Harald Anlauf <anlauf@gmx.de>]

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:

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?

Cheers,
Harald