Re: [pushed] c++: constant, array, lambda, template [PR108975]

[Jason Merrill <jason@redhat.com>]

On 4/25/23 10:55, Patrick Palka wrote:
> On Sat, 18 Mar 2023, Patrick Palka wrote:
> 
>> On Fri, 17 Mar 2023, Jason Merrill via Gcc-patches wrote:
>>
>>> Tested x86_64-pc-linux-gnu, applying to trunk.
>>>
>>> -- 8< --
>>>
>>> When a lambda refers to a constant local variable in the enclosing scope, we
>>> tentatively capture it, but if we end up pulling out its constant value, we
>>> go back at the end of the lambda and prune any unneeded captures.  Here
>>> while parsing the template we decided that the dim capture was unneeded,
>>> because we folded it away, but then we brought back the use in the template
>>> trees that try to preserve the source representation with added type info.
>>> So then when we tried to instantiate that use, we couldn't find what it was
>>> trying to use, and crashed.
>>>
>>> Fixed by not trying to prune when parsing a template; we'll prune at
>>> instantiation time.
>>>
>>> 	PR c++/108975
>>>
>>> gcc/cp/ChangeLog:
>>>
>>> 	* lambda.cc (prune_lambda_captures): Don't bother in a template.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 	* g++.dg/cpp0x/lambda/lambda-const11.C: New test.
>>> ---
>>>   gcc/cp/lambda.cc                                   |  3 +++
>>>   gcc/testsuite/g++.dg/cpp0x/lambda/lambda-const11.C | 14 ++++++++++++++
>>>   2 files changed, 17 insertions(+)
>>>   create mode 100644 gcc/testsuite/g++.dg/cpp0x/lambda/lambda-const11.C
>>>
>>> diff --git a/gcc/cp/lambda.cc b/gcc/cp/lambda.cc
>>> index 212990a21bf..9925209b2ed 100644
>>> --- a/gcc/cp/lambda.cc
>>> +++ b/gcc/cp/lambda.cc
>>> @@ -1760,6 +1760,9 @@ prune_lambda_captures (tree body)
>>>     if (LAMBDA_EXPR_DEFAULT_CAPTURE_MODE (lam) == CPLD_NONE)
>>>       /* No default captures, and we don't prune explicit captures.  */
>>>       return;
>>> +  /* Don't bother pruning in a template, we'll prune at instantiation time.  */
>>> +  if (dependent_type_p (TREE_TYPE (lam)))
>>> +    return;
>>>   
>>>     hash_map<tree,tree*> const_vars;
>>>   
>>> diff --git a/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-const11.C b/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-const11.C
>>> new file mode 100644
>>> index 00000000000..26af75bf132
>>> --- /dev/null
>>> +++ b/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-const11.C
>>> @@ -0,0 +1,14 @@
>>> +// PR c++/108975
>>> +// { dg-do compile { target c++11 } }
>>> +
>>> +template<class T>
>>> +void f() {
>>> +  constexpr int dim = 1;
>>> +  auto l = [&] {
>>> +    int n[dim * 1];
>>> +  };
>>> +  // In f<int>, we shouldn't actually capture dim.
>>> +  static_assert (sizeof(l) == 1, "");
>>> +}
>>> +
>>> +template void f<int>();
>>
>> Sadly I think more is needed to fix the generic lambda case, we still
>> crash for:
>>
>>    template<class T>
>>    void f() {
>>      constexpr int dim = 1;
>>      auto l = [&] (auto) {
>>        int n[dim * 1];
>>      };
>>      l(0);
>>      // In f<int>, we shouldn't actually capture dim.
>>      static_assert (sizeof(l) == 1, "");
>>    }
>>
>>    template void f<int>();
>>
>> It seems prune_lambda_captures doesn't thoroughly walk the lambda body,
>> and so fails to find all uses of constant captures such as in 'int n[dim * 1]'
>> or 'using type = decltype(g<dim * 1>())' (in the original testcase the
>> constant capture use occurred inside an alias declaration).
> 
> I was curious why we ICE only with a by-ref capture and not with a by-value
> capture, and it turns out the difference is that we consider a by-ref
> capture of the constant variable 'dim' to be value-dependent, which
> prevents us from constant folding its uses ahead of time.
> 
> So another approach to fixing the generic lambda version of this
> testcase would be to make value_dependent_expression_p treat by-value
> and by-ref captures of constant variables more similarly.  What do you
> think of the following?
> 
> -- >8 --
> 
> Subject: [PATCH] c++: value dependence of by-ref lambda capture [PR108975]
> 
> We are still ICEing in the generic lambda version of the testcase from
> this PR even after r13-6743-g6f90de97634d6f due to the by-ref capture of
> the constant local variable 'dim' being considered value-dependent when
> regenerating the lambda (at which point processing_template_decl is set
> since the lambda is generic), which prevents us from constant folding
> its uses.  Later when calling prune_lambda_captures, we end up not
> thoroughly walking the body of the lambda and miss the non-folded uses
> of 'dim' within the array bounds and using-decl, and we prematurely
> prune the capture.
> 
> We could fix this by making prune_lambda_captures walk the body of the
> lambda more thoroughly so that it finds these non-folded uses of 'dim',
> but ideally we should be able to constant fold all uses of 'dim' ahead
> of time and prune the implicit capture after all.
> 
> To that end this patch makes value_dependent_expression_p consistently
> return false for such captures of constant local variables even if they
> are by-ref, allowing their uses to get constant folded ahead of time.
> It seems we just need to relax the conservative early exit for reference
> variables (added by r5-5022-g51d72abe5ea04e) when DECL_HAS_VALUE_EXPR_P.
> 
> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> trunk?

OK.

> 	PR c++/108975
> 
> gcc/cp/ChangeLog:
> 
> 	* pt.cc (value_dependent_expression_p) <case VAR_DECL>:
> 	Suppress conservative early exit for reference variables
> 	when DECL_HAS_VALUE_EXPR_P.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/cpp0x/lambda/lambda-const11a.C: New test.
> ---
>   gcc/cp/pt.cc                                  |  7 ++++---
>   .../g++.dg/cpp0x/lambda/lambda-const11a.C     | 21 +++++++++++++++++++
>   2 files changed, 25 insertions(+), 3 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/cpp0x/lambda/lambda-const11a.C
> 
> diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> index 3e5f0104056..a668825b5f9 100644
> --- a/gcc/cp/pt.cc
> +++ b/gcc/cp/pt.cc
> @@ -27959,9 +27959,7 @@ value_dependent_expression_p (tree expression)
>       case VAR_DECL:
>          /* A constant with literal type and is initialized
>   	  with an expression that is value-dependent.  */
> -      if (DECL_DEPENDENT_INIT_P (expression)
> -	  /* FIXME cp_finish_decl doesn't fold reference initializers.  */
> -	  || TYPE_REF_P (TREE_TYPE (expression)))
> +      if (DECL_DEPENDENT_INIT_P (expression))
>   	return true;
>         if (DECL_HAS_VALUE_EXPR_P (expression))
>   	{
> @@ -27976,6 +27974,9 @@ value_dependent_expression_p (tree expression)
>   		  && value_expr == error_mark_node))
>   	    return true;
>   	}
> +      else if (TYPE_REF_P (TREE_TYPE (expression)))
> +	/* FIXME cp_finish_decl doesn't fold reference initializers.  */
> +	return true;
>         return false;
>   
>       case DYNAMIC_CAST_EXPR:
> diff --git a/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-const11a.C b/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-const11a.C
> new file mode 100644
> index 00000000000..d3a6de8a138
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-const11a.C
> @@ -0,0 +1,21 @@
> +// PR c++/108975
> +// A version of lambda-const11.C using a generic lambda.
> +// { dg-do compile { target c++11 } }
> +
> +template<int> void g();
> +template<int> struct A { };
> +
> +template<class T>
> +void f() {
> +  constexpr int dim = 1;
> +  auto f = [&](auto) {
> +    int n[dim * 1];
> +    using ty1 = decltype(g<dim * 2>());
> +    using ty2 = A<dim * 3>;
> +  };
> +  // In f<int>, we shouldn't actually capture dim.
> +  static_assert (sizeof(f) == 1, "");
> +  f(0);
> +}
> +
> +template void f<int>();