[Bug c++/91987] -fstrict-eval-order issues
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91987 Martin Liška changed: What|Removed |Added Status|UNCONFIRMED |NEW Last reconfirmed||2020-01-27 CC||marxin at gcc dot gnu.org Ever confirmed|0 |1
[Bug c++/91987] -fstrict-eval-order issues
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91987 --- Comment #11 from Jakub Jelinek --- Partially fixed, the call argument issue is unresolved, and I guess some analysis about say .*/->* is needed too.
[Bug c++/91987] -fstrict-eval-order issues
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91987 --- Comment #10 from Jakub Jelinek --- Author: jakub Date: Fri Oct 11 07:36:07 2019 New Revision: 276860 URL: https://gcc.gnu.org/viewcvs?rev=276860&root=gcc&view=rev Log: PR c++/91987 cp/ * decl2.c (grok_array_decl): For -fstrong-eval-order, when array ref operands have been swapped and at least one operand has side-effects, revert the swapping before calling build_array_ref. * typeck.c (cp_build_array_ref): For non-ARRAY_TYPE array ref with side-effects on the index operand, if -fstrong-eval-order use save_expr around the array operand. (cp_build_binary_op): For shifts with side-effects in the second operand, wrap first operand into SAVE_EXPR and evaluate it before the shift. * semantics.c (handle_omp_array_sections_1): Temporarily disable flag_strong_eval_order during OMP_CLAUSE_REDUCTION array section processing. * cp-gimplify.c (gimplify_to_rvalue): New function. (cp_gimplify_expr): Use it. testsuite/ * g++.dg/cpp1z/eval-order6.C: New test. * g++.dg/cpp1z/eval-order7.C: New test. * g++.dg/cpp1z/eval-order8.C: New test. * c-c++-common/gomp/pr91987.c: New test. Added: trunk/gcc/testsuite/c-c++-common/gomp/pr91987.c trunk/gcc/testsuite/g++.dg/cpp1z/eval-order6.C trunk/gcc/testsuite/g++.dg/cpp1z/eval-order7.C trunk/gcc/testsuite/g++.dg/cpp1z/eval-order8.C Modified: trunk/gcc/cp/ChangeLog trunk/gcc/cp/cp-gimplify.c trunk/gcc/cp/decl2.c trunk/gcc/cp/semantics.c trunk/gcc/cp/typeck.c trunk/gcc/testsuite/ChangeLog
[Bug c++/91987] -fstrict-eval-order issues
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91987
--- Comment #9 from Jakub Jelinek ---
Another testcase for the same reason as above:
int a[4] = { 1, 2, 3, 4 };
int c;
int
main ()
{
int *x = a;
c = 1;
int r = (c = 4, x)[(c *= 2, 3)];
if (c != 8 || r != 3)
__builtin_abort ();
c = 1;
r = (c = 3, 2)[(c *= 2, x)];
if (c != 6 || r != 2)
__builtin_abort ();
}
If we want to represent E1[E2] for non-array E1/E2 as *(E1+E2), we need to
force the evaluation of E1's side-effects, but not just those seen under
TREE_SIDE_EFFECTS, some other way. Perhaps SAVE_EXPR, *(SAVE_EXPR+E2).
Thoughts on this?
[Bug c++/91987] -fstrict-eval-order issues
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91987
--- Comment #8 from Jakub Jelinek ---
Another testcase, another reason:
int a[4] = { 1, 2, 3, 4 };
int b[4] = { 5, 6, 7, 8 };
int
foo (void)
{
int *x = a;
int r = x[(x = b, 3)];
if (x != b)
__builtin_abort ();
return r;
}
int
main ()
{
if (foo () != 3)
__builtin_abort ();
}
The problem here is that we somewhere fold the x[(x = b, 3)] which with
-fstrong-eval-order should have well defined ordering between x and (x = b, 3)
into
*(x + (x = b, 3)) which is undefined.
[Bug c++/91987] -fstrict-eval-order issues
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91987 --- Comment #7 from Jakub Jelinek --- So, if clang is right here, we'd need to force arguments not just with is_gimple_reg_type, but also with all other types that are not TREE_ADDRESSABLE, into temporaries (perhaps with a first check we really need to do that, as in whether the other argument(s) has/have side-effects.
[Bug c++/91987] -fstrict-eval-order issues
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91987
--- Comment #6 from Jakub Jelinek ---
(In reply to Jakub Jelinek from comment #4)
> Probably, but aggregate copy of TREE_ADDRESSABLE aggregates might be a
> problem.
> For the arguments, I'm not planning to do anything myself, because I don't
> understand it well, just wanted to raise possible issue.
For this I meant something like:
struct S { S (); ~S (); S (const S &); int s; S operator<< (const S &); };
void
foo ()
{
S a, b, c;
c = a << (a.s = 5, b);
}
which according to godbolt all of gcc, clang and icpc handle the same in
-std=c++17 mode, the first argument (this) to the method points to a that has
been modified, then
struct S { S (); ~S (); S (const S &); int s; };
S operator<< (const S &, const S &);
void
foo ()
{
S a, b, c;
c = a << (a.s = 5, b);
}
(likewise) and finally
struct S { int s; };
S operator<< (S, S);
void
foo ()
{
S a, b, c;
a.s = 1;
b.s = 2;
c.s = 3;
c = a << (a.s = 5, b);
}
This one according to godbolt is handled differently, gcc and icpc will call
the operator with S{5}, S{2}, while clang with S{1}, S{2}.
[Bug c++/91987] -fstrict-eval-order issues
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91987
--- Comment #5 from rguenther at suse dot de ---
On Fri, 4 Oct 2019, jakub at gcc dot gnu.org wrote:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91987
>
> --- Comment #4 from Jakub Jelinek ---
> (In reply to rguent...@suse.de from comment #3)
> > Ick. I'd say we should unconditionally guard the transform
> > with the appropriate TREE_SIDE_EFFECTS check?
>
> See above, wouldn't that mean throwing the optimization away completely,
> because COMPOUND_EXPRs with no side-effects in the rhs1 should be just
> optimized into rhs2?
> Do you mean unconditionally for all binary/comparison ops, or just the
> shifts/rotates?
> For languages where it is UB if the side-effects modify the other operands, I
> don't see
> anything wrong with continuing with the optimization.
Well, AFAICS this "optimization" is purely for frontend folding
since in the middle-end we have expressions not "obfuscated" with
COMPOUND_EXPRs. So I wouldn't mind dropping it completely either...
> The other C++17 rules are summarized in
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91415#c5
> (though that PR is also unfinished).
>
> > > One thing I'm worried about are the special cases where we enforce some
> > > argument order, evaluate one argument before the other or vice versa.
> > > For is_gimple_reg_type args it can be just a matter of forcing it into an
> > > SSA_NAME or a new VAR_DECL, but if a function argument is a structure,
> > > struct S { int a, b; } c = { 1, 2 };
> > > fun (c, (c.b = 3, 5);
> > > and fun is one of the magic one that enforce operand ordering and the
> > > first
> > > argument needs to be sequenced before the second, what can we do?
> >
> > You need to emit an aggregate copy, don't you? Consider
>
> Probably, but aggregate copy of TREE_ADDRESSABLE aggregates might be a
> problem.
> For the arguments, I'm not planning to do anything myself, because I don't
> understand it well, just wanted to raise possible issue.
Likewise. I don't even _want_ to understand :P
[Bug c++/91987] -fstrict-eval-order issues
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91987
--- Comment #4 from Jakub Jelinek ---
(In reply to rguent...@suse.de from comment #3)
> Ick. I'd say we should unconditionally guard the transform
> with the appropriate TREE_SIDE_EFFECTS check?
See above, wouldn't that mean throwing the optimization away completely,
because COMPOUND_EXPRs with no side-effects in the rhs1 should be just
optimized into rhs2?
Do you mean unconditionally for all binary/comparison ops, or just the
shifts/rotates?
For languages where it is UB if the side-effects modify the other operands, I
don't see
anything wrong with continuing with the optimization.
The other C++17 rules are summarized in
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91415#c5
(though that PR is also unfinished).
> > One thing I'm worried about are the special cases where we enforce some
> > argument order, evaluate one argument before the other or vice versa.
> > For is_gimple_reg_type args it can be just a matter of forcing it into an
> > SSA_NAME or a new VAR_DECL, but if a function argument is a structure,
> > struct S { int a, b; } c = { 1, 2 };
> > fun (c, (c.b = 3, 5);
> > and fun is one of the magic one that enforce operand ordering and the first
> > argument needs to be sequenced before the second, what can we do?
>
> You need to emit an aggregate copy, don't you? Consider
Probably, but aggregate copy of TREE_ADDRESSABLE aggregates might be a problem.
For the arguments, I'm not planning to do anything myself, because I don't
understand it well, just wanted to raise possible issue.
[Bug c++/91987] -fstrict-eval-order issues
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91987
--- Comment #3 from rguenther at suse dot de ---
On Fri, 4 Oct 2019, jakub at gcc dot gnu.org wrote:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91987
>
> --- Comment #2 from Jakub Jelinek ---
> So for the shifts we'd need additionally:
> --- gcc/fold-const.c.jj 2019-09-02 15:29:34.548515139 +0200
> +++ gcc/fold-const.c2019-10-04 10:44:23.319883187 +0200
> @@ -9447,16 +9447,23 @@ fold_binary_loc (location_t loc, enum tr
>if (TREE_CODE (arg0) == COMPOUND_EXPR)
> {
> tem = fold_build2_loc (loc, code, type,
> -fold_convert_loc (loc, TREE_TYPE (op0),
> - TREE_OPERAND (arg0, 1)), op1);
> +fold_convert_loc (loc, TREE_TYPE (op0),
> + TREE_OPERAND (arg0, 1)),
> + op1);
> return build2_loc (loc, COMPOUND_EXPR, type, TREE_OPERAND (arg0, 0),
> tem);
> }
> - if (TREE_CODE (arg1) == COMPOUND_EXPR)
> + if (TREE_CODE (arg1) == COMPOUND_EXPR
> + && (flag_strong_eval_order != 2
> + /* C++17 disallows this canonicalization for shifts. */
> + || (code != LSHIFT_EXPR
> + && code != RSHIFT_EXPR
> + && code != LROTATE_EXPR
> + && code != RROTATE_EXPR)))
> {
> tem = fold_build2_loc (loc, code, type, op0,
> -fold_convert_loc (loc, TREE_TYPE (op1),
> - TREE_OPERAND (arg1, 1)));
> +fold_convert_loc (loc, TREE_TYPE (op1),
> + TREE_OPERAND (arg1, 1)));
> return build2_loc (loc, COMPOUND_EXPR, type, TREE_OPERAND (arg1, 0),
> tem);
> }
Ick. I'd say we should unconditionally guard the transform
with the appropriate TREE_SIDE_EFFECTS check?
> One thing I'm worried about are the special cases where we enforce some
> argument order, evaluate one argument before the other or vice versa.
> For is_gimple_reg_type args it can be just a matter of forcing it into an
> SSA_NAME or a new VAR_DECL, but if a function argument is a structure,
> struct S { int a, b; } c = { 1, 2 };
> fun (c, (c.b = 3, 5);
> and fun is one of the magic one that enforce operand ordering and the first
> argument needs to be sequenced before the second, what can we do?
You need to emit an aggregate copy, don't you? Consider
int i;
void fun (int i, int j)
{
assert (i == 0 && j == 1);
}
and
i = 0;
fun (i, (i = 1, 5));
so you cannot gimplify to the post-call sequence. For the
aggregate case it might be
fun (S &a, S &b)
so handling of
fun (c, c = {})
needs a temporary anyways?
[Bug c++/91987] -fstrict-eval-order issues
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91987
--- Comment #2 from Jakub Jelinek ---
So for the shifts we'd need additionally:
--- gcc/fold-const.c.jj 2019-09-02 15:29:34.548515139 +0200
+++ gcc/fold-const.c2019-10-04 10:44:23.319883187 +0200
@@ -9447,16 +9447,23 @@ fold_binary_loc (location_t loc, enum tr
if (TREE_CODE (arg0) == COMPOUND_EXPR)
{
tem = fold_build2_loc (loc, code, type,
-fold_convert_loc (loc, TREE_TYPE (op0),
- TREE_OPERAND (arg0, 1)), op1);
+fold_convert_loc (loc, TREE_TYPE (op0),
+ TREE_OPERAND (arg0, 1)),
+ op1);
return build2_loc (loc, COMPOUND_EXPR, type, TREE_OPERAND (arg0, 0),
tem);
}
- if (TREE_CODE (arg1) == COMPOUND_EXPR)
+ if (TREE_CODE (arg1) == COMPOUND_EXPR
+ && (flag_strong_eval_order != 2
+ /* C++17 disallows this canonicalization for shifts. */
+ || (code != LSHIFT_EXPR
+ && code != RSHIFT_EXPR
+ && code != LROTATE_EXPR
+ && code != RROTATE_EXPR)))
{
tem = fold_build2_loc (loc, code, type, op0,
-fold_convert_loc (loc, TREE_TYPE (op1),
- TREE_OPERAND (arg1, 1)));
+fold_convert_loc (loc, TREE_TYPE (op1),
+ TREE_OPERAND (arg1, 1)));
return build2_loc (loc, COMPOUND_EXPR, type, TREE_OPERAND (arg1, 0),
tem);
}
One thing I'm worried about are the special cases where we enforce some
argument order, evaluate one argument before the other or vice versa.
For is_gimple_reg_type args it can be just a matter of forcing it into an
SSA_NAME or a new VAR_DECL, but if a function argument is a structure,
struct S { int a, b; } c = { 1, 2 };
fun (c, (c.b = 3, 5);
and fun is one of the magic one that enforce operand ordering and the first
argument needs to be sequenced before the second, what can we do?
[Bug c++/91987] -fstrict-eval-order issues
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91987
--- Comment #1 from Jakub Jelinek ---
To sum up IRC discussion with richi, he doesn't want this to be in the
gimplifier, as it is one FE specific, which means cp-gimplify.c is where this
needs to be done.
Furthermore, if we there have a predicate which for flag_strong_eval_order == 2
will fail for user decls (note, I still don't know how to safely recognize the
"safe" gimple temporaries), we could force stuff into temporaries too often,
e.g. for this case, we only need to do it if the other operand that is to be
sequenced after has side-effects.
So, I thought something along the lines of:
--- gcc/cp/cp-gimplify.c.jj 2019-10-04 08:53:01.737740712 +0200
+++ gcc/cp/cp-gimplify.c2019-10-04 10:15:07.629709617 +0200
@@ -884,6 +884,29 @@ cp_gimplify_expr (tree *expr_p, gimple_s
}
break;
+case LSHIFT_EXPR:
+case RSHIFT_EXPR:
+case LROTATE_EXPR:
+case RROTATE_EXPR:
+ /* If the second operand has side-effects, ensure the first operand
+is evaluated first and is not a decl that the side-effects of the
+second operand could modify. */
+ if (flag_strong_eval_order == 2
+ && TREE_SIDE_EFFECTS (TREE_OPERAND (*expr_p, 1)))
+ {
+ enum gimplify_status t
+ = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p, post_p,
+is_gimple_val, fb_rvalue);
+ if (t == GS_ERROR)
+ break;
+ else if (is_gimple_variable (TREE_OPERAND (*expr_p, 0))
+ && TREE_CODE (TREE_OPERAND (*expr_p, 0)) != SSA_NAME)
+ TREE_OPERAND (*expr_p, 0)
+ = get_initialized_tmp_var (TREE_OPERAND (*expr_p, 0), pre_p,
+NULL);
+ }
+ goto do_default;
+
case RETURN_EXPR:
if (TREE_OPERAND (*expr_p, 0)
&& (TREE_CODE (TREE_OPERAND (*expr_p, 0)) == INIT_EXPR
@@ -897,6 +920,7 @@ cp_gimplify_expr (tree *expr_p, gimple_s
}
/* Fall through. */
+do_default:
default:
ret = (enum gimplify_status) c_gimplify_expr (expr_p, pre_p, post_p);
break;
and similarly for the other occurrences of E? is sequenced before E? in the
standard.
Unfortunately, on the above testcase this doesn't work, because it is already
broken earlier, fold_binary_loc (invoked during cp_fold) has:
if (TREE_CODE_CLASS (code) == tcc_binary
|| TREE_CODE_CLASS (code) == tcc_comparison)
{
if (TREE_CODE (arg0) == COMPOUND_EXPR)
{
tem = fold_build2_loc (loc, code, type,
fold_convert_loc (loc, TREE_TYPE (op0),
TREE_OPERAND (arg0, 1)), op1);
return build2_loc (loc, COMPOUND_EXPR, type, TREE_OPERAND (arg0, 0),
tem);
}
if (TREE_CODE (arg1) == COMPOUND_EXPR)
{
tem = fold_build2_loc (loc, code, type, op0,
fold_convert_loc (loc, TREE_TYPE (op1),
TREE_OPERAND (arg1, 1)));
return build2_loc (loc, COMPOUND_EXPR, type, TREE_OPERAND (arg1, 0),
tem);
}
where the last if is invalid for the shifts (and rotates?) in
flag_strict_eval_order == 2 (at least if TREE_SIDE_EFFECTS (TREE_OPERAND (arg1,
1)), but for a COMPOUND_EXPR it would likely be optimized into just the last
operand if the first one doesn't have side-effects).
