| Message ID | 20210831020531.3295265-1-ppalka@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | c++: parameter pack inside constexpr if [PR101764] | expand |
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? 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. > 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? > + && 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. > + 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? > + WALK_SUBTREE (t); > + } > + > + if (parameter_packs) > + { > + /* We found some parameter packs within this extra args tree. */ > + ppd->found_pack_within_extra_args_tree_p = true; > + /* Walk them so that they get added to the overall list. */ > + WALK_SUBTREE (parameter_packs); > + } > + > + *walk_subtrees = 0; > + return NULL_TREE; > + } > + > if (TYPE_P (t)) > cp_walk_tree (&TYPE_CONTEXT (t), > &find_parameter_packs_r, ppd, ppd->visited); > @@ -4229,6 +4284,11 @@ 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_pack_within_extra_args_tree_p) > + /* If the pattern of this pack expansion contains a parameter pack within > + a subtree that uses the extra args mechanism, then the pack expansion > + must also use the extra args mechanism (101764). */ > + PACK_EXPANSION_FORCE_EXTRA_ARGS_P (result) = true; > > return result; > } > @@ -12392,10 +12452,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 > @@ -13148,7 +13213,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); > +} >
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); +}
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); > +} >
On Sun, Sep 12, 2021 at 10:29 PM Jason Merrill <jason@redhat.com> wrote: > > 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. I'm not sure, but I'd suspect that situation would be fine since the pattern of the inner pack expansion is outside the purview of the outer pack expansion, so we'd be oblivious to any unlowered parameter packs within the inner pack expansion (in particular such packs won't be recorded in the PACK_EXPANSION_PARAMETER_PACKS of the outer pack expansion), and ordinary substitution should do the right thing. > > > + 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); > > +} > > >
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 + && 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. */ + 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); + WALK_SUBTREE (t); + } + + if (parameter_packs) + { + /* We found some parameter packs within this extra args tree. */ + ppd->found_pack_within_extra_args_tree_p = true; + /* Walk them so that they get added to the overall list. */ + WALK_SUBTREE (parameter_packs); + } + + *walk_subtrees = 0; + return NULL_TREE; + } + if (TYPE_P (t)) cp_walk_tree (&TYPE_CONTEXT (t), &find_parameter_packs_r, ppd, ppd->visited); @@ -4229,6 +4284,11 @@ 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_pack_within_extra_args_tree_p) + /* If the pattern of this pack expansion contains a parameter pack within + a subtree that uses the extra args mechanism, then the pack expansion + must also use the extra args mechanism (101764). */ + PACK_EXPANSION_FORCE_EXTRA_ARGS_P (result) = true; return result; } @@ -12392,10 +12452,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 @@ -13148,7 +13213,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); +}