On Tue, 7 Jun 2022, Jason Merrill wrote:
> 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<class T>
> > > > > > > > > > > > +void f(T) {
> > > > > > > > > > > > + T::fail; // { dg-error "not a member" }
> > > > > > > > > > > > +}
> > > > > > > > > > > > +
> > > > > > > > > > > > +template<void(int)>
> > > > > > > > > > > > +struct A { };
> > > > > > > > > > > > +
> > > > > > > > > > > > +template<int>
> > > > > > > > > > > > +void g() {
> > > > > > > > > > > > + A<f> 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<void p(int)>
> > > > > > > > > > > struct A {
> > > > > > > > > > > static void h() {
> > > > > > > > > > > p(0);
> > > > > > > > > > > }
> > > > > > > > > > > };
> > > > > > > > > > >
> > > > > > > > > > > template<int>
> > > > > > > > > > > void g() {
> > > > > > > > > > > A<f>::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<f>, but convert_nontype_argument_function seems like a
> > > > > > > > > > weird
> > > > > > > > > > place
> > > > > > > > > > to
> > > > > > > > > > implement that; it's only called again during instantiation
> > > > > > > > > > of
> > > > > > > > > > A<f>,
> > > > > > > > > > when we
> > > > > > > > > > instantiate the injected-class-name. If A<f> isn't
> > > > > > > > > > instantiated,
> > > > > > > > > > e.g.
> > > > > > > > > > if 'a'
> > > > > > > > > > is a pointer to A<f>, we again don't instantiate f<int>.
> > > > > > > > >
> > > > > > > > > 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<int>
> > > > > > > > > ahead
> > > > > > > > > of
> > > > > > > > > time (during which mark_used doesn't actually instantiate
> > > > > > > > > f<int>
> > > > > > > > > because
> > > > > > > > > we're inside a template), at instantiation time the type A<f>
> > > > > > > > > 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<f> a;', it does reject
> > > > > > > > > 'using type = A<f>;' 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 P()>
> > > > > > > > void g() {
> > > > > > > > P(); // error: ‘static void A::h()’ is private within
> > > > > > > > this
> > > > > > > > context
> > > > > > > > }
> > > > > > > >
> > > > > > > > struct A {
> > > > > > > > void f() {
> > > > > > > > g<h>();
> > > > > > > > }
> > > > > > > > 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<f> 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<f>
> > > > > > gets
> > > > > > resolved ahead of time to the FUNCTION_DECL f<int>, and we defer
> > > > > > marking
> > > > > > f<int> as used until instantiation (of g) as usual.
> > > > > >
> > > > > > Later when instantiating g the type A<f> (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<int> as used
> > > > > > (which
> > > > > > means
> > > > > > we never instantiate it) even though A<f>::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) <case CALL_EXPR>: 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.
I suspect the same, but will benchmark to make sure.
>
> > -- >8 --
> >
> > Subject: [PATCH] c++: improve "NTTP argument considered unused" fix
> > [PR53164]
> >
> > PR c++/53164
> > PR c++/105848
> >
> > gcc/cp/ChangeLog:
> >
> > * pt.cc (tsubst) <case TEMPLATE_PARM_INDEX>: Call mark_used on
> > an ADDR_EXPR argument.
> > (tsubst_copy_and_build) <case CALL_EXPR>: Revert r13-995 change.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * g++.dg/template/fn-ptr3a.C: New test.
> >
> > Co-authored-by: Jason Merrill <ja...@redhat.com>
> > ---
> > 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?
Ah yeah, NOP_EXPR as well as REFERENCE_REF_P it seems. (And I guess in
light of C++20 class NTTPs we might need to walk the entire argument in
search of decls to call mark_used on, but that seems overkill, Clang
doesn't seem to handle that either.)
I'm testing the following, which additionally handles/tests the
reference-to-function case:
-- >8 --
Subject: [PATCH] c++: improve "NTTP argument considered unused" fix [PR53164,
PR105848]
PR c++/53164
PR c++/105848
gcc/cp/ChangeLog:
* pt.cc (tsubst) <case TEMPLATE_PARM_INDEX>: Call mark_used on
an ADDR_EXPR argument.
(tsubst_copy_and_build) <case CALL_EXPR>: Revert r13-995 change.
gcc/testsuite/ChangeLog:
* g++.dg/template/fn-ptr3a.C: New test.
Co-authored-by: Jason Merrill <ja...@redhat.com>
---
gcc/cp/pt.cc | 43 +++++++++++++-----------
gcc/testsuite/g++.dg/template/fn-ptr3a.C | 25 ++++++++++++++
2 files changed, 49 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..ac9b5a5bb04 100644
--- a/gcc/cp/pt.cc
+++ b/gcc/cp/pt.cc
@@ -15858,9 +15858,26 @@ 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) */
+ {
+ tree sub = arg;
+ while (CONVERT_EXPR_P (sub) || REFERENCE_REF_P (sub))
+ sub = TREE_OPERAND (sub, 0);
+ if (TREE_CODE (sub) == ADDR_EXPR
+ && DECL_P (TREE_OPERAND (sub, 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 so that we don't issue
+ redundant diagnostics -- we just want to make sure
+ this entity is odr-used. */
+ mark_used (TREE_OPERAND (sub, 0),
+ complain & ~tf_warning_or_error);
+ return convert_from_reference (unshare_expr (arg));
+ }
}
if (level == 1)
@@ -20884,22 +20901,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..7456be5d51f
--- /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<class T>
+void f(T) { T::fail; } // { dg-error "fail" }
+
+template<void (*P)(int)>
+struct A {
+ static void wrap();
+};
+
+template<void (&P)(char)>
+void wrap();
+
+template<int>
+void g() {
+ A<f>::wrap(); // { dg-message "required from here" }
+ wrap<f>(); // { dg-message "required from here" }
+}
+
+int main() {
+ g<0>();
+}
--
2.36.1.299.gab336e8f1c
I'm testing the folowing:
