Re: [PATCH, v2] Fortran: error recovery for invalid types in array constructors [PR107000]
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.
Re: [PATCH, 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:
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
Re: [PATCH, v2] Fortran: error recovery for invalid types in array constructors [PR107000]
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.
[PATCH, v3] Fortran: error recovery for invalid types in array constructors [PR107000]
Le 07/10/2022 à 21:47, Mikael Morin a écrit :
Let me have a look.
The attached patch works with your test, I just moved the checks into
the loops.
I'm now checking the patch against the full fortran testsuite.
I'm (finally) fine with that version, what do you think of it?From a2b393cab384a08164946916ff96dd576ebf7c97 Mon Sep 17 00:00:00 2001
From: Harald Anlauf
Date: Tue, 4 Oct 2022 23:04:06 +0200
Subject: [PATCH] Fortran: error recovery for invalid types in array
constructors [PR107000]
gcc/fortran/ChangeLog:
PR fortran/107000
* arith.cc (gfc_arith_error): Define error message for
ARITH_INVALID_TYPE.
(reduce_unary): Catch arithmetic expressions with invalid type.
(reduce_binary_ac): Likewise.
(reduce_binary_ca): Likewise.
(reduce_binary_aa): Likewise.
(eval_intrinsic): Likewise.
(gfc_real2complex): Source expression must be of type REAL.
* gfortran.h (enum arith): Add ARITH_INVALID_TYPE.
gcc/testsuite/ChangeLog:
PR fortran/107000
* gfortran.dg/pr107000.f90: New test.
Co-authored-by: Mikael Morin
---
gcc/fortran/arith.cc | 30 +---
gcc/fortran/gfortran.h | 2 +-
gcc/testsuite/gfortran.dg/pr107000.f90 | 50 ++
3 files changed, 76 insertions(+), 6 deletions(-)
create mode 100644 gcc/testsuite/gfortran.dg/pr107000.f90
diff --git a/gcc/fortran/arith.cc b/gcc/fortran/arith.cc
index d57059a375f..086b1f856b1 100644
--- a/gcc/fortran/arith.cc
+++ b/gcc/fortran/arith.cc
@@ -118,6 +118,9 @@ gfc_arith_error (arith code)
case ARITH_WRONGCONCAT:
p = G_("Illegal type in character concatenation at %L");
break;
+case ARITH_INVALID_TYPE:
+ p = G_("Invalid type in arithmetic operation at %L");
+ break;
default:
gfc_internal_error ("gfc_arith_error(): Bad error code");
@@ -1268,7 +1271,10 @@ reduce_unary (arith (*eval) (gfc_expr *, gfc_expr **), gfc_expr *op,
head = gfc_constructor_copy (op->value.constructor);
for (c = gfc_constructor_first (head); c; c = gfc_constructor_next (c))
{
- rc = reduce_unary (eval, c->expr, &r);
+ if (c->expr->expr_type == EXPR_OP && c->expr->ts.type == BT_UNKNOWN)
+ rc = ARITH_INVALID_TYPE;
+ else
+ rc = reduce_unary (eval, c->expr, &r);
if (rc != ARITH_OK)
break;
@@ -1309,6 +1315,8 @@ reduce_binary_ac (arith (*eval) (gfc_expr *, gfc_expr *, gfc_expr **),
if (c->expr->expr_type == EXPR_CONSTANT)
rc = eval (c->expr, op2, &r);
+ else if (c->expr->expr_type == EXPR_OP && c->expr->ts.type == BT_UNKNOWN)
+ rc = ARITH_INVALID_TYPE;
else
rc = reduce_binary_ac (eval, c->expr, op2, &r);
@@ -1361,6 +1369,8 @@ reduce_binary_ca (arith (*eval) (gfc_expr *, gfc_expr *, gfc_expr **),
if (c->expr->expr_type == EXPR_CONSTANT)
rc = eval (op1, c->expr, &r);
+ else if (c->expr->expr_type == EXPR_OP && c->expr->ts.type == BT_UNKNOWN)
+ rc = ARITH_INVALID_TYPE;
else
rc = reduce_binary_ca (eval, op1, c->expr, &r);
@@ -1420,14 +1430,19 @@ reduce_binary_aa (arith (*eval) (gfc_expr *, gfc_expr *, gfc_expr **),
c && d;
c = gfc_constructor_next (c), d = gfc_constructor_next (d))
{
+ if ((c->expr->expr_type == EXPR_OP && c->expr->ts.type == BT_UNKNOWN)
+ || (d->expr->expr_type == EXPR_OP && d->expr->ts.type == BT_UNKNOWN))
+ rc = ARITH_INVALID_TYPE;
+ else
rc = reduce_binary (eval, c->expr, d->expr, &r);
- if (rc != ARITH_OK)
- break;
- gfc_replace_expr (c->expr, r);
+ if (rc != ARITH_OK)
+ break;
+
+ gfc_replace_expr (c->expr, r);
}
- if (c || d)
+ if (rc == ARITH_OK && (c || d))
rc = ARITH_INCOMMENSURATE;
if (rc != ARITH_OK)
@@ -1638,6 +1653,8 @@ eval_intrinsic (gfc_intrinsic_op op,
else
rc = reduce_binary (eval.f3, op1, op2, &result);
+ if (rc == ARITH_INVALID_TYPE)
+goto runtime;
/* Something went wrong. */
if (op == INTRINSIC_POWER && rc == ARITH_PROHIBIT)
@@ -2238,6 +2255,9 @@ gfc_real2complex (gfc_expr *src, int kind)
arith rc;
bool did_warn = false;
+ if (src->ts.type != BT_REAL)
+return NULL;
+
result = gfc_get_constant_expr (BT_COMPLEX, kind, &src->where);
mpc_set_fr (result->value.complex, src->value.real, GFC_MPC_RND_MODE);
diff --git a/gcc/fortran/gfortran.h b/gcc/fortran/gfortran.h
index 4babd77924b..fc0aa51df57 100644
--- a/gcc/fortran/gfortran.h
+++ b/gcc/fortran/gfortran.h
@@ -226,7 +226,7 @@ enum gfc_intrinsic_op
enum arith
{ ARITH_OK = 1, ARITH_OVERFLOW, ARITH_UNDERFLOW, ARITH_NAN,
ARITH_DIV0, ARITH_INCOMMENSURATE, ARITH_ASYMMETRIC, ARITH_PROHIBIT,
- ARITH_WRONGCONCAT
+ ARITH_WRONGCONCAT, ARITH_INVALID_TYPE
};
/* Statements. */
diff --git a/gcc/testsuite/gfortran.dg/pr107000.f90 b/gcc/testsuite/gfortran.dg/pr107000.f90
new file mode 100644
index 000..30289078c57
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/pr107000.f90
@@ -0,0 +1,50 @@
+! { dg-do compile }
+! PR fortran/107000 - ICE in gfc_real2complex, reduce_unary, reduce_binary_*
+
Re: [PATCH, v3] Fortran: error recovery for invalid types in array constructors [PR107000]
Hi Mikael, Am 07.10.22 um 22:26 schrieb Mikael Morin: Le 07/10/2022 à 21:47, Mikael Morin a écrit : Let me have a look. The attached patch works with your test, I just moved the checks into the loops. I'm now checking the patch against the full fortran testsuite. I'm (finally) fine with that version, what do you think of it? I'm fine with it. If it regtests ok, then this should be it.
