Bug 91987 - -fstrict-eval-order issues
Summary: -fstrict-eval-order issues
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 10.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on: 91974
Blocks:
  Show dependency treegraph
 
Reported: 2019-10-04 07:23 UTC by Jakub Jelinek
Modified: 2020-01-27 14:28 UTC (History)
6 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2020-01-27 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jakub Jelinek 2019-10-04 07:23:12 UTC
+++ This bug was initially created as a clone of Bug #91974 +++

I wonder if we don't need to double check all the C++17 evaluation rules for similar problems as the above bug for call expressions.

At least:
int
foo (void)
{
  int x = 5;
  return x << (x = 3, 2);
}

int
main ()
{
  if (foo () != (5 << 2))
    __builtin_abort ();
}

fails when compiled with g++ trunk and icpc 19, but succeeds with clang++.
The problem is similar, we gimplify_expr the left operand before right operand in this case and think it is enough, but because it is gimplified to a VAR_DECL which satisfies the predicate, yet is a user variable that can be overwritten in the expression that needs to be sequenced after it, we don't actually enforce the language rules.
Not sure what is the right way to deal with this.
Depend on flag_strong_eval_order (which is in c-family/c.opt!) in the gimplifier and use different predicates in that case that will enforce it (how to distinguish between fresh gimplifier temporaries that aren't problematic vs. variables that can be overwritten?  Is DECL_ARTIFICIAL usable for this, or can we have DECL_ARTIFICAL vars that can be overwritten?  Or wrap the operands in NON_LVALUE_EXPR or similar?
Comment 1 Jakub Jelinek 2019-10-04 08:37:04 UTC
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.c	2019-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).
Comment 2 Jakub Jelinek 2019-10-04 08:52:55 UTC
So for the shifts we'd need additionally:
--- gcc/fold-const.c.jj	2019-09-02 15:29:34.548515139 +0200
+++ gcc/fold-const.c	2019-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?
Comment 3 rguenther@suse.de 2019-10-04 09:06:37 UTC
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 <jakub at gcc dot gnu.org> ---
> So for the shifts we'd need additionally:
> --- gcc/fold-const.c.jj 2019-09-02 15:29:34.548515139 +0200
> +++ gcc/fold-const.c    2019-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?
Comment 4 Jakub Jelinek 2019-10-04 09:20:16 UTC
(In reply to rguenther@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.
Comment 5 rguenther@suse.de 2019-10-04 09:25:21 UTC
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 <jakub at gcc dot gnu.org> ---
> (In reply to rguenther@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
Comment 6 Jakub Jelinek 2019-10-04 09:40:12 UTC
(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}.
Comment 7 Jakub Jelinek 2019-10-04 09:43:44 UTC
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.
Comment 8 Jakub Jelinek 2019-10-05 07:53:40 UTC
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.
Comment 9 Jakub Jelinek 2019-10-05 08:24:33 UTC
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<E1>, *(SAVE_EXPR<E1>+E2).
Thoughts on this?
Comment 10 Jakub Jelinek 2019-10-11 07:36:38 UTC
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
Comment 11 Jakub Jelinek 2019-10-11 07:40:21 UTC
Partially fixed, the call argument issue is unresolved, and I guess some analysis about say .*/->* is needed too.