I'm a little dubious about MaybeOwningPtr in general. Why not just make Diagnostics reference-counted? That seems like a clearer idiom than "maybe ownership." We have IntrusiveRefCntPtr if we want to use a smart pointer here.
On Apr 5, 2010, at 2:10 PM, Douglas Gregor wrote: > Author: dgregor > Date: Mon Apr 5 16:10:19 2010 > New Revision: 100464 > > URL: http://llvm.org/viewvc/llvm-project?rev=100464&view=rev > Log: > 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>. > > Modified: > cfe/trunk/include/clang/Frontend/ASTUnit.h > cfe/trunk/lib/Frontend/ASTMerge.cpp > cfe/trunk/lib/Frontend/ASTUnit.cpp > cfe/trunk/lib/Frontend/FrontendAction.cpp > cfe/trunk/tools/CIndex/CIndex.cpp > > Modified: cfe/trunk/include/clang/Frontend/ASTUnit.h > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Frontend/ASTUnit.h?rev=100464&r1=100463&r2=100464&view=diff > ============================================================================== > --- cfe/trunk/include/clang/Frontend/ASTUnit.h (original) > +++ cfe/trunk/include/clang/Frontend/ASTUnit.h Mon Apr 5 16:10:19 2010 > @@ -52,6 +52,7 @@ > 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 @@ > 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 @@ > > 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 @@ > /// > /// \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 @@ > // 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 @@ > // 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 @@ > 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 > > Modified: cfe/trunk/lib/Frontend/ASTMerge.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/ASTMerge.cpp?rev=100464&r1=100463&r2=100464&view=diff > ============================================================================== > --- cfe/trunk/lib/Frontend/ASTMerge.cpp (original) > +++ cfe/trunk/lib/Frontend/ASTMerge.cpp Mon Apr 5 16:10:19 2010 > @@ -37,7 +37,8 @@ > 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; > > Modified: cfe/trunk/lib/Frontend/ASTUnit.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/ASTUnit.cpp?rev=100464&r1=100463&r2=100464&view=diff > ============================================================================== > --- cfe/trunk/lib/Frontend/ASTUnit.cpp (original) > +++ cfe/trunk/lib/Frontend/ASTUnit.cpp Mon Apr 5 16:10:19 2010 > @@ -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 @@ > } > > 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 @@ > 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 @@ > 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 @@ > > 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 @@ > 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 @@ > } > > 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 @@ > 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 @@ > "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 @@ > > 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 @@ > > // 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 @@ > 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 @@ > 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) > > Modified: cfe/trunk/lib/Frontend/FrontendAction.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/FrontendAction.cpp?rev=100464&r1=100463&r2=100464&view=diff > ============================================================================== > --- cfe/trunk/lib/Frontend/FrontendAction.cpp (original) > +++ cfe/trunk/lib/Frontend/FrontendAction.cpp Mon Apr 5 16:10:19 2010 > @@ -46,7 +46,8 @@ > 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; > > > Modified: cfe/trunk/tools/CIndex/CIndex.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/CIndex/CIndex.cpp?rev=100464&r1=100463&r2=100464&view=diff > ============================================================================== > --- cfe/trunk/tools/CIndex/CIndex.cpp (original) > +++ cfe/trunk/tools/CIndex/CIndex.cpp Mon Apr 5 16:10:19 2010 > @@ -996,11 +996,7 @@ > > 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 @@ > > // 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 @@ > > llvm::OwningPtr<ASTUnit> Unit( > ASTUnit::LoadFromCommandLine(Args.data(), Args.data() + Args.size(), > - *Diags, > + Diags, > CXXIdx->getClangResourcesPath(), > CXXIdx->getOnlyLocalDecls(), > RemappedFiles.data(), > @@ -1169,7 +1165,7 @@ > 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(), > > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
