On 9/12/21 7:48 PM, Patrick Palka wrote:
On Thu, 2 Sep 2021, Jason Merrill wrote:
On 8/30/21 10:05 PM, Patrick Palka wrote:
Here when partially substituting into the pack expansion, substitution
into the constexpr if yields a still-dependent tree, so tsubst_expr
returns an IF_STMT with an unsubstituted IF_COND and with
IF_STMT_EXTRA_ARGS added to. Hence after partial substitution
the pack expansion pattern still refers to the parameter pack 'ts' of
level 2 (and it's thus represented in the new
PACK_EXPANSION_PARAMETER_PACKS)
even though the partially instantiated generic lambda admits only one
level of template arguments.
This causes us to crash during the
subsequent instantiation with the lambda's template arguments because of
the level mismatch. (Likewise when the constexpr if is replaced by a
requires-expr, which too uses the extra args mechanism for delaying
partial instantiation.)
So essentially, a pack expansion pattern that contains a parameter pack
inside an "extra args" tree doesn't play well with partial substitution
thereof. This patch fixes this by forcing such pack expansions to use
the extra args mechanism as well.
Why is this specific to parameter packs? Won't non-pack template parameters
also suffer from the level mismatch?
IIUC it's specific to parameter packs because each parameter pack in the
pattern is also recorded in PACK_EXPANSION_PARAMETER_PACKS, which
tsubst_pack_expansion looks at to extra all argument packs from 'args'
that are relevant to the pattern.
I should clarify it's during the loop over PACK_EXPANSION_PARAMETER_PACKS
that we crash, because we fail to find an argument pack for 'ts' (which
still has the unlowered level 2), and we trip over the assert:
{
/* We can't substitute for this parameter pack. We use a flag as
well as the missing_level counter because function parameter
packs don't have a level. */
gcc_assert (processing_template_decl || is_auto (parm_pack));
unsubstituted_packs = true;
}
For non-pack template parameters (even those within extra args trees),
ordinary substitution is sufficient and does the right thing.
I'd think it would be simpler to just
note when the pattern contains a constexpr if or requires-expression, for
which we can't substitute into the pattern like a pack expansion, and know we
need to use extra args in that case.
Sounds good. We'd force the extra args mechanism more than is strictly
necessary, but IIUC that should be harmless.
Agreed.
Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
trunk?
PR c++/101764
gcc/cp/ChangeLog:
* cp-tree.h (PACK_EXPANSION_FORCE_EXTRA_ARGS_P): New accessor
macro.
* pt.c (uses_extra_args_mechanism_p): New function.
(find_parameter_pack_data::found_pack_within_extra_args_tree_p):
New data member.
(find_parameter_pack_data::inside_extra_args_tree_p): Likewise.
(find_parameter_packs_r): Detect parameter packs within "extra
args" trees and set found_pack_within_extra_args_tree_p
appropriately.
(make_pack_expansion): Set PACK_EXPANSION_FORCE_EXTRA_ARGS_P if
found_pack_within_extra_args_tree_p.
(use_pack_expansion_extra_args_p): Return true if there were
unsubstituted packs and PACK_EXPANSION_FORCE_EXTRA_ARGS_P.
(tsubst_pack_expansion): Pass the pack expansion to
use_pack_expansion_extra_args_p.
gcc/testsuite/ChangeLog:
* g++.dg/cpp1z/constexpr-if35.C: New test.
---
gcc/cp/cp-tree.h | 5 ++
gcc/cp/pt.c | 69 ++++++++++++++++++++-
gcc/testsuite/g++.dg/cpp1z/constexpr-if35.C | 18 ++++++
3 files changed, 90 insertions(+), 2 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/cpp1z/constexpr-if35.C
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index ce7ca53a113..06dec495428 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -493,6 +493,7 @@ extern GTY(()) tree cp_global_trees[CPTI_MAX];
CONSTRUCTOR_C99_COMPOUND_LITERAL (in CONSTRUCTOR)
OVL_NESTED_P (in OVERLOAD)
DECL_MODULE_EXPORT_P (in _DECL)
+ PACK_EXPANSION_FORCE_EXTRA_ARGS_P (in *_PACK_EXPANSION)
4: IDENTIFIER_MARKED (IDENTIFIER_NODEs)
TREE_HAS_CONSTRUCTOR (in INDIRECT_REF, SAVE_EXPR, CONSTRUCTOR,
CALL_EXPR, or FIELD_DECL).
@@ -3902,6 +3903,10 @@ struct GTY(()) lang_decl {
/* True iff this pack expansion is for auto... in lambda init-capture. */
#define PACK_EXPANSION_AUTO_P(NODE) TREE_LANG_FLAG_2 (NODE)
+/* True if we must use PACK_EXPANSION_EXTRA_ARGS to avoid partial
+ substitution into this pack expansion. */
+#define PACK_EXPANSION_FORCE_EXTRA_ARGS_P(NODE) TREE_LANG_FLAG_3 (NODE)
+
/* True iff the wildcard can match a template parameter pack. */
#define WILDCARD_PACK_P(NODE) TREE_LANG_FLAG_0 (NODE)
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index fcf3ac31b25..a92dff88d9d 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -3855,6 +3855,20 @@ expand_builtin_pack_call (tree call, tree args,
tsubst_flags_t complain,
return NULL_TREE;
}
+/* Return true if the tree T uses the extra args mechanism for
+ deferring partial substitution into it. */
+
+static bool
+uses_extra_args_mechanism_p (tree t)
+{
+ return (PACK_EXPANSION_P (t)
+ || TREE_CODE (t) == REQUIRES_EXPR
+ || (TREE_CODE (t) == IF_STMT
+ && IF_STMT_CONSTEXPR_P (t)));
+}
+
+static tree find_parameter_packs_r (tree *, int *, void*);
+
/* Structure used to track the progress of find_parameter_packs_r. */
struct find_parameter_pack_data
{
@@ -3867,6 +3881,16 @@ struct find_parameter_pack_data
/* True iff we're making a type pack expansion. */
bool type_pack_expansion_p;
+
+ /* True iff we found a pack inside a subtree that uses the extra
+ args mechanism. */
+ bool found_pack_within_extra_args_tree_p = false;
+
+private:
+ /* True iff find_parameter_packs_r is currently visiting a tree
+ that uses the extra args mechanism or a subtree thereof. */
+ bool inside_extra_args_tree_p = false;
+ friend tree find_parameter_packs_r (tree *, int *, void*);
};
/* Identifies all of the argument packs that occur in a template
@@ -3968,6 +3992,37 @@ find_parameter_packs_r (tree *tp, int *walk_subtrees,
void* data)
*ppd->parameter_packs = tree_cons (NULL_TREE, t,
*ppd->parameter_packs);
}
+ if (!ppd->inside_extra_args_tree_p
Why this flag?
With the revised patch below this flag is gone, but: it was needed
because inside this branch we walk again into 't' (rather than directly
into its subtrees), so without setting/checking inside_extra_args_tree_p
we'd get infinite recursion. It allows us to avoid having to
specifically recurse into each subtree of IF_STMT / REQUIRES_EXPR here
(and any future extra args trees that get added).
+ && uses_extra_args_mechanism_p (t)
+ && !PACK_EXPANSION_P (t))
+ {
+ /* This tree is using the extra args mechanism. Update *ppd and
recurse
+ so that we can try to detect a parameter pack within. */
This comment needs to mention why pack expansions are different. And it seems
wrong to say that the tree *is using* the mechanism; it's a tree that would
use the mechanism in case of partial instantiation.
Ack; I should probably say 'has (the extra args mechanism)' instead of
'uses' throughout.
+ tree parameter_packs = NULL_TREE;
+ hash_set<tree> visited;
+
+ {
+ auto iovr = make_temp_override (ppd->inside_extra_args_tree_p,
+ true);
+ auto povr = make_temp_override (ppd->parameter_packs,
+ ¶meter_packs);
+ auto vovr = make_temp_override (ppd->visited,
+ &visited);
Why override these?
This too is also gone with the revised patch, but: we needed to override
e.g. 'visited' because if a pack appears earlier in the pattern and then
later in the extra args tree, we wouldn't walk into the pack a second
time and so wouldn't notice it in inside the extra args tree.
Here's v2, which takes the simpler approach:
-- >8 --
PR c++/101764
gcc/cp/ChangeLog:
* cp-tree.h (PACK_EXPANSION_FORCE_EXTRA_ARGS_P): New accessor
macro.
* pt.c (has_extra_args_mechanism_p): New function.
(find_parameter_pack_data::found_extra_args_tree_p):
New data member.
(find_parameter_packs_r): Set found_extra_args_tree_p
appropriately.
(make_pack_expansion): Set PACK_EXPANSION_FORCE_EXTRA_ARGS_P if
found_extra_args_tree_p.
(use_pack_expansion_extra_args_p): Return true if there were
unsubstituted packs and PACK_EXPANSION_FORCE_EXTRA_ARGS_P.
(tsubst_pack_expansion): Pass the pack expansion to
use_pack_expansion_extra_args_p.
gcc/testsuite/ChangeLog:
* g++.dg/cpp1z/constexpr-if35.C: New test.
---
gcc/cp/cp-tree.h | 5 +++
gcc/cp/pt.c | 35 +++++++++++++++++++--
gcc/testsuite/g++.dg/cpp1z/constexpr-if35.C | 18 +++++++++++
3 files changed, 56 insertions(+), 2 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/cpp1z/constexpr-if35.C
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index ceb53591926..706a51bd80c 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -493,6 +493,7 @@ extern GTY(()) tree cp_global_trees[CPTI_MAX];
CONSTRUCTOR_C99_COMPOUND_LITERAL (in CONSTRUCTOR)
OVL_NESTED_P (in OVERLOAD)
DECL_MODULE_EXPORT_P (in _DECL)
+ PACK_EXPANSION_FORCE_EXTRA_ARGS_P (in *_PACK_EXPANSION)
4: IDENTIFIER_MARKED (IDENTIFIER_NODEs)
TREE_HAS_CONSTRUCTOR (in INDIRECT_REF, SAVE_EXPR, CONSTRUCTOR,
CALL_EXPR, or FIELD_DECL).
@@ -3903,6 +3904,10 @@ struct GTY(()) lang_decl {
/* True iff this pack expansion is for auto... in lambda init-capture. */
#define PACK_EXPANSION_AUTO_P(NODE) TREE_LANG_FLAG_2 (NODE)
+/* True if we must use PACK_EXPANSION_EXTRA_ARGS to avoid partial
+ instantiation of this pack expansion. */
+#define PACK_EXPANSION_FORCE_EXTRA_ARGS_P(NODE) TREE_LANG_FLAG_3 (NODE)
+
/* True iff the wildcard can match a template parameter pack. */
#define WILDCARD_PACK_P(NODE) TREE_LANG_FLAG_0 (NODE)
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 1b81501386b..224dd9ebd2b 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -3855,6 +3855,18 @@ expand_builtin_pack_call (tree call, tree args,
tsubst_flags_t complain,
return NULL_TREE;
}
+/* Return true if the tree T has the extra args mechanism for
+ avoiding partial instantiation. */
+
+static bool
+has_extra_args_mechanism_p (const_tree t)
+{
+ return (PACK_EXPANSION_P (t) /* PACK_EXPANSION_EXTRA_ARGS */
+ || TREE_CODE (t) == REQUIRES_EXPR /* REQUIRES_EXPR_EXTRA_ARGS */
+ || (TREE_CODE (t) == IF_STMT
+ && IF_STMT_CONSTEXPR_P (t))); /* IF_STMT_EXTRA_ARGS */
+}
+
/* Structure used to track the progress of find_parameter_packs_r. */
struct find_parameter_pack_data
{
@@ -3867,6 +3879,9 @@ struct find_parameter_pack_data
/* True iff we're making a type pack expansion. */
bool type_pack_expansion_p;
+
+ /* True iff we found a subtree that has the extra args mechanism. */
+ bool found_extra_args_tree_p = false;
};
/* Identifies all of the argument packs that occur in a template
@@ -3968,6 +3983,9 @@ find_parameter_packs_r (tree *tp, int *walk_subtrees,
void* data)
*ppd->parameter_packs = tree_cons (NULL_TREE, t, *ppd->parameter_packs);
}
+ if (has_extra_args_mechanism_p (t) && !PACK_EXPANSION_P (t))
The patch is OK as is, but I wonder if it's possible to run into the
same problem with an inner pack expansion that ends up needing to use
_EXTRA_ARGS for whatever reason.
+ ppd->found_extra_args_tree_p = true;
+
if (TYPE_P (t))
cp_walk_tree (&TYPE_CONTEXT (t),
&find_parameter_packs_r, ppd, ppd->visited);
@@ -4229,6 +4247,14 @@ make_pack_expansion (tree arg, tsubst_flags_t complain)
PACK_EXPANSION_PARAMETER_PACKS (result) = parameter_packs;
PACK_EXPANSION_LOCAL_P (result) = at_function_scope_p ();
+ if (ppd.found_extra_args_tree_p)
+ /* If the pattern of this pack expansion contains a subtree that has
+ the extra args mechanism for avoiding partial instantiation, then
+ force this pack expansion to also use extra args. Otherwise
+ partial instantiation of this pack expansion may not lower the
+ level of some parameter packs within the pattern, which would
+ confuse tsubst_pack_expansion later (PR101764). */
+ PACK_EXPANSION_FORCE_EXTRA_ARGS_P (result) = true;
return result;
}
@@ -12405,10 +12431,15 @@ make_argument_pack_select (tree arg_pack, unsigned
index)
substitution. */
static bool
-use_pack_expansion_extra_args_p (tree parm_packs,
+use_pack_expansion_extra_args_p (tree t,
+ tree parm_packs,
int arg_pack_len,
bool has_empty_arg)
{
+ if (has_empty_arg
+ && PACK_EXPANSION_FORCE_EXTRA_ARGS_P (t))
+ return true;
+
/* If one pack has an expansion and another pack has a normal
argument or if one pack has an empty argument and an another
one hasn't then tsubst_pack_expansion cannot perform the
@@ -13161,7 +13192,7 @@ tsubst_pack_expansion (tree t, tree args,
tsubst_flags_t complain,
/* We cannot expand this expansion expression, because we don't have
all of the argument packs we need. */
- if (use_pack_expansion_extra_args_p (packs, len, unsubstituted_packs))
+ if (use_pack_expansion_extra_args_p (t, packs, len, unsubstituted_packs))
{
/* We got some full packs, but we can't substitute them in until we
have values for all the packs. So remember these until then. */
diff --git a/gcc/testsuite/g++.dg/cpp1z/constexpr-if35.C
b/gcc/testsuite/g++.dg/cpp1z/constexpr-if35.C
new file mode 100644
index 00000000000..cae690ba82c
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1z/constexpr-if35.C
@@ -0,0 +1,18 @@
+// PR c++/101764
+// { dg-do compile { target c++17 } }
+
+void g(...);
+
+template<class>
+auto f() {
+ return [](auto... ts) {
+ g([] { if constexpr (decltype(ts){0}); }...);
+#if __cpp_concepts
+ g(requires { decltype(ts){0}; }...);
+#endif
+ };
+}
+
+int main() {
+ f<int>()('a', true);
+}
