diff --git a/clang/include/clang/AST/CXXInheritance.h b/clang/include/clang/AST/CXXInheritance.h index 77356f2f4d8b..1491b1edbbac 100644 --- a/clang/include/clang/AST/CXXInheritance.h +++ b/clang/include/clang/AST/CXXInheritance.h @@ -65,12 +65,21 @@ struct CXXBasePathElement { /// subobject is being used. class CXXBasePath : public llvm::SmallVector { public: - /// \brief The access along this inheritance path. + CXXBasePath() : Access(AS_public) {} + + /// \brief The access along this inheritance path. This is only + /// calculated when recording paths. AS_none is a special value + /// used to indicate a path which permits no legal access. AccessSpecifier Access; /// \brief The set of declarations found inside this base class /// subobject. DeclContext::lookup_result Decls; + + void clear() { + llvm::SmallVectorImpl::clear(); + Access = AS_public; + } }; /// BasePaths - Represents the set of paths from a derived class to @@ -138,10 +147,6 @@ class CXXBasePaths { /// to help build the set of paths. CXXBasePath ScratchPath; - /// ScratchAccess - A stack of accessibility annotations used by - /// Sema::lookupInBases. - llvm::SmallVector ScratchAccess; - /// DetectedVirtual - The base class that is virtual. const RecordType *DetectedVirtual; diff --git a/clang/include/clang/AST/DeclCXX.h b/clang/include/clang/AST/DeclCXX.h index 6588886ac900..73ebf52d383b 100644 --- a/clang/include/clang/AST/DeclCXX.h +++ b/clang/include/clang/AST/DeclCXX.h @@ -817,6 +817,15 @@ public: /// GraphViz. void viewInheritance(ASTContext& Context) const; + /// MergeAccess - Calculates the access of a decl that is reached + /// along a path. + static AccessSpecifier MergeAccess(AccessSpecifier PathAccess, + AccessSpecifier DeclAccess) { + assert(DeclAccess != AS_none); + if (DeclAccess == AS_private) return AS_none; + return (PathAccess > DeclAccess ? PathAccess : DeclAccess); + } + static bool classof(const Decl *D) { return D->getKind() == CXXRecord || D->getKind() == ClassTemplateSpecialization || diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 353cd0e3db6a..4cfe94ec37cf 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -412,6 +412,15 @@ def err_class_redeclared_with_different_access : Error< "%0 redeclared with '%1' access">; def note_previous_access_declaration : Note< "previously declared '%1' here">; +def err_access_outside_class : Error< + "access to %select{private|protected}0 member outside any class context">; +def note_access_natural : Note<"declared %select{private|protected}0 here">; +def note_access_constrained_by_path : Note< + "access to decl constrained by %select{private|protected}0 inheritance">; +def err_access_protected : Error< + "access to protected member of %0 from %1, which is not a subclass">; +def err_access_private : Error< + "access to private member of %0 from %1">; // C++ name lookup def err_incomplete_nested_name_spec : Error< diff --git a/clang/include/clang/Basic/Specifiers.h b/clang/include/clang/Basic/Specifiers.h index 88e439659372..4cace86dd18d 100644 --- a/clang/include/clang/Basic/Specifiers.h +++ b/clang/include/clang/Basic/Specifiers.h @@ -68,13 +68,14 @@ namespace clang { bool ModeAttr : 1; }; - /// AccessSpecifier - A C++ access specifier (none, public, private, - /// protected). + /// AccessSpecifier - A C++ access specifier (public, private, + /// protected), plus the special value "none" which means + /// different things in different contexts. enum AccessSpecifier { - AS_none, AS_public, AS_protected, - AS_private + AS_private, + AS_none }; } diff --git a/clang/lib/AST/CXXInheritance.cpp b/clang/lib/AST/CXXInheritance.cpp index d575ccd98265..720832869266 100644 --- a/clang/lib/AST/CXXInheritance.cpp +++ b/clang/lib/AST/CXXInheritance.cpp @@ -61,7 +61,6 @@ void CXXBasePaths::clear() { Paths.clear(); ClassSubobjects.clear(); ScratchPath.clear(); - ScratchAccess.clear(); DetectedVirtual = 0; } @@ -147,6 +146,10 @@ bool CXXRecordDecl::lookupInBases(BaseMatchesCallback *BaseMatches, CXXBasePaths &Paths) const { bool FoundPath = false; + // The access of the path down to this record. + AccessSpecifier AccessToHere = Paths.ScratchPath.Access; + bool IsFirstStep = Paths.ScratchPath.empty(); + ASTContext &Context = getASTContext(); for (base_class_const_iterator BaseSpec = bases_begin(), BaseSpecEnd = bases_end(); BaseSpec != BaseSpecEnd; ++BaseSpec) { @@ -191,20 +194,30 @@ bool CXXRecordDecl::lookupInBases(BaseMatchesCallback *BaseMatches, Element.SubobjectNumber = Subobjects.second; Paths.ScratchPath.push_back(Element); - // C++0x [class.access.base]p1 (paraphrased): - // The access of a member of a base class is the less permissive - // of its access within the base class and the access of the base - // class within the derived class. - // We're just calculating the access along the path, so we ignore - // the access specifiers of whatever decls we've found. - AccessSpecifier PathAccess = Paths.ScratchPath.Access; - Paths.ScratchAccess.push_back(PathAccess); - Paths.ScratchPath.Access - = std::max(PathAccess, BaseSpec->getAccessSpecifier()); + // Calculate the "top-down" access to this base class. + // The spec actually describes this bottom-up, but top-down is + // equivalent because the definition works out as follows: + // 1. Write down the access along each step in the inheritance + // chain, followed by the access of the decl itself. + // For example, in + // class A { public: int foo; }; + // class B : protected A {}; + // class C : public B {}; + // class D : private C {}; + // we would write: + // private public protected public + // 2. If 'private' appears anywhere except far-left, access is denied. + // 3. Otherwise, overall access is determined by the most restrictive + // access in the sequence. + if (IsFirstStep) + Paths.ScratchPath.Access = BaseSpec->getAccessSpecifier(); + else + Paths.ScratchPath.Access + = MergeAccess(AccessToHere, BaseSpec->getAccessSpecifier()); } if (BaseMatches(BaseSpec, Paths.ScratchPath, UserData)) { - // We've found a path that terminates that this base. + // We've found a path that terminates at this base. FoundPath = true; if (Paths.isRecordingPaths()) { // We have a path. Make a copy of it before moving on. @@ -237,8 +250,6 @@ bool CXXRecordDecl::lookupInBases(BaseMatchesCallback *BaseMatches, // collecting paths). if (Paths.isRecordingPaths()) { Paths.ScratchPath.pop_back(); - Paths.ScratchPath.Access = Paths.ScratchAccess.back(); - Paths.ScratchAccess.pop_back(); } // If we set a virtual earlier, and this isn't a path, forget it again. @@ -246,6 +257,9 @@ bool CXXRecordDecl::lookupInBases(BaseMatchesCallback *BaseMatches, Paths.DetectedVirtual = 0; } } + + // Reset the scratch path access. + Paths.ScratchPath.Access = AccessToHere; return FoundPath; } diff --git a/clang/lib/Parse/ParseDeclCXX.cpp b/clang/lib/Parse/ParseDeclCXX.cpp index a5ef817d43d0..efaf8ee3270e 100644 --- a/clang/lib/Parse/ParseDeclCXX.cpp +++ b/clang/lib/Parse/ParseDeclCXX.cpp @@ -983,7 +983,7 @@ Parser::BaseResult Parser::ParseBaseSpecifier(DeclPtrTy ClassDecl) { // Parse an (optional) access specifier. AccessSpecifier Access = getAccessSpecifierIfPresent(); - if (Access) + if (Access != AS_none) ConsumeToken(); // Parse the 'virtual' keyword (again!), in case it came after the diff --git a/clang/lib/Sema/Lookup.h b/clang/lib/Sema/Lookup.h index 274a3dabbf09..f4cea2e88a6d 100644 --- a/clang/lib/Sema/Lookup.h +++ b/clang/lib/Sema/Lookup.h @@ -131,6 +131,7 @@ public: Sema::RedeclarationKind Redecl = Sema::NotForRedeclaration) : ResultKind(NotFound), Paths(0), + NamingClass(0), SemaRef(SemaRef), Name(Name), NameLoc(NameLoc), @@ -150,6 +151,7 @@ public: LookupResult(TemporaryToken _, const LookupResult &Other) : ResultKind(NotFound), Paths(0), + NamingClass(0), SemaRef(Other.SemaRef), Name(Other.Name), NameLoc(Other.NameLoc), @@ -245,6 +247,37 @@ public: return IDNS; } + /// \brief Returns whether these results arose from performing a + /// lookup into a class. + bool isClassLookup() const { + return NamingClass != 0; + } + + /// \brief Returns the 'naming class' for this lookup, i.e. the + /// class which was looked into to find these results. + /// + /// C++0x [class.access.base]p5: + /// The access to a member is affected by the class in which the + /// member is named. This naming class is the class in which the + /// member name was looked up and found. [Note: this class can be + /// explicit, e.g., when a qualified-id is used, or implicit, + /// e.g., when a class member access operator (5.2.5) is used + /// (including cases where an implicit "this->" is added). If both + /// a class member access operator and a qualified-id are used to + /// name the member (as in p->T::m), the class naming the member + /// is the class named by the nested-name-specifier of the + /// qualified-id (that is, T). -- end note ] + /// + /// This is set by the lookup routines when they find results in a class. + CXXRecordDecl *getNamingClass() const { + return NamingClass; + } + + /// \brief Sets the 'naming class' for this lookup. + void setNamingClass(CXXRecordDecl *Record) { + NamingClass = Record; + } + /// \brief Add a declaration to these results with its natural access. /// Does not test the acceptance criteria. void addDecl(NamedDecl *D) { @@ -465,6 +498,8 @@ private: void diagnose() { if (isAmbiguous()) SemaRef.DiagnoseAmbiguousLookup(*this); + else if (isClassLookup() && SemaRef.getLangOptions().AccessControl) + SemaRef.CheckAccess(*this); } void setAmbiguous(AmbiguityKind AK) { @@ -504,6 +539,7 @@ private: AmbiguityKind Ambiguity; // ill-defined unless ambiguous UnresolvedSet<8> Decls; CXXBasePaths *Paths; + CXXRecordDecl *NamingClass; // Parameters. Sema &SemaRef; diff --git a/clang/lib/Sema/Sema.h b/clang/lib/Sema/Sema.h index 90a755aa0ccc..06e9e3ae9e64 100644 --- a/clang/lib/Sema/Sema.h +++ b/clang/lib/Sema/Sema.h @@ -2361,6 +2361,9 @@ public: CXXBasePaths &Paths, bool NoPrivileges = false); + void CheckAccess(const LookupResult &R); + bool CheckAccess(const LookupResult &R, NamedDecl *D, AccessSpecifier Access); + bool CheckBaseClassAccess(QualType Derived, QualType Base, unsigned InaccessibleBaseID, CXXBasePaths& Paths, SourceLocation AccessLoc, diff --git a/clang/lib/Sema/SemaAccess.cpp b/clang/lib/Sema/SemaAccess.cpp index b7cc37b6c9a7..51f9e300ef90 100644 --- a/clang/lib/Sema/SemaAccess.cpp +++ b/clang/lib/Sema/SemaAccess.cpp @@ -12,6 +12,7 @@ //===----------------------------------------------------------------------===// #include "Sema.h" +#include "Lookup.h" #include "clang/AST/ASTContext.h" #include "clang/AST/CXXInheritance.h" #include "clang/AST/DeclCXX.h" @@ -137,3 +138,101 @@ bool Sema::CheckBaseClassAccess(QualType Derived, QualType Base, return false; } + +/// Diagnose the path which caused the given declaration to become +/// inaccessible. +static void DiagnoseAccessPath(Sema &S, const LookupResult &R, NamedDecl *D, + AccessSpecifier Access) { + // Easy case: the decl's natural access determined its path access. + if (Access == D->getAccess() || D->getAccess() == AS_private) { + S.Diag(D->getLocation(), diag::note_access_natural) + << (unsigned) (Access == AS_protected); + return; + } + + // TODO: flesh this out + S.Diag(D->getLocation(), diag::note_access_constrained_by_path) + << (unsigned) (Access == AS_protected); +} + +/// Checks access to the given declaration in the current context. +/// +/// \param R the means via which the access was made; must have a naming +/// class set +/// \param D the declaration accessed +/// \param Access the best access along any inheritance path from the +/// naming class to the declaration. AS_none means the path is impossible +bool Sema::CheckAccess(const LookupResult &R, NamedDecl *D, + AccessSpecifier Access) { + assert(R.getNamingClass() && "performing access check without naming class"); + + // If the access path is public, it's accessible everywhere. + if (Access == AS_public) + return false; + + // Otherwise, derive the current class context. + DeclContext *DC = CurContext; + while (isa(DC) && + cast(DC)->isAnonymousStructOrUnion()) + DC = DC->getParent(); + + CXXRecordDecl *CurRecord; + if (isa(DC)) + CurRecord = cast(DC); + else if (isa(DC)) + CurRecord = cast(DC)->getParent(); + else { + Diag(R.getNameLoc(), diag::err_access_outside_class) + << (Access == AS_protected); + DiagnoseAccessPath(*this, R, D, Access); + return true; + } + + CXXRecordDecl *NamingClass = R.getNamingClass(); + while (NamingClass->isAnonymousStructOrUnion()) + // This should be guaranteed by the fact that the decl has + // non-public access. If not, we should make it guaranteed! + NamingClass = cast(NamingClass); + + // White-list accesses from within the declaring class. + if (Access != AS_none && + CurRecord->getCanonicalDecl() == NamingClass->getCanonicalDecl()) + return false; + + // Protected access. + if (Access == AS_protected) { + // FIXME: implement [class.protected]p1 + if (CurRecord->isDerivedFrom(NamingClass)) + return false; + + // FIXME: dependent classes + } + + // FIXME: friends + + // Okay, it's a bad access, reject it. + + + CXXRecordDecl *DeclaringClass = cast(D->getDeclContext()); + + if (Access == AS_protected) { + Diag(R.getNameLoc(), diag::err_access_protected) + << Context.getTypeDeclType(DeclaringClass) + << Context.getTypeDeclType(CurRecord); + DiagnoseAccessPath(*this, R, D, Access); + return true; + } + + assert(Access == AS_private || Access == AS_none); + Diag(R.getNameLoc(), diag::err_access_private) + << Context.getTypeDeclType(DeclaringClass) + << Context.getTypeDeclType(CurRecord); + DiagnoseAccessPath(*this, R, D, Access); + return true; +} + +/// Checks access to all the declarations in the given result set. +void Sema::CheckAccess(const LookupResult &R) { + for (LookupResult::iterator I = R.begin(), E = R.end(); I != E; ++I) + CheckAccess(R, *I, I.getAccess()); +} diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index 4066a646a5f7..fbe02894ace5 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -5732,8 +5732,10 @@ Sema::DeclPtrTy Sema::ActOnEnumConstant(Scope *S, DeclPtrTy theEnumDecl, IdLoc, Id, Owned(Val)); // Register this decl in the current scope stack. - if (New) + if (New) { + New->setAccess(TheEnumDecl->getAccess()); PushOnScopeChains(New, S); + } return DeclPtrTy::make(New); } diff --git a/clang/lib/Sema/SemaLookup.cpp b/clang/lib/Sema/SemaLookup.cpp index ddce4a4c23db..f5d2a7d89988 100644 --- a/clang/lib/Sema/SemaLookup.cpp +++ b/clang/lib/Sema/SemaLookup.cpp @@ -967,6 +967,8 @@ bool Sema::LookupQualifiedName(LookupResult &R, DeclContext *LookupCtx, // Perform qualified name lookup into the LookupCtx. if (LookupDirect(R, LookupCtx)) { R.resolveKind(); + if (isa(LookupCtx)) + R.setNamingClass(cast(LookupCtx)); return true; } @@ -1039,6 +1041,8 @@ bool Sema::LookupQualifiedName(LookupResult &R, DeclContext *LookupCtx, R.getLookupName().getAsOpaquePtr(), Paths)) return false; + R.setNamingClass(LookupRec); + // C++ [class.member.lookup]p2: // [...] If the resulting set of declarations are not all from // sub-objects of the same type, or the set has a nonstatic member @@ -1111,8 +1115,12 @@ bool Sema::LookupQualifiedName(LookupResult &R, DeclContext *LookupCtx, // Lookup in a base class succeeded; return these results. DeclContext::lookup_iterator I, E; - for (llvm::tie(I,E) = Paths.front().Decls; I != E; ++I) - R.addDecl(*I, std::max(SubobjectAccess, (*I)->getAccess())); + for (llvm::tie(I,E) = Paths.front().Decls; I != E; ++I) { + NamedDecl *D = *I; + AccessSpecifier AS = CXXRecordDecl::MergeAccess(SubobjectAccess, + D->getAccess()); + R.addDecl(D, AS); + } R.resolveKind(); return true; } diff --git a/clang/test/CXX/class.access/class.access.base/p1.cpp b/clang/test/CXX/class.access/class.access.base/p1.cpp new file mode 100644 index 000000000000..fd0d9f68e1e2 --- /dev/null +++ b/clang/test/CXX/class.access/class.access.base/p1.cpp @@ -0,0 +1,152 @@ +// RUN: %clang_cc1 -fsyntax-only -faccess-control -verify %s + +// C++0x [class.access.base]p1(a): +// If a class is declared to be a base class for another class using +// the public access specifier, the public members of the base class +// are accessible as public members of the derived class and protected +// members of the base class are accessible as protected members of +// the derived class. +namespace test0 { + class Base { + public: int pub; static int spub; + protected: int prot; static int sprot; // expected-note 4 {{declared protected here}} + private: int priv; static int spriv; // expected-note 8 {{declared private here}} + }; + + class Test : public Base { + void test() { + pub++; + spub++; + prot++; + sprot++; + priv++; // expected-error {{private member}} + spriv++; // expected-error {{private member}} + + Base::pub++; + Base::spub++; + Base::prot++; + Base::sprot++; + Base::priv++; // expected-error {{private member}} + Base::spriv++; // expected-error {{private member}} + } + }; + + void test(Test *t) { + t->pub++; + t->spub++; + t->prot++; // expected-error {{protected member}} + t->sprot++; // expected-error {{protected member}} + t->priv++; // expected-error {{private member}} + t->spriv++; // expected-error {{private member}} + + t->Base::pub++; + t->Base::spub++; + t->Base::prot++; // expected-error {{protected member}} + t->Base::sprot++; // expected-error {{protected member}} + t->Base::priv++; // expected-error {{private member}} + t->Base::spriv++; // expected-error {{private member}} + } +} + +// C++0x [class.access.base]p1(b): +// If a class is declared to be a base class for another class using +// the protected access specifier, the public and protected members +// of the base class are accessible as protected members of the +// derived class. +namespace test1 { + class Base { // expected-note 6 {{constrained by protected inheritance}} + public: int pub; static int spub; // expected-note 2 {{constrained by protected inheritance}} + protected: int prot; static int sprot; // expected-note 4 {{declared protected here}} + private: int priv; static int spriv; // expected-note 8 {{declared private here}} + }; + + class Test : protected Base { + void test() { + pub++; + spub++; + prot++; + sprot++; + priv++; // expected-error {{private member}} + spriv++; // expected-error {{private member}} + + Base::pub++; + Base::spub++; + Base::prot++; + Base::sprot++; + Base::priv++; // expected-error {{private member}} + Base::spriv++; // expected-error {{private member}} + } + }; + + void test(Test *t) { + t->pub++; // expected-error {{protected member}} + t->spub++; // expected-error {{protected member}} + t->prot++; // expected-error {{protected member}} + t->sprot++; // expected-error {{protected member}} + t->priv++; // expected-error {{private member}} + t->spriv++; // expected-error {{private member}} + + // Two possible errors here: one for Base, one for the member + t->Base::pub++; // expected-error {{protected member}} + t->Base::spub++; // expected-error {{protected member}} + t->Base::prot++; // expected-error 2 {{protected member}} + t->Base::sprot++; // expected-error 2 {{protected member}} + t->Base::priv++; // expected-error {{protected member}} expected-error {{private member}} + t->Base::spriv++; // expected-error {{protected member}} expected-error {{private member}} + } +} + +// C++0x [class.access.base]p1(b): +// If a class is declared to be a base class for another class using +// the private access specifier, the public and protected members of +// the base class are accessible as private members of the derived +// class. +namespace test2 { + class Base { //expected-note 6 {{constrained by private inheritance}} + public: + int pub; // expected-note {{constrained by private inheritance}} + static int spub; // expected-note {{constrained by private inheritance}} + protected: + int prot; // expected-note {{constrained by private inheritance}} \ + // expected-note {{declared protected here}} + static int sprot; // expected-note {{constrained by private inheritance}} \ + // expected-note {{declared protected here}} + private: + int priv; // expected-note 4 {{declared private here}} + static int spriv; // expected-note 4 {{declared private here}} + }; + + class Test : private Base { // expected-note 6 {{'private' inheritance specifier here}} + void test() { + pub++; + spub++; + prot++; + sprot++; + priv++; // expected-error {{private member}} + spriv++; // expected-error {{private member}} + + Base::pub++; + Base::spub++; + Base::prot++; + Base::sprot++; + Base::priv++; // expected-error {{private member}} + Base::spriv++; // expected-error {{private member}} + } + }; + + void test(Test *t) { + t->pub++; // expected-error {{private member}} expected-error {{inaccessible base class}} + t->spub++; // expected-error {{private member}} + t->prot++; // expected-error {{private member}} expected-error {{inaccessible base class}} + t->sprot++; // expected-error {{private member}} + t->priv++; // expected-error {{private member}} expected-error {{inaccessible base class}} + t->spriv++; // expected-error {{private member}} + + t->Base::pub++; // expected-error {{private member}} expected-error {{inaccessible base class}} + t->Base::spub++; // expected-error {{private member}} + t->Base::prot++; // expected-error {{protected member}} expected-error {{private member}} expected-error {{inaccessible base class}} + t->Base::sprot++; // expected-error {{protected member}} expected-error {{private member}} + t->Base::priv++; // expected-error 2 {{private member}} expected-error {{inaccessible base class}} + t->Base::spriv++; // expected-error 2 {{private member}} + } +} diff --git a/clang/test/SemaCXX/access-control-check.cpp b/clang/test/SemaCXX/access-control-check.cpp index e6e261c1a89f..cf2d191a294f 100644 --- a/clang/test/SemaCXX/access-control-check.cpp +++ b/clang/test/SemaCXX/access-control-check.cpp @@ -5,12 +5,11 @@ class M { }; class P { - int iP; - int PPR(); + int iP; // expected-note {{declared private here}} + int PPR(); // expected-note {{declared private here}} }; class N : M,P { N() {} - // FIXME. No access violation is reported in method call or member access. - int PR() { return iP + PPR(); } + int PR() { return iP + PPR(); } // expected-error 2 {{access to private member of 'class P'}} };