Improve ASTUnit's capture of diagnostics so that the

diagnostic-capturing client lives as long as the ASTUnit itself
does. Otherwise, we can end up with crashes when we get a diagnostic
outside of parsing/code completion. The circumstances under which this
happen are really hard to reproduce, because a file needs to change
from under us.

llvm-svn: 118751

clang/include/clang/Frontend/ASTUnit.h

@@ -218,6 +218,9 @@ private:
   /// \brief Whether we should be caching code-completion results.
   bool ShouldCacheCodeCompletionResults;
   
+  static void ConfigureDiags(llvm::IntrusiveRefCntPtr<Diagnostic> &Diags,
+                             ASTUnit &AST, bool CaptureDiagnostics);
+
 public:
   /// \brief A cached code-completion result, which may be introduced in one of
   /// many different contexts.
@@ -540,9 +543,9 @@ public:
                                     llvm::IntrusiveRefCntPtr<Diagnostic> Diags,
                                       llvm::StringRef ResourceFilesPath,
                                       bool OnlyLocalDecls = false,
+                                      bool CaptureDiagnostics = false,
                                       RemappedFile *RemappedFiles = 0,
                                       unsigned NumRemappedFiles = 0,
-                                      bool CaptureDiagnostics = false,
                                       bool PrecompilePreamble = false,
                                       bool CompleteTranslationUnit = true,
                                       bool CacheCodeCompletionResults = false,

clang/include/clang/Frontend/CompilerInstance.h

@@ -477,8 +477,14 @@ public:
   /// Create the diagnostics engine using the invocation's diagnostic options
   /// and replace any existing one with it.
   ///
-  /// Note that this routine also replaces the diagnostic client.
+  /// Note that this routine also replaces the diagnostic client,
-  void createDiagnostics(int Argc, const char* const *Argv);
+  /// allocating one if one is not provided.
+  ///
+  /// \param Client If non-NULL, a diagnostic client that will be
+  /// attached to (and, then, owned by) the Diagnostic inside this AST
+  /// unit.
+  void createDiagnostics(int Argc, const char* const *Argv,
+                         DiagnosticClient *Client = 0);
 
   /// Create a Diagnostic object with a the TextDiagnosticPrinter.
   ///
@@ -486,18 +492,24 @@ public:
   /// when the diagnostic options indicate that the compiler should output
   /// logging information.
   ///
-  /// Note that this creates an unowned DiagnosticClient, if using directly the
+  /// If no diagnostic client is provided, this creates a
-  /// caller is responsible for releasing the returned Diagnostic's client
+  /// DiagnosticClient that is owned by the returned diagnostic
-  /// eventually.
+  /// object, if using directly the caller is responsible for
+  /// releasing the returned Diagnostic's client eventually.
   ///
   /// \param Opts - The diagnostic options; note that the created text
   /// diagnostic object contains a reference to these options and its lifetime
   /// must extend past that of the diagnostic engine.
   ///
+  /// \param Client If non-NULL, a diagnostic client that will be
+  /// attached to (and, then, owned by) the returned Diagnostic
+  /// object.
+  ///
   /// \return The new object on success, or null on failure.
   static llvm::IntrusiveRefCntPtr<Diagnostic> 
   createDiagnostics(const DiagnosticOptions &Opts, int Argc,
-                    const char* const *Argv);
+                    const char* const *Argv,
+                    DiagnosticClient *Client = 0);
 
   /// Create the file manager and replace any existing one with it.
   void createFileManager();

clang/lib/Frontend/ASTUnit.cpp

@@ -406,7 +406,7 @@ class CaptureDroppedDiagnostics {
 
 public:
   CaptureDroppedDiagnostics(bool RequestCapture, Diagnostic &Diags, 
-                           llvm::SmallVectorImpl<StoredDiagnostic> &StoredDiags)
+                          llvm::SmallVectorImpl<StoredDiagnostic> &StoredDiags)
     : Diags(Diags), Client(StoredDiags), PreviousClient(0)
   {
     if (RequestCapture || Diags.getClient() == 0) {
@@ -447,6 +447,22 @@ llvm::MemoryBuffer *ASTUnit::getBufferForFile(llvm::StringRef Filename,
                                    ErrorStr, FileSize, FileInfo);
 }
 
+/// \brief Configure the diagnostics object for use with ASTUnit.
+void ASTUnit::ConfigureDiags(llvm::IntrusiveRefCntPtr<Diagnostic> &Diags,
+                             ASTUnit &AST, bool CaptureDiagnostics) {
+  if (!Diags.getPtr()) {
+    // No diagnostics engine was provided, so create our own diagnostics object
+    // with the default options.
+    DiagnosticOptions DiagOpts;
+    DiagnosticClient *Client = 0;
+    if (CaptureDiagnostics)
+      Client = new StoredDiagnosticClient(AST.StoredDiagnostics);
+    Diags = CompilerInstance::createDiagnostics(DiagOpts, 0, 0, Client);
+  } else if (CaptureDiagnostics) {
+    Diags->setClient(new StoredDiagnosticClient(AST.StoredDiagnostics));
+  }
+}
+
 ASTUnit *ASTUnit::LoadFromASTFile(const std::string &Filename,
                                   llvm::IntrusiveRefCntPtr<Diagnostic> Diags,
                                   const FileSystemOptions &FileSystemOpts,
@@ -455,16 +471,10 @@ ASTUnit *ASTUnit::LoadFromASTFile(const std::string &Filename,
                                   unsigned NumRemappedFiles,
                                   bool CaptureDiagnostics) {
   llvm::OwningPtr<ASTUnit> AST(new ASTUnit(true));
+  ConfigureDiags(Diags, *AST, CaptureDiagnostics);
 
-  if (!Diags.getPtr()) {
-    // No diagnostics engine was provided, so create our own diagnostics object
-    // with the default options.
-    DiagnosticOptions DiagOpts;
-    Diags = CompilerInstance::createDiagnostics(DiagOpts, 0, 0);
-  }
-
-  AST->CaptureDiagnostics = CaptureDiagnostics;
   AST->OnlyLocalDecls = OnlyLocalDecls;
+  AST->CaptureDiagnostics = CaptureDiagnostics;
   AST->Diagnostics = Diags;
   AST->FileSystemOpts = FileSystemOpts;
   AST->FileMgr.reset(new FileManager);
@@ -474,10 +484,6 @@ ASTUnit *ASTUnit::LoadFromASTFile(const std::string &Filename,
   AST->HeaderInfo.reset(new HeaderSearch(AST->getFileManager(),
                                          AST->getFileSystemOpts()));
   
-  // If requested, capture diagnostics in the ASTUnit.
-  CaptureDroppedDiagnostics Capture(CaptureDiagnostics, AST->getDiagnostics(),
-                                    AST->StoredDiagnostics);
-
   for (unsigned I = 0; I != NumRemappedFiles; ++I) {
     // Create the file entry for the file that we're mapping from.
     const FileEntry *FromFile
@@ -708,9 +714,6 @@ bool ASTUnit::Parse(llvm::MemoryBuffer *OverrideMainBuffer) {
   // Set up diagnostics, capturing any diagnostics that would
   // otherwise be dropped.
   Clang.setDiagnostics(&getDiagnostics());
-  CaptureDroppedDiagnostics Capture(CaptureDiagnostics, 
-                                    getDiagnostics(),
-                                    StoredDiagnostics);
   
   // Create the target instance.
   Clang.setTarget(TargetInfo::CreateTargetInfo(Clang.getDiagnostics(),
@@ -1200,9 +1203,6 @@ llvm::MemoryBuffer *ASTUnit::getMainBufferWithPrecompiledPreamble(
   
   // Set up diagnostics, capturing all of the diagnostics produced.
   Clang.setDiagnostics(&getDiagnostics());
-  CaptureDroppedDiagnostics Capture(CaptureDiagnostics, 
-                                    getDiagnostics(),
-                                    StoredDiagnostics);
   
   // Create the target instance.
   Clang.setTarget(TargetInfo::CreateTargetInfo(Clang.getDiagnostics(),
@@ -1368,19 +1368,13 @@ ASTUnit *ASTUnit::LoadFromCompilerInvocation(CompilerInvocation *CI,
                                              bool PrecompilePreamble,
                                              bool CompleteTranslationUnit,
                                              bool CacheCodeCompletionResults) {  
-  if (!Diags.getPtr()) {
-    // No diagnostics engine was provided, so create our own diagnostics object
-    // with the default options.
-    DiagnosticOptions DiagOpts;
-    Diags = CompilerInstance::createDiagnostics(DiagOpts, 0, 0);
-  }
-  
   // Create the AST unit.
   llvm::OwningPtr<ASTUnit> AST;
   AST.reset(new ASTUnit(false));
+  ConfigureDiags(Diags, *AST, CaptureDiagnostics);
   AST->Diagnostics = Diags;
-  AST->CaptureDiagnostics = CaptureDiagnostics;
   AST->OnlyLocalDecls = OnlyLocalDecls;
+  AST->CaptureDiagnostics = CaptureDiagnostics;
   AST->CompleteTranslationUnit = CompleteTranslationUnit;
   AST->ShouldCacheCodeCompletionResults = CacheCodeCompletionResults;
   AST->Invocation.reset(CI);
@@ -1393,22 +1387,19 @@ ASTUnit *ASTUnit::LoadFromCommandLine(const char **ArgBegin,
                                     llvm::IntrusiveRefCntPtr<Diagnostic> Diags,
                                       llvm::StringRef ResourceFilesPath,
                                       bool OnlyLocalDecls,
+                                      bool CaptureDiagnostics,
                                       RemappedFile *RemappedFiles,
                                       unsigned NumRemappedFiles,
-                                      bool CaptureDiagnostics,
                                       bool PrecompilePreamble,
                                       bool CompleteTranslationUnit,
                                       bool CacheCodeCompletionResults,
                                       bool CXXPrecompilePreamble,
                                       bool CXXChainedPCH) {
-  bool CreatedDiagnosticsObject = false;
-  
   if (!Diags.getPtr()) {
     // No diagnostics engine was provided, so create our own diagnostics object
     // with the default options.
     DiagnosticOptions DiagOpts;
     Diags = CompilerInstance::createDiagnostics(DiagOpts, 0, 0);
-    CreatedDiagnosticsObject = true;
   }
   
   llvm::SmallVector<const char *, 16> Args;
@@ -1422,9 +1413,9 @@ ASTUnit *ASTUnit::LoadFromCommandLine(const char **ArgBegin,
   llvm::SmallVector<StoredDiagnostic, 4> StoredDiagnostics;
   
   llvm::OwningPtr<CompilerInvocation> CI;
+
   {
-    CaptureDroppedDiagnostics Capture(CaptureDiagnostics, 
+    CaptureDroppedDiagnostics Capture(CaptureDiagnostics, *Diags, 
-                                      *Diags,
                                       StoredDiagnostics);
 
     // FIXME: We shouldn't have to pass in the path info.
@@ -1457,8 +1448,8 @@ ASTUnit *ASTUnit::LoadFromCommandLine(const char **ArgBegin,
     const driver::ArgStringList &CCArgs = Cmd->getArguments();
     CI.reset(new CompilerInvocation);
     CompilerInvocation::CreateFromArgs(*CI,
-                                       const_cast<const char **>(CCArgs.data()),
+                                     const_cast<const char **>(CCArgs.data()),
-                                       const_cast<const char **>(CCArgs.data()) +
+                                     const_cast<const char **>(CCArgs.data()) +
                                        CCArgs.size(),
                                        *Diags);
   }
@@ -1484,9 +1475,10 @@ ASTUnit *ASTUnit::LoadFromCommandLine(const char **ArgBegin,
   // Create the AST unit.
   llvm::OwningPtr<ASTUnit> AST;
   AST.reset(new ASTUnit(false));
+  ConfigureDiags(Diags, *AST, CaptureDiagnostics);
   AST->Diagnostics = Diags;
-  AST->CaptureDiagnostics = CaptureDiagnostics;
   AST->OnlyLocalDecls = OnlyLocalDecls;
+  AST->CaptureDiagnostics = CaptureDiagnostics;
   AST->CompleteTranslationUnit = CompleteTranslationUnit;
   AST->ShouldCacheCodeCompletionResults = CacheCodeCompletionResults;
   AST->NumStoredDiagnosticsFromDriver = StoredDiagnostics.size();

clang/lib/Frontend/CompilerInstance.cpp

@@ -113,19 +113,23 @@ static void SetUpBuildDumpLog(const DiagnosticOptions &DiagOpts,
   Diags.setClient(new ChainedDiagnosticClient(Diags.takeClient(), Logger));
 }
 
-void CompilerInstance::createDiagnostics(int Argc, const char* const *Argv) {
+void CompilerInstance::createDiagnostics(int Argc, const char* const *Argv,
-  Diagnostics = createDiagnostics(getDiagnosticOpts(), Argc, Argv);
+                                         DiagnosticClient *Client) {
+  Diagnostics = createDiagnostics(getDiagnosticOpts(), Argc, Argv, Client);
 }
 
 llvm::IntrusiveRefCntPtr<Diagnostic> 
 CompilerInstance::createDiagnostics(const DiagnosticOptions &Opts,
-                                    int Argc, const char* const *Argv) {
+                                    int Argc, const char* const *Argv,
+                                    DiagnosticClient *Client) {
   llvm::IntrusiveRefCntPtr<Diagnostic> Diags(new Diagnostic());
 
   // Create the diagnostic client for reporting errors or for
   // implementing -verify.
-  llvm::OwningPtr<DiagnosticClient> DiagClient;
+  if (Client)
-  Diags->setClient(new TextDiagnosticPrinter(llvm::errs(), Opts));
+    Diags->setClient(Client);
+  else
+    Diags->setClient(new TextDiagnosticPrinter(llvm::errs(), Opts));
 
   // Chain in -verify checker, if requested.
   if (Opts.VerifyDiagnostics)

clang/test/Index/complete-driver-errors.c

@@ -2,20 +2,21 @@ int *blah = 1;
 
 int
 
-// Test driver errors with code completion
-// RUN: c-index-test -code-completion-at=%s:4:1 -std= %s 2> %t | FileCheck -check-prefix=CHECK-RESULTS %s
-// RUN: FileCheck -check-prefix=CHECK-DIAGS %s < %t
 // CHECK-RESULTS: NotImplemented:{TypedText const} (40)
 // CHECK-RESULTS: NotImplemented:{TypedText restrict} (40)
 // CHECK-RESULTS: NotImplemented:{TypedText volatile} (40)
+// CHECK-DIAGS: error: invalid value '' in '-std='
+// CHECK-DIAGS: complete-driver-errors.c:1:6:{1:13-1:14}: warning: incompatible integer to pointer conversion initializing 'int *' with an expression of type 'int'
+
+// Test driver errors with code completion
+// RUN: c-index-test -code-completion-at=%s:4:1 -std= %s 2> %t | FileCheck -check-prefix=CHECK-RESULTS %s
+// RUN: FileCheck -check-prefix=CHECK-DIAGS %s < %t
 
 // Test driver errors with parsing
 // RUN: c-index-test -test-load-source all -std= %s 2> %t | FileCheck -check-prefix=CHECK-LOAD %s
 // RUN: FileCheck -check-prefix=CHECK-DIAGS %s < %t
 // CHECK-LOAD: complete-driver-errors.c:1:6: VarDecl=blah:1:6
 
-// CHECK-DIAGS: error: invalid value '' in '-std='
-// CHECK-DIAGS: complete-driver-errors.c:1:6:{1:13-1:14}: warning: incompatible integer to pointer conversion initializing 'int *' with an expression of type 'int'
 // Test driver errors with code completion and precompiled preamble
 // RUN: env CINDEXTEST_EDITING=1 c-index-test -code-completion-at=%s:4:1 -std= %s 2> %t | FileCheck -check-prefix=CHECK-RESULTS %s
 // RUN: FileCheck -check-prefix=CHECK-DIAGS %s < %t

clang/tools/libclang/CIndex.cpp

@@ -2186,9 +2186,9 @@ static void clang_parseTranslationUnit_Impl(void *UserData) {
                                  Diags,
                                  CXXIdx->getClangResourcesPath(),
                                  CXXIdx->getOnlyLocalDecls(),
+                                 /*CaptureDiagnostics=*/true,
                                  RemappedFiles.data(),
                                  RemappedFiles.size(),
-                                 /*CaptureDiagnostics=*/true,
                                  PrecompilePreamble,
                                  CompleteTranslationUnit,
                                  CacheCodeCompetionResults,