Update VerifyDiagnosticConsumer to only get directives during parsing.

The old behavior was to re-scan any files (like modules) where we may have
directives but won't actually be parsing during the -verify invocation.
Now, we keep the old behavior in Debug builds as a sanity check (though
modules are a known entity), and expect all legitimate directives to come
from comments seen by the preprocessor.

This also affects the ARC migration tool, which captures diagnostics in
order to filter some out. This change adds an explicit cleanup to
CaptureDiagnosticsConsumer in order to let its sub-consumer handle the
real end of diagnostics.

This was originally split into four patches, but the tests do not run
cleanly without all four, so I've combined them into one commit.

Patches by Andy Gibbs, with slight modifications from me.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@161650 91177308-0d34-0410-b5e6-96231b3b80d8
diff --git a/include/clang/Frontend/VerifyDiagnosticConsumer.h b/include/clang/Frontend/VerifyDiagnosticConsumer.h
index 4a6081b..a74589e 100644
--- a/include/clang/Frontend/VerifyDiagnosticConsumer.h
+++ b/include/clang/Frontend/VerifyDiagnosticConsumer.h
@@ -166,17 +166,22 @@
     }
   };
 
-private:
+#ifndef NDEBUG
   typedef llvm::DenseSet<FileID> FilesWithDiagnosticsSet;
-  typedef llvm::SmallPtrSet<const FileEntry *, 4> FilesWithDirectivesSet;
+  typedef llvm::SmallPtrSet<const FileEntry *, 4> FilesParsedForDirectivesSet;
+#endif
 
+private:
   DiagnosticsEngine &Diags;
   DiagnosticConsumer *PrimaryClient;
   bool OwnsPrimaryClient;
   OwningPtr<TextDiagnosticBuffer> Buffer;
   const Preprocessor *CurrentPreprocessor;
+  unsigned ActiveSourceFiles;
+#ifndef NDEBUG
   FilesWithDiagnosticsSet FilesWithDiagnostics;
-  FilesWithDirectivesSet FilesWithDirectives;
+  FilesParsedForDirectivesSet FilesParsedForDirectives;
+#endif
   ExpectedData ED;
   void CheckDiagnostics();
 
@@ -192,6 +197,13 @@
 
   virtual void EndSourceFile();
 
+  /// \brief Manually register a file as parsed.
+  inline void appendParsedFile(const FileEntry *File) {
+#ifndef NDEBUG
+    FilesParsedForDirectives.insert(File);
+#endif
+  }
+
   virtual bool HandleComment(Preprocessor &PP, SourceRange Comment);
 
   virtual void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
diff --git a/lib/ARCMigrate/ARCMT.cpp b/lib/ARCMigrate/ARCMT.cpp
index dd9461b..f291dec 100644
--- a/lib/ARCMigrate/ARCMT.cpp
+++ b/lib/ARCMigrate/ARCMT.cpp
@@ -91,11 +91,40 @@
 
 class CaptureDiagnosticConsumer : public DiagnosticConsumer {
   DiagnosticsEngine &Diags;
+  DiagnosticConsumer &DiagClient;
   CapturedDiagList &CapturedDiags;
+  bool HasBegunSourceFile;
 public:
   CaptureDiagnosticConsumer(DiagnosticsEngine &diags,
-                           CapturedDiagList &capturedDiags)
-    : Diags(diags), CapturedDiags(capturedDiags) { }
+                            DiagnosticConsumer &client,
+                            CapturedDiagList &capturedDiags)
+    : Diags(diags), DiagClient(client), CapturedDiags(capturedDiags),
+      HasBegunSourceFile(false) { }
+
+  virtual void BeginSourceFile(const LangOptions &Opts,
+                               const Preprocessor *PP) {
+    // Pass BeginSourceFile message onto DiagClient on first call.
+    // The corresponding EndSourceFile call will be made from an
+    // explicit call to FinishCapture.
+    if (!HasBegunSourceFile) {
+      DiagClient.BeginSourceFile(Opts, PP);
+      HasBegunSourceFile = true;
+    }
+  }
+
+  void FinishCapture() {
+    // Call EndSourceFile on DiagClient on completion of capture to
+    // enable VerifyDiagnosticConsumer to check diagnostics *after*
+    // it has received the diagnostic list.
+    if (HasBegunSourceFile) {
+      DiagClient.EndSourceFile();
+      HasBegunSourceFile = false;
+    }
+  }
+
+  virtual ~CaptureDiagnosticConsumer() {
+    assert(!HasBegunSourceFile && "FinishCapture not called!");
+  }
 
   virtual void HandleDiagnostic(DiagnosticsEngine::Level level,
                                 const Diagnostic &Info) {
@@ -260,13 +289,15 @@
       new DiagnosticsEngine(DiagID, DiagClient, /*ShouldOwnClient=*/false));
 
   // Filter of all diagnostics.
-  CaptureDiagnosticConsumer errRec(*Diags, capturedDiags);
+  CaptureDiagnosticConsumer errRec(*Diags, *DiagClient, capturedDiags);
   Diags->setClient(&errRec, /*ShouldOwnClient=*/false);
 
   OwningPtr<ASTUnit> Unit(
       ASTUnit::LoadFromCompilerInvocationAction(CInvok.take(), Diags));
-  if (!Unit)
+  if (!Unit) {
+    errRec.FinishCapture();
     return true;
+  }
 
   // Don't filter diagnostics anymore.
   Diags->setClient(DiagClient, /*ShouldOwnClient=*/false);
@@ -278,6 +309,7 @@
     DiagClient->BeginSourceFile(Ctx.getLangOpts(), &Unit->getPreprocessor());
     capturedDiags.reportDiagnostics(*Diags);
     DiagClient->EndSourceFile();
+    errRec.FinishCapture();
     return true;
   }
 
@@ -315,6 +347,7 @@
   capturedDiags.reportDiagnostics(*Diags);
 
   DiagClient->EndSourceFile();
+  errRec.FinishCapture();
 
   // If we are migrating code that gets the '-fobjc-arc' flag, make sure
   // to remove it so that we don't get errors from normal compilation.
@@ -563,7 +596,7 @@
       new DiagnosticsEngine(DiagID, DiagClient, /*ShouldOwnClient=*/false));
 
   // Filter of all diagnostics.
-  CaptureDiagnosticConsumer errRec(*Diags, capturedDiags);
+  CaptureDiagnosticConsumer errRec(*Diags, *DiagClient, capturedDiags);
   Diags->setClient(&errRec, /*ShouldOwnClient=*/false);
 
   OwningPtr<ARCMTMacroTrackerAction> ASTAction;
@@ -572,8 +605,10 @@
   OwningPtr<ASTUnit> Unit(
       ASTUnit::LoadFromCompilerInvocationAction(CInvok.take(), Diags,
                                                 ASTAction.get()));
-  if (!Unit)
+  if (!Unit) {
+    errRec.FinishCapture();
     return true;
+  }
   Unit->setOwnsRemappedFileBuffers(false); // FileRemapper manages that.
 
   // Don't filter diagnostics anymore.
@@ -586,6 +621,7 @@
     DiagClient->BeginSourceFile(Ctx.getLangOpts(), &Unit->getPreprocessor());
     capturedDiags.reportDiagnostics(*Diags);
     DiagClient->EndSourceFile();
+    errRec.FinishCapture();
     return true;
   }
 
@@ -609,6 +645,7 @@
   }
 
   DiagClient->EndSourceFile();
+  errRec.FinishCapture();
 
   if (DiagClient->getNumErrors())
     return true;
diff --git a/lib/Frontend/VerifyDiagnosticConsumer.cpp b/lib/Frontend/VerifyDiagnosticConsumer.cpp
index 8aa65f1..71c3f6b 100644
--- a/lib/Frontend/VerifyDiagnosticConsumer.cpp
+++ b/lib/Frontend/VerifyDiagnosticConsumer.cpp
@@ -15,6 +15,7 @@
 #include "clang/Frontend/VerifyDiagnosticConsumer.h"
 #include "clang/Frontend/FrontendDiagnostic.h"
 #include "clang/Frontend/TextDiagnosticBuffer.h"
+#include "clang/Lex/HeaderSearch.h"
 #include "clang/Lex/Preprocessor.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/Support/Regex.h"
@@ -26,46 +27,91 @@
 typedef VerifyDiagnosticConsumer::ExpectedData ExpectedData;
 
 VerifyDiagnosticConsumer::VerifyDiagnosticConsumer(DiagnosticsEngine &_Diags)
-  : Diags(_Diags), PrimaryClient(Diags.getClient()),
-    OwnsPrimaryClient(Diags.ownsClient()),
-    Buffer(new TextDiagnosticBuffer()), CurrentPreprocessor(0) 
+  : Diags(_Diags),
+    PrimaryClient(Diags.getClient()), OwnsPrimaryClient(Diags.ownsClient()),
+    Buffer(new TextDiagnosticBuffer()), CurrentPreprocessor(0),
+    ActiveSourceFiles(0)
 {
   Diags.takeClient();
 }
 
 VerifyDiagnosticConsumer::~VerifyDiagnosticConsumer() {
+  assert(!ActiveSourceFiles && "Incomplete parsing of source files!");
+  assert(!CurrentPreprocessor && "CurrentPreprocessor should be invalid!");
   CheckDiagnostics();  
   Diags.takeClient();
   if (OwnsPrimaryClient)
     delete PrimaryClient;
 }
 
+#ifndef NDEBUG
+namespace {
+class VerifyFileTracker : public PPCallbacks {
+  typedef VerifyDiagnosticConsumer::FilesParsedForDirectivesSet ListType;
+  ListType &FilesList;
+  SourceManager &SM;
+
+public:
+  VerifyFileTracker(ListType &FilesList, SourceManager &SM)
+    : FilesList(FilesList), SM(SM) { }
+
+  /// \brief Hook into the preprocessor and update the list of parsed
+  /// files when the preprocessor indicates a new file is entered.
+  virtual void FileChanged(SourceLocation Loc, FileChangeReason Reason,
+                           SrcMgr::CharacteristicKind FileType,
+                           FileID PrevFID) {
+    if (const FileEntry *E = SM.getFileEntryForID(SM.getFileID(Loc)))
+      FilesList.insert(E);
+  }
+};
+} // End anonymous namespace.
+#endif
+
 // DiagnosticConsumer interface.
 
 void VerifyDiagnosticConsumer::BeginSourceFile(const LangOptions &LangOpts,
                                                const Preprocessor *PP) {
-  CurrentPreprocessor = PP;
-  if (PP) const_cast<Preprocessor*>(PP)->addCommentHandler(this);
+  // Attach comment handler on first invocation.
+  if (++ActiveSourceFiles == 1) {
+    if (PP) {
+      CurrentPreprocessor = PP;
+      const_cast<Preprocessor*>(PP)->addCommentHandler(this);
+#ifndef NDEBUG
+      VerifyFileTracker *V = new VerifyFileTracker(FilesParsedForDirectives,
+                                                   PP->getSourceManager());
+      const_cast<Preprocessor*>(PP)->addPPCallbacks(V);
+#endif
+    }
+  }
 
+  assert((!PP || CurrentPreprocessor == PP) && "Preprocessor changed!");
   PrimaryClient->BeginSourceFile(LangOpts, PP);
 }
 
 void VerifyDiagnosticConsumer::EndSourceFile() {
-  if (CurrentPreprocessor)
-    const_cast<Preprocessor*>(CurrentPreprocessor)->removeCommentHandler(this);
-  CheckDiagnostics();
-
+  assert(ActiveSourceFiles && "No active source files!");
   PrimaryClient->EndSourceFile();
 
-  CurrentPreprocessor = 0;
+  // Detach comment handler once last active source file completed.
+  if (--ActiveSourceFiles == 0) {
+    if (CurrentPreprocessor)
+      const_cast<Preprocessor*>(CurrentPreprocessor)->removeCommentHandler(this);
+
+    // Check diagnostics once last file completed.
+    CheckDiagnostics();
+    CurrentPreprocessor = 0;
+  }
 }
 
 void VerifyDiagnosticConsumer::HandleDiagnostic(
       DiagnosticsEngine::Level DiagLevel, const Diagnostic &Info) {
+#ifndef NDEBUG
   if (Info.hasSourceManager()) {
-    const SourceManager &SM = Info.getSourceManager();
-    FilesWithDiagnostics.insert(SM.getFileID(Info.getLocation()));
+    FileID FID = Info.getSourceManager().getFileID(Info.getLocation());
+    if (!FID.isInvalid())
+      FilesWithDiagnostics.insert(FID);
   }
+#endif
   // Send the diagnostic to the buffer, we will check it once we reach the end
   // of the source file (or are destructed).
   Buffer->HandleDiagnostic(DiagLevel, Info);
@@ -192,7 +238,7 @@
 /// diagnostics. If so, then put them in the appropriate directive list.
 ///
 /// Returns true if any valid directives were found.
-static bool ParseDirective(StringRef S, ExpectedData &ED, SourceManager &SM,
+static bool ParseDirective(StringRef S, ExpectedData *ED, SourceManager &SM,
                            SourceLocation Pos, DiagnosticsEngine &Diags) {
   // A single comment may contain multiple directives.
   bool FoundDirective = false;
@@ -210,15 +256,20 @@
     // Next token: { error | warning | note }
     DirectiveList* DL = NULL;
     if (PH.Next("error"))
-      DL = &ED.Errors;
+      DL = ED ? &ED->Errors : NULL;
     else if (PH.Next("warning"))
-      DL = &ED.Warnings;
+      DL = ED ? &ED->Warnings : NULL;
     else if (PH.Next("note"))
-      DL = &ED.Notes;
+      DL = ED ? &ED->Notes : NULL;
     else
       continue;
     PH.Advance();
 
+    // If a directive has been found but we're not interested
+    // in storing the directive information, return now.
+    if (!DL)
+      return true;
+
     // Default directive kind.
     bool RegexKind = false;
     const char* KindStr = "string";
@@ -360,9 +411,7 @@
   // Fold any "\<EOL>" sequences
   size_t loc = C.find('\\');
   if (loc == StringRef::npos) {
-    if (ParseDirective(C, ED, SM, CommentBegin, PP.getDiagnostics()))
-      if (const FileEntry *E = SM.getFileEntryForID(SM.getFileID(CommentBegin)))
-        FilesWithDirectives.insert(E);
+    ParseDirective(C, &ED, SM, CommentBegin, PP.getDiagnostics());
     return false;
   }
 
@@ -392,19 +441,20 @@
   }
 
   if (!C2.empty())
-    if (ParseDirective(C2, ED, SM, CommentBegin, PP.getDiagnostics()))
-      if (const FileEntry *E = SM.getFileEntryForID(SM.getFileID(CommentBegin)))
-        FilesWithDirectives.insert(E);
+    ParseDirective(C2, &ED, SM, CommentBegin, PP.getDiagnostics());
   return false;
 }
 
-/// FindExpectedDiags - Lex the main source file to find all of the
-//   expected errors and warnings.
-static void FindExpectedDiags(const Preprocessor &PP, ExpectedData &ED,
-                              FileID FID) {
+#ifndef NDEBUG
+/// \brief Lex the specified source file to determine whether it contains
+/// any expected-* directives.  As a Lexer is used rather than a full-blown
+/// Preprocessor, directives inside skipped #if blocks will still be found.
+///
+/// \return true if any directives were found.
+static bool findDirectives(const Preprocessor &PP, FileID FID) {
   // Create a raw lexer to pull all the comments out of FID.
   if (FID.isInvalid())
-    return;
+    return false;
 
   SourceManager& SM = PP.getSourceManager();
   // Create a lexer to lex all the tokens of the main file in raw mode.
@@ -416,6 +466,7 @@
 
   Token Tok;
   Tok.setKind(tok::comment);
+  bool Found = false;
   while (Tok.isNot(tok::eof)) {
     RawLex.Lex(Tok);
     if (!Tok.is(tok::comment)) continue;
@@ -424,9 +475,12 @@
     if (Comment.empty()) continue;
 
     // Find all expected errors/warnings/notes.
-    ParseDirective(Comment, ED, SM, Tok.getLocation(), PP.getDiagnostics());
-  };
+    Found |= ParseDirective(Comment, 0, SM, Tok.getLocation(),
+                            PP.getDiagnostics());
+  }
+  return Found;
 }
+#endif // !NDEBUG
 
 /// \brief Takes a list of diagnostics that have been generated but not matched
 /// by an expected-* directive and produces a diagnostic to the user from this.
@@ -556,22 +610,28 @@
   // markers. If not then any diagnostics are unexpected.
   if (CurrentPreprocessor) {
     SourceManager &SM = CurrentPreprocessor->getSourceManager();
-    // Only check for expectations in other diagnostic locations not
-    // captured during normal parsing.
-    // FIXME: This check is currently necessary while synthesized files may
-    // not have their expected-* directives captured during parsing.  These
-    // cases should be fixed and the following loop replaced with one which
-    // checks only during a debug build and asserts on a mismatch.
+
+#ifndef NDEBUG
+    // In a debug build, scan through any files that may have been missed
+    // during parsing and issue a fatal error if directives are contained
+    // within these files.  If a fatal error occurs, this suggests that
+    // this file is being parsed separately from the main file.
+    HeaderSearch &HS = CurrentPreprocessor->getHeaderSearchInfo();
     for (FilesWithDiagnosticsSet::iterator I = FilesWithDiagnostics.begin(),
                                          End = FilesWithDiagnostics.end();
             I != End; ++I) {
       const FileEntry *E = SM.getFileEntryForID(*I);
-      if (!E || !FilesWithDirectives.count(E)) {
-        if (E)
-          FilesWithDirectives.insert(E);
-        FindExpectedDiags(*CurrentPreprocessor, ED, *I);
-      }
+      // Don't check files already parsed or those handled as modules.
+      if (E && (FilesParsedForDirectives.count(E)
+                  || HS.findModuleForHeader(E)))
+        continue;
+
+      if (findDirectives(*CurrentPreprocessor, *I))
+        llvm::report_fatal_error(Twine("-verify directives found after rather"
+                                       " than during normal parsing of ",
+                                 StringRef(E ? E->getName() : "(unknown)")));
     }
+#endif
 
     // Check that the expected diagnostics occurred.
     NumErrors += CheckResults(Diags, SM, *Buffer, ED);
diff --git a/test/ARCMT/verify.m b/test/ARCMT/verify.m
new file mode 100644
index 0000000..9110fe6
--- /dev/null
+++ b/test/ARCMT/verify.m
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -arcmt-check -verify %s
+// RUN: %clang_cc1 -arcmt-check -verify %t.invalid 2>&1 | FileCheck %s
+
+#if 0
+// expected-error {{should be ignored}}
+#endif
+
+#error should not be ignored
+// expected-error@-1 {{should not be ignored}}
+
+//      CHECK: error: 'error' diagnostics seen but not expected:
+// CHECK-NEXT:   (frontend): error reading '{{.*}}verify.m.tmp.invalid'
+// CHECK-NEXT: 1 error generated.
diff --git a/test/ASTMerge/function.c b/test/ASTMerge/function.c
index f97ecee..320bca2 100644
--- a/test/ASTMerge/function.c
+++ b/test/ASTMerge/function.c
@@ -1,9 +1,15 @@
 // RUN: %clang_cc1 -emit-pch -o %t.1.ast %S/Inputs/function1.c
 // RUN: %clang_cc1 -emit-pch -o %t.2.ast %S/Inputs/function2.c
 // RUN: %clang_cc1 -ast-merge %t.1.ast -ast-merge %t.2.ast -fsyntax-only %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -ast-merge %t.1.ast -ast-merge %t.2.ast -fsyntax-only -verify %s
 
 // CHECK: function2.c:3:6: error: external function 'f1' declared with incompatible types in different translation units ('void (Int, double)' vs. 'void (int, float)')
 // CHECK: function1.c:2:6: note: declared here with type 'void (int, float)'
 // CHECK: function2.c:5:6: error: external function 'f3' declared with incompatible types in different translation units ('void (int)' vs. 'void (void)')
 // CHECK: function1.c:4:6: note: declared here with type 'void (void)'
 // CHECK: 2 errors generated
+
+// expected-error@3 {{external function 'f1' declared with incompatible types}}
+// expected-note@2 {{declared here}}
+// expected-error@5 {{external function 'f3' declared with incompatible types}}
+// expected-note@4 {{declared here}}
diff --git a/test/Frontend/verify.c b/test/Frontend/verify.c
index 59a441d..f8d0f42 100644
--- a/test/Frontend/verify.c
+++ b/test/Frontend/verify.c
@@ -108,3 +108,18 @@
 // CHECK5-NEXT: 2 errors generated.
 #endif
 
+#if 0
+// RUN: %clang_cc1 -verify %t.invalid 2>&1 | FileCheck -check-prefix=CHECK6 %s
+
+//      CHECK6: error: 'error' diagnostics seen but not expected:
+// CHECK6-NEXT:   (frontend): error reading '{{.*}}verify.c.tmp.invalid'
+// CHECK6-NEXT: 1 error generated.
+
+// RUN: echo -e '//expected-error@2{{1}}\n#error 2' | %clang_cc1 -verify 2>&1 | FileCheck -check-prefix=CHECK7 %s
+
+//      CHECK7: error: 'error' diagnostics expected but not seen:
+// CHECK7-NEXT:   Line 2 (directive at <stdin>:1): 1
+// CHECK7-NEXT: error: 'error' diagnostics seen but not expected:
+// CHECK7-NEXT:   Line 2: 2
+// CHECK7-NEXT: 2 errors generated.
+#endif
diff --git a/test/Frontend/verify2.c b/test/Frontend/verify2.c
new file mode 100644
index 0000000..a1c7975
--- /dev/null
+++ b/test/Frontend/verify2.c
@@ -0,0 +1,19 @@
+#if 0
+// RUN: %clang_cc1 -verify %s 2>&1 | FileCheck %s
+
+// Please note that all comments are inside "#if 0" blocks so that
+// VerifyDiagnosticConsumer sees no comments while processing this
+// test-case.
+#endif
+
+#include "verify2.h"
+#error source
+
+#if 0
+// expected-error {{should be ignored}}
+
+//      CHECK: error: 'error' diagnostics seen but not expected:
+// CHECK-NEXT:   Line 1: header
+// CHECK-NEXT:   Line 10: source
+// CHECK-NEXT: 2 errors generated.
+#endif
diff --git a/test/Frontend/verify2.h b/test/Frontend/verify2.h
new file mode 100644
index 0000000..8acbf6e
--- /dev/null
+++ b/test/Frontend/verify2.h
@@ -0,0 +1,5 @@
+#error header
+
+#if 0
+// expected-error {{should be ignored}}
+#endif
diff --git a/test/Modules/Inputs/category_right.h b/test/Modules/Inputs/category_right.h
index d993b50..812a840 100644
--- a/test/Modules/Inputs/category_right.h
+++ b/test/Modules/Inputs/category_right.h
@@ -8,5 +8,5 @@
 -(void)right2;
 @end
 
-@interface Foo(Duplicate) // expected-warning {{duplicate definition of category}}
+@interface Foo(Duplicate)
 @end
diff --git a/test/Modules/lookup.cpp b/test/Modules/lookup.cpp
index 9839035..70d2107 100644
--- a/test/Modules/lookup.cpp
+++ b/test/Modules/lookup.cpp
@@ -5,6 +5,8 @@
 #define IMPORT(X) @__experimental_modules_import X
 IMPORT(lookup_right_cxx);
 
+// in lookup_left.hpp: expected-warning@3 {{weak identifier 'weak_identifier' never declared}}
+
 void test(int i, float f) {
   // unqualified lookup
   f0(&i);
diff --git a/test/Modules/objc-categories.m b/test/Modules/objc-categories.m
index 340f279..b267592 100644
--- a/test/Modules/objc-categories.m
+++ b/test/Modules/objc-categories.m
@@ -12,6 +12,7 @@
 
 
 // in category_left.h: expected-note {{previous definition}}
+// in category_right.h: expected-warning@11 {{duplicate definition of category}}
 
 @interface Foo(Source)
 -(void)source;