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"
> >
>
>
