Re: [Patch, Fortran] PR 85599: warn about short-circuiting of logical expressions for non-pure functions

[Janus Weil <janus at gcc dot gnu dot org>]

Hi all,

here is a minor update of the patch, which cures some problems with
implicitly pure functions in the previous version.

Most implicitly pure functions were actually detected correctly, but
implicitly pure functions that called other implicitly pure functions
were not detected properly, and therefore could trigger a warning.
This is fixed in the current version, which still regtests cleanly
(note that alloc-comp-3.f90 currently fails due to PR 86417). The test
case is also enhanced to include the problematic case.

Ok for trunk?

Cheers,
Janus



2018-07-11 23:06 GMT+02:00 Janus Weil <janus@gcc.gnu.org>:
> Hi all,
>
> after the dust of the heated discussion around this PR has settled a
> bit, here is another attempt to implement at least some basic warnings
> about compiler-dependent behavior concerning the short-circuiting of
> logical expressions.
>
> As a reminder (and recap of the previous discussion), the Fortran
> standard unfortunately is a bit sloppy in this area: It allows
> compilers to short-circuit the second operand of .AND. / .OR.
> operators, but does not require this. As a result, compilers can do
> what they want without conflicting with the standard, and they do:
> gfortran does short-circuiting (via TRUTH_ANDIF_EXPR/TRUTH_ORIF_EXPR),
> ifort does not.
>
> I'm continuing here the least-invasive approach of keeping gfortran's
> current behavior, but warning about cases where compilers may produce
> different results.
>
> The attached patch is very close to the version I posted previously
> (which was already approved by Janne), with the difference that the
> warnings are now triggered by -Wextra and not -Wsurprising (which is
> included in -Wall), as suggested by Nick Maclaren. I think this is
> more reasonable, since not everyone may want to see these warnings.
>
> Note that I don't want to warn about all possible optimizations that
> might be allowed by the standard, but only about those that are
> actually problematic in practice and result in compiler-dependent
> behavior.
>
> The patch regtests cleanly on x86_64-linux-gnu. Ok for trunk?
>
> Cheers,
> Janus
>
>
> 2018-07-11  Thomas Koenig  <tkoenig@gcc.gnu.org>
>         Janus Weil  <janus@gcc.gnu.org>
>
>         PR fortran/85599
>         * dump-parse-tree (show_attr): Add handling of implicit_pure.
>         * resolve.c (impure_function_callback): New function.
>         (resolve_operator): Call it vial gfc_expr_walker.
>
>
> 2018-07-11  Janus Weil  <janus@gcc.gnu.org>
>
>         PR fortran/85599
>         * gfortran.dg/short_circuiting.f90: New test.

Index: gcc/fortran/dump-parse-tree.c
===================================================================
--- gcc/fortran/dump-parse-tree.c	(revision 262563)
+++ gcc/fortran/dump-parse-tree.c	(working copy)
@@ -716,6 +716,8 @@ show_attr (symbol_attribute *attr, const char * mo
     fputs (" ELEMENTAL", dumpfile);
   if (attr->pure)
     fputs (" PURE", dumpfile);
+  if (attr->implicit_pure)
+    fputs (" IMPLICIT_PURE", dumpfile);
   if (attr->recursive)
     fputs (" RECURSIVE", dumpfile);
 
Index: gcc/fortran/resolve.c
===================================================================
--- gcc/fortran/resolve.c	(revision 262563)
+++ gcc/fortran/resolve.c	(working copy)
@@ -2982,6 +2982,19 @@ pure_function (gfc_expr *e, const char **name)
 }
 
 
+static int
+implicit_pure_function (gfc_expr *e)
+{
+  gfc_component *comp = gfc_get_proc_ptr_comp (e);
+  if (comp)
+    return gfc_implicit_pure (comp->ts.interface);
+  else if (e->value.function.esym)
+    return gfc_implicit_pure (e->value.function.esym);
+  else
+    return 0;
+}
+
+
 static bool
 impure_stmt_fcn (gfc_expr *e, gfc_symbol *sym,
 		 int *f ATTRIBUTE_UNUSED)
@@ -3034,7 +3047,8 @@ static bool check_pure_function (gfc_expr *e)
 		     "within a PURE procedure", name, &e->where);
 	  return false;
 	}
-      gfc_unset_implicit_pure (NULL);
+      if (!implicit_pure_function (e))
+	gfc_unset_implicit_pure (NULL);
     }
   return true;
 }
@@ -3822,6 +3836,46 @@ lookup_uop_fuzzy (const char *op, gfc_symtree *uop
 }
 
 
+/* Callback finding an impure function as an operand to an .and. or
+   .or.  expression.  Remember the last function warned about to
+   avoid double warnings when recursing.  */
+
+static int
+impure_function_callback (gfc_expr **e, int *walk_subtrees ATTRIBUTE_UNUSED,
+			  void *data)
+{
+  gfc_expr *f = *e;
+  const char *name;
+  static gfc_expr *last = NULL;
+  bool *found = (bool *) data;
+
+  if (f->expr_type == EXPR_FUNCTION)
+    {
+      *found = 1;
+      if (f != last && !pure_function (f, &name))
+	{
+	  /* This could still be a function without side effects, i.e.
+	     implicit pure.  Do not warn for that case.  */
+	  if (f->symtree == NULL || f->symtree->n.sym == NULL
+	      || !gfc_implicit_pure (f->symtree->n.sym))
+	    {
+	      if (name)
+		gfc_warning (OPT_Wextra,
+			     "Function %qs at %L might not be evaluated",
+			     name, &f->where);
+	      else
+		gfc_warning (OPT_Wextra,
+			     "Function at %L might not be evaluated",
+			     &f->where);
+	    }
+	}
+      last = f;
+    }
+
+  return 0;
+}
+
+
 /* Resolve an operator expression node.  This can involve replacing the
    operation with a user defined function call.  */
 
@@ -3930,6 +3984,14 @@ resolve_operator (gfc_expr *e)
 	    gfc_convert_type (op1, &e->ts, 2);
 	  else if (op2->ts.kind < e->ts.kind)
 	    gfc_convert_type (op2, &e->ts, 2);
+
+	  if (e->value.op.op == INTRINSIC_AND || e->value.op.op == INTRINSIC_OR)
+	    {
+	      /* Warn about short-circuiting
+	         with impure function as second operand.  */
+	      bool op2_f = false;
+	      gfc_expr_walker (&op2, impure_function_callback, &op2_f);
+	    }
 	  break;
 	}
 

! { dg-do compile }
! { dg-additional-options "-Wextra" }
!
! PR 85599: warn about short-circuiting of logical expressions for non-pure functions
!
! Contributed by Janus Weil <janus@gcc.gnu.org>

program short_circuit

   logical :: flag
   flag = .false.
   flag = check() .and. flag
   flag = flag .and. check()        ! { dg-warning "might not be evaluated" }
   flag = flag .and. pure_check()
   flag = flag .and. impl_pure_1()
   flag = flag .and. impl_pure_2()

contains

   logical function check()
      integer, save :: i = 1
      print *, "check", i
      i = i + 1
      check = .true.
   end function

   logical pure function pure_check()
      pure_check = .true.
   end function

    logical function impl_pure_1()
      impl_pure_1 = .true.
   end function

    logical function impl_pure_2()
      impl_pure_2 = impl_pure_1()
   end function
end