On Thu, 3 Feb 2022, Jason Merrill wrote:
> On 2/3/22 15:55, Patrick Palka wrote:
> > On Thu, Feb 3, 2022 at 3:20 PM Jason Merrill <ja...@redhat.com> wrote:
> > >
> > > On 2/3/22 12:04, Patrick Palka wrote:
> > > > On Wed, 2 Feb 2022, Jason Merrill wrote:
> > > >
> > > > > On 2/2/22 12:09, Patrick Palka wrote:
> > > > > > The satisfaction cache needs to look through ARGUMENT_PACK_SELECT
> > > > > > template arguments before calling iterative_hash_template_arg and
> > > > > > template_args_equal, which would otherwise crash.
> > > > >
> > > > > Maybe we should handle ARGUMENT_PACK_SELECT in
> > > > > iterative_hash_template_arg and
> > > > > template_args_equal, rather than their callers?
> > > >
> > > > Like so? I can't think of a good reason not to handle
> > > > ARGUMENT_PACK_SELECT
> > > > ther, but FWIW it looks like we had such handling in the past, until it
> > > > was removed in r7-2082.
> > >
> > > Ah, right. That came after my fix for 67164,
> > >
> > > https://gcc.gnu.org/pipermail/gcc-patches/2016-March/443660.html
> > >
> > > with that PR, we were seeing ARGUMENT_PACK_SELECT unexpectedly because
> > > of excessive sharing of template argument lists. Why are we seeing it
> > > in satisfaction?
> >
> > When instantiating the lambda inside the pack expansion, we call
> > tsubst_lambda_expr with args={ARGUMENT_PACK_SELECT}, wherein we add
> > these args to the lambda's LAMBDA_EXPR_REGEN_INFO for use later during
> > satisfaction.
>
> Yes, that seems like the problem: We're remembering a transient artifact in
> LAMBDA_EXPR_REGEN_INFO. Presumably if the pack is longer than 1, the lambdas
> for all elements of the expansion will have the same ARGUMENT_PACK_SELECT in
> their LAMBDA_EXPR_REGEN_INFO, so they will all think they're in the final
> element.
Aha, I think I get it now. So ARGUMENT_PACK_SELECT is for reusing the
same template argument vector across different elements of a pack
expansion.
>
> I guess tsubst_lambda_expr needs to replace an 'args' with
> ARGUMENT_PACK_SELECT in it with a copy. Or we could do away with
> ARGUMENT_PACK_SELECT entirely in favor of tsubst_pack_expansion using separate
> TREE_VECs for each element substitution.
Since ARGUMENT_PACK_SELECT seems like an important optimization I
think the other more localized approach would be more appropriate for
now. How does the following look?
-- >8 --
Subject: [PATCH] c++: lambda in pack expansion using pack in constraint
[PR103706]
Here when expanding the pack expansion pattern containing a constrained
lambda, the template argument for each Ts is an ARGUMENT_PACK_SELECT,
which we store inside the lambda's LAMBDA_EXPR_REGEN_INFO. Then during
satisfaction of the lambda's constraint C<Ts> the satisfaction cache
uses this argument as part of the key to the corresponding sat_entry, but
iterative_hash_template_arg and template_args_equal deliberately don't
handle ARGUMENT_PACK_SELECT.
Since it's wrong to preserve ARGUMENT_PACK_SELECT inside a hash table
due to its instability (as documented in iterative_hash_template_arg),
this patch helps make sure the satisfaction cache doesn't see such trees
by resolving ARGUMENT_PACK_SELECT arguments before adding them to
LAMBDA_EXPR_REGEN_INFO.
PR c++/103706
gcc/cp/ChangeLog:
* pt.cc (preserve_args): New function.
(tsubst_lambda_expr): Use it when setting LAMBDA_EXPR_REGEN_INFO.
gcc/testsuite/ChangeLog:
* g++.dg/cpp2a/concepts-lambda19.C: New test.
---
gcc/cp/pt.cc | 41 ++++++++++++++++++-
.../g++.dg/cpp2a/concepts-lambda19.C | 11 +++++
2 files changed, 50 insertions(+), 2 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-lambda19.C
diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
index b58067d50e9..07dd544aa52 100644
--- a/gcc/cp/pt.cc
+++ b/gcc/cp/pt.cc
@@ -3743,6 +3743,42 @@ argument_pack_select_arg (tree t)
return arg;
}
+/* Return a modification of ARGS (if necessary) that's suitable for
+ preserving inside a hash table. In particular, this replaces each
+ ARGUMENT_PACK_SELECT with its underlying argument. ARGS is copied
+ (upon modifying it) iff COW_P. */
+
+static tree
+preserve_args (tree args, bool cow_p = true)
+{
+ if (args == NULL_TREE)
+ return NULL_TREE;
+
+ for (int i = 0; i < TREE_VEC_LENGTH (args); ++i)
+ {
+ tree t = TREE_VEC_ELT (args, i);
+ tree r;
+ if (!t)
+ r = NULL_TREE;
+ else if (TREE_CODE (t) == ARGUMENT_PACK_SELECT)
+ r = argument_pack_select_arg (t);
+ else if (TREE_CODE (t) == TREE_VEC)
+ r = preserve_args (t, cow_p);
+ else
+ r = t;
+ if (r != t)
+ {
+ if (cow_p)
+ {
+ args = copy_template_args (args);
+ cow_p = false;
+ }
+ TREE_VEC_ELT (args, i) = r;
+ }
+ }
+
+ return args;
+}
/* True iff FN is a function representing a built-in variadic parameter
pack. */
@@ -19511,10 +19547,11 @@ tsubst_lambda_expr (tree t, tree args, tsubst_flags_t
complain, tree in_decl)
LAMBDA_EXPR_MUTABLE_P (r) = LAMBDA_EXPR_MUTABLE_P (t);
if (tree ti = LAMBDA_EXPR_REGEN_INFO (t))
LAMBDA_EXPR_REGEN_INFO (r)
- = build_template_info (t, add_to_template_args (TI_ARGS (ti), args));
+ = build_template_info (t, add_to_template_args (TI_ARGS (ti),
+ preserve_args (args)));
else
LAMBDA_EXPR_REGEN_INFO (r)
- = build_template_info (t, args);
+ = build_template_info (t, preserve_args (args));
gcc_assert (LAMBDA_EXPR_THIS_CAPTURE (t) == NULL_TREE
&& LAMBDA_EXPR_PENDING_PROXIES (t) == NULL);
diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-lambda19.C
b/gcc/testsuite/g++.dg/cpp2a/concepts-lambda19.C
new file mode 100644
index 00000000000..1921f4892fd
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/concepts-lambda19.C
@@ -0,0 +1,11 @@
+// PR c++/103706
+// { dg-do compile { target c++20 } }
+
+template<class T> concept C = __is_same(T, int);
+
+template<class... Ts> void f() {
+ ([]() requires C<Ts> { return Ts(); }(), ...); // { dg-error "no match" }
+}
+
+template void f<int, int, int>(); // { dg-bogus "" }
+template void f<int, int, char>(); // { dg-message "required from here" }
--
2.35.1.46.g38062e73e0
