On Thu, 7 Jan 2021, Jason Merrill wrote:
> On 1/7/21 5:47 PM, Patrick Palka wrote:
> > On Thu, 7 Jan 2021, Jason Merrill wrote:
> >
> > > On 1/6/21 1:19 PM, Patrick Palka wrote:
> > > > In the first testcase below, we incorrectly reject the use of the
> > > > protected non-static member A::var0 from C<int>::g() because
> > > > check_accessibility_of_qualified_id, at template parse time, determines
> > > > that the access doesn't go through 'this'. (This happens because the
> > > > dependent base B<T> of C<T> doesn't have a binfo object, so it appears
> > > > to DERIVED_FROM_P that A is not an indirect base of C<T>.) From there
> > > > we create the corresponding deferred access check, which we then
> > > > perform at instantiation time and which (unsurprisingly) fails.
> > > >
> > > > The problem ultimately seems to be that we can't, in general, know
> > > > whether a use of a scoped non-static member goes through 'this' until
> > > > instantiation time, as the second testcase below demonstrates. So this
> > > > patch makes check_accessibility_of_qualified_id punt in this situation.
> > > >
> > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK to
> > > > commit?
> > > >
> > > > gcc/cp/ChangeLog:
> > > >
> > > > PR c++/98515
> > > > * semantics.c (check_accessibility_of_qualified_id): Punt if
> > > > we're checking the access of a scoped non-static member at
> > > > class template parse time.
> > > >
> > > > gcc/testsuite/ChangeLog:
> > > >
> > > > PR c++/98515
> > > > * g++.dg/template/access32.C: New test.
> > > > * g++.dg/template/access33.C: New test.
> > > > ---
> > > > gcc/cp/semantics.c | 20 +++++++++++++++-----
> > > > gcc/testsuite/g++.dg/template/access32.C | 8 ++++++++
> > > > gcc/testsuite/g++.dg/template/access33.C | 9 +++++++++
> > > > 3 files changed, 32 insertions(+), 5 deletions(-)
> > > > create mode 100644 gcc/testsuite/g++.dg/template/access32.C
> > > > create mode 100644 gcc/testsuite/g++.dg/template/access33.C
> > > >
> > > > diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
> > > > index b448efe024a..f52b2e4d1e7 100644
> > > > --- a/gcc/cp/semantics.c
> > > > +++ b/gcc/cp/semantics.c
> > > > @@ -2107,14 +2107,24 @@ check_accessibility_of_qualified_id (tree decl,
> > > > /* If the reference is to a non-static member of the
> > > > current class, treat it as if it were referenced through
> > > > `this'. */
> > > > - tree ct;
> > > > if (DECL_NONSTATIC_MEMBER_P (decl)
> > > > - && current_class_ptr
> > > > - && DERIVED_FROM_P (scope, ct = current_nonlambda_class_type
> > > > ()))
> > > > - qualifying_type = ct;
> > > > + && current_class_ptr)
> > > > + {
> > > > + if (dependent_type_p (TREE_TYPE (current_class_ptr)))
> > >
> > > This should also look at current_nonlambda_class_type.
> >
> > Ah, ack. But it seems to me we really only need to be checking
> > dependence of current_nonlambda_class_type here.
>
> Yes, that's what I meant, sorry about the ambiguous use of "also". :)
Oops, I see what you had meant now, sorry about the confusion :)
>
> > IIUC, dependence of
> > these two types should coincide except in the case where we're inside a
> > generic lambda within a non-template class (in which case
> > current_class_ptr will dependent and current_nonlambda_class_type won't).
> > But in this case we have enough information to be able to resolve the
> > access check at parse time, I think (and so we shouldn't punt).
> >
> > The below patch, which seems to pass 'make check-c++', checks the
> > dependence of current_nonlambda_class_type instead of that of
> > current_class_ptr. Does this approach seem right?
>
> OK.
Thanks.
>
> > -- >8 --
> >
> > Subject: [PATCH] c++: Fix access checking of scoped non-static member
> > [PR98515]
> >
> > In the first testcase below, we incorrectly reject the use of the
> > protected non-static member A::var0 from C<int>::g() because
> > check_accessibility_of_qualified_id, at template parse time, determines
> > that the access doesn't go through 'this'. (This happens because the
> > dependent base B<T> of C<T> doesn't have a binfo object, so it appears
> > to DERIVED_FROM_P that A is not an indirect base of C<T>.) From there
> > we create the corresponding deferred access check, which we then
> > perform at instantiation time and which (unsurprisingly) fails.
> >
> > The problem ultimately seems to be that we can't in general determine
> > whether a use of a scoped non-static member goes through 'this' until
> > instantiation time, as the second testcase below illustrates. So this
> > patch makes check_accessibility_of_qualified_id punt in such situations
> > to avoid creating a bogus deferred access check.
> >
> > gcc/cp/ChangeLog:
> >
> > PR c++/98515
> > * semantics.c (check_accessibility_of_qualified_id): Punt if
> > we're checking access of a scoped non-static member inside a
> > class template.
> >
> > gcc/testsuite/ChangeLog:
> >
> > PR c++/98515
> > * g++.dg/template/access32.C: New test.
> > * g++.dg/template/access33.C: New test.
> > ---
> > gcc/cp/semantics.c | 22 +++++++++++++++++-----
> > gcc/testsuite/g++.dg/template/access32.C | 8 ++++++++
> > gcc/testsuite/g++.dg/template/access33.C | 9 +++++++++
> > 3 files changed, 34 insertions(+), 5 deletions(-)
> > create mode 100644 gcc/testsuite/g++.dg/template/access32.C
> > create mode 100644 gcc/testsuite/g++.dg/template/access33.C
> >
> > diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
> > index b448efe024a..51f7c114b03 100644
> > --- a/gcc/cp/semantics.c
> > +++ b/gcc/cp/semantics.c
> > @@ -2107,14 +2107,26 @@ check_accessibility_of_qualified_id (tree decl,
> > /* If the reference is to a non-static member of the
> > current class, treat it as if it were referenced through
> > `this'. */
> > - tree ct;
> > if (DECL_NONSTATIC_MEMBER_P (decl)
> > - && current_class_ptr
> > - && DERIVED_FROM_P (scope, ct = current_nonlambda_class_type ()))
> > - qualifying_type = ct;
> > + && current_class_ptr)
> > + {
> > + if (tree current = current_nonlambda_class_type ())
> > + {
> > + if (dependent_type_p (current))
> > + /* In general we can't know whether this access goes through
> > + `this' until instantiation time. Punt now, or else we might
> > + create a deferred access check that's not relative to 'this'
> > + when it ought to be. We'll check this access again after
> > + substitution, e.g. from tsubst_qualified_id. */
> > + return true;
> > +
> > + if (DERIVED_FROM_P (scope, current))
> > + qualifying_type = current;
> > + }
> > + }
> > /* Otherwise, use the type indicated by the
> > nested-name-specifier. */
> > - else
> > + if (!qualifying_type)
> > qualifying_type = nested_name_specifier;
> > }
> > else
> > diff --git a/gcc/testsuite/g++.dg/template/access32.C
> > b/gcc/testsuite/g++.dg/template/access32.C
> > new file mode 100644
> > index 00000000000..08faa9f0f97
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/template/access32.C
> > @@ -0,0 +1,8 @@
> > +// PR c++/98515
> > +// { dg-do compile }
> > +
> > +struct A { protected: int var0; };
> > +template <class> struct B : public A { };
> > +template <class T> struct C : public B<T> { void g(); };
> > +template <class T> void C<T>::g() { A::var0++; }
> > +template class C<int>;
> > diff --git a/gcc/testsuite/g++.dg/template/access33.C
> > b/gcc/testsuite/g++.dg/template/access33.C
> > new file mode 100644
> > index 00000000000..9fb9b9a1236
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/template/access33.C
> > @@ -0,0 +1,9 @@
> > +// PR c++/98515
> > +// { dg-do compile }
> > +
> > +struct A { protected: int var0; };
> > +template <class> struct B : public A { };
> > +template <class T> struct C : public B<T> { void g(); };
> > +template <class T> void C<T>::g() { A::var0++; } // { dg-error
> > "protected|invalid" }
> > +template <> struct B<char> { };
> > +template class C<char>;
> >
>
>
