[PATCH] Friend class cannot access protected field of parent

Hi,

So I spend my week end, trying to get into the clang code. I chosen to try to
fix Bug 6840 [1] which is one of the last issue preventing Qt to compile.

So the problem is with code like this:

class N { protected: static int m; };
class P : public N { friend class R; };
class R { int foo() { return N::n; } };

The C++ specification says that [class.access.base]
A member m is accessible at the point R when named in class N if [...]
m as a member of N is protected, and R occurs in a [...] friend of a class P
derived from N, where m as a member of P is public, private, or protected

So this code should compile, because in R::foo() we can access N::m though P's
friendship.

But it does not with clang currently.

The problem is that when checking the access of N::n within the R::foo()
context, we do not have information about P. We know which class are friend of
N (that's enough for private:), but we don't know which class R is friend of.

I called that the reverseFriend relationship, and I added this information
onto the CXXRecordDecl and the FunctionDecl.

You can find the patch attached.

It is not complete, as it does not work with template friends yet. (see FIXME
in the test). But it works with the common case.

How do you find it?

Regards

clang_6840.diff (17.2 KB)

Actually, I intentionally haven't implemented this rule yet. It is either a
drafting error or a horrible mistake. It neuters the entire 'protected'
specifier, it makes the well-formedness of code dependent on the
existence of completely unrelated classes, it imposes high costs on the
implementation, and it's formally undecidable in the presence of templates.
To wit:

  // Assume this has lots of specializations
  template <class> class Predicate;

  class A { protected: typedef int foo; };
  template <class T, bool = Predicate<T>::value> class B {};
  template <class T> class B<T, true> : public A { friend class C; };

  class C { A::foo x };

This reference is well-formed if and only if there exists a class T for which
Predicate<T>::value is true.

For a somewhat less abstract example, consider instead:
  template <class> class D;
  template <class T> class E : public D<T> { friend class C; };
Now 'C' is ill-formed unless there's a specialization of D which derives from A.

John.

> So I spend my week end, trying to get into the clang code. I chosen to
> try to fix Bug 6840 [1] which is one of the last issue preventing Qt to
> compile.
>
>
> So the problem is with code like this:
>
> class N { protected: static int m; };
> class P : public N { friend class R; };
> class R { int foo() { return N::n; } };
>
> The C++ specification says that [class.access.base]
> A member m is accessible at the point R when named in class N if [...]
> m as a member of N is protected, and R occurs in a [...] friend of a
> class P derived from N, where m as a member of P is public, private, or
> protected
>
> So this code should compile, because in R::foo() we can access N::m
> though P's friendship.

Actually, I intentionally haven't implemented this rule yet. It is either
a drafting error or a horrible mistake. It neuters the entire 'protected'
specifier, it makes the well-formedness of code dependent on the
existence of completely unrelated classes, it imposes high costs on the
implementation, and it's formally undecidable in the presence of templates.
To wit:

I must agree this is weird. But this is the specification, and there is code
that uses such constructs

  // Assume this has lots of specializations
  template <class> class Predicate;

  class A { protected: typedef int foo; };
  template <class T, bool = Predicate<T>::value> class B {};
  template <class T> class B<T, true> : public A { friend class C; };

  class C { A::foo x };

This reference is well-formed if and only if there exists a class T for
which Predicate<T>::value is true.

I just noticed that my implementation of the patch always say that this code
is valid.
GCC only accept the code if there is an instantiation of B<T,true>.
I guess I can modify the patch to require instantiation if needed.

For a somewhat less abstract example, consider instead:
  template <class> class D;
  template <class T> class E : public D<T> { friend class C; };
Now 'C' is ill-formed unless there's a specialization of D which derives
from A.

Both with gcc and my patch, this will be ill-formed unless there is an
instantiation of E which makes it derives from A

But I get your point.

Also would code like
template<class T, class U> class A : T { friend class U; };
mean that every protected is public.

GCC forces instantiations, and i think it is a sensible choice.

So you think it is better not to try to make it work, even for the non-
template case?

Yes. Like I said, I don't think this is an intended feature; I think it's a drafting
error. [class.access.base] contains many repetitions of the phrase "member
or friend"; I think someone just wrote "member or friend" here without really
thinking. I think when they were drafting the example in [class.protected]
they were only thinking about the effect of the protected access restriction, not
whether the member itself was accessible from the naming class. I'm willing
to consider evidence in the form of a DR that I'm wrong, but I don't really want
to implement the rule, and I'm not going to implement it as long as it's
undecidable.

Since I think this only affects people trying to compile Qt itself (rather than
people relying on Qt headers), I don't think it qualifies for a workaround.
It should be really easy to fix in the source, though.

Also, thank you for reminding me to add "introduces crazy dependencies on
instantiation order" to the list of reasons why this is a terrible mistake. :slight_smile:

John.

I'm willing to consider evidence in the form of a DR that I'm wrong, but
I don't really want to implement the rule, and I'm not going to

implement

it as long as it's undecidable.

A sensible decision, IMO. Anyway, the only DR about this bullet point is
#747, and it's been open since Nov 2008, with apparently no work done on it
at all. Maybe there's still time to contact some representative of a
national body to get a comment in?

I agree that this rule doesn't make sense. If R is a friend of P, then it
should name the member through P if it wants access, not through N.

Sebastian

http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#747

> So you think it is better not to try to make it work, even for the non-
> template case?

Yes. Like I said, I don't think this is an intended feature; I think it's
a drafting error. [class.access.base] contains many repetitions of the
phrase "member or friend"; I think someone just wrote "member or friend"
here without really thinking. I think when they were drafting the example
in [class.protected] they were only thinking about the effect of the
protected access restriction, not whether the member itself was accessible
from the naming class. I'm willing to consider evidence in the form of a
DR that I'm wrong, but I don't really want to implement the rule, and I'm
not going to implement it as long as it's undecidable.

Alright.
Still, according to the examples in [class.protected], it does not seem to be
a simple mistake.

Especially the
p2->B::i = 4; // OK (access through a D2, even though naming class is B)
B::j = 5; //OK (because refers to static member)
in the fr function.

Those examples do not compile with pristine clang, but does compile with the
patch.

Since I think this only affects people trying to compile Qt itself (rather
than people relying on Qt headers), I don't think it qualifies for a
workaround. It should be really easy to fix in the source, though.

Not trivial, but not that difficult, true.

Also, thank you for reminding me to add "introduces crazy dependencies on
instantiation order" to the list of reasons why this is a terrible mistake.
:slight_smile:

Thanks for your comments.

Right, I acknowledge that.

For what it's worth, your patch looks pretty good, except (1) I don't see how you're
dealing with multiple declarations of the target function/record and (2) you're
pushing friend decls in template definitions, which seems to be inconsistent
with what you say g++ does.

John.

>>> So you think it is better not to try to make it work, even for the non-
>>> template case?
>>
>> Yes. Like I said, I don't think this is an intended feature; I think
>> it's a drafting error. [class.access.base] contains many repetitions
>> of the phrase "member or friend"; I think someone just wrote "member
>> or friend" here without really thinking. I think when they were
>> drafting the example in [class.protected] they were only thinking about
>> the effect of the protected access restriction, not whether the member
>> itself was accessible from the naming class. I'm willing to consider
>> evidence in the form of a DR that I'm wrong, but I don't really want to
>> implement the rule, and I'm not going to implement it as long as it's
>> undecidable.
>
> Alright.
> Still, according to the examples in [class.protected], it does not seem
> to be a simple mistake.

Right, I acknowledge that.

For what it's worth, your patch looks pretty good, except (1) I don't see
how you're dealing with multiple declarations of the target function/record

I don't know what you mean here, can you elaborate?

and (2) you're pushing friend decls in template
definitions, which seems to be inconsistent with what you say g++ does.

Indeed. That need to be changed.

There should be one linked list of "reverse friends" per function or record, right?
Except every separate declaration of a record or function becomes a separate Decl
node, and each Decl node has its own list root. Your code inserts a FriendDecl into
the reverse-friends list of whatever declaration is associated with the friend declaration.
For friend classes, this will be whatever decl was active in scope at the time (or
the decl introduced by the elaborated-type-specifier if none was around). For friend
functions, it'll always be the "target declaration" of the friend declaration itself.
The access-control code works with the canonical declaration, so your code works
only as long as that's the one you push the friend into the reverse-friends chain of.

Try
  void foo(); // canonical decl
  ...
    friend void foo(); // separate decl
  ...
  (access here)
or
  class A; // canonical decl
  class A; // decl found by friend lookup
  ...
    friend class A;
  ...
  (access here)
or
    friend class A;
  ...
  class A {}; // this retroactively becomes the canonical decl
  ...
  (access here)

Anyway, if you're dealing with this issue, I didn't see it.

John.

>>>>> So you think it is better not to try to make it work, even for the
>>>>> non- template case?
>>>>
>>>> Yes. Like I said, I don't think this is an intended feature; I think
>>>> it's a drafting error. [class.access.base] contains many repetitions
>>>> of the phrase "member or friend"; I think someone just wrote "member
>>>> or friend" here without really thinking. I think when they were
>>>> drafting the example in [class.protected] they were only thinking
>>>> about the effect of the protected access restriction, not whether the
>>>> member itself was accessible from the naming class. I'm willing to
>>>> consider evidence in the form of a DR that I'm wrong, but I don't
>>>> really want to implement the rule, and I'm not going to implement it
>>>> as long as it's undecidable.
>>>
>>> Alright.
>>> Still, according to the examples in [class.protected], it does not seem
>>> to be a simple mistake.
>>
>> Right, I acknowledge that.

By that you mean you would accept the patch?

>>
>> For what it's worth, your patch looks pretty good, except (1) I don't
>> see how you're dealing with multiple declarations of the target
>> function/record
>
> I don't know what you mean here, can you elaborate?

There should be one linked list of "reverse friends" per function or
record, right? Except every separate declaration of a record or function
becomes a separate Decl node, and each Decl node has its own list root.
Your code inserts a FriendDecl into the reverse-friends list of whatever
declaration is associated with the friend declaration. For friend classes,
this will be whatever decl was active in scope at the time (or the decl
introduced by the elaborated-type-specifier if none was around). For
friend functions, it'll always be the "target declaration" of the friend
declaration itself. The access-control code works with the canonical
declaration, so your code works only as long as that's the one you push
the friend into the reverse-friends chain of.

I thought it is always the canonical one.
(line 479 and 586, I take the canonical form.)
Now, I'm new to clang code, so I might have forgot something.

This case does not work:

class A;
class B : public A { friend class C; friend void foo();};
class A { protected: typedef int foo; };
class C { A::foo x; };

It seems I do not support the fact it retroactively become the canonical decl.
I guess I can find the point where it becomes canonical, and move the linked
list to it. Do you have any pointer on where that happens?
I'll try to solve this next weekend.

(I'd also appreciate hints or suggestions for friend of template class (the
FIXME in the test))

So you think it is better not to try to make it work, even for the
non- template case?

Yes. Like I said, I don't think this is an intended feature; I think
it's a drafting error. [class.access.base] contains many repetitions
of the phrase "member or friend"; I think someone just wrote "member
or friend" here without really thinking. I think when they were
drafting the example in [class.protected] they were only thinking
about the effect of the protected access restriction, not whether the
member itself was accessible from the naming class. I'm willing to
consider evidence in the form of a DR that I'm wrong, but I don't
really want to implement the rule, and I'm not going to implement it
as long as it's undecidable.

Alright.
Still, according to the examples in [class.protected], it does not seem
to be a simple mistake.

Right, I acknowledge that.

By that you mean you would accept the patch?

No, sorry, not without some more support. It's a hard question — at what point does
an apparent drafting mistake in the standard become obligatory? — but I am
comfortable with my current answer here.

I was offering some feedback since you said you'd done this as a way of getting into
clang development.

Incidentally, even if we were going to support this, we would balk at adding an extra
pointer of storage to every FunctionDecl (and to a lesser degree, to every
CXXRecordDecl). A much more acceptable storage solution would be a side-table
stored on the ASTContext.

For what it's worth, your patch looks pretty good, except (1) I don't
see how you're dealing with multiple declarations of the target
function/record

I don't know what you mean here, can you elaborate?

There should be one linked list of "reverse friends" per function or
record, right? Except every separate declaration of a record or function
becomes a separate Decl node, and each Decl node has its own list root.
Your code inserts a FriendDecl into the reverse-friends list of whatever
declaration is associated with the friend declaration. For friend classes,
this will be whatever decl was active in scope at the time (or the decl
introduced by the elaborated-type-specifier if none was around). For
friend functions, it'll always be the "target declaration" of the friend
declaration itself. The access-control code works with the canonical
declaration, so your code works only as long as that's the one you push
the friend into the reverse-friends chain of.

I thought it is always the canonical one.
(line 479 and 586, I take the canonical form.)
Now, I'm new to clang code, so I might have forgot something.

Ah, no, I missed that, sorry.

We typically use assertions to document requirements like this.

This case does not work:

class A;
class B : public A { friend class C; friend void foo();};
class A { protected: typedef int foo; };
class C { A::foo x; };

It seems I do not support the fact it retroactively become the canonical decl.
I guess I can find the point where it becomes canonical, and move the linked
list to it. Do you have any pointer on where that happens?

During startDefinition.

(I'd also appreciate hints or suggestions for friend of template class (the
FIXME in the test))

You'd need to make a reverse-friend list for ClassTemplateDecls (and
FunctionTemplateDecls) and check whether the context class is associated
with a template.

Mostly, though, even if we were going to support this as a compatibility hack,
I see no reason to extend it to friend templates in the absence of the evidence
that anyone needs it.

John.

No, sorry, not without some more support. It's a hard question — at what
point does an apparent drafting mistake in the standard become obligatory?
— but I am comfortable with my current answer here.

ok

I was offering some feedback since you said you'd done this as a way of
getting into clang development.

Yes, thanks.

Incidentally, even if we were going to support this, we would balk at
adding an extra pointer of storage to every FunctionDecl (and to a lesser
degree, to every CXXRecordDecl). A much more acceptable storage solution
would be a side-table stored on the ASTContext.

>>>> For what it's worth, your patch looks pretty good, except (1) I don't
>>>> see how you're dealing with multiple declarations of the target
>>>> function/record
>>>
>>> I don't know what you mean here, can you elaborate?
>>
>> There should be one linked list of "reverse friends" per function or
>> record, right? Except every separate declaration of a record or function
>> becomes a separate Decl node, and each Decl node has its own list root.
>> Your code inserts a FriendDecl into the reverse-friends list of whatever
>> declaration is associated with the friend declaration. For friend
>> classes, this will be whatever decl was active in scope at the time (or
>> the decl introduced by the elaborated-type-specifier if none was
>> around). For friend functions, it'll always be the "target
>> declaration" of the friend declaration itself. The access-control code
>> works with the canonical declaration, so your code works only as long
>> as that's the one you push the friend into the reverse-friends chain
>> of.
>
> I thought it is always the canonical one.
> (line 479 and 586, I take the canonical form.)
> Now, I'm new to clang code, so I might have forgot something.

Ah, no, I missed that, sorry.

We typically use assertions to document requirements like this.

> This case does not work:
>
> class A;
> class B : public A { friend class C; friend void foo();};
> class A { protected: typedef int foo; };
> class C { A::foo x; };
>
> It seems I do not support the fact it retroactively become the canonical
> decl. I guess I can find the point where it becomes canonical, and move
> the linked list to it. Do you have any pointer on where that happens?

I typed this code too fast. It is wrong because B need A to be declared.
So I got the canonical form right after all.

During startDefinition.

> (I'd also appreciate hints or suggestions for friend of template class
> (the FIXME in the test))

You'd need to make a reverse-friend list for ClassTemplateDecls (and
FunctionTemplateDecls) and check whether the context class is associated
with a template.

Mostly, though, even if we were going to support this as a compatibility
hack, I see no reason to extend it to friend templates in the absence of
the evidence that anyone needs it.

Ok, thanks for your feedback.