When parsing an out-of-line member function declaration, we must delay

access-control diagnostics which arise from the portion of the declarator
following the scope specifier, just in case access is granted by
friending the individual method.  This can also happen with in-line
member function declarations of class templates due to templated-scope
friend declarations.

We were really playing fast-and-loose before with this sort of thing,
and it turned out to work because *most* friend functions are in file
scope.  Making us delay regardless of context exposed several bugs with
how we were manipulating delay.  I ended up needing a concept of a
context that's independent of the declarations in which it appears,
and then I actually had to make some things save contexts correctly,
but delay should be much cleaner now.

I also encapsulated all the delayed-diagnostics machinery in a single
subobject of Sema;  this is a pattern we might want to consider rolling
out to other components of Sema.



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

include/clang/Parse/Parser.h

@@ -725,7 +725,7 @@ private:
   /// class or function definition.
   class ParsingDeclRAIIObject {
     Sema &Actions;
-    Sema::ParsingDeclStackState State;
+    Sema::ParsingDeclState State;
     bool Popped;
 
   public:
@@ -837,23 +837,24 @@ private:
   class ParsingClassDefinition {
     Parser &P;
     bool Popped;
+    Sema::ParsingClassState State;
 
   public:
     ParsingClassDefinition(Parser &P, Decl *TagOrTemplate, bool TopLevelClass)
-      : P(P), Popped(false) {
-      P.PushParsingClass(TagOrTemplate, TopLevelClass);
+      : P(P), Popped(false),
+        State(P.PushParsingClass(TagOrTemplate, TopLevelClass)) {
     }
 
     /// \brief Pop this class of the stack.
     void Pop() {
       assert(!Popped && "Nested class has already been popped");
       Popped = true;
-      P.PopParsingClass();
+      P.PopParsingClass(State);
     }
 
     ~ParsingClassDefinition() {
       if (!Popped)
-        P.PopParsingClass();
+        P.PopParsingClass(State);
     }
   };
 
@@ -907,11 +908,12 @@ private:
     SourceRange getSourceRange() const;
   };
 
-  void PushParsingClass(Decl *TagOrTemplate, bool TopLevelClass);
+  Sema::ParsingClassState
+  PushParsingClass(Decl *TagOrTemplate, bool TopLevelClass);
   void DeallocateParsedClasses(ParsingClass *Class);
-  void PopParsingClass();
+  void PopParsingClass(Sema::ParsingClassState);
 
-  Decl *ParseCXXInlineMethodDef(AccessSpecifier AS, Declarator &D,
+  Decl *ParseCXXInlineMethodDef(AccessSpecifier AS, ParsingDeclarator &D,
                                 const ParsedTemplateInfo &TemplateInfo,
                                 const VirtSpecifiers& VS);
   void ParseLexedMethodDeclarations(ParsingClass &Class);

include/clang/Sema/DelayedDiagnostic.h

@@ -101,7 +101,7 @@ public:
 
 private:
   unsigned Access : 2;
-  bool IsMember;
+  unsigned IsMember : 1;
   NamedDecl *Target;
   CXXRecordDecl *NamingClass;
   QualType BaseObjectType;

include/clang/Sema/Sema.h

@@ -233,36 +233,6 @@ public:
   /// This is used as part of a hack to omit that class from ADL results.
   DeclarationName VAListTagName;
 
-  /// A RAII object to temporarily push a declaration context.
-  class ContextRAII {
-  private:
-    Sema &S;
-    DeclContext *SavedContext;
-    unsigned SavedParsingDeclDepth;
-    
-  public:
-    ContextRAII(Sema &S, DeclContext *ContextToPush,
-                unsigned ParsingDeclDepth = 0)
-      : S(S), SavedContext(S.CurContext), 
-        SavedParsingDeclDepth(S.ParsingDeclDepth) 
-    {
-      assert(ContextToPush && "pushing null context");
-      S.CurContext = ContextToPush;
-      S.ParsingDeclDepth = 0;
-    }
-
-    void pop() {
-      if (!SavedContext) return;
-      S.CurContext = SavedContext;
-      S.ParsingDeclDepth = SavedParsingDeclDepth;
-      SavedContext = 0;
-    }
-
-    ~ContextRAII() {
-      pop();
-    }
-  };
-
   /// PackContext - Manages the stack for #pragma pack. An alignment
   /// of 0 indicates default alignment.
   void *PackContext; // Really a "PragmaPackStack*"
@@ -331,14 +301,125 @@ public:
   /// and must warn if not used. Only contains the first declaration.
   llvm::SmallVector<const DeclaratorDecl*, 4> UnusedFileScopedDecls;
 
-  /// \brief The stack of diagnostics that were delayed due to being
-  /// produced during the parsing of a declaration.
-  llvm::SmallVector<sema::DelayedDiagnostic, 0> DelayedDiagnostics;
+  class DelayedDiagnostics;
 
-  /// \brief The depth of the current ParsingDeclaration stack.
-  /// If nonzero, we are currently parsing a declaration (and
-  /// hence should delay deprecation warnings).
-  unsigned ParsingDeclDepth;
+  class ParsingDeclState {
+    unsigned SavedStackSize;
+    friend class Sema::DelayedDiagnostics;
+  };
+
+  class ProcessingContextState {
+    unsigned SavedParsingDepth;
+    unsigned SavedActiveStackBase;
+    friend class Sema::DelayedDiagnostics;
+  };
+
+  /// A class which encapsulates the logic for delaying diagnostics
+  /// during parsing and other processing.
+  class DelayedDiagnostics {
+    /// \brief The stack of diagnostics that were delayed due to being
+    /// produced during the parsing of a declaration.
+    sema::DelayedDiagnostic *Stack;
+
+    /// \brief The number of objects on the delayed-diagnostics stack.
+    unsigned StackSize;
+
+    /// \brief The current capacity of the delayed-diagnostics stack.
+    unsigned StackCapacity;
+
+    /// \brief The index of the first "active" delayed diagnostic in
+    /// the stack.  When parsing class definitions, we ignore active
+    /// delayed diagnostics from the surrounding context.
+    unsigned ActiveStackBase;
+
+    /// \brief The depth of the declarations we're currently parsing.
+    /// This gets saved and reset whenever we enter a class definition.
+    unsigned ParsingDepth;
+
+  public:
+    DelayedDiagnostics() : Stack(0), StackSize(0), StackCapacity(0),
+      ActiveStackBase(0), ParsingDepth(0) {}
+
+    ~DelayedDiagnostics() {
+      delete[] reinterpret_cast<char*>(Stack);
+    }
+
+    /// Adds a delayed diagnostic.
+    void add(const sema::DelayedDiagnostic &diag);
+
+    /// Determines whether diagnostics should be delayed.
+    bool shouldDelayDiagnostics() { return ParsingDepth > 0; }
+
+    /// Observe that we've started parsing a declaration.  Access and
+    /// deprecation diagnostics will be delayed; when the declaration
+    /// is completed, all active delayed diagnostics will be evaluated
+    /// in its context, and then active diagnostics stack will be
+    /// popped down to the saved depth.
+    ParsingDeclState pushParsingDecl() {
+      ParsingDepth++;
+
+      ParsingDeclState state;
+      state.SavedStackSize = StackSize;
+      return state;
+    }
+
+    /// Observe that we're completed parsing a declaration.
+    static void popParsingDecl(Sema &S, ParsingDeclState state, Decl *decl);
+
+    /// Observe that we've started processing a different context, the
+    /// contents of which are semantically separate from the
+    /// declarations it may lexically appear in.  This sets aside the
+    /// current stack of active diagnostics and starts afresh.
+    ProcessingContextState pushContext() {
+      assert(StackSize >= ActiveStackBase);
+
+      ProcessingContextState state;
+      state.SavedParsingDepth = ParsingDepth;
+      state.SavedActiveStackBase = ActiveStackBase;
+
+      ActiveStackBase = StackSize;
+      ParsingDepth = 0;
+
+      return state;
+    }
+
+    /// Observe that we've stopped processing a context.  This
+    /// restores the previous stack of active diagnostics.
+    void popContext(ProcessingContextState state) {
+      assert(ActiveStackBase == StackSize);
+      assert(ParsingDepth == 0);
+      ActiveStackBase = state.SavedActiveStackBase;
+      ParsingDepth = state.SavedParsingDepth;
+    }  
+  } DelayedDiagnostics;
+
+  /// A RAII object to temporarily push a declaration context.
+  class ContextRAII {
+  private:
+    Sema &S;
+    DeclContext *SavedContext;
+    ProcessingContextState SavedContextState;
+    
+  public:
+    ContextRAII(Sema &S, DeclContext *ContextToPush)
+      : S(S), SavedContext(S.CurContext), 
+        SavedContextState(S.DelayedDiagnostics.pushContext()) 
+    {
+      assert(ContextToPush && "pushing null context");
+      S.CurContext = ContextToPush;
+    }
+
+    void pop() {
+      if (!SavedContext) return;
+      S.CurContext = SavedContext;
+      S.DelayedDiagnostics.popContext(SavedContextState);
+      SavedContext = 0;
+    }
+
+    ~ContextRAII() {
+      pop();
+    }
+  };
 
   /// WeakUndeclaredIdentifiers - Identifiers contained in
   /// #pragma weak before declared. rare. may alias another
@@ -1740,10 +1821,21 @@ public:
   void DiagnoseUnusedExprResult(const Stmt *S);
   void DiagnoseUnusedDecl(const NamedDecl *ND);
   
-  typedef uintptr_t ParsingDeclStackState;
+  ParsingDeclState PushParsingDeclaration() {
+    return DelayedDiagnostics.pushParsingDecl();
+  }
+  void PopParsingDeclaration(ParsingDeclState state, Decl *decl) {
+    DelayedDiagnostics::popParsingDecl(*this, state, decl);
+  }
+
+  typedef ProcessingContextState ParsingClassState;
+  ParsingClassState PushParsingClass() {
+    return DelayedDiagnostics.pushContext();
+  }
+  void PopParsingClass(ParsingClassState state) {
+    DelayedDiagnostics.popContext(state);
+  }
 
-  ParsingDeclStackState PushParsingDeclaration();
-  void PopParsingDeclaration(ParsingDeclStackState S, Decl *D);
   void EmitDeprecationWarning(NamedDecl *D, llvm::StringRef Message,
                               SourceLocation Loc, bool UnknownObjCClass=false);
 

lib/Parse/ParseCXXInlineMethods.cpp

@@ -20,7 +20,7 @@ using namespace clang;
 /// ParseCXXInlineMethodDef - We parsed and verified that the specified
 /// Declarator is a well formed C++ inline method definition. Now lex its body
 /// and store its tokens for parsing after the C++ class is complete.
-Decl *Parser::ParseCXXInlineMethodDef(AccessSpecifier AS, Declarator &D,
+Decl *Parser::ParseCXXInlineMethodDef(AccessSpecifier AS, ParsingDeclarator &D,
                                 const ParsedTemplateInfo &TemplateInfo,
                                 const VirtSpecifiers& VS) {
   assert(D.isFunctionDeclarator() && "This isn't a function declarator!");
@@ -51,6 +51,8 @@ Decl *Parser::ParseCXXInlineMethodDef(AccessSpecifier AS, Declarator &D,
 
   HandleMemberFunctionDefaultArgs(D, FnD);
 
+  D.complete(FnD);
+
   // Consume the tokens and store them for later parsing.
 
   LexedMethod* LM = new LexedMethod(this, FnD);

lib/Parse/ParseDeclCXX.cpp

@@ -2087,10 +2087,12 @@ TypeResult Parser::ParseTrailingReturnType() {
 /// \brief We have just started parsing the definition of a new class,
 /// so push that class onto our stack of classes that is currently
 /// being parsed.
-void Parser::PushParsingClass(Decl *ClassDecl, bool NonNestedClass) {
+Sema::ParsingClassState
+Parser::PushParsingClass(Decl *ClassDecl, bool NonNestedClass) {
   assert((NonNestedClass || !ClassStack.empty()) &&
          "Nested class without outer class");
   ClassStack.push(new ParsingClass(ClassDecl, NonNestedClass));
+  return Actions.PushParsingClass();
 }
 
 /// \brief Deallocate the given parsed class and all of its nested
@@ -2110,9 +2112,11 @@ void Parser::DeallocateParsedClasses(Parser::ParsingClass *Class) {
 ///
 /// \returns true if the class we've popped is a top-level class,
 /// false otherwise.
-void Parser::PopParsingClass() {
+void Parser::PopParsingClass(Sema::ParsingClassState state) {
   assert(!ClassStack.empty() && "Mismatched push/pop for class parsing");
 
+  Actions.PopParsingClass(state);
+
   ParsingClass *Victim = ClassStack.top();
   ClassStack.pop();
   if (Victim->TopLevelClass) {

lib/Parse/ParseTemplate.cpp

@@ -246,7 +246,7 @@ Parser::ParseSingleDeclarationAfterTemplate(
 
     // Eat the semi colon after the declaration.
     ExpectAndConsume(tok::semi, diag::err_expected_semi_declaration);
-    DS.complete(ThisDecl);
+    DeclaratorInfo.complete(ThisDecl);
     return ThisDecl;
   }
 

lib/Sema/Sema.cpp

@@ -135,7 +135,7 @@ Sema::Sema(Preprocessor &pp, ASTContext &ctxt, ASTConsumer &consumer,
     LangOpts(pp.getLangOptions()), PP(pp), Context(ctxt), Consumer(consumer),
     Diags(PP.getDiagnostics()), SourceMgr(PP.getSourceManager()),
     ExternalSource(0), CodeCompleter(CodeCompleter), CurContext(0), 
-    PackContext(0), VisContext(0), ParsingDeclDepth(0),
+    PackContext(0), VisContext(0),
     IdResolver(pp.getLangOptions()), CXXTypeInfoDecl(0), MSVCGuidDecl(0),
     GlobalNewDeleteDeclared(false), 
     CompleteTranslationUnit(CompleteTranslationUnit),

lib/Sema/SemaAccess.cpp

@@ -1260,13 +1260,19 @@ static Sema::AccessResult CheckAccess(Sema &S, SourceLocation Loc,
   if (S.SuppressAccessChecking)
     return Sema::AR_accessible;
 
-  // If we're currently parsing a top-level declaration, delay
-  // diagnostics.  This is the only case where parsing a declaration
-  // can actually change our effective context for the purposes of
-  // access control.
-  if (S.CurContext->isFileContext() && S.ParsingDeclDepth) {
-    S.DelayedDiagnostics.push_back(
-        DelayedDiagnostic::makeAccess(Loc, Entity));
+  // If we're currently parsing a declaration, we may need to delay
+  // access control checking, because our effective context might be
+  // different based on what the declaration comes out as.
+  //
+  // For example, we might be parsing a declaration with a scope
+  // specifier, like this:
+  //   A::private_type A::foo() { ... }
+  //
+  // Or we might be parsing something that will turn out to be a friend:
+  //   void foo(A::private_type);
+  //   void B::foo(A::private_type);
+  if (S.DelayedDiagnostics.shouldDelayDiagnostics()) {
+    S.DelayedDiagnostics.add(DelayedDiagnostic::makeAccess(Loc, Entity));
     return Sema::AR_delayed;
   }
 

lib/Sema/SemaDeclAttr.cpp

@@ -2923,59 +2923,77 @@ void Sema::ProcessDeclAttributes(Scope *S, Decl *D, const Declarator &PD,
     ProcessDeclAttributeList(S, D, Attrs, NonInheritable, Inheritable);
 }
 
-/// PushParsingDeclaration - Enter a new "scope" of deprecation
-/// warnings.
-///
-/// The state token we use is the start index of this scope
-/// on the warning stack.
-Sema::ParsingDeclStackState Sema::PushParsingDeclaration() {
-  ParsingDeclDepth++;
-  return (ParsingDeclStackState) DelayedDiagnostics.size();
+// This duplicates a vector push_back but hides the need to know the
+// size of the type.
+void Sema::DelayedDiagnostics::add(const DelayedDiagnostic &diag) {
+  assert(StackSize <= StackCapacity);
+
+  // Grow the stack if necessary.
+  if (StackSize == StackCapacity) {
+    unsigned newCapacity = 2 * StackCapacity + 2;
+    char *newBuffer = new char[newCapacity * sizeof(DelayedDiagnostic)];
+    const char *oldBuffer = (const char*) Stack;
+
+    if (StackCapacity)
+      memcpy(newBuffer, oldBuffer, StackCapacity * sizeof(DelayedDiagnostic));
+    
+    delete[] oldBuffer;
+    Stack = reinterpret_cast<sema::DelayedDiagnostic*>(newBuffer);
+    StackCapacity = newCapacity;
+  }
+
+  assert(StackSize < StackCapacity);
+  new (&Stack[StackSize++]) DelayedDiagnostic(diag);
 }
 
-void Sema::PopParsingDeclaration(ParsingDeclStackState S, Decl *D) {
-  assert(ParsingDeclDepth > 0 && "empty ParsingDeclaration stack");
-  ParsingDeclDepth--;
+void Sema::DelayedDiagnostics::popParsingDecl(Sema &S, ParsingDeclState state,
+                                              Decl *decl) {
+  DelayedDiagnostics &DD = S.DelayedDiagnostics;
 
-  if (DelayedDiagnostics.empty())
+  // Check the invariants.
+  assert(DD.StackSize >= state.SavedStackSize);
+  assert(state.SavedStackSize >= DD.ActiveStackBase);
+  assert(DD.ParsingDepth > 0);
+
+  // Drop the parsing depth.
+  DD.ParsingDepth--;
+
+  // If there are no active diagnostics, we're done.
+  if (DD.StackSize == DD.ActiveStackBase)
     return;
 
-  unsigned SavedIndex = (unsigned) S;
-  assert(SavedIndex <= DelayedDiagnostics.size() &&
-         "saved index is out of bounds");
-
-  unsigned E = DelayedDiagnostics.size();
-
   // We only want to actually emit delayed diagnostics when we
   // successfully parsed a decl.
-  if (D) {
-    // We really do want to start with 0 here.  We get one push for a
+  if (decl) {
+    // We emit all the active diagnostics, not just those starting
+    // from the saved state.  The idea is this:  we get one push for a
     // decl spec and another for each declarator;  in a decl group like:
     //   deprecated_typedef foo, *bar, baz();
     // only the declarator pops will be passed decls.  This is correct;
     // we really do need to consider delayed diagnostics from the decl spec
     // for each of the different declarations.
-    for (unsigned I = 0; I != E; ++I) {
-      if (DelayedDiagnostics[I].Triggered)
+    for (unsigned i = DD.ActiveStackBase, e = DD.StackSize; i != e; ++i) {
+      DelayedDiagnostic &diag = DD.Stack[i];
+      if (diag.Triggered)
         continue;
 
-      switch (DelayedDiagnostics[I].Kind) {
+      switch (diag.Kind) {
       case DelayedDiagnostic::Deprecation:
-        HandleDelayedDeprecationCheck(DelayedDiagnostics[I], D);
+        S.HandleDelayedDeprecationCheck(diag, decl);
         break;
 
       case DelayedDiagnostic::Access:
-        HandleDelayedAccessCheck(DelayedDiagnostics[I], D);
+        S.HandleDelayedAccessCheck(diag, decl);
         break;
       }
     }
   }
 
   // Destroy all the delayed diagnostics we're about to pop off.
-  for (unsigned I = SavedIndex; I != E; ++I)
-    DelayedDiagnostics[I].destroy();
+  for (unsigned i = state.SavedStackSize, e = DD.StackSize; i != e; ++i)
+    DD.Stack[i].destroy();
 
-  DelayedDiagnostics.set_size(SavedIndex);
+  DD.StackSize = state.SavedStackSize;
 }
 
 static bool isDeclDeprecated(Decl *D) {
@@ -3005,9 +3023,8 @@ void Sema::EmitDeprecationWarning(NamedDecl *D, llvm::StringRef Message,
                                   SourceLocation Loc,
                                   bool UnknownObjCClass) {
   // Delay if we're currently parsing a declaration.
-  if (ParsingDeclDepth) {
-    DelayedDiagnostics.push_back(DelayedDiagnostic::makeDeprecation(Loc, D, 
-                                                                    Message));
+  if (DelayedDiagnostics.shouldDelayDiagnostics()) {
+    DelayedDiagnostics.add(DelayedDiagnostic::makeDeprecation(Loc, D, Message));
     return;
   }
 

lib/Sema/SemaDeclCXX.cpp

@@ -4556,13 +4556,12 @@ namespace {
   /// to implicitly define the body of a C++ member function;
   class ImplicitlyDefinedFunctionScope {
     Sema &S;
-    DeclContext *PreviousContext;
+    Sema::ContextRAII SavedContext;
     
   public:
     ImplicitlyDefinedFunctionScope(Sema &S, CXXMethodDecl *Method)
-      : S(S), PreviousContext(S.CurContext) 
+      : S(S), SavedContext(S, Method) 
     {
-      S.CurContext = Method;
       S.PushFunctionScope();
       S.PushExpressionEvaluationContext(Sema::PotentiallyEvaluated);
     }
@@ -4570,7 +4569,6 @@ namespace {
     ~ImplicitlyDefinedFunctionScope() {
       S.PopExpressionEvaluationContext();
       S.PopFunctionOrBlockScope();
-      S.CurContext = PreviousContext;
     }
   };
 }
@@ -7281,6 +7279,10 @@ bool Sema::CheckOverridingFunctionReturnType(const CXXMethodDecl *New,
                     diag::err_covariant_return_ambiguous_derived_to_base_conv,
                     // FIXME: Should this point to the return type?
                     New->getLocation(), SourceRange(), New->getDeclName(), 0)) {
+      // FIXME: this note won't trigger for delayed access control
+      // diagnostics, and it's impossible to get an undelayed error
+      // here from access control during the original parse because
+      // the ParsingDeclSpec/ParsingDeclarator are still in scope.
       Diag(Old->getLocation(), diag::note_overridden_virtual_function);
       return true;
     }

lib/Sema/SemaTemplateInstantiateDecl.cpp

@@ -2310,8 +2310,7 @@ void Sema::InstantiateFunctionDefinition(SourceLocation PointOfInstantiation,
 
   // Enter the scope of this instantiation. We don't use
   // PushDeclContext because we don't have a scope.
-  DeclContext *PreviousContext = CurContext;
-  CurContext = Function;
+  Sema::ContextRAII savedContext(*this, Function);
 
   MultiLevelTemplateArgumentList TemplateArgs =
     getTemplateInstantiationArgs(Function, 0, false, PatternDecl);
@@ -2334,7 +2333,7 @@ void Sema::InstantiateFunctionDefinition(SourceLocation PointOfInstantiation,
 
   PerformDependentDiagnostics(PatternDecl, TemplateArgs);
 
-  CurContext = PreviousContext;
+  savedContext.pop();
 
   DeclGroupRef DG(Function);
   Consumer.HandleTopLevelDecl(DG);

test/CXX/class.access/class.friend/p1.cpp

@@ -305,3 +305,24 @@ namespace test10 {
     NS::bar->foo(); // expected-error {{private member}}
   }
 }
+
+// PR8705
+namespace test11 {
+  class A {
+    void test0(int);
+    void test1(int);
+    void test2(int);
+    void test3(int);
+  };
+
+  class B {
+    typedef int private_type; // expected-note 2 {{implicitly declared private here}}
+    friend void A::test0(int);
+    friend void A::test1(int);
+  };
+
+  void A::test0(B::private_type x) {}
+  void A::test1(int x = B::private_type()) {}
+  void A::test2(B::private_type x) {} // expected-error {{'private_type' is a private member of 'test11::B'}}
+  void A::test3(int x = B::private_type()) {} // expected-error {{'private_type' is a private member of 'test11::B'}}
+}

test/SemaCXX/virtual-override.cpp

@@ -32,7 +32,7 @@ struct a { };
 struct b : private a { }; // expected-note{{declared private here}}
   
 class A {
-  virtual a* f(); // expected-note{{overridden virtual function is here}}
+  virtual a* f(); // FIXME: desired-note{{overridden virtual function is here}}
 };
 
 class B : A {