[cfe-commits] r100464 - in /cfe/trunk: include/clang/Frontend/ASTUnit.h lib/Frontend/ASTMerge.cpp lib/Frontend/ASTUnit.cpp lib/Frontend/FrontendAction.cpp tools/CIndex/CIndex.cpp
Ted Kremenek
kremenek at apple.com
Mon Apr 5 16:11:44 PDT 2010
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
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
More information about the cfe-commits
mailing list