Re: [PATCH] c++: access for hidden friend of nested class template [PR100502]

[Patrick Palka <ppalka@redhat.com>]

On Mon, 24 May 2021, Jason Merrill wrote:

> On 5/21/21 4:35 PM, Patrick Palka wrote:
> > Here, during ahead of time access checking for the private member
> > EnumeratorRange::end_reached_ in the hidden friend f, we're triggering
> > the the assert in enforce_access that verifies we're not trying to add a
> > dependent access check to TI_DEFERRED_ACCESS_CHECKS.
> > 
> > The special thing about this class member access expression is that it's
> > considered to be non-type-dependent (so finish_class_member_access_expr
> > doesn't exit early at template parse time), and then accessible_p
> > rejects the access (so we don't exit early from enforce access either,
> > and end up triggering the assert).  I think we're correct to reject it
> > because a hidden friend is not a member function, so [class.access.nest]
> > doesn't apply, and also a hidden friend of a nested class is not a
> > friend of the enclosing class.  (Note that Clang accepts the testcase
> > and MSVC and ICC reject it.)
> 
> Hmm, I think you're right, but that seems inconsistent with the change (long
> ago) to give nested classes access to members of the enclosing class.

I guess the question is whether a hidden friend is considered to be a
class member for sake of access checking.  Along that note, I noticed
Clang/GCC/MSVC/ICC all accept the access of A::f in:

  struct A {
  protected:
    static void f();
  };

  struct B : A {
    friend void g() { A::f(); }
  };

But arguably this is valid iff g is considered to be a member of B.

If we adjust the above example to define the friend g at namespace
scope:

  struct A {
  protected:
    static void f();
  };

  struct B : A {
    friend void g();
  };

  void g() { A::f(); }

then GCC/MSVC/ICC accept and Clang rejects.  But this second example is
definitely invalid since it's just a special case of the example in
[class.protected], which says:

  void fr() {
    ...
    B::j = 5;                     // error: not a friend of naming class B
    ...
  }

> 
> > This patch relaxes the problematic assert in enforce_access to check
> > dependent_scope_p instead of uses_template_parms, which is the more
> > accurate notion of dependence we care about.
> 
> Agreed.
> 
> > This change alone is
> > sufficient to fix the ICE, but we now end up diagnosing each access
> > twice, once at substitution time and again from TI_DEFERRED_ACCESS_CHECKS.
> > So this patch additionally disables ahead of time access checking
> > during the call to lookup_member from finish_class_member_access_expr;
> > we're going to check the same access again at substitution time anyway.
> 
> That seems undesirable; it's better to diagnose when parsing if we can. Why is
> it going on TI_DEFERRED_ACCESS_CHECKS after we already checked it?

At parse time, a negative accessible_p answer only means "maybe not
accessible" rather than "definitely not accessible", since access
may still be granted to some specialization of the current template
via a friend declaration.  I think we'd need to beef up accessible_p a
bit before we can begin diagnosing accesses at template parse time.
This probably wouldn't be too hairy to implement; I'll look into it.

For now, would the assert relaxation in enforce_access be OK for
trunk/11?

> 
> > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> > trunk?  For GCC 11, should we just backport the enforce_access hunk?
> > 
> > 	PR c++/100502
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	* semantics.c (enforce_access): Relax assert about the type
> > 	depedence of the DECL_CONTEXT of the declaration.
> > 	* typeck.c (finish_class_member_access_expr): Disable ahead
> > 	of time access checking during the member lookup.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	* g++.dg/template/access37.C: New test.
> > 	* g++.dg/template/access37a.C: New test.
> > ---
> >   gcc/cp/semantics.c                        |  2 +-
> >   gcc/cp/typeck.c                           |  6 ++++++
> >   gcc/testsuite/g++.dg/template/access37.C  | 26 +++++++++++++++++++++++
> >   gcc/testsuite/g++.dg/template/access37a.C |  6 ++++++
> >   4 files changed, 39 insertions(+), 1 deletion(-)
> >   create mode 100644 gcc/testsuite/g++.dg/template/access37.C
> >   create mode 100644 gcc/testsuite/g++.dg/template/access37a.C
> > 
> > diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
> > index 0d590c318fb..0de14316bba 100644
> > --- a/gcc/cp/semantics.c
> > +++ b/gcc/cp/semantics.c
> > @@ -365,7 +365,7 @@ enforce_access (tree basetype_path, tree decl, tree
> > diag_decl,
> >   	   check here.  */
> >   	gcc_assert (!uses_template_parms (decl));
> >   	if (TREE_CODE (decl) == FIELD_DECL)
> > -	  gcc_assert (!uses_template_parms (DECL_CONTEXT (decl)));
> > +	  gcc_assert (!dependent_scope_p (DECL_CONTEXT (decl)));
> >     	/* Defer this access check until instantiation time.  */
> >   	deferred_access_check access_check;
> > diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
> > index 703ddd3cc7a..86cf26b9c84 100644
> > --- a/gcc/cp/typeck.c
> > +++ b/gcc/cp/typeck.c
> > @@ -3201,9 +3201,15 @@ finish_class_member_access_expr (cp_expr object, tree
> > name, bool template_p,
> >   	{
> >   	  /* Look up the member.  */
> >   	  access_failure_info afi;
> > +	  if (processing_template_decl)
> > +	    /* We're going to redo this member lookup at instantiation
> > +	       time, so don't bother checking access ahead of time here.  */
> > +	    push_deferring_access_checks (dk_no_check);
> >   	  member = lookup_member (access_path, name, /*protect=*/1,
> >   				  /*want_type=*/false, complain,
> >   				  &afi);
> > +	  if (processing_template_decl)
> > +	    pop_deferring_access_checks ();
> >   	  afi.maybe_suggest_accessor (TYPE_READONLY (object_type));
> >   	  if (member == NULL_TREE)
> >   	    {
> > diff --git a/gcc/testsuite/g++.dg/template/access37.C
> > b/gcc/testsuite/g++.dg/template/access37.C
> > new file mode 100644
> > index 00000000000..92fed3e97cb
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/template/access37.C
> > @@ -0,0 +1,26 @@
> > +// PR c++/100502
> > +
> > +template <class>
> > +struct EnumeratorRange {
> > +  struct Iterator {
> > +    EnumeratorRange range_;
> > +
> > +    friend void f(Iterator i) {
> > +      i.range_.end_reached_; // { dg-error "private" }
> > +      i.range_.EnumeratorRange::end_reached_; // { dg-error "private" }
> > +      &i.range_.end_reached_; // { dg-error "private" }
> > +      &i.range_.EnumeratorRange::end_reached_; // { dg-error "private" }
> > +    }
> > +  };
> > +
> > + private:
> > +  bool end_reached_;
> > +#if DECLARE_FRIEND
> > +  friend void f(Iterator);
> > +#endif
> > +};
> > +
> > +int main() {
> > +  EnumeratorRange<int>::Iterator i;
> > +  f(i);
> > +}
> > diff --git a/gcc/testsuite/g++.dg/template/access37a.C
> > b/gcc/testsuite/g++.dg/template/access37a.C
> > new file mode 100644
> > index 00000000000..4ce1b2718a0
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/template/access37a.C
> > @@ -0,0 +1,6 @@
> > +// PR c++/100502
> > +// { dg-additional-options "-DDECLARE_FRIEND -Wno-non-template-friend" }
> > +
> > +// Verify that access37.C is accepted if the appropriate friend relation
> > +// is declared (controlled by the macro DECLARE_FRIEND).
> > +#include "access37.C"
> > 
> 
>