Avoid double deletion in Clang driver.

[sepavloff]

Summary

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.

[sepavloff]

Any feedback?

Thanks,
--Serge

[sepavloff]

Can somebody have a look at this fix?
Thank you!

--Serge

[sepavloff]

Added regression test.

[sepavloff]

Any feedback?

Thanks,
--Serge

[sepavloff]

CodeGen action defined in the unit test does not call HandleTranslationUnit,
so the hack implemented in BackendConsumer::HandleTranslationUnit does
not run and clang crashes. With the fix no crash occurs.

Thanks,
--Serge

[yaron.keren]

It certainly makes sense to redirect the module request to its owner instead of duplicating it in a local copy.

There may be a change in behaviour here, pre-patch CodeGenAction::EndSourceFileAction() would take the BackendConsumer Module, keeping CodeGen Module intact whereas post-patch it takes the CodeGen Module.
How does this work out?

[sepavloff]

In D15450#354606, @yaron.keren wrote:

It certainly makes sense to redirect the module request to its owner instead of duplicating it in a local copy.

It is not even a duplication, it is simultaneous sharing by two unique_ptr's.

There may be a change in behaviour here, pre-patch CodeGenAction::EndSourceFileAction() would take the BackendConsumer Module, keeping CodeGen Module intact whereas post-patch it takes the CodeGen Module.
How does this work out?

It should not be a problem. CodeGenAction::EndSourceFileAction() is called when code generation is finished, so taking ownership from GodeGen looks safe. Anyway, module must not be owned by two unique_ptr's.

There are at least 3 unique_ptr's that owns the same module object:

• CodeGenAction::TheModule
• BackendConsumer::TheModule
• CodeGeneratorImpl::M

It looks like CodeGenAction::TheModule does not cause any problem, it takes ownership according to usual unique_ptr semantics. But the last two variables owns the same object simultaneously, which violates semantics of unique_ptr. The patch fixes ownership by allowing only CodeGeneratorImpl::M to be an owner.

[yaron.keren]

Would be nice to have CodeGenAction::TheModule redirect to CodeGeneratorImpl::M if possible, but that's for another patch. LGTM.

lib/CodeGen/CodeGenAction.cpp

class BackendConsumer : public ASTConsumer {
     const LangOptions &LangOpts;
     raw_pwrite_stream *AsmOutStream;
     ASTContext *Context;
 
     Timer LLVMIRGeneration;
 
     std::unique_ptr<CodeGenerator> Gen;
 
-    std::unique_ptr<llvm::Module> TheModule;
     SmallVector<std::pair<unsigned, std::unique_ptr<llvm::Module>>, 4>
         LinkModules;
 
   public:
     BackendConsumer(
         BackendAction Action, DiagnosticsEngine &Diags,
         const HeaderSearchOptions &HeaderSearchOpts,
         const PreprocessorOptions &PPOpts, const CodeGenOptions &CodeGenOpts,
         const TargetOptions &TargetOpts, const LangOptions &LangOpts,
         bool TimePasses, const std::string &InFile,
         const SmallVectorImpl<std::pair<unsigned, llvm::Module *>> &LinkModules,
         raw_pwrite_stream *OS, LLVMContext &C,
         CoverageSourceInfo *CoverageInfo = nullptr)
         : Diags(Diags), Action(Action), CodeGenOpts(CodeGenOpts),
           TargetOpts(TargetOpts), LangOpts(LangOpts), AsmOutStream(OS),
           Context(nullptr), LLVMIRGeneration("LLVM IR Generation Time"),
           Gen(CreateLLVMCodeGen(Diags, InFile, HeaderSearchOpts, PPOpts,
                                 CodeGenOpts, C, CoverageInfo)) {
       llvm::TimePassesIsEnabled = TimePasses;
       for (auto &I : LinkModules)
         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();
     }
 
     void HandleCXXStaticMemberVarInstantiation(VarDecl *VD) override {
       Gen->HandleCXXStaticMemberVarInstantiation(VD);
     }
 
     void Initialize(ASTContext &Ctx) override {
       assert(!Context && "initialized multiple times");
 
       Context = &Ctx;
 
       if (llvm::TimePassesIsEnabled)
         LLVMIRGeneration.startTimer();
 
       Gen->Initialize(Ctx);
 
-      TheModule.reset(Gen->GetModule());
-
       if (llvm::TimePassesIsEnabled)
         LLVMIRGeneration.stopTimer();
     }
 
     bool HandleTopLevelDecl(DeclGroupRef D) override {
       PrettyStackTraceDecl CrashInfo(*D.begin(), SourceLocation(),
                                      Context->getSourceManager(),
                                      "LLVM IR generation of declaration");
void HandleTranslationUnit(ASTContext &C) override {
 
         Gen->HandleTranslationUnit(C);
 
         if (llvm::TimePassesIsEnabled)
           LLVMIRGeneration.stopTimer();
       }
 
       // Silently ignore if we weren't initialized for some reason.
-      if (!TheModule)
+      if (!getModule())
         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();
-        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);
                                 },
                                 LinkFlags))
           return;
       }
 
       // 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();
       Ctx.setInlineAsmDiagnosticHandler(InlineAsmDiagHandler, this);
 
       LLVMContext::DiagnosticHandlerTy OldDiagnosticHandler =
           Ctx.getDiagnosticHandler();
       void *OldDiagnosticContext = Ctx.getDiagnosticContext();
       Ctx.setDiagnosticHandler(DiagnosticHandler, this);
 
       EmitBackendOutput(Diags, CodeGenOpts, TargetOpts, LangOpts,
                         C.getTargetInfo().getDataLayoutString(),
-                        TheModule.get(), Action, AsmOutStream);
+                        getModule(), Action, AsmOutStream);
 
       Ctx.setInlineAsmDiagnosticHandler(OldHandler, OldContext);
 
       Ctx.setDiagnosticHandler(OldDiagnosticHandler, OldDiagnosticContext);
     }
 
     void HandleTagDeclDefinition(TagDecl *D) override {
       PrettyStackTraceDecl CrashInfo(D, SourceLocation(),