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



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

13 files changed, 136 insertions, 15 deletions

diff --git a/include/clang/Basic/Diagnostic.h b/include/clang/Basic/Diagnostic.h
index 032763672b..2d9d288964 100644
--- a/include/clang/Basic/Diagnostic.h
+++ b/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/include/clang/Lex/ModuleMap.h b/include/clang/Lex/ModuleMap.h
index 33c92f59a4..dc75f1803c 100644
--- a/include/clang/Lex/ModuleMap.h
+++ b/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/lib/Basic/Diagnostic.cpp b/lib/Basic/Diagnostic.cpp
index 4a5b191aab..5eff86c2b8 100644
--- a/lib/Basic/Diagnostic.cpp
+++ b/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/lib/Frontend/ASTMerge.cpp b/lib/Frontend/ASTMerge.cpp
index bfb30836d8..b6c644eba7 100644
--- a/lib/Frontend/ASTMerge.cpp
+++ b/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/lib/Frontend/ASTUnit.cpp b/lib/Frontend/ASTUnit.cpp
index c1115aedbf..1b4e5a0b39 100644
--- a/lib/Frontend/ASTUnit.cpp
+++ b/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/lib/Frontend/CompilerInstance.cpp b/lib/Frontend/CompilerInstance.cpp
index df06a816e8..027e99a29d 100644
--- a/lib/Frontend/CompilerInstance.cpp
+++ b/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/lib/Frontend/VerifyDiagnosticConsumer.cpp b/lib/Frontend/VerifyDiagnosticConsumer.cpp
index 4f05aef94f..91aa52b771 100644
--- a/lib/Frontend/VerifyDiagnosticConsumer.cpp
+++ b/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/lib/Lex/ModuleMap.cpp b/lib/Lex/ModuleMap.cpp
index 1458a73a7d..3e7a44c0e3 100644
--- a/lib/Lex/ModuleMap.cpp
+++ b/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/test/Modules/Inputs/ModuleDiags/has_errors.h b/test/Modules/Inputs/ModuleDiags/has_errors.h
new file mode 100644
index 0000000000..2c0929a6f5
--- /dev/null
+++ b/test/Modules/Inputs/ModuleDiags/has_errors.h
@@ -0,0 +1,2 @@
+static void foo(void) { }
+static void foo(void) { }
diff --git a/test/Modules/Inputs/ModuleDiags/has_warnings.h b/test/Modules/Inputs/ModuleDiags/has_warnings.h
new file mode 100644
index 0000000000..87112be695
--- /dev/null
+++ b/test/Modules/Inputs/ModuleDiags/has_warnings.h
@@ -0,0 +1,3 @@
+
+int int_val;
+float *float_ptr = &int_val;
diff --git a/test/Modules/Inputs/ModuleDiags/module.map b/test/Modules/Inputs/ModuleDiags/module.map
new file mode 100644
index 0000000000..09b25088e6
--- /dev/null
+++ b/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/test/Modules/cycles.c b/test/Modules/cycles.c
index 4326e76a75..5f83092c95 100644
--- a/test/Modules/cycles.c
+++ b/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/test/Modules/serialized-diags.m b/test/Modules/serialized-diags.m
new file mode 100644
index 0000000000..99ba80a780
--- /dev/null
+++ b/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
+