On Mon, Feb 13, 2017 at 05:44:54PM +0100, Jakub Jelinek wrote:
> On Mon, Feb 13, 2017 at 11:41:48AM -0500, Nathan Sidwell wrote:
> > On 02/13/2017 10:46 AM, Jakub Jelinek wrote:
> > > Hi!
> > >
> > > I'd like to ping a couple of patches:
> > >
> > > - C++ P1 PR79232 - ICEs and wrong-code with COMPOUND_EXPR on lhs of
> > > assignment
> > > http://gcc.gnu.org/ml/gcc-patches/2017-01/msg02341.html
> >
> > What puzzles me about (and may be an existing orthogonal issue), is the
> > checking for TYPE(rhs) == VOID. In the current code it's only explicitly
> > checked in the lhs == COND_EXPR case, which is strange. Why isn't it a
> > general constraint?
>
> I'll double check; copied this from the COND_EXPR case which had the same.
> I believe the reason is that this is (or ought to be) checked later on,
> but stabilitize_expr wouldn't work well if it is called with a void
> expression.
Ok, so tried
void bar (int);
int
foo (int x, int *y, int z, int w, int u)
{
y[++z] = bar (++u);
(x++, y[++z]) = bar (++u);
(w ? y[++z] : y[z++]) = bar (++u);
(x++, (w ? y[++z] : y[z++])) = bar (++u);
return x + z + u;
}
The reason for that VOID_TYPE_P stuff in COND_EXPR handling is not
that stabilize_expr doesn't like it, but to avoid warning twice.
If I replace all 3 VOID_TYPE_P tests (with the patch) in
cp_build_modify_expr with 0 && VOID_TYPE_P, the above testcase
errors 6 times instead of 4 times as it should.
For the case when lhs is a pre-inc/decrement or modify_expr, we don't have
this problem, rhs is used just one in cp_convert_for_assignment where this
error is diagnosed normally.
Thus, I'll bootstrap/regtest following simplified version of the patch
instead:
2017-02-13 Jakub Jelinek <ja...@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-31 22:36:33.762297835 +0100
+++ gcc/cp/typeck.c 2017-02-13 18:22:06.927396122 +0100
@@ -7571,16 +7571,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))
@@ -7588,20 +7598,41 @@ 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)
+ 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)
+ 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:
@@ -7629,7 +7660,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)))
{
@@ -7655,14 +7685,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;
}
@@ -7678,7 +7725,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. */;
@@ -7691,7 +7738,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
@@ -7706,7 +7753,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. */
@@ -7720,7 +7767,7 @@ cp_build_modify_expr (location_t loc, tr
complain);
if (result == NULL_TREE)
return error_mark_node;
- return result;
+ goto ret;
}
lhstype = olhstype;
}
@@ -7765,7 +7812,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);
@@ -7861,9 +7908,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)
@@ -7902,7 +7950,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,
@@ -7912,6 +7960,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-02-13 18:11:00.744134797
+0100
+++ gcc/testsuite/g++.dg/cpp1z/eval-order4.C 2017-02-13 18:11:00.744134797
+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-02-13 18:11:00.745134784
+0100
+++ gcc/testsuite/g++.dg/other/pr79232.C 2017-02-13 18:11:00.745134784
+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
