[PATCH] c++: odr-use argument to a function NTTP [PR53164]

[Patrick Palka]

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.

> 
> -- >8 --
> 
> 	PR c++/53164
> 
> gcc/cp/ChangeLog:
> 
> 	* pt.c (tsubst_copy_and_build) <case CALL_EXPR>: Look through
> 	ADDR_EXPR after substituting into the callee.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/template/fn-ptr3.C: New test.
> ---
>  gcc/cp/pt.c                             |  4 ++++
>  gcc/testsuite/g++.dg/template/fn-ptr3.C | 20 ++++++++++++++++++++
>  2 files changed, 24 insertions(+)
>  create mode 100644 gcc/testsuite/g++.dg/template/fn-ptr3.C
> 
> diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
> index 19e03369ffa..5af3a6472f8 100644
> --- a/gcc/cp/pt.c
> +++ b/gcc/cp/pt.c
> @@ -20310,6 +20310,10 @@ tsubst_copy_and_build (tree t,
>  
>  	    if (BASELINK_P (function))
>  	      qualified_p = true;
> +
> +	    if (TREE_CODE (function) == ADDR_EXPR
> +		&& TREE_CODE (TREE_OPERAND (function, 0)) == FUNCTION_DECL)
> +	      function = TREE_OPERAND (function, 0);
>  	  }
>  
>  	nargs = call_expr_nargs (t);
> diff --git a/gcc/testsuite/g++.dg/template/fn-ptr3.C b/gcc/testsuite/g++.dg/template/fn-ptr3.C
> new file mode 100644
> index 00000000000..4aa193bd961
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/template/fn-ptr3.C
> @@ -0,0 +1,20 @@
> +// PR c++/53164
> +
> +template<class T>
> +void f(T) {
> +  T::fail; // { dg-error "not a member" }
> +}
> +
> +template<void p(int)>
> +struct A {
> +  static void h() {
> +    p(0);
> +  }
> +};
> +
> +template<int>
> +void g() {
> +  A<f>::h();
> +}
> +
> +template void g<0>();
> -- 
> 2.33.0.664.g0785eb7698
> 
>