[PATCH] D15450: Avoid double deletion in Clang driver.
Jonathan Roelofs via cfe-commits
cfe-commits at lists.llvm.org
Fri Jan 29 11:54:22 PST 2016
On 1/29/16 12:50 PM, Serge Pavlov wrote:
> Crash is observed when HandleTranslation unit is not called, clang
> always calls this method but a clang based project may don't call it if
> it is not needed. I saw the crash in such project and used this patch to
> fix it. I do not know how to make a regression test, I am not even sure
> such test would be practical. The fact that an object is owned by two
> unique_ptr's may be enought to change implementation. However any
> thoughts how to test this behavior are wellcome.
This is what the clang/unittests is for (i.e. you can write a testcase
that doesn't have to go through the clang driver).
Jon
>
> Thanks,
> --Serge
>
> 2016-01-30 0:54 GMT+06:00 Jonathan Roelofs <jonathan at codesourcery.com
> <mailto:jonathan at codesourcery.com>>:
>
>
>
> On 1/29/16 11:51 AM, Serge Pavlov via cfe-commits wrote:
>
> Can somebody have a look at this fix?
> Thank you!
>
>
> Testcase?
>
>
> --Serge
>
> 2016-01-11 23:50 GMT+06:00 Serge Pavlov <sepavloff at gmail.com
> <mailto:sepavloff at gmail.com>
> <mailto:sepavloff at gmail.com <mailto:sepavloff at gmail.com>>>:
>
> Any feedback?
>
> Thanks,
> --Serge
>
> 2015-12-11 20:24 GMT+06:00 Serge Pavlov
> <sepavloff at gmail.com <mailto:sepavloff at gmail.com>
> <mailto:sepavloff at gmail.com <mailto:sepavloff at gmail.com>>>:
>
>
> sepavloff created this revision.
> sepavloff added a subscriber: cfe-commits.
>
> Llvm module object is shared between CodeGenerator and
> BackendConsumer,
> in both classes it is stored as std::unique_ptr, which
> is not a good
> design solution and can cause double deletion error.
> Usually it does
> not occur because in
> BackendConsumer::HandleTranslationUnit the
> ownership of CodeGenerator over the module is taken
> away. If however
> this method is not called, the module is deleted twice and
> compiler crashes.
>
> As the module owned by BackendConsumer is always the
> same as
> CodeGenerator
> has, local copy of llvm module can be removed from
> BackendGenerator.
>
> http://reviews.llvm.org/D15450
>
> Files:
> lib/CodeGen/CodeGenAction.cpp
>
> Index: lib/CodeGen/CodeGenAction.cpp
>
> ===================================================================
> --- lib/CodeGen/CodeGenAction.cpp
> +++ lib/CodeGen/CodeGenAction.cpp
> @@ -73,7 +73,6 @@
>
> std::unique_ptr<CodeGenerator> Gen;
>
> - std::unique_ptr<llvm::Module> TheModule;
> SmallVector<std::pair<unsigned,
> std::unique_ptr<llvm::Module>>, 4>
> LinkModules;
>
> @@ -97,7 +96,10 @@
> this->LinkModules.push_back(
> std::make_pair(I.first,
> std::unique_ptr<llvm::Module>(I.second)));
> }
> - std::unique_ptr<llvm::Module> takeModule() { return
> std::move(TheModule); }
> + llvm::Module *getModule() const { return
> Gen->GetModule(); }
> + std::unique_ptr<llvm::Module> takeModule() {
> + return
> std::unique_ptr<llvm::Module>(Gen->ReleaseModule());
> + }
> void releaseLinkModules() {
> for (auto &I : LinkModules)
> I.second.release();
> @@ -117,8 +119,6 @@
>
> Gen->Initialize(Ctx);
>
> - TheModule.reset(Gen->GetModule());
> -
> if (llvm::TimePassesIsEnabled)
> LLVMIRGeneration.stopTimer();
> }
> @@ -165,27 +165,14 @@
> }
>
> // Silently ignore if we weren't initialized
> for some
> reason.
> - if (!TheModule)
> - return;
> -
> - // Make sure IR generation is happy with the
> module. This
> is released by
> - // the module provider.
> - llvm::Module *M = Gen->ReleaseModule();
> - if (!M) {
> - // The module has been released by IR gen on
> failures,
> do not double
> - // free.
> - TheModule.release();
> + if (!getModule())
> return;
> - }
> -
> - assert(TheModule.get() == M &&
> - "Unexpected module change during IR
> generation");
>
> // Link LinkModule into this module if present,
> preserving its validity.
> for (auto &I : LinkModules) {
> unsigned LinkFlags = I.first;
> llvm::Module *LinkModule = I.second.get();
> - if (Linker::linkModules(*M, *LinkModule,
> + if (Linker::linkModules(*getModule(), *LinkModule,
> [=](const
> DiagnosticInfo &DI) {
>
> linkerDiagnosticHandler(DI,
> LinkModule, Diags);
> },
> @@ -195,7 +182,7 @@
>
> // Install an inline asm handler so that
> diagnostics get
> printed through
> // our diagnostics hooks.
> - LLVMContext &Ctx = TheModule->getContext();
> + LLVMContext &Ctx = getModule()->getContext();
> LLVMContext::InlineAsmDiagHandlerTy OldHandler =
> Ctx.getInlineAsmDiagnosticHandler();
> void *OldContext =
> Ctx.getInlineAsmDiagnosticContext();
> @@ -208,7 +195,7 @@
>
> EmitBackendOutput(Diags, CodeGenOpts,
> TargetOpts, LangOpts,
>
> C.getTargetInfo().getDataLayoutString(),
> - TheModule.get(), Action,
> AsmOutStream);
> + getModule(), Action,
> AsmOutStream);
>
> Ctx.setInlineAsmDiagnosticHandler(OldHandler,
> OldContext);
>
>
>
>
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org <mailto:cfe-commits at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
> --
> Jon Roelofs
> jonathan at codesourcery.com <mailto:jonathan at codesourcery.com>
> CodeSourcery / Mentor Embedded
>
>
--
Jon Roelofs
jonathan at codesourcery.com
CodeSourcery / Mentor Embedded
More information about the cfe-commits
mailing list