* [PATCH] c++: access for hidden friend of nested class template [PR100502] @ 2021-05-21 20:35 Patrick Palka 2021-05-24 21:13 ` Jason Merrill 0 siblings, 1 reply; 6+ messages in thread From: Patrick Palka @ 2021-05-21 20:35 UTC (permalink / raw) To: gcc-patches 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.) 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. 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. 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" -- 2.32.0.rc0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] c++: access for hidden friend of nested class template [PR100502] 2021-05-21 20:35 [PATCH] c++: access for hidden friend of nested class template [PR100502] Patrick Palka @ 2021-05-24 21:13 ` Jason Merrill 2021-05-25 14:50 ` Patrick Palka 0 siblings, 1 reply; 6+ messages in thread From: Jason Merrill @ 2021-05-24 21:13 UTC (permalink / raw) To: Patrick Palka, gcc-patches 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. > 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? > 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" > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] c++: access for hidden friend of nested class template [PR100502] 2021-05-24 21:13 ` Jason Merrill @ 2021-05-25 14:50 ` Patrick Palka 2021-05-25 15:18 ` Jason Merrill 0 siblings, 1 reply; 6+ messages in thread From: Patrick Palka @ 2021-05-25 14:50 UTC (permalink / raw) To: Jason Merrill; +Cc: Patrick Palka, gcc-patches 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" > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] c++: access for hidden friend of nested class template [PR100502] 2021-05-25 14:50 ` Patrick Palka @ 2021-05-25 15:18 ` Jason Merrill 2021-05-26 14:31 ` Patrick Palka 0 siblings, 1 reply; 6+ messages in thread From: Jason Merrill @ 2021-05-25 15:18 UTC (permalink / raw) To: Patrick Palka; +Cc: gcc-patches On 5/25/21 10:50 AM, Patrick Palka wrote: > 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. Ah, I missed that you were saying twice at substitution time. You're right that in general we can't diagnose at parse time. > For now, would the assert relaxation in enforce_access be OK for > trunk/11? Yes. And the other hunk is OK for trunk. >>> 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" >>> >> >> > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] c++: access for hidden friend of nested class template [PR100502] 2021-05-25 15:18 ` Jason Merrill @ 2021-05-26 14:31 ` Patrick Palka 2021-05-26 15:29 ` Jason Merrill 0 siblings, 1 reply; 6+ messages in thread From: Patrick Palka @ 2021-05-26 14:31 UTC (permalink / raw) To: Jason Merrill; +Cc: Patrick Palka, gcc-patches On Tue, 25 May 2021, Jason Merrill wrote: > On 5/25/21 10:50 AM, Patrick Palka wrote: > > 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. > > Ah, I missed that you were saying twice at substitution time. You're right > that in general we can't diagnose at parse time. > > > For now, would the assert relaxation in enforce_access be OK for > > trunk/11? > > Yes. And the other hunk is OK for trunk. Thanks a lot. Sadly it appears the assert relaxation is only sufficient to suppress the ICE, and with it alone we still don't accept the access37a.C testcase below, so backporting just the assert change wouldn't fully fix the regression. The reason is that without the finish_class_member_access_expr hunk, we still push an access check for the dependent member EnumeratorRange<T>::end_reached_ onto TI_DEFERRED_ACCESS_CHECKS. And because perform_instantiation_time_access_checks substitutes only into the scope of the member and not also into the member itself, we effectively end up checking whether EnumeratorRange<T>::end_reached_ is accessible from EnumeratorRange<int>, which'll always fail when the member is private. Perhaps we could also substitute into the declaration here, but that seems wrong because it'd create a new declaration. So in light of this, it seems we _shouldn't_ relax the assert in enforce_access because it currently accurately mirrors the assumptions of perform_instantiation_time_access_checks. And it seems it's necessary to backport the finish_class_member_access_expr hunk. Sorry, I should have realized this sooner.. Would then the following be OK for trunk/11? -- >8 -- PR c++/100502 gcc/cp/ChangeLog: * 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/typeck.c | 6 ++++++ gcc/testsuite/g++.dg/template/access37.C | 26 +++++++++++++++++++++++ gcc/testsuite/g++.dg/template/access37a.C | 6 ++++++ 3 files changed, 38 insertions(+) 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/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" -- 2.32.0.rc0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] c++: access for hidden friend of nested class template [PR100502] 2021-05-26 14:31 ` Patrick Palka @ 2021-05-26 15:29 ` Jason Merrill 0 siblings, 0 replies; 6+ messages in thread From: Jason Merrill @ 2021-05-26 15:29 UTC (permalink / raw) To: Patrick Palka; +Cc: gcc-patches On 5/26/21 10:31 AM, Patrick Palka wrote: > On Tue, 25 May 2021, Jason Merrill wrote: > >> On 5/25/21 10:50 AM, Patrick Palka wrote: >>> 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. >> >> Ah, I missed that you were saying twice at substitution time. You're right >> that in general we can't diagnose at parse time. >> >>> For now, would the assert relaxation in enforce_access be OK for >>> trunk/11? >> >> Yes. And the other hunk is OK for trunk. > > Thanks a lot. Sadly it appears the assert relaxation is only sufficient > to suppress the ICE, and with it alone we still don't accept the > access37a.C testcase below, so backporting just the assert change > wouldn't fully fix the regression. > > The reason is that without the finish_class_member_access_expr hunk, we still > push an access check for the dependent member EnumeratorRange<T>::end_reached_ > onto TI_DEFERRED_ACCESS_CHECKS. And because > perform_instantiation_time_access_checks substitutes only into the scope > of the member and not also into the member itself, we effectively end up > checking whether EnumeratorRange<T>::end_reached_ is accessible from > EnumeratorRange<int>, which'll always fail when the member is private. > Perhaps we could also substitute into the declaration here, but that > seems wrong because it'd create a new declaration. > > So in light of this, it seems we _shouldn't_ relax the assert in > enforce_access because it currently accurately mirrors the assumptions > of perform_instantiation_time_access_checks. And it seems it's > necessary to backport the finish_class_member_access_expr hunk. > Sorry, I should have realized this sooner.. > > Would then the following be OK for trunk/11? > > -- >8 -- > > PR c++/100502 > > gcc/cp/ChangeLog: > > * 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/typeck.c | 6 ++++++ > gcc/testsuite/g++.dg/template/access37.C | 26 +++++++++++++++++++++++ > gcc/testsuite/g++.dg/template/access37a.C | 6 ++++++ > 3 files changed, 38 insertions(+) > 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/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. */ Please reword the comment now that we know this is needed for correctness. OK with that change. > + 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" > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-05-26 15:29 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-21 20:35 [PATCH] c++: access for hidden friend of nested class template [PR100502] Patrick Palka 2021-05-24 21:13 ` Jason Merrill 2021-05-25 14:50 ` Patrick Palka 2021-05-25 15:18 ` Jason Merrill 2021-05-26 14:31 ` Patrick Palka 2021-05-26 15:29 ` Jason Merrill
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).