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.129.124]) by sourceware.org (Postfix) with ESMTPS id 87C823858D28 for ; Sat, 1 Apr 2023 03:50:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 87C823858D28 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1680321006; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=u9nNdJqDCyvFc+0KE7klZOF6gpw4C6Oj7wkI3OxNanw=; b=EdrECXNFSDmzu7actrFnAoXXPmFrEfdGs4e84U3eE2Chm6umw5zNu0WX59WX1ZbDPasPSv v8GzrN1+2HsnS75JKGAOG4efMuzKSfR+Myl4ArFCQVin8HzuwxVhTHxoL5l1eNGiNHYs6i q6qC2bS6mqxFgmc7mnVrbZXi4VsK9+k= Received: from mail-qk1-f197.google.com (mail-qk1-f197.google.com [209.85.222.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-141-shpYfV94O72OgjwvnqqZhg-1; Fri, 31 Mar 2023 23:50:04 -0400 X-MC-Unique: shpYfV94O72OgjwvnqqZhg-1 Received: by mail-qk1-f197.google.com with SMTP id c80-20020ae9ed53000000b007468c560e1bso11369286qkg.2 for ; Fri, 31 Mar 2023 20:50:04 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1680321004; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=u9nNdJqDCyvFc+0KE7klZOF6gpw4C6Oj7wkI3OxNanw=; b=gAnILeCdSLa60+2oDaAIZDuZYfNS3Aptxn/89WHA34EeFKtLsYlrpEm3MgarP8Xw5Z dZRbWlyGY0AMd0ZV5Bfp5UMV7wTlwoCNOzAdLX3Iye9KLIF1eBNInO1i9FZo44418mhV elQBv8l/9FSrt5/LmvHVQsswlU8LVWToBLR9wEhGjZyFdKp4P+Nk0ASyYLcjiUupp+Ai dnzKlFc5h30sdCDXLsxOOwiVinHRsbbvusmwFdvq7mdPK/kEImwCMk4a97Bu6Mgle3QX fqHKOxN21r6qI4ysQFBWv4PDdsOGLtnxSoYd0Ho7b0rFK9bphN/o+nF5n4Jn/Wjx1l66 VcMA== X-Gm-Message-State: AO0yUKWWxEJJUtrvvPGQoFoFUUBC++9Duxl0W5u9OreNhOFFc6VbGtTR C0C70lPVVdiIQldobmlVO7k7Xs95YUyM/+3TfSKDyE/axlvTPg56ImnLWchUIknP5vGIDI/Ts9t PduuOw0ROlHICARVO0ssWm/q/hQ== X-Received: by 2002:a05:622a:c6:b0:3b6:2c3b:8c00 with SMTP id p6-20020a05622a00c600b003b62c3b8c00mr50298228qtw.66.1680321003697; Fri, 31 Mar 2023 20:50:03 -0700 (PDT) X-Google-Smtp-Source: AK7set9bfi5+/VSN2sGNEf7zyshRyeYe5tKK6PiombLhzYFfIC9poqiKAEJfAFvecALXhUmPRk7GsA== X-Received: by 2002:a05:622a:c6:b0:3b6:2c3b:8c00 with SMTP id p6-20020a05622a00c600b003b62c3b8c00mr50298194qtw.66.1680321003040; Fri, 31 Mar 2023 20:50:03 -0700 (PDT) Received: from [192.168.1.108] (130-44-146-16.s12558.c3-0.arl-cbr1.sbo-arl.ma.cable.rcncustomer.com. [130.44.146.16]) by smtp.gmail.com with ESMTPSA id t18-20020a37ea12000000b0074860fcfbecsm1149389qkj.21.2023.03.31.20.50.02 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 31 Mar 2023 20:50:02 -0700 (PDT) Message-ID: <74ccb540-b964-fc2f-d52d-c5832b5df563@redhat.com> Date: Fri, 31 Mar 2023 23:50:01 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.9.1 Subject: Re: [PATCH 1/2] c++: improve "NTTP argument considered unused" fix [PR53164, PR105848] To: Patrick Palka Cc: gcc-patches@gcc.gnu.org References: <20230323211803.396326-1-ppalka@redhat.com> <4a04306f-faa9-5dd8-c850-05aa4a21d36e@idea> <1aca5d63-ec57-abc2-422f-68b197bb6c47@idea> <8fe724c0-aa3c-a35d-63dd-7e5c890969ea@redhat.com> <7c047507-896a-5fbf-f1ce-14a1377b0b9f@idea> From: Jason Merrill In-Reply-To: <7c047507-896a-5fbf-f1ce-14a1377b0b9f@idea> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.5 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_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 3/31/23 15:06, Patrick Palka wrote: > On Fri, 31 Mar 2023, Jason Merrill wrote: > >> On 3/31/23 11:55, Patrick Palka wrote: >>> On Fri, 31 Mar 2023, Patrick Palka wrote: >>> >>>> On Thu, 30 Mar 2023, Jason Merrill wrote: >>>> >>>>> On 3/30/23 14:53, Patrick Palka wrote: >>>>>> On Wed, 29 Mar 2023, Jason Merrill wrote: >>>>>> >>>>>>> On 3/27/23 09:30, Patrick Palka wrote: >>>>>>>> On Thu, 23 Mar 2023, Patrick Palka wrote: >>>>>>>> >>>>>>>>> r13-995-g733a792a2b2e16 worked around the problem of >>>>>>>>> FUNCTION_DECL >>>>>>>>> template arguments not always getting marked as odr-used by >>>>>>>>> redundantly >>>>>>>>> calling mark_used on the substituted ADDR_EXPR callee of a >>>>>>>>> CALL_EXPR. >>>>>>>>> This is just a narrow workaround however, since using a >>>>>>>>> FUNCTION_DECL >>>>>>>>> as >>>>>>>>> a template argument alone should constitutes an odr-use; we >>>>>>>>> shouldn't >>>>>>>>> need to subsequently e.g. call the function or take its address. >>>>>>> >>>>>>> Agreed. But why didn't we already wrap it in an ADDR_EXPR? Even >>>>>>> for >>>>>>> reference tparms convert_nontype_argument should do that. >>>>>> >>>>>> Indeed we do, the commit message was just rather sloppy/inaccurate... >>>>>> I'll try to correct it. >>>>>> >>>>>>> >>>>>>>>> This patch fixes this in a more general way at template >>>>>>>>> specialization >>>>>>>>> time by walking the template arguments of the specialization and >>>>>>>>> calling >>>>>>>>> mark_used on all entities used within. As before, the call to >>>>>>>>> mark_used >>>>>>>>> as it worst a no-op, but it compensates for the situation where >>>>>>>>> we end >>>>>>>>> up >>>>>>>>> forming a specialization from a template context in which >>>>>>>>> mark_used is >>>>>>>>> inhibited. Another approach would be to call mark_used whenever >>>>>>>>> we >>>>>>>>> substitute a TEMPLATE_PARM_INDEX, but that would result in many >>>>>>>>> more >>>>>>>>> redundant calls to mark_used compared to this approach. >>>>>>>>> >>>>>>>>> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this >>>>>>>>> look OK >>>>>>>>> for >>>>>>>>> trunk? >>>>>>>>> >>>>>>>>> PR c++/53164 >>>>>>>>> PR c++/105848 >>>>>>>>> >>>>>>>>> gcc/cp/ChangeLog: >>>>>>>>> >>>>>>>>> * pt.cc (instantiate_class_template): Call >>>>>>>>> mark_template_arguments_used. >>>>>>>>> (tsubst_copy_and_build) : Revert r13-995 >>>>>>>>> change. >>>>>>>>> (mark_template_arguments_used): Define. >>>>>>>>> (instantiate_template): Call mark_template_arguments_used. >>>>>>>>> >>>>>>>>> gcc/testsuite/ChangeLog: >>>>>>>>> >>>>>>>>> * g++.dg/template/fn-ptr3a.C: New test. >>>>>>>>> * g++.dg/template/fn-ptr4.C: New test. >>>>>>>>> --- >>>>>>>>> gcc/cp/pt.cc | 51 >>>>>>>>> ++++++++++++++++-------- >>>>>>>>> gcc/testsuite/g++.dg/template/fn-ptr3a.C | 25 ++++++++++++ >>>>>>>>> gcc/testsuite/g++.dg/template/fn-ptr4.C | 14 +++++++ >>>>>>>>> 3 files changed, 74 insertions(+), 16 deletions(-) >>>>>>>>> create mode 100644 gcc/testsuite/g++.dg/template/fn-ptr3a.C >>>>>>>>> create mode 100644 gcc/testsuite/g++.dg/template/fn-ptr4.C >>>>>>>>> >>>>>>>>> diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc >>>>>>>>> index 7e4a8de0c8b..9b3cc33331c 100644 >>>>>>>>> --- a/gcc/cp/pt.cc >>>>>>>>> +++ b/gcc/cp/pt.cc >>>>>>>>> @@ -220,6 +220,7 @@ static tree make_argument_pack (tree); >>>>>>>>> static tree enclosing_instantiation_of (tree tctx); >>>>>>>>> static void instantiate_body (tree pattern, tree args, tree >>>>>>>>> d, bool >>>>>>>>> nested); >>>>>>>>> static tree maybe_dependent_member_ref (tree, tree, >>>>>>>>> tsubst_flags_t, >>>>>>>>> tree); >>>>>>>>> +static void mark_template_arguments_used (tree); >>>>>>>>> /* Make the current scope suitable for access checking >>>>>>>>> when we >>>>>>>>> are >>>>>>>>> processing T. T can be FUNCTION_DECL for instantiated >>>>>>>>> function >>>>>>>>> @@ -12142,6 +12143,9 @@ instantiate_class_template (tree type) >>>>>>>>> cp_unevaluated_operand = 0; >>>>>>>>> c_inhibit_evaluation_warnings = 0; >>>>>>>>> } >>>>>>>>> + >>>>>>>>> + mark_template_arguments_used (INNERMOST_TEMPLATE_ARGS >>>>>>>>> (args)); >>>>>>>>> + >>>>>>>>> /* Use #pragma pack from the template context. */ >>>>>>>>> saved_maximum_field_alignment = maximum_field_alignment; >>>>>>>>> maximum_field_alignment = TYPE_PRECISION (pattern); >>>>>>>>> @@ -21173,22 +21177,10 @@ tsubst_copy_and_build (tree t, >>>>>>>>> } >>>>>>>>> /* Remember that there was a reference to this entity. >>>>>>>>> */ >>>>>>>>> - if (function != NULL_TREE) >>>>>>>>> - { >>>>>>>>> - tree inner = function; >>>>>>>>> - if (TREE_CODE (inner) == ADDR_EXPR >>>>>>>>> - && TREE_CODE (TREE_OPERAND (inner, 0)) == >>>>>>>>> FUNCTION_DECL) >>>>>>>>> - /* We should already have called mark_used when taking >>>>>>>>> the >>>>>>>>> - address of this function, but do so again anyway to >>>>>>>>> make >>>>>>>>> - sure it's odr-used: at worst this is a no-op, but if >>>>>>>>> we >>>>>>>>> - obtained this FUNCTION_DECL as part of ahead-of-time >>>>>>>>> overload >>>>>>>>> - resolution then that call to mark_used wouldn't have >>>>>>>>> marked >>>>>>>>> it >>>>>>>>> - odr-used yet (53164). */ >>>>>>>>> - inner = TREE_OPERAND (inner, 0); >>>>>>>>> - if (DECL_P (inner) >>>>>>>>> - && !mark_used (inner, complain) && !(complain & >>>>>>>>> tf_error)) >>>>>>>>> - RETURN (error_mark_node); >>>>>>>>> - } >>>>>>>>> + if (function != NULL_TREE >>>>>>>>> + && DECL_P (function) >>>>>>>>> + && !mark_used (function, complain) && !(complain & >>>>>>>>> tf_error)) >>>>>>>>> + RETURN (error_mark_node); >>>>>>>>> if (!maybe_fold_fn_template_args (function, complain)) >>>>>>>>> return error_mark_node; >>>>>>>>> @@ -21883,6 +21875,31 @@ check_instantiated_args (tree tmpl, >>>>>>>>> tree >>>>>>>>> args, >>>>>>>>> tsubst_flags_t complain) >>>>>>>>> return result; >>>>>>>>> } >>>>>>>>> +/* Call mark_used on each entity within the template >>>>>>>>> arguments >>>>>>>>> ARGS of >>>>>>>>> some >>>>>>>>> + template specialization, to ensure that each such entity is >>>>>>>>> considered >>>>>>>>> + odr-used regardless of whether the specialization was first >>>>>>>>> formed >>>>>>>>> in >>>>>>>>> a >>>>>>>>> + template context. >>>>>>>>> + >>>>>>>>> + This function assumes push_to_top_level has been called >>>>>>>>> beforehand, >>>>>>>>> and >>>>>>>>> + that processing_template_decl has been set iff the template >>>>>>>>> arguments >>>>>>>>> + are dependent. */ >>>>>>>>> + >>>>>>>>> +static void >>>>>>>>> +mark_template_arguments_used (tree args) >>>>>>>>> +{ >>>>>>>>> + gcc_checking_assert (TMPL_ARGS_DEPTH (args) == 1); >>>>>>>>> + >>>>>>>>> + if (processing_template_decl) >>>>>>>>> + return; >>>>>>>>> + >>>>>>>>> + auto mark_used_r = [](tree *tp, int *, void *) { >>>>>>>>> + if (DECL_P (*tp)) >>>>>>>>> + mark_used (*tp, tf_none); >>>>>>>>> + return NULL_TREE; >>>>>>>>> + }; >>>>>>>>> + cp_walk_tree_without_duplicates (&args, mark_used_r, >>>>>>>>> nullptr); >>>>>>>> >>>>>>>> Here's v2 which optimizes this function by not walking if >>>>>>>> !PRIMARY_TEMPLATE_P (since in that case the innermost arguments >>>>>>>> have >>>>>>>> already been walked when instantiating the context), and walking >>>>>>>> only >>>>>>>> non-type arguments: >>>>>>>> >>>>>>>> -- >8 >>>>>>>> >>>>>>>> Subject: [PATCH] c++: improve "NTTP argument considered unused" >>>>>>>> fix >>>>>>>> [PR53164, >>>>>>>> PR105848] >>>>>>>> >>>>>>>> PR c++/53164 >>>>>>>> PR c++/105848 >>>>>>>> >>>>>>>> gcc/cp/ChangeLog: >>>>>>>> >>>>>>>> * pt.cc (instantiate_class_template): Call >>>>>>>> mark_template_arguments_used. >>>>>>>> (tsubst_copy_and_build) : Revert r13-995 >>>>>>>> change. >>>>>>>> (mark_template_arguments_used): Define. >>>>>>>> (instantiate_template): Call mark_template_arguments_used. >>>>>>>> >>>>>>>> gcc/testsuite/ChangeLog: >>>>>>>> >>>>>>>> * g++.dg/template/fn-ptr3a.C: New test. >>>>>>>> * g++.dg/template/fn-ptr4.C: New test. >>>>>>>> --- >>>>>>>> gcc/cp/pt.cc | 57 >>>>>>>> +++++++++++++++++------- >>>>>>>> gcc/testsuite/g++.dg/template/fn-ptr3a.C | 25 +++++++++++ >>>>>>>> gcc/testsuite/g++.dg/template/fn-ptr4.C | 14 ++++++ >>>>>>>> 3 files changed, 80 insertions(+), 16 deletions(-) >>>>>>>> create mode 100644 gcc/testsuite/g++.dg/template/fn-ptr3a.C >>>>>>>> create mode 100644 gcc/testsuite/g++.dg/template/fn-ptr4.C >>>>>>>> >>>>>>>> diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc >>>>>>>> index 3bb98ebeac1..a87366bb616 100644 >>>>>>>> --- a/gcc/cp/pt.cc >>>>>>>> +++ b/gcc/cp/pt.cc >>>>>>>> @@ -220,6 +220,7 @@ static tree make_argument_pack (tree); >>>>>>>> static tree enclosing_instantiation_of (tree tctx); >>>>>>>> static void instantiate_body (tree pattern, tree args, tree d, >>>>>>>> bool >>>>>>>> nested); >>>>>>>> static tree maybe_dependent_member_ref (tree, tree, >>>>>>>> tsubst_flags_t, >>>>>>>> tree); >>>>>>>> +static void mark_template_arguments_used (tree, tree); >>>>>>>> /* Make the current scope suitable for access checking when >>>>>>>> we are >>>>>>>> processing T. T can be FUNCTION_DECL for instantiated >>>>>>>> function >>>>>>>> @@ -12152,6 +12153,9 @@ instantiate_class_template (tree type) >>>>>>>> cp_unevaluated_operand = 0; >>>>>>>> c_inhibit_evaluation_warnings = 0; >>>>>>>> } >>>>>>>> + >>>>>>>> + mark_template_arguments_used (templ, CLASSTYPE_TI_ARGS (type)); >>>>>>>> + >>>>>>>> /* Use #pragma pack from the template context. */ >>>>>>>> saved_maximum_field_alignment = maximum_field_alignment; >>>>>>>> maximum_field_alignment = TYPE_PRECISION (pattern); >>>>>>>> @@ -21192,22 +21196,10 @@ tsubst_copy_and_build (tree t, >>>>>>>> } >>>>>>>> /* Remember that there was a reference to this entity. >>>>>>>> */ >>>>>>>> - if (function != NULL_TREE) >>>>>>>> - { >>>>>>>> - tree inner = function; >>>>>>>> - if (TREE_CODE (inner) == ADDR_EXPR >>>>>>>> - && TREE_CODE (TREE_OPERAND (inner, 0)) == >>>>>>>> FUNCTION_DECL) >>>>>>>> - /* We should already have called mark_used when taking >>>>>>>> the >>>>>>>> - address of this function, but do so again anyway to >>>>>>>> make >>>>>>>> - sure it's odr-used: at worst this is a no-op, but if >>>>>>>> we >>>>>>>> - obtained this FUNCTION_DECL as part of ahead-of-time >>>>>>>> overload >>>>>>>> - resolution then that call to mark_used wouldn't have >>>>>>>> marked >>>>>>>> it >>>>>>>> - odr-used yet (53164). */ >>>>>>>> - inner = TREE_OPERAND (inner, 0); >>>>>>>> - if (DECL_P (inner) >>>>>>>> - && !mark_used (inner, complain) && !(complain & >>>>>>>> tf_error)) >>>>>>>> - RETURN (error_mark_node); >>>>>>>> - } >>>>>>>> + if (function != NULL_TREE >>>>>>>> + && DECL_P (function) >>>>>>>> + && !mark_used (function, complain) && !(complain & >>>>>>>> tf_error)) >>>>>>>> + RETURN (error_mark_node); >>>>>>>> if (!maybe_fold_fn_template_args (function, complain)) >>>>>>>> return error_mark_node; >>>>>>>> @@ -21902,6 +21894,37 @@ check_instantiated_args (tree tmpl, tree >>>>>>>> args, >>>>>>>> tsubst_flags_t complain) >>>>>>>> return result; >>>>>>>> } >>>>>>>> +/* Call mark_used on each entity within the non-type template >>>>>>>> arguments >>>>>>>> in >>>>>>>> + ARGS for a specialization of TMPL, to ensure that each such >>>>>>>> entity >>>>>>>> is >>>>>>>> + considered odr-used regardless of whether the specialization >>>>>>>> was >>>>>>>> first >>>>>>>> + formed in a template context. >>>>>>>> + >>>>>>>> + This function assumes push_to_top_level has been called >>>>>>>> beforehand, >>>>>>>> and >>>>>>>> + that processing_template_decl is set iff the template >>>>>>>> arguments are >>>>>>>> + dependent. */ >>>>>>>> + >>>>>>>> +static void >>>>>>>> +mark_template_arguments_used (tree tmpl, tree args) >>>>>>>> +{ >>>>>>>> + /* It suffices to do this only when fully specializing a >>>>>>>> primary >>>>>>>> template. */ >>>>>>>> + if (processing_template_decl || !PRIMARY_TEMPLATE_P (tmpl)) >>>>>>>> + return; >>>>>>>> + >>>>>>>> + /* We already marked outer arguments when specializing the >>>>>>>> context. >>>>>>>> */ >>>>>>>> + args = INNERMOST_TEMPLATE_ARGS (args); >>>>>>>> + >>>>>>>> + for (tree arg : tree_vec_range (args)) >>>>>>>> + if (!TYPE_P (arg)) >>>>>>>> + { >>>>>>>> + auto mark_used_r = [](tree *tp, int *, void *) { >>>>>>>> + if (DECL_P (*tp)) >>>>>>>> + mark_used (*tp, tf_none); >>>>>>>> + return NULL_TREE; >>>>>>>> + }; >>>>>>>> + cp_walk_tree_without_duplicates (&args, mark_used_r, nullptr); >>>>>>> >>>>>>> Do we really need to walk into the args? >>>>>> >>>>>> Hmm, I think we might need to some sort for walking for class NTTP >>>>>> arguments since they could be nested CONSTRUCTORs. But for non-class >>>>>> arguments we know that an entity will only appear as a pointer- or >>>>>> reference-to-function/variable, so we could pattern match for such a >>>>>> structure directly. And doing so should be preferable because >>>>>> cp_walk_tree is relatively expensive and this is a relatively hot code >>>>>> path. So maybe the following would be preferable? >>>>>> >>>>>> -- >8 -- >>>>>> >>>>>> Subject: [PATCH] c++: improve "NTTP argument considered unused" fix >>>>>> [PR53164, >>>>>> PR105848] >>>>>> >>>>>> r13-995-g733a792a2b2e16 worked around the problem of (pointer to) >>>>>> function NTTP arguments not always getting marked as odr-used by >>>>>> redundantly calling mark_used on the substituted ADDR_EXPR callee of a >>>>>> CALL_EXPR. That is just a narrow workaround however, since it assumes >>>>>> the function is later called. But the use as a template argument >>>>>> alone >>>>>> should constitute an odr-use of the function (since template arguments >>>>>> are an evaluated context, and we need its address); we shouldn't need >>>>>> to >>>>>> subsequently call or otherwise use the NTTP argument. >>>>>> >>>>>> This patch fixes this in a more general way at template specialization >>>>>> time by walking the template arguments of the specialization and >>>>>> calling >>>>>> mark_used on all entities used within. As before, the call to >>>>>> mark_used >>>>>> as it worst a no-op, but it compensates for the situation where we end >>>>>> up forming a specialization in a template context in which mark_used >>>>>> is >>>>>> inhibited. >>>>>> >>>>>> Another approach would be to call mark_used whenever we substitute a >>>>>> TEMPLATE_PARM_INDEX, but that would result in many more redundant >>>>>> calls >>>>>> to mark_used compared to this approach. And as the second testcase >>>>>> below illustrates, we also need to walk C++20 class NTTP arguments >>>>>> which >>>>>> can in theory be large and thus expensive to walk repeatedly. The >>>>>> change to invalid_tparm_referent_p is needed to avoid bogusly >>>>>> rejecting >>>>>> the testcase's class NTTP arguments containing function pointers. >>>>>> >>>>>> (The third testcase is unrelated to this fix, but it helped rule out >>>>>> an >>>>>> earlier approach I was considering and it seems we don't have existing >>>>>> test coverage for this situation.) >>>>>> >>>>>> PR c++/53164 >>>>>> PR c++/105848 >>>>>> >>>>>> gcc/cp/ChangeLog: >>>>>> >>>>>> * pt.cc (invalid_tparm_referent_p): Accept ADDR_EXPR of >>>>>> FUNCTION_DECL. >>>>>> (instantiate_class_template): Call >>>>>> mark_template_arguments_used. >>>>>> (tsubst_copy_and_build) : Revert r13-995 >>>>>> change. >>>>>> (mark_template_arguments_used): Define. >>>>>> (instantiate_template): Call mark_template_arguments_used. >>>>>> >>>>>> gcc/testsuite/ChangeLog: >>>>>> >>>>>> * g++.dg/template/fn-ptr3a.C: New test. >>>>>> * g++.dg/template/fn-ptr3b.C: New test. >>>>>> * g++.dg/template/fn-ptr4.C: New test. >>>>>> --- >>>>>> gcc/cp/pt.cc | 78 >>>>>> ++++++++++++++++++------ >>>>>> gcc/testsuite/g++.dg/template/fn-ptr3a.C | 25 ++++++++ >>>>>> gcc/testsuite/g++.dg/template/fn-ptr3b.C | 28 +++++++++ >>>>>> gcc/testsuite/g++.dg/template/fn-ptr4.C | 14 +++++ >>>>>> 4 files changed, 127 insertions(+), 18 deletions(-) >>>>>> create mode 100644 gcc/testsuite/g++.dg/template/fn-ptr3a.C >>>>>> create mode 100644 gcc/testsuite/g++.dg/template/fn-ptr3b.C >>>>>> create mode 100644 gcc/testsuite/g++.dg/template/fn-ptr4.C >>>>>> >>>>>> diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc >>>>>> index e514a277872..01ab220320c 100644 >>>>>> --- a/gcc/cp/pt.cc >>>>>> +++ b/gcc/cp/pt.cc >>>>>> @@ -220,6 +220,7 @@ static tree make_argument_pack (tree); >>>>>> static tree enclosing_instantiation_of (tree tctx); >>>>>> static void instantiate_body (tree pattern, tree args, tree d, bool >>>>>> nested); >>>>>> static tree maybe_dependent_member_ref (tree, tree, tsubst_flags_t, >>>>>> tree); >>>>>> +static void mark_template_arguments_used (tree, tree); >>>>>> /* Make the current scope suitable for access checking when we >>>>>> are >>>>>> processing T. T can be FUNCTION_DECL for instantiated function >>>>>> @@ -7142,12 +7143,13 @@ invalid_tparm_referent_p (tree type, tree >>>>>> expr, >>>>>> tsubst_flags_t complain) >>>>>> decl = TREE_OPERAND (decl, 0); >>>>>> } >>>>>> - if (!VAR_P (decl)) >>>>>> + if (!VAR_OR_FUNCTION_DECL_P (decl)) >>>>>> { >>>>>> if (complain & tf_error) >>>>>> error_at (cp_expr_loc_or_input_loc (expr), >>>>>> "%qE is not a valid template argument of type >>>>>> %qT " >>>>>> - "because %qE is not a variable", expr, type, >>>>>> decl); >>>>>> + "because %qE is not a variable or function", >>>>>> + expr, type, decl); >>>>>> return true; >>>>>> } >>>>>> else if (cxx_dialect < cxx11 && !DECL_EXTERNAL_LINKAGE_P >>>>>> (decl)) >>>>>> @@ -12152,6 +12154,9 @@ instantiate_class_template (tree type) >>>>>> cp_unevaluated_operand = 0; >>>>>> c_inhibit_evaluation_warnings = 0; >>>>>> } >>>>>> + >>>>>> + mark_template_arguments_used (templ, CLASSTYPE_TI_ARGS (type)); >>>>>> + >>>>>> /* Use #pragma pack from the template context. */ >>>>>> saved_maximum_field_alignment = maximum_field_alignment; >>>>>> maximum_field_alignment = TYPE_PRECISION (pattern); >>>>>> @@ -21192,22 +21197,10 @@ tsubst_copy_and_build (tree t, >>>>>> } >>>>>> /* Remember that there was a reference to this entity. */ >>>>>> - if (function != NULL_TREE) >>>>>> - { >>>>>> - tree inner = function; >>>>>> - if (TREE_CODE (inner) == ADDR_EXPR >>>>>> - && TREE_CODE (TREE_OPERAND (inner, 0)) == >>>>>> FUNCTION_DECL) >>>>>> - /* We should already have called mark_used when taking >>>>>> the >>>>>> - address of this function, but do so again anyway to >>>>>> make >>>>>> - sure it's odr-used: at worst this is a no-op, but if >>>>>> we >>>>>> - obtained this FUNCTION_DECL as part of ahead-of-time >>>>>> overload >>>>>> - resolution then that call to mark_used wouldn't have >>>>>> marked >>>>>> it >>>>>> - odr-used yet (53164). */ >>>>>> - inner = TREE_OPERAND (inner, 0); >>>>>> - if (DECL_P (inner) >>>>>> - && !mark_used (inner, complain) && !(complain & >>>>>> tf_error)) >>>>>> - RETURN (error_mark_node); >>>>>> - } >>>>>> + if (function != NULL_TREE >>>>>> + && DECL_P (function) >>>>>> + && !mark_used (function, complain) && !(complain & >>>>>> tf_error)) >>>>>> + RETURN (error_mark_node); >>>>>> if (!maybe_fold_fn_template_args (function, complain)) >>>>>> return error_mark_node; >>>>>> @@ -21902,6 +21895,53 @@ check_instantiated_args (tree tmpl, tree >>>>>> args, >>>>>> tsubst_flags_t complain) >>>>>> return result; >>>>>> } >>>>>> +/* Call mark_used on each entity within the non-type template >>>>>> arguments >>>>>> in >>>>>> + ARGS for a specialization of TMPL, to ensure that each such entity >>>>>> is >>>>>> + considered odr-used regardless of whether the specialization was >>>>>> first >>>>>> + formed in a template context. >>>>>> + >>>>>> + This function assumes push_to_top_level has been called >>>>>> beforehand, and >>>>>> + that processing_template_decl is set iff the template arguments >>>>>> are >>>>>> + dependent. */ >>>>>> + >>>>>> +static void >>>>>> +mark_template_arguments_used (tree tmpl, tree args) >>>>>> +{ >>>>>> + /* It suffices to do this only when fully specializing a primary >>>>>> template. */ >>>>>> + if (processing_template_decl || !PRIMARY_TEMPLATE_P (tmpl)) >>>>>> + return; >>>>>> + >>>>>> + /* We already marked outer arguments when specializing the context. >>>>>> */ >>>>>> + args = INNERMOST_TEMPLATE_ARGS (args); >>>>>> + >>>>>> + for (tree arg : tree_vec_range (args)) >>>>>> + { >>>>>> + /* A (pointer/reference to) function or variable NTTP argument. >>>>>> */ >>>>>> + if (TREE_CODE (arg) == ADDR_EXPR >>>>>> + || TREE_CODE (arg) == INDIRECT_REF) >>>>>> + { >>>>>> + while (TREE_CODE (arg) == ADDR_EXPR >>>>>> + || REFERENCE_REF_P (arg) >>>>>> + || CONVERT_EXPR_P (arg)) >>>>>> + arg = TREE_OPERAND (arg, 0); >>>>>> + if (DECL_P (arg)) >>>>>> + mark_used (arg, tf_none); >>>>> >>>>> Passing tf_none and also ignoring the return value needs a comment >>>>> justifying >>>>> why you assume it can't fail. >>>> >>>> *nod* Since these calls to mark_used ought to be at worst redundant, it >>>> should be safe to even assert >>>> >>>> bool ok = mark_used (arg, tf_none); >>>> gcc_checking_assert (ok || seen_error ()); >>>> >>>>> >>>>>> + } >>>>>> + /* A class NTTP argument. */ >>>>>> + else if (VAR_P (arg) >>>>>> + && DECL_NTTP_OBJECT_P (arg)) >>>>> >>>>> Maybe avoid doing this multiple times for the same NTTP object by gating >>>>> it on >>>>> DECL_ODR_USED (arg)? >>>> >>>> Looks like these VAR_DECLs don't have DECL_LANG_SPECIFIC, so we can't >>>> inspect DECL_ODR_USED for them.. >>>> >>>> It occurred to me that instantiate_class_template is perhaps the wrong >>>> place to do this marking; it'd be more appropriate to do it in >>>> lookup_template_class when forming the type specialization rather than >>>> when instantiating its definition. So in order to handle all template >>>> kinds consistently, I think we should do the marking in either >>>> instantiate_class_template and instantiate_body (i.e. at instantiation >>>> time), or in lookup_template_class and instantiate_template (i.e. at >>>> specialization time), but currently the patch does it in >>>> instantiate_class_template and instantiate_template which sounds >>>> consistent but it's sadly not. >>>> However, lookup_template_class doesn't seem to call push_to_top_level at >>>> all, so if we want to call mark_used uninhibited on the template >>>> arguments from there I think we need to add a flag to mark_used that >>>> simply disables its >>>> >>>> if (processing_template_decl || in_template_function ()) >>>> return true; >>>> >>>> early exit. IIUC the purpose of this early exit is to avoid >>>> instantiating a template due to a use from an uninstantiated template, >>>> under the assumption that we'll call mark_used again at instantiation >>>> time for that use, but in this case it seems we need to allow such >>>> ahead-of-time instantiation since we don't reprocess the template >>>> arguments of a non-dependent specialization again at instantiation time. >>>> >>>> This is quite the can of worms :/ >> >> Yes, I think I'd prefer to move toward the >> instantiate_class_template/instantiate_body consistency, to avoid unnecessary >> marking in uninstantiated templates. > Aha, makes sense, that approach seems to align best with our existing > efforts to avoid instantiation from uninstantiated templates. Like so? > Bootstrapped and regtested on x86_64-pc-linux-gnu. OK. > -- >8 -- > > Subject: [PATCH] c++: improve "NTTP argument considered unused" fix [PR53164, > PR105848] > > r13-995-g733a792a2b2e16 worked around the problem of (pointer to) > function NTTP arguments not always getting marked as odr-used, by > redundantly calling mark_used on the substituted ADDR_EXPR callee of a > CALL_EXPR. That is just a narrow workaround however, since it assumes > the function is later called. But the use as a template argument alone > should constitute an odr-use of the function (since template arguments > are an evaluated context, and we're really passing its address); we > shouldn't need to subsequently call or otherwise use the NTTP argument. > > This patch fixes this in a more general way at template instantiation > time by walking the template arguments of the specialization and calling > mark_used on all entities used within. As before, the call to mark_used > as it worst a no-op, but it compensates for the situation where we end > up forming a specialization in a template context in which mark_used is > inhibited. > > Another approach would be to call mark_used whenever we substitute a > TEMPLATE_PARM_INDEX, but that would result in many more redundant calls > to mark_used compared to this approach. And as the second testcase > below illustrates, we also need to walk C++20 class NTTP arguments which > can be large and thus expensive to walk repeatedly. The change to > invalid_tparm_referent_p is needed to avoid incorrectly rejecting class > NTTP arguments containing function pointers as in the testcase. > > (The third testcase is unrelated to this fix, but it helped rule out an > earlier approach I was considering and it seems we don't have existing > test coverage for this situation.) > > PR c++/53164 > PR c++/105848 > > gcc/cp/ChangeLog: > > * pt.cc (invalid_tparm_referent_p): Accept ADDR_EXPR of > FUNCTION_DECL. > (instantiate_class_template): Call mark_template_arguments_used. > (tsubst_copy_and_build) : Revert r13-995 change. > (mark_template_arguments_used): Define. > (instantiate_body): Call mark_template_arguments_used. > > gcc/testsuite/ChangeLog: > > * g++.dg/template/fn-ptr3a.C: New test. > * g++.dg/template/fn-ptr3b.C: New test. > * g++.dg/template/fn-ptr4.C: New test. > --- > gcc/cp/pt.cc | 86 +++++++++++++++++++----- > gcc/testsuite/g++.dg/template/fn-ptr3a.C | 27 ++++++++ > gcc/testsuite/g++.dg/template/fn-ptr3b.C | 30 +++++++++ > gcc/testsuite/g++.dg/template/fn-ptr4.C | 14 ++++ > 4 files changed, 139 insertions(+), 18 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/template/fn-ptr3a.C > create mode 100644 gcc/testsuite/g++.dg/template/fn-ptr3b.C > create mode 100644 gcc/testsuite/g++.dg/template/fn-ptr4.C > > diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc > index dd7f0db9658..022f295b36f 100644 > --- a/gcc/cp/pt.cc > +++ b/gcc/cp/pt.cc > @@ -220,6 +220,7 @@ static tree make_argument_pack (tree); > static tree enclosing_instantiation_of (tree tctx); > static void instantiate_body (tree pattern, tree args, tree d, bool nested); > static tree maybe_dependent_member_ref (tree, tree, tsubst_flags_t, tree); > +static void mark_template_arguments_used (tree, tree); > > /* Make the current scope suitable for access checking when we are > processing T. T can be FUNCTION_DECL for instantiated function > @@ -7142,12 +7143,13 @@ invalid_tparm_referent_p (tree type, tree expr, tsubst_flags_t complain) > decl = TREE_OPERAND (decl, 0); > } > > - if (!VAR_P (decl)) > + if (!VAR_OR_FUNCTION_DECL_P (decl)) > { > if (complain & tf_error) > error_at (cp_expr_loc_or_input_loc (expr), > "%qE is not a valid template argument of type %qT " > - "because %qE is not a variable", expr, type, decl); > + "because %qE is not a variable or function", > + expr, type, decl); > return true; > } > else if (cxx_dialect < cxx11 && !DECL_EXTERNAL_LINKAGE_P (decl)) > @@ -12152,6 +12154,9 @@ instantiate_class_template (tree type) > cp_unevaluated_operand = 0; > c_inhibit_evaluation_warnings = 0; > } > + > + mark_template_arguments_used (templ, CLASSTYPE_TI_ARGS (type)); > + > /* Use #pragma pack from the template context. */ > saved_maximum_field_alignment = maximum_field_alignment; > maximum_field_alignment = TYPE_PRECISION (pattern); > @@ -21192,22 +21197,10 @@ tsubst_copy_and_build (tree t, > } > > /* Remember that there was a reference to this entity. */ > - if (function != NULL_TREE) > - { > - tree inner = function; > - if (TREE_CODE (inner) == ADDR_EXPR > - && TREE_CODE (TREE_OPERAND (inner, 0)) == FUNCTION_DECL) > - /* We should already have called mark_used when taking the > - address of this function, but do so again anyway to make > - sure it's odr-used: at worst this is a no-op, but if we > - obtained this FUNCTION_DECL as part of ahead-of-time overload > - resolution then that call to mark_used wouldn't have marked it > - odr-used yet (53164). */ > - inner = TREE_OPERAND (inner, 0); > - if (DECL_P (inner) > - && !mark_used (inner, complain) && !(complain & tf_error)) > - RETURN (error_mark_node); > - } > + if (function != NULL_TREE > + && DECL_P (function) > + && !mark_used (function, complain) && !(complain & tf_error)) > + RETURN (error_mark_node); > > if (!maybe_fold_fn_template_args (function, complain)) > return error_mark_node; > @@ -21902,6 +21895,61 @@ check_instantiated_args (tree tmpl, tree args, tsubst_flags_t complain) > return result; > } > > +/* Call mark_used on each entity within the non-type template arguments in > + ARGS for an instantiation of TMPL, to ensure that each such entity is > + considered odr-used (and therefore marked for instantiation) regardless of > + whether the specialization was first formed in a template context (which > + inhibits mark_used). > + > + This function assumes push_to_top_level has been called beforehand. */ > + > +static void > +mark_template_arguments_used (tree tmpl, tree args) > +{ > + /* It suffices to do this only when instantiating a primary template. */ > + if (TREE_CODE (tmpl) != TEMPLATE_DECL || !PRIMARY_TEMPLATE_P (tmpl)) > + return; > + > + /* We already marked outer arguments when specializing the context. */ > + args = INNERMOST_TEMPLATE_ARGS (args); > + > + for (tree arg : tree_vec_range (args)) > + { > + /* A (pointer/reference to) function or variable NTTP argument. */ > + if (TREE_CODE (arg) == ADDR_EXPR > + || TREE_CODE (arg) == INDIRECT_REF) > + { > + while (TREE_CODE (arg) == ADDR_EXPR > + || REFERENCE_REF_P (arg) > + || CONVERT_EXPR_P (arg)) > + arg = TREE_OPERAND (arg, 0); > + if (VAR_OR_FUNCTION_DECL_P (arg)) > + { > + /* Pass tf_none to avoid duplicate diagnostics: if this call > + fails then an earlier call to mark_used for this argument > + must have also failed and emitted a diagnostic. */ > + bool ok = mark_used (arg, tf_none); > + gcc_checking_assert (ok || seen_error ()); > + } > + } > + /* A class NTTP argument. */ > + else if (VAR_P (arg) > + && DECL_NTTP_OBJECT_P (arg)) > + { > + auto mark_used_r = [](tree *tp, int *, void *) { > + if (VAR_OR_FUNCTION_DECL_P (*tp)) > + { > + bool ok = mark_used (*tp, tf_none); > + gcc_checking_assert (ok || seen_error ()); > + } > + return NULL_TREE; > + }; > + cp_walk_tree_without_duplicates (&DECL_INITIAL (arg), > + mark_used_r, nullptr); > + } > + } > +} > + > /* We're out of SFINAE context now, so generate diagnostics for the access > errors we saw earlier when instantiating D from TMPL and ARGS. */ > > @@ -26775,6 +26823,8 @@ instantiate_body (tree pattern, tree args, tree d, bool nested_p) > c_inhibit_evaluation_warnings = 0; > } > > + mark_template_arguments_used (pattern, args); > + > if (VAR_P (d)) > { > /* The variable might be a lambda's extra scope, and that > diff --git a/gcc/testsuite/g++.dg/template/fn-ptr3a.C b/gcc/testsuite/g++.dg/template/fn-ptr3a.C > new file mode 100644 > index 00000000000..345c621a0c6 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/template/fn-ptr3a.C > @@ -0,0 +1,27 @@ > +// PR c++/53164 > +// PR c++/105848 > +// A stricter version of fn-ptr3.C that verifies using f as a template > +// argument alone constitutes an odr-use. > + > +template > +void f(T) { T::fail; } // { dg-error "fail" } > + > +template > +struct A { > + // P not called > +}; > + > +template > +void wrap() { > + // P not called > +} > + > +template > +void g() { > + A a; // { dg-message "required from" } > + wrap(); // { dg-message "required from" } > +} > + > +int main() { > + g<0>(); > +} > diff --git a/gcc/testsuite/g++.dg/template/fn-ptr3b.C b/gcc/testsuite/g++.dg/template/fn-ptr3b.C > new file mode 100644 > index 00000000000..899c355fb38 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/template/fn-ptr3b.C > @@ -0,0 +1,30 @@ > +// PR c++/53164 > +// PR c++/105848 > +// A C++20 version of fn-ptr3a.C using class NTTPs. > +// { dg-do compile { target c++20 } } > + > +template > +void f(T) { T::fail; } // { dg-error "fail" } > + > +struct B_int { void (*P)(int); }; > +struct B_char { void (&P)(char); }; > + > +template > +struct A { > + // B.P not called > +}; > + > +template > +void wrap() { > + // B.P not called > +} > + > +template > +void g() { > + A a; // { dg-message "required from" } > + wrap(); // { dg-message "required from" } > +} > + > +int main() { > + g<0>(); > +} > diff --git a/gcc/testsuite/g++.dg/template/fn-ptr4.C b/gcc/testsuite/g++.dg/template/fn-ptr4.C > new file mode 100644 > index 00000000000..36551ec5d7f > --- /dev/null > +++ b/gcc/testsuite/g++.dg/template/fn-ptr4.C > @@ -0,0 +1,14 @@ > +// { dg-do compile } > + > +template > +void wrap() { > + P(); // OK, despite A::g not being accessible from here. > +} > + > +struct A { > + static void f() { > + wrap(); > + } > +private: > + static void g(); > +};