[PATCH 2/2] c++: Extend PR96204 fix to variable templates

Patrick Palka ppalka@redhat.com
Wed Jun 30 20:42:10 GMT 2021


On Wed, 30 Jun 2021, Jason Merrill wrote:

> On 6/30/21 10:48 AM, Patrick Palka wrote:
> > On Tue, 29 Jun 2021, Jason Merrill wrote:
> > 
> > > On 6/29/21 1:57 PM, Patrick Palka wrote:
> > > > r12-1829 corrected the access scope during partial specialization
> > > > matching of class templates, but neglected the variable template case.
> > > > This patch moves the access scope adjustment to inside
> > > > most_specialized_partial_spec, so that all callers can benefit.
> > > > 
> > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> > > > trunk?
> > > > 
> > > > 	PR c++/96204
> > > > 
> > > > gcc/cp/ChangeLog:
> > > > 
> > > > 	* pt.c (instantiate_class_template_1): Remove call to
> > > > 	push_nested_class and pop_nested_class added by r12-1829.
> > > > 	(most_specialized_partial_spec): Use push_access_scope_guard
> > > > 	and deferring_access_check_sentinel.
> > > > 
> > > > gcc/testsuite/ChangeLog:
> > > > 
> > > > 	* g++.dg/template/access40b.C: New test.
> > > > ---
> > > >    gcc/cp/pt.c                               | 12 +++++++----
> > > >    gcc/testsuite/g++.dg/template/access40b.C | 26
> > > > +++++++++++++++++++++++
> > > >    2 files changed, 34 insertions(+), 4 deletions(-)
> > > >    create mode 100644 gcc/testsuite/g++.dg/template/access40b.C
> > > > 
> > > > diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
> > > > index bd8b17ca047..1e2e2ba5329 100644
> > > > --- a/gcc/cp/pt.c
> > > > +++ b/gcc/cp/pt.c
> > > > @@ -11776,11 +11776,8 @@ instantiate_class_template_1 (tree type)
> > > >      deferring_access_check_sentinel acs (dk_no_deferred);
> > > >        /* Determine what specialization of the original template to
> > > > -     instantiate; do this relative to the scope of the class for
> > > > -     sake of access checking.  */
> > > > -  push_nested_class (type);
> > > > +     instantiate.  */
> > > >      t = most_specialized_partial_spec (type, tf_warning_or_error);
> > > > -  pop_nested_class ();
> > > >      if (t == error_mark_node)
> > > >        return error_mark_node;
> > > >      else if (t)
> > > > @@ -24989,26 +24986,33 @@ most_specialized_partial_spec (tree target,
> > > > tsubst_flags_t complain)
> > > >      tree outer_args = NULL_TREE;
> > > >      tree tmpl, args;
> > > >    +  tree decl;
> > > >      if (TYPE_P (target))
> > > >        {
> > > >          tree tinfo = CLASSTYPE_TEMPLATE_INFO (target);
> > > >          tmpl = TI_TEMPLATE (tinfo);
> > > >          args = TI_ARGS (tinfo);
> > > > +      decl = TYPE_NAME (target);
> > > >        }
> > > >      else if (TREE_CODE (target) == TEMPLATE_ID_EXPR)
> > > >        {
> > > >          tmpl = TREE_OPERAND (target, 0);
> > > >          args = TREE_OPERAND (target, 1);
> > > > +      decl = DECL_TEMPLATE_RESULT (tmpl);
> > > 
> > > Hmm, this won't get us the right scope; we get here for the result of
> > > finish_template_variable, where tmpl is the most general template and args
> > > are
> > > args for it.  So in the below testcase, tmpl is outer<T>::N<T2,U>:
> > > 
> > > template <typename T> struct outer {
> > >    template <typename T2, typename U>
> > >    static constexpr int f() { return N<T,int>; };
> > > 
> > >    template <typename T2, typename U>
> > >    static const int N = f<T2, U>();
> > > };
> > > 
> > > template <typename T>
> > > template <typename U>
> > > const int outer<T>::N<T,U> = 1;
> > > 
> > > int i = outer<int>::N<double,int>;
> > > 
> > > Oddly, I notice that we also get here for static data members of class
> > > templates that are not themselves templates, as in mem-partial1.C that I
> > > adapted the above from.  Fixed by the attached patch.
> > 
> > Makes sense.  I was wondering if the VAR_P (pattern) test in
> > instantiate_template_1 should be adjusted as well, but that doesn't seem
> > to be strictly necessary since a VAR_DECL there will always be a
> > variable template specialization.
> > 
> > > 
> > > Since the type of the variable depends on the specialization, we can't
> > > actually get the decl before doing the resolution, but we should be able
> > > to
> > > push into the right enclosing class.  Perhaps we should pass the partially
> > > instantiated template and its args to lookup_template_variable instead of
> > > the
> > > most general template and its args.
> > > 
> > 
> > It seems what works is passing the partially instantiated template and
> > the full set of args to lookup_template_variable, because the outer
> > args of the partial specialization may be dependent as in e.g. the
> > above testcase.  One would hope that 'tmpl' contains the partially
> > instantiated template, but that's not the case because
> > finish_template_variable passes only the most general template
> > to us.  So we need to adjust finish_template_variable to pass the
> > partially instantiated template instead.
> > 
> > And instantiate_decl needs an adjustment as well, since it too calls
> > most_specialized_partial_spec.  Here, we could just pass the VAR_DECL
> > 'd' to most_specialized_partial_spec, which'll set up the right
> > context for us.
> > 
> > How does the following look?  Passes make check-c++, full testing in
> > progress.  The addded second testcase below should adequately test all
> > this IIUC..
> > 
> > -- >8 --
> > 
> > 	PR c++/96204
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	* pt.c (finish_template_variable): Pass the partially
> > 	instantiated template and args to instantiate_template.
> > 	(instantiate_class_template_1): No need to call
> > 	push_nested_class and pop_nested_class around the call to
> > 	most_specialized_partial_spec.
> > 	(instantiate_template_1): Pass the partially instantiated
> > 	template to lookup_template_variable.
> > 	(most_specialized_partial_spec):  Use push_access_scope_guard
> > 	to set the access scope appropriately.  Use
> > 	deferring_access_check_sentinel to disable deferment of access
> > 	checking.
> > 	(instantiate_decl): Just pass the VAR_DECL to
> > 	most_specialized_partial_spec.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	* g++.dg/template/access41.C: New test.
> > 	* g++.dg/template/access41a.C: New test.
> > ---
> >   gcc/cp/pt.c                               | 29 ++++++++++-----------
> >   gcc/testsuite/g++.dg/template/access41.C  | 24 ++++++++++++++++++
> >   gcc/testsuite/g++.dg/template/access41a.C | 31 +++++++++++++++++++++++
> >   3 files changed, 69 insertions(+), 15 deletions(-)
> >   create mode 100644 gcc/testsuite/g++.dg/template/access41.C
> >   create mode 100644 gcc/testsuite/g++.dg/template/access41a.C
> > 
> > diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
> > index d66ae134580..bed41402801 100644
> > --- a/gcc/cp/pt.c
> > +++ b/gcc/cp/pt.c
> > @@ -10266,14 +10266,10 @@ finish_template_variable (tree var, tsubst_flags_t
> > complain)
> >     tree templ = TREE_OPERAND (var, 0);
> >     tree arglist = TREE_OPERAND (var, 1);
> >   -  tree tmpl_args = DECL_TI_ARGS (DECL_TEMPLATE_RESULT (templ));
> > -  arglist = add_outermost_template_args (tmpl_args, arglist);
> > -
> > -  templ = most_general_template (templ);
> > -  tree parms = DECL_TEMPLATE_PARMS (templ);
> > -  arglist = coerce_innermost_template_parms (parms, arglist, templ,
> > complain,
> > -					     /*req_all*/true,
> > -					     /*use_default*/true);
> > +  tree parms = DECL_INNERMOST_TEMPLATE_PARMS (templ);
> > +  arglist = coerce_template_parms (parms, arglist, templ, complain,
> > +				   /*req_all*/true,
> > +				   /*use_default*/true);
> 
> Is the change from DECL_TEMPLATE_PARMS/coerce_innermost to
> DECL_INNERMOST_TEMPLATE_PARMS/coerce_template_parms necessary?  I'd expect
> them to have the same effect.
> 
> OK either way.

Ah yeah, the effect seems to be the same.  I thought I ran into a crash
before changing that part of finish_template_parm, but I can't reproduce
that anymore.  I'll change it back to using DECL_TEMPLATE_PARMS /
coerce_innermost_template_parms and commit the patch after another round
of testing.  Thanks a lot!

> 
> >     if (arglist == error_mark_node)
> >       return error_mark_node;
> >   @@ -11772,11 +11768,8 @@ instantiate_class_template_1 (tree type)
> >     deferring_access_check_sentinel acs (dk_no_deferred);
> >       /* Determine what specialization of the original template to
> > -     instantiate; do this relative to the scope of the class for
> > -     sake of access checking.  */
> > -  push_nested_class (type);
> > +     instantiate.  */
> >     t = most_specialized_partial_spec (type, tf_warning_or_error);
> > -  pop_nested_class ();
> >     if (t == error_mark_node)
> >       return error_mark_node;
> >     else if (t)
> > @@ -21132,7 +21125,7 @@ instantiate_template_1 (tree tmpl, tree orig_args,
> > tsubst_flags_t complain)
> >         /* We need to determine if we're using a partial or explicit
> >   	 specialization now, because the type of the variable could be
> >   	 different.  */
> > -      tree tid = lookup_template_variable (gen_tmpl, targ_ptr);
> > +      tree tid = lookup_template_variable (tmpl, targ_ptr);
> >         tree elt = most_specialized_partial_spec (tid, complain);
> >         if (elt == error_mark_node)
> >   	pattern = error_mark_node;
> > @@ -24985,26 +24978,33 @@ most_specialized_partial_spec (tree target,
> > tsubst_flags_t complain)
> >     tree outer_args = NULL_TREE;
> >     tree tmpl, args;
> >   +  tree decl;
> >     if (TYPE_P (target))
> >       {
> >         tree tinfo = CLASSTYPE_TEMPLATE_INFO (target);
> >         tmpl = TI_TEMPLATE (tinfo);
> >         args = TI_ARGS (tinfo);
> > +      decl = TYPE_NAME (target);
> >       }
> >     else if (TREE_CODE (target) == TEMPLATE_ID_EXPR)
> >       {
> >         tmpl = TREE_OPERAND (target, 0);
> >         args = TREE_OPERAND (target, 1);
> > +      decl = DECL_TEMPLATE_RESULT (tmpl);
> >       }
> >     else if (VAR_P (target))
> >       {
> >         tree tinfo = DECL_TEMPLATE_INFO (target);
> >         tmpl = TI_TEMPLATE (tinfo);
> >         args = TI_ARGS (tinfo);
> > +      decl = target;
> >       }
> >     else
> >       gcc_unreachable ();
> >   +  push_access_scope_guard pas (decl);
> > +  deferring_access_check_sentinel acs (dk_no_deferred);
> > +
> >     tree main_tmpl = most_general_template (tmpl);
> >       /* For determining which partial specialization to use, only the
> > @@ -26009,8 +26009,7 @@ instantiate_decl (tree d, bool defer_ok, bool
> > expl_inst_class_mem_p)
> >     if (variable_template_specialization_p (d))
> >       {
> >         /* Look up an explicit specialization, if any.  */
> > -      tree tid = lookup_template_variable (gen_tmpl, gen_args);
> > -      tree elt = most_specialized_partial_spec (tid, tf_warning_or_error);
> > +      tree elt = most_specialized_partial_spec (d, tf_warning_or_error);
> >         if (elt && elt != error_mark_node)
> >   	{
> >   	  td = TREE_VALUE (elt);
> > diff --git a/gcc/testsuite/g++.dg/template/access41.C
> > b/gcc/testsuite/g++.dg/template/access41.C
> > new file mode 100644
> > index 00000000000..1ab9a1ab243
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/template/access41.C
> > @@ -0,0 +1,24 @@
> > +// PR c++/96204
> > +// { dg-do compile { target c++14 } }
> > +// A variant of access40.C where has_type_member is a variable template
> > instead
> > +// of a class template.
> > +
> > +template<class, class = void>
> > +constexpr bool has_type_member = false;
> > +
> > +template<class T>
> > +constexpr bool has_type_member<T, typename T::type> = true;
> > +
> > +struct Parent;
> > +
> > +struct Child {
> > +private:
> > +  friend struct Parent;
> > +  typedef void type;
> > +};
> > +
> > +struct Parent {
> > +  // The partial specialization does not match despite Child::type
> > +  // being accessible from the current scope.
> > +  static_assert(!has_type_member<Child>, "");
> > +};
> > diff --git a/gcc/testsuite/g++.dg/template/access41a.C
> > b/gcc/testsuite/g++.dg/template/access41a.C
> > new file mode 100644
> > index 00000000000..e3608e2dffc
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/template/access41a.C
> > @@ -0,0 +1,31 @@
> > +// PR c++/96204
> > +// { dg-do compile { target c++14 } }
> > +// A variant of access40a.C where has_type_member is a variable template
> > instead
> > +// of a class template.
> > +
> > +template<class T>
> > +struct A {
> > +  template<class, class = void>
> > +  static constexpr bool has_type_member = false;
> > +};
> > +
> > +template<class T>
> > +template<class U>
> > +constexpr int A<T>::has_type_member<U, typename U::type> = true;
> > +
> > +struct Parent;
> > +
> > +struct Child {
> > +private:
> > +  friend struct A<int>;
> > +  typedef void type;
> > +};
> > +
> > +// The partial specialization matches because A<int> is a friend of Child.
> > +static_assert(A<int>::has_type_member<Child>, "");
> > +using type1 = const int;
> > +using type1 = decltype(A<int>::has_type_member<Child>);
> > +
> > +static_assert(!A<char>::has_type_member<Child>, "");
> > +using type2 = const bool;
> > +using type2 = decltype(A<char>::has_type_member<Child>);
> > 
> 
> 



More information about the Gcc-patches mailing list