| Message ID | 20210507163359.3079556-1-ppalka@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | c++: argument pack expansion inside constraint [PR100138] | expand |
On 5/7/21 12:33 PM, Patrick Palka wrote: > This PR is about CTAD but the underlying problems are more general; > CTAD is a good trigger for them because of the necessary substitution > into constraints that deduction guide generation entails. > > In the testcase below, when generating the implicit deduction guide for > the constrained constructor template for A, we substitute the generic > flattening map 'tsubst_args' into the constructor's constraints. During > this substitution, tsubst_pack_expansion returns a rebuilt pack > expansion for sizeof...(xs), but it's neglecting to carry over the > PACK_EXPANSION_LOCAL_P (and PACK_EXPANSION_SIZEOF_P) flag from the > original tree to the rebuilt one. The flag is otherwise unset on the > original tree[1] but set for the rebuilt tree from make_pack_expansion > only because we're doing the CTAD at function scope (inside main). This > leads us to crash when substituting into the pack expansion during > satisfaction because we don't have local_specializations set up (it'd be > set up for us if PACK_EXPANSION_LOCAL_P is unset) > > Similarly, when substituting into a constraint we need to set > cp_unevaluated since constraints are unevaluated operands. This avoids > a crash during CTAD for C below. > > [1]: Although the original pack expansion is in a function context, I > guess it makes sense that PACK_EXPANSION_LOCAL_P is unset for it because > we can't rely on local specializations (which are formed when > substituting into the function declaration) during satisfaction. > > Bootstrapped and regtested on x86_64-pc-linux-gnu, also tested on > cmcstl2 and range-v3, does this look OK for trunk? OK. > gcc/cp/ChangeLog: > > PR c++/100138 > * constraint.cc (tsubst_constraint): Set up cp_unevaluated. > (satisfy_atom): Set up iloc_sentinel before calling > cxx_constant_value. > * pt.c (tsubst_pack_expansion): When returning a rebuilt pack > expansion, carry over PACK_EXPANSION_LOCAL_P and > PACK_EXPANSION_SIZEOF_P from the original pack expansion. > > gcc/testsuite/ChangeLog: > > PR c++/100138 > * g++.dg/cpp2a/concepts-ctad4.C: New test. > --- > gcc/cp/constraint.cc | 6 ++++- > gcc/cp/pt.c | 2 ++ > gcc/testsuite/g++.dg/cpp2a/concepts-ctad4.C | 25 +++++++++++++++++++++ > 3 files changed, 32 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-ctad4.C > > diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc > index 0709695fd08..30fccc46678 100644 > --- a/gcc/cp/constraint.cc > +++ b/gcc/cp/constraint.cc > @@ -2747,6 +2747,7 @@ tsubst_constraint (tree t, tree args, tsubst_flags_t complain, tree in_decl) > /* We also don't want to evaluate concept-checks when substituting the > constraint-expressions of a declaration. */ > processing_constraint_expression_sentinel s; > + cp_unevaluated u; > tree expr = tsubst_expr (t, args, complain, in_decl, false); > return expr; > } > @@ -3005,7 +3006,10 @@ satisfy_atom (tree t, tree args, sat_info info) > > /* Compute the value of the constraint. */ > if (info.noisy ()) > - result = cxx_constant_value (result); > + { > + iloc_sentinel ils (EXPR_LOCATION (result)); > + result = cxx_constant_value (result); > + } > else > { > result = maybe_constant_value (result, NULL_TREE, > diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c > index 36a8cb5df5d..0d27dd1af65 100644 > --- a/gcc/cp/pt.c > +++ b/gcc/cp/pt.c > @@ -13203,6 +13203,8 @@ tsubst_pack_expansion (tree t, tree args, tsubst_flags_t complain, > else > result = tsubst (pattern, args, complain, in_decl); > result = make_pack_expansion (result, complain); > + PACK_EXPANSION_LOCAL_P (result) = PACK_EXPANSION_LOCAL_P (t); > + PACK_EXPANSION_SIZEOF_P (result) = PACK_EXPANSION_SIZEOF_P (t); > if (PACK_EXPANSION_AUTO_P (t)) > { > /* This is a fake auto... pack expansion created in add_capture with > diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-ctad4.C b/gcc/testsuite/g++.dg/cpp2a/concepts-ctad4.C > new file mode 100644 > index 00000000000..95a3a22dd04 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-ctad4.C > @@ -0,0 +1,25 @@ > +// PR c++/100138 > +// { dg-do compile { target c++20 } } > + > +template <class T> > +struct A { > + A(T, auto... xs) requires (sizeof...(xs) != 0) { } > +}; > + > +constexpr bool f(...) { return true; } > + > +template <class T> > +struct B { > + B(T, auto... xs) requires (f(xs...)); // { dg-error "constant expression" } > +}; > + > +template <class T> > +struct C { > + C(T, auto x) requires (f(x)); // { dg-error "constant expression" } > +}; > + > +int main() { > + A x{1, 2}; // { dg-bogus "" } > + B y{1, 2}; // { dg-error "deduction|no match" } > + C z{1, 2}; // { dg-error "deduction|no match" } > +} >
On Sat, May 8, 2021 at 8:42 AM Jason Merrill <jason@redhat.com> wrote: > > On 5/7/21 12:33 PM, Patrick Palka wrote: > > This PR is about CTAD but the underlying problems are more general; > > CTAD is a good trigger for them because of the necessary substitution > > into constraints that deduction guide generation entails. > > > > In the testcase below, when generating the implicit deduction guide for > > the constrained constructor template for A, we substitute the generic > > flattening map 'tsubst_args' into the constructor's constraints. During > > this substitution, tsubst_pack_expansion returns a rebuilt pack > > expansion for sizeof...(xs), but it's neglecting to carry over the > > PACK_EXPANSION_LOCAL_P (and PACK_EXPANSION_SIZEOF_P) flag from the > > original tree to the rebuilt one. The flag is otherwise unset on the > > original tree[1] but set for the rebuilt tree from make_pack_expansion > > only because we're doing the CTAD at function scope (inside main). This > > leads us to crash when substituting into the pack expansion during > > satisfaction because we don't have local_specializations set up (it'd be > > set up for us if PACK_EXPANSION_LOCAL_P is unset) > > > > Similarly, when substituting into a constraint we need to set > > cp_unevaluated since constraints are unevaluated operands. This avoids > > a crash during CTAD for C below. > > > > [1]: Although the original pack expansion is in a function context, I > > guess it makes sense that PACK_EXPANSION_LOCAL_P is unset for it because > > we can't rely on local specializations (which are formed when > > substituting into the function declaration) during satisfaction. > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, also tested on > > cmcstl2 and range-v3, does this look OK for trunk? > > OK. Would it be ok to backport this patch to the 11 branch given its impact on concepts (or perhaps backport only part of it, say all but the PACK_EXPANSION_LOCAL_P propagation since that part just avoids ICEing on the invalid portions of the testcase)? > > > gcc/cp/ChangeLog: > > > > PR c++/100138 > > * constraint.cc (tsubst_constraint): Set up cp_unevaluated. > > (satisfy_atom): Set up iloc_sentinel before calling > > cxx_constant_value. > > * pt.c (tsubst_pack_expansion): When returning a rebuilt pack > > expansion, carry over PACK_EXPANSION_LOCAL_P and > > PACK_EXPANSION_SIZEOF_P from the original pack expansion. > > > > gcc/testsuite/ChangeLog: > > > > PR c++/100138 > > * g++.dg/cpp2a/concepts-ctad4.C: New test. > > --- > > gcc/cp/constraint.cc | 6 ++++- > > gcc/cp/pt.c | 2 ++ > > gcc/testsuite/g++.dg/cpp2a/concepts-ctad4.C | 25 +++++++++++++++++++++ > > 3 files changed, 32 insertions(+), 1 deletion(-) > > create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-ctad4.C > > > > diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc > > index 0709695fd08..30fccc46678 100644 > > --- a/gcc/cp/constraint.cc > > +++ b/gcc/cp/constraint.cc > > @@ -2747,6 +2747,7 @@ tsubst_constraint (tree t, tree args, tsubst_flags_t complain, tree in_decl) > > /* We also don't want to evaluate concept-checks when substituting the > > constraint-expressions of a declaration. */ > > processing_constraint_expression_sentinel s; > > + cp_unevaluated u; > > tree expr = tsubst_expr (t, args, complain, in_decl, false); > > return expr; > > } > > @@ -3005,7 +3006,10 @@ satisfy_atom (tree t, tree args, sat_info info) > > > > /* Compute the value of the constraint. */ > > if (info.noisy ()) > > - result = cxx_constant_value (result); > > + { > > + iloc_sentinel ils (EXPR_LOCATION (result)); > > + result = cxx_constant_value (result); > > + } > > else > > { > > result = maybe_constant_value (result, NULL_TREE, > > diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c > > index 36a8cb5df5d..0d27dd1af65 100644 > > --- a/gcc/cp/pt.c > > +++ b/gcc/cp/pt.c > > @@ -13203,6 +13203,8 @@ tsubst_pack_expansion (tree t, tree args, tsubst_flags_t complain, > > else > > result = tsubst (pattern, args, complain, in_decl); > > result = make_pack_expansion (result, complain); > > + PACK_EXPANSION_LOCAL_P (result) = PACK_EXPANSION_LOCAL_P (t); > > + PACK_EXPANSION_SIZEOF_P (result) = PACK_EXPANSION_SIZEOF_P (t); > > if (PACK_EXPANSION_AUTO_P (t)) > > { > > /* This is a fake auto... pack expansion created in add_capture with > > diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-ctad4.C b/gcc/testsuite/g++.dg/cpp2a/concepts-ctad4.C > > new file mode 100644 > > index 00000000000..95a3a22dd04 > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-ctad4.C > > @@ -0,0 +1,25 @@ > > +// PR c++/100138 > > +// { dg-do compile { target c++20 } } > > + > > +template <class T> > > +struct A { > > + A(T, auto... xs) requires (sizeof...(xs) != 0) { } > > +}; > > + > > +constexpr bool f(...) { return true; } > > + > > +template <class T> > > +struct B { > > + B(T, auto... xs) requires (f(xs...)); // { dg-error "constant expression" } > > +}; > > + > > +template <class T> > > +struct C { > > + C(T, auto x) requires (f(x)); // { dg-error "constant expression" } > > +}; > > + > > +int main() { > > + A x{1, 2}; // { dg-bogus "" } > > + B y{1, 2}; // { dg-error "deduction|no match" } > > + C z{1, 2}; // { dg-error "deduction|no match" } > > +} > > >
On 7/15/21 12:56 PM, Patrick Palka wrote: > On Sat, May 8, 2021 at 8:42 AM Jason Merrill <jason@redhat.com> wrote: >> >> On 5/7/21 12:33 PM, Patrick Palka wrote: >>> This PR is about CTAD but the underlying problems are more general; >>> CTAD is a good trigger for them because of the necessary substitution >>> into constraints that deduction guide generation entails. >>> >>> In the testcase below, when generating the implicit deduction guide for >>> the constrained constructor template for A, we substitute the generic >>> flattening map 'tsubst_args' into the constructor's constraints. During >>> this substitution, tsubst_pack_expansion returns a rebuilt pack >>> expansion for sizeof...(xs), but it's neglecting to carry over the >>> PACK_EXPANSION_LOCAL_P (and PACK_EXPANSION_SIZEOF_P) flag from the >>> original tree to the rebuilt one. The flag is otherwise unset on the >>> original tree[1] but set for the rebuilt tree from make_pack_expansion >>> only because we're doing the CTAD at function scope (inside main). This >>> leads us to crash when substituting into the pack expansion during >>> satisfaction because we don't have local_specializations set up (it'd be >>> set up for us if PACK_EXPANSION_LOCAL_P is unset) >>> >>> Similarly, when substituting into a constraint we need to set >>> cp_unevaluated since constraints are unevaluated operands. This avoids >>> a crash during CTAD for C below. >>> >>> [1]: Although the original pack expansion is in a function context, I >>> guess it makes sense that PACK_EXPANSION_LOCAL_P is unset for it because >>> we can't rely on local specializations (which are formed when >>> substituting into the function declaration) during satisfaction. >>> >>> Bootstrapped and regtested on x86_64-pc-linux-gnu, also tested on >>> cmcstl2 and range-v3, does this look OK for trunk? >> >> OK. > > Would it be ok to backport this patch to the 11 branch given its > impact on concepts (or perhaps backport only part of it, say all but > the PACK_EXPANSION_LOCAL_P propagation since that part just avoids > ICEing on the invalid portions of the testcase)? The whole patch, I think. >>> gcc/cp/ChangeLog: >>> >>> PR c++/100138 >>> * constraint.cc (tsubst_constraint): Set up cp_unevaluated. >>> (satisfy_atom): Set up iloc_sentinel before calling >>> cxx_constant_value. >>> * pt.c (tsubst_pack_expansion): When returning a rebuilt pack >>> expansion, carry over PACK_EXPANSION_LOCAL_P and >>> PACK_EXPANSION_SIZEOF_P from the original pack expansion. >>> >>> gcc/testsuite/ChangeLog: >>> >>> PR c++/100138 >>> * g++.dg/cpp2a/concepts-ctad4.C: New test. >>> --- >>> gcc/cp/constraint.cc | 6 ++++- >>> gcc/cp/pt.c | 2 ++ >>> gcc/testsuite/g++.dg/cpp2a/concepts-ctad4.C | 25 +++++++++++++++++++++ >>> 3 files changed, 32 insertions(+), 1 deletion(-) >>> create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-ctad4.C >>> >>> diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc >>> index 0709695fd08..30fccc46678 100644 >>> --- a/gcc/cp/constraint.cc >>> +++ b/gcc/cp/constraint.cc >>> @@ -2747,6 +2747,7 @@ tsubst_constraint (tree t, tree args, tsubst_flags_t complain, tree in_decl) >>> /* We also don't want to evaluate concept-checks when substituting the >>> constraint-expressions of a declaration. */ >>> processing_constraint_expression_sentinel s; >>> + cp_unevaluated u; >>> tree expr = tsubst_expr (t, args, complain, in_decl, false); >>> return expr; >>> } >>> @@ -3005,7 +3006,10 @@ satisfy_atom (tree t, tree args, sat_info info) >>> >>> /* Compute the value of the constraint. */ >>> if (info.noisy ()) >>> - result = cxx_constant_value (result); >>> + { >>> + iloc_sentinel ils (EXPR_LOCATION (result)); >>> + result = cxx_constant_value (result); >>> + } >>> else >>> { >>> result = maybe_constant_value (result, NULL_TREE, >>> diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c >>> index 36a8cb5df5d..0d27dd1af65 100644 >>> --- a/gcc/cp/pt.c >>> +++ b/gcc/cp/pt.c >>> @@ -13203,6 +13203,8 @@ tsubst_pack_expansion (tree t, tree args, tsubst_flags_t complain, >>> else >>> result = tsubst (pattern, args, complain, in_decl); >>> result = make_pack_expansion (result, complain); >>> + PACK_EXPANSION_LOCAL_P (result) = PACK_EXPANSION_LOCAL_P (t); >>> + PACK_EXPANSION_SIZEOF_P (result) = PACK_EXPANSION_SIZEOF_P (t); >>> if (PACK_EXPANSION_AUTO_P (t)) >>> { >>> /* This is a fake auto... pack expansion created in add_capture with >>> diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-ctad4.C b/gcc/testsuite/g++.dg/cpp2a/concepts-ctad4.C >>> new file mode 100644 >>> index 00000000000..95a3a22dd04 >>> --- /dev/null >>> +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-ctad4.C >>> @@ -0,0 +1,25 @@ >>> +// PR c++/100138 >>> +// { dg-do compile { target c++20 } } >>> + >>> +template <class T> >>> +struct A { >>> + A(T, auto... xs) requires (sizeof...(xs) != 0) { } >>> +}; >>> + >>> +constexpr bool f(...) { return true; } >>> + >>> +template <class T> >>> +struct B { >>> + B(T, auto... xs) requires (f(xs...)); // { dg-error "constant expression" } >>> +}; >>> + >>> +template <class T> >>> +struct C { >>> + C(T, auto x) requires (f(x)); // { dg-error "constant expression" } >>> +}; >>> + >>> +int main() { >>> + A x{1, 2}; // { dg-bogus "" } >>> + B y{1, 2}; // { dg-error "deduction|no match" } >>> + C z{1, 2}; // { dg-error "deduction|no match" } >>> +} >>> >> >
diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc index 0709695fd08..30fccc46678 100644 --- a/gcc/cp/constraint.cc +++ b/gcc/cp/constraint.cc @@ -2747,6 +2747,7 @@ tsubst_constraint (tree t, tree args, tsubst_flags_t complain, tree in_decl) /* We also don't want to evaluate concept-checks when substituting the constraint-expressions of a declaration. */ processing_constraint_expression_sentinel s; + cp_unevaluated u; tree expr = tsubst_expr (t, args, complain, in_decl, false); return expr; } @@ -3005,7 +3006,10 @@ satisfy_atom (tree t, tree args, sat_info info) /* Compute the value of the constraint. */ if (info.noisy ()) - result = cxx_constant_value (result); + { + iloc_sentinel ils (EXPR_LOCATION (result)); + result = cxx_constant_value (result); + } else { result = maybe_constant_value (result, NULL_TREE, diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c index 36a8cb5df5d..0d27dd1af65 100644 --- a/gcc/cp/pt.c +++ b/gcc/cp/pt.c @@ -13203,6 +13203,8 @@ tsubst_pack_expansion (tree t, tree args, tsubst_flags_t complain, else result = tsubst (pattern, args, complain, in_decl); result = make_pack_expansion (result, complain); + PACK_EXPANSION_LOCAL_P (result) = PACK_EXPANSION_LOCAL_P (t); + PACK_EXPANSION_SIZEOF_P (result) = PACK_EXPANSION_SIZEOF_P (t); if (PACK_EXPANSION_AUTO_P (t)) { /* This is a fake auto... pack expansion created in add_capture with diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-ctad4.C b/gcc/testsuite/g++.dg/cpp2a/concepts-ctad4.C new file mode 100644 index 00000000000..95a3a22dd04 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-ctad4.C @@ -0,0 +1,25 @@ +// PR c++/100138 +// { dg-do compile { target c++20 } } + +template <class T> +struct A { + A(T, auto... xs) requires (sizeof...(xs) != 0) { } +}; + +constexpr bool f(...) { return true; } + +template <class T> +struct B { + B(T, auto... xs) requires (f(xs...)); // { dg-error "constant expression" } +}; + +template <class T> +struct C { + C(T, auto x) requires (f(x)); // { dg-error "constant expression" } +}; + +int main() { + A x{1, 2}; // { dg-bogus "" } + B y{1, 2}; // { dg-error "deduction|no match" } + C z{1, 2}; // { dg-error "deduction|no match" } +}