From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTP id A15573857C4F for ; Mon, 13 Sep 2021 02:29:31 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A15573857C4F Received: from mail-qk1-f200.google.com (mail-qk1-f200.google.com [209.85.222.200]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-8-fjYEjHh0PmWkTV6SR98pjQ-1; Sun, 12 Sep 2021 22:29:30 -0400 X-MC-Unique: fjYEjHh0PmWkTV6SR98pjQ-1 Received: by mail-qk1-f200.google.com with SMTP id 62-20020a3706410000b02903d2cdd9acf0so39844034qkg.21 for ; Sun, 12 Sep 2021 19:29:30 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=XxfbSUv8HKeAKkVCJrwNpk5fkvCGdG1JeHMr6V2Hdzk=; b=Wy8M4Z2k57VEAh8JApJk8Pl+9VODO30DNhqGwUP+/Nfo1HHCADM0yhNgivMJl48xGN /hBkHwqSjuBYoFTDiNfR058B8BGSPbn74ErGu+tQbKocPKMofAMvll4zKxz0zaHf2jT9 NVGbiaQ5qg54kCsrV4yH6OgV2G+DAihXbot/O2DWyuW7ww47ZLtnhHPcjK907n/BZcie lw8uZ1xzBuwfpyHT9O/+pZiujIYCBDfiRUwgWnqDYjEOfwG6P2i0mcyyjxma/0gcA37s RMBH4pSv9U7Pigj3quZUTjTAiAXpa+GU8Zbr9pf+HGoDLAn7HH29Bl1TDiNyp0mdZvj7 QP8g== X-Gm-Message-State: AOAM532cIT7OthvJP7J9XbaUSE0SkIeKr75wdKbxD44smEfMUYrzizhc gBj3uILiGAp+ycpP5KOIt6Ja85QYj4wZw4g+rbgo0xt2t7trQ/xMeuPaPUUkXJYNNAY4bf3i+dz SM30RzrA7OT7HwOgiwPqupk7OeJuEI59vg6SitTZdIDfmdbvazhOQ5YMT93nxYq41qg== X-Received: by 2002:ac8:148b:: with SMTP id l11mr7667768qtj.298.1631500169055; Sun, 12 Sep 2021 19:29:29 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzI8TesEFFYVmf0ob5ivzn+Szemf8cvJJBeN7eF+2hCnVVZtgiyTpJoRMKdne+HLgIqWWAZUw== X-Received: by 2002:ac8:148b:: with SMTP id l11mr7667741qtj.298.1631500168509; Sun, 12 Sep 2021 19:29:28 -0700 (PDT) Received: from [192.168.1.149] (130-44-159-43.s11817.c3-0.arl-cbr1.sbo-arl.ma.cable.rcncustomer.com. [130.44.159.43]) by smtp.gmail.com with ESMTPSA id 13sm4334669qka.56.2021.09.12.19.29.27 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 12 Sep 2021 19:29:27 -0700 (PDT) Subject: Re: [PATCH] c++: parameter pack inside constexpr if [PR101764] To: Patrick Palka Cc: gcc-patches@gcc.gnu.org References: <20210831020531.3295265-1-ppalka@redhat.com> <2a8ed7d1-c7a0-a63a-d3d0-0f00f06f3846@redhat.com> <749c684b-ac22-f658-84e5-2de46fdac668@idea> From: Jason Merrill Message-ID: <0d7278cf-90f1-7286-5fe8-ba3f41e01629@redhat.com> Date: Sun, 12 Sep 2021 22:29:26 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: <749c684b-ac22-f658-84e5-2de46fdac668@idea> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-14.8 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 13 Sep 2021 02:29:33 -0000 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 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 > +auto f() { > + return [](auto... ts) { > + g([] { if constexpr (decltype(ts){0}); }...); > +#if __cpp_concepts > + g(requires { decltype(ts){0}; }...); > +#endif > + }; > +} > + > +int main() { > + f()('a', true); > +} >