Message ID | 20220526150145.23850-1-polacek@redhat.com |
---|---|
State | New |
Headers | show |
Series | c++: fix broken copy elision with nested TARGET_EXPRs [PR105550] | expand |
On 5/26/22 11:01, Marek Polacek wrote: > In this problem, we are failing to properly perform copy elision with > a conditional operator, so this: > > constexpr A a = true ? A{} : A{}; > > fails with: > > error: 'A{((const A*)(&<anonymous>))}' is not a constant expression > > The whole initializer is > > TARGET_EXPR <D.2395, 1 ? TARGET_EXPR <D.2393, {.p=(const struct A *) &<PLACEHOLDER_EXPR struct A>}> : TARGET_EXPR <D.2394, {.p=(const struct A *) &<PLACEHOLDER_EXPR struct A>}>> > > where the outermost TARGET_EXPR is elided, but not the nested ones. > Then we end up replacing the PLACEHOLDER_EXPRs with the temporaries the > TARGET_EXPRs represent, which is precisely what should *not* happen with > copy elision. > > I've tried the approach of tweaking ctx->object, but I ran into gazillion > problems with that. I thought that I would let cxx_eval_constant_expression > /TARGET_EXPR create a new object only when ctx->object was null, then > adjust setting of ctx->object in places like cxx_bind_parameters_in_call > and cxx_eval_component_reference but that failed completely. Sometimes > ctx->object has to be reset, sometimes it cannot be reset, 'this' needed > special handling, etc. I gave up. > > But now that we have potential_prvalue_result_of, a simple but less > elegant solution is the following. I thought about setting a flag on > a TARGET_EXPR to avoid adding ctx.full_expr, but a new flag would be > overkill and using TARGET_EXPR_DIRECT_INIT_P broke things. This doesn't seem like a general solution; the same issue would also apply to ?: of TARGET_EXPR when it's a subexpression rather than the full expression, like f(true ? A{} : B{}). Another simple approach, but more general, would be to routinely strip TARGET_EXPR from the operands of ?: like we do in various other places in constexpr.c. > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > PR c++/105550 > > gcc/cp/ChangeLog: > > * constexpr.cc (struct constexpr_ctx): Add a tree member. > (init_subob_ctx): Set it. > (cxx_eval_constant_expression): Don't initialize a temporary object > if potential_prvalue_result_of says true. > (cxx_eval_outermost_constant_expr): Adjust the ctx initializer. Set > ctx.full_expr. > * cp-tree.h (potential_prvalue_result_of): Declare. > * typeck2.cc (potential_prvalue_result_of): No longer static. Return > if full_expr is null. > > gcc/testsuite/ChangeLog: > > * g++.dg/cpp1y/constexpr-elision1.C: New test. > --- > gcc/cp/constexpr.cc | 33 +++++++++--- > gcc/cp/cp-tree.h | 1 + > gcc/cp/typeck2.cc | 4 +- > .../g++.dg/cpp1y/constexpr-elision1.C | 53 +++++++++++++++++++ > 4 files changed, 82 insertions(+), 9 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-elision1.C > > diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc > index 45208478c3f..73880fb089e 100644 > --- a/gcc/cp/constexpr.cc > +++ b/gcc/cp/constexpr.cc > @@ -1129,6 +1129,9 @@ struct constexpr_ctx { > tree ctor; > /* The object we're building the CONSTRUCTOR for. */ > tree object; > + /* The whole initializer expression. Currently only used when the whole > + expression is a TARGET_EXPR. */ > + tree full_expr; > /* If inside SWITCH_EXPR. */ > constexpr_switch_state *css_state; > /* The aggregate initialization context inside which this one is nested. This > @@ -4700,8 +4703,11 @@ init_subob_ctx (const constexpr_ctx *ctx, constexpr_ctx &new_ctx, > new_ctx.ctor = elt; > > if (TREE_CODE (value) == TARGET_EXPR) > - /* Avoid creating another CONSTRUCTOR when we expand the TARGET_EXPR. */ > - value = TARGET_EXPR_INITIAL (value); > + { > + new_ctx.full_expr = value; > + /* Avoid creating another CONSTRUCTOR when we expand the TARGET_EXPR. */ > + value = TARGET_EXPR_INITIAL (value); > + } > } > > /* We're about to process an initializer for a class or array TYPE. Make > @@ -6822,6 +6828,14 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx, tree t, > break; > } > gcc_checking_assert (!TARGET_EXPR_DIRECT_INIT_P (t)); > + /* This TARGET_EXPR may be nested inside another TARGET_EXPR, but > + still serve as the initializer for the same object as the outer > + TARGET_EXPR, as in > + A a = true ? A{} : A{}; > + so we can't materialize a temporary. IOW, don't set ctx->object > + to the TARGET_EXPR's slot. */ > + const bool prvalue = potential_prvalue_result_of (t, ctx->full_expr); > + gcc_checking_assert (!prvalue || lval == vc_prvalue); > /* Avoid evaluating a TARGET_EXPR more than once. */ > tree slot = TARGET_EXPR_SLOT (t); > if (tree *p = ctx->global->values.get (slot)) > @@ -6831,7 +6845,7 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx, tree t, > r = *p; > break; > } > - if ((AGGREGATE_TYPE_P (type) || VECTOR_TYPE_P (type))) > + if (!prvalue && (AGGREGATE_TYPE_P (type) || VECTOR_TYPE_P (type))) > { > /* We're being expanded without an explicit target, so start > initializing a new object; expansion with an explicit target > @@ -7748,8 +7762,8 @@ cxx_eval_outermost_constant_expr (tree t, bool allow_non_constant, > } > > constexpr_global_ctx global_ctx; > - constexpr_ctx ctx = { &global_ctx, NULL, NULL, NULL, NULL, NULL, NULL, > - allow_non_constant, strict, > + constexpr_ctx ctx = { &global_ctx, nullptr, nullptr, NULL_TREE, NULL_TREE, > + NULL_TREE, nullptr, nullptr, allow_non_constant, strict, > manifestly_const_eval || !allow_non_constant }; > > /* Turn off -frounding-math for manifestly constant evaluation. */ > @@ -7834,9 +7848,12 @@ cxx_eval_outermost_constant_expr (tree t, bool allow_non_constant, > if (object && DECL_P (object)) > global_ctx.values.put (object, ctx.ctor); > if (TREE_CODE (r) == TARGET_EXPR) > - /* Avoid creating another CONSTRUCTOR when we expand the > - TARGET_EXPR. */ > - r = TARGET_EXPR_INITIAL (r); > + { > + ctx.full_expr = r; > + /* Avoid creating another CONSTRUCTOR when we expand the > + TARGET_EXPR. */ > + r = TARGET_EXPR_INITIAL (r); > + } > } > > auto_vec<tree, 16> cleanups; > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h > index d77fd1eb8a9..c3c1f5889a3 100644 > --- a/gcc/cp/cp-tree.h > +++ b/gcc/cp/cp-tree.h > @@ -8160,6 +8160,7 @@ extern tree build_functional_cast (location_t, tree, tree, > tsubst_flags_t); > extern tree add_exception_specifier (tree, tree, tsubst_flags_t); > extern tree merge_exception_specifiers (tree, tree); > +extern bool potential_prvalue_result_of (tree, tree); > > /* in mangle.cc */ > extern void init_mangle (void); > diff --git a/gcc/cp/typeck2.cc b/gcc/cp/typeck2.cc > index 1a96be3d412..6578d5ed6d2 100644 > --- a/gcc/cp/typeck2.cc > +++ b/gcc/cp/typeck2.cc > @@ -1383,9 +1383,11 @@ digest_init_flags (tree type, tree init, int flags, tsubst_flags_t complain) > > FULL_EXPR is the whole expression, SUBOB is its TARGET_EXPR subobject. */ > > -static bool > +bool > potential_prvalue_result_of (tree subob, tree full_expr) > { > + if (!full_expr) > + return false; > if (subob == full_expr) > return true; > else if (TREE_CODE (full_expr) == TARGET_EXPR) > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-elision1.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-elision1.C > new file mode 100644 > index 00000000000..b225ae29cde > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-elision1.C > @@ -0,0 +1,53 @@ > +// PR c++/105550 > +// { dg-do compile { target c++14 } } > + > +struct A { > + const A *p = this; > +}; > + > +struct B { > + const B *p = this; > + constexpr operator A() const { return {}; } > +}; > + > +constexpr A > +bar (A) > +{ > + return {}; > +} > + > +constexpr A baz() { return {}; } > + > +struct E { > + A a1 = true ? A{} : A{}; > + A a2 = true ? A{} : B{}; > + A a3 = false ? A{} : B{}; > + A a4 = false ? B{} : B{}; > + A a5 = A{}; > + A a6 = B{}; > + A a7 = false ? B{} : (true ? A{} : A{}); > + A a8 = false ? (true ? A{} : B{}) : (true ? A{} : A{}); > + A a9 = (A{}); > + A a10 = (true, A{}); > + A a11 = bar (A{}); > + A a12 = baz (); > + A a13 = (A{}, A{}); > +}; > + > +constexpr E e{}; > + > +constexpr A a1 = true ? A{} : A{}; > +constexpr A a2 = true ? A{} : B{}; > +constexpr A a3 = false ? A{} : B{}; > +constexpr A a4 = false ? B{} : B{}; > +constexpr A a5 = A{}; > +constexpr A a6 = B{}; > +constexpr A a7 = false ? B{} : (true ? A{} : A{}); > +constexpr A a8 = false ? (true ? A{} : B{}) : (true ? A{} : A{}); > +constexpr A a9 = (A{}); > +constexpr A a10 = (true, A{}); > +constexpr A a11 = bar (A{}); > +//static_assert(a10.p == &a10, ""); // bug, 105619 > +constexpr A a12 = baz (); > +//static_assert(a11.p == &a11, ""); // bug, 105619 > +constexpr A a13 = (A{}, A{}); > > base-commit: 97dc78d705a90c1ae83c78a7f2e24942cc3a6257
diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc index 45208478c3f..73880fb089e 100644 --- a/gcc/cp/constexpr.cc +++ b/gcc/cp/constexpr.cc @@ -1129,6 +1129,9 @@ struct constexpr_ctx { tree ctor; /* The object we're building the CONSTRUCTOR for. */ tree object; + /* The whole initializer expression. Currently only used when the whole + expression is a TARGET_EXPR. */ + tree full_expr; /* If inside SWITCH_EXPR. */ constexpr_switch_state *css_state; /* The aggregate initialization context inside which this one is nested. This @@ -4700,8 +4703,11 @@ init_subob_ctx (const constexpr_ctx *ctx, constexpr_ctx &new_ctx, new_ctx.ctor = elt; if (TREE_CODE (value) == TARGET_EXPR) - /* Avoid creating another CONSTRUCTOR when we expand the TARGET_EXPR. */ - value = TARGET_EXPR_INITIAL (value); + { + new_ctx.full_expr = value; + /* Avoid creating another CONSTRUCTOR when we expand the TARGET_EXPR. */ + value = TARGET_EXPR_INITIAL (value); + } } /* We're about to process an initializer for a class or array TYPE. Make @@ -6822,6 +6828,14 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx, tree t, break; } gcc_checking_assert (!TARGET_EXPR_DIRECT_INIT_P (t)); + /* This TARGET_EXPR may be nested inside another TARGET_EXPR, but + still serve as the initializer for the same object as the outer + TARGET_EXPR, as in + A a = true ? A{} : A{}; + so we can't materialize a temporary. IOW, don't set ctx->object + to the TARGET_EXPR's slot. */ + const bool prvalue = potential_prvalue_result_of (t, ctx->full_expr); + gcc_checking_assert (!prvalue || lval == vc_prvalue); /* Avoid evaluating a TARGET_EXPR more than once. */ tree slot = TARGET_EXPR_SLOT (t); if (tree *p = ctx->global->values.get (slot)) @@ -6831,7 +6845,7 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx, tree t, r = *p; break; } - if ((AGGREGATE_TYPE_P (type) || VECTOR_TYPE_P (type))) + if (!prvalue && (AGGREGATE_TYPE_P (type) || VECTOR_TYPE_P (type))) { /* We're being expanded without an explicit target, so start initializing a new object; expansion with an explicit target @@ -7748,8 +7762,8 @@ cxx_eval_outermost_constant_expr (tree t, bool allow_non_constant, } constexpr_global_ctx global_ctx; - constexpr_ctx ctx = { &global_ctx, NULL, NULL, NULL, NULL, NULL, NULL, - allow_non_constant, strict, + constexpr_ctx ctx = { &global_ctx, nullptr, nullptr, NULL_TREE, NULL_TREE, + NULL_TREE, nullptr, nullptr, allow_non_constant, strict, manifestly_const_eval || !allow_non_constant }; /* Turn off -frounding-math for manifestly constant evaluation. */ @@ -7834,9 +7848,12 @@ cxx_eval_outermost_constant_expr (tree t, bool allow_non_constant, if (object && DECL_P (object)) global_ctx.values.put (object, ctx.ctor); if (TREE_CODE (r) == TARGET_EXPR) - /* Avoid creating another CONSTRUCTOR when we expand the - TARGET_EXPR. */ - r = TARGET_EXPR_INITIAL (r); + { + ctx.full_expr = r; + /* Avoid creating another CONSTRUCTOR when we expand the + TARGET_EXPR. */ + r = TARGET_EXPR_INITIAL (r); + } } auto_vec<tree, 16> cleanups; diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index d77fd1eb8a9..c3c1f5889a3 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -8160,6 +8160,7 @@ extern tree build_functional_cast (location_t, tree, tree, tsubst_flags_t); extern tree add_exception_specifier (tree, tree, tsubst_flags_t); extern tree merge_exception_specifiers (tree, tree); +extern bool potential_prvalue_result_of (tree, tree); /* in mangle.cc */ extern void init_mangle (void); diff --git a/gcc/cp/typeck2.cc b/gcc/cp/typeck2.cc index 1a96be3d412..6578d5ed6d2 100644 --- a/gcc/cp/typeck2.cc +++ b/gcc/cp/typeck2.cc @@ -1383,9 +1383,11 @@ digest_init_flags (tree type, tree init, int flags, tsubst_flags_t complain) FULL_EXPR is the whole expression, SUBOB is its TARGET_EXPR subobject. */ -static bool +bool potential_prvalue_result_of (tree subob, tree full_expr) { + if (!full_expr) + return false; if (subob == full_expr) return true; else if (TREE_CODE (full_expr) == TARGET_EXPR) diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-elision1.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-elision1.C new file mode 100644 index 00000000000..b225ae29cde --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-elision1.C @@ -0,0 +1,53 @@ +// PR c++/105550 +// { dg-do compile { target c++14 } } + +struct A { + const A *p = this; +}; + +struct B { + const B *p = this; + constexpr operator A() const { return {}; } +}; + +constexpr A +bar (A) +{ + return {}; +} + +constexpr A baz() { return {}; } + +struct E { + A a1 = true ? A{} : A{}; + A a2 = true ? A{} : B{}; + A a3 = false ? A{} : B{}; + A a4 = false ? B{} : B{}; + A a5 = A{}; + A a6 = B{}; + A a7 = false ? B{} : (true ? A{} : A{}); + A a8 = false ? (true ? A{} : B{}) : (true ? A{} : A{}); + A a9 = (A{}); + A a10 = (true, A{}); + A a11 = bar (A{}); + A a12 = baz (); + A a13 = (A{}, A{}); +}; + +constexpr E e{}; + +constexpr A a1 = true ? A{} : A{}; +constexpr A a2 = true ? A{} : B{}; +constexpr A a3 = false ? A{} : B{}; +constexpr A a4 = false ? B{} : B{}; +constexpr A a5 = A{}; +constexpr A a6 = B{}; +constexpr A a7 = false ? B{} : (true ? A{} : A{}); +constexpr A a8 = false ? (true ? A{} : B{}) : (true ? A{} : A{}); +constexpr A a9 = (A{}); +constexpr A a10 = (true, A{}); +constexpr A a11 = bar (A{}); +//static_assert(a10.p == &a10, ""); // bug, 105619 +constexpr A a12 = baz (); +//static_assert(a11.p == &a11, ""); // bug, 105619 +constexpr A a13 = (A{}, A{});