* [PATCH] c++: constexpr and copy elision within mem init [PR100368] @ 2021-05-24 17:48 Patrick Palka 2021-05-24 21:31 ` Jason Merrill 0 siblings, 1 reply; 4+ messages in thread From: Patrick Palka @ 2021-05-24 17:48 UTC (permalink / raw) To: gcc-patches In the testcase below, the initializer for C::b inside C's default constructor is encoded as a TARGET_EXPR wrapping the CALL_EXPR f() in C++17 mode. During massaging of this constexpr constructor, build_target_expr_with_type called from bot_manip ends up trying to use B's implicitly deleted copy constructor rather than preserving the copy elision. This patch makes bot_manip use force_target_expr instead of build_target_expr_with_type so that it copies TARGET_EXPRs in a more oblivious manner. Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for trunk? PR c++/100368 gcc/cp/ChangeLog: * tree.c (build_target_expr_with_type): Simplify now that bot_manip is no longer a caller. (bot_manip): Use force_target_expr instead of build_target_expr_with_type. gcc/testsuite/ChangeLog: * g++.dg/cpp1z/elide6.C: New test. --- gcc/cp/tree.c | 8 +++----- gcc/testsuite/g++.dg/cpp1z/elide6.C | 16 ++++++++++++++++ 2 files changed, 19 insertions(+), 5 deletions(-) create mode 100644 gcc/testsuite/g++.dg/cpp1z/elide6.C diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c index 72f498f4b3b..84b84621d35 100644 --- a/gcc/cp/tree.c +++ b/gcc/cp/tree.c @@ -848,12 +848,10 @@ build_target_expr_with_type (tree init, tree type, tsubst_flags_t complain) || init == error_mark_node) return init; else if (CLASS_TYPE_P (type) && type_has_nontrivial_copy_init (type) - && !VOID_TYPE_P (TREE_TYPE (init)) && TREE_CODE (init) != COND_EXPR && TREE_CODE (init) != CONSTRUCTOR && TREE_CODE (init) != VA_ARG_EXPR) - /* We need to build up a copy constructor call. A void initializer - means we're being called from bot_manip. COND_EXPR is a special + /* We need to build up a copy constructor call. COND_EXPR is a special case because we already have copies on the arms and we don't want another one here. A CONSTRUCTOR is aggregate initialization, which is handled separately. A VA_ARG_EXPR is magic creation of an @@ -3112,8 +3110,8 @@ bot_manip (tree* tp, int* walk_subtrees, void* data_) AGGR_INIT_ZERO_FIRST (TREE_OPERAND (u, 1)) = true; } else - u = build_target_expr_with_type (TREE_OPERAND (t, 1), TREE_TYPE (t), - tf_warning_or_error); + u = force_target_expr (TREE_TYPE (t), TREE_OPERAND (t, 1), + tf_warning_or_error); TARGET_EXPR_IMPLICIT_P (u) = TARGET_EXPR_IMPLICIT_P (t); TARGET_EXPR_LIST_INIT_P (u) = TARGET_EXPR_LIST_INIT_P (t); diff --git a/gcc/testsuite/g++.dg/cpp1z/elide6.C b/gcc/testsuite/g++.dg/cpp1z/elide6.C new file mode 100644 index 00000000000..399e1a9a1ef --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1z/elide6.C @@ -0,0 +1,16 @@ +// PR c++/100368 +// { dg-do compile { target c++11 } } + +struct A { + A() = default; + A(const A&) = delete; +}; + +struct B { A a; }; // { dg-error "deleted" "" { target c++14_down } } + +constexpr B f() { return {}; } + +struct C { + constexpr C() : b(f()) {} // { dg-error "deleted" "" { target c++14_down } } + B b; +}; -- 2.32.0.rc0 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] c++: constexpr and copy elision within mem init [PR100368] 2021-05-24 17:48 [PATCH] c++: constexpr and copy elision within mem init [PR100368] Patrick Palka @ 2021-05-24 21:31 ` Jason Merrill 2021-05-25 17:12 ` Patrick Palka 0 siblings, 1 reply; 4+ messages in thread From: Jason Merrill @ 2021-05-24 21:31 UTC (permalink / raw) To: Patrick Palka, gcc-patches On 5/24/21 1:48 PM, Patrick Palka wrote: > In the testcase below, the initializer for C::b inside C's default > constructor is encoded as a TARGET_EXPR wrapping the CALL_EXPR f() in > C++17 mode. During massaging of this constexpr constructor, > build_target_expr_with_type called from bot_manip ends up trying to use > B's implicitly deleted copy constructor rather than preserving the > copy elision. > This patch makes bot_manip use force_target_expr instead of > build_target_expr_with_type so that it copies TARGET_EXPRs in a more > oblivious manner. Even with that change we should fix build_target_expr_with_type to handle CALL_EXPR properly; adding an extra copy is just wrong. > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK > for trunk? > > PR c++/100368 > > gcc/cp/ChangeLog: > > * tree.c (build_target_expr_with_type): Simplify now that > bot_manip is no longer a caller. > (bot_manip): Use force_target_expr instead of > build_target_expr_with_type. > > gcc/testsuite/ChangeLog: > > * g++.dg/cpp1z/elide6.C: New test. > --- > gcc/cp/tree.c | 8 +++----- > gcc/testsuite/g++.dg/cpp1z/elide6.C | 16 ++++++++++++++++ > 2 files changed, 19 insertions(+), 5 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/cpp1z/elide6.C > > diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c > index 72f498f4b3b..84b84621d35 100644 > --- a/gcc/cp/tree.c > +++ b/gcc/cp/tree.c > @@ -848,12 +848,10 @@ build_target_expr_with_type (tree init, tree type, tsubst_flags_t complain) > || init == error_mark_node) > return init; > else if (CLASS_TYPE_P (type) && type_has_nontrivial_copy_init (type) > - && !VOID_TYPE_P (TREE_TYPE (init)) > && TREE_CODE (init) != COND_EXPR > && TREE_CODE (init) != CONSTRUCTOR > && TREE_CODE (init) != VA_ARG_EXPR) > - /* We need to build up a copy constructor call. A void initializer > - means we're being called from bot_manip. In general, a void initializer for a TARGET_EXPR means that the initialization is more complex than initializing the object from the value of the expression. The caller would need to handle making that initialization apply to the new TARGET_EXPR_SLOT (and bot_manip does). If we change bot_manip to not call this function, I think this function should reject void init. COND_EXPR is a special > + /* We need to build up a copy constructor call. COND_EXPR is a special > case because we already have copies on the arms and we don't want > another one here. A CONSTRUCTOR is aggregate initialization, which > is handled separately. A VA_ARG_EXPR is magic creation of an > @@ -3112,8 +3110,8 @@ bot_manip (tree* tp, int* walk_subtrees, void* data_) > AGGR_INIT_ZERO_FIRST (TREE_OPERAND (u, 1)) = true; > } > else > - u = build_target_expr_with_type (TREE_OPERAND (t, 1), TREE_TYPE (t), > - tf_warning_or_error); > + u = force_target_expr (TREE_TYPE (t), TREE_OPERAND (t, 1), > + tf_warning_or_error); > > TARGET_EXPR_IMPLICIT_P (u) = TARGET_EXPR_IMPLICIT_P (t); > TARGET_EXPR_LIST_INIT_P (u) = TARGET_EXPR_LIST_INIT_P (t); > diff --git a/gcc/testsuite/g++.dg/cpp1z/elide6.C b/gcc/testsuite/g++.dg/cpp1z/elide6.C > new file mode 100644 > index 00000000000..399e1a9a1ef > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1z/elide6.C > @@ -0,0 +1,16 @@ > +// PR c++/100368 > +// { dg-do compile { target c++11 } } > + > +struct A { > + A() = default; > + A(const A&) = delete; > +}; > + > +struct B { A a; }; // { dg-error "deleted" "" { target c++14_down } } > + > +constexpr B f() { return {}; } > + > +struct C { > + constexpr C() : b(f()) {} // { dg-error "deleted" "" { target c++14_down } } > + B b; > +}; > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] c++: constexpr and copy elision within mem init [PR100368] 2021-05-24 21:31 ` Jason Merrill @ 2021-05-25 17:12 ` Patrick Palka 2021-05-25 20:38 ` Jason Merrill 0 siblings, 1 reply; 4+ messages in thread From: Patrick Palka @ 2021-05-25 17:12 UTC (permalink / raw) To: Jason Merrill; +Cc: Patrick Palka, gcc-patches On Mon, 24 May 2021, Jason Merrill wrote: > On 5/24/21 1:48 PM, Patrick Palka wrote: > > In the testcase below, the initializer for C::b inside C's default > > constructor is encoded as a TARGET_EXPR wrapping the CALL_EXPR f() in > > C++17 mode. During massaging of this constexpr constructor, > > build_target_expr_with_type called from bot_manip ends up trying to use > > B's implicitly deleted copy constructor rather than preserving the > > copy elision. > > > This patch makes bot_manip use force_target_expr instead of > > build_target_expr_with_type so that it copies TARGET_EXPRs in a more > > oblivious manner. > > Even with that change we should fix build_target_expr_with_type to handle > CALL_EXPR properly; adding an extra copy is just wrong. Sounds good. > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK > > for trunk? > > > > PR c++/100368 > > > > gcc/cp/ChangeLog: > > > > * tree.c (build_target_expr_with_type): Simplify now that > > bot_manip is no longer a caller. > > (bot_manip): Use force_target_expr instead of > > build_target_expr_with_type. > > > > gcc/testsuite/ChangeLog: > > > > * g++.dg/cpp1z/elide6.C: New test. > > --- > > gcc/cp/tree.c | 8 +++----- > > gcc/testsuite/g++.dg/cpp1z/elide6.C | 16 ++++++++++++++++ > > 2 files changed, 19 insertions(+), 5 deletions(-) > > create mode 100644 gcc/testsuite/g++.dg/cpp1z/elide6.C > > > > diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c > > index 72f498f4b3b..84b84621d35 100644 > > --- a/gcc/cp/tree.c > > +++ b/gcc/cp/tree.c > > @@ -848,12 +848,10 @@ build_target_expr_with_type (tree init, tree type, > > tsubst_flags_t complain) > > || init == error_mark_node) > > return init; > > else if (CLASS_TYPE_P (type) && type_has_nontrivial_copy_init (type) > > - && !VOID_TYPE_P (TREE_TYPE (init)) > > && TREE_CODE (init) != COND_EXPR > > && TREE_CODE (init) != CONSTRUCTOR > > && TREE_CODE (init) != VA_ARG_EXPR) > > - /* We need to build up a copy constructor call. A void initializer > > - means we're being called from bot_manip. > > In general, a void initializer for a TARGET_EXPR means that the initialization > is more complex than initializing the object from the value of the expression. > The caller would need to handle making that initialization apply to the new > TARGET_EXPR_SLOT (and bot_manip does). If we change bot_manip to not call this > function, I think this function should reject void init. I see, thanks. How does the following look for trunk? Bootstrapped and regetested on x86_64-pc-linux-gnu. -- >8 -- Subject: [PATCH] c++: constexpr and copy elision within mem init [PR100368] In the testcase below, the member initializer b(f()) inside C's default constructor is encoded as a TARGET_EXPR wrapping the CALL_EXPR f() in C++17 mode. During massaging of this constexpr constructor, build_target_expr_with_type called from bot_manip tries to add an extra copy using B's implicitly deleted copy constructor rather than just preserving the copy elision. Since it's wrong to introduce an extra copy when initializing a temporary from a CALL_EXPR, this patch makes build_target_expr_with_type avoid calling force_rvalue in this case. Additionally, bot_manip should be copying TARGET_EXPRs in a more oblivious manner, so this patch makes bot_manip use force_target_expr instead of build_target_expr_with_type. And since bot_manip is now no longer a caller, we can remove the void initializer handling in build_target_expr_with_type and instead reject such initializers. PR c++/100368 gcc/cp/ChangeLog: * tree.c (build_target_expr_with_type): Don't call force_rvalue on CALL_EXPR initializer. Simplify now that bot_manip is no longer a caller. (bot_manip): Use force_target_expr instead of build_target_expr_with_type. gcc/testsuite/ChangeLog: * g++.dg/cpp1z/elide6.C: New test. --- gcc/cp/tree.c | 12 ++++++------ gcc/testsuite/g++.dg/cpp1z/elide6.C | 16 ++++++++++++++++ 2 files changed, 22 insertions(+), 6 deletions(-) create mode 100644 gcc/testsuite/g++.dg/cpp1z/elide6.C diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c index 72f498f4b3b..d97b220423d 100644 --- a/gcc/cp/tree.c +++ b/gcc/cp/tree.c @@ -843,17 +843,17 @@ tree build_target_expr_with_type (tree init, tree type, tsubst_flags_t complain) { gcc_assert (!VOID_TYPE_P (type)); + gcc_assert (!VOID_TYPE_P (TREE_TYPE (init))); if (TREE_CODE (init) == TARGET_EXPR || init == error_mark_node) return init; else if (CLASS_TYPE_P (type) && type_has_nontrivial_copy_init (type) - && !VOID_TYPE_P (TREE_TYPE (init)) && TREE_CODE (init) != COND_EXPR && TREE_CODE (init) != CONSTRUCTOR - && TREE_CODE (init) != VA_ARG_EXPR) - /* We need to build up a copy constructor call. A void initializer - means we're being called from bot_manip. COND_EXPR is a special + && TREE_CODE (init) != VA_ARG_EXPR + && TREE_CODE (init) != CALL_EXPR) + /* We need to build up a copy constructor call. COND_EXPR is a special case because we already have copies on the arms and we don't want another one here. A CONSTRUCTOR is aggregate initialization, which is handled separately. A VA_ARG_EXPR is magic creation of an @@ -3112,8 +3112,8 @@ bot_manip (tree* tp, int* walk_subtrees, void* data_) AGGR_INIT_ZERO_FIRST (TREE_OPERAND (u, 1)) = true; } else - u = build_target_expr_with_type (TREE_OPERAND (t, 1), TREE_TYPE (t), - tf_warning_or_error); + u = force_target_expr (TREE_TYPE (t), TREE_OPERAND (t, 1), + tf_warning_or_error); TARGET_EXPR_IMPLICIT_P (u) = TARGET_EXPR_IMPLICIT_P (t); TARGET_EXPR_LIST_INIT_P (u) = TARGET_EXPR_LIST_INIT_P (t); diff --git a/gcc/testsuite/g++.dg/cpp1z/elide6.C b/gcc/testsuite/g++.dg/cpp1z/elide6.C new file mode 100644 index 00000000000..399e1a9a1ef --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1z/elide6.C @@ -0,0 +1,16 @@ +// PR c++/100368 +// { dg-do compile { target c++11 } } + +struct A { + A() = default; + A(const A&) = delete; +}; + +struct B { A a; }; // { dg-error "deleted" "" { target c++14_down } } + +constexpr B f() { return {}; } + +struct C { + constexpr C() : b(f()) {} // { dg-error "deleted" "" { target c++14_down } } + B b; +}; -- 2.32.0.rc0 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] c++: constexpr and copy elision within mem init [PR100368] 2021-05-25 17:12 ` Patrick Palka @ 2021-05-25 20:38 ` Jason Merrill 0 siblings, 0 replies; 4+ messages in thread From: Jason Merrill @ 2021-05-25 20:38 UTC (permalink / raw) To: Patrick Palka; +Cc: gcc-patches On 5/25/21 1:12 PM, Patrick Palka wrote: > On Mon, 24 May 2021, Jason Merrill wrote: > >> On 5/24/21 1:48 PM, Patrick Palka wrote: >>> In the testcase below, the initializer for C::b inside C's default >>> constructor is encoded as a TARGET_EXPR wrapping the CALL_EXPR f() in >>> C++17 mode. During massaging of this constexpr constructor, >>> build_target_expr_with_type called from bot_manip ends up trying to use >>> B's implicitly deleted copy constructor rather than preserving the >>> copy elision. >> >>> This patch makes bot_manip use force_target_expr instead of >>> build_target_expr_with_type so that it copies TARGET_EXPRs in a more >>> oblivious manner. >> >> Even with that change we should fix build_target_expr_with_type to handle >> CALL_EXPR properly; adding an extra copy is just wrong. > > Sounds good. > >> >>> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK >>> for trunk? >>> >>> PR c++/100368 >>> >>> gcc/cp/ChangeLog: >>> >>> * tree.c (build_target_expr_with_type): Simplify now that >>> bot_manip is no longer a caller. >>> (bot_manip): Use force_target_expr instead of >>> build_target_expr_with_type. >>> >>> gcc/testsuite/ChangeLog: >>> >>> * g++.dg/cpp1z/elide6.C: New test. >>> --- >>> gcc/cp/tree.c | 8 +++----- >>> gcc/testsuite/g++.dg/cpp1z/elide6.C | 16 ++++++++++++++++ >>> 2 files changed, 19 insertions(+), 5 deletions(-) >>> create mode 100644 gcc/testsuite/g++.dg/cpp1z/elide6.C >>> >>> diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c >>> index 72f498f4b3b..84b84621d35 100644 >>> --- a/gcc/cp/tree.c >>> +++ b/gcc/cp/tree.c >>> @@ -848,12 +848,10 @@ build_target_expr_with_type (tree init, tree type, >>> tsubst_flags_t complain) >>> || init == error_mark_node) >>> return init; >>> else if (CLASS_TYPE_P (type) && type_has_nontrivial_copy_init (type) >>> - && !VOID_TYPE_P (TREE_TYPE (init)) >>> && TREE_CODE (init) != COND_EXPR >>> && TREE_CODE (init) != CONSTRUCTOR >>> && TREE_CODE (init) != VA_ARG_EXPR) >>> - /* We need to build up a copy constructor call. A void initializer >>> - means we're being called from bot_manip. >> >> In general, a void initializer for a TARGET_EXPR means that the initialization >> is more complex than initializing the object from the value of the expression. >> The caller would need to handle making that initialization apply to the new >> TARGET_EXPR_SLOT (and bot_manip does). If we change bot_manip to not call this >> function, I think this function should reject void init. > > I see, thanks. How does the following look for trunk? Bootstrapped and > regetested on x86_64-pc-linux-gnu. > > -- >8 -- > > > Subject: [PATCH] c++: constexpr and copy elision within mem init [PR100368] > > In the testcase below, the member initializer b(f()) inside C's default > constructor is encoded as a TARGET_EXPR wrapping the CALL_EXPR f() in > C++17 mode. During massaging of this constexpr constructor, > build_target_expr_with_type called from bot_manip tries to add an extra > copy using B's implicitly deleted copy constructor rather than just > preserving the copy elision. > > Since it's wrong to introduce an extra copy when initializing a > temporary from a CALL_EXPR, this patch makes build_target_expr_with_type > avoid calling force_rvalue in this case. Additionally, bot_manip should > be copying TARGET_EXPRs in a more oblivious manner, so this patch makes > bot_manip use force_target_expr instead of build_target_expr_with_type. > And since bot_manip is now no longer a caller, we can remove the void > initializer handling in build_target_expr_with_type and instead reject > such initializers. > > PR c++/100368 > > gcc/cp/ChangeLog: > > * tree.c (build_target_expr_with_type): Don't call force_rvalue > on CALL_EXPR initializer. Simplify now that bot_manip is no > longer a caller. > (bot_manip): Use force_target_expr instead of > build_target_expr_with_type. > > gcc/testsuite/ChangeLog: > > * g++.dg/cpp1z/elide6.C: New test. > --- > gcc/cp/tree.c | 12 ++++++------ > gcc/testsuite/g++.dg/cpp1z/elide6.C | 16 ++++++++++++++++ > 2 files changed, 22 insertions(+), 6 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/cpp1z/elide6.C > > diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c > index 72f498f4b3b..d97b220423d 100644 > --- a/gcc/cp/tree.c > +++ b/gcc/cp/tree.c > @@ -843,17 +843,17 @@ tree > build_target_expr_with_type (tree init, tree type, tsubst_flags_t complain) > { > gcc_assert (!VOID_TYPE_P (type)); > + gcc_assert (!VOID_TYPE_P (TREE_TYPE (init))); > > if (TREE_CODE (init) == TARGET_EXPR > || init == error_mark_node) > return init; > else if (CLASS_TYPE_P (type) && type_has_nontrivial_copy_init (type) > - && !VOID_TYPE_P (TREE_TYPE (init)) > && TREE_CODE (init) != COND_EXPR > && TREE_CODE (init) != CONSTRUCTOR > - && TREE_CODE (init) != VA_ARG_EXPR) > - /* We need to build up a copy constructor call. A void initializer > - means we're being called from bot_manip. COND_EXPR is a special > + && TREE_CODE (init) != VA_ARG_EXPR > + && TREE_CODE (init) != CALL_EXPR) > + /* We need to build up a copy constructor call. COND_EXPR is a special > case because we already have copies on the arms and we don't want > another one here. A CONSTRUCTOR is aggregate initialization, which > is handled separately. A VA_ARG_EXPR is magic creation of an Let's add "A CALL_EXPR already creates a prvalue." to this comment. OK with that. > @@ -3112,8 +3112,8 @@ bot_manip (tree* tp, int* walk_subtrees, void* data_) > AGGR_INIT_ZERO_FIRST (TREE_OPERAND (u, 1)) = true; > } > else > - u = build_target_expr_with_type (TREE_OPERAND (t, 1), TREE_TYPE (t), > - tf_warning_or_error); > + u = force_target_expr (TREE_TYPE (t), TREE_OPERAND (t, 1), > + tf_warning_or_error); > > TARGET_EXPR_IMPLICIT_P (u) = TARGET_EXPR_IMPLICIT_P (t); > TARGET_EXPR_LIST_INIT_P (u) = TARGET_EXPR_LIST_INIT_P (t); > diff --git a/gcc/testsuite/g++.dg/cpp1z/elide6.C b/gcc/testsuite/g++.dg/cpp1z/elide6.C > new file mode 100644 > index 00000000000..399e1a9a1ef > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1z/elide6.C > @@ -0,0 +1,16 @@ > +// PR c++/100368 > +// { dg-do compile { target c++11 } } > + > +struct A { > + A() = default; > + A(const A&) = delete; > +}; > + > +struct B { A a; }; // { dg-error "deleted" "" { target c++14_down } } > + > +constexpr B f() { return {}; } > + > +struct C { > + constexpr C() : b(f()) {} // { dg-error "deleted" "" { target c++14_down } } > + B b; > +}; > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-05-25 20:38 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-24 17:48 [PATCH] c++: constexpr and copy elision within mem init [PR100368] Patrick Palka 2021-05-24 21:31 ` Jason Merrill 2021-05-25 17:12 ` Patrick Palka 2021-05-25 20:38 ` Jason Merrill
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).