[bugpoint] Fix crash when testing for miscompilation

[ajanik]

Summary

Method BugDriver::performFinalCleanups(...) would delete Module object it worked on, which was also deleted by its caller (e.g. TestCodeGenerator(...)). Changed the code to avoid double delete and make Module ownership slightly clearer.

[• espindola]

LGTM

[davide]

LGTM

[ajanik]

@espindola @davide, could you commit this change?

[• espindola]

330763

bugpoint/BugDriver.h

public:
   std::unique_ptr<Module> deleteInstructionFromProgram(const Instruction *I,
                                                        unsigned Simp);
 
   /// This method clones the current Program and performs a series of cleanups
   /// intended to get rid of extra cruft on the module. If the
   /// MayModifySemantics argument is true, then the cleanups is allowed to
   /// modify how the code behaves.
   ///
-  std::unique_ptr<Module> performFinalCleanups(Module *M,
+  std::unique_ptr<Module> performFinalCleanups(std::unique_ptr<Module> M,
                                                bool MayModifySemantics = false);
 
   /// Given a module, extract up to one loop from it into a new function. This
   /// returns null if there are no extractable loops in the program or if the
   /// loop extractor crashes.
   std::unique_ptr<Module> extractLoop(Module *M);
 
   /// Extract all but the specified basic blocks into their own functions. The

bugpoint/CrashDebugger.cpp

if (!BugpointIsInterrupted) {
         return E;
     }
     BD.EmitProgressBitcode(BD.getProgram(), "reduced-named-md");
   }
 
   // Try to clean up the testcase by running funcresolve and globaldce...
   if (!BugpointIsInterrupted) {
     outs() << "\n*** Attempting to perform final cleanups: ";
-    Module *M = CloneModule(BD.getProgram()).release();
-    M = BD.performFinalCleanups(M, true).release();
+    std::unique_ptr<Module> M = CloneModule(BD.getProgram());
+    M = BD.performFinalCleanups(std::move(M), true);
 
     // Find out if the pass still crashes on the cleaned up program...
-    if (TestFn(BD, M)) {
-      BD.setNewProgram(M); // Yup, it does, keep the reduced version...
-    } else {
-      delete M;
+    if (TestFn(BD, M.get())) {
+      BD.setNewProgram(M.get()); // Yup, it does, keep the reduced version...
     }
   }
 
   BD.EmitProgressBitcode(BD.getProgram(), "reduced-simplified");
 
   return Error::success();
 }
 

bugpoint/ExtractFunction.cpp

BugDriver::deleteInstructionFromProgram(const Instruction *I,
   if (!New) {
     errs() << "Instruction removal failed.  Sorry. :(  Please report a bug!\n";
     exit(1);
   }
   return New;
 }
 
 std::unique_ptr<Module>
-BugDriver::performFinalCleanups(Module *M, bool MayModifySemantics) {
+BugDriver::performFinalCleanups(std::unique_ptr<Module> M, bool MayModifySemantics) {
   // Make all functions external, so GlobalDCE doesn't delete them...
   for (Module::iterator I = M->begin(), E = M->end(); I != E; ++I)
     I->setLinkage(GlobalValue::ExternalLinkage);
 
   std::vector<std::string> CleanupPasses;
   CleanupPasses.push_back("globaldce");
 
   if (MayModifySemantics)
     CleanupPasses.push_back("deadarghaX0r");
   else
     CleanupPasses.push_back("deadargelim");
 
-  std::unique_ptr<Module> New = runPassesOn(M, CleanupPasses);
+  std::unique_ptr<Module> New = runPassesOn(M.get(), CleanupPasses);
   if (!New) {
     errs() << "Final cleanups failed.  Sorry. :(  Please report a bug!\n";
     return nullptr;
   }
-  delete M;
   return New;
 }
 
 std::unique_ptr<Module> BugDriver::extractLoop(Module *M) {
   std::vector<std::string> LoopExtractPasses;
   LoopExtractPasses.push_back("loop-extract-single");
 
   std::unique_ptr<Module> NewM = runPassesOn(M, LoopExtractPasses);

bugpoint/Miscompilation.cpp

Error BugDriver::debugMiscompilation() {
   EmitProgressBitcode(ToOptimize, "tooptimize");
   delete ToOptimize; // Delete hacked module.
 
   return Error::success();
 }
 
 /// Get the specified modules ready for code generator testing.
 ///
-static void CleanupAndPrepareModules(BugDriver &BD,
-                                     std::unique_ptr<Module> &Test,
-                                     Module *Safe) {
+static std::unique_ptr<Module>
+CleanupAndPrepareModules(BugDriver &BD, std::unique_ptr<Module> Test,
   // Clean up the modules, removing extra cruft that we don't need anymore...
-  Test = BD.performFinalCleanups(Test.get());
+  Test = BD.performFinalCleanups(std::move(Test));
 
   // If we are executing the JIT, we have several nasty issues to take care of.
   if (!BD.isExecutingJIT())
-    return;
+    return Test;
 
   // First, if the main function is in the Safe module, we must add a stub to
   // the Test module to call into it.  Thus, we create a new function `main'
   // which just calls the old one.
   if (Function *oldMain = Safe->getFunction("main"))
     if (!oldMain->isDeclaration()) {
       // Rename it
       oldMain->setName("llvm_bugpoint_old_main");
if (F->isDeclaration() && !F->use_empty() && &*F != resolverFunc &&
       }
     }
   }
 
   if (verifyModule(*Test) || verifyModule(*Safe)) {
     errs() << "Bugpoint has a bug, which corrupted a module!!\n";
     abort();
   }
+
+  return Test;
 }
 
 /// This is the predicate function used to check to see if the "Test" portion of
 /// the program is miscompiled by the code generator under test.  If so, return
 /// true.  In any case, both module arguments are deleted.
 ///
 static Expected<bool> TestCodeGenerator(BugDriver &BD,
                                         std::unique_ptr<Module> Test,
                                         std::unique_ptr<Module> Safe) {
-  CleanupAndPrepareModules(BD, Test, Safe.get());
+  Test = CleanupAndPrepareModules(BD, std::move(Test), Safe.get());
 
   SmallString<128> TestModuleBC;
   int TestModuleFD;
   std::error_code EC = sys::fs::createTemporaryFile("bugpoint.test", "bc",
                                                     TestModuleFD, TestModuleBC);
   if (EC) {
     errs() << BD.getToolName()
            << "Error making unique filename: " << EC.message() << "\n";
Error BugDriver::debugCodeGenerator() {
 
   // Split the module into the two halves of the program we want.
   ValueToValueMapTy VMap;
   std::unique_ptr<Module> ToNotCodeGen = CloneModule(getProgram(), VMap);
   std::unique_ptr<Module> ToCodeGen =
       SplitFunctionsOutOfModule(ToNotCodeGen.get(), *Funcs, VMap);
 
   // Condition the modules
-  CleanupAndPrepareModules(*this, ToCodeGen, ToNotCodeGen.get());
+  ToCodeGen = CleanupAndPrepareModules(*this, std::move(ToCodeGen), ToNotCodeGen.get());
 
   SmallString<128> TestModuleBC;
   int TestModuleFD;
   std::error_code EC = sys::fs::createTemporaryFile("bugpoint.test", "bc",
                                                     TestModuleFD, TestModuleBC);
   if (EC) {
     errs() << getToolName() << "Error making unique filename: " << EC.message()
            << "\n";