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

  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
  

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.