[C++ PATCH] Partial fix for further -fstrong-eval-order issues (PR c++/91987, take 2)
Jason Merrill
jason@redhat.com
Thu Oct 10 17:57:00 GMT 2019
On 10/10/19 4:13 AM, Jakub Jelinek wrote:
> On Mon, Oct 07, 2019 at 05:17:56PM -0400, Jason Merrill wrote:
>> Ah, I see. And it occurs to me this situation fails condition #1 for
>> get_formal_tmp_var anyway. So I guess the predicate direction doesn't make
>> sense. Let's factor out the above pattern differently, then. Maybe a
>> preevaluate function that takes a predicate as an argument?
>
> Ok, so this patch does, compared to the previous one:
> 1) shifts are now done in cp_build_binary_op with the cp_save_expr,
> so no fold-const.c or cp-gimplify.c change is needed
> 2) the array ref x[y] -> *(x + y) where y has side-effects is now handled
> as *(SAVE_EXPR<x>, SAVE_EXPR<x> + y) rather than
> SAVE_EXPR<x>, *(SAVE_EXPR<x> + y) where the former is friendlier to
> wrapping it in ADDR_EXPR etc.; nevertheless, I had to disable it for the
> OpenMP reductions, I'll discuss in the language committee what to do if
> anything in that case
> 3) I've added the new function in cp-gimplify.c and used it in the CALL_EXPR
> case
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> If wanted, can commit the 3 unrelated changes separately.
>
> And the call argument side-effects aren't handled yet.
>
> 2019-10-10 Jakub Jelinek <jakub@redhat.com>
>
> 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.
>
> --- gcc/cp/decl2.c.jj 2019-10-09 10:27:12.704400889 +0200
> +++ gcc/cp/decl2.c 2019-10-09 10:32:44.932416373 +0200
> @@ -405,6 +405,7 @@ grok_array_decl (location_t loc, tree ar
> else
> {
> tree p1, p2, i1, i2;
> + bool swapped = false;
>
> /* Otherwise, create an ARRAY_REF for a pointer or array type.
> It is a little-known fact that, if `a' is an array and `i' is
> @@ -431,7 +432,7 @@ grok_array_decl (location_t loc, tree ar
> if (p1 && i2)
> array_expr = p1, index_exp = i2;
> else if (i1 && p2)
> - array_expr = p2, index_exp = i1;
> + swapped = true, array_expr = p2, index_exp = i1;
> else
> {
> error_at (loc, "invalid types %<%T[%T]%> for array subscript",
> @@ -447,7 +448,12 @@ grok_array_decl (location_t loc, tree ar
> else
> array_expr = mark_lvalue_use_nonread (array_expr);
> index_exp = mark_rvalue_use (index_exp);
> - expr = build_array_ref (input_location, array_expr, index_exp);
> + if (swapped
> + && flag_strong_eval_order == 2
> + && (TREE_SIDE_EFFECTS (array_expr) || TREE_SIDE_EFFECTS (index_exp)))
> + expr = build_array_ref (input_location, index_exp, array_expr);
> + else
> + expr = build_array_ref (input_location, array_expr, index_exp);
> }
> if (processing_template_decl && expr != error_mark_node)
> {
> --- gcc/cp/typeck.c.jj 2019-10-09 10:27:12.625402077 +0200
> +++ gcc/cp/typeck.c 2019-10-09 11:21:09.058765959 +0200
> @@ -3550,6 +3550,10 @@ cp_build_array_ref (location_t loc, tree
> {
> tree ar = cp_default_conversion (array, complain);
> tree ind = cp_default_conversion (idx, complain);
> + tree first = NULL_TREE;
> +
> + if (flag_strong_eval_order == 2 && TREE_SIDE_EFFECTS (ind))
> + ar = first = cp_save_expr (ar);
>
> /* Put the integer in IND to simplify error checking. */
> if (TREE_CODE (TREE_TYPE (ar)) == INTEGER_TYPE)
> @@ -3573,11 +3577,10 @@ cp_build_array_ref (location_t loc, tree
>
> warn_array_subscript_with_type_char (loc, idx);
>
> - ret = cp_build_indirect_ref (cp_build_binary_op (input_location,
> - PLUS_EXPR, ar, ind,
> - complain),
> - RO_ARRAY_INDEXING,
> - complain);
> + ret = cp_build_binary_op (input_location, PLUS_EXPR, ar, ind, complain);
> + if (first)
> + ret = build2_loc (loc, COMPOUND_EXPR, TREE_TYPE (ret), first, ret);
> + ret = cp_build_indirect_ref (ret, RO_ARRAY_INDEXING, complain);
> protected_set_expr_location (ret, loc);
> if (non_lvalue)
> ret = non_lvalue_loc (loc, ret);
> @@ -5555,6 +5558,17 @@ cp_build_binary_op (const op_location_t
> if (build_type == NULL_TREE)
> build_type = result_type;
>
> + if (doing_shift
> + && flag_strong_eval_order == 2
> + && TREE_SIDE_EFFECTS (op1)
> + && !processing_template_decl)
> + {
> + /* In C++17, in both op0 << op1 and op0 >> op1 op0 is sequenced before
> + op1, so if op1 has side-effects, use SAVE_EXPR around op0. */
> + op0 = cp_save_expr (op0);
> + instrument_expr = op0;
> + }
> +
> if (sanitize_flags_p ((SANITIZE_SHIFT
> | SANITIZE_DIVIDE | SANITIZE_FLOAT_DIVIDE))
> && current_function_decl != NULL_TREE
> @@ -5566,6 +5580,7 @@ cp_build_binary_op (const op_location_t
> op1 = cp_save_expr (op1);
> op0 = fold_non_dependent_expr (op0, complain);
> op1 = fold_non_dependent_expr (op1, complain);
> + tree instrument_expr1 = NULL_TREE;
> if (doing_div_or_mod
> && sanitize_flags_p (SANITIZE_DIVIDE | SANITIZE_FLOAT_DIVIDE))
> {
> @@ -5578,10 +5593,15 @@ cp_build_binary_op (const op_location_t
> cop0 = cp_convert (orig_type, op0, complain);
> if (TREE_TYPE (cop1) != orig_type)
> cop1 = cp_convert (orig_type, op1, complain);
> - instrument_expr = ubsan_instrument_division (location, cop0, cop1);
> + instrument_expr1 = ubsan_instrument_division (location, cop0, cop1);
> }
> else if (doing_shift && sanitize_flags_p (SANITIZE_SHIFT))
> - instrument_expr = ubsan_instrument_shift (location, code, op0, op1);
> + instrument_expr1 = ubsan_instrument_shift (location, code, op0, op1);
> + if (instrument_expr != NULL)
> + instrument_expr = build2 (COMPOUND_EXPR, TREE_TYPE (instrument_expr1),
> + instrument_expr, instrument_expr1);
> + else
> + instrument_expr = instrument_expr1;
You could use add_stmt_to_compound here. OK either way.
Jason
More information about the Gcc-patches
mailing list