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

[Jason Merrill <jason@redhat.com>]

On 6/7/22 09:24, Patrick Palka wrote:
> On Mon, 6 Jun 2022, Jason Merrill wrote:
> 
>> On 6/6/22 14:27, Patrick Palka wrote:
>>> On Thu, 7 Oct 2021, Jason Merrill wrote:
>>>
>>>> On 10/7/21 11:17, Patrick Palka wrote:
>>>>> On Wed, 6 Oct 2021, Jason Merrill wrote:
>>>>>
>>>>>> On 10/6/21 15:52, Patrick Palka wrote:
>>>>>>> On Wed, 6 Oct 2021, Patrick Palka wrote:
>>>>>>>
>>>>>>>> On Tue, 5 Oct 2021, Jason Merrill wrote:
>>>>>>>>
>>>>>>>>> On 10/5/21 15:17, Patrick Palka wrote:
>>>>>>>>>> On Mon, 4 Oct 2021, Patrick Palka wrote:
>>>>>>>>>>
>>>>>>>>>>> When passing a function template as the argument to a
>>>>>>>>>>> function
>>>>>>>>>>> NTTP
>>>>>>>>>>> inside a template, we resolve it to the right specialization
>>>>>>>>>>> ahead
>>>>>>>>>>> of
>>>>>>>>>>> time via resolve_address_of_overloaded_function, though the
>>>>>>>>>>> call
>>>>>>>>>>> to
>>>>>>>>>>> mark_used within defers odr-using it until instantiation
>>>>>>>>>>> time
>>>>>>>>>>> (as
>>>>>>>>>>> usual).
>>>>>>>>>>> But at instantiation time we end up never calling mark_used
>>>>>>>>>>> on
>>>>>>>>>>> the
>>>>>>>>>>> specialization.
>>>>>>>>>>>
>>>>>>>>>>> This patch fixes this by adding a call to mark_used in
>>>>>>>>>>> convert_nontype_argument_function.
>>>>>>>>>>>
>>>>>>>>>>> 	PR c++/53164
>>>>>>>>>>>
>>>>>>>>>>> gcc/cp/ChangeLog:
>>>>>>>>>>>
>>>>>>>>>>> 	* pt.c (convert_nontype_argument_function): Call
>>>>>>>>>>> mark_used.
>>>>>>>>>>>
>>>>>>>>>>> gcc/testsuite/ChangeLog:
>>>>>>>>>>>
>>>>>>>>>>> 	* g++.dg/template/non-dependent16.C: New test.
>>>>>>>>>>> ---
>>>>>>>>>>>       gcc/cp/pt.c                                     |  3
>>>>>>>>>>> +++
>>>>>>>>>>>       gcc/testsuite/g++.dg/template/non-dependent16.C | 16
>>>>>>>>>>> ++++++++++++++++
>>>>>>>>>>>       2 files changed, 19 insertions(+)
>>>>>>>>>>>       create mode 100644
>>>>>>>>>>> gcc/testsuite/g++.dg/template/non-dependent16.C
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
>>>>>>>>>>> index f950f4a21b7..5e819c9598c 100644
>>>>>>>>>>> --- a/gcc/cp/pt.c
>>>>>>>>>>> +++ b/gcc/cp/pt.c
>>>>>>>>>>> @@ -6668,6 +6668,9 @@ convert_nontype_argument_function
>>>>>>>>>>> (tree
>>>>>>>>>>> type,
>>>>>>>>>>> tree
>>>>>>>>>>> expr,
>>>>>>>>>>>             return NULL_TREE;
>>>>>>>>>>>           }
>>>>>>>>>>>       +  if (!mark_used (fn_no_ptr, complain) && !(complain &
>>>>>>>>>>> tf_error))
>>>>>>>>>>> +    return NULL_TREE;
>>>>>>>>>>> +
>>>>>>>>>>>         linkage = decl_linkage (fn_no_ptr);
>>>>>>>>>>>         if (cxx_dialect >= cxx11 ? linkage == lk_none :
>>>>>>>>>>> linkage !=
>>>>>>>>>>> lk_external)
>>>>>>>>>>>           {
>>>>>>>>>>> diff --git a/gcc/testsuite/g++.dg/template/non-dependent16.C
>>>>>>>>>>> b/gcc/testsuite/g++.dg/template/non-dependent16.C
>>>>>>>>>>> new file mode 100644
>>>>>>>>>>> index 00000000000..b7dca8f6752
>>>>>>>>>>> --- /dev/null
>>>>>>>>>>> +++ b/gcc/testsuite/g++.dg/template/non-dependent16.C
>>>>>>>>>>> @@ -0,0 +1,16 @@
>>>>>>>>>>> +// PR c++/53164
>>>>>>>>>>> +
>>>>>>>>>>> +template<class T>
>>>>>>>>>>> +void f(T) {
>>>>>>>>>>> +  T::fail; // { dg-error "not a member" }
>>>>>>>>>>> +}
>>>>>>>>>>> +
>>>>>>>>>>> +template<void(int)>
>>>>>>>>>>> +struct A { };
>>>>>>>>>>> +
>>>>>>>>>>> +template<int>
>>>>>>>>>>> +void g() {
>>>>>>>>>>> +  A<f> a;
>>>>>>>>>>> +}
>>>>>>>>>>
>>>>>>>>>> I should mention that the original testcase in the PR was
>>>>>>>>>> slightly
>>>>>>>>>> different than this one in that it also performed a call to
>>>>>>>>>> the
>>>>>>>>>> NTTP,
>>>>>>>>>> e.g.
>>>>>>>>>>
>>>>>>>>>>        template<void p(int)>
>>>>>>>>>>        struct A {
>>>>>>>>>>          static void h() {
>>>>>>>>>>            p(0);
>>>>>>>>>>          }
>>>>>>>>>>        };
>>>>>>>>>>
>>>>>>>>>>        template<int>
>>>>>>>>>>        void g() {
>>>>>>>>>>          A<f>::h();
>>>>>>>>>>        }
>>>>>>>>>>
>>>>>>>>>>        templated void g<0>();
>>>>>>>>>>
>>>>>>>>>> and not even the call was enough to odr-use f, apparently
>>>>>>>>>> because
>>>>>>>>>> the
>>>>>>>>>> CALL_EXPR case of tsubst_expr calls mark_used on the callee
>>>>>>>>>> only
>>>>>>>>>> when
>>>>>>>>>> it's a FUNCTION_DECL, but in this case after substitution it's
>>>>>>>>>> an
>>>>>>>>>> ADDR_EXPR of a FUNCTION_DECL.  Fixing this by looking through
>>>>>>>>>> the
>>>>>>>>>> ADDR_EXPR
>>>>>>>>>> worked, but IIUC the call isn't necessary for f to be
>>>>>>>>>> odr-used,
>>>>>>>>>> simply
>>>>>>>>>> using f as a template argument should be sufficient, so it
>>>>>>>>>> seems
>>>>>>>>>> the
>>>>>>>>>> above is better fix.
>>>>>>>>>
>>>>>>>>> I agree that pedantically the use happens when substituting into
>>>>>>>>> the
>>>>>>>>> use
>>>>>>>>> of
>>>>>>>>> A<f>, but convert_nontype_argument_function seems like a weird
>>>>>>>>> place
>>>>>>>>> to
>>>>>>>>> implement that; it's only called again during instantiation of
>>>>>>>>> A<f>,
>>>>>>>>> when we
>>>>>>>>> instantiate the injected-class-name.  If A<f> isn't
>>>>>>>>> instantiated,
>>>>>>>>> e.g.
>>>>>>>>> if 'a'
>>>>>>>>> is a pointer to A<f>, we again don't instantiate f<int>.
>>>>>>>>
>>>>>>>> I see, makes sense..  I'm not sure where else we can mark the use,
>>>>>>>> then.
>>>>>>>> Since we resolve the OVERLOAD f to the FUNCTION_DECL f<int> ahead
>>>>>>>> of
>>>>>>>> time (during which mark_used doesn't actually instantiate f<int>
>>>>>>>> because
>>>>>>>> we're inside a template), at instantiation time the type A<f> is
>>>>>>>> already
>>>>>>>> non-dependent so tsubst_aggr_type avoids doing the work that would
>>>>>>>> end
>>>>>>>> up calling convert_nontype_argument_function.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> I see that clang doesn't reject your testcase, either, but MSVC
>>>>>>>>> and
>>>>>>>>> icc
>>>>>>>>> do
>>>>>>>>> (even with 'a' a pointer): https://godbolt.org/z/MGE6TcMch
>>>>>>>>
>>>>>>>> FWIW although Clang doesn't reject 'A<f> a;', it does reject
>>>>>>>> 'using type = A<f>;' weirdly enough:
>>>>>>>> https://godbolt.org/z/T9qEn6bWW
>>>>>>>>
>>>>>>>>
>>>>>>>> Shall we just go with the other more specific approach, that makes
>>>>>>>> sure
>>>>>>>> the CALL_EXPR case of tsubst_expr calls mark_used when the callee
>>>>>>>> is
>>>>>>>> an
>>>>>>>> ADDR_EXPR?  Something like (bootstrapped and regtested):
>>>>>>>
>>>>>>> Err, this approach is wrong because by stripping the ADDR_EXPR here
>>>>>>> we
>>>>>>> end up checking access of the unwrapped FUNCTION_DECL again after
>>>>>>> substituting into the call.  So we incorrectly reject e.g.
>>>>>>>
>>>>>>>       template<void P()>
>>>>>>>       void g() {
>>>>>>>         P(); // error: ‘static void A::h()’ is private within this
>>>>>>> context
>>>>>>>       }
>>>>>>>
>>>>>>>       struct A {
>>>>>>>         void f() {
>>>>>>>           g<h>();
>>>>>>>         }
>>>>>>>       private:
>>>>>>>         static void h();
>>>>>>>       };
>>>>>>>
>>>>>>> since A::h isn't accessible from g.
>>>>>>
>>>>>> I guess you could call mark_used directly instead of stripping the
>>>>>> ADDR_EXPR.
>>>>>
>>>>> That seems to work nicely, how does the below look?  Bootstrapped and
>>>>> regtested on x86_64-pc-linux-gnu.
>>>>>
>>>>>>
>>>>>> Or for the general problem, perhaps we could mark the TEMPLATE_INFO or
>>>>>> TI_ARGS
>>>>>> to indicate that we still need to mark_used the arguments when we
>>>>>> encounter
>>>>>> A<f> again during instantiation?
>>>>>
>>>>> That sounds plausible, though I suppose it might not be worth it only to
>>>>> handle such a corner case..
>>>>
>>>> Indeed.  A lower-overhead possibility would be to remember, for a
>>>> template,
>>>> decls that we wanted to mark_used but didn't because we were in a
>>>> template.
>>>> But I wouldn't worry about it for now.
>>>>
>>>>> -- >8 --
>>>>>
>>>>> Subject: [PATCH] c++: function NTTP argument considered unused [PR53164]
>>>>>
>>>>> Here at parse time the template argument f (an OVERLOAD) in A<f> gets
>>>>> resolved ahead of time to the FUNCTION_DECL f<int>, and we defer marking
>>>>> f<int> as used until instantiation (of g) as usual.
>>>>>
>>>>> Later when instantiating g the type A<f> (where f has already been
>>>>> resolved)
>>>>> is non-dependent, so tsubst_aggr_type avoids re-processing its template
>>>>> arguments, and we end up never actually marking f<int> as used (which
>>>>> means
>>>>> we never instantiate it) even though A<f>::h() calls it.
>>>>>
>>>>> This patch works around this problem by making us look through ADDR_EXPR
>>>>> when calling mark_used on the callee of a substituted CALL_EXPR.
>>>>>
>>>>> 	PR c++/53164
>>>>>
>>>>> gcc/cp/ChangeLog:
>>>>>
>>>>> 	* pt.c (tsubst_copy_and_build) <case CALL_EXPR>: Look through an
>>>>> 	ADDR_EXPR callee when calling mark_used.
>>>>>
>>>>> gcc/testsuite/ChangeLog:
>>>>>
>>>>> 	* g++.dg/template/fn-ptr3.C: New test.
>>>>> ---
>>>>>     gcc/cp/pt.c                             | 12 ++++++++----
>>>>>     gcc/testsuite/g++.dg/template/fn-ptr3.C | 20 ++++++++++++++++++++
>>>>>     2 files changed, 28 insertions(+), 4 deletions(-)
>>>>>     create mode 100644 gcc/testsuite/g++.dg/template/fn-ptr3.C
>>>>>
>>>>> diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
>>>>> index 1e52aa757e1..cd10340ce12 100644
>>>>> --- a/gcc/cp/pt.c
>>>>> +++ b/gcc/cp/pt.c
>>>>> @@ -20508,10 +20508,14 @@ tsubst_copy_and_build (tree t,
>>>>>     	  }
>>>>>       	/* Remember that there was a reference to this entity.  */
>>>>> -	if (function != NULL_TREE
>>>>> -	    && DECL_P (function)
>>>>> -	    && !mark_used (function, complain) && !(complain & tf_error))
>>>>> -	  RETURN (error_mark_node);
>>>>> +	if (function)
>>>>> +	  {
>>>>> +	    tree sub = function;
>>>>> +	    if (TREE_CODE (sub) == ADDR_EXPR)
>>>>> +	      sub = TREE_OPERAND (sub, 0);
>>>>
>>>> Let's add a comment about why this is needed.  OK with that change.
>>>
>>> Thanks.  I ended up neglecting to commit this patch for GCC 12, and
>>> turns out it also fixes the recently reported regression PR105848
>>> (which is essentially another manifestation of PR53164 exposed by the
>>> non-dependent overload set pruning patch r12-6075), so I'm going to
>>> commit this now.  I suppose it's also OK to backport to 12?
>>
>> What if the use is not as a call, but say assigning the address to a global
>> pointer?
>>
>> I wonder about calling mark_used when substituting the TEMPLATE_PARM_INDEX,
>> probably with tf_none or tf_error to avoid redundant deprecated warnings.
> 
> Ah, that seems to work very nicely -- it even handles the case where there the
> only use is as a template argument and there is no "use" inside the body
> of the function, because we always have to substitute the TEMPLATE_PARM_INDEX
> in the generic template arguments.
> 
> Like so?  Bootstrapped and tested on x86_64-pc-linux-gnu.  I'm not sure
> if it's worth also checking !processing_template_decl && !DECL_ODR_USED
> before calling mark_used here, since otherwise the call ought to be
> certainly redundant.

Wouldn't hurt to benchmark that, but I wouldn't expect it to make a 
measurable difference.

> -- >8 --
> 
> Subject: [PATCH] c++: improve "NTTP argument considered unused" fix [PR53164]
> 
> 	PR c++/53164
> 	PR c++/105848
> 
> gcc/cp/ChangeLog:
> 
> 	* pt.cc (tsubst) <case TEMPLATE_PARM_INDEX>: Call mark_used on
> 	an ADDR_EXPR argument.
> 	(tsubst_copy_and_build) <case CALL_EXPR>: Revert r13-995 change.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/template/fn-ptr3a.C: New test.
> 
> Co-authored-by: Jason Merrill <jason@redhat.com>
> ---
>   gcc/cp/pt.cc                             | 40 +++++++++++++-----------
>   gcc/testsuite/g++.dg/template/fn-ptr3a.C | 25 +++++++++++++++
>   2 files changed, 46 insertions(+), 19 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/template/fn-ptr3a.C
> 
> diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> index dcacba56a1c..5863d83b4fd 100644
> --- a/gcc/cp/pt.cc
> +++ b/gcc/cp/pt.cc
> @@ -15858,9 +15858,23 @@ tsubst (tree t, tree args, tsubst_flags_t complain, tree in_decl)
>   	      }
>   	    else if (code == TEMPLATE_TEMPLATE_PARM)
>   	      return arg;
> -	    else
> -	      /* TEMPLATE_PARM_INDEX.  */
> -	      return convert_from_reference (unshare_expr (arg));
> +	    else /* if (code == TEMPLATE_PARM_INDEX)  */
> +	      {
> +		if (TREE_CODE (arg) == ADDR_EXPR
> +		    && DECL_P (TREE_OPERAND (arg, 0)))

If the parameter is a reference to function, rather than a pointer, do 
we need to look through a NOP_EXPR?

> +		  /* Remember that there was a reference to this entity.
> +		     We should already have called mark_used when taking
> +		     the address of this entity, but do so again to make
> +		     sure: at worst it's redundant, but if we so far only
> +		     called mark_used in a template context then we might
> +		     not have yet marked it odr-used (53164).  */
> +		  /* Mask out tf_warning_or_error to avoid issuing redundant
> +		     diagnostics -- we just care about making sure this
> +		     entity is odr-used.  */
> +		  mark_used (TREE_OPERAND (arg, 0),
> +			     complain & ~tf_warning_or_error);
> +		return convert_from_reference (unshare_expr (arg));
> +	      }
>   	  }
>   
>   	if (level == 1)
> @@ -20884,22 +20898,10 @@ tsubst_copy_and_build (tree t,
>   	  }
>   
>   	/* Remember that there was a reference to this entity.  */
> -	if (function != NULL_TREE)
> -	  {
> -	    tree inner = function;
> -	    if (TREE_CODE (inner) == ADDR_EXPR
> -		&& TREE_CODE (TREE_OPERAND (inner, 0)) == FUNCTION_DECL)
> -	      /* We should already have called mark_used when taking the
> -		 address of this function, but do so again anyway to make
> -		 sure it's odr-used: at worst this is a no-op, but if we
> -		 obtained this FUNCTION_DECL as part of ahead-of-time overload
> -		 resolution then that call to mark_used wouldn't have marked it
> -		 odr-used yet (53164).  */
> -	      inner = TREE_OPERAND (inner, 0);
> -	    if (DECL_P (inner)
> -		&& !mark_used (inner, complain) && !(complain & tf_error))
> -	      RETURN (error_mark_node);
> -	  }
> +	if (function != NULL_TREE
> +	    && DECL_P (function)
> +	    && !mark_used (function, complain) && !(complain & tf_error))
> +	  RETURN (error_mark_node);
>   
>   	if (!maybe_fold_fn_template_args (function, complain))
>   	  return error_mark_node;
> diff --git a/gcc/testsuite/g++.dg/template/fn-ptr3a.C b/gcc/testsuite/g++.dg/template/fn-ptr3a.C
> new file mode 100644
> index 00000000000..c76b5cc32b3
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/template/fn-ptr3a.C
> @@ -0,0 +1,25 @@
> +// PR c++/53164
> +// PR c++/105848
> +// A stricter version of fn-ptr3.C that verifies using f as a template
> +// argument alone constitutes an odr-use.
> +
> +template<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>();
> +}