Clarify the ownership semantics of the Diagnostic object used by

ASTUnit. Previously, we would end up with use-after-free errors
because the Diagnostic object would be creating in one place (say,
CIndex) and its ownership would not be transferred into the
ASTUnit. Fixes <rdar://problem/7818608>.

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

diff --git a/include/clang/Frontend/ASTUnit.h b/include/clang/Frontend/ASTUnit.h
index 573a3f2723d2b01ea0a0baa8bc2f78f79ace2fb8..7fc3bf9b1e39894d81d846a5371f751625040e9b 100644 (file)
--- a/include/clang/Frontend/ASTUnit.h
+++ b/include/clang/Frontend/ASTUnit.h
@@ -52,6 +52,7 @@ public:
   typedef std::map<FileID, std::vector<PreprocessedEntity *> > 
     PreprocessedEntitiesByFileMap;
 private:
+  llvm::MaybeOwningPtr<Diagnostic>  Diagnostics;
   llvm::OwningPtr<FileManager>      FileMgr;
   llvm::OwningPtr<SourceManager>    SourceMgr;
   llvm::OwningPtr<HeaderSearch>     HeaderInfo;
@@ -116,7 +117,7 @@ private:
   ASTUnit(const ASTUnit&); // DO NOT IMPLEMENT
   ASTUnit &operator=(const ASTUnit &); // DO NOT IMPLEMENT
   
-  ASTUnit(Diagnostic &Diag, bool MainFileIsAST);
+  explicit ASTUnit(bool MainFileIsAST);
 
 public:
   class ConcurrencyCheck {
@@ -141,6 +142,9 @@ public:
 
   bool isMainFileAST() const { return MainFileIsAST; }
 
+  const Diagnostic &getDiagnostics() const { return *Diagnostics; }
+  Diagnostic &getDiagnostics()             { return *Diagnostics; }
+  
   const SourceManager &getSourceManager() const { return *SourceMgr; }
         SourceManager &getSourceManager()       { return *SourceMgr; }
 
@@ -210,7 +214,7 @@ public:
   ///
   /// \returns - The initialized ASTUnit or null if the PCH failed to load.
   static ASTUnit *LoadFromPCHFile(const std::string &Filename,
-                                  Diagnostic &Diags,
+                                  llvm::MaybeOwningPtr<Diagnostic> Diags,
                                   bool OnlyLocalDecls = false,
                                   RemappedFile *RemappedFiles = 0,
                                   unsigned NumRemappedFiles = 0,
@@ -228,7 +232,7 @@ public:
   // FIXME: Move OnlyLocalDecls, UseBumpAllocator to setters on the ASTUnit, we
   // shouldn't need to specify them at construction time.
   static ASTUnit *LoadFromCompilerInvocation(CompilerInvocation *CI,
-                                             Diagnostic &Diags,
+                                       llvm::MaybeOwningPtr<Diagnostic> Diags,
                                              bool OnlyLocalDecls = false,
                                              bool CaptureDiagnostics = false);
 
@@ -248,7 +252,7 @@ public:
   // shouldn't need to specify them at construction time.
   static ASTUnit *LoadFromCommandLine(const char **ArgBegin,
                                       const char **ArgEnd,
-                                      Diagnostic &Diags,
+                                      llvm::MaybeOwningPtr<Diagnostic> Diags,
                                       llvm::StringRef ResourceFilesPath,
                                       bool OnlyLocalDecls = false,
                                       RemappedFile *RemappedFiles = 0,
@@ -256,6 +260,24 @@ public:
                                       bool CaptureDiagnostics = false);
 };
 
+/// \brief Return an potentially-owning pointer for the given diagnostic engine
+/// that owns the pointer.
+inline llvm::MaybeOwningPtr<Diagnostic> OwnedDiag(Diagnostic &Diags) {
+  return llvm::MaybeOwningPtr<Diagnostic>(&Diags, true);
+}
+
+/// \brief Return a potentially-owning pointer for the given diagnostic engine
+/// that does not own the pointer.
+inline llvm::MaybeOwningPtr<Diagnostic> UnownedDiag(Diagnostic &Diags) {
+  return llvm::MaybeOwningPtr<Diagnostic>(&Diags, false);
+}
+
+/// \brief Return an potentially-owning pointer that indicates that the
+/// default diagnostic engine should be used.
+inline llvm::MaybeOwningPtr<Diagnostic> DefaultDiag() {
+  return llvm::MaybeOwningPtr<Diagnostic>();
+}
+  
 } // namespace clang
 
 #endif

diff --git a/lib/Frontend/ASTMerge.cpp b/lib/Frontend/ASTMerge.cpp
index 2228ea45df3331b3dad7a7cfa7ea1f36efc7c58c..24658615a9aff37a6a4c3670ce6029fca78ca07c 100644 (file)
--- a/lib/Frontend/ASTMerge.cpp
+++ b/lib/Frontend/ASTMerge.cpp
@@ -37,7 +37,8 @@ void ASTMergeAction::ExecuteAction() {
   CI.getDiagnostics().SetArgToStringFn(&FormatASTNodeDiagnosticArgument,
                                        &CI.getASTContext());
   for (unsigned I = 0, N = ASTFiles.size(); I != N; ++I) {
-    ASTUnit *Unit = ASTUnit::LoadFromPCHFile(ASTFiles[I], CI.getDiagnostics(),
+    ASTUnit *Unit = ASTUnit::LoadFromPCHFile(ASTFiles[I], 
+                                             UnownedDiag(CI.getDiagnostics()),
                                              false);
     if (!Unit)
       continue;

diff --git a/lib/Frontend/ASTUnit.cpp b/lib/Frontend/ASTUnit.cpp
index 38aeedc39ada93c85c305d2c5488146952f5d2c8..2a76d6f37bc9ef1109121d3ecad8ccc45edfd430 100644 (file)
--- a/lib/Frontend/ASTUnit.cpp
+++ b/lib/Frontend/ASTUnit.cpp
@@ -35,12 +35,9 @@
 #include "llvm/System/Path.h"
 using namespace clang;
 
-ASTUnit::ASTUnit(Diagnostic &Diag, bool _MainFileIsAST)
-  : MainFileIsAST(_MainFileIsAST), 
-    ConcurrencyCheckValue(CheckUnlocked) {
-  FileMgr.reset(new FileManager);
-  SourceMgr.reset(new SourceManager(Diag));
-}
+ASTUnit::ASTUnit(bool _MainFileIsAST)
+  : MainFileIsAST(_MainFileIsAST), ConcurrencyCheckValue(CheckUnlocked) { }
+
 ASTUnit::~ASTUnit() {
   ConcurrencyCheckValue = CheckLocked;
   for (unsigned I = 0, N = TemporaryFiles.size(); I != N; ++I)
@@ -144,17 +141,30 @@ const std::string &ASTUnit::getPCHFileName() {
 }
 
 ASTUnit *ASTUnit::LoadFromPCHFile(const std::string &Filename,
-                                  Diagnostic &Diags,
+                                  llvm::MaybeOwningPtr<Diagnostic> Diags,
                                   bool OnlyLocalDecls,
                                   RemappedFile *RemappedFiles,
                                   unsigned NumRemappedFiles,
                                   bool CaptureDiagnostics) {
-  llvm::OwningPtr<ASTUnit> AST(new ASTUnit(Diags, true));
+  llvm::OwningPtr<ASTUnit> AST(new ASTUnit(true));
+  
+  if (Diags.get())
+    AST->Diagnostics = Diags;
+  else {
+    // No diagnostics engine was provided, so create our own diagnostics object
+    // with the default options.
+    DiagnosticOptions DiagOpts;
+    AST->Diagnostics.reset(CompilerInstance::createDiagnostics(DiagOpts, 0, 0),
+                           true);
+  }
+  
   AST->OnlyLocalDecls = OnlyLocalDecls;
+  AST->FileMgr.reset(new FileManager);
+  AST->SourceMgr.reset(new SourceManager(AST->getDiagnostics()));
   AST->HeaderInfo.reset(new HeaderSearch(AST->getFileManager()));
 
   // If requested, capture diagnostics in the ASTUnit.
-  CaptureDroppedDiagnostics Capture(CaptureDiagnostics, Diags, 
+  CaptureDroppedDiagnostics Capture(CaptureDiagnostics, AST->getDiagnostics(),
                                     AST->StoredDiagnostics);
 
   for (unsigned I = 0; I != NumRemappedFiles; ++I) {
@@ -164,7 +174,7 @@ ASTUnit *ASTUnit::LoadFromPCHFile(const std::string &Filename,
                                     RemappedFiles[I].second->getBufferSize(),
                                              0);
     if (!FromFile) {
-      Diags.Report(diag::err_fe_remap_missing_from_file)
+      AST->getDiagnostics().Report(diag::err_fe_remap_missing_from_file)
         << RemappedFiles[I].first;
       delete RemappedFiles[I].second;
       continue;
@@ -188,7 +198,7 @@ ASTUnit *ASTUnit::LoadFromPCHFile(const std::string &Filename,
   llvm::OwningPtr<ExternalASTSource> Source;
 
   Reader.reset(new PCHReader(AST->getSourceManager(), AST->getFileManager(),
-                             Diags));
+                             AST->getDiagnostics()));
   Reader->setListener(new PCHInfoCollector(LangInfo, HeaderInfo, TargetTriple,
                                            Predefines, Counter));
 
@@ -198,7 +208,7 @@ ASTUnit *ASTUnit::LoadFromPCHFile(const std::string &Filename,
 
   case PCHReader::Failure:
   case PCHReader::IgnorePCH:
-    Diags.Report(diag::err_fe_unable_to_load_pch);
+    AST->getDiagnostics().Report(diag::err_fe_unable_to_load_pch);
     return NULL;
   }
 
@@ -214,8 +224,10 @@ ASTUnit *ASTUnit::LoadFromPCHFile(const std::string &Filename,
   TargetOpts.CPU = "";
   TargetOpts.Features.clear();
   TargetOpts.Triple = TargetTriple;
-  AST->Target.reset(TargetInfo::CreateTargetInfo(Diags, TargetOpts));
-  AST->PP.reset(new Preprocessor(Diags, LangInfo, *AST->Target.get(),
+  AST->Target.reset(TargetInfo::CreateTargetInfo(AST->getDiagnostics(),
+                                                 TargetOpts));
+  AST->PP.reset(new Preprocessor(AST->getDiagnostics(), LangInfo, 
+                                 *AST->Target.get(),
                                  AST->getSourceManager(), HeaderInfo));
   Preprocessor &PP = *AST->PP.get();
 
@@ -278,7 +290,7 @@ public:
 }
 
 ASTUnit *ASTUnit::LoadFromCompilerInvocation(CompilerInvocation *CI,
-                                             Diagnostic &Diags,
+                                         llvm::MaybeOwningPtr<Diagnostic> Diags,
                                              bool OnlyLocalDecls,
                                              bool CaptureDiagnostics) {
   // Create the compiler instance to use for building the AST.
@@ -286,10 +298,17 @@ ASTUnit *ASTUnit::LoadFromCompilerInvocation(CompilerInvocation *CI,
   llvm::OwningPtr<ASTUnit> AST;
   llvm::OwningPtr<TopLevelDeclTrackerAction> Act;
 
+  if (!Diags.get()) {
+    // No diagnostics engine was provided, so create our own diagnostics object
+    // with the default options.
+    DiagnosticOptions DiagOpts;
+    Diags.reset(CompilerInstance::createDiagnostics(DiagOpts, 0, 0), true);
+  }
+  
   Clang.setInvocation(CI);
 
-  Clang.setDiagnostics(&Diags);
-  Clang.setDiagnosticClient(Diags.getClient());
+  Clang.setDiagnostics(Diags.get());
+  Clang.setDiagnosticClient(Diags->getClient());
 
   // Create the target instance.
   Clang.setTarget(TargetInfo::CreateTargetInfo(Clang.getDiagnostics(),
@@ -312,7 +331,10 @@ ASTUnit *ASTUnit::LoadFromCompilerInvocation(CompilerInvocation *CI,
          "FIXME: AST inputs not yet supported here!");
 
   // Create the AST unit.
-  AST.reset(new ASTUnit(Diags, false));
+  AST.reset(new ASTUnit(false));
+  AST->Diagnostics = Diags;
+  AST->FileMgr.reset(new FileManager);
+  AST->SourceMgr.reset(new SourceManager(AST->getDiagnostics()));
   AST->OnlyLocalDecls = OnlyLocalDecls;
   AST->OriginalSourceFile = Clang.getFrontendOpts().Inputs[0].second;
 
@@ -364,12 +386,19 @@ error:
 
 ASTUnit *ASTUnit::LoadFromCommandLine(const char **ArgBegin,
                                       const char **ArgEnd,
-                                      Diagnostic &Diags,
+                                      llvm::MaybeOwningPtr<Diagnostic> Diags,
                                       llvm::StringRef ResourceFilesPath,
                                       bool OnlyLocalDecls,
                                       RemappedFile *RemappedFiles,
                                       unsigned NumRemappedFiles,
                                       bool CaptureDiagnostics) {
+  if (!Diags.get()) {
+    // No diagnostics engine was provided, so create our own diagnostics object
+    // with the default options.
+    DiagnosticOptions DiagOpts;
+    Diags.reset(CompilerInstance::createDiagnostics(DiagOpts, 0, 0), true);
+  }
+  
   llvm::SmallVector<const char *, 16> Args;
   Args.push_back("<clang>"); // FIXME: Remove dummy argument.
   Args.insert(Args.end(), ArgBegin, ArgEnd);
@@ -380,7 +409,7 @@ ASTUnit *ASTUnit::LoadFromCommandLine(const char **ArgBegin,
 
   // FIXME: We shouldn't have to pass in the path info.
   driver::Driver TheDriver("clang", "/", llvm::sys::getHostTriple(),
-                           "a.out", false, false, Diags);
+                           "a.out", false, false, *Diags);
 
   // Don't check that inputs exist, they have been remapped.
   TheDriver.setCheckInputsExist(false);
@@ -395,13 +424,13 @@ ASTUnit *ASTUnit::LoadFromCommandLine(const char **ArgBegin,
     llvm::SmallString<256> Msg;
     llvm::raw_svector_ostream OS(Msg);
     C->PrintJob(OS, C->getJobs(), "; ", true);
-    Diags.Report(diag::err_fe_expected_compiler_job) << OS.str();
+    Diags->Report(diag::err_fe_expected_compiler_job) << OS.str();
     return 0;
   }
 
   const driver::Command *Cmd = cast<driver::Command>(*Jobs.begin());
   if (llvm::StringRef(Cmd->getCreator().getName()) != "clang") {
-    Diags.Report(diag::err_fe_expected_clang_command);
+    Diags->Report(diag::err_fe_expected_clang_command);
     return 0;
   }
 
@@ -409,7 +438,7 @@ ASTUnit *ASTUnit::LoadFromCommandLine(const char **ArgBegin,
   llvm::OwningPtr<CompilerInvocation> CI(new CompilerInvocation);
   CompilerInvocation::CreateFromArgs(*CI, (const char**) CCArgs.data(),
                                      (const char**) CCArgs.data()+CCArgs.size(),
-                                     Diags);
+                                     *Diags);
 
   // Override any files that need remapping
   for (unsigned I = 0; I != NumRemappedFiles; ++I)

diff --git a/lib/Frontend/FrontendAction.cpp b/lib/Frontend/FrontendAction.cpp
index 110612d03b5860562e38cae8bdf713c95169f347..4c2b1e9b8299dfc35c567670ccf917a1b0ffd825 100644 (file)
--- a/lib/Frontend/FrontendAction.cpp
+++ b/lib/Frontend/FrontendAction.cpp
@@ -46,7 +46,8 @@ bool FrontendAction::BeginSourceFile(CompilerInstance &CI,
     assert(hasASTSupport() && "This action does not have AST support!");
 
     std::string Error;
-    ASTUnit *AST = ASTUnit::LoadFromPCHFile(Filename, CI.getDiagnostics());
+    ASTUnit *AST = ASTUnit::LoadFromPCHFile(Filename, 
+                                            UnownedDiag(CI.getDiagnostics()));
     if (!AST)
       goto failure;
 

diff --git a/tools/CIndex/CIndex.cpp b/tools/CIndex/CIndex.cpp
index 9db5ba79cef7ea88043182a7814c46bb3d1b333f..a6e284e904fc13bef83caca684962e663f99eaa3 100644 (file)
--- a/tools/CIndex/CIndex.cpp
+++ b/tools/CIndex/CIndex.cpp
@@ -996,11 +996,7 @@ CXTranslationUnit clang_createTranslationUnit(CXIndex CIdx,
 
   CIndexer *CXXIdx = static_cast<CIndexer *>(CIdx);
 
-  // Configure the diagnostics.
-  DiagnosticOptions DiagOpts;
-  llvm::OwningPtr<Diagnostic> Diags;
-  Diags.reset(CompilerInstance::createDiagnostics(DiagOpts, 0, 0));
-  return ASTUnit::LoadFromPCHFile(ast_filename, *Diags,
+  return ASTUnit::LoadFromPCHFile(ast_filename, DefaultDiag(),
                                   CXXIdx->getOnlyLocalDecls(),
                                   0, 0, true);
 }
@@ -1019,8 +1015,8 @@ clang_createTranslationUnitFromSourceFile(CXIndex CIdx,
 
   // Configure the diagnostics.
   DiagnosticOptions DiagOpts;
-  llvm::OwningPtr<Diagnostic> Diags;
-  Diags.reset(CompilerInstance::createDiagnostics(DiagOpts, 0, 0));
+  llvm::MaybeOwningPtr<Diagnostic> Diags;
+  Diags.reset(CompilerInstance::createDiagnostics(DiagOpts, 0, 0), true);
 
   llvm::SmallVector<ASTUnit::RemappedFile, 4> RemappedFiles;
   for (unsigned I = 0; I != num_unsaved_files; ++I) {
@@ -1052,7 +1048,7 @@ clang_createTranslationUnitFromSourceFile(CXIndex CIdx,
 
     llvm::OwningPtr<ASTUnit> Unit(
       ASTUnit::LoadFromCommandLine(Args.data(), Args.data() + Args.size(),
-                                   *Diags,
+                                   Diags,
                                    CXXIdx->getClangResourcesPath(),
                                    CXXIdx->getOnlyLocalDecls(),
                                    RemappedFiles.data(),
@@ -1169,7 +1165,7 @@ clang_createTranslationUnitFromSourceFile(CXIndex CIdx,
     Diags->Report(diag::err_fe_invoking) << AllArgs << ErrMsg;
   }
 
-  ASTUnit *ATU = ASTUnit::LoadFromPCHFile(astTmpFile, *Diags,
+  ASTUnit *ATU = ASTUnit::LoadFromPCHFile(astTmpFile, Diags,
                                           CXXIdx->getOnlyLocalDecls(),
                                           RemappedFiles.data(),
                                           RemappedFiles.size(),