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 A0B173814FD8 for ; Tue, 7 Jun 2022 13:24:28 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A0B173814FD8 Received: from mail-qt1-f197.google.com (mail-qt1-f197.google.com [209.85.160.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-112-OFsjocNSNJOKHBNObcfQtQ-1; Tue, 07 Jun 2022 09:24:27 -0400 X-MC-Unique: OFsjocNSNJOKHBNObcfQtQ-1 Received: by mail-qt1-f197.google.com with SMTP id c1-20020ac81101000000b002f9219952f0so13914030qtj.15 for ; Tue, 07 Jun 2022 06:24:23 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:date:to:cc:subject:in-reply-to:message-id :references:mime-version; bh=BTSpqCBwYIvj5WETQdDastnkzCr6sOUggWwEVEXI+lE=; b=F987YtA7NXpdiuFsIu309xUsVbs9FKWEdXsIcA0gTz9WAZEIC/3Yc2fXCru1ibK9Kj GJO61a+mrB6WzroCsY4ks7F3re/DVVI5ZppCVV4e59Xp6Le7H5VALVeZUZnvpm/vP2i+ otixDvN1hytdmlc5WoM5uvUzFoxdbY7qIa/IpEdCkgS3xZ73FnA5tafslF5+l/jIH791 zbU8vwBqa8JPcZ4kWftPpKUJdIFRZlM0RlxoRta22/9HtrLG2S6mDs+yDaJ6kI5limSH Ki0d8ywAGWglRB96a8qj4j2vSyknULXeEzg1CGq7W/kEYCqmn43luUmJZ3baHHmA57J9 MXmQ== X-Gm-Message-State: AOAM531Dpq9pm7U86sRm3Lk+Lk7/cRc/TxX5EwWOAJdQwj2skTkc642N liaeaFRp9GocBF2MqIf0nWG1m/QmGoxoUKF6Hw4SVR4GK6oV871xR2tTrpSr31ZQJsEUeLKMazo LBrdAt4i9V0esyuAQAA== X-Received: by 2002:a05:6214:ace:b0:461:e896:b653 with SMTP id g14-20020a0562140ace00b00461e896b653mr19355598qvi.91.1654608262384; Tue, 07 Jun 2022 06:24:22 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzk4ZlnKtttrVDXNvNRVui85pDzxfGObrlcmoWuChxb9YPt0wszTIfudSI23cmymQhWeWj2TA== X-Received: by 2002:a05:6214:ace:b0:461:e896:b653 with SMTP id g14-20020a0562140ace00b00461e896b653mr19355555qvi.91.1654608261834; Tue, 07 Jun 2022 06:24:21 -0700 (PDT) Received: from [192.168.1.130] (ool-457670bb.dyn.optonline.net. [69.118.112.187]) by smtp.gmail.com with ESMTPSA id e5-20020ac81305000000b00304e38fb3dasm7332324qtj.35.2022.06.07.06.24.21 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 07 Jun 2022 06:24:21 -0700 (PDT) From: Patrick Palka X-Google-Original-From: Patrick Palka Date: Tue, 7 Jun 2022 09:24:20 -0400 (EDT) To: Jason Merrill cc: Patrick Palka , gcc-patches@gcc.gnu.org Subject: Re: [PATCH] c++: function NTTP argument considered unused [PR53164, PR105848] In-Reply-To: <41cec505-46f4-5622-ab08-e693fa8b7d90@redhat.com> Message-ID: <50baf432-cc69-45b9-0325-fe1bb7418308@idea> References: <95c2097c-5466-fcae-fdcd-fafdf1ab9431@idea> <41cec505-46f4-5622-ab08-e693fa8b7d90@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-14.7 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, 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 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT X-Content-Filtered-By: Mailman/MimeDel 2.1.29 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 13:24:31 -0000 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. -- >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))) + /* 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>(); +} -- 2.36.1.299.gab336e8f1c