c++: Fix constexpr evaluation of parameters passed by invisible reference [PR111284]

Hi!

My r9-6136 changes to make a copy of constexpr function bodies before
genericization modifies it broke the constant evaluation of non-POD
arguments passed by value.
In the callers such arguments are passed as reference to usually a
TARGET_EXPR, but on the callee side until genericization they are just
direct uses of a PARM_DECL with some class type.
In cxx_bind_parameters_in_call I've used convert_from_reference to
pretend it is passed by value and then cxx_eval_constant_expression
is called there and evaluates that as an rvalue, followed by
adjust_temp_type if the types don't match exactly (e.g. const Foo
argument and passing to it reference to Foo TARGET_EXPR).

The reason this doesn't work is that when the TARGET_EXPR in the caller
is constant initialized, this for it is the address of the TARGET_EXPR_SLOT,
but if the code later on pretends the PARM_DECL is just initialized to the
rvalue of the constant evaluation of the TARGET_EXPR, it is as if there
is a bitwise copy of the TARGET_EXPR to the callee, so this in the callee
is then address of the PARM_DECL in the callee.

The following patch attempts to fix that by constexpr evaluation of such
arguments in the caller as an lvalue instead of rvalue, and on the callee
side when seeing such a PARM_DECL, if we want an lvalue, lookup the value
(lvalue) saved in ctx->globals (if any), and if wanting an rvalue,
recursing with vc_prvalue on the looked up value (because it is there
as an lvalue, nor rvalue).

adjust_temp_type doesn't work for lvalues of non-scalarish types, for
such types it relies on changing the type of a CONSTRUCTOR, but on the
other side we know what we pass to the argument is addressable, so
the patch on type mismatch takes address of the argument value, casts
to reference to the desired type and dereferences it.

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

2024-03-08  Jakub Jelinek  <jakub@redhat.com>

	PR c++/111284
	* constexpr.cc (cxx_bind_parameters_in_call): For PARM_DECLs with
	TREE_ADDRESSABLE types use vc_glvalue rather than vc_prvalue for
	cxx_eval_constant_expression and if it doesn't have the same
	type as it should, cast the reference type to reference to type
	before convert_from_reference and instead of adjust_temp_type
	take address of the arg, cast to reference to type and then
	convert_from_reference.
	(cxx_eval_constant_expression) <case PARM_DECL>: For lval case
	on parameters with TREE_ADDRESSABLE types lookup result in
	ctx->globals if possible.  Otherwise if lookup in ctx->globals
	was successful for parameter with TREE_ADDRESSABLE type,
	recurse with vc_prvalue on the returned value.

	* g++.dg/cpp1z/constexpr-111284.C: New test.
	* g++.dg/cpp1y/constexpr-lifetime7.C: Expect one error on a different
	line.


	Jakub

Hi!

I'd like to ping the
https://gcc.gnu.org/pipermail/gcc-patches/2024-March/647445.html
PR111284 P2 patch.

Thanks.

	Jakub

On 3/8/24 03:56, Jakub Jelinek wrote:
> Hi!
> 
> My r9-6136 changes to make a copy of constexpr function bodies before
> genericization modifies it broke the constant evaluation of non-POD
> arguments passed by value.
> In the callers such arguments are passed as reference to usually a
> TARGET_EXPR, but on the callee side until genericization they are just
> direct uses of a PARM_DECL with some class type.
> In cxx_bind_parameters_in_call I've used convert_from_reference to
> pretend it is passed by value and then cxx_eval_constant_expression
> is called there and evaluates that as an rvalue, followed by
> adjust_temp_type if the types don't match exactly (e.g. const Foo
> argument and passing to it reference to Foo TARGET_EXPR).
> 
> The reason this doesn't work is that when the TARGET_EXPR in the caller
> is constant initialized, this for it is the address of the TARGET_EXPR_SLOT,
> but if the code later on pretends the PARM_DECL is just initialized to the
> rvalue of the constant evaluation of the TARGET_EXPR, it is as if there
> is a bitwise copy of the TARGET_EXPR to the callee, so this in the callee
> is then address of the PARM_DECL in the callee.
> 
> The following patch attempts to fix that by constexpr evaluation of such
> arguments in the caller as an lvalue instead of rvalue, and on the callee
> side when seeing such a PARM_DECL, if we want an lvalue, lookup the value
> (lvalue) saved in ctx->globals (if any), and if wanting an rvalue,
> recursing with vc_prvalue on the looked up value (because it is there
> as an lvalue, nor rvalue).
> 
> adjust_temp_type doesn't work for lvalues of non-scalarish types, for
> such types it relies on changing the type of a CONSTRUCTOR, but on the
> other side we know what we pass to the argument is addressable, so
> the patch on type mismatch takes address of the argument value, casts
> to reference to the desired type and dereferences it.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> 2024-03-08  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/111284
> 	* constexpr.cc (cxx_bind_parameters_in_call): For PARM_DECLs with
> 	TREE_ADDRESSABLE types use vc_glvalue rather than vc_prvalue for
> 	cxx_eval_constant_expression and if it doesn't have the same
> 	type as it should, cast the reference type to reference to type
> 	before convert_from_reference and instead of adjust_temp_type
> 	take address of the arg, cast to reference to type and then
> 	convert_from_reference.
> 	(cxx_eval_constant_expression) <case PARM_DECL>: For lval case
> 	on parameters with TREE_ADDRESSABLE types lookup result in
> 	ctx->globals if possible.  Otherwise if lookup in ctx->globals
> 	was successful for parameter with TREE_ADDRESSABLE type,
> 	recurse with vc_prvalue on the returned value.
> 
> 	* g++.dg/cpp1z/constexpr-111284.C: New test.
> 	* g++.dg/cpp1y/constexpr-lifetime7.C: Expect one error on a different
> 	line.
> 
> --- gcc/cp/constexpr.cc.jj	2024-02-13 10:29:57.979155641 +0100
> +++ gcc/cp/constexpr.cc	2024-03-07 19:35:01.032412221 +0100
> @@ -1877,13 +1877,21 @@ cxx_bind_parameters_in_call (const const
>   	  x = build_address (x);
>   	}
>         if (TREE_ADDRESSABLE (type))
> -	/* Undo convert_for_arg_passing work here.  */
> -	x = convert_from_reference (x);
> -      /* Normally we would strip a TARGET_EXPR in an initialization context
> -	 such as this, but here we do the elision differently: we keep the
> -	 TARGET_EXPR, and use its CONSTRUCTOR as the value of the parm.  */
> -      arg = cxx_eval_constant_expression (ctx, x, vc_prvalue,
> -					  non_constant_p, overflow_p);
> +	{
> +	  /* Undo convert_for_arg_passing work here.  */
> +	  if (TYPE_REF_P (TREE_TYPE (x))
> +	      && !same_type_p (type, TREE_TYPE (TREE_TYPE (x))))
> +	    x = cp_fold_convert (build_reference_type (type), x);
> +	  x = convert_from_reference (x);
> +	  arg = cxx_eval_constant_expression (ctx, x, vc_glvalue,
> +					      non_constant_p, overflow_p);
> +	}
> +      else
> +	/* Normally we would strip a TARGET_EXPR in an initialization context
> +	   such as this, but here we do the elision differently: we keep the
> +	   TARGET_EXPR, and use its CONSTRUCTOR as the value of the parm.  */
> +	arg = cxx_eval_constant_expression (ctx, x, vc_prvalue,
> +					    non_constant_p, overflow_p);

It seems simpler to move the convert_for_reference after the 
cxx_eval_constant_expression rather than duplicate the call to 
cxx_eval_constant_expression.

>         /* Check we aren't dereferencing a null pointer when calling a non-static
>   	 member function, which is undefined behaviour.  */
>         if (i == 0 && DECL_OBJECT_MEMBER_FUNCTION_P (fun)
> @@ -1909,7 +1917,16 @@ cxx_bind_parameters_in_call (const const
>   	{
>   	  /* Make sure the binding has the same type as the parm.  But
>   	     only for constant args.  */
> -	  if (!TYPE_REF_P (type))
> +	  if (TREE_ADDRESSABLE (type))
> +	    {
> +	      if (!same_type_p (type, TREE_TYPE (arg)))
> +		{
> +		  arg = build_fold_addr_expr (arg);
> +		  arg = cp_fold_convert (build_reference_type (type), arg);
> +		  arg = convert_from_reference (arg);
> +		}
> +	    }

It shouldn't be necessary to convert both here and above?  In fact, the 
testcase still passes with neither conversion, just moving the 
convert_for_reference and the change below.

> +	  else if (!TYPE_REF_P (type))
>   	    arg = adjust_temp_type (type, arg);
>   	  if (!TREE_CONSTANT (arg))
>   	    *non_constant_args = true;
> @@ -7499,9 +7516,19 @@ cxx_eval_constant_expression (const cons
>   
>       case PARM_DECL:
>         if (lval && !TYPE_REF_P (TREE_TYPE (t)))
> -	/* glvalue use.  */;
> +	{
> +	  /* glvalue use.  */
> +	  if (TREE_ADDRESSABLE (TREE_TYPE (t)))
> +	    if (tree v = ctx->global->get_value (t))
> +	      r = v;
> +	}
>         else if (tree v = ctx->global->get_value (t))
> -	r = v;
> +	{
> +	  r = v;
> +	  if (TREE_ADDRESSABLE (TREE_TYPE (t)))
> +	    r = cxx_eval_constant_expression (ctx, r, vc_prvalue,
> +					      non_constant_p, overflow_p);
> +	}
>         else if (lval)
>   	/* Defer in case this is only used for its type.  */;
>         else if (ctx->global->is_outside_lifetime (t))
> --- gcc/testsuite/g++.dg/cpp1z/constexpr-111284.C.jj	2024-03-07 16:27:48.113651999 +0100
> +++ gcc/testsuite/g++.dg/cpp1z/constexpr-111284.C	2024-03-07 16:26:49.565466606 +0100
> @@ -0,0 +1,19 @@
> +// PR c++/111284
> +// { dg-do compile { target c++17 } }
> +
> +struct S {
> +  S () = default;
> +  constexpr S (const S &) noexcept : s{this} {}
> +  constexpr S & operator= (const S &) noexcept { return *this; }
> +  constexpr bool foo () const noexcept { return s == this; }
> +  S *s = this;
> +};
> +
> +constexpr bool
> +bar (S x) noexcept
> +{
> +  return x.foo ();
> +}
> +
> +static_assert (bar (S {}), "");
> +static_assert ([] (S x) { return x.foo (); } (S {}), "");
> --- gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime7.C.jj	2023-12-13 19:09:33.252657826 +0100
> +++ gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime7.C	2024-03-07 19:49:49.342334090 +0100
> @@ -87,7 +87,7 @@ constexpr bool n1 = test_access<NonTrivi
>   constexpr bool n2 = test_modification<NonTrivial>();  // { dg-message "in .constexpr." "" { target c++20 } }
>   constexpr bool n3 = test_scope<NonTrivial>();         // { dg-message "in .constexpr." "" { target c++20 } }
>   constexpr bool n4 = test_destroy_temp<NonTrivial>();  // { dg-message "in .constexpr." "" { target c++20 } }
> -constexpr bool n5 = test_parameter(NonTrivial{});     // { dg-error "destroying" "" { target c++20 } }
> +constexpr bool n5 = test_parameter(NonTrivial{});     // { dg-message "in .constexpr." "" { target c++20 } }
>   constexpr bool n6 = test_bindings<NonTrivial>();
>   #endif
>   
> 
> 	Jakub
>

On Fri, Apr 12, 2024 at 03:05:26PM -0400, Jason Merrill wrote:
> > --- gcc/cp/constexpr.cc.jj	2024-02-13 10:29:57.979155641 +0100
> > +++ gcc/cp/constexpr.cc	2024-03-07 19:35:01.032412221 +0100
> > @@ -1877,13 +1877,21 @@ cxx_bind_parameters_in_call (const const
> >   	  x = build_address (x);
> >   	}
> >         if (TREE_ADDRESSABLE (type))
> > -	/* Undo convert_for_arg_passing work here.  */
> > -	x = convert_from_reference (x);
> > -      /* Normally we would strip a TARGET_EXPR in an initialization context
> > -	 such as this, but here we do the elision differently: we keep the
> > -	 TARGET_EXPR, and use its CONSTRUCTOR as the value of the parm.  */
> > -      arg = cxx_eval_constant_expression (ctx, x, vc_prvalue,
> > -					  non_constant_p, overflow_p);
> > +	{
> > +	  /* Undo convert_for_arg_passing work here.  */
> > +	  if (TYPE_REF_P (TREE_TYPE (x))
> > +	      && !same_type_p (type, TREE_TYPE (TREE_TYPE (x))))
> > +	    x = cp_fold_convert (build_reference_type (type), x);
> > +	  x = convert_from_reference (x);
> > +	  arg = cxx_eval_constant_expression (ctx, x, vc_glvalue,
> > +					      non_constant_p, overflow_p);
> > +	}
> > +      else
> > +	/* Normally we would strip a TARGET_EXPR in an initialization context
> > +	   such as this, but here we do the elision differently: we keep the
> > +	   TARGET_EXPR, and use its CONSTRUCTOR as the value of the parm.  */
> > +	arg = cxx_eval_constant_expression (ctx, x, vc_prvalue,
> > +					    non_constant_p, overflow_p);
> 
> It seems simpler to move the convert_for_reference after the
> cxx_eval_constant_expression rather than duplicate the call to
> cxx_eval_constant_expression.

They weren't the same, one was trying to evaluate the convert_from_reference
with vc_glvalue, the other evaluates it without that and with vc_prvalue.
Now, I guess the
+     /* Undo convert_for_arg_passing work here.  */
+     if (TYPE_REF_P (TREE_TYPE (x))
+         && !same_type_p (type, TREE_TYPE (TREE_TYPE (x))))
+       x = cp_fold_convert (build_reference_type (type), x);
part could be thrown away, given the other !same_type_p check (that one is
needed because adjust_temp_type can't deal with that), at least
when I remove that
GXX_TESTSUITE_STDS=98,11,14,17,20,23,26 make check-g++ RUNTESTFLAGS="dg.exp='constexpr-dtor* pr114426.C constexpr-111284.C constexpr-lifetime7.C'"
still passes.  But if I try to remove even more, like in the patch below,
then I see
FAIL: g++.dg/cpp2a/constexpr-dtor5.C  -std=c++20 (test for excess errors)
FAIL: g++.dg/cpp2a/constexpr-dtor5.C  -std=c++23 (test for excess errors)
FAIL: g++.dg/cpp2a/constexpr-dtor5.C  -std=c++26 (test for excess errors)
FAIL: g++.dg/cpp2a/constexpr-dtor7.C  -std=c++20  (test for errors, line 6)
FAIL: g++.dg/cpp2a/constexpr-dtor7.C  -std=c++20 (test for excess errors)
FAIL: g++.dg/cpp2a/constexpr-dtor7.C  -std=c++23  (test for errors, line 6)
FAIL: g++.dg/cpp2a/constexpr-dtor7.C  -std=c++23 (test for excess errors)
FAIL: g++.dg/cpp2a/constexpr-dtor7.C  -std=c++26  (test for errors, line 6)
FAIL: g++.dg/cpp2a/constexpr-dtor7.C  -std=c++26 (test for excess errors)
regressions.
E.g.
.../g++.dg/cpp2a/constexpr-dtor5.C:30:20: error: non-constant condition for static assertion
.../g++.dg/cpp2a/constexpr-dtor5.C:13:7: error: modification of '<anonymous>' from outside current evaluation is not a constant expression
.../g++.dg/cpp2a/constexpr-dtor5.C:31:20: error: non-constant condition for static assertion
.../g++.dg/cpp2a/constexpr-dtor5.C:13:7: error: modification of '<anonymous>' from outside current evaluation is not a constant expression
.../g++.dg/cpp2a/constexpr-dtor5.C:32:20: error: non-constant condition for static assertion
.../g++.dg/cpp2a/constexpr-dtor5.C:13:7: error: modification of '<anonymous>' from outside current evaluation is not a constant expression
.../g++.dg/cpp2a/constexpr-dtor5.C:13:7: error: modification of '<anonymous>' from outside current evaluation is not a constant expression
.../g++.dg/cpp2a/constexpr-dtor5.C:13:7: error: modification of '<anonymous>' from outside current evaluation is not a constant expression
.../g++.dg/cpp2a/constexpr-dtor5.C:13:7: error: modification of '<anonymous>' from outside current evaluation is not a constant expression

--- gcc/cp/constexpr.cc.jj	2024-02-13 10:29:57.979155641 +0100
+++ gcc/cp/constexpr.cc	2024-03-07 19:35:01.032412221 +0100
@@ -1871,9 +1871,6 @@ cxx_bind_parameters_in_call (const const
 	  x = ctx->object;
 	  x = build_address (x);
 	}
-      if (TREE_ADDRESSABLE (type))
-	/* Undo convert_for_arg_passing work here.  */
-	x = convert_from_reference (x);
       /* Normally we would strip a TARGET_EXPR in an initialization context
 	 such as this, but here we do the elision differently: we keep the
 	 TARGET_EXPR, and use its CONSTRUCTOR as the value of the parm.  */
@@ -1904,7 +1901,13 @@ cxx_bind_parameters_in_call (const const
 	{
 	  /* Make sure the binding has the same type as the parm.  But
 	     only for constant args.  */
-	  if (!TYPE_REF_P (type))
+	  if (TREE_ADDRESSABLE (type))
+	    {
+	      if (!same_type_p (type, TREE_TYPE (TREE_TYPE (arg))))
+		arg = cp_fold_convert (build_reference_type (type), arg);
+	      arg = convert_from_reference (arg);
+	    }
+	  else if (!TYPE_REF_P (type))
 	    arg = adjust_temp_type (type, arg);
 	  if (!TREE_CONSTANT (arg))
 	    *non_constant_args = true;
@@ -7494,9 +7497,19 @@ cxx_eval_constant_expression (const cons
 
     case PARM_DECL:
       if (lval && !TYPE_REF_P (TREE_TYPE (t)))
-	/* glvalue use.  */;
+	{
+	  /* glvalue use.  */
+	  if (TREE_ADDRESSABLE (TREE_TYPE (t)))
+	    if (tree v = ctx->global->get_value (t))
+	      r = v;
+	}
       else if (tree v = ctx->global->get_value (t))
-	r = v;
+	{
+	  r = v;
+	  if (TREE_ADDRESSABLE (TREE_TYPE (t)))
+	    r = cxx_eval_constant_expression (ctx, r, vc_prvalue,
+					      non_constant_p, overflow_p);
+	}
       else if (lval)
 	/* Defer in case this is only used for its type.  */;
       else if (ctx->global->is_outside_lifetime (t))

	Jakub