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

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;

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);
 

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;

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)

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();
 }
 

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

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);

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);
 }
 

test/Modules/Inputs/ModuleDiags/has_errors.h

@@ -0,0 +1,2 @@
+static void foo(void) { }
+static void foo(void) { }

test/Modules/Inputs/ModuleDiags/has_warnings.h

@@ -0,0 +1,3 @@
+
+int int_val;
+float *float_ptr = &int_val;

test/Modules/Inputs/ModuleDiags/module.map

@@ -0,0 +1,7 @@
+module HasWarnings {
+  header "has_warnings.h"
+}
+
+module HasErrors {
+  header "has_errors.h"
+}

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
+
 

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
+