Re: [PATCH] c++: constexpr and copy elision within mem init [PR100368]

[Jason Merrill <jason@redhat.com>]

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;
> +};
>