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"
