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

  Hi!

The following testcase is miscompiled.  We see the constructor is immediate,
in build_over_call we trigger:
          if (obj_arg && is_dummy_object (obj_arg))
            {
              call = build_cplus_new (DECL_CONTEXT (fndecl), call, complain);
              obj_arg = NULL_TREE;
            }
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.

The following patch fixes that by reusing the earlier TARGET_EXPR and
overwriting TARGET_EXPR_INITIAL in it; I think the TARGET_EXPR_SLOT can't
appear in the TARGET_EXPR_INITIAL, because the CONSTRUCTOR has been built
already before get_target_expr_sfinae has been called and addresses of
an automatic var aren't constant expression.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

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

	PR c++/104055
	* call.c (build_over_call): If in a call to consteval ctor call
	is a TARGET_EXPR with cleanups and cxx_constant_value returns
	a different TARGET_EXPR, reuse the original TARGET_EXPR with
	the new TARGET_EXPR_INITIAL.

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


	Jakub
  

On 1/18/22 05:10, Jakub Jelinek wrote:
> Hi!
> 
> The following testcase is miscompiled.  We see the constructor is immediate,
> in build_over_call we trigger:
>            if (obj_arg && is_dummy_object (obj_arg))
>              {
>                call = build_cplus_new (DECL_CONTEXT (fndecl), call, complain);
>                obj_arg = NULL_TREE;
>              }
> 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?

> The following patch fixes that by reusing the earlier TARGET_EXPR and
> overwriting TARGET_EXPR_INITIAL in it; I think the TARGET_EXPR_SLOT can't
> appear in the TARGET_EXPR_INITIAL, because the CONSTRUCTOR has been built
> already before get_target_expr_sfinae has been called and addresses of
> an automatic var aren't constant expression.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> 2022-01-18  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/104055
> 	* call.c (build_over_call): If in a call to consteval ctor call
> 	is a TARGET_EXPR with cleanups and cxx_constant_value returns
> 	a different TARGET_EXPR, reuse the original TARGET_EXPR with
> 	the new TARGET_EXPR_INITIAL.
> 
> 	* g++.dg/cpp2a/consteval27.C: New test.
> 
> --- gcc/cp/call.c.jj	2022-01-11 23:11:22.049294946 +0100
> +++ gcc/cp/call.c	2022-01-17 11:48:50.272044412 +0100
> @@ -9935,7 +9935,14 @@ build_over_call (struct z_candidate *can
>   		    obj_arg = TREE_OPERAND (addr, 0);
>   		}
>   	    }
> -	  call = cxx_constant_value (call, obj_arg);
> +	  tree cst_call = cxx_constant_value (call, obj_arg);
> +	  if (call != cst_call
> +	      && TREE_CODE (call) == TARGET_EXPR
> +	      && TARGET_EXPR_CLEANUP (call)
> +	      && TREE_CODE (cst_call) == TARGET_EXPR)
> +	    TARGET_EXPR_INITIAL (call) = TARGET_EXPR_INITIAL (cst_call);
> +	  else
> +	    call = cst_call;
>   	  if (obj_arg && !error_operand_p (call))
>   	    call = build2 (INIT_EXPR, void_type_node, obj_arg, call);
>   	  call = convert_from_reference (call);
> --- gcc/testsuite/g++.dg/cpp2a/consteval27.C.jj	2022-01-17 11:54:09.656459199 +0100
> +++ gcc/testsuite/g++.dg/cpp2a/consteval27.C	2022-01-17 11:54:37.035068677 +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
>
  

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.
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;

	Jakub
  

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.

Jason