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.

llvm-svn: 161650

clang/include/clang/Frontend/VerifyDiagnosticConsumer.h

@@ -166,17 +166,22 @@ public:
     }
   };
 
-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 @@ public:
 
   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,

clang/lib/ARCMigrate/ARCMT.cpp

@@ -91,11 +91,40 @@ namespace {
 
 class CaptureDiagnosticConsumer : public DiagnosticConsumer {
   DiagnosticsEngine &Diags;
+  DiagnosticConsumer &DiagClient;
   CapturedDiagList &CapturedDiags;
+  bool HasBegunSourceFile;
 public:
   CaptureDiagnosticConsumer(DiagnosticsEngine &diags,
-                           CapturedDiagList &capturedDiags)
+                            DiagnosticConsumer &client,
-    : Diags(diags), CapturedDiags(capturedDiags) { }
+                            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 @@ bool arcmt::checkForManualIssues(CompilerInvocation &origCI,
       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 @@ bool arcmt::checkForManualIssues(CompilerInvocation &origCI,
     DiagClient->BeginSourceFile(Ctx.getLangOpts(), &Unit->getPreprocessor());
     capturedDiags.reportDiagnostics(*Diags);
     DiagClient->EndSourceFile();
+    errRec.FinishCapture();
     return true;
   }
 
@@ -315,6 +347,7 @@ bool arcmt::checkForManualIssues(CompilerInvocation &origCI,
   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 @@ bool MigrationProcess::applyTransform(TransformFn trans,
       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 @@ bool MigrationProcess::applyTransform(TransformFn trans,
   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 @@ bool MigrationProcess::applyTransform(TransformFn trans,
     DiagClient->BeginSourceFile(Ctx.getLangOpts(), &Unit->getPreprocessor());
     capturedDiags.reportDiagnostics(*Diags);
     DiagClient->EndSourceFile();
+    errRec.FinishCapture();
     return true;
   }
 
@@ -609,6 +645,7 @@ bool MigrationProcess::applyTransform(TransformFn trans,
   }
 
   DiagClient->EndSourceFile();
+  errRec.FinishCapture();
 
   if (DiagClient->getNumErrors())
     return true;

clang/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::DirectiveList DirectiveList;
 typedef VerifyDiagnosticConsumer::ExpectedData ExpectedData;
 
 VerifyDiagnosticConsumer::VerifyDiagnosticConsumer(DiagnosticsEngine &_Diags)
-  : Diags(_Diags), PrimaryClient(Diags.getClient()),
+  : Diags(_Diags),
-    OwnsPrimaryClient(Diags.ownsClient()),
+    PrimaryClient(Diags.getClient()), OwnsPrimaryClient(Diags.ownsClient()),
-    Buffer(new TextDiagnosticBuffer()), CurrentPreprocessor(0) 
+    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;
+  // Attach comment handler on first invocation.
-  if (PP) const_cast<Preprocessor*>(PP)->addCommentHandler(this);
+  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)
+  assert(ActiveSourceFiles && "No active source files!");
-    const_cast<Preprocessor*>(CurrentPreprocessor)->removeCommentHandler(this);
-  CheckDiagnostics();
-
   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();
+    FileID FID = Info.getSourceManager().getFileID(Info.getLocation());
-    FilesWithDiagnostics.insert(SM.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 @@ private:
 /// 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 @@ static bool ParseDirective(StringRef S, ExpectedData &ED, SourceManager &SM,
     // 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 @@ bool VerifyDiagnosticConsumer::HandleComment(Preprocessor &PP,
   // Fold any "\<EOL>" sequences
   size_t loc = C.find('\\');
   if (loc == StringRef::npos) {
-    if (ParseDirective(C, ED, SM, CommentBegin, PP.getDiagnostics()))
+    ParseDirective(C, &ED, SM, CommentBegin, PP.getDiagnostics());
-      if (const FileEntry *E = SM.getFileEntryForID(SM.getFileID(CommentBegin)))
-        FilesWithDirectives.insert(E);
     return false;
   }
 
@@ -392,19 +441,20 @@ bool VerifyDiagnosticConsumer::HandleComment(Preprocessor &PP,
   }
 
   if (!C2.empty())
-    if (ParseDirective(C2, ED, SM, CommentBegin, PP.getDiagnostics()))
+    ParseDirective(C2, &ED, SM, CommentBegin, PP.getDiagnostics());
-      if (const FileEntry *E = SM.getFileEntryForID(SM.getFileID(CommentBegin)))
-        FilesWithDirectives.insert(E);
   return false;
 }
 
-/// FindExpectedDiags - Lex the main source file to find all of the
+#ifndef NDEBUG
-//   expected errors and warnings.
+/// \brief Lex the specified source file to determine whether it contains
-static void FindExpectedDiags(const Preprocessor &PP, ExpectedData &ED,
+/// any expected-* directives.  As a Lexer is used rather than a full-blown
-                              FileID FID) {
+/// 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 @@ static void FindExpectedDiags(const Preprocessor &PP, ExpectedData &ED,
 
   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 @@ static void FindExpectedDiags(const Preprocessor &PP, ExpectedData &ED,
     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 @@ void VerifyDiagnosticConsumer::CheckDiagnostics() {
   // 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.
+#ifndef NDEBUG
-    // FIXME: This check is currently necessary while synthesized files may
+    // In a debug build, scan through any files that may have been missed
-    // not have their expected-* directives captured during parsing.  These
+    // during parsing and issue a fatal error if directives are contained
-    // cases should be fixed and the following loop replaced with one which
+    // within these files.  If a fatal error occurs, this suggests that
-    // checks only during a debug build and asserts on a mismatch.
+    // 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)) {
+      // Don't check files already parsed or those handled as modules.
-        if (E)
+      if (E && (FilesParsedForDirectives.count(E)
-          FilesWithDirectives.insert(E);
+                  || HS.findModuleForHeader(E)))
-        FindExpectedDiags(*CurrentPreprocessor, ED, *I);
+        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);

clang/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.

clang/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}}

clang/test/Frontend/verify.c

@@ -108,3 +108,18 @@ unexpected b; // expected-error@33 1-1 {{unknown type}}
 // 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

clang/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

clang/test/Frontend/verify2.h

@@ -0,0 +1,5 @@
+#error header
+
+#if 0
+// expected-error {{should be ignored}}
+#endif

clang/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

clang/test/Modules/lookup.cpp

@@ -5,6 +5,8 @@ import lookup_left_cxx;
 #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);

clang/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;