Message ID | 20181129215227.GV12380@tucnak |
---|---|
State | New |
Headers | show |
Series | [C++] Fix xvalue COND_EXPR handling (PR c++/88103) | expand |
On 11/29/18 4:52 PM, Jakub Jelinek wrote: > Hi! > > On the following testcase, build_conditional_expr_1 tries hard to make sure > that if both arguments are xvalue_p (or one is and the other throw) the > result is still xvalue_p. But, later on we call unary_complex_lvalue, > which does rationalize_conditional_expr which changes it from > cond ? x : y to *(cond ? &x : &y) and that change turns something formerly > xvalue_p into newly lvalue_p. > > Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, > ok for trunk? > > 2018-11-29 Jakub Jelinek <jakub@redhat.com> > > PR c++/88103 > * typeck.c (unary_complex_lvalue): If a COND_EXPR is xvalue_p, make > sure the result is as well. > > * g++.dg/cpp0x/rv-cond3.C: New test. > > --- gcc/cp/typeck.c.jj 2018-11-27 09:48:58.506103668 +0100 > +++ gcc/cp/typeck.c 2018-11-29 21:00:33.900636750 +0100 > @@ -6503,7 +6503,16 @@ unary_complex_lvalue (enum tree_code cod > /* Handle (a ? b : c) used as an "lvalue". */ > if (TREE_CODE (arg) == COND_EXPR > || TREE_CODE (arg) == MIN_EXPR || TREE_CODE (arg) == MAX_EXPR) > - return rationalize_conditional_expr (code, arg, tf_warning_or_error); > + { > + tree ret = rationalize_conditional_expr (code, arg, tf_warning_or_error); > + /* Preserve xvalue kind. */ > + if (xvalue_p (arg)) > + { > + tree reftype = cp_build_reference_type (TREE_TYPE (arg), true); > + ret = cp_convert (reftype, ret, tf_warning_or_error); Is there a reason not to use the 'move' function here? Jason
On Sat, Dec 01, 2018 at 07:11:08PM -0500, Jason Merrill wrote: > > On the following testcase, build_conditional_expr_1 tries hard to make sure > > that if both arguments are xvalue_p (or one is and the other throw) the > > result is still xvalue_p. But, later on we call unary_complex_lvalue, > > which does rationalize_conditional_expr which changes it from > > cond ? x : y to *(cond ? &x : &y) and that change turns something formerly > > xvalue_p into newly lvalue_p. > > > > Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, > > ok for trunk? > > > > 2018-11-29 Jakub Jelinek <jakub@redhat.com> > > > > PR c++/88103 > > * typeck.c (unary_complex_lvalue): If a COND_EXPR is xvalue_p, make > > sure the result is as well. > > > > * g++.dg/cpp0x/rv-cond3.C: New test. > > > > --- gcc/cp/typeck.c.jj 2018-11-27 09:48:58.506103668 +0100 > > +++ gcc/cp/typeck.c 2018-11-29 21:00:33.900636750 +0100 > > @@ -6503,7 +6503,16 @@ unary_complex_lvalue (enum tree_code cod > > /* Handle (a ? b : c) used as an "lvalue". */ > > if (TREE_CODE (arg) == COND_EXPR > > || TREE_CODE (arg) == MIN_EXPR || TREE_CODE (arg) == MAX_EXPR) > > - return rationalize_conditional_expr (code, arg, tf_warning_or_error); > > + { > > + tree ret = rationalize_conditional_expr (code, arg, tf_warning_or_error); > > + /* Preserve xvalue kind. */ > > + if (xvalue_p (arg)) > > + { > > + tree reftype = cp_build_reference_type (TREE_TYPE (arg), true); > > + ret = cp_convert (reftype, ret, tf_warning_or_error); > > Is there a reason not to use the 'move' function here? That doesn't work at all. move doesn't call cp_convert, but build_static_cast (though for the same reference && type). But while cp_convert only adds NOP_EXPR around it, build_static_cast adds a target_expr, addr_expr around that, nop_expr cast to the reference && type and finally indirect_ref that the caller doesn't expect, because it adds it by itself, e.g. in 2424 if (temp) 2425 object = cp_build_fold_indirect_ref (temp); Jakub
On 12/2/18 8:07 AM, Jakub Jelinek wrote: > On Sat, Dec 01, 2018 at 07:11:08PM -0500, Jason Merrill wrote: >>> On the following testcase, build_conditional_expr_1 tries hard to make sure >>> that if both arguments are xvalue_p (or one is and the other throw) the >>> result is still xvalue_p. But, later on we call unary_complex_lvalue, >>> which does rationalize_conditional_expr which changes it from >>> cond ? x : y to *(cond ? &x : &y) and that change turns something formerly >>> xvalue_p into newly lvalue_p. >>> >>> Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, >>> ok for trunk? >>> >>> 2018-11-29 Jakub Jelinek <jakub@redhat.com> >>> >>> PR c++/88103 >>> * typeck.c (unary_complex_lvalue): If a COND_EXPR is xvalue_p, make >>> sure the result is as well. >>> >>> * g++.dg/cpp0x/rv-cond3.C: New test. >>> >>> --- gcc/cp/typeck.c.jj 2018-11-27 09:48:58.506103668 +0100 >>> +++ gcc/cp/typeck.c 2018-11-29 21:00:33.900636750 +0100 >>> @@ -6503,7 +6503,16 @@ unary_complex_lvalue (enum tree_code cod >>> /* Handle (a ? b : c) used as an "lvalue". */ >>> if (TREE_CODE (arg) == COND_EXPR >>> || TREE_CODE (arg) == MIN_EXPR || TREE_CODE (arg) == MAX_EXPR) >>> - return rationalize_conditional_expr (code, arg, tf_warning_or_error); >>> + { >>> + tree ret = rationalize_conditional_expr (code, arg, tf_warning_or_error); >>> + /* Preserve xvalue kind. */ >>> + if (xvalue_p (arg)) >>> + { >>> + tree reftype = cp_build_reference_type (TREE_TYPE (arg), true); >>> + ret = cp_convert (reftype, ret, tf_warning_or_error); >> >> Is there a reason not to use the 'move' function here? > > That doesn't work at all. move doesn't call cp_convert, but > build_static_cast (though for the same reference && type). > But while cp_convert only adds NOP_EXPR around it, build_static_cast adds > a target_expr, addr_expr around that, nop_expr cast to the reference && type > and finally indirect_ref that the caller doesn't expect, because it adds it > by itself, e.g. in > 2424 if (temp) > 2425 object = cp_build_fold_indirect_ref (temp); So the caller is trying to take the address of the COND_EXPR, which should have POINTER_TYPE. And then indirecting that gives an lvalue, as it should. The bug is in the caller, build_class_member_access_expr. Jason
On Mon, Dec 03, 2018 at 02:44:32PM -0500, Jason Merrill wrote: > > > Is there a reason not to use the 'move' function here? > > > > That doesn't work at all. move doesn't call cp_convert, but > > build_static_cast (though for the same reference && type). > > But while cp_convert only adds NOP_EXPR around it, build_static_cast adds > > a target_expr, addr_expr around that, nop_expr cast to the reference && type > > and finally indirect_ref that the caller doesn't expect, because it adds it > > by itself, e.g. in > > 2424 if (temp) > > 2425 object = cp_build_fold_indirect_ref (temp); > > So the caller is trying to take the address of the COND_EXPR, which should > have POINTER_TYPE. And then indirecting that gives an lvalue, as it should. > The bug is in the caller, build_class_member_access_expr. So like this then (if it passes bootstrap/regtest)? Seems to fix the testcase. 2018-12-03 Jakub Jelinek <jakub@redhat.com> PR c++/88103 * typeck.c (build_class_member_access_expr): If unary_complex_lvalue turned xvalue_p into non-xvalue_p, call move on it. * g++.dg/cpp0x/rv-cond3.C: New test. --- gcc/cp/typeck.c.jj 2018-12-02 21:41:09.824475721 +0100 +++ gcc/cp/typeck.c 2018-12-03 22:06:04.425357227 +0100 @@ -2422,7 +2422,13 @@ build_class_member_access_expr (cp_expr { tree temp = unary_complex_lvalue (ADDR_EXPR, object); if (temp) - object = cp_build_fold_indirect_ref (temp); + { + temp = cp_build_fold_indirect_ref (temp); + if (xvalue_p (object) && !xvalue_p (temp)) + /* Preserve xvalue kind. */ + temp = move (temp); + object = temp; + } } /* In [expr.ref], there is an explicit list of the valid choices for --- gcc/testsuite/g++.dg/cpp0x/rv-cond3.C.jj 2018-12-03 22:04:14.064144468 +0100 +++ gcc/testsuite/g++.dg/cpp0x/rv-cond3.C 2018-12-03 22:04:14.064144468 +0100 @@ -0,0 +1,22 @@ +// PR c++/88103 +// { dg-do compile { target c++11 } } + +struct A { + A (int); + A&& foo () &&; + int i; +}; +void free (A&&); + +void test_xvalue (A a){ + A&& ref = true ? static_cast<A&&> (a) : static_cast<A&&> (a); + free (true ? static_cast<A&&> (a) : static_cast<A&&> (a)); + (true ? static_cast<A&&> (a) : static_cast<A&&> (a)).foo (); + int&& k = (true ? static_cast<A&&> (a) : static_cast<A&&> (a)).i; +} +void test_prvalue (A a){ + A&& ref = true ? static_cast<A&&> (a) : 1; + free (true ? static_cast<A&&> (a) : 1); + (true ? static_cast<A&&> (a) : 1).foo (); + int&& k = (true ? static_cast<A&&> (a) : 1).i; +} Jakub
On Mon, Dec 3, 2018 at 4:36 PM Jakub Jelinek <jakub@redhat.com> wrote: > > On Mon, Dec 03, 2018 at 02:44:32PM -0500, Jason Merrill wrote: > > > > Is there a reason not to use the 'move' function here? > > > > > > That doesn't work at all. move doesn't call cp_convert, but > > > build_static_cast (though for the same reference && type). > > > But while cp_convert only adds NOP_EXPR around it, build_static_cast adds > > > a target_expr, addr_expr around that, nop_expr cast to the reference && type > > > and finally indirect_ref that the caller doesn't expect, because it adds it > > > by itself, e.g. in > > > 2424 if (temp) > > > 2425 object = cp_build_fold_indirect_ref (temp); > > > > So the caller is trying to take the address of the COND_EXPR, which should > > have POINTER_TYPE. And then indirecting that gives an lvalue, as it should. > > The bug is in the caller, build_class_member_access_expr. > > So like this then (if it passes bootstrap/regtest)? Seems to fix the > testcase. > > 2018-12-03 Jakub Jelinek <jakub@redhat.com> > > PR c++/88103 > * typeck.c (build_class_member_access_expr): If unary_complex_lvalue > turned xvalue_p into non-xvalue_p, call move on it. > > * g++.dg/cpp0x/rv-cond3.C: New test. OK, thanks. Jason
--- gcc/cp/typeck.c.jj 2018-11-27 09:48:58.506103668 +0100 +++ gcc/cp/typeck.c 2018-11-29 21:00:33.900636750 +0100 @@ -6503,7 +6503,16 @@ unary_complex_lvalue (enum tree_code cod /* Handle (a ? b : c) used as an "lvalue". */ if (TREE_CODE (arg) == COND_EXPR || TREE_CODE (arg) == MIN_EXPR || TREE_CODE (arg) == MAX_EXPR) - return rationalize_conditional_expr (code, arg, tf_warning_or_error); + { + tree ret = rationalize_conditional_expr (code, arg, tf_warning_or_error); + /* Preserve xvalue kind. */ + if (xvalue_p (arg)) + { + tree reftype = cp_build_reference_type (TREE_TYPE (arg), true); + ret = cp_convert (reftype, ret, tf_warning_or_error); + } + return ret; + } /* Handle (a = b), (++a), and (--a) used as an "lvalue". */ if (TREE_CODE (arg) == MODIFY_EXPR --- gcc/testsuite/g++.dg/cpp0x/rv-cond3.C.jj 2018-11-29 21:04:48.228440774 +0100 +++ gcc/testsuite/g++.dg/cpp0x/rv-cond3.C 2018-11-29 21:06:22.315888491 +0100 @@ -0,0 +1,22 @@ +// PR c++/88103 +// { dg-do compile { target c++11 } } + +struct A { + A (int); + A&& foo () &&; + int i; +}; +void free (A&&); + +void test_xvalue (A a){ + A&& ref = true ? static_cast<A&&> (a) : static_cast<A&&> (a); + free (true ? static_cast<A&&> (a) : static_cast<A&&> (a)); + (true ? static_cast<A&&> (a) : static_cast<A&&> (a)).foo (); + int&& k = (true ? static_cast<A&&> (a) : static_cast<A&&> (a)).i; +} +void test_prvalue (A a){ + A&& ref = true ? static_cast<A&&> (a) : 1; + free (true ? static_cast<A&&> (a) : 1); + (true ? static_cast<A&&> (a) : 1).foo (); + int&& k = (true ? static_cast<A&&> (a) : 1).i; +}