[PATCH] D15450: Avoid double deletion in Clang driver.

[Serge Pavlov via cfe-commits]

Any feedback?

Thanks,
--Serge

2015-12-11 20:24 GMT+06:00 Serge Pavlov <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);
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160111/bfb84fa1/attachment-0001.html>