diff mbox

RFA (gimplify): PATCH to implement C++ order of evaluation paper

Message ID CADzB+2nReoZvmMHCLF42zp4zOv2hkQF_-eJXd+bVCR13Amt=bg@mail.gmail.com
State New
Headers show

Commit Message

Jason Merrill July 8, 2016, 8:21 p.m. UTC
On Fri, Jul 8, 2016 at 9:42 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Thu, Jul 07, 2016 at 03:18:13PM -0400, Jason Merrill wrote:
>> How about this?  I also have a patch to handle assignment order
>> entirely in the front end, but my impression has been that you wanted
>> to make this change for other reasons as well.
>
> So what exactly is supposed to be the evaluation order for function calls
> with lhs in C++17?
> Reading
> http://en.cppreference.com/w/cpp/language/eval_order
> I'm confused.
> struct S { S (); ~S (); ... };
> S s[1024];
> typedef S (*fn) (int, int);
> fn a[1024];
> void foo (int *i, int *j, int *k, int *l)
> {
>   s[i[0]++] = (a[j[0]++]) (k[0]++, l[0]++);
> }
> So, j[0]++ needs to happen first, then k[0]++ and l[0]++ (indeterminately
> sequenced), but what about the function call vs. i[0]++?
>
> There is the rule that for E1 = E2 all side-effects of E2 happen before all
> side-effects of E1.
>
> I mean, if the function return type is a gimple reg type, then I see no
> problem in honoring that, the function call returns a temporary, then the
> side-effects of the lhs are evaluated and then it is stored to that lvalue.
>
> But, if the return type is non-POD, then we need to pass the address of the
> lhs as invisible reference to the function call, how can we do it if we
> can't yet evaluate the side-effects of the lhs?
>
> Perhaps better testcase is:
>
> int bar (int);
> void baz ()
> {
>   s[bar (0)] = (a[bar (1)]) (bar (2), 0);
> }
>
> In which order all the 4 calls are made?
>
> What the patch you've posted does is that it gimplifies from_p first,
> and gimplify_call_expr will first evaluate bar (1), then bar (2),
> but then it is a CALL_EXPR; then it gimplifies the lhs, i.e. bar (0)
> call, and finally the indirect call.

As we discussed in IRC, to get the required semantics the front-end
needs to prevent this gimplifier optimization, as in this patch.  The
second patch changes -fargs-in-order to -fstrong-eval-order and
removes the ordering of function arguments.

Tested x86_64-pc-linux-gnu, applying to trunk.
commit 6f19d771aa9d1afde0e56aca5b69235fb19d1daa
Author: Jason Merrill <jason@redhat.com>
Date:   Thu Jul 7 14:30:43 2016 -0400

    	P0145R2: Refining Expression Order for C++ (assignment 2).
    
    	* cp-gimplify.c (lvalue_has_side_effects): New.
    	(cp_gimplify_expr): Implement assignment ordering.
commit 6f4916044fa7c0f94a68b8895ff20c5cee41796f
Author: Jason Merrill <jason@redhat.com>
Date:   Sat Jun 25 19:09:42 2016 +0300

    	P0145: Refining Expression Order for C++ (-fstrong-eval-order).
    
    gcc/c-family/
    	* c.opts (-fargs-in-order): Rename to -fstrong-eval-order.
    	* c-opts.c: Adjust.
    gcc/cp/
    	* call.c (op_is_ordered, build_over_call): Adjust for
    	-fargs-in-order renaming to -fstrong-eval-order.
    	* cp-gimplify.c (cp_gimplify_expr): Likewise.

diff --git a/gcc/c-family/c-opts.c b/gcc/c-family/c-opts.c
index ff6339c..d945825 100644
--- a/gcc/c-family/c-opts.c
+++ b/gcc/c-family/c-opts.c
@@ -910,11 +910,11 @@ c_common_post_options (const char **pfilename)
   else if (warn_narrowing == -1)
     warn_narrowing = 0;
 
-  /* C++17 requires that function arguments be evaluated left-to-right even on
-     PUSH_ARGS_REVERSED targets.  */
+  /* C++17 has stricter evaluation order requirements; let's use some of them
+     for earlier C++ as well, so chaining works as expected.  */
   if (c_dialect_cxx ()
-      && flag_args_in_order == -1)
-    flag_args_in_order = 2 /*(cxx_dialect >= cxx1z) ? 2 : 0*/;
+      && flag_strong_eval_order == -1)
+    flag_strong_eval_order = (cxx_dialect >= cxx1z ? 2 : 1);
 
   /* Global sized deallocation is new in C++14.  */
   if (flag_sized_deallocation == -1)
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 83fd84c..8c70152 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -1043,14 +1043,6 @@ falt-external-templates
 C++ ObjC++ Ignore Warn(switch %qs is no longer supported)
 No longer supported.
 
-fargs-in-order
-C++ ObjC++ Alias(fargs-in-order=, 2, 0)
-Always evaluate function arguments in left-to-right order.
-
-fargs-in-order=
-C++ ObjC++ Var(flag_args_in_order) Joined UInteger Init(-1)
-Always evaluate function arguments in left-to-right order.
-
 fasm
 C ObjC C++ ObjC++ Var(flag_no_asm, 0)
 Recognize the \"asm\" keyword.
@@ -1518,6 +1510,28 @@ Assume that values of enumeration type are always within the minimum range of th
 fstrict-prototype
 C++ ObjC++ Ignore Warn(switch %qs is no longer supported)
 
+fstrong-eval-order
+C++ ObjC++ Common Alias(fstrong-eval-order=, all, none)
+Follow the C++17 evaluation order requirements for assignment expressions,
+shift, member function calls, etc.
+
+fstrong-eval-order=
+C++ ObjC++ Common Var(flag_strong_eval_order) Joined Enum(strong_eval_order) Init(-1)
+Follow the C++17 evaluation order requirements for assignment expressions,
+shift, member function calls, etc.
+
+Enum
+Name(strong_eval_order) Type(int)
+
+EnumValue
+Enum(strong_eval_order) String(none) Value(0)
+
+EnumValue
+Enum(strong_eval_order) String(some) Value(1)
+
+EnumValue
+Enum(strong_eval_order) String(all) Value(2)
+
 ftabstop=
 C ObjC C++ ObjC++ Joined RejectNegative UInteger
 -ftabstop=<number>	Distance between tab stops for column reporting.
diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index d77092b..8b93c61 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -5378,14 +5378,15 @@ add_candidates (tree fns, tree first_arg, const vec<tree, va_gc> *args,
 static int
 op_is_ordered (tree_code code)
 {
-  if (!flag_args_in_order)
-    return 0;
-
   switch (code)
     {
       // 5. b @= a
     case MODIFY_EXPR:
-      return -1;
+      return (flag_strong_eval_order > 1 ? -1 : 0);
+
+      // 6. a[b]
+    case ARRAY_REF:
+      return (flag_strong_eval_order > 1 ? 1 : 0);
 
       // 1. a.b
       // Not overloadable (yet).
@@ -5393,13 +5394,11 @@ op_is_ordered (tree_code code)
       // Only one argument.
       // 3. a->*b
     case MEMBER_REF:
-      // 6. a[b]
-    case ARRAY_REF:
       // 7. a << b
     case LSHIFT_EXPR:
       // 8. a >> b
     case RSHIFT_EXPR:
-      return 1;
+      return (flag_strong_eval_order ? 1 : 0);
 
     default:
       return 0;
@@ -7830,9 +7829,7 @@ build_over_call (struct z_candidate *cand, int flags, tsubst_flags_t complain)
 
   tree call = build_cxx_call (fn, nargs, argarray, complain|decltype_flag);
   if (call != error_mark_node
-      && !magic
-      && (flag_args_in_order > 1
-	  || (cand->flags & LOOKUP_LIST_INIT_CTOR)))
+      && cand->flags & LOOKUP_LIST_INIT_CTOR)
     {
       tree c = extract_call_expr (call);
       /* build_new_op_1 will clear this when appropriate.  */
diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c
index 1d81fb1..c04368f 100644
--- a/gcc/cp/cp-gimplify.c
+++ b/gcc/cp/cp-gimplify.c
@@ -780,11 +780,10 @@ cp_gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p)
 		ret = GS_ERROR;
 	    }
 	}
-      else if (flag_args_in_order == 1
+      else if (flag_strong_eval_order
 	       && !CALL_EXPR_OPERATOR_SYNTAX (*expr_p))
 	{
-	  /* If flag_args_in_order == 1, we don't force an order on all
-	     function arguments, but do evaluate the object argument first.  */
+	  /* If flag_strong_eval_order, evaluate the object argument first.  */
 	  tree fntype = TREE_TYPE (CALL_EXPR_FN (*expr_p));
 	  if (POINTER_TYPE_P (fntype))
 	    fntype = TREE_TYPE (fntype);
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index b6398ff..65a6885 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -2237,14 +2237,6 @@ option is used for the warning.
 Turn off all access checking.  This switch is mainly useful for working
 around bugs in the access control code.
 
-@item -fargs-in-order
-@opindex fargs-in-order
-Evaluate function arguments and operands of some binary expressions in
-left-to-right order, and evaluate the right side of an assignment
-before the left side, as proposed in P0145R2.  Enabled by default with
-@option{-std=c++1z}.  @option{-fargs-in-order=1} implements all of the
-ordering requirements except function arguments.
-
 @item -fcheck-new
 @opindex fcheck-new
 Check that the pointer returned by @code{operator new} is non-null
@@ -2483,6 +2475,15 @@ represented in the minimum number of bits needed to represent all the
 enumerators).  This assumption may not be valid if the program uses a
 cast to convert an arbitrary integer value to the enumerated type.
 
+@item -fstrong-eval-order
+@opindex fstrong-eval-order
+Evaluate member access, array subscripting, and shift expressions in
+left-to-right order, and evaluate assignment in right-to-left order,
+as adopted for C++17.  Enabled by default with @option{-std=c++1z}.
+@option{-fstrong-eval-order=some} enables just the ordering of member
+access and shift expressions, and is the default without
+@option{-std=c++1z}.
+
 @item -ftemplate-backtrace-limit=@var{n}
 @opindex ftemplate-backtrace-limit
 Set the maximum number of template instantiation notes for a single
diff --git a/gcc/testsuite/g++.dg/cpp1z/eval-order1.C b/gcc/testsuite/g++.dg/cpp1z/eval-order1.C
deleted file mode 100644
index 278990d..0000000
--- a/gcc/testsuite/g++.dg/cpp1z/eval-order1.C
+++ /dev/null
@@ -1,21 +0,0 @@
-// P0145R2: Refining Expression Order for C++
-// { dg-do run }
-// { dg-options "-std=c++1z" }
-
-extern "C" int printf (const char *, ...);
-void sink(...) { }
-
-int last = 0;
-int f(int i)
-{
-  if (i < last)
-    __builtin_abort ();
-  last = i;
-  return i;
-}
-
-int main()
-{
-  sink(f(1), f(2));
-  sink(f(3), f(4), f(5));
-}
diff --git a/gcc/testsuite/g++.dg/cpp1z/eval-order3.C b/gcc/testsuite/g++.dg/cpp1z/eval-order3.C
index 15df903..d382d07 100644
--- a/gcc/testsuite/g++.dg/cpp1z/eval-order3.C
+++ b/gcc/testsuite/g++.dg/cpp1z/eval-order3.C
@@ -1,6 +1,6 @@
 // P0145R2: Refining Expression Order for C++
 // { dg-do run }
-// { dg-options "-std=c++1z -fargs-in-order=1" }
+// { dg-options "-std=c++1z" }
 
 extern "C" int printf (const char *, ...);
 void sink(...) { }
diff mbox

Patch

diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c
index c04368f..8496d7c 100644
--- a/gcc/cp/cp-gimplify.c
+++ b/gcc/cp/cp-gimplify.c
@@ -559,6 +559,33 @@  simple_empty_class_p (tree type, tree op)
     && is_really_empty_class (type);
 }
 
+/* Returns true if evaluating E as an lvalue has side-effects;
+   specifically, a volatile lvalue has TREE_SIDE_EFFECTS, but it doesn't really
+   have side-effects until there is a read or write through it.  */
+
+static bool
+lvalue_has_side_effects (tree e)
+{
+  if (!TREE_SIDE_EFFECTS (e))
+    return false;
+  while (handled_component_p (e))
+    {
+      if (TREE_CODE (e) == ARRAY_REF
+	  && TREE_SIDE_EFFECTS (TREE_OPERAND (e, 1)))
+	return true;
+      e = TREE_OPERAND (e, 0);
+    }
+  if (DECL_P (e))
+    /* Just naming a variable has no side-effects.  */
+    return false;
+  else if (INDIRECT_REF_P (e))
+    /* Similarly, indirection has no side-effects.  */
+    return TREE_SIDE_EFFECTS (TREE_OPERAND (e, 0));
+  else
+    /* For anything else, trust TREE_SIDE_EFFECTS.  */
+    return TREE_SIDE_EFFECTS (e);
+}
+
 /* Do C++-specific gimplification.  Args are as for gimplify_expr.  */
 
 int
@@ -659,8 +686,6 @@  cp_gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p)
 	    /* Remove any copies of empty classes.  Also drop volatile
 	       variables on the RHS to avoid infinite recursion from
 	       gimplify_expr trying to load the value.  */
-	    gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p, post_p,
-			   is_gimple_lvalue, fb_lvalue);
 	    if (TREE_SIDE_EFFECTS (op1))
 	      {
 		if (TREE_THIS_VOLATILE (op1)
@@ -669,8 +694,29 @@  cp_gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p)
 
 		gimplify_and_add (op1, pre_p);
 	      }
+	    gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p, post_p,
+			   is_gimple_lvalue, fb_lvalue);
 	    *expr_p = TREE_OPERAND (*expr_p, 0);
 	  }
+	/* P0145 says that the RHS is sequenced before the LHS.
+	   gimplify_modify_expr gimplifies the RHS before the LHS, but that
+	   isn't quite strong enough in two cases:
+
+	   1) gimplify.c wants to leave a CALL_EXPR on the RHS, which would
+	   mean it's evaluated after the LHS.
+
+	   2) the value calculation of the RHS is also sequenced before the
+	   LHS, so for scalar assignment we need to preevaluate if the
+	   RHS could be affected by LHS side-effects even if it has no
+	   side-effects of its own.  We don't need this for classes because
+	   class assignment takes its RHS by reference.  */
+       else if (flag_strong_eval_order > 1
+                && TREE_CODE (*expr_p) == MODIFY_EXPR
+                && lvalue_has_side_effects (op0)
+		&& (TREE_CODE (op1) == CALL_EXPR
+		    || (SCALAR_TYPE_P (TREE_TYPE (op1))
+			&& !TREE_CONSTANT (op1))))
+	 TREE_OPERAND (*expr_p, 1) = get_formal_tmp_var (op1, pre_p);
       }
       ret = GS_OK;
       break;
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 65a6885..9544853 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -4080,6 +4080,13 @@  diagnosed by this option, and it may give an occasional false positive
 result, but in general it has been found fairly effective at detecting
 this sort of problem in programs.
 
+The C++17 standard will define the order of evaluation of operands in
+more cases: in particular it requires that the right-hand side of an
+assignment be evaluated before the left-hand side, so the above
+examples are no longer undefined.  But this warning will still warn
+about them, to help people avoid writing code that is undefined in C
+and earlier revisions of C++.
+
 The standard is worded confusingly, therefore there is some debate
 over the precise meaning of the sequence point rules in subtle cases.
 Links to discussions of the problem, including proposed formal
diff --git a/gcc/testsuite/g++.dg/cpp1z/eval-order3.C b/gcc/testsuite/g++.dg/cpp1z/eval-order3.C
index d382d07..e87dce4 100644
--- a/gcc/testsuite/g++.dg/cpp1z/eval-order3.C
+++ b/gcc/testsuite/g++.dg/cpp1z/eval-order3.C
@@ -31,6 +31,13 @@  struct A
   A& operator+=(int i) { return *this; }
 };
 
+struct B
+{
+  int _i;
+  B(): _i(0) {}
+  B(int i): _i(f(i)) { }
+};
+
 int operator>>(A&, int i) { }
 
 A a(0);
@@ -46,6 +53,18 @@  A& aref(int i)
   return a;
 }
 
+B b;
+B bval(int i)
+{
+  return B(i);
+}
+
+B& bref(int i)
+{
+  f(i);
+  return b;
+}
+
 static int si;
 int* ip (int i)
 {
@@ -84,10 +103,18 @@  template <class T> void f()
 
   // b @= a
   aref(19)=A(18);
-  //iref(21)=f(20);
+  iref(21)=f(20);
   aref(23)+=f(22);
+  bref(25)=bval(24);
+  (f(27), b) = bval(26);
   last = 0;
 
+  int ar[4] = {};
+  int i = 0;
+  ar[i++] = i;
+  if (ar[0] != 0)
+    __builtin_abort();
+
   // a[b]
   afn(20)[f(21)-21].memfn(f(22),23);
   ip(24)[f(25)-25] = 0;
@@ -123,10 +150,18 @@  void g()
 
   // b @= a
   aref(19)=A(18);
-  //iref(21)=f(20);
+  iref(21)=f(20);
   aref(23)+=f(22);
+  bref(25)=bval(24);
+  (f(27), b) = bval(26);
   last = 0;
 
+  int ar[4] = {};
+  int i = 0;
+  ar[i++] = i;
+  if (ar[0] != 0)
+    __builtin_abort();
+
   // a[b]
   afn(20)[f(21)-21].memfn(f(22),23);
   ip(24)[f(25)-25] = 0;