[C++ PATCH] Fix assignments with comma expression on lhs (PR c++/79232)

[Jakub Jelinek <jakub at redhat dot com>]

Hi!

cp_build_modify_expr has some special code for the case when the lhs
is pre{inc,dec}rement, assignment, ?: or min/max, but the comma expression
handling has been removed (so that -fstrong-eval-order is honored; can't
we keep the previous behavior for -fstrong-eval-order=none and/or some?),
which in turn means if there is COMPOUND_EXPR with one of the above
expressions in its right operand (or series of nested COMPOUND_EXPR where the
rightmost expression is one of those), we generate invalid generic and ICE
on it, and even if we don't, we generate code with wrong evaluation order.

The following patch addresses that by handling COMPOUND_EXPR as it does
right now if the right-most expression isn't one of those, and otherwise
ensures the right thing happens.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2017-01-30  Jakub Jelinek  <jakub@redhat.com>

	PR c++/79232
	* typeck.c (cp_build_modify_expr): Handle properly COMPOUND_EXPRs
	on lhs that have {PRE{DEC,INC}REMENT,MODIFY,MIN,MAX,COND}_EXPR
	in the rightmost operand.

	* g++.dg/cpp1z/eval-order4.C: New test.
	* g++.dg/other/pr79232.C: New test.

--- gcc/cp/typeck.c.jj	2017-01-30 09:31:43.076595640 +0100
+++ gcc/cp/typeck.c	2017-01-30 15:56:33.601002577 +0100
@@ -7568,16 +7568,26 @@ tree
 cp_build_modify_expr (location_t loc, tree lhs, enum tree_code modifycode,
 		      tree rhs, tsubst_flags_t complain)
 {
-  tree result;
+  tree result = NULL_TREE;
   tree newrhs = rhs;
   tree lhstype = TREE_TYPE (lhs);
+  tree olhs = lhs;
   tree olhstype = lhstype;
   bool plain_assign = (modifycode == NOP_EXPR);
+  bool compound_side_effects_p = false;
+  tree preeval = NULL_TREE;
 
   /* Avoid duplicate error messages from operands that had errors.  */
   if (error_operand_p (lhs) || error_operand_p (rhs))
     return error_mark_node;
 
+  while (TREE_CODE (lhs) == COMPOUND_EXPR)
+    {
+      if (TREE_SIDE_EFFECTS (TREE_OPERAND (lhs, 0)))
+	compound_side_effects_p = true;
+      lhs = TREE_OPERAND (lhs, 1);
+    }
+
   /* Handle control structure constructs used as "lvalues".  Note that we
      leave COMPOUND_EXPR on the LHS because it is sequenced after the RHS.  */
   switch (TREE_CODE (lhs))
@@ -7585,20 +7595,57 @@ cp_build_modify_expr (location_t loc, tr
       /* Handle --foo = 5; as these are valid constructs in C++.  */
     case PREDECREMENT_EXPR:
     case PREINCREMENT_EXPR:
+      if (compound_side_effects_p)
+	{
+	  if (VOID_TYPE_P (TREE_TYPE (rhs)))
+	    {
+	      if (complain & tf_error)
+		error ("void value not ignored as it ought to be");
+	      return error_mark_node;
+	    }
+	  newrhs = rhs = stabilize_expr (rhs, &preeval);
+	}
       if (TREE_SIDE_EFFECTS (TREE_OPERAND (lhs, 0)))
 	lhs = build2 (TREE_CODE (lhs), TREE_TYPE (lhs),
 		      cp_stabilize_reference (TREE_OPERAND (lhs, 0)),
 		      TREE_OPERAND (lhs, 1));
       lhs = build2 (COMPOUND_EXPR, lhstype, lhs, TREE_OPERAND (lhs, 0));
+    maybe_add_compound:
+      /* If we had (bar, --foo) = 5; or (bar, (baz, --foo)) = 5;
+	 and looked through the COMPOUND_EXPRs, readd them now around
+	 the resulting lhs.  */
+      if (TREE_CODE (olhs) == COMPOUND_EXPR)
+	{
+	  lhs = build2 (COMPOUND_EXPR, lhstype, TREE_OPERAND (olhs, 0), lhs);
+	  tree *ptr = &TREE_OPERAND (lhs, 1);
+	  for (olhs = TREE_OPERAND (olhs, 1);
+	       TREE_CODE (olhs) == COMPOUND_EXPR;
+	       olhs = TREE_OPERAND (olhs, 1))
+	    {
+	      *ptr = build2 (COMPOUND_EXPR, lhstype,
+			     TREE_OPERAND (olhs, 0), *ptr);
+	      ptr = &TREE_OPERAND (*ptr, 1);
+	    }
+	}
       break;
 
     case MODIFY_EXPR:
+      if (compound_side_effects_p)
+	{
+	  if (VOID_TYPE_P (TREE_TYPE (rhs)))
+	    {
+	      if (complain & tf_error)
+		error ("void value not ignored as it ought to be");
+	      return error_mark_node;
+	    }
+	  newrhs = rhs = stabilize_expr (rhs, &preeval);
+	}
       if (TREE_SIDE_EFFECTS (TREE_OPERAND (lhs, 0)))
 	lhs = build2 (TREE_CODE (lhs), TREE_TYPE (lhs),
 		      cp_stabilize_reference (TREE_OPERAND (lhs, 0)),
 		      TREE_OPERAND (lhs, 1));
       lhs = build2 (COMPOUND_EXPR, lhstype, lhs, TREE_OPERAND (lhs, 0));
-      break;
+      goto maybe_add_compound;
 
     case MIN_EXPR:
     case MAX_EXPR:
@@ -7626,7 +7673,6 @@ cp_build_modify_expr (location_t loc, tr
 	   except that the RHS goes through a save-expr
 	   so the code to compute it is only emitted once.  */
 	tree cond;
-	tree preeval = NULL_TREE;
 
 	if (VOID_TYPE_P (TREE_TYPE (rhs)))
 	  {
@@ -7652,14 +7698,31 @@ cp_build_modify_expr (location_t loc, tr
 
 	if (cond == error_mark_node)
 	  return cond;
+	/* If we had (e, (a ? b : c)) = d; or (e, (f, (a ? b : c))) = d;
+	   and looked through the COMPOUND_EXPRs, readd them now around
+	   the resulting cond before adding the preevaluated rhs.  */
+	if (TREE_CODE (olhs) == COMPOUND_EXPR)
+	  {
+	    cond = build2 (COMPOUND_EXPR, TREE_TYPE (cond),
+			   TREE_OPERAND (olhs, 0), cond);
+	    tree *ptr = &TREE_OPERAND (cond, 1);
+	    for (olhs = TREE_OPERAND (olhs, 1);
+		 TREE_CODE (olhs) == COMPOUND_EXPR;
+		 olhs = TREE_OPERAND (olhs, 1))
+	      {
+		*ptr = build2 (COMPOUND_EXPR, TREE_TYPE (cond),
+			       TREE_OPERAND (olhs, 0), *ptr);
+		ptr = &TREE_OPERAND (*ptr, 1);
+	      }
+	  }
 	/* Make sure the code to compute the rhs comes out
 	   before the split.  */
-	if (preeval)
-	  cond = build2 (COMPOUND_EXPR, TREE_TYPE (lhs), preeval, cond);
-	return cond;
+	result = cond;
+	goto ret;
       }
 
     default:
+      lhs = olhs;
       break;
     }
 
@@ -7675,7 +7738,7 @@ cp_build_modify_expr (location_t loc, tr
 	    rhs = convert (lhstype, rhs);
 	  result = build2 (INIT_EXPR, lhstype, lhs, rhs);
 	  TREE_SIDE_EFFECTS (result) = 1;
-	  return result;
+	  goto ret;
 	}
       else if (! MAYBE_CLASS_TYPE_P (lhstype))
 	/* Do the default thing.  */;
@@ -7688,7 +7751,7 @@ cp_build_modify_expr (location_t loc, tr
 	  release_tree_vector (rhs_vec);
 	  if (result == NULL_TREE)
 	    return error_mark_node;
-	  return result;
+	  goto ret;
 	}
     }
   else
@@ -7703,7 +7766,7 @@ cp_build_modify_expr (location_t loc, tr
 	    {
 	      result = objc_maybe_build_modify_expr (lhs, rhs);
 	      if (result)
-		return result;
+		goto ret;
 	    }
 
 	  /* `operator=' is not an inheritable operator.  */
@@ -7717,7 +7780,7 @@ cp_build_modify_expr (location_t loc, tr
 				     complain);
 	      if (result == NULL_TREE)
 		return error_mark_node;
-	      return result;
+	      goto ret;
 	    }
 	  lhstype = olhstype;
 	}
@@ -7762,7 +7825,7 @@ cp_build_modify_expr (location_t loc, tr
 	    {
 	      result = objc_maybe_build_modify_expr (lhs, newrhs);
 	      if (result)
-		return result;
+		goto ret;
 	    }
 	}
       gcc_assert (TREE_CODE (lhstype) != REFERENCE_TYPE);
@@ -7858,9 +7921,10 @@ cp_build_modify_expr (location_t loc, tr
 
       from_array = TREE_CODE (TREE_TYPE (newrhs)) == ARRAY_TYPE
 		   ? 1 + (modifycode != INIT_EXPR): 0;
-      return build_vec_init (lhs, NULL_TREE, newrhs,
-			     /*explicit_value_init_p=*/false,
-			     from_array, complain);
+      result = build_vec_init (lhs, NULL_TREE, newrhs,
+			       /*explicit_value_init_p=*/false,
+			       from_array, complain);
+      goto ret;
     }
 
   if (modifycode == INIT_EXPR)
@@ -7899,7 +7963,7 @@ cp_build_modify_expr (location_t loc, tr
       result = objc_generate_write_barrier (lhs, modifycode, newrhs);
 
       if (result)
-	return result;
+	goto ret;
     }
 
   result = build2 (modifycode == NOP_EXPR ? MODIFY_EXPR : INIT_EXPR,
@@ -7909,6 +7973,9 @@ cp_build_modify_expr (location_t loc, tr
   if (!plain_assign)
     TREE_NO_WARNING (result) = 1;
 
+ ret:
+  if (preeval)
+    result = build2 (COMPOUND_EXPR, TREE_TYPE (result), preeval, result);
   return result;
 }
 
--- gcc/testsuite/g++.dg/cpp1z/eval-order4.C.jj	2017-01-30 16:08:22.195641383 +0100
+++ gcc/testsuite/g++.dg/cpp1z/eval-order4.C	2017-01-30 16:08:09.000000000 +0100
@@ -0,0 +1,80 @@
+// PR c++/79232
+// { dg-do run }
+// { dg-options "-fstrong-eval-order" }
+
+int last = 0;
+
+int
+foo (int i)
+{
+  if (i != last + 1)
+    __builtin_abort ();
+  last = i;
+  return i;
+}
+
+char a, b;
+int c;
+
+char &
+bar (int i, int j)
+{
+  foo (i);
+  return j ? a : b;
+}
+
+int
+main ()
+{
+  (foo (2) ? bar (3, 0) : bar (3, 1)) = foo (1);
+  if (last != 3)
+    __builtin_abort ();
+  last = 0;
+  (foo (2), foo (3) ? bar (4, 0) : bar (4, 1)) = foo (1);
+  if (last != 4)
+    __builtin_abort ();
+  last = 0;
+  (foo (2), (foo (3) ? bar (4, 0) : bar (4, 1))) = foo (1);
+  if (last != 4)
+    __builtin_abort ();
+  last = 0;
+  (foo (2), foo (3), foo (4) ? bar (5, 0) : bar (5, 1)) = foo (1);
+  if (last != 5)
+    __builtin_abort ();
+  last = 0;
+  (foo (2), (foo (3), (foo (4) ? bar (5, 0) : bar (5, 1)))) = foo (1);
+  if (last != 5)
+    __builtin_abort ();
+  last = 0;
+  --c = foo (1);
+  if (c != 1)
+    __builtin_abort ();
+  last = 0;
+  (foo (2), --c) = foo (1);
+  if (last != 2 || c != 1)
+    __builtin_abort ();
+  last = 0;
+  (foo (2), foo (3), --c) = foo (1);
+  if (last != 3 || c != 1)
+    __builtin_abort ();
+  last = 0;
+  (foo (2), (foo (3), --c)) = foo (1);
+  if (last != 3 || c != 1)
+    __builtin_abort ();
+  last = 0;
+  bar (2, 0) = foo (1);
+  if (last != 2)
+    __builtin_abort ();
+  last = 0;
+  (foo (2), bar (3, 0)) = foo (1);
+  if (last != 3)
+    __builtin_abort ();
+  last = 0;
+  (foo (2), foo (3), bar (4, 0)) = foo (1);
+  if (last != 4)
+    __builtin_abort ();
+  last = 0;
+  (foo (2), (foo (3), bar (4, 0))) = foo (1);
+  if (last != 4)
+    __builtin_abort ();
+}
--- gcc/testsuite/g++.dg/other/pr79232.C.jj	2017-01-30 13:37:32.095090643 +0100
+++ gcc/testsuite/g++.dg/other/pr79232.C	2017-01-30 13:35:17.000000000 +0100
@@ -0,0 +1,12 @@
+// PR c++/79232
+// { dg-do compile }
+
+extern char a[];
+int b;
+char c, e;
+
+void
+foo (long d)
+{
+  (0, b ? &c : a)[d] = e;
+}

	Jakub