c++: constantness of local var in constexpr fn [PR111703, PR112269]

Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
trunk?  Does it look OK for release branches as well for sake of PR111703?

-- >8 --

potential_constant_expression was incorrectly treating most local
variables from a constexpr function as (potentially) constant because it
wasn't considering the 'now' parameter.  This patch fixes this by
relaxing some var_in_maybe_constexpr_fn checks accordingly, which turns
out to partially fix two recently reported regressions:

PR111703 is a regression caused by r11-550-gf65a3299a521a4 for
restricting constexpr evaluation during warning-dependent folding.
The mechanism is intended to restrict only constant evaluation of the
instantiated non-dependent expression, but it also ends up restricting
constant evaluation (as part of satisfaction) during instantiation of
the expression, in particular when resolving the ck_rvalue conversion of
the 'x' argument into a copy constructor call.  This seems like a bug in
the mechanism[1], though I don't know if we want to refine the mechanism
or get rid of it completely since the original testcases which motivated
the mechanism are fixed more simply by r13-1225-gb00b95198e6720.  In any
case, this patch partially fixes this by making us correctly treat 'x'
and therefore 'f(x)' in the below testcase as non-constant, which
prevents the problematic warning-dependent folding from occurring at
all.  If this bug crops up again then I figure we could decide what to
do with the mechanism then.

PR112269 is caused by r14-4796-g3e3d73ed5e85e7 for merging tsubst_copy
into tsubst_copy_and_build.  tsubst_copy used to exit early when 'args'
was empty, behavior which that commit deliberately didn't preserve.
This early exit masked the fact that COMPLEX_EXPR wasn't handled by
tsubst at all, and is a tree code that apparently we could see during
warning-dependent folding on some targets.  A complete fix is to add
handling for this tree code in tsubst_expr, but this patch should fix
the reported testsuite failures since the situations where COMPLEX_EXPR
crops up in <complex> turn out to not be constant expressions in the
first place after this patch.

[1]: The mechanism incorrectly assumes that instantiation of the
non-dependent expression shouldn't induce any template instantiation
since ahead of time checking of the expression should've already induced
whatever template instantiation was needed, but in this case although
overload resolution was performed ahead of time, a ck_rvalue conversion
gets resolved to a copy constructor call only at instantiation time.

	PR c++/111703

gcc/cp/ChangeLog:

	* constexpr.cc (potential_constant_expression_1) <case VAR_DECL>:
	Only consider var_in_maybe_constexpr_fn if 'now' is false.
	<case INDIRECT_REF>: Likewise.

gcc/testsuite/ChangeLog:

	* g++.dg/cpp2a/concepts-fn8.C: New test.
---
 gcc/cp/constexpr.cc                       |  4 ++--
 gcc/testsuite/g++.dg/cpp2a/concepts-fn8.C | 24 +++++++++++++++++++++++
 2 files changed, 26 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-fn8.C

On Tue, 31 Oct 2023, Patrick Palka wrote:

> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> trunk?  Does it look OK for release branches as well for sake of PR111703?
> 
> -- >8 --
> 
> potential_constant_expression was incorrectly treating most local
> variables from a constexpr function as (potentially) constant because it
> wasn't considering the 'now' parameter.  This patch fixes this by
> relaxing some var_in_maybe_constexpr_fn checks accordingly, which turns
> out to partially fix two recently reported regressions:
> 
> PR111703 is a regression caused by r11-550-gf65a3299a521a4 for
> restricting constexpr evaluation during warning-dependent folding.
> The mechanism is intended to restrict only constant evaluation of the
> instantiated non-dependent expression, but it also ends up restricting
> constant evaluation (as part of satisfaction) during instantiation of
> the expression, in particular when resolving the ck_rvalue conversion of
> the 'x' argument into a copy constructor call.

Oops, this analysis is inaccurate for this specific testcase (although
the general idea is the same)...  We don't call fold_for_warn on 'f(x)'
but rather on its 'x' argument that has been processed by
convert_arguments into an IMPLICIT_CONV_EXPR.  And it's the
instantiation of this IMPLICIT_CONV_EXPR that turns it into a copy
constructor call.  There is no ck_rvalue conversion at all here since
'f' is a function pointer, not an actual function, and so ICSes don't
get computed (IIUC).  If 'f' is changed to be an actual function then
there's no issue since build_over_call doesn't perform argument
conversions when in a template context and therefore doesn't call
check_function_arguments on the converted arguments (from which the
problematic fold_for_warn call occurs).

> This seems like a bug in
> the mechanism[1], though I don't know if we want to refine the mechanism
> or get rid of it completely since the original testcases which motivated
> the mechanism are fixed more simply by r13-1225-gb00b95198e6720.  In any
> case, this patch partially fixes this by making us correctly treat 'x'
> and therefore 'f(x)' in the below testcase as non-constant, which
> prevents the problematic warning-dependent folding from occurring at
> all.  If this bug crops up again then I figure we could decide what to
> do with the mechanism then.
> 
> PR112269 is caused by r14-4796-g3e3d73ed5e85e7 for merging tsubst_copy
> into tsubst_copy_and_build.  tsubst_copy used to exit early when 'args'
> was empty, behavior which that commit deliberately didn't preserve.
> This early exit masked the fact that COMPLEX_EXPR wasn't handled by
> tsubst at all, and is a tree code that apparently we could see during
> warning-dependent folding on some targets.  A complete fix is to add
> handling for this tree code in tsubst_expr, but this patch should fix
> the reported testsuite failures since the situations where COMPLEX_EXPR
> crops up in <complex> turn out to not be constant expressions in the
> first place after this patch.
> 
> [1]: The mechanism incorrectly assumes that instantiation of the
> non-dependent expression shouldn't induce any template instantiation
> since ahead of time checking of the expression should've already induced
> whatever template instantiation was needed, but in this case although
> overload resolution was performed ahead of time, a ck_rvalue conversion
> gets resolved to a copy constructor call only at instantiation time.
> 
> 	PR c++/111703
> 
> gcc/cp/ChangeLog:
> 
> 	* constexpr.cc (potential_constant_expression_1) <case VAR_DECL>:
> 	Only consider var_in_maybe_constexpr_fn if 'now' is false.
> 	<case INDIRECT_REF>: Likewise.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/cpp2a/concepts-fn8.C: New test.
> ---
>  gcc/cp/constexpr.cc                       |  4 ++--
>  gcc/testsuite/g++.dg/cpp2a/concepts-fn8.C | 24 +++++++++++++++++++++++
>  2 files changed, 26 insertions(+), 2 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-fn8.C
> 
> diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc
> index c05760e6789..8a6b210144a 100644
> --- a/gcc/cp/constexpr.cc
> +++ b/gcc/cp/constexpr.cc
> @@ -9623,7 +9623,7 @@ potential_constant_expression_1 (tree t, bool want_rval, bool strict, bool now,
>  	  return RECUR (DECL_VALUE_EXPR (t), rval);
>  	}
>        if (want_rval
> -	  && !var_in_maybe_constexpr_fn (t)
> +	  && (now || !var_in_maybe_constexpr_fn (t))
>  	  && !type_dependent_expression_p (t)
>  	  && !decl_maybe_constant_var_p (t)
>  	  && (strict
> @@ -9737,7 +9737,7 @@ potential_constant_expression_1 (tree t, bool want_rval, bool strict, bool now,
>          STRIP_NOPS (x);
>          if (is_this_parameter (x) && !is_capture_proxy (x))
>  	  {
> -	    if (!var_in_maybe_constexpr_fn (x))
> +	    if (now || !var_in_maybe_constexpr_fn (x))
>  	      {
>  		if (flags & tf_error)
>  		  constexpr_error (loc, fundef_p, "use of %<this%> in a "
> diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-fn8.C b/gcc/testsuite/g++.dg/cpp2a/concepts-fn8.C
> new file mode 100644
> index 00000000000..3f63a5b28d7
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-fn8.C
> @@ -0,0 +1,24 @@
> +// PR c++/111703
> +// { dg-do compile { target c++20 } }
> +
> +template<class T>
> +constexpr bool always_true() { return true; }
> +
> +struct P {
> +  P() = default;
> +
> +  template<class T>
> +    requires (always_true<T>()) // { dg-bogus "used before its definition" }
> +  constexpr P(const T&) { }
> +
> +  int n, m;
> +};
> +
> +void (*f)(P);
> +
> +template<class T>
> +constexpr bool g() {
> +  P x;
> +  f(x); // { dg-bogus "from here" }
> +  return true;
> +}
> -- 
> 2.42.0.526.g3130c155df
> 
>

On Wed, 1 Nov 2023, Patrick Palka wrote:

> On Tue, 31 Oct 2023, Patrick Palka wrote:
> 
> > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> > trunk?  Does it look OK for release branches as well for sake of PR111703?

Ping.

> > 
> > -- >8 --
> > 
> > potential_constant_expression was incorrectly treating most local
> > variables from a constexpr function as (potentially) constant because it
> > wasn't considering the 'now' parameter.  This patch fixes this by
> > relaxing some var_in_maybe_constexpr_fn checks accordingly, which turns
> > out to partially fix two recently reported regressions:
> > 
> > PR111703 is a regression caused by r11-550-gf65a3299a521a4 for
> > restricting constexpr evaluation during warning-dependent folding.
> > The mechanism is intended to restrict only constant evaluation of the
> > instantiated non-dependent expression, but it also ends up restricting
> > constant evaluation (as part of satisfaction) during instantiation of
> > the expression, in particular when resolving the ck_rvalue conversion of
> > the 'x' argument into a copy constructor call.
> 
> Oops, this analysis is inaccurate for this specific testcase (although
> the general idea is the same)...  We don't call fold_for_warn on 'f(x)'
> but rather on its 'x' argument that has been processed by
> convert_arguments into an IMPLICIT_CONV_EXPR.  And it's the
> instantiation of this IMPLICIT_CONV_EXPR that turns it into a copy
> constructor call.  There is no ck_rvalue conversion at all here since
> 'f' is a function pointer, not an actual function, and so ICSes don't
> get computed (IIUC).  If 'f' is changed to be an actual function then
> there's no issue since build_over_call doesn't perform argument
> conversions when in a template context and therefore doesn't call
> check_function_arguments on the converted arguments (from which the
> problematic fold_for_warn call occurs).
> 
> > This seems like a bug in
> > the mechanism[1], though I don't know if we want to refine the mechanism
> > or get rid of it completely since the original testcases which motivated
> > the mechanism are fixed more simply by r13-1225-gb00b95198e6720.  In any
> > case, this patch partially fixes this by making us correctly treat 'x'
> > and therefore 'f(x)' in the below testcase as non-constant, which
> > prevents the problematic warning-dependent folding from occurring at
> > all.  If this bug crops up again then I figure we could decide what to
> > do with the mechanism then.
> > 
> > PR112269 is caused by r14-4796-g3e3d73ed5e85e7 for merging tsubst_copy
> > into tsubst_copy_and_build.  tsubst_copy used to exit early when 'args'
> > was empty, behavior which that commit deliberately didn't preserve.
> > This early exit masked the fact that COMPLEX_EXPR wasn't handled by
> > tsubst at all, and is a tree code that apparently we could see during
> > warning-dependent folding on some targets.  A complete fix is to add
> > handling for this tree code in tsubst_expr, but this patch should fix
> > the reported testsuite failures since the situations where COMPLEX_EXPR
> > crops up in <complex> turn out to not be constant expressions in the
> > first place after this patch.

N.B. adding COMPLEX_EXPR handling to tsubst_expr is complicated by the
fact that these COMPLEX_EXRRs are created by convert_to_complex (a
middle-end routine) which occasionally creates SAVE_EXPR sub trees which
we don't expect to see inside templated trees...

> > 
> > [1]: The mechanism incorrectly assumes that instantiation of the
> > non-dependent expression shouldn't induce any template instantiation
> > since ahead of time checking of the expression should've already induced
> > whatever template instantiation was needed, but in this case although
> > overload resolution was performed ahead of time, a ck_rvalue conversion
> > gets resolved to a copy constructor call only at instantiation time.
> > 
> > 	PR c++/111703
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	* constexpr.cc (potential_constant_expression_1) <case VAR_DECL>:
> > 	Only consider var_in_maybe_constexpr_fn if 'now' is false.
> > 	<case INDIRECT_REF>: Likewise.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	* g++.dg/cpp2a/concepts-fn8.C: New test.
> > ---
> >  gcc/cp/constexpr.cc                       |  4 ++--
> >  gcc/testsuite/g++.dg/cpp2a/concepts-fn8.C | 24 +++++++++++++++++++++++
> >  2 files changed, 26 insertions(+), 2 deletions(-)
> >  create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-fn8.C
> > 
> > diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc
> > index c05760e6789..8a6b210144a 100644
> > --- a/gcc/cp/constexpr.cc
> > +++ b/gcc/cp/constexpr.cc
> > @@ -9623,7 +9623,7 @@ potential_constant_expression_1 (tree t, bool want_rval, bool strict, bool now,
> >  	  return RECUR (DECL_VALUE_EXPR (t), rval);
> >  	}
> >        if (want_rval
> > -	  && !var_in_maybe_constexpr_fn (t)
> > +	  && (now || !var_in_maybe_constexpr_fn (t))
> >  	  && !type_dependent_expression_p (t)
> >  	  && !decl_maybe_constant_var_p (t)
> >  	  && (strict
> > @@ -9737,7 +9737,7 @@ potential_constant_expression_1 (tree t, bool want_rval, bool strict, bool now,
> >          STRIP_NOPS (x);
> >          if (is_this_parameter (x) && !is_capture_proxy (x))
> >  	  {
> > -	    if (!var_in_maybe_constexpr_fn (x))
> > +	    if (now || !var_in_maybe_constexpr_fn (x))
> >  	      {
> >  		if (flags & tf_error)
> >  		  constexpr_error (loc, fundef_p, "use of %<this%> in a "
> > diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-fn8.C b/gcc/testsuite/g++.dg/cpp2a/concepts-fn8.C
> > new file mode 100644
> > index 00000000000..3f63a5b28d7
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-fn8.C
> > @@ -0,0 +1,24 @@
> > +// PR c++/111703
> > +// { dg-do compile { target c++20 } }
> > +
> > +template<class T>
> > +constexpr bool always_true() { return true; }
> > +
> > +struct P {
> > +  P() = default;
> > +
> > +  template<class T>
> > +    requires (always_true<T>()) // { dg-bogus "used before its definition" }
> > +  constexpr P(const T&) { }
> > +
> > +  int n, m;
> > +};
> > +
> > +void (*f)(P);
> > +
> > +template<class T>
> > +constexpr bool g() {
> > +  P x;
> > +  f(x); // { dg-bogus "from here" }
> > +  return true;
> > +}
> > -- 
> > 2.42.0.526.g3130c155df
> > 
> > 
>

On 11/1/23 11:07, Patrick Palka wrote:
> On Tue, 31 Oct 2023, Patrick Palka wrote:
> 
>> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
>> trunk?  Does it look OK for release branches as well for sake of PR111703?
>>
>> -- >8 --
>>
>> potential_constant_expression was incorrectly treating most local
>> variables from a constexpr function as (potentially) constant because it
>> wasn't considering the 'now' parameter.  This patch fixes this by
>> relaxing some var_in_maybe_constexpr_fn checks accordingly, which turns
>> out to partially fix two recently reported regressions:
>>
>> PR111703 is a regression caused by r11-550-gf65a3299a521a4 for
>> restricting constexpr evaluation during warning-dependent folding.
>> The mechanism is intended to restrict only constant evaluation of the
>> instantiated non-dependent expression, but it also ends up restricting
>> constant evaluation (as part of satisfaction) during instantiation of
>> the expression, in particular when resolving the ck_rvalue conversion of
>> the 'x' argument into a copy constructor call.
> 
> Oops, this analysis is inaccurate for this specific testcase (although
> the general idea is the same)...  We don't call fold_for_warn on 'f(x)'
> but rather on its 'x' argument that has been processed by
> convert_arguments into an IMPLICIT_CONV_EXPR.  And it's the
> instantiation of this IMPLICIT_CONV_EXPR that turns it into a copy
> constructor call.  There is no ck_rvalue conversion at all here since
> 'f' is a function pointer, not an actual function, and so ICSes don't
> get computed (IIUC).  If 'f' is changed to be an actual function then
> there's no issue since build_over_call doesn't perform argument
> conversions when in a template context and therefore doesn't call
> check_function_arguments on the converted arguments (from which the
> problematic fold_for_warn call occurs).

Sounds like a we want to adjust a call to a function pointer to compute 
conversions without performing them in template context?

In any case, the patch is OK.

>> This seems like a bug in
>> the mechanism[1], though I don't know if we want to refine the mechanism
>> or get rid of it completely since the original testcases which motivated
>> the mechanism are fixed more simply by r13-1225-gb00b95198e6720.  In any
>> case, this patch partially fixes this by making us correctly treat 'x'
>> and therefore 'f(x)' in the below testcase as non-constant, which
>> prevents the problematic warning-dependent folding from occurring at
>> all.  If this bug crops up again then I figure we could decide what to
>> do with the mechanism then.
>>
>> PR112269 is caused by r14-4796-g3e3d73ed5e85e7 for merging tsubst_copy
>> into tsubst_copy_and_build.  tsubst_copy used to exit early when 'args'
>> was empty, behavior which that commit deliberately didn't preserve.
>> This early exit masked the fact that COMPLEX_EXPR wasn't handled by
>> tsubst at all, and is a tree code that apparently we could see during
>> warning-dependent folding on some targets.  A complete fix is to add
>> handling for this tree code in tsubst_expr, but this patch should fix
>> the reported testsuite failures since the situations where COMPLEX_EXPR
>> crops up in <complex> turn out to not be constant expressions in the
>> first place after this patch.
>>
>> [1]: The mechanism incorrectly assumes that instantiation of the
>> non-dependent expression shouldn't induce any template instantiation
>> since ahead of time checking of the expression should've already induced
>> whatever template instantiation was needed, but in this case although
>> overload resolution was performed ahead of time, a ck_rvalue conversion
>> gets resolved to a copy constructor call only at instantiation time.
>>
>> 	PR c++/111703
>>
>> gcc/cp/ChangeLog:
>>
>> 	* constexpr.cc (potential_constant_expression_1) <case VAR_DECL>:
>> 	Only consider var_in_maybe_constexpr_fn if 'now' is false.
>> 	<case INDIRECT_REF>: Likewise.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 	* g++.dg/cpp2a/concepts-fn8.C: New test.
>> ---
>>   gcc/cp/constexpr.cc                       |  4 ++--
>>   gcc/testsuite/g++.dg/cpp2a/concepts-fn8.C | 24 +++++++++++++++++++++++
>>   2 files changed, 26 insertions(+), 2 deletions(-)
>>   create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-fn8.C
>>
>> diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc
>> index c05760e6789..8a6b210144a 100644
>> --- a/gcc/cp/constexpr.cc
>> +++ b/gcc/cp/constexpr.cc
>> @@ -9623,7 +9623,7 @@ potential_constant_expression_1 (tree t, bool want_rval, bool strict, bool now,
>>   	  return RECUR (DECL_VALUE_EXPR (t), rval);
>>   	}
>>         if (want_rval
>> -	  && !var_in_maybe_constexpr_fn (t)
>> +	  && (now || !var_in_maybe_constexpr_fn (t))
>>   	  && !type_dependent_expression_p (t)
>>   	  && !decl_maybe_constant_var_p (t)
>>   	  && (strict
>> @@ -9737,7 +9737,7 @@ potential_constant_expression_1 (tree t, bool want_rval, bool strict, bool now,
>>           STRIP_NOPS (x);
>>           if (is_this_parameter (x) && !is_capture_proxy (x))
>>   	  {
>> -	    if (!var_in_maybe_constexpr_fn (x))
>> +	    if (now || !var_in_maybe_constexpr_fn (x))
>>   	      {
>>   		if (flags & tf_error)
>>   		  constexpr_error (loc, fundef_p, "use of %<this%> in a "
>> diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-fn8.C b/gcc/testsuite/g++.dg/cpp2a/concepts-fn8.C
>> new file mode 100644
>> index 00000000000..3f63a5b28d7
>> --- /dev/null
>> +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-fn8.C
>> @@ -0,0 +1,24 @@
>> +// PR c++/111703
>> +// { dg-do compile { target c++20 } }
>> +
>> +template<class T>
>> +constexpr bool always_true() { return true; }
>> +
>> +struct P {
>> +  P() = default;
>> +
>> +  template<class T>
>> +    requires (always_true<T>()) // { dg-bogus "used before its definition" }
>> +  constexpr P(const T&) { }
>> +
>> +  int n, m;
>> +};
>> +
>> +void (*f)(P);
>> +
>> +template<class T>
>> +constexpr bool g() {
>> +  P x;
>> +  f(x); // { dg-bogus "from here" }
>> +  return true;
>> +}
>> -- 
>> 2.42.0.526.g3130c155df
>>
>>
>