[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
Douglas Gregor
dgregor at apple.com
Mon Apr 5 17:01:58 PDT 2010
Yeah, that's better.
Sent from my iPhone
On Apr 5, 2010, at 4:11 PM, Ted Kremenek <kremenek at apple.com> wrote:
> 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