diff mbox series

[fortran] Fix PR 96018, wrong code caused by implicit_pure

Message ID 3d4ad562-5532-f4e1-0311-b708133c22ff@netcologne.de
State New
Headers show
Series [fortran] Fix PR 96018, wrong code caused by implicit_pure | expand

Commit Message

Thomas Koenig July 10, 2020, 4:58 p.m. UTC
Hello world,

the attached patch fixes a 9/10/11 regression where we left over
an implicit_pure attribute when a non-implicit_pure procedure was
called.

The fix is explained in the ChangeLog.

If there is a quick review, please also indicate if you think it is
still suitable for gcc 10.2.  If the review comes later, then
it obviously will not be :-)  On the code side, this should be
fairly safe, because all we are doing is to be more defensive
about our optimizations.

OK for trunk/backporting?  This is not something I want to commit
without a review because it is not quite obvious and simple :-)

Best regards

	Thomas

Fix handling of implicit_pure by checking if non-pure procedures are called.

Procedures are marked as implicit_pure if they fulfill the criteria of
pure procedures.  In this case, a procedure was not marked as not being
implicit_pure which called another procedure, which had not yet been
marked as not being implicit_impure.

Fixed by iterating over all procedures, setting callers of procedures
which are non-pure and non-implicit_pure as non-implicit_pure and
doing this until no more procedure has been changed.


gcc/fortran/ChangeLog:

2020-07-10  Thomas Koenig  <tkoenig@gcc.gnu.org>

	PR fortran/96018
	* frontend-passes.c (gfc_check_externals): Adjust formatting.
	(implicit_pure_call): New function.
	(implicit_pure_expr): New function.
	(gfc_fix_implicit_pure): New function.
	* gfortran.h (gfc_fix_implicit_pure): New prototype.
	* parse.c (translate_all_program_units): Call gfc_fix_implicit_pure.

Comments

Thomas Koenig July 18, 2020, 10:57 a.m. UTC | #1
Ping?

> the attached patch fixes a 9/10/11 regression where we left over
> an implicit_pure attribute when a non-implicit_pure procedure was
> called.
> 
> The fix is explained in the ChangeLog.
Paul Richard Thomas July 18, 2020, 4:44 p.m. UTC | #2
Hi Thomas,

The patch looks fine to me but I have two questions:

(i) Why is this not done in resolve.c?
(ii) Is Martin's reduced reproducer not the basis for a testcase?

Regards

Paul


On Sat, 18 Jul 2020 at 11:58, Thomas Koenig via Fortran <fortran@gcc.gnu.org>
wrote:

> Ping?
>
> > the attached patch fixes a 9/10/11 regression where we left over
> > an implicit_pure attribute when a non-implicit_pure procedure was
> > called.
> >
> > The fix is explained in the ChangeLog.
>
>
>
Thomas Koenig July 18, 2020, 5:57 p.m. UTC | #3
Hi Paul,

> The patch looks fine to me but I have two questions:
> 
> (i) Why is this not done in resolve.c?

Of course it doesn't matter where the function resides :-)  I put it
into frontend-passes.c because it makes heavy use of gfc_code_walker,
and out of habit. If you prefer, I can of course move the code to
resolve.c.

If your question is more like "why is that not done during
normal resolution" - while fixing this bug, I began to understand
why the standard has an explicit PURE attribute.  If the compiler
is chasing after a procedure which may or may not be implicit_pure, then
everything else needs to have been resolved beforehand. And in normal
resolution, something needs to be the last.

> (ii) Is Martin's reduced reproducer not the basis for a testcase?

There, git got the better of me.  I thought it was included in the
patch, but, as I reread it and found that this wasn't the case.

I have attache the test case now.

OK for trunk and backport once gcc 10 is open again?

Regards

	Thomas
Paul Richard Thomas July 19, 2020, 6:58 a.m. UTC | #4
Hi Thomas,

I am fine with this being in frontend-passes.c - it was just a question
:-)  resolve.c has become too large anyway.

The testcase looks familiar! Don't forget to commit and push the additional
source too.

OK for 10-- and all the affected branches.

Cheers

Paul

On Sat, 18 Jul 2020 at 18:57, Thomas Koenig <tkoenig@netcologne.de> wrote:

> Hi Paul,
>
> > The patch looks fine to me but I have two questions:
> >
> > (i) Why is this not done in resolve.c?
>
> Of course it doesn't matter where the function resides :-)  I put it
> into frontend-passes.c because it makes heavy use of gfc_code_walker,
> and out of habit. If you prefer, I can of course move the code to
> resolve.c.
>
> If your question is more like "why is that not done during
> normal resolution" - while fixing this bug, I began to understand
> why the standard has an explicit PURE attribute.  If the compiler
> is chasing after a procedure which may or may not be implicit_pure, then
> everything else needs to have been resolved beforehand. And in normal
> resolution, something needs to be the last.
>
> > (ii) Is Martin's reduced reproducer not the basis for a testcase?
>
> There, git got the better of me.  I thought it was included in the
> patch, but, as I reread it and found that this wasn't the case.
>
> I have attache the test case now.
>
> OK for trunk and backport once gcc 10 is open again?
>
> Regards
>
>         Thomas
>
diff mbox series

Patch

diff --git a/gcc/fortran/frontend-passes.c b/gcc/fortran/frontend-passes.c
index 69f9ca64c97..4b1d3f2feda 100644
--- a/gcc/fortran/frontend-passes.c
+++ b/gcc/fortran/frontend-passes.c
@@ -5550,7 +5550,8 @@  gfc_check_externals0 (gfc_namespace *ns)
 
 /* Called routine.  */
 
-void gfc_check_externals (gfc_namespace *ns)
+void
+gfc_check_externals (gfc_namespace *ns)
 {
   gfc_clear_error ();
 
@@ -5565,3 +5566,76 @@  void gfc_check_externals (gfc_namespace *ns)
   gfc_errors_to_warnings (false);
 }
 
+/* Callback function. If there is a call to a subroutine which is
+   neither pure nor implicit_pure, unset the implicit_pure flag for
+   the caller and return -1.  */
+
+static int
+implicit_pure_call (gfc_code **c, int *walk_subtrees ATTRIBUTE_UNUSED,
+		    void *sym_data)
+{
+  gfc_code *co = *c;
+  gfc_symbol *caller_sym;
+  symbol_attribute *a;
+
+  if (co->op != EXEC_CALL || co->resolved_sym == NULL)
+    return 0;
+
+  a = &co->resolved_sym->attr;
+  if (a->intrinsic || a->pure || a->implicit_pure)
+    return 0;
+
+  caller_sym = (gfc_symbol *) sym_data;
+  gfc_unset_implicit_pure (caller_sym);
+  return 1;
+}
+
+/* Callback function. If there is a call to a function which is
+   neither pure nor implicit_pure, unset the implicit_pure flag for
+   the caller and return 1.  */
+
+static int
+implicit_pure_expr (gfc_expr **e, int *walk ATTRIBUTE_UNUSED, void *sym_data)
+{
+  gfc_expr *expr = *e;
+  gfc_symbol *caller_sym;
+  gfc_symbol *sym;
+  symbol_attribute *a;
+
+  if (expr->expr_type != EXPR_FUNCTION || expr->value.function.isym)
+    return 0;
+
+  sym = expr->symtree->n.sym;
+  a = &sym->attr;
+  if (a->pure || a->implicit_pure)
+    return 0;
+
+  caller_sym = (gfc_symbol *) sym_data;
+  gfc_unset_implicit_pure (caller_sym);
+  return 1;
+}
+
+/* Go through all procedures in the namespace and unset the
+   implicit_pure attribute for any procedure that calls something not
+   pure or implicit pure.  */
+
+bool
+gfc_fix_implicit_pure (gfc_namespace *ns)
+{
+  bool changed = false;
+  gfc_symbol *proc = ns->proc_name;
+
+  if (proc && proc->attr.flavor == FL_PROCEDURE && proc->attr.implicit_pure
+      && ns->code
+      && gfc_code_walker (&ns->code, implicit_pure_call, implicit_pure_expr,
+			  (void *) ns->proc_name))
+    changed = true;
+
+  for (ns = ns->contained; ns; ns = ns->sibling)
+    {
+      if (gfc_fix_implicit_pure (ns))
+	changed = true;
+    }
+
+  return changed;
+}
diff --git a/gcc/fortran/gfortran.h b/gcc/fortran/gfortran.h
index 24c5101c4cb..264822ef9f8 100644
--- a/gcc/fortran/gfortran.h
+++ b/gcc/fortran/gfortran.h
@@ -3623,6 +3623,7 @@  int gfc_expr_walker (gfc_expr **, walk_expr_fn_t, void *);
 int gfc_code_walker (gfc_code **, walk_code_fn_t, walk_expr_fn_t, void *);
 bool gfc_has_dimen_vector_ref (gfc_expr *e);
 void gfc_check_externals (gfc_namespace *);
+bool gfc_fix_implicit_pure (gfc_namespace *);
 
 /* simplify.c */
 
diff --git a/gcc/fortran/parse.c b/gcc/fortran/parse.c
index 36715134a2c..d30208febb1 100644
--- a/gcc/fortran/parse.c
+++ b/gcc/fortran/parse.c
@@ -6447,6 +6447,11 @@  loop:
 
   gfc_resolve (gfc_current_ns);
 
+  /* Fix the implicit_pure attribute for those procedures who should
+     not have it.  */
+  while (gfc_fix_implicit_pure (gfc_current_ns))
+    ;
+
   /* Dump the parse tree if requested.  */
   if (flag_dump_fortran_original)
     gfc_dump_parse_tree (gfc_current_ns, stdout);
@@ -6492,6 +6497,23 @@  done:
   /* Do the resolution.  */
   resolve_all_program_units (gfc_global_ns_list);
 
+  /* Go through all top-level namespaces and unset the implicit_pure
+     attribute for any procedures that call something not pure or
+     implicit_pure.  Because the a procedure marked as not implicit_pure
+     in one sweep may be called by another routine, we repeat this
+     process until there are no more changes.  */
+  bool changed;
+  do
+    {
+      changed = false;
+      for (gfc_current_ns = gfc_global_ns_list; gfc_current_ns;
+	   gfc_current_ns = gfc_current_ns->sibling)
+	{
+	  if (gfc_fix_implicit_pure (gfc_current_ns))
+	    changed = true;
+	}
+    }
+  while (changed);
 
   /* Fixup for external procedures.  */
   for (gfc_current_ns = gfc_global_ns_list; gfc_current_ns;