c++, v2: Fix handling of temporaries with consteval ctors and non-trivial dtors [PR104055]

On Tue, Jan 18, 2022 at 11:51:31AM -0500, Jason Merrill wrote:
> On 1/18/22 09:26, Jakub Jelinek wrote:
> > On Tue, Jan 18, 2022 at 09:08:03AM -0500, Jason Merrill wrote:
> > > > which makes call a TARGET_EXPR with the dtor in TARGET_EXPR_CLEANUP,
> > > > but then call cxx_constant_value on it.  In cxx_eval_outermost_constant_expr
> > > > it triggers the:
> > > >         else if (TREE_CODE (t) != CONSTRUCTOR)
> > > >           {
> > > >             r = get_target_expr_sfinae (r, tf_warning_or_error | tf_no_cleanup);
> > > >             TREE_CONSTANT (r) = true;
> > > >           }
> > > > which wraps the CONSTRUCTOR r into a new TARGET_EXPR, but one without
> > > > dtors (I think we need e.g. the TREE_CONSTANT for the callers),
> > > > and finally build_over_call uses that.
> > > 
> > > Looks like you added the tf_no_cleanup in r10-3661 (constexpr new). Maybe
> > > that should only be added if TARGET_EXPR_CLEANUP (t) is null?
> > 
> > I thought about that, but I'm worried that if the cleanup action
> > is non-empty, then the TREE_CONSTANT (r) = true; after it isn't appropriate,
> > because the TARGET_EXPR has side-effects.
> 
> Hmm.  If we e.g. pull out a subobject value from the TARGET_EXPR temporary,
> we still need to run the cleanup, so we don't want to optimize the temporary
> away, so not setting TREE_CONSTANT sounds right.
> 
> > And the callers like cxx_constant_value_sfinae just return error_mark_node
> > if it isn't set:
> >    if (sfinae && !TREE_CONSTANT (r))
> >      r = error_mark_node;
> 
> Yes.  Is that wrong?  I guess we need to be more careful about adjusting the
> arguments to cxx_constant_value_sfinae if we still want a constant value.
> 
> Perhaps we also still want to set tf_no_cleanup if object is nonnull, since
> in that case we know the prvalue is going to be an initializer rather than a
> temporary.

This passed bootstrap/regtest on i686-linux and bootstrap on
x86_64-linux (regtest still pending):

2022-01-18  Jakub Jelinek  <jakub@redhat.com>

	PR c++/104055
	* constexpr.cc (cxx_eval_outermost_constant_expr): If t is a
	TARGET_EXPR with TARGET_EXPR_CLEANUP, use get_target_expr rather
	than get_target_expr_sfinae with tf_no_cleanup, and don't set
	TREE_CONSTANT.

	* g++.dg/cpp2a/consteval27.C: New test.



	Jakub

On 1/18/22 18:07, Jakub Jelinek wrote:
> On Tue, Jan 18, 2022 at 11:51:31AM -0500, Jason Merrill wrote:
>> On 1/18/22 09:26, Jakub Jelinek wrote:
>>> On Tue, Jan 18, 2022 at 09:08:03AM -0500, Jason Merrill wrote:
>>>>> which makes call a TARGET_EXPR with the dtor in TARGET_EXPR_CLEANUP,
>>>>> but then call cxx_constant_value on it.  In cxx_eval_outermost_constant_expr
>>>>> it triggers the:
>>>>>          else if (TREE_CODE (t) != CONSTRUCTOR)
>>>>>            {
>>>>>              r = get_target_expr_sfinae (r, tf_warning_or_error | tf_no_cleanup);
>>>>>              TREE_CONSTANT (r) = true;
>>>>>            }
>>>>> which wraps the CONSTRUCTOR r into a new TARGET_EXPR, but one without
>>>>> dtors (I think we need e.g. the TREE_CONSTANT for the callers),
>>>>> and finally build_over_call uses that.
>>>>
>>>> Looks like you added the tf_no_cleanup in r10-3661 (constexpr new). Maybe
>>>> that should only be added if TARGET_EXPR_CLEANUP (t) is null?
>>>
>>> I thought about that, but I'm worried that if the cleanup action
>>> is non-empty, then the TREE_CONSTANT (r) = true; after it isn't appropriate,
>>> because the TARGET_EXPR has side-effects.
>>
>> Hmm.  If we e.g. pull out a subobject value from the TARGET_EXPR temporary,
>> we still need to run the cleanup, so we don't want to optimize the temporary
>> away, so not setting TREE_CONSTANT sounds right.
>>
>>> And the callers like cxx_constant_value_sfinae just return error_mark_node
>>> if it isn't set:
>>>     if (sfinae && !TREE_CONSTANT (r))
>>>       r = error_mark_node;
>>
>> Yes.  Is that wrong?  I guess we need to be more careful about adjusting the
>> arguments to cxx_constant_value_sfinae if we still want a constant value.
>>
>> Perhaps we also still want to set tf_no_cleanup if object is nonnull, since
>> in that case we know the prvalue is going to be an initializer rather than a
>> temporary.
> 
> This passed bootstrap/regtest on i686-linux and bootstrap on
> x86_64-linux (regtest still pending):

OK if it passes.

> 2022-01-18  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/104055
> 	* constexpr.cc (cxx_eval_outermost_constant_expr): If t is a
> 	TARGET_EXPR with TARGET_EXPR_CLEANUP, use get_target_expr rather
> 	than get_target_expr_sfinae with tf_no_cleanup, and don't set
> 	TREE_CONSTANT.
> 
> 	* g++.dg/cpp2a/consteval27.C: New test.
> 
> --- gcc/cp/constexpr.cc.jj	2022-01-18 11:58:59.296986142 +0100
> +++ gcc/cp/constexpr.cc	2022-01-18 20:07:28.362485336 +0100
> @@ -7821,7 +7821,11 @@ cxx_eval_outermost_constant_expr (tree t
>         if (TREE_CODE (t) == TARGET_EXPR
>   	  && TARGET_EXPR_INITIAL (t) == r)
>   	return t;
> -      else if (TREE_CODE (t) != CONSTRUCTOR)
> +      else if (TREE_CODE (t) == CONSTRUCTOR)
> +	;
> +      else if (TREE_CODE (t) == TARGET_EXPR && TARGET_EXPR_CLEANUP (t))
> +	r = get_target_expr (r);
> +      else
>   	{
>   	  r = get_target_expr_sfinae (r, tf_warning_or_error | tf_no_cleanup);
>   	  TREE_CONSTANT (r) = true;
> --- gcc/testsuite/g++.dg/cpp2a/consteval27.C.jj	2022-01-18 20:12:55.326006216 +0100
> +++ gcc/testsuite/g++.dg/cpp2a/consteval27.C	2022-01-18 20:13:01.442921759 +0100
> @@ -0,0 +1,18 @@
> +// PR c++/104055
> +// { dg-do run { target c++20 } }
> +
> +int g;
> +
> +struct A {
> +  ~A () { if (a != 17 || b != 26) __builtin_abort (); g = 42; }
> +  consteval A () : a (17), b (26) {}
> +  int a, b;
> +};
> +
> +int
> +main ()
> +{
> +  A{};
> +  if (g != 42)
> +    __builtin_abort ();
> +}
> 
> 
> 	Jakub
>