public inbox for gcc-bugs@sourceware.org
help / color / mirror / Atom feed
* [Bug c++/70796] [DR 1030] Initialization order with braced-init-lists still broken
       [not found] <bug-70796-4@http.gcc.gnu.org/bugzilla/>
@ 2021-11-03 13:30 ` matthijsvanduin at gmail dot com
  2021-11-03 13:46 ` jason at gcc dot gnu.org
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 10+ messages in thread
From: matthijsvanduin at gmail dot com @ 2021-11-03 13:30 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70796

Matthijs van Duin <matthijsvanduin at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |matthijsvanduin at gmail dot com

--- Comment #3 from Matthijs van Duin <matthijsvanduin at gmail dot com> ---
This specifically appears to happen when the constructor has parameters of
trivially copyable non-reference types, e.g. this fails:

#include <assert.h>
struct IntWrap {
        int x = 0;
        IntWrap &operator ++() { ++x; return *this; }
};
struct Pair {
        IntWrap first, second;
        Pair( IntWrap x, IntWrap y ) : first{ x }, second{ y } { }
};
int main() {
        IntWrap i;
        Pair p{ ++i, ++i };
        assert( p.first.x == 1 && p.second.x == 2 );  // FAIL (p.first.x is 2)
}

but adding a destructor to IntWrap suffices to make it pass.


Interestingly, when using simple ints there also appear to be very narrow
constraints on the initializer arguments to trigger the bug:

#include <assert.h>
struct IntPair {
        int first, second;
        IntPair( int x, int y ) : first{ x }, second{ y } { }
};
void testcase_fail() {
        int i = 0;
        IntPair p{ ++i, ++i };
        assert( p.first == 1 && p.second == 2 );  // FAIL (p.first is 2)
}
void testcase_ok_1() {
        int i = 0;
        IntPair p{ ++i, ++i };
        assert( p.first == 1 && p.second == 2 );  // ok
        int &j = i;
        IntPair q{ ++j, ++j };
        assert( q.first == 3 && q.second == 4 );  // ok
}
void testcase_ok_2() {
        int i = 0;
        IntPair p{ (int &)++i, (int &)++i };
        assert( p.first == 1 && p.second == 2 );  // ok
}
int main() {
        testcase_ok_1();
        testcase_ok_2();
        testcase_fail();
}

even though the analogous testcases for IntWrap all fail.


Related:
bug 51253  (was supposed to have fixed this but evidently missed some cases)
bug 65866  (incorrect -Wsequence-point diagnostic still being emitted)
bug 70792  (dup of bug 65866 but discussion in comments covered this case)

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Bug c++/70796] [DR 1030] Initialization order with braced-init-lists still broken
       [not found] <bug-70796-4@http.gcc.gnu.org/bugzilla/>
  2021-11-03 13:30 ` [Bug c++/70796] [DR 1030] Initialization order with braced-init-lists still broken matthijsvanduin at gmail dot com
@ 2021-11-03 13:46 ` jason at gcc dot gnu.org
  2021-11-03 18:56 ` jakub at gcc dot gnu.org
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 10+ messages in thread
From: jason at gcc dot gnu.org @ 2021-11-03 13:46 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70796

Jason Merrill <jason at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW
                 CC|                            |jason at gcc dot gnu.org
   Last reconfirmed|                            |2021-11-03

--- Comment #4 from Jason Merrill <jason at gcc dot gnu.org> ---
The CALL_EXPR_ORDERED_ARGS code is supposed to make this work properly.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Bug c++/70796] [DR 1030] Initialization order with braced-init-lists still broken
       [not found] <bug-70796-4@http.gcc.gnu.org/bugzilla/>
  2021-11-03 13:30 ` [Bug c++/70796] [DR 1030] Initialization order with braced-init-lists still broken matthijsvanduin at gmail dot com
  2021-11-03 13:46 ` jason at gcc dot gnu.org
@ 2021-11-03 18:56 ` jakub at gcc dot gnu.org
  2021-11-03 19:42 ` jakub at gcc dot gnu.org
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 10+ messages in thread
From: jakub at gcc dot gnu.org @ 2021-11-03 18:56 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70796

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
#c3 in a single testcase without headers:
struct A
{
  int x = 0;
  A & operator ++ () { ++x; return *this; }
};
struct B
{
  A first, second;
  B (A x, A y) : first{x}, second{y} {}
};
struct C
{
  int first, second;
  C (int x, int y) : first{x}, second{y} {}
};

void
foo ()
{
  int i = 0;
  C p{++i, ++i};
  if (p.first != 1 || p.second != 2)
    __builtin_abort ();
}

void
bar ()
{
  int i = 0;
  C p{++i, ++i};
  if (p.first != 1 || p.second != 2)
    __builtin_abort ();
  int &j = i;
  C q{++j, ++j};
  if (q.first != 3 || q.second != 4)
    __builtin_abort ();
}

void
baz ()
{
  int i = 0;
  C p{(int &) ++i, (int &) ++i};
  if (p.first != 1 || p.second != 2)
    __builtin_abort ();
}

void
qux ()
{
  A i;
  B p{++i, ++i};
  if (p.first.x != 1 || p.second.x != 2)
    __builtin_abort ();
}

int
main ()
{
  bar ();
  baz ();
  foo ();
  qux ();
}

Passes with clang++.
Looking at the CALL, e.g. on C::C (&p,  ++i,  ++i) call CALL_EXPR_ORDERED_ARGS
is set and neither CALL_EXPR_REVERSE_ARGS nor CALL_EXPR_OPERATOR_SYNTAX is set.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Bug c++/70796] [DR 1030] Initialization order with braced-init-lists still broken
       [not found] <bug-70796-4@http.gcc.gnu.org/bugzilla/>
                   ` (2 preceding siblings ...)
  2021-11-03 18:56 ` jakub at gcc dot gnu.org
@ 2021-11-03 19:42 ` jakub at gcc dot gnu.org
  2021-11-19  9:09 ` cvs-commit at gcc dot gnu.org
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 10+ messages in thread
From: jakub at gcc dot gnu.org @ 2021-11-03 19:42 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70796

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
cp_gimplify_expr uses for CALL_EXPR_REVERSE_ARGS and CALL_EXPR_ORDERED_ARGS
(and for !CALL_EXPR_OPERATOR_SYNTAX method calls) gimplify_arg, but that
clearly isn't enough once there are any TREE_SIDE_EFFECTS on any of the
arguments.
Because e.g. for the ++i, ++i arguments for scalar non-addressable VAR_DECL i,
we end up with:
      i = i + 1;
      i = i + 1;
      C::C (&p, i, i);
which is wrong, is_gimple_val is true on i, but we really need to gimplify the
argument using gimplify_to_rvalue with is_gimple_val test if is_gimple_reg_type
(TREE_TYPE (arg)) and any of the args that are supposed to be evaluated after
it has TREE_SIDE_EFFECTS set.
That seems an easy change and would be (except for -O0) even cheap.
This would fix foo in the #c5 testcase.
bar works because i is addressable and thus gimplification already forces it
into  SSA_NAME or temporary.
baz works because the (int &) casts also force
C::C (&p, *( ++i;, (int &) &i;), *( ++i;, (int &) &i;))
before gimplification, so it is again gimplified into SSA_NAMEs or temporaries.
But, qux needs some other fix.
B::B (&p, TARGET_EXPR <D.2274, *(const struct A &) A::operator++ (&i)>,
TARGET_EXPR <D.2275, *(const struct A &) A::operator++ (&i)>)
The problem is that gimplify_arg optimizes:
  /* In general, we allow lvalues for function arguments to avoid
     extra overhead of copying large aggregates out of even larger
     aggregates into temporaries only to copy the temporaries to
     the argument list.  Make optimizers happy by pulling out to
     temporaries those types that fit in registers.  */
  if (is_gimple_reg_type (TREE_TYPE (*arg_p)))
    test = is_gimple_val, fb = fb_rvalue;
  else
    {
      test = is_gimple_lvalue, fb = fb_either;
      /* Also strip a TARGET_EXPR that would force an extra copy.  */
      if (TREE_CODE (*arg_p) == TARGET_EXPR)
        {
          tree init = TARGET_EXPR_INITIAL (*arg_p);
          if (init
              && !VOID_TYPE_P (TREE_TYPE (init)))
            *arg_p = init;
        }
    }
where the stripping of the TARGET_EXPR avoids the copy that is needed.
So perhaps add some ordered argument to gimplify_arg, in calls from gimplify.c
pass false, and in calls from cp_gimplify_expr pass true if the current
argument in the desired evaluation order is followed by any TREE_SIDE_EFFECTS
arguments.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Bug c++/70796] [DR 1030] Initialization order with braced-init-lists still broken
       [not found] <bug-70796-4@http.gcc.gnu.org/bugzilla/>
                   ` (3 preceding siblings ...)
  2021-11-03 19:42 ` jakub at gcc dot gnu.org
@ 2021-11-19  9:09 ` cvs-commit at gcc dot gnu.org
  2021-11-19  9:13 ` jakub at gcc dot gnu.org
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 10+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2021-11-19  9:09 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70796

--- Comment #7 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:a84177aff7ca86f501d6aa5ef407fac5e71f56fb

commit r12-5397-ga84177aff7ca86f501d6aa5ef407fac5e71f56fb
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Fri Nov 19 10:05:01 2021 +0100

    c++: Fix up -fstrong-eval-order handling of call arguments [PR70796]

    For -fstrong-eval-order (default for C++17 and later) we make sure to
    gimplify arguments in the right order, but as the following testcase
    shows that is not enough.
    The problem is that some lvalues can satisfy the is_gimple_val / fb_rvalue
    predicate used by gimplify_arg for is_gimple_reg_type typed expressions,
    or is_gimple_lvalue / fb_either used for other types.
    E.g. in foo we have:
      C::C (&p,  ++i,  ++i)
    before gimplification where i is an automatic int variable and without this
    patch gimplify that as:
      i = i + 1;
      i = i + 1;
      C::C (&p, i, i);
    which means that the ctor is called with the original i value incremented
    by 2 in both arguments, while because the call is CALL_EXPR_ORDERED_ARGS
    the first argument should be different.  Similarly in qux we have:
      B::B (&p, TARGET_EXPR <D.2274, *(const struct A &) A::operator++ (&i)>,
            TARGET_EXPR <D.2275, *(const struct A &) A::operator++ (&i)>)
    and gimplify it as:
          _1 = A::operator++ (&i);
          _2 = A::operator++ (&i);
          B::B (&p, MEM[(const struct A &)_1], MEM[(const struct A &)_2]);
    but because A::operator++ returns the passed in argument, again we have
    the same value in both cases due to gimplify_arg doing:
          /* Also strip a TARGET_EXPR that would force an extra copy.  */
          if (TREE_CODE (*arg_p) == TARGET_EXPR)
            {
              tree init = TARGET_EXPR_INITIAL (*arg_p);
              if (init
                  && !VOID_TYPE_P (TREE_TYPE (init)))
                *arg_p = init;
            }
    which is perfectly fine optimization for calls with unordered arguments,
    but breaks the ordered ones.
    Lastly, in corge, we have before gimplification:
      D::foo (NON_LVALUE_EXPR <p>, 3,  ++p)
    and gimplify it as
      p = p + 4;
      D::foo (p, 3, p);
    which is again wrong, because the this argument isn't before the
    side-effects but after it.
    The following patch adds cp_gimplify_arg wrapper, which if ordered
    and is_gimple_reg_type forces non-SSA_NAME is_gimple_variable
    result into a temporary, and if ordered, not is_gimple_reg_type
    and argument is TARGET_EXPR bypasses the gimplify_arg optimization.
    So, in foo with this patch we gimplify it as:
      i = i + 1;
      i.0_1 = i;
      i = i + 1;
      C::C (&p, i.0_1, i);
    in qux as:
          _1 = A::operator++ (&i);
          D.2312 = MEM[(const struct A &)_1];
          _2 = A::operator++ (&i);
          B::B (&p, D.2312, MEM[(const struct A &)_2]);
    where D.2312 is a temporary and in corge as:
      p.9_1 = p;
      p = p + 4;
      D::foo (p.9_1, 3, p);
    The is_gimple_reg_type forcing into a temporary should be really cheap
    (I think even at -O0 it should be optimized if there is no modification in
    between), the aggregate copies might be more expensive but I think e.g. SRA
    or FRE should be able to deal with those if there are no intervening
    changes.  But still, the patch tries to avoid those when it is cheaply
    provable that nothing bad happens (if no argument following it in the
    strong evaluation order doesn't have TREE_SIDE_EFFECTS, then even VAR_DECLs
    etc. shouldn't be modified after it).  There is also an optimization to
    avoid doing that for this or for arguments with reference types as nothing
    can modify the parameter values during evaluation of other argument's
    side-effects.

    I've tried if e.g.
      int i = 1;
      return i << ++i;
    doesn't suffer from this problem as well, but it doesn't, the FE uses
      SAVE_EXPR <i>, SAVE_EXPR <i> << ++i;
    in that case which gimplifies the way we want (temporary in the first
    operand).

    2021-11-19  Jakub Jelinek  <jakub@redhat.com>

            PR c++/70796
            * cp-gimplify.c (cp_gimplify_arg): New function.
            (cp_gimplify_expr): Use cp_gimplify_arg instead of gimplify_arg,
            pass true as last argument to it if there are any following
            arguments in strong evaluation order with side-effects.

            * g++.dg/cpp1z/eval-order11.C: New test.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Bug c++/70796] [DR 1030] Initialization order with braced-init-lists still broken
       [not found] <bug-70796-4@http.gcc.gnu.org/bugzilla/>
                   ` (4 preceding siblings ...)
  2021-11-19  9:09 ` cvs-commit at gcc dot gnu.org
@ 2021-11-19  9:13 ` jakub at gcc dot gnu.org
  2021-11-29  8:50 ` cvs-commit at gcc dot gnu.org
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 10+ messages in thread
From: jakub at gcc dot gnu.org @ 2021-11-19  9:13 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70796

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |jakub at gcc dot gnu.org

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed on the trunk so far.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Bug c++/70796] [DR 1030] Initialization order with braced-init-lists still broken
       [not found] <bug-70796-4@http.gcc.gnu.org/bugzilla/>
                   ` (5 preceding siblings ...)
  2021-11-19  9:13 ` jakub at gcc dot gnu.org
@ 2021-11-29  8:50 ` cvs-commit at gcc dot gnu.org
  2022-05-10  8:21 ` cvs-commit at gcc dot gnu.org
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 10+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2021-11-29  8:50 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70796

--- Comment #9 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-11 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:98cbc9b6ae37fd8b1b1bea1d15f0e427e8f36daa

commit r11-9335-g98cbc9b6ae37fd8b1b1bea1d15f0e427e8f36daa
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Fri Nov 19 10:05:01 2021 +0100

    c++: Fix up -fstrong-eval-order handling of call arguments [PR70796]

    For -fstrong-eval-order (default for C++17 and later) we make sure to
    gimplify arguments in the right order, but as the following testcase
    shows that is not enough.
    The problem is that some lvalues can satisfy the is_gimple_val / fb_rvalue
    predicate used by gimplify_arg for is_gimple_reg_type typed expressions,
    or is_gimple_lvalue / fb_either used for other types.
    E.g. in foo we have:
      C::C (&p,  ++i,  ++i)
    before gimplification where i is an automatic int variable and without this
    patch gimplify that as:
      i = i + 1;
      i = i + 1;
      C::C (&p, i, i);
    which means that the ctor is called with the original i value incremented
    by 2 in both arguments, while because the call is CALL_EXPR_ORDERED_ARGS
    the first argument should be different.  Similarly in qux we have:
      B::B (&p, TARGET_EXPR <D.2274, *(const struct A &) A::operator++ (&i)>,
            TARGET_EXPR <D.2275, *(const struct A &) A::operator++ (&i)>)
    and gimplify it as:
          _1 = A::operator++ (&i);
          _2 = A::operator++ (&i);
          B::B (&p, MEM[(const struct A &)_1], MEM[(const struct A &)_2]);
    but because A::operator++ returns the passed in argument, again we have
    the same value in both cases due to gimplify_arg doing:
          /* Also strip a TARGET_EXPR that would force an extra copy.  */
          if (TREE_CODE (*arg_p) == TARGET_EXPR)
            {
              tree init = TARGET_EXPR_INITIAL (*arg_p);
              if (init
                  && !VOID_TYPE_P (TREE_TYPE (init)))
                *arg_p = init;
            }
    which is perfectly fine optimization for calls with unordered arguments,
    but breaks the ordered ones.
    Lastly, in corge, we have before gimplification:
      D::foo (NON_LVALUE_EXPR <p>, 3,  ++p)
    and gimplify it as
      p = p + 4;
      D::foo (p, 3, p);
    which is again wrong, because the this argument isn't before the
    side-effects but after it.
    The following patch adds cp_gimplify_arg wrapper, which if ordered
    and is_gimple_reg_type forces non-SSA_NAME is_gimple_variable
    result into a temporary, and if ordered, not is_gimple_reg_type
    and argument is TARGET_EXPR bypasses the gimplify_arg optimization.
    So, in foo with this patch we gimplify it as:
      i = i + 1;
      i.0_1 = i;
      i = i + 1;
      C::C (&p, i.0_1, i);
    in qux as:
          _1 = A::operator++ (&i);
          D.2312 = MEM[(const struct A &)_1];
          _2 = A::operator++ (&i);
          B::B (&p, D.2312, MEM[(const struct A &)_2]);
    where D.2312 is a temporary and in corge as:
      p.9_1 = p;
      p = p + 4;
      D::foo (p.9_1, 3, p);
    The is_gimple_reg_type forcing into a temporary should be really cheap
    (I think even at -O0 it should be optimized if there is no modification in
    between), the aggregate copies might be more expensive but I think e.g. SRA
    or FRE should be able to deal with those if there are no intervening
    changes.  But still, the patch tries to avoid those when it is cheaply
    provable that nothing bad happens (if no argument following it in the
    strong evaluation order doesn't have TREE_SIDE_EFFECTS, then even VAR_DECLs
    etc. shouldn't be modified after it).  There is also an optimization to
    avoid doing that for this or for arguments with reference types as nothing
    can modify the parameter values during evaluation of other argument's
    side-effects.

    I've tried if e.g.
      int i = 1;
      return i << ++i;
    doesn't suffer from this problem as well, but it doesn't, the FE uses
      SAVE_EXPR <i>, SAVE_EXPR <i> << ++i;
    in that case which gimplifies the way we want (temporary in the first
    operand).

    2021-11-19  Jakub Jelinek  <jakub@redhat.com>

            PR c++/70796
            * cp-gimplify.c (cp_gimplify_arg): New function.
            (cp_gimplify_expr): Use cp_gimplify_arg instead of gimplify_arg,
            pass true as last argument to it if there are any following
            arguments in strong evaluation order with side-effects.

            * g++.dg/cpp1z/eval-order11.C: New test.

    (cherry picked from commit a84177aff7ca86f501d6aa5ef407fac5e71f56fb)

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Bug c++/70796] [DR 1030] Initialization order with braced-init-lists still broken
       [not found] <bug-70796-4@http.gcc.gnu.org/bugzilla/>
                   ` (6 preceding siblings ...)
  2021-11-29  8:50 ` cvs-commit at gcc dot gnu.org
@ 2022-05-10  8:21 ` cvs-commit at gcc dot gnu.org
  2022-05-11  6:23 ` cvs-commit at gcc dot gnu.org
  2022-05-11  6:32 ` jakub at gcc dot gnu.org
  9 siblings, 0 replies; 10+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2022-05-10  8:21 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70796

--- Comment #10 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-10 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:13d2dd6aee56b9e06d5fe3a6f2d9b76ee0818d54

commit r10-10655-g13d2dd6aee56b9e06d5fe3a6f2d9b76ee0818d54
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Fri Nov 19 10:05:01 2021 +0100

    c++: Fix up -fstrong-eval-order handling of call arguments [PR70796]

    For -fstrong-eval-order (default for C++17 and later) we make sure to
    gimplify arguments in the right order, but as the following testcase
    shows that is not enough.
    The problem is that some lvalues can satisfy the is_gimple_val / fb_rvalue
    predicate used by gimplify_arg for is_gimple_reg_type typed expressions,
    or is_gimple_lvalue / fb_either used for other types.
    E.g. in foo we have:
      C::C (&p,  ++i,  ++i)
    before gimplification where i is an automatic int variable and without this
    patch gimplify that as:
      i = i + 1;
      i = i + 1;
      C::C (&p, i, i);
    which means that the ctor is called with the original i value incremented
    by 2 in both arguments, while because the call is CALL_EXPR_ORDERED_ARGS
    the first argument should be different.  Similarly in qux we have:
      B::B (&p, TARGET_EXPR <D.2274, *(const struct A &) A::operator++ (&i)>,
            TARGET_EXPR <D.2275, *(const struct A &) A::operator++ (&i)>)
    and gimplify it as:
          _1 = A::operator++ (&i);
          _2 = A::operator++ (&i);
          B::B (&p, MEM[(const struct A &)_1], MEM[(const struct A &)_2]);
    but because A::operator++ returns the passed in argument, again we have
    the same value in both cases due to gimplify_arg doing:
          /* Also strip a TARGET_EXPR that would force an extra copy.  */
          if (TREE_CODE (*arg_p) == TARGET_EXPR)
            {
              tree init = TARGET_EXPR_INITIAL (*arg_p);
              if (init
                  && !VOID_TYPE_P (TREE_TYPE (init)))
                *arg_p = init;
            }
    which is perfectly fine optimization for calls with unordered arguments,
    but breaks the ordered ones.
    Lastly, in corge, we have before gimplification:
      D::foo (NON_LVALUE_EXPR <p>, 3,  ++p)
    and gimplify it as
      p = p + 4;
      D::foo (p, 3, p);
    which is again wrong, because the this argument isn't before the
    side-effects but after it.
    The following patch adds cp_gimplify_arg wrapper, which if ordered
    and is_gimple_reg_type forces non-SSA_NAME is_gimple_variable
    result into a temporary, and if ordered, not is_gimple_reg_type
    and argument is TARGET_EXPR bypasses the gimplify_arg optimization.
    So, in foo with this patch we gimplify it as:
      i = i + 1;
      i.0_1 = i;
      i = i + 1;
      C::C (&p, i.0_1, i);
    in qux as:
          _1 = A::operator++ (&i);
          D.2312 = MEM[(const struct A &)_1];
          _2 = A::operator++ (&i);
          B::B (&p, D.2312, MEM[(const struct A &)_2]);
    where D.2312 is a temporary and in corge as:
      p.9_1 = p;
      p = p + 4;
      D::foo (p.9_1, 3, p);
    The is_gimple_reg_type forcing into a temporary should be really cheap
    (I think even at -O0 it should be optimized if there is no modification in
    between), the aggregate copies might be more expensive but I think e.g. SRA
    or FRE should be able to deal with those if there are no intervening
    changes.  But still, the patch tries to avoid those when it is cheaply
    provable that nothing bad happens (if no argument following it in the
    strong evaluation order doesn't have TREE_SIDE_EFFECTS, then even VAR_DECLs
    etc. shouldn't be modified after it).  There is also an optimization to
    avoid doing that for this or for arguments with reference types as nothing
    can modify the parameter values during evaluation of other argument's
    side-effects.

    I've tried if e.g.
      int i = 1;
      return i << ++i;
    doesn't suffer from this problem as well, but it doesn't, the FE uses
      SAVE_EXPR <i>, SAVE_EXPR <i> << ++i;
    in that case which gimplifies the way we want (temporary in the first
    operand).

    2021-11-19  Jakub Jelinek  <jakub@redhat.com>

            PR c++/70796
            * cp-gimplify.c (cp_gimplify_arg): New function.
            (cp_gimplify_expr): Use cp_gimplify_arg instead of gimplify_arg,
            pass true as last argument to it if there are any following
            arguments in strong evaluation order with side-effects.

            * g++.dg/cpp1z/eval-order11.C: New test.

    (cherry picked from commit a84177aff7ca86f501d6aa5ef407fac5e71f56fb)

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Bug c++/70796] [DR 1030] Initialization order with braced-init-lists still broken
       [not found] <bug-70796-4@http.gcc.gnu.org/bugzilla/>
                   ` (7 preceding siblings ...)
  2022-05-10  8:21 ` cvs-commit at gcc dot gnu.org
@ 2022-05-11  6:23 ` cvs-commit at gcc dot gnu.org
  2022-05-11  6:32 ` jakub at gcc dot gnu.org
  9 siblings, 0 replies; 10+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2022-05-11  6:23 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70796

--- Comment #11 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-9 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:547692808b419da9ed33a9259d031cf62c614dfc

commit r9-10111-g547692808b419da9ed33a9259d031cf62c614dfc
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Fri Nov 19 10:05:01 2021 +0100

    c++: Fix up -fstrong-eval-order handling of call arguments [PR70796]

    For -fstrong-eval-order (default for C++17 and later) we make sure to
    gimplify arguments in the right order, but as the following testcase
    shows that is not enough.
    The problem is that some lvalues can satisfy the is_gimple_val / fb_rvalue
    predicate used by gimplify_arg for is_gimple_reg_type typed expressions,
    or is_gimple_lvalue / fb_either used for other types.
    E.g. in foo we have:
      C::C (&p,  ++i,  ++i)
    before gimplification where i is an automatic int variable and without this
    patch gimplify that as:
      i = i + 1;
      i = i + 1;
      C::C (&p, i, i);
    which means that the ctor is called with the original i value incremented
    by 2 in both arguments, while because the call is CALL_EXPR_ORDERED_ARGS
    the first argument should be different.  Similarly in qux we have:
      B::B (&p, TARGET_EXPR <D.2274, *(const struct A &) A::operator++ (&i)>,
            TARGET_EXPR <D.2275, *(const struct A &) A::operator++ (&i)>)
    and gimplify it as:
          _1 = A::operator++ (&i);
          _2 = A::operator++ (&i);
          B::B (&p, MEM[(const struct A &)_1], MEM[(const struct A &)_2]);
    but because A::operator++ returns the passed in argument, again we have
    the same value in both cases due to gimplify_arg doing:
          /* Also strip a TARGET_EXPR that would force an extra copy.  */
          if (TREE_CODE (*arg_p) == TARGET_EXPR)
            {
              tree init = TARGET_EXPR_INITIAL (*arg_p);
              if (init
                  && !VOID_TYPE_P (TREE_TYPE (init)))
                *arg_p = init;
            }
    which is perfectly fine optimization for calls with unordered arguments,
    but breaks the ordered ones.
    Lastly, in corge, we have before gimplification:
      D::foo (NON_LVALUE_EXPR <p>, 3,  ++p)
    and gimplify it as
      p = p + 4;
      D::foo (p, 3, p);
    which is again wrong, because the this argument isn't before the
    side-effects but after it.
    The following patch adds cp_gimplify_arg wrapper, which if ordered
    and is_gimple_reg_type forces non-SSA_NAME is_gimple_variable
    result into a temporary, and if ordered, not is_gimple_reg_type
    and argument is TARGET_EXPR bypasses the gimplify_arg optimization.
    So, in foo with this patch we gimplify it as:
      i = i + 1;
      i.0_1 = i;
      i = i + 1;
      C::C (&p, i.0_1, i);
    in qux as:
          _1 = A::operator++ (&i);
          D.2312 = MEM[(const struct A &)_1];
          _2 = A::operator++ (&i);
          B::B (&p, D.2312, MEM[(const struct A &)_2]);
    where D.2312 is a temporary and in corge as:
      p.9_1 = p;
      p = p + 4;
      D::foo (p.9_1, 3, p);
    The is_gimple_reg_type forcing into a temporary should be really cheap
    (I think even at -O0 it should be optimized if there is no modification in
    between), the aggregate copies might be more expensive but I think e.g. SRA
    or FRE should be able to deal with those if there are no intervening
    changes.  But still, the patch tries to avoid those when it is cheaply
    provable that nothing bad happens (if no argument following it in the
    strong evaluation order doesn't have TREE_SIDE_EFFECTS, then even VAR_DECLs
    etc. shouldn't be modified after it).  There is also an optimization to
    avoid doing that for this or for arguments with reference types as nothing
    can modify the parameter values during evaluation of other argument's
    side-effects.

    I've tried if e.g.
      int i = 1;
      return i << ++i;
    doesn't suffer from this problem as well, but it doesn't, the FE uses
      SAVE_EXPR <i>, SAVE_EXPR <i> << ++i;
    in that case which gimplifies the way we want (temporary in the first
    operand).

    2021-11-19  Jakub Jelinek  <jakub@redhat.com>

            PR c++/70796
            * cp-gimplify.c (cp_gimplify_arg): New function.
            (cp_gimplify_expr): Use cp_gimplify_arg instead of gimplify_arg,
            pass true as last argument to it if there are any following
            arguments in strong evaluation order with side-effects.

            * g++.dg/cpp1z/eval-order11.C: New test.

    (cherry picked from commit a84177aff7ca86f501d6aa5ef407fac5e71f56fb)

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Bug c++/70796] [DR 1030] Initialization order with braced-init-lists still broken
       [not found] <bug-70796-4@http.gcc.gnu.org/bugzilla/>
                   ` (8 preceding siblings ...)
  2022-05-11  6:23 ` cvs-commit at gcc dot gnu.org
@ 2022-05-11  6:32 ` jakub at gcc dot gnu.org
  9 siblings, 0 replies; 10+ messages in thread
From: jakub at gcc dot gnu.org @ 2022-05-11  6:32 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70796

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #12 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-05-11  6:32 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <bug-70796-4@http.gcc.gnu.org/bugzilla/>
2021-11-03 13:30 ` [Bug c++/70796] [DR 1030] Initialization order with braced-init-lists still broken matthijsvanduin at gmail dot com
2021-11-03 13:46 ` jason at gcc dot gnu.org
2021-11-03 18:56 ` jakub at gcc dot gnu.org
2021-11-03 19:42 ` jakub at gcc dot gnu.org
2021-11-19  9:09 ` cvs-commit at gcc dot gnu.org
2021-11-19  9:13 ` jakub at gcc dot gnu.org
2021-11-29  8:50 ` cvs-commit at gcc dot gnu.org
2022-05-10  8:21 ` cvs-commit at gcc dot gnu.org
2022-05-11  6:23 ` cvs-commit at gcc dot gnu.org
2022-05-11  6:32 ` jakub at gcc dot gnu.org

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).