Fix several problems with protected access control:

  - The [class.protected] restriction is non-trivial for any instance
    member, even if the access lacks an object (for example, if it's
    a pointer-to-member constant).  In this case, it is equivalent to
    requiring the naming class to equal the context class.
  - The [class.protected] restriction applies to accesses to constructors
    and destructors.  A protected constructor or destructor can only be
    used to create or destroy a base subobject, as a direct result.
  - Several places were dropping or misapplying object information.

The standard could really be much clearer about what the object type is
supposed to be in some of these accesses.  Usually it's easy enough to
find a reasonable answer, but still, the standard makes a very confident
statement about accesses to instance members only being possible in
either pointer-to-member literals or member access expressions, which
just completely ignores concepts like constructor and destructor
calls, using declarations, unevaluated field references, etc.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@154248 91177308-0d34-0410-b5e6-96231b3b80d8

1 files changed, 140 insertions, 48 deletions

diff --git a/lib/Sema/SemaAccess.cpp b/lib/Sema/SemaAccess.cpp
index ea74346c16..74c4f34d75 100644
--- a/lib/Sema/SemaAccess.cpp
+++ b/lib/Sema/SemaAccess.cpp
@@ -165,6 +165,10 @@ struct AccessTarget : public AccessedEntity {
     initialize();
   }
 
+  bool isInstanceMember() const {
+    return (isMemberAccess() && getTargetDecl()->isCXXInstanceMember());
+  }
+
   bool hasInstanceContext() const {
     return HasInstanceContext;
   }
@@ -671,18 +675,25 @@ struct ProtectedFriendContext {
 }
 
 /// Search for a class P that EC is a friend of, under the constraint
-///   InstanceContext <= P <= NamingClass
+///   InstanceContext <= P
+/// if InstanceContext exists, or else
+///   NamingClass <= P
 /// and with the additional restriction that a protected member of
-/// NamingClass would have some natural access in P.
+/// NamingClass would have some natural access in P, which implicitly
+/// imposes the constraint that P <= NamingClass.
 ///
-/// That second condition isn't actually quite right: the condition in
-/// the standard is whether the target would have some natural access
-/// in P.  The difference is that the target might be more accessible
-/// along some path not passing through NamingClass.  Allowing that
+/// This isn't quite the condition laid out in the standard.
+/// Instead of saying that a notional protected member of NamingClass
+/// would have to have some natural access in P, it says the actual
+/// target has to have some natural access in P, which opens up the
+/// possibility that the target (which is not necessarily a member
+/// of NamingClass) might be more accessible along some path not
+/// passing through it.  That's really a bad idea, though, because it
 /// introduces two problems:
-///   - It breaks encapsulation because you can suddenly access a
-///     forbidden base class's members by subclassing it elsewhere.
-///   - It makes access substantially harder to compute because it
+///   - Most importantly, it breaks encapsulation because you can
+///     access a forbidden base class's members by directly subclassing
+///     it elsewhere.
+///   - It also makes access substantially harder to compute because it
 ///     breaks the hill-climbing algorithm: knowing that the target is
 ///     accessible in some base class would no longer let you change
 ///     the question solely to whether the base class is accessible,
@@ -692,9 +703,15 @@ struct ProtectedFriendContext {
 static AccessResult GetProtectedFriendKind(Sema &S, const EffectiveContext &EC,
                                            const CXXRecordDecl *InstanceContext,
                                            const CXXRecordDecl *NamingClass) {
-  assert(InstanceContext->getCanonicalDecl() == InstanceContext);
+  assert(InstanceContext == 0 ||
+         InstanceContext->getCanonicalDecl() == InstanceContext);
   assert(NamingClass->getCanonicalDecl() == NamingClass);
 
+  // If we don't have an instance context, our constraints give us
+  // that NamingClass <= P <= NamingClass, i.e. P == NamingClass.
+  // This is just the usual friendship check.
+  if (!InstanceContext) return GetFriendKind(S, EC, NamingClass);
+
   ProtectedFriendContext PRC(S, EC, InstanceContext, NamingClass);
   if (PRC.findFriendship(InstanceContext)) return AR_accessible;
   if (PRC.EverDependent) return AR_dependent;
@@ -737,15 +754,6 @@ static AccessResult HasAccess(Sema &S,
       case AR_dependent: OnFailure = AR_dependent; continue;
       }
 
-      if (!Target.hasInstanceContext())
-        return AR_accessible;
-
-      const CXXRecordDecl *InstanceContext = Target.resolveInstanceContext(S);
-      if (!InstanceContext) {
-        OnFailure = AR_dependent;
-        continue;
-      }
-
       // C++ [class.protected]p1:
       //   An additional access check beyond those described earlier in
       //   [class.access] is applied when a non-static data member or
@@ -758,8 +766,49 @@ static AccessResult HasAccess(Sema &S,
       //   expression. In this case, the class of the object expression
       //   shall be C or a class derived from C.
       //
-      // We interpret this as a restriction on [M3].  Most of the
-      // conditions are encoded by not having any instance context.
+      // We interpret this as a restriction on [M3].
+
+      // In this part of the code, 'C' is just our context class ECRecord.
+      
+      // These rules are different if we don't have an instance context.
+      if (!Target.hasInstanceContext()) {
+        // If it's not an instance member, these restrictions don't apply.
+        if (!Target.isInstanceMember()) return AR_accessible;
+
+        // If it's an instance member, use the pointer-to-member rule
+        // that the naming class has to be derived from the effective
+        // context.
+
+        // Despite the standard's confident wording, there is a case
+        // where you can have an instance member that's neither in a
+        // pointer-to-member expression nor in a member access:  when
+        // it names a field in an unevaluated context that can't be an
+        // implicit member.  Pending clarification, we just apply the
+        // same naming-class restriction here.
+        //   FIXME: we're probably not correctly adding the
+        //   protected-member restriction when we retroactively convert
+        //   an expression to being evaluated.
+
+        // We know that ECRecord derives from NamingClass.  The
+        // restriction says to check whether NamingClass derives from
+        // ECRecord, but that's not really necessary: two distinct
+        // classes can't be recursively derived from each other.  So
+        // along this path, we just need to check whether the classes
+        // are equal.
+        if (NamingClass == ECRecord) return AR_accessible;
+
+        // Otherwise, this context class tells us nothing;  on to the next.
+        continue;
+      }
+
+      assert(Target.isInstanceMember());
+
+      const CXXRecordDecl *InstanceContext = Target.resolveInstanceContext(S);
+      if (!InstanceContext) {
+        OnFailure = AR_dependent;
+        continue;
+      }
+
       switch (IsDerivedFromInclusive(InstanceContext, ECRecord)) {
       case AR_accessible: return AR_accessible;
       case AR_inaccessible: continue;
@@ -778,9 +827,14 @@ static AccessResult HasAccess(Sema &S,
   // *unless* the [class.protected] restriction applies.  If it does,
   // however, we should ignore whether the naming class is a friend,
   // and instead rely on whether any potential P is a friend.
-  if (Access == AS_protected && Target.hasInstanceContext()) {
-    const CXXRecordDecl *InstanceContext = Target.resolveInstanceContext(S);
-    if (!InstanceContext) return AR_dependent;
+  if (Access == AS_protected && Target.isInstanceMember()) {
+    // Compute the instance context if possible.
+    const CXXRecordDecl *InstanceContext = 0;
+    if (Target.hasInstanceContext()) {
+      InstanceContext = Target.resolveInstanceContext(S);
+      if (!InstanceContext) return AR_dependent;
+    }
+
     switch (GetProtectedFriendKind(S, EC, InstanceContext, NamingClass)) {
     case AR_accessible: return AR_accessible;
     case AR_inaccessible: return OnFailure;
@@ -950,31 +1004,46 @@ static CXXBasePath *FindBestPath(Sema &S,
 static bool TryDiagnoseProtectedAccess(Sema &S, const EffectiveContext &EC,
                                        AccessTarget &Target) {
   // Only applies to instance accesses.
-  if (!Target.hasInstanceContext())
+  if (!Target.isInstanceMember())
     return false;
+
   assert(Target.isMemberAccess());
-  NamedDecl *D = Target.getTargetDecl();
 
-  const CXXRecordDecl *DeclaringClass = Target.getDeclaringClass();
-  DeclaringClass = DeclaringClass->getCanonicalDecl();
+  const CXXRecordDecl *NamingClass = Target.getNamingClass();
+  NamingClass = NamingClass->getCanonicalDecl();
 
   for (EffectiveContext::record_iterator
          I = EC.Records.begin(), E = EC.Records.end(); I != E; ++I) {
     const CXXRecordDecl *ECRecord = *I;
-    switch (IsDerivedFromInclusive(ECRecord, DeclaringClass)) {
+    switch (IsDerivedFromInclusive(ECRecord, NamingClass)) {
     case AR_accessible: break;
     case AR_inaccessible: continue;
     case AR_dependent: continue;
     }
 
     // The effective context is a subclass of the declaring class.
-    // If that class isn't a superclass of the instance context,
-    // then the [class.protected] restriction applies.
+    // Check whether the [class.protected] restriction is limiting
+    // access.
 
     // To get this exactly right, this might need to be checked more
     // holistically;  it's not necessarily the case that gaining
     // access here would grant us access overall.
 
+    NamedDecl *D = Target.getTargetDecl();
+
+    // If we don't have an instance context, [class.protected] says the
+    // naming class has to equal the context class.
+    if (!Target.hasInstanceContext()) {
+      // If it does, the restriction doesn't apply.
+      if (NamingClass == ECRecord) continue;
+
+      // TODO: it would be great to have a fixit here, since this is
+      // such an obvious error.
+      S.Diag(D->getLocation(), diag::note_access_protected_restricted_noobject)
+        << S.Context.getTypeDeclType(ECRecord);
+      return true;
+    }
+
     const CXXRecordDecl *InstanceContext = Target.resolveInstanceContext(S);
     assert(InstanceContext && "diagnosing dependent access");
 
@@ -982,12 +1051,25 @@ static bool TryDiagnoseProtectedAccess(Sema &S, const EffectiveContext &EC,
     case AR_accessible: continue;
     case AR_dependent: continue;
     case AR_inaccessible:
-      S.Diag(D->getLocation(), diag::note_access_protected_restricted)
-        << (InstanceContext != Target.getNamingClass()->getCanonicalDecl())
-        << S.Context.getTypeDeclType(InstanceContext)
-        << S.Context.getTypeDeclType(ECRecord);
+      break;
+    }
+
+    // Okay, the restriction seems to be what's limiting us.
+
+    // Use a special diagnostic for constructors and destructors.
+    if (isa<CXXConstructorDecl>(D) || isa<CXXDestructorDecl>(D) ||
+        (isa<FunctionTemplateDecl>(D) &&
+         isa<CXXConstructorDecl>(
+                cast<FunctionTemplateDecl>(D)->getTemplatedDecl()))) {
+      S.Diag(D->getLocation(), diag::note_access_protected_restricted_ctordtor)
+        << isa<CXXDestructorDecl>(D);
       return true;
     }
+
+    // Otherwise, use the generic diagnostic.
+    S.Diag(D->getLocation(), diag::note_access_protected_restricted_object)
+      << S.Context.getTypeDeclType(ECRecord);
+    return true;
   }
 
   return false;
@@ -1427,7 +1509,8 @@ Sema::AccessResult Sema::CheckUnresolvedMemberAccess(UnresolvedMemberExpr *E,
 
 Sema::AccessResult Sema::CheckDestructorAccess(SourceLocation Loc,
                                                CXXDestructorDecl *Dtor,
-                                               const PartialDiagnostic &PDiag) {
+                                               const PartialDiagnostic &PDiag,
+                                               QualType ObjectTy) {
   if (!getLangOpts().AccessControl)
     return AR_accessible;
 
@@ -1437,9 +1520,11 @@ Sema::AccessResult Sema::CheckDestructorAccess(SourceLocation Loc,
     return AR_accessible;
 
   CXXRecordDecl *NamingClass = Dtor->getParent();
+  if (ObjectTy.isNull()) ObjectTy = Context.getTypeDeclType(NamingClass);
+
   AccessTarget Entity(Context, AccessTarget::Member, NamingClass,
                       DeclAccessPair::make(Dtor, Access),
-                      QualType());
+                      ObjectTy);
   Entity.setDiag(PDiag); // TODO: avoid copy
 
   return CheckAccess(*this, Loc, Entity);
@@ -1451,14 +1536,9 @@ Sema::AccessResult Sema::CheckConstructorAccess(SourceLocation UseLoc,
                                                 const InitializedEntity &Entity,
                                                 AccessSpecifier Access,
                                                 bool IsCopyBindingRefToTemp) {
-  if (!getLangOpts().AccessControl ||
-      Access == AS_public)
+  if (!getLangOpts().AccessControl || Access == AS_public)
     return AR_accessible;
 
-  CXXRecordDecl *NamingClass = Constructor->getParent();
-  AccessTarget AccessEntity(Context, AccessTarget::Member, NamingClass,
-                            DeclAccessPair::make(Constructor, Access),
-                            QualType());
   PartialDiagnostic PD(PDiag());
   switch (Entity.getKind()) {
   default:
@@ -1490,26 +1570,38 @@ Sema::AccessResult Sema::CheckConstructorAccess(SourceLocation UseLoc,
 
   }
 
-  return CheckConstructorAccess(UseLoc, Constructor, Access, PD);
+  return CheckConstructorAccess(UseLoc, Constructor, Entity, Access, PD);
 }
 
 /// Checks access to a constructor.
 Sema::AccessResult Sema::CheckConstructorAccess(SourceLocation UseLoc,
                                                 CXXConstructorDecl *Constructor,
+                                                const InitializedEntity &Entity,
                                                 AccessSpecifier Access,
-                                                PartialDiagnostic PD) {
+                                                const PartialDiagnostic &PD) {
   if (!getLangOpts().AccessControl ||
       Access == AS_public)
     return AR_accessible;
 
   CXXRecordDecl *NamingClass = Constructor->getParent();
+
+  // Initializing a base sub-object is an instance method call on an
+  // object of the derived class.  Otherwise, we have an instance method
+  // call on an object of the constructed type.
+  CXXRecordDecl *ObjectClass;
+  if (Entity.getKind() == InitializedEntity::EK_Base) {
+    ObjectClass = cast<CXXConstructorDecl>(CurContext)->getParent();
+  } else {
+    ObjectClass = NamingClass;
+  }
+
   AccessTarget AccessEntity(Context, AccessTarget::Member, NamingClass,
                             DeclAccessPair::make(Constructor, Access),
-                            QualType());
+                            Context.getTypeDeclType(ObjectClass));
   AccessEntity.setDiag(PD);
 
   return CheckAccess(*this, UseLoc, AccessEntity);
-}
+} 
 
 /// Checks direct (i.e. non-inherited) access to an arbitrary class
 /// member.
@@ -1583,7 +1675,7 @@ Sema::AccessResult Sema::CheckAddressOfMemberAccess(Expr *OvlExpr,
   CXXRecordDecl *NamingClass = Ovl->getNamingClass();
 
   AccessTarget Entity(Context, AccessTarget::Member, NamingClass, Found,
-                      Context.getTypeDeclType(NamingClass));
+                      /*no instance context*/ QualType());
   Entity.setDiag(diag::err_access)
     << Ovl->getSourceRange();