On Sat, May 8, 2021 at 8:42 AM Jason Merrill <ja...@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" }
> > +}
> >
>
