[1/2] c++: improve "NTTP argument considered unused" fix [PR53164, PR105848]

  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.

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) <case CALL_EXPR>: 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
  

On Thu, Mar 23, 2023 at 5:18 PM Patrick Palka <ppalka@redhat.com> 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.
>
> 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.

Note we previously discussed this TEMPLATE_PARM_INDEX approach
here https://gcc.gnu.org/pipermail/gcc-patches/2022-June/596257.html
though I never pushed it since it felt somewhat overkill to me every single
substituted use of an NTTP would be considered for marking;
 perhaps this approach might be preferable? Yet another approach would
be to do it from tsubst_template_args..

>
> 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) <case CALL_EXPR>: 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);
> +}
> +
>  /* We're out of SFINAE context now, so generate diagnostics for the access
>     errors we saw earlier when instantiating D from TMPL and ARGS.  */
>
> @@ -22012,6 +22029,8 @@ instantiate_template (tree tmpl, tree orig_args, tsubst_flags_t complain)
>        push_nested_class (ctx);
>      }
>
> +  mark_template_arguments_used (INNERMOST_TEMPLATE_ARGS (targ_ptr));
> +
>    tree pattern = DECL_TEMPLATE_RESULT (gen_tmpl);
>
>    fndecl = NULL_TREE;
> 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>();
> +}
> 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..e7425ba96cc
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/template/fn-ptr4.C
> @@ -0,0 +1,14 @@
> +// { dg-do compile }
> +
> +template<void (*P)()>
> +void wrap() {
> +  P(); // OK, despite A::g not being accessible from foo.
> +}
> +
> +struct A {
> +  static void f() {
> +    wrap<A::g>();
> +  }
> +private:
> +  static void g();
> +};
> --
> 2.40.0.130.g27d43aaaf5
>
  

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.
> 
> 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) <case CALL_EXPR>: 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) <case CALL_EXPR>: 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);
+      }
+}
+
 /* We're out of SFINAE context now, so generate diagnostics for the access
    errors we saw earlier when instantiating D from TMPL and ARGS.  */
 
@@ -22031,6 +22054,8 @@ instantiate_template (tree tmpl, tree orig_args, tsubst_flags_t complain)
       push_nested_class (ctx);
     }
 
+  mark_template_arguments_used (gen_tmpl, targ_ptr);
+
   tree pattern = DECL_TEMPLATE_RESULT (gen_tmpl);
 
   fndecl = NULL_TREE;
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>();
+}
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 (*P)()>
+void wrap() {
+  P(); // OK, despite A::g not being accessible from here.
+}
+
+struct A {
+  static void f() {
+    wrap<A::g>();
+  }
+private:
+  static void g();
+};
  

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.

>> 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) <case CALL_EXPR>: 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) <case CALL_EXPR>: 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?

> +      }
> +}
> +
>   /* We're out of SFINAE context now, so generate diagnostics for the access
>      errors we saw earlier when instantiating D from TMPL and ARGS.  */
>   
> @@ -22031,6 +22054,8 @@ instantiate_template (tree tmpl, tree orig_args, tsubst_flags_t complain)
>         push_nested_class (ctx);
>       }
>   
> +  mark_template_arguments_used (gen_tmpl, targ_ptr);
> +
>     tree pattern = DECL_TEMPLATE_RESULT (gen_tmpl);
>   
>     fndecl = NULL_TREE;
> 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>();
> +}
> 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 (*P)()>
> +void wrap() {
> +  P(); // OK, despite A::g not being accessible from here.
> +}
> +
> +struct A {
> +  static void f() {
> +    wrap<A::g>();
> +  }
> +private:
> +  static void g();
> +};
  

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) <case CALL_EXPR>: 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) <case CALL_EXPR>: 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) <case CALL_EXPR>: 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);
+	}
+      /* A class NTTP argument.  */
+      else if (VAR_P (arg)
+	       && DECL_NTTP_OBJECT_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 (&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.  */
 
@@ -22031,6 +22071,8 @@ instantiate_template (tree tmpl, tree orig_args, tsubst_flags_t complain)
       push_nested_class (ctx);
     }
 
+  mark_template_arguments_used (gen_tmpl, targ_ptr);
+
   tree pattern = DECL_TEMPLATE_RESULT (gen_tmpl);
 
   fndecl = NULL_TREE;
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>();
+}
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..90d988ce896
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/fn-ptr3b.C
@@ -0,0 +1,28 @@
+// PR c++/53164
+// PR c++/105848
+// A C++20 version of fn-ptr3a.C using class NTTPs.
+// { dg-do compile { target c++20 } }
+
+template<class T>
+void f(T) { T::fail; } // { dg-error "fail" }
+
+struct B_int { void (*P)(int); };
+struct B_char { void (*P)(char); };
+
+template<B_int B>
+struct A {
+  static void wrap();
+};
+
+template<B_char B>
+void wrap();
+
+template<int>
+void g() {
+  A<B_int{f}>::wrap(); // { dg-message "required from here" }
+  wrap<B_char{f}>(); // { dg-message "required from here" }
+}
+
+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 (*P)()>
+void wrap() {
+  P(); // OK, despite A::g not being accessible from here.
+}
+
+struct A {
+  static void f() {
+    wrap<A::g>();
+  }
+private:
+  static void g();
+};
  

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) <case CALL_EXPR>: 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) <case CALL_EXPR>: 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) <case CALL_EXPR>: 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.

> +	}
> +      /* 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)?

> +	{
> +	  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 (&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.  */
>   
> @@ -22031,6 +22071,8 @@ instantiate_template (tree tmpl, tree orig_args, tsubst_flags_t complain)
>         push_nested_class (ctx);
>       }
>   
> +  mark_template_arguments_used (gen_tmpl, targ_ptr);
> +
>     tree pattern = DECL_TEMPLATE_RESULT (gen_tmpl);
>   
>     fndecl = NULL_TREE;
> 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>();
> +}
> 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..90d988ce896
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/template/fn-ptr3b.C
> @@ -0,0 +1,28 @@
> +// PR c++/53164
> +// PR c++/105848
> +// A C++20 version of fn-ptr3a.C using class NTTPs.
> +// { dg-do compile { target c++20 } }
> +
> +template<class T>
> +void f(T) { T::fail; } // { dg-error "fail" }
> +
> +struct B_int { void (*P)(int); };
> +struct B_char { void (*P)(char); };
> +
> +template<B_int B>
> +struct A {
> +  static void wrap();
> +};
> +
> +template<B_char B>
> +void wrap();
> +
> +template<int>
> +void g() {
> +  A<B_int{f}>::wrap(); // { dg-message "required from here" }
> +  wrap<B_char{f}>(); // { dg-message "required from here" }
> +}
> +
> +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 (*P)()>
> +void wrap() {
> +  P(); // OK, despite A::g not being accessible from here.
> +}
> +
> +struct A {
> +  static void f() {
> +    wrap<A::g>();
> +  }
> +private:
> +  static void g();
> +};
  

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) <case CALL_EXPR>: 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) <case CALL_EXPR>: 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) <case CALL_EXPR>: 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 :/

> 
> > +	{
> > +	  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 (&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.  */
> >   @@ -22031,6 +22071,8 @@ instantiate_template (tree tmpl, tree orig_args,
> > tsubst_flags_t complain)
> >         push_nested_class (ctx);
> >       }
> >   +  mark_template_arguments_used (gen_tmpl, targ_ptr);
> > +
> >     tree pattern = DECL_TEMPLATE_RESULT (gen_tmpl);
> >       fndecl = NULL_TREE;
> > 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>();
> > +}
> > 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..90d988ce896
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/template/fn-ptr3b.C
> > @@ -0,0 +1,28 @@
> > +// PR c++/53164
> > +// PR c++/105848
> > +// A C++20 version of fn-ptr3a.C using class NTTPs.
> > +// { dg-do compile { target c++20 } }
> > +
> > +template<class T>
> > +void f(T) { T::fail; } // { dg-error "fail" }
> > +
> > +struct B_int { void (*P)(int); };
> > +struct B_char { void (*P)(char); };
> > +
> > +template<B_int B>
> > +struct A {
> > +  static void wrap();
> > +};
> > +
> > +template<B_char B>
> > +void wrap();
> > +
> > +template<int>
> > +void g() {
> > +  A<B_int{f}>::wrap(); // { dg-message "required from here" }
> > +  wrap<B_char{f}>(); // { dg-message "required from here" }
> > +}
> > +
> > +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 (*P)()>
> > +void wrap() {
> > +  P(); // OK, despite A::g not being accessible from here.
> > +}
> > +
> > +struct A {
> > +  static void f() {
> > +    wrap<A::g>();
> > +  }
> > +private:
> > +  static void g();
> > +};
> 
>
  

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) <case CALL_EXPR>: 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) <case CALL_EXPR>: 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) <case CALL_EXPR>: 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 :/

This is what I have so far, it's imperfect but perhaps good enough?
bootstrapped and regtested on x86_64-pc-linux-gnu:

-- >8 --

Subject: [PATCH] c++: improve "NTTP argument considered unused" fix [PR53164,
 PR105848]

	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) <case CALL_EXPR>: 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                             | 88 +++++++++++++++++++-----
 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, 137 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..83902b0be8d 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,63 @@ 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 (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, 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 (VAR_OR_FUNCTION_DECL_P (arg))
+	    {
+	      /* Pass tf_none to avoid duplicate diagnostics: the call
+		 ought to succeed without error, or a previous call
+		 must have already issued an error.  */
+	      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.  */
 
@@ -22031,6 +22081,8 @@ instantiate_template (tree tmpl, tree orig_args, tsubst_flags_t complain)
       push_nested_class (ctx);
     }
 
+  mark_template_arguments_used (gen_tmpl, targ_ptr);
+
   tree pattern = DECL_TEMPLATE_RESULT (gen_tmpl);
 
   fndecl = NULL_TREE;
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>();
+}
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..90d988ce896
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/fn-ptr3b.C
@@ -0,0 +1,28 @@
+// PR c++/53164
+// PR c++/105848
+// A C++20 version of fn-ptr3a.C using class NTTPs.
+// { dg-do compile { target c++20 } }
+
+template<class T>
+void f(T) { T::fail; } // { dg-error "fail" }
+
+struct B_int { void (*P)(int); };
+struct B_char { void (*P)(char); };
+
+template<B_int B>
+struct A {
+  static void wrap();
+};
+
+template<B_char B>
+void wrap();
+
+template<int>
+void g() {
+  A<B_int{f}>::wrap(); // { dg-message "required from here" }
+  wrap<B_char{f}>(); // { dg-message "required from here" }
+}
+
+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 (*P)()>
+void wrap() {
+  P(); // OK, despite A::g not being accessible from here.
+}
+
+struct A {
+  static void f() {
+    wrap<A::g>();
+  }
+private:
+  static void g();
+};
  

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) <case CALL_EXPR>: 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) <case CALL_EXPR>: 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) <case CALL_EXPR>: 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.

> This is what I have so far, it's imperfect but perhaps good enough?
> bootstrapped and regtested on x86_64-pc-linux-gnu:
> 
> -- >8 --
> 
> Subject: [PATCH] c++: improve "NTTP argument considered unused" fix [PR53164,
>   PR105848]
> 
> 	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) <case CALL_EXPR>: 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                             | 88 +++++++++++++++++++-----
>   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, 137 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..83902b0be8d 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,63 @@ 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 (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, 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 (VAR_OR_FUNCTION_DECL_P (arg))
> +	    {
> +	      /* Pass tf_none to avoid duplicate diagnostics: the call
> +		 ought to succeed without error, or a previous call
> +		 must have already issued an error.  */
> +	      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.  */
>   
> @@ -22031,6 +22081,8 @@ instantiate_template (tree tmpl, tree orig_args, tsubst_flags_t complain)
>         push_nested_class (ctx);
>       }
>   
> +  mark_template_arguments_used (gen_tmpl, targ_ptr);
> +
>     tree pattern = DECL_TEMPLATE_RESULT (gen_tmpl);
>   
>     fndecl = NULL_TREE;
> 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>();
> +}
> 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..90d988ce896
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/template/fn-ptr3b.C
> @@ -0,0 +1,28 @@
> +// PR c++/53164
> +// PR c++/105848
> +// A C++20 version of fn-ptr3a.C using class NTTPs.
> +// { dg-do compile { target c++20 } }
> +
> +template<class T>
> +void f(T) { T::fail; } // { dg-error "fail" }
> +
> +struct B_int { void (*P)(int); };
> +struct B_char { void (*P)(char); };
> +
> +template<B_int B>
> +struct A {
> +  static void wrap();
> +};
> +
> +template<B_char B>
> +void wrap();
> +
> +template<int>
> +void g() {
> +  A<B_int{f}>::wrap(); // { dg-message "required from here" }
> +  wrap<B_char{f}>(); // { dg-message "required from here" }
> +}
> +
> +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 (*P)()>
> +void wrap() {
> +  P(); // OK, despite A::g not being accessible from here.
> +}
> +
> +struct A {
> +  static void f() {
> +    wrap<A::g>();
> +  }
> +private:
> +  static void g();
> +};
  

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) <case CALL_EXPR>: 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) <case CALL_EXPR>: 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) <case CALL_EXPR>: 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.

-- >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) <case CALL_EXPR>: 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<class T>
+void f(T) { T::fail; } // { dg-error "fail" }
+
+template<void (*P)(int)>
+struct A {
+  // P not called
+};
+
+template<void (&P)(char)>
+void wrap() {
+  // P not called
+}
+
+template<int>
+void g() {
+  A<f> a; // { dg-message "required from" }
+  wrap<f>(); // { 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<class T>
+void f(T) { T::fail; } // { dg-error "fail" }
+
+struct B_int { void (*P)(int); };
+struct B_char { void (&P)(char); };
+
+template<B_int B>
+struct A {
+  // B.P not called
+};
+
+template<B_char B>
+void wrap() {
+  // B.P not called
+}
+
+template<int>
+void g() {
+  A<B_int{f}> a; // { dg-message "required from" }
+  wrap<B_char{f}>(); // { 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 (*P)()>
+void wrap() {
+  P(); // OK, despite A::g not being accessible from here.
+}
+
+struct A {
+  static void f() {
+    wrap<A::g>();
+  }
+private:
+  static void g();
+};
  

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) <case CALL_EXPR>: 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) <case CALL_EXPR>: 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) <case CALL_EXPR>: 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) <case CALL_EXPR>: 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<class T>
> +void f(T) { T::fail; } // { dg-error "fail" }
> +
> +template<void (*P)(int)>
> +struct A {
> +  // P not called
> +};
> +
> +template<void (&P)(char)>
> +void wrap() {
> +  // P not called
> +}
> +
> +template<int>
> +void g() {
> +  A<f> a; // { dg-message "required from" }
> +  wrap<f>(); // { 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<class T>
> +void f(T) { T::fail; } // { dg-error "fail" }
> +
> +struct B_int { void (*P)(int); };
> +struct B_char { void (&P)(char); };
> +
> +template<B_int B>
> +struct A {
> +  // B.P not called
> +};
> +
> +template<B_char B>
> +void wrap() {
> +  // B.P not called
> +}
> +
> +template<int>
> +void g() {
> +  A<B_int{f}> a; // { dg-message "required from" }
> +  wrap<B_char{f}>(); // { 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 (*P)()>
> +void wrap() {
> +  P(); // OK, despite A::g not being accessible from here.
> +}
> +
> +struct A {
> +  static void f() {
> +    wrap<A::g>();
> +  }
> +private:
> +  static void g();
> +};