From patchwork Fri Oct 7 18:46:20 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Harald Anlauf X-Patchwork-Id: 58779 From: anlauf@gmx.de (Harald Anlauf) Date: Fri, 7 Oct 2022 20:46:20 +0200 Subject: [PATCH, v2] Fortran: error recovery for invalid types in array constructors [PR107000] In-Reply-To: <05a23138-adcd-2526-698c-1fa846f1810b@orange.fr> References: <1bf3b7b5-39ac-0c94-256c-f739a4746a7b@orange.fr> <97dd508f-83b0-5ed0-8cb5-f4f7c8fe08e6@orange.fr> <05a23138-adcd-2526-698c-1fa846f1810b@orange.fr> Message-ID: <05f415c0-80bc-c04e-a142-1251bf82bb1d@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: 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 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),