Re: [PATCH] c++: function NTTP argument considered unused [PR53164, PR105848]

[Patrick Palka <ppalka@redhat.com>]

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.

-- >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 <jason@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)))
+		  /* 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<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