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. > > > 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)) + 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); +} -- 2.33.0.328.g8b7c11b866