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 ESMTPS id 85F993952021 for ; Tue, 7 Jun 2022 18:05:08 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 85F993952021 Received: from mail-qv1-f70.google.com (mail-qv1-f70.google.com [209.85.219.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-490-rl1C6l09MDySZxJ3Fn3P5Q-1; Tue, 07 Jun 2022 14:05:07 -0400 X-MC-Unique: rl1C6l09MDySZxJ3Fn3P5Q-1 Received: by mail-qv1-f70.google.com with SMTP id fw9-20020a056214238900b0043522aa5b81so11379972qvb.21 for ; Tue, 07 Jun 2022 11:05:07 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=K67SlL77Is4C9bpd0xRutWCJB5DYIk2wiI5JF+sYrLc=; b=N0b0htvd83AyKUnGi0YW2joOr38iUSKgt6ijMaZbrmR8uRxx9nk6zd0L0tMSgCTcl5 ahFto+nDRimWQ9+Dm7rDBmDfRBLUv/qiVHO6y5zlPglLptnkDvRQiS/C587hkduxHqnL f/wNsMErwAnSv/BBpW6P/IC2bgXmy3srXiNCK0DC43QbDoHNeTrehMwLJYFFEdGOZNGI oriLONNQBfbLRMusYjq/augvKLZOXUS2dAVGUaZv3sK819XKYDAFXQPIprl86cLSRewo 1EXmREI7K+UFtK67k5Pu/vEsHWrw8Ejw+7y0MGrNFH6zy0lf/JPgyyBcbp3g+h37yIgL F7JQ== X-Gm-Message-State: AOAM532YTGaorfd223lAMC4DK1LQLGwvVyaB9RUEt2ASuMHOvO74o0JR 4lzqyhRdTJp2DivAjHXYOhF4QRoxpe1lsCNQVQHE8Ehd4aBgnXbRWn88HNsiOfs7W17X9GVu6rY QH0zGj95+LIYtKtuZdA== X-Received: by 2002:a05:6214:29ca:b0:464:4830:b0e7 with SMTP id gh10-20020a05621429ca00b004644830b0e7mr22510228qvb.8.1654625106036; Tue, 07 Jun 2022 11:05:06 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxabdXgj5Rg03l6D+f1EV2ErlAM7mTwlLCNKyNIfJX+1jBHHwq+2AqZa5uA8uMlyXmyejwI3g== X-Received: by 2002:a05:6214:29ca:b0:464:4830:b0e7 with SMTP id gh10-20020a05621429ca00b004644830b0e7mr22510172qvb.8.1654625105432; Tue, 07 Jun 2022 11:05:05 -0700 (PDT) Received: from [192.168.1.100] (130-44-159-43.s15913.c3-0.arl-cbr1.sbo-arl.ma.cable.rcncustomer.com. [130.44.159.43]) by smtp.gmail.com with ESMTPSA id l18-20020a05620a28d200b006a6d365e9b1sm3056103qkp.57.2022.06.07.11.05.04 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 07 Jun 2022 11:05:04 -0700 (PDT) Message-ID: Date: Tue, 7 Jun 2022 14:05:03 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.10.0 Subject: Re: [PATCH] c++: function NTTP argument considered unused [PR53164, PR105848] To: Patrick Palka Cc: gcc-patches@gcc.gnu.org References: <95c2097c-5466-fcae-fdcd-fafdf1ab9431@idea> <41cec505-46f4-5622-ab08-e693fa8b7d90@redhat.com> <50baf432-cc69-45b9-0325-fe1bb7418308@idea> From: Jason Merrill In-Reply-To: <50baf432-cc69-45b9-0325-fe1bb7418308@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: 8bit X-Spam-Status: No, score=-13.6 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, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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: Tue, 07 Jun 2022 18:05:11 -0000 On 6/7/22 09:24, Patrick Palka wrote: > On Mon, 6 Jun 2022, Jason Merrill wrote: > >> On 6/6/22 14:27, Patrick Palka wrote: >>> On Thu, 7 Oct 2021, Jason Merrill wrote: >>> >>>> On 10/7/21 11:17, Patrick Palka wrote: >>>>> On Wed, 6 Oct 2021, Jason Merrill wrote: >>>>> >>>>>> On 10/6/21 15:52, Patrick Palka wrote: >>>>>>> On Wed, 6 Oct 2021, Patrick Palka wrote: >>>>>>> >>>>>>>> On Tue, 5 Oct 2021, Jason Merrill wrote: >>>>>>>> >>>>>>>>> On 10/5/21 15:17, Patrick Palka wrote: >>>>>>>>>> On Mon, 4 Oct 2021, Patrick Palka wrote: >>>>>>>>>> >>>>>>>>>>> When passing a function template as the argument to a >>>>>>>>>>> function >>>>>>>>>>> NTTP >>>>>>>>>>> inside a template, we resolve it to the right specialization >>>>>>>>>>> ahead >>>>>>>>>>> of >>>>>>>>>>> time via resolve_address_of_overloaded_function, though the >>>>>>>>>>> call >>>>>>>>>>> to >>>>>>>>>>> mark_used within defers odr-using it until instantiation >>>>>>>>>>> time >>>>>>>>>>> (as >>>>>>>>>>> usual). >>>>>>>>>>> But at instantiation time we end up never calling mark_used >>>>>>>>>>> on >>>>>>>>>>> the >>>>>>>>>>> specialization. >>>>>>>>>>> >>>>>>>>>>> This patch fixes this by adding a call to mark_used in >>>>>>>>>>> convert_nontype_argument_function. >>>>>>>>>>> >>>>>>>>>>> PR c++/53164 >>>>>>>>>>> >>>>>>>>>>> gcc/cp/ChangeLog: >>>>>>>>>>> >>>>>>>>>>> * pt.c (convert_nontype_argument_function): Call >>>>>>>>>>> mark_used. >>>>>>>>>>> >>>>>>>>>>> gcc/testsuite/ChangeLog: >>>>>>>>>>> >>>>>>>>>>> * g++.dg/template/non-dependent16.C: New test. >>>>>>>>>>> --- >>>>>>>>>>> gcc/cp/pt.c | 3 >>>>>>>>>>> +++ >>>>>>>>>>> gcc/testsuite/g++.dg/template/non-dependent16.C | 16 >>>>>>>>>>> ++++++++++++++++ >>>>>>>>>>> 2 files changed, 19 insertions(+) >>>>>>>>>>> create mode 100644 >>>>>>>>>>> gcc/testsuite/g++.dg/template/non-dependent16.C >>>>>>>>>>> >>>>>>>>>>> diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c >>>>>>>>>>> index f950f4a21b7..5e819c9598c 100644 >>>>>>>>>>> --- a/gcc/cp/pt.c >>>>>>>>>>> +++ b/gcc/cp/pt.c >>>>>>>>>>> @@ -6668,6 +6668,9 @@ convert_nontype_argument_function >>>>>>>>>>> (tree >>>>>>>>>>> type, >>>>>>>>>>> tree >>>>>>>>>>> expr, >>>>>>>>>>> return NULL_TREE; >>>>>>>>>>> } >>>>>>>>>>> + if (!mark_used (fn_no_ptr, complain) && !(complain & >>>>>>>>>>> tf_error)) >>>>>>>>>>> + return NULL_TREE; >>>>>>>>>>> + >>>>>>>>>>> linkage = decl_linkage (fn_no_ptr); >>>>>>>>>>> if (cxx_dialect >= cxx11 ? linkage == lk_none : >>>>>>>>>>> linkage != >>>>>>>>>>> lk_external) >>>>>>>>>>> { >>>>>>>>>>> diff --git a/gcc/testsuite/g++.dg/template/non-dependent16.C >>>>>>>>>>> b/gcc/testsuite/g++.dg/template/non-dependent16.C >>>>>>>>>>> new file mode 100644 >>>>>>>>>>> index 00000000000..b7dca8f6752 >>>>>>>>>>> --- /dev/null >>>>>>>>>>> +++ b/gcc/testsuite/g++.dg/template/non-dependent16.C >>>>>>>>>>> @@ -0,0 +1,16 @@ >>>>>>>>>>> +// PR c++/53164 >>>>>>>>>>> + >>>>>>>>>>> +template >>>>>>>>>>> +void f(T) { >>>>>>>>>>> + T::fail; // { dg-error "not a member" } >>>>>>>>>>> +} >>>>>>>>>>> + >>>>>>>>>>> +template >>>>>>>>>>> +struct A { }; >>>>>>>>>>> + >>>>>>>>>>> +template >>>>>>>>>>> +void g() { >>>>>>>>>>> + A a; >>>>>>>>>>> +} >>>>>>>>>> >>>>>>>>>> I should mention that the original testcase in the PR was >>>>>>>>>> slightly >>>>>>>>>> different than this one in that it also performed a call to >>>>>>>>>> the >>>>>>>>>> NTTP, >>>>>>>>>> e.g. >>>>>>>>>> >>>>>>>>>> template >>>>>>>>>> struct A { >>>>>>>>>> static void h() { >>>>>>>>>> p(0); >>>>>>>>>> } >>>>>>>>>> }; >>>>>>>>>> >>>>>>>>>> template >>>>>>>>>> void g() { >>>>>>>>>> A::h(); >>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> templated void g<0>(); >>>>>>>>>> >>>>>>>>>> and not even the call was enough to odr-use f, apparently >>>>>>>>>> because >>>>>>>>>> the >>>>>>>>>> CALL_EXPR case of tsubst_expr calls mark_used on the callee >>>>>>>>>> only >>>>>>>>>> when >>>>>>>>>> it's a FUNCTION_DECL, but in this case after substitution it's >>>>>>>>>> an >>>>>>>>>> ADDR_EXPR of a FUNCTION_DECL. Fixing this by looking through >>>>>>>>>> the >>>>>>>>>> ADDR_EXPR >>>>>>>>>> worked, but IIUC the call isn't necessary for f to be >>>>>>>>>> odr-used, >>>>>>>>>> simply >>>>>>>>>> using f as a template argument should be sufficient, so it >>>>>>>>>> seems >>>>>>>>>> the >>>>>>>>>> above is better fix. >>>>>>>>> >>>>>>>>> I agree that pedantically the use happens when substituting into >>>>>>>>> the >>>>>>>>> use >>>>>>>>> of >>>>>>>>> A, but convert_nontype_argument_function seems like a weird >>>>>>>>> place >>>>>>>>> to >>>>>>>>> implement that; it's only called again during instantiation of >>>>>>>>> A, >>>>>>>>> when we >>>>>>>>> instantiate the injected-class-name. If A isn't >>>>>>>>> instantiated, >>>>>>>>> e.g. >>>>>>>>> if 'a' >>>>>>>>> is a pointer to A, we again don't instantiate f. >>>>>>>> >>>>>>>> I see, makes sense.. I'm not sure where else we can mark the use, >>>>>>>> then. >>>>>>>> Since we resolve the OVERLOAD f to the FUNCTION_DECL f ahead >>>>>>>> of >>>>>>>> time (during which mark_used doesn't actually instantiate f >>>>>>>> because >>>>>>>> we're inside a template), at instantiation time the type A is >>>>>>>> already >>>>>>>> non-dependent so tsubst_aggr_type avoids doing the work that would >>>>>>>> end >>>>>>>> up calling convert_nontype_argument_function. >>>>>>>> >>>>>>>>> >>>>>>>>> I see that clang doesn't reject your testcase, either, but MSVC >>>>>>>>> and >>>>>>>>> icc >>>>>>>>> do >>>>>>>>> (even with 'a' a pointer): https://godbolt.org/z/MGE6TcMch >>>>>>>> >>>>>>>> FWIW although Clang doesn't reject 'A a;', it does reject >>>>>>>> 'using type = A;' weirdly enough: >>>>>>>> https://godbolt.org/z/T9qEn6bWW >>>>>>>> >>>>>>>> >>>>>>>> Shall we just go with the other more specific approach, that makes >>>>>>>> sure >>>>>>>> the CALL_EXPR case of tsubst_expr calls mark_used when the callee >>>>>>>> is >>>>>>>> an >>>>>>>> ADDR_EXPR? Something like (bootstrapped and regtested): >>>>>>> >>>>>>> Err, this approach is wrong because by stripping the ADDR_EXPR here >>>>>>> we >>>>>>> end up checking access of the unwrapped FUNCTION_DECL again after >>>>>>> substituting into the call. So we incorrectly reject e.g. >>>>>>> >>>>>>> template >>>>>>> void g() { >>>>>>> P(); // error: ‘static void A::h()’ is private within this >>>>>>> context >>>>>>> } >>>>>>> >>>>>>> struct A { >>>>>>> void f() { >>>>>>> g(); >>>>>>> } >>>>>>> private: >>>>>>> static void h(); >>>>>>> }; >>>>>>> >>>>>>> since A::h isn't accessible from g. >>>>>> >>>>>> I guess you could call mark_used directly instead of stripping the >>>>>> ADDR_EXPR. >>>>> >>>>> That seems to work nicely, how does the below look? Bootstrapped and >>>>> regtested on x86_64-pc-linux-gnu. >>>>> >>>>>> >>>>>> Or for the general problem, perhaps we could mark the TEMPLATE_INFO or >>>>>> TI_ARGS >>>>>> to indicate that we still need to mark_used the arguments when we >>>>>> encounter >>>>>> A again during instantiation? >>>>> >>>>> That sounds plausible, though I suppose it might not be worth it only to >>>>> handle such a corner case.. >>>> >>>> Indeed. A lower-overhead possibility would be to remember, for a >>>> template, >>>> decls that we wanted to mark_used but didn't because we were in a >>>> template. >>>> But I wouldn't worry about it for now. >>>> >>>>> -- >8 -- >>>>> >>>>> Subject: [PATCH] c++: function NTTP argument considered unused [PR53164] >>>>> >>>>> Here at parse time the template argument f (an OVERLOAD) in A gets >>>>> resolved ahead of time to the FUNCTION_DECL f, and we defer marking >>>>> f as used until instantiation (of g) as usual. >>>>> >>>>> Later when instantiating g the type A (where f has already been >>>>> resolved) >>>>> is non-dependent, so tsubst_aggr_type avoids re-processing its template >>>>> arguments, and we end up never actually marking f as used (which >>>>> means >>>>> we never instantiate it) even though A::h() calls it. >>>>> >>>>> This patch works around this problem by making us look through ADDR_EXPR >>>>> when calling mark_used on the callee of a substituted CALL_EXPR. >>>>> >>>>> PR c++/53164 >>>>> >>>>> gcc/cp/ChangeLog: >>>>> >>>>> * pt.c (tsubst_copy_and_build) : Look through an >>>>> ADDR_EXPR callee when calling mark_used. >>>>> >>>>> gcc/testsuite/ChangeLog: >>>>> >>>>> * g++.dg/template/fn-ptr3.C: New test. >>>>> --- >>>>> gcc/cp/pt.c | 12 ++++++++---- >>>>> gcc/testsuite/g++.dg/template/fn-ptr3.C | 20 ++++++++++++++++++++ >>>>> 2 files changed, 28 insertions(+), 4 deletions(-) >>>>> create mode 100644 gcc/testsuite/g++.dg/template/fn-ptr3.C >>>>> >>>>> diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c >>>>> index 1e52aa757e1..cd10340ce12 100644 >>>>> --- a/gcc/cp/pt.c >>>>> +++ b/gcc/cp/pt.c >>>>> @@ -20508,10 +20508,14 @@ tsubst_copy_and_build (tree t, >>>>> } >>>>> /* Remember that there was a reference to this entity. */ >>>>> - if (function != NULL_TREE >>>>> - && DECL_P (function) >>>>> - && !mark_used (function, complain) && !(complain & tf_error)) >>>>> - RETURN (error_mark_node); >>>>> + if (function) >>>>> + { >>>>> + tree sub = function; >>>>> + if (TREE_CODE (sub) == ADDR_EXPR) >>>>> + sub = TREE_OPERAND (sub, 0); >>>> >>>> Let's add a comment about why this is needed. OK with that change. >>> >>> Thanks. I ended up neglecting to commit this patch for GCC 12, and >>> turns out it also fixes the recently reported regression PR105848 >>> (which is essentially another manifestation of PR53164 exposed by the >>> non-dependent overload set pruning patch r12-6075), so I'm going to >>> commit this now. I suppose it's also OK to backport to 12? >> >> What if the use is not as a call, but say assigning the address to a global >> pointer? >> >> I wonder about calling mark_used when substituting the TEMPLATE_PARM_INDEX, >> probably with tf_none or tf_error to avoid redundant deprecated warnings. > > Ah, that seems to work very nicely -- it even handles the case where there the > only use is as a template argument and there is no "use" inside the body > of the function, because we always have to substitute the TEMPLATE_PARM_INDEX > in the generic template arguments. > > Like so? Bootstrapped and tested on x86_64-pc-linux-gnu. I'm not sure > if it's worth also checking !processing_template_decl && !DECL_ODR_USED > before calling mark_used here, since otherwise the call ought to be > certainly redundant. Wouldn't hurt to benchmark that, but I wouldn't expect it to make a measurable difference. > -- >8 -- > > Subject: [PATCH] c++: improve "NTTP argument considered unused" fix [PR53164] > > PR c++/53164 > PR c++/105848 > > gcc/cp/ChangeLog: > > * pt.cc (tsubst) : Call mark_used on > an ADDR_EXPR argument. > (tsubst_copy_and_build) : Revert r13-995 change. > > gcc/testsuite/ChangeLog: > > * g++.dg/template/fn-ptr3a.C: New test. > > Co-authored-by: Jason Merrill > --- > gcc/cp/pt.cc | 40 +++++++++++++----------- > gcc/testsuite/g++.dg/template/fn-ptr3a.C | 25 +++++++++++++++ > 2 files changed, 46 insertions(+), 19 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/template/fn-ptr3a.C > > diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc > index dcacba56a1c..5863d83b4fd 100644 > --- a/gcc/cp/pt.cc > +++ b/gcc/cp/pt.cc > @@ -15858,9 +15858,23 @@ tsubst (tree t, tree args, tsubst_flags_t complain, tree in_decl) > } > else if (code == TEMPLATE_TEMPLATE_PARM) > return arg; > - else > - /* TEMPLATE_PARM_INDEX. */ > - return convert_from_reference (unshare_expr (arg)); > + else /* if (code == TEMPLATE_PARM_INDEX) */ > + { > + if (TREE_CODE (arg) == ADDR_EXPR > + && DECL_P (TREE_OPERAND (arg, 0))) If the parameter is a reference to function, rather than a pointer, do we need to look through a NOP_EXPR? > + /* Remember that there was a reference to this entity. > + We should already have called mark_used when taking > + the address of this entity, but do so again to make > + sure: at worst it's redundant, but if we so far only > + called mark_used in a template context then we might > + not have yet marked it odr-used (53164). */ > + /* Mask out tf_warning_or_error to avoid issuing redundant > + diagnostics -- we just care about making sure this > + entity is odr-used. */ > + mark_used (TREE_OPERAND (arg, 0), > + complain & ~tf_warning_or_error); > + return convert_from_reference (unshare_expr (arg)); > + } > } > > if (level == 1) > @@ -20884,22 +20898,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; > 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..c76b5cc32b3 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/template/fn-ptr3a.C > @@ -0,0 +1,25 @@ > +// 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 { > + static void wrap(); > +}; > + > +template > +void wrap(); > + > +template > +void g() { > + A::wrap(); // { dg-message "required from here" } > + wrap(); // { dg-message "required from here" } > +} > + > +int main() { > + g<0>(); > +}