When building a module, forward diagnostics to the outer diagnostic consumer.

Previously, we would clone the current diagnostic consumer to produce
a new diagnostic consumer to use when building a module. The problem
here is that we end up losing diagnostics for important diagnostic
consumers, such as serialized diagnostics (where we'd end up with two
diagnostic consumers writing the same output file). With forwarding,
the diagnostics from all of the different modules being built get
forwarded to the one serialized-diagnostic consumer and are emitted in
a sane way.

Fixes <rdar://problem/13663996>.

llvm-svn: 181067

13 files changed, 136 insertions, 15 deletions

diff --git a/clang/include/clang/Basic/Diagnostic.h b/clang/include/clang/Basic/Diagnostic.h
index 032763672b4..2d9d288964d 100644
--- a/clang/include/clang/Basic/Diagnostic.h
+++ b/clang/include/clang/Basic/Diagnostic.h
@@ -1319,6 +1319,26 @@ class IgnoringDiagConsumer : public DiagnosticConsumer {
   }
 };
 
+/// \brief Diagnostic consumer that forwards diagnostics along to an
+/// existing, already-initialized diagnostic consumer.
+///
+class ForwardingDiagnosticConsumer : public DiagnosticConsumer {
+  DiagnosticConsumer &Target;
+
+public:
+  ForwardingDiagnosticConsumer(DiagnosticConsumer &Target) : Target(Target) {}
+
+  virtual ~ForwardingDiagnosticConsumer();
+
+  virtual void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
+                                const Diagnostic &Info);
+  virtual void clear();
+
+  virtual bool IncludeInDiagnosticCounts() const;
+
+  virtual DiagnosticConsumer *clone(DiagnosticsEngine &Diags) const;
+};
+
 // Struct used for sending info about how a type should be printed.
 struct TemplateDiffTypes {
   intptr_t FromType;
diff --git a/clang/include/clang/Lex/ModuleMap.h b/clang/include/clang/Lex/ModuleMap.h
index 33c92f59a44..dc75f1803cb 100644
--- a/clang/include/clang/Lex/ModuleMap.h
+++ b/clang/include/clang/Lex/ModuleMap.h
@@ -161,7 +161,7 @@ public:
   /// \param LangOpts Language options for this translation unit.
   ///
   /// \param Target The target for this translation unit.
-  ModuleMap(FileManager &FileMgr, const DiagnosticConsumer &DC,
+  ModuleMap(FileManager &FileMgr, DiagnosticConsumer &DC,
             const LangOptions &LangOpts, const TargetInfo *Target,
             HeaderSearch &HeaderInfo);
 
diff --git a/clang/lib/Basic/Diagnostic.cpp b/clang/lib/Basic/Diagnostic.cpp
index 4a5b191aab7..5eff86c2b88 100644
--- a/clang/lib/Basic/Diagnostic.cpp
+++ b/clang/lib/Basic/Diagnostic.cpp
@@ -972,6 +972,28 @@ bool DiagnosticConsumer::IncludeInDiagnosticCounts() const { return true; }
 
 void IgnoringDiagConsumer::anchor() { }
 
+ForwardingDiagnosticConsumer::~ForwardingDiagnosticConsumer() {}
+
+void ForwardingDiagnosticConsumer::HandleDiagnostic(
+       DiagnosticsEngine::Level DiagLevel,
+       const Diagnostic &Info) {
+  Target.HandleDiagnostic(DiagLevel, Info);
+}
+
+void ForwardingDiagnosticConsumer::clear() {
+  DiagnosticConsumer::clear();
+  Target.clear();
+}
+
+bool ForwardingDiagnosticConsumer::IncludeInDiagnosticCounts() const {
+  return Target.IncludeInDiagnosticCounts();
+}
+
+DiagnosticConsumer *
+ForwardingDiagnosticConsumer::clone(DiagnosticsEngine &Diags) const {
+  return new ForwardingDiagnosticConsumer(Target);
+}
+
 PartialDiagnostic::StorageAllocator::StorageAllocator() {
   for (unsigned I = 0; I != NumCached; ++I)
     FreeList[I] = Cached + I;
diff --git a/clang/lib/Frontend/ASTMerge.cpp b/clang/lib/Frontend/ASTMerge.cpp
index bfb30836d81..b6c644eba78 100644
--- a/clang/lib/Frontend/ASTMerge.cpp
+++ b/clang/lib/Frontend/ASTMerge.cpp
@@ -34,7 +34,7 @@ bool ASTMergeAction::BeginSourceFileAction(CompilerInstance &CI,
 void ASTMergeAction::ExecuteAction() {
   CompilerInstance &CI = getCompilerInstance();
   CI.getDiagnostics().getClient()->BeginSourceFile(
-                                         CI.getASTContext().getLangOpts());
+                                             CI.getASTContext().getLangOpts());
   CI.getDiagnostics().SetArgToStringFn(&FormatASTNodeDiagnosticArgument,
                                        &CI.getASTContext());
   IntrusiveRefCntPtr<DiagnosticIDs>
@@ -42,8 +42,9 @@ void ASTMergeAction::ExecuteAction() {
   for (unsigned I = 0, N = ASTFiles.size(); I != N; ++I) {
     IntrusiveRefCntPtr<DiagnosticsEngine>
         Diags(new DiagnosticsEngine(DiagIDs, &CI.getDiagnosticOpts(),
-                                    CI.getDiagnostics().getClient(),
-                                    /*ShouldOwnClient=*/false));
+                                    new ForwardingDiagnosticConsumer(
+                                          *CI.getDiagnostics().getClient()),
+                                    /*ShouldOwnClient=*/true));
     ASTUnit *Unit = ASTUnit::LoadFromASTFile(ASTFiles[I], Diags,
                                              CI.getFileSystemOpts(), false);
     if (!Unit)
diff --git a/clang/lib/Frontend/ASTUnit.cpp b/clang/lib/Frontend/ASTUnit.cpp
index c1115aedbf8..1b4e5a0b39a 100644
--- a/clang/lib/Frontend/ASTUnit.cpp
+++ b/clang/lib/Frontend/ASTUnit.cpp
@@ -236,6 +236,11 @@ ASTUnit::ASTUnit(bool _MainFileIsAST)
 }
 
 ASTUnit::~ASTUnit() {
+  // If we loaded from an AST file, balance out the BeginSourceFile call.
+  if (MainFileIsAST && getDiagnostics().getClient()) {
+    getDiagnostics().getClient()->EndSourceFile();
+  }
+
   clearFileLevelDecls();
 
   // Clean up the temporary files and the preamble file.
@@ -581,14 +586,22 @@ private:
   }
 };
 
+  /// \brief Diagnostic consumer that saves each diagnostic it is given.
 class StoredDiagnosticConsumer : public DiagnosticConsumer {
   SmallVectorImpl<StoredDiagnostic> &StoredDiags;
-  
+  SourceManager *SourceMgr;
+
 public:
   explicit StoredDiagnosticConsumer(
                           SmallVectorImpl<StoredDiagnostic> &StoredDiags)
-    : StoredDiags(StoredDiags) { }
-  
+    : StoredDiags(StoredDiags), SourceMgr(0) { }
+
+  virtual void BeginSourceFile(const LangOptions &LangOpts,
+                               const Preprocessor *PP = 0) {
+    if (PP)
+      SourceMgr = &PP->getSourceManager();
+  }
+
   virtual void HandleDiagnostic(DiagnosticsEngine::Level Level,
                                 const Diagnostic &Info);
   
@@ -635,7 +648,11 @@ void StoredDiagnosticConsumer::HandleDiagnostic(DiagnosticsEngine::Level Level,
   // Default implementation (Warnings/errors count).
   DiagnosticConsumer::HandleDiagnostic(Level, Info);
 
-  StoredDiags.push_back(StoredDiagnostic(Level, Info));
+  // Only record the diagnostic if it's part of the source manager we know
+  // about. This effectively drops diagnostics from modules we're building.
+  // FIXME: In the long run, ee don't want to drop source managers from modules.
+  if (!Info.hasSourceManager() || &Info.getSourceManager() == SourceMgr)
+    StoredDiags.push_back(StoredDiagnostic(Level, Info));
 }
 
 ASTDeserializationListener *ASTUnit::getDeserializationListener() {
@@ -835,6 +852,9 @@ ASTUnit *ASTUnit::LoadFromASTFile(const std::string &Filename,
   ReaderPtr->InitializeSema(*AST->TheSema);
   AST->Reader = ReaderPtr;
 
+  // Tell the diagnostic client that we have started a source file.
+  AST->getDiagnostics().getClient()->BeginSourceFile(Context.getLangOpts(),&PP);
+
   return AST.take();
 }
 
diff --git a/clang/lib/Frontend/CompilerInstance.cpp b/clang/lib/Frontend/CompilerInstance.cpp
index df06a816e84..027e99a29d4 100644
--- a/clang/lib/Frontend/CompilerInstance.cpp
+++ b/clang/lib/Frontend/CompilerInstance.cpp
@@ -868,9 +868,11 @@ static void compileModule(CompilerInstance &ImportingInstance,
   // module.
   CompilerInstance Instance;
   Instance.setInvocation(&*Invocation);
-  Instance.createDiagnostics(&ImportingInstance.getDiagnosticClient(),
+
+  Instance.createDiagnostics(new ForwardingDiagnosticConsumer(
+                                   ImportingInstance.getDiagnosticClient()),
                              /*ShouldOwnClient=*/true,
-                             /*ShouldCloneClient=*/true);
+                             /*ShouldCloneClient=*/false);
 
   // Note that this module is part of the module build stack, so that we
   // can detect cycles in the module graph.
@@ -892,6 +894,7 @@ static void compileModule(CompilerInstance &ImportingInstance,
   llvm::CrashRecoveryContext CRC;
   CompileModuleMapData Data = { Instance, CreateModuleAction };
   CRC.RunSafelyOnThread(&doCompileMapModule, &Data, ThreadStackSize);
+
   
   // Delete the temporary module map file.
   // FIXME: Even though we're executing under crash protection, it would still
diff --git a/clang/lib/Frontend/VerifyDiagnosticConsumer.cpp b/clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
index 4f05aef94f3..91aa52b771b 100644
--- a/clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
+++ b/clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
@@ -111,8 +111,13 @@ void VerifyDiagnosticConsumer::EndSourceFile() {
 
 void VerifyDiagnosticConsumer::HandleDiagnostic(
       DiagnosticsEngine::Level DiagLevel, const Diagnostic &Info) {
-  if (Info.hasSourceManager())
+  if (Info.hasSourceManager()) {
+    // If this diagnostic is for a different source manager, ignore it.
+    if (SrcManager && &Info.getSourceManager() != SrcManager)
+      return;
+
     setSourceManager(Info.getSourceManager());
+  }
 
 #ifndef NDEBUG
   // Debug build tracks unparsed files for possible
@@ -476,6 +481,11 @@ static bool ParseDirective(StringRef S, ExpectedData *ED, SourceManager &SM,
 bool VerifyDiagnosticConsumer::HandleComment(Preprocessor &PP,
                                              SourceRange Comment) {
   SourceManager &SM = PP.getSourceManager();
+
+  // If this comment is for a different source manager, ignore it.
+  if (SrcManager && &SM != SrcManager)
+    return false;
+
   SourceLocation CommentBegin = Comment.getBegin();
 
   const char *CommentRaw = SM.getCharacterData(CommentBegin);
diff --git a/clang/lib/Lex/ModuleMap.cpp b/clang/lib/Lex/ModuleMap.cpp
index 1458a73a7d3..3e7a44c0e35 100644
--- a/clang/lib/Lex/ModuleMap.cpp
+++ b/clang/lib/Lex/ModuleMap.cpp
@@ -83,7 +83,7 @@ Module *ModuleMap::resolveModuleId(const ModuleId &Id, Module *Mod,
   return Context;
 }
 
-ModuleMap::ModuleMap(FileManager &FileMgr, const DiagnosticConsumer &DC,
+ModuleMap::ModuleMap(FileManager &FileMgr, DiagnosticConsumer &DC,
                      const LangOptions &LangOpts, const TargetInfo *Target,
                      HeaderSearch &HeaderInfo)
   : LangOpts(LangOpts), Target(Target), HeaderInfo(HeaderInfo),
@@ -92,7 +92,8 @@ ModuleMap::ModuleMap(FileManager &FileMgr, const DiagnosticConsumer &DC,
   IntrusiveRefCntPtr<DiagnosticIDs> DiagIDs(new DiagnosticIDs);
   Diags = IntrusiveRefCntPtr<DiagnosticsEngine>(
             new DiagnosticsEngine(DiagIDs, new DiagnosticOptions));
-  Diags->setClient(DC.clone(*Diags), /*ShouldOwnClient=*/true);
+  Diags->setClient(new ForwardingDiagnosticConsumer(DC),
+                   /*ShouldOwnClient=*/true);
   SourceMgr = new SourceManager(*Diags, FileMgr);
 }
 
diff --git a/clang/test/Modules/Inputs/ModuleDiags/has_errors.h b/clang/test/Modules/Inputs/ModuleDiags/has_errors.h
new file mode 100644
index 00000000000..2c0929a6f58
--- /dev/null
+++ b/clang/test/Modules/Inputs/ModuleDiags/has_errors.h
@@ -0,0 +1,2 @@
+static void foo(void) { }
+static void foo(void) { }
diff --git a/clang/test/Modules/Inputs/ModuleDiags/has_warnings.h b/clang/test/Modules/Inputs/ModuleDiags/has_warnings.h
new file mode 100644
index 00000000000..87112be6952
--- /dev/null
+++ b/clang/test/Modules/Inputs/ModuleDiags/has_warnings.h
@@ -0,0 +1,3 @@
+
+int int_val;
+float *float_ptr = &int_val;
diff --git a/clang/test/Modules/Inputs/ModuleDiags/module.map b/clang/test/Modules/Inputs/ModuleDiags/module.map
new file mode 100644
index 00000000000..09b25088e67
--- /dev/null
+++ b/clang/test/Modules/Inputs/ModuleDiags/module.map
@@ -0,0 +1,7 @@
+module HasWarnings {
+  header "has_warnings.h"
+}
+
+module HasErrors {
+  header "has_errors.h"
+}
diff --git a/clang/test/Modules/cycles.c b/clang/test/Modules/cycles.c
index 4326e76a75f..5f83092c95f 100644
--- a/clang/test/Modules/cycles.c
+++ b/clang/test/Modules/cycles.c
@@ -6,8 +6,8 @@
 // CHECK: While building module 'MutuallyRecursive1' imported from
 // CHECK: While building module 'MutuallyRecursive2' imported from
 // CHECK: MutuallyRecursive2.h:3:9: fatal error: cyclic dependency in module 'MutuallyRecursive1': MutuallyRecursive1 -> MutuallyRecursive2 -> MutuallyRecursive1
-// CHECK: While building module 'MutuallyRecursive1' imported from
 // CHECK: MutuallyRecursive1.h:2:9: fatal error: could not build module 'MutuallyRecursive2'
 // CHECK: cycles.c:4:9: fatal error: could not build module 'MutuallyRecursive1'
-// CHECK-NOT: error:
+// CHECK: 3 errors generated
+
 
diff --git a/clang/test/Modules/serialized-diags.m b/clang/test/Modules/serialized-diags.m
new file mode 100644
index 00000000000..99ba80a7800
--- /dev/null
+++ b/clang/test/Modules/serialized-diags.m
@@ -0,0 +1,32 @@
+@import HasWarnings;
+
+#ifdef WITH_ERRORS
+@import HasErrors;
+#endif
+
+float float_val;
+double *double_ptr = &float_val;
+
+// RUN: rm -rf %t %t.diag %t.out
+// RUN: %clang -fmodules -fmodules-cache-path=%t/ModuleCache -I %S/Inputs/ModuleDiags -fsyntax-only %s --serialize-diagnostics %t.diag > /dev/null 2>&1
+// RUN: c-index-test -read-diagnostics %t.diag > %t.out 2>&1
+// RUN: FileCheck --input-file=%t.out %s
+
+// CHECK: Inputs/ModuleDiags/has_warnings.h:3:8: warning: incompatible pointer types initializing 'float *'
+// CHECK: serialized-diags.m:1:9: note: while building module 'HasWarnings' imported from
+// CHECK: serialized-diags.m:8:9: warning: incompatible pointer types initializing 'double *'
+// CHECK: Number of diagnostics: 2
+
+// RUN: rm -rf %t %t.diag_errors %t.out_errors
+// RUN: not %clang -fmodules -fmodules-cache-path=%t/ModuleCache -I %S/Inputs/ModuleDiags -fsyntax-only -DWITH_ERRORS %s --serialize-diagnostics %t.diag_errors > /dev/null 2>&1
+// RUN: c-index-test -read-diagnostics %t.diag_errors > %t.out_errors 2>&1
+// RUN: FileCheck -check-prefix=CHECK-WITH-ERRORS --input-file=%t.out_errors %s
+
+// CHECK-WITH-ERRORS: has_warnings.h:3:8: warning: incompatible pointer types initializing 'float *'
+// CHECK-WITH-ERRORS: serialized-diags.m:1:9: note: while building module 'HasWarnings'
+// CHECK-WITH-ERRORS: has_errors.h:2:13: error: redefinition of 'foo'
+// CHECK-WITH-ERRORS: serialized-diags.m:4:9: note: while building module 'HasErrors'
+// CHECK-WITH-ERRORS: has_errors.h:1:13: note: previous definition is here
+// CHECK-WITH-ERRORS: serialized-diags.m:4:9: fatal: could not build module 'HasErrors'
+// CHECK-WITH-ERRORS: Number of diagnostics: 3
+