| Message ID | 20230317233249.1406928-1-jason@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [pushed] c++: constant, array, lambda, template [PR108975] | expand |
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 guess generic lambdas are special in that their body is always templated and so fewer constant captures are folded away by instantiation time. So they are more sensitive to incomplete walking by prune_lambda_captures.
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? 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>();
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>();
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>();