Re: [PATCH v3 2/2] fortran: Fix specification expression error with dummy procedures [PR111781]
Hi Mikael,
This is very good. I am pleased to see global variables disappear and I
like the new helper functions.
As before, OK for mainline and, if you wish, 13-branch.
Thanks
Paul
On Tue, 19 Mar 2024 at 15:49, Mikael Morin wrote:
> This fixes a spurious invalid variable in specification expression error.
> The error was caused on the testcase from the PR by two different bugs.
> First, the call to is_parent_of_current_ns was unable to recognize
> correct host association and returned false. Second, an ad-hoc
> condition coming next was using a global variable previously improperly
> restored to false (instead of restoring it to its initial value). The
> latter happened on the testcase because one dummy argument was a procedure,
> and checking that argument what causing a check of all its arguments with
> the (improper) reset of the flag at the end, and that preceded the check of
> the next argument.
>
> For the first bug, the wrong result of is_parent_of_current_ns is fixed by
> correcting the namespaces that function deals with, both the one passed
> as argument and the current one tracked in the gfc_current_ns global. Two
> new functions are introduced to select the right namespace.
>
> Regarding the second bug, the problematic condition is removed, together
> with the formal_arg_flag associated with it. Indeed, that condition was
> (wrongly) allowing local variables to be used in array bounds of dummy
> arguments.
>
> PR fortran/111781
>
> gcc/fortran/ChangeLog:
>
> * symbol.cc (gfc_get_procedure_ns, gfc_get_spec_ns): New functions.
> * gfortran.h (gfc_get_procedure_ns, gfc_get_spec ns): Declare them.
> (gfc_is_formal_arg): Remove.
> * expr.cc (check_restricted): Remove special case allowing local
> variable in dummy argument bound expressions. Use gfc_get_spec_ns
> to get the right namespace.
> * resolve.cc (gfc_is_formal_arg, formal_arg_flag): Remove.
> (gfc_resolve_formal_arglist): Set gfc_current_ns. Quit loop and
> restore gfc_current_ns instead of early returning.
> (resolve_symbol): Factor common array spec resolution code to...
> (resolve_symbol_array_spec): ... this new function. Additionnally
> set and restore gfc_current_ns.
>
> gcc/testsuite/ChangeLog:
>
> * gfortran.dg/spec_expr_8.f90: New test.
> * gfortran.dg/spec_expr_9.f90: New test.
> ---
> gcc/fortran/expr.cc | 8 +--
> gcc/fortran/gfortran.h| 4 +-
> gcc/fortran/resolve.cc| 77 +++
> gcc/fortran/symbol.cc | 58 +
> gcc/testsuite/gfortran.dg/spec_expr_8.f90 | 24 +++
> gcc/testsuite/gfortran.dg/spec_expr_9.f90 | 19 ++
> 6 files changed, 140 insertions(+), 50 deletions(-)
> create mode 100644 gcc/testsuite/gfortran.dg/spec_expr_8.f90
> create mode 100644 gcc/testsuite/gfortran.dg/spec_expr_9.f90
>
> diff --git a/gcc/fortran/expr.cc b/gcc/fortran/expr.cc
> index e4b1e8307e3..9a042cd7040 100644
> --- a/gcc/fortran/expr.cc
> +++ b/gcc/fortran/expr.cc
> @@ -3514,19 +3514,13 @@ check_restricted (gfc_expr *e)
>if (!check_references (e->ref, _restricted))
> break;
>
> - /* gfc_is_formal_arg broadcasts that a formal argument list is being
> -processed in resolve.cc(resolve_formal_arglist). This is done so
> -that host associated dummy array indices are accepted (PR23446).
> -This mechanism also does the same for the specification
> expressions
> -of array-valued functions. */
>if (e->error
> || sym->attr.in_common
> || sym->attr.use_assoc
> || sym->attr.dummy
> || sym->attr.implied_index
> || sym->attr.flavor == FL_PARAMETER
> - || is_parent_of_current_ns (sym->ns)
> - || (gfc_is_formal_arg () && (sym->ns == gfc_current_ns)))
> + || is_parent_of_current_ns (gfc_get_spec_ns (sym)))
> {
> t = true;
> break;
> diff --git a/gcc/fortran/gfortran.h b/gcc/fortran/gfortran.h
> index c7039730fad..26aa56b3358 100644
> --- a/gcc/fortran/gfortran.h
> +++ b/gcc/fortran/gfortran.h
> @@ -3612,6 +3612,9 @@ bool gfc_is_associate_pointer (gfc_symbol*);
> gfc_symbol * gfc_find_dt_in_generic (gfc_symbol *);
> gfc_formal_arglist *gfc_sym_get_dummy_args (gfc_symbol *);
>
> +gfc_namespace * gfc_get_procedure_ns (gfc_symbol *);
> +gfc_namespace * gfc_get_spec_ns (gfc_symbol *);
> +
> /* intrinsic.cc -- true if working in an init-expr, false otherwise. */
> extern bool gfc_init_expr_flag;
>
> @@ -3821,7 +3824,6 @@ bool gfc_resolve_iterator (gfc_iterator *, bool,
> bool);
> bool find_forall_index (gfc_expr *, gfc_symbol *, int);
> bool gfc_resolve_index (gfc_expr *, int);
> bool gfc_resolve_dim_arg (gfc_expr *);
> -bool gfc_is_formal_arg (void);
> bool gfc_resolve_substring (gfc_ref *, bool *);
> void
[PATCH v3 2/2] fortran: Fix specification expression error with dummy procedures [PR111781]
This fixes a spurious invalid variable in specification expression error.
The error was caused on the testcase from the PR by two different bugs.
First, the call to is_parent_of_current_ns was unable to recognize
correct host association and returned false. Second, an ad-hoc
condition coming next was using a global variable previously improperly
restored to false (instead of restoring it to its initial value). The
latter happened on the testcase because one dummy argument was a procedure,
and checking that argument what causing a check of all its arguments with
the (improper) reset of the flag at the end, and that preceded the check of
the next argument.
For the first bug, the wrong result of is_parent_of_current_ns is fixed by
correcting the namespaces that function deals with, both the one passed
as argument and the current one tracked in the gfc_current_ns global. Two
new functions are introduced to select the right namespace.
Regarding the second bug, the problematic condition is removed, together
with the formal_arg_flag associated with it. Indeed, that condition was
(wrongly) allowing local variables to be used in array bounds of dummy
arguments.
PR fortran/111781
gcc/fortran/ChangeLog:
* symbol.cc (gfc_get_procedure_ns, gfc_get_spec_ns): New functions.
* gfortran.h (gfc_get_procedure_ns, gfc_get_spec ns): Declare them.
(gfc_is_formal_arg): Remove.
* expr.cc (check_restricted): Remove special case allowing local
variable in dummy argument bound expressions. Use gfc_get_spec_ns
to get the right namespace.
* resolve.cc (gfc_is_formal_arg, formal_arg_flag): Remove.
(gfc_resolve_formal_arglist): Set gfc_current_ns. Quit loop and
restore gfc_current_ns instead of early returning.
(resolve_symbol): Factor common array spec resolution code to...
(resolve_symbol_array_spec): ... this new function. Additionnally
set and restore gfc_current_ns.
gcc/testsuite/ChangeLog:
* gfortran.dg/spec_expr_8.f90: New test.
* gfortran.dg/spec_expr_9.f90: New test.
---
gcc/fortran/expr.cc | 8 +--
gcc/fortran/gfortran.h| 4 +-
gcc/fortran/resolve.cc| 77 +++
gcc/fortran/symbol.cc | 58 +
gcc/testsuite/gfortran.dg/spec_expr_8.f90 | 24 +++
gcc/testsuite/gfortran.dg/spec_expr_9.f90 | 19 ++
6 files changed, 140 insertions(+), 50 deletions(-)
create mode 100644 gcc/testsuite/gfortran.dg/spec_expr_8.f90
create mode 100644 gcc/testsuite/gfortran.dg/spec_expr_9.f90
diff --git a/gcc/fortran/expr.cc b/gcc/fortran/expr.cc
index e4b1e8307e3..9a042cd7040 100644
--- a/gcc/fortran/expr.cc
+++ b/gcc/fortran/expr.cc
@@ -3514,19 +3514,13 @@ check_restricted (gfc_expr *e)
if (!check_references (e->ref, _restricted))
break;
- /* gfc_is_formal_arg broadcasts that a formal argument list is being
-processed in resolve.cc(resolve_formal_arglist). This is done so
-that host associated dummy array indices are accepted (PR23446).
-This mechanism also does the same for the specification expressions
-of array-valued functions. */
if (e->error
|| sym->attr.in_common
|| sym->attr.use_assoc
|| sym->attr.dummy
|| sym->attr.implied_index
|| sym->attr.flavor == FL_PARAMETER
- || is_parent_of_current_ns (sym->ns)
- || (gfc_is_formal_arg () && (sym->ns == gfc_current_ns)))
+ || is_parent_of_current_ns (gfc_get_spec_ns (sym)))
{
t = true;
break;
diff --git a/gcc/fortran/gfortran.h b/gcc/fortran/gfortran.h
index c7039730fad..26aa56b3358 100644
--- a/gcc/fortran/gfortran.h
+++ b/gcc/fortran/gfortran.h
@@ -3612,6 +3612,9 @@ bool gfc_is_associate_pointer (gfc_symbol*);
gfc_symbol * gfc_find_dt_in_generic (gfc_symbol *);
gfc_formal_arglist *gfc_sym_get_dummy_args (gfc_symbol *);
+gfc_namespace * gfc_get_procedure_ns (gfc_symbol *);
+gfc_namespace * gfc_get_spec_ns (gfc_symbol *);
+
/* intrinsic.cc -- true if working in an init-expr, false otherwise. */
extern bool gfc_init_expr_flag;
@@ -3821,7 +3824,6 @@ bool gfc_resolve_iterator (gfc_iterator *, bool, bool);
bool find_forall_index (gfc_expr *, gfc_symbol *, int);
bool gfc_resolve_index (gfc_expr *, int);
bool gfc_resolve_dim_arg (gfc_expr *);
-bool gfc_is_formal_arg (void);
bool gfc_resolve_substring (gfc_ref *, bool *);
void gfc_resolve_substring_charlen (gfc_expr *);
gfc_expr *gfc_expr_to_initialize (gfc_expr *);
diff --git a/gcc/fortran/resolve.cc b/gcc/fortran/resolve.cc
index c5ae826bd6e..50d51b06c92 100644
--- a/gcc/fortran/resolve.cc
+++ b/gcc/fortran/resolve.cc
@@ -72,9 +72,6 @@ static bool first_actual_arg = false;
static int omp_workshare_flag;
-/* True if we are processing a formal arglist. The corresponding function
- resets the flag each
