From 1f2e1a96bec2ba6418ae7f2d2b525a3575203b6a Mon Sep 17 00:00:00 2001 From: John McCall Date: Fri, 10 Aug 2012 03:15:35 +0000 Subject: [PATCH] Check access to friend declarations. There's a number of different things going on here that were problematic: - We were missing the actual access check, or rather, it was suppressed on account of being a redeclaration lookup. - The access check would naturally happen during delay, which isn't appropriate in this case. - We weren't actually emitting dependent diagnostics associated with class templates, which was unfortunate. - Access was being propagated incorrectly for friend method declarations that couldn't be matched at parse-time. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@161652 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/Basic/DiagnosticSemaKinds.td | 3 + include/clang/Sema/Sema.h | 4 +- lib/Sema/SemaAccess.cpp | 57 ++++++--- lib/Sema/SemaDeclCXX.cpp | 6 +- lib/Sema/SemaTemplateInstantiate.cpp | 3 + lib/Sema/SemaTemplateInstantiateDecl.cpp | 48 ++++--- test/CXX/class.access/class.friend/p1.cpp | 2 + .../class.access/class.friend/p9-cxx0x.cpp | 117 ++++++++++++++++++ test/CXX/temp/temp.decls/temp.friend/p1.cpp | 3 + 9 files changed, 205 insertions(+), 38 deletions(-) create mode 100644 test/CXX/class.access/class.friend/p9-cxx0x.cpp diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index 357a63bce6..6d7b52e55f 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -909,6 +909,9 @@ def err_access_field_ctor : Error< "field of type %0 has %select{private|protected}2 " "%select{default |copy |move |*ERROR* |*ERROR* |*ERROR* |}1constructor">, AccessControl; +def err_access_friend_function : Error< + "friend function %1 is a %select{private|protected}0 member of %3">, + AccessControl; def err_access_dtor : Error< "calling a %select{private|protected}1 destructor of class %0">, diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h index 07dc4b4aeb..058e0d9d39 100644 --- a/include/clang/Sema/Sema.h +++ b/include/clang/Sema/Sema.h @@ -4427,9 +4427,7 @@ public: CXXDestructorDecl *Dtor, const PartialDiagnostic &PDiag, QualType objectType = QualType()); - AccessResult CheckDirectMemberAccess(SourceLocation Loc, - NamedDecl *D, - const PartialDiagnostic &PDiag); + AccessResult CheckFriendAccess(NamedDecl *D); AccessResult CheckMemberOperatorAccess(SourceLocation Loc, Expr *ObjectExpr, Expr *ArgExpr, diff --git a/lib/Sema/SemaAccess.cpp b/lib/Sema/SemaAccess.cpp index f71a38816f..3481171c77 100644 --- a/lib/Sema/SemaAccess.cpp +++ b/lib/Sema/SemaAccess.cpp @@ -1632,25 +1632,6 @@ Sema::AccessResult Sema::CheckConstructorAccess(SourceLocation UseLoc, return CheckAccess(*this, UseLoc, AccessEntity); } -/// Checks direct (i.e. non-inherited) access to an arbitrary class -/// member. -Sema::AccessResult Sema::CheckDirectMemberAccess(SourceLocation UseLoc, - NamedDecl *Target, - const PartialDiagnostic &Diag) { - AccessSpecifier Access = Target->getAccess(); - if (!getLangOpts().AccessControl || - Access == AS_public) - return AR_accessible; - - CXXRecordDecl *NamingClass = cast(Target->getDeclContext()); - AccessTarget Entity(Context, AccessTarget::Member, NamingClass, - DeclAccessPair::make(Target, Access), - QualType()); - Entity.setDiag(Diag); - return CheckAccess(*this, UseLoc, Entity); -} - - /// Checks access to an overloaded operator new or delete. Sema::AccessResult Sema::CheckAllocationAccess(SourceLocation OpLoc, SourceRange PlacementRange, @@ -1693,6 +1674,44 @@ Sema::AccessResult Sema::CheckMemberOperatorAccess(SourceLocation OpLoc, return CheckAccess(*this, OpLoc, Entity); } +/// Checks access to the target of a friend declaration. +Sema::AccessResult Sema::CheckFriendAccess(NamedDecl *target) { + assert(isa(target) || + (isa(target) && + isa(cast(target) + ->getTemplatedDecl()))); + + // Friendship lookup is a redeclaration lookup, so there's never an + // inheritance path modifying access. + AccessSpecifier access = target->getAccess(); + + if (!getLangOpts().AccessControl || access == AS_public) + return AR_accessible; + + CXXMethodDecl *method = dyn_cast(target); + if (!method) + method = cast( + cast(target)->getTemplatedDecl()); + assert(method->getQualifier()); + + AccessTarget entity(Context, AccessTarget::Member, + cast(target->getDeclContext()), + DeclAccessPair::make(target, access), + /*no instance context*/ QualType()); + entity.setDiag(diag::err_access_friend_function) + << method->getQualifierLoc().getSourceRange(); + + // We need to bypass delayed-diagnostics because we might be called + // while the ParsingDeclarator is active. + EffectiveContext EC(CurContext); + switch (CheckEffectiveAccess(*this, EC, target->getLocation(), entity)) { + case AR_accessible: return Sema::AR_accessible; + case AR_inaccessible: return Sema::AR_inaccessible; + case AR_dependent: return Sema::AR_dependent; + } + llvm_unreachable("falling off end"); +} + Sema::AccessResult Sema::CheckAddressOfMemberAccess(Expr *OvlExpr, DeclAccessPair Found) { if (!getLangOpts().AccessControl || diff --git a/lib/Sema/SemaDeclCXX.cpp b/lib/Sema/SemaDeclCXX.cpp index c259c8125a..539ed0eac5 100644 --- a/lib/Sema/SemaDeclCXX.cpp +++ b/lib/Sema/SemaDeclCXX.cpp @@ -10342,9 +10342,11 @@ Decl *Sema::ActOnFriendFunctionDecl(Scope *S, Declarator &D, FrD->setAccess(AS_public); CurContext->addDecl(FrD); - if (ND->isInvalidDecl()) + if (ND->isInvalidDecl()) { FrD->setInvalidDecl(); - else { + } else { + if (DC->isRecord()) CheckFriendAccess(ND); + FunctionDecl *FD; if (FunctionTemplateDecl *FTD = dyn_cast(ND)) FD = FTD->getTemplatedDecl(); diff --git a/lib/Sema/SemaTemplateInstantiate.cpp b/lib/Sema/SemaTemplateInstantiate.cpp index 102ec993ef..c7cbc41b30 100644 --- a/lib/Sema/SemaTemplateInstantiate.cpp +++ b/lib/Sema/SemaTemplateInstantiate.cpp @@ -2005,6 +2005,9 @@ Sema::InstantiateClass(SourceLocation PointOfInstantiation, } if (!Instantiation->isInvalidDecl()) { + // Perform any dependent diagnostics from the pattern. + PerformDependentDiagnostics(Pattern, TemplateArgs); + // Instantiate any out-of-line class template partial // specializations now. for (TemplateDeclInstantiator::delayed_partial_spec_iterator diff --git a/lib/Sema/SemaTemplateInstantiateDecl.cpp b/lib/Sema/SemaTemplateInstantiateDecl.cpp index 15ddd7df98..bdbe71dd8a 100644 --- a/lib/Sema/SemaTemplateInstantiateDecl.cpp +++ b/lib/Sema/SemaTemplateInstantiateDecl.cpp @@ -935,8 +935,6 @@ TemplateDeclInstantiator::VisitFunctionTemplateDecl(FunctionTemplateDecl *D) { if (!Instantiated) return 0; - Instantiated->setAccess(D->getAccess()); - // Link the instantiated function template declaration to the function // template from which it was instantiated. FunctionTemplateDecl *InstTemplate @@ -954,8 +952,12 @@ TemplateDeclInstantiator::VisitFunctionTemplateDecl(FunctionTemplateDecl *D) { InstTemplate->setInstantiatedFromMemberTemplate(D); // Make declarations visible in the appropriate context. - if (!isFriend) + if (!isFriend) { Owner->addDecl(InstTemplate); + } else if (InstTemplate->getDeclContext()->isRecord() && + !D->getPreviousDecl()) { + SemaRef.CheckFriendAccess(InstTemplate); + } return InstTemplate; } @@ -1526,7 +1528,15 @@ TemplateDeclInstantiator::VisitCXXMethodDecl(CXXMethodDecl *D, if (D->isPure()) SemaRef.CheckPureMethod(Method, SourceRange()); - Method->setAccess(D->getAccess()); + // Propagate access. For a non-friend declaration, the access is + // whatever we're propagating from. For a friend, it should be the + // previous declaration we just found. + if (isFriend && Method->getPreviousDecl()) + Method->setAccess(Method->getPreviousDecl()->getAccess()); + else + Method->setAccess(D->getAccess()); + if (FunctionTemplate) + FunctionTemplate->setAccess(Method->getAccess()); SemaRef.CheckOverrideControl(Method); @@ -1536,18 +1546,28 @@ TemplateDeclInstantiator::VisitCXXMethodDecl(CXXMethodDecl *D, if (D->isDeletedAsWritten()) Method->setDeletedAsWritten(); + // If there's a function template, let our caller handle it. if (FunctionTemplate) { - // If there's a function template, let our caller handle it. + // do nothing + + // Don't hide a (potentially) valid declaration with an invalid one. } else if (Method->isInvalidDecl() && !Previous.empty()) { - // Don't hide a (potentially) valid declaration with an invalid one. - } else { - NamedDecl *DeclToAdd = (TemplateParams - ? cast(FunctionTemplate) - : Method); - if (isFriend) - Record->makeDeclVisibleInContext(DeclToAdd); - else if (!IsClassScopeSpecialization) - Owner->addDecl(DeclToAdd); + // do nothing + + // Otherwise, check access to friends and make them visible. + } else if (isFriend) { + // We only need to re-check access for methods which we didn't + // manage to match during parsing. + if (!D->getPreviousDecl()) + SemaRef.CheckFriendAccess(Method); + + Record->makeDeclVisibleInContext(Method); + + // Otherwise, add the declaration. We don't need to do this for + // class-scope specializations because we'll have matched them with + // the appropriate template. + } else if (!IsClassScopeSpecialization) { + Owner->addDecl(Method); } if (D->isExplicitlyDefaulted()) { diff --git a/test/CXX/class.access/class.friend/p1.cpp b/test/CXX/class.access/class.friend/p1.cpp index 68ff83fee8..7cb192b5a1 100644 --- a/test/CXX/class.access/class.friend/p1.cpp +++ b/test/CXX/class.access/class.friend/p1.cpp @@ -64,6 +64,7 @@ namespace test0 { }; class MemberFriend { + public: void test(); }; @@ -309,6 +310,7 @@ namespace test10 { // PR8705 namespace test11 { class A { + public: void test0(int); void test1(int); void test2(int); diff --git a/test/CXX/class.access/class.friend/p9-cxx0x.cpp b/test/CXX/class.access/class.friend/p9-cxx0x.cpp new file mode 100644 index 0000000000..f748a2bb3b --- /dev/null +++ b/test/CXX/class.access/class.friend/p9-cxx0x.cpp @@ -0,0 +1,117 @@ +// RUN: %clang_cc1 -fsyntax-only -verify %s + +// C++98 [class.friend]p7: +// C++11 [class.friend]p9: +// A name nominated by a friend declaration shall be accessible in +// the scope of the class containing the friend declaration. + +// PR12328 +// Simple, non-templated case. +namespace test0 { + class X { + void f(); // expected-note {{implicitly declared private here}} + }; + + class Y { + friend void X::f(); // expected-error {{friend function 'f' is a private member of 'test0::X'}} + }; +} + +// Templated but non-dependent. +namespace test1 { + class X { + void f(); // expected-note {{implicitly declared private here}} + }; + + template class Y { + friend void X::f(); // expected-error {{friend function 'f' is a private member of 'test1::X'}} + }; +} + +// Dependent but instantiated at the right type. +namespace test2 { + template class Y; + + class X { + void f(); + friend class Y; + }; + + template class Y { + friend void X::f(); + }; + + template class Y; +} + +// Dependent and instantiated at the wrong type. +namespace test3 { + template class Y; + + class X { + void f(); // expected-note {{implicitly declared private here}} + friend class Y; + }; + + template class Y { + friend void X::f(); // expected-error {{friend function 'f' is a private member of 'test3::X'}} + }; + + template class Y; // expected-note {{in instantiation}} +} + +// Dependent because dependently-scoped. +namespace test4 { + template class X { + void f(); + }; + + template class Y { + friend void X::f(); + }; +} + +// Dependently-scoped, no friends. +namespace test5 { + template class X { + void f(); // expected-note {{implicitly declared private here}} + }; + + template class Y { + friend void X::f(); // expected-error {{friend function 'f' is a private member of 'test5::X'}} + }; + + template class Y; // expected-note {{in instantiation}} +} + +// Dependently-scoped, wrong friend. +namespace test6 { + template class Y; + + template class X { + void f(); // expected-note {{implicitly declared private here}} + friend class Y; + }; + + template class Y { + friend void X::f(); // expected-error {{friend function 'f' is a private member of 'test6::X'}} + }; + + template class Y; // expected-note {{in instantiation}} +} + +// Dependently-scoped, right friend. +namespace test7 { + template class Y; + + template class X { + void f(); + friend class Y; + }; + + template class Y { + friend void X::f(); + }; + + template class Y; +} diff --git a/test/CXX/temp/temp.decls/temp.friend/p1.cpp b/test/CXX/temp/temp.decls/temp.friend/p1.cpp index 63f569be08..640d03d4c9 100644 --- a/test/CXX/temp/temp.decls/temp.friend/p1.cpp +++ b/test/CXX/temp/temp.decls/temp.friend/p1.cpp @@ -302,6 +302,7 @@ namespace test14 { }; template class B { + public: void foo() { return A::foo(); } // expected-error {{'foo' is a private member of 'test14::A'}} }; @@ -320,10 +321,12 @@ namespace test15 { }; template class B { + public: void foo() { return A::foo(); } // expected-error {{'foo' is a private member of 'test15::A'}} }; template <> class B { + public: void foo() { return A::foo(); } template void bar(U u) { (void) A::foo(); -- 2.40.0