[Bugpoint] Use `unique_ptr` correctly.

[bryant]

Summary

$ ../debugbuild/bin/bugpoint -pdse orig/bugpoint-passinput.bc -safe-run-custom -run-custom -exec-command ./rebuild-clang --opt-command ../build/bin/opt
Read input file      : 'orig/bugpoint-passinput.bc'
*** All input ok
Running selected passes on program to test for crash: Success!
Initializing execution environment: Found command in: ./rebuild-clang
Running the code generator to test for a crash: 
Generating reference output from raw program: 
Reference output is: bugpoint.reference.out-67652c0

*** Checking the code generator...

*** Output matches: Debugging miscompilation!
Checking to see if '' compiles correctly:  yup.
Checking to see if '-pdse' compiles correctly:  nope.

*** Found miscompiling pass: -pdse
Emitted bitcode to 'bugpoint-passinput.bc'

*** You can reproduce the problem with: opt bugpoint-passinput.bc -pdse
Checking to see if the program is misoptimized when these functions are run through the pass: _ZN5clang6Parser25isCXXDeclarationStatementEv _ZN5clang6Parser22isCXXSimpleDeclarationEb _ZN5clang6Parser25isCXXDeclarationSpecifierEN
S0_8TPResultEPb _ZN5clang6Parser25TryParseSimpleDeclarationEb _ZN5clang6Parser30TryConsumeDeclarationSpecifierEv _ZN5clang6Parser26TryParseProtocolQualifiersEv _ZN5clang6Parser26TryParseInitDeclaratorListEv _ZN5clang6Parser18Try
ParseDeclaratorEbb _ZN5clang6Parser40isCXXConditionDeclarationOrInitStatementEb _ZN5clang6Parser40ConditionDeclarationOrInitStatementState17markNotExpressionEv... <27 total>
  Optimizing functions being tested: done.
  Checking to see if the merged program executes correctly:  nope.
Checking to see if the program is misoptimized when these functions are run through the pass: _ZN5clang6Parser18TryParseOperatorIdEv _ZN5clang6Parser26TryParseFunctionDeclaratorEv _ZN5clang6Parser23isCXXFunctionDeclaratorEPb _ZN
5clang6Parser25TryParseBracketDeclaratorEv _ZN5clang6Parser33isExpressionOrTypeSpecifierSimpleENS_3tok9TokenKindE _ZN5clang6Parser21isTentativelyDeclaredEPNS_14IdentifierInfoE _ZN5clang6Parser23TryParseTypeofSpecifierEv _ZN5clan
g6Parser30isCXXDeclarationSpecifierATypeEv _ZN5clang6Parser34TryParseParameterDeclarationClauseEPbb _ZN5clang6Parser22TentativeParsingAction6RevertEv... <14 total>
  Optimizing functions being tested: done.
  Checking to see if the merged program executes correctly:  yup.
Checking to see if the program is misoptimized when these functions are run through the pass: _ZN5clang6Parser25isCXXDeclarationStatementEv _ZN5clang6Parser22isCXXSimpleDeclarationEb _ZN5clang6Parser25isCXXDeclarationSpecifierEN
S0_8TPResultEPb _ZN5clang6Parser25TryParseSimpleDeclarationEb _ZN5clang6Parser30TryConsumeDeclarationSpecifierEv _ZN5clang6Parser26TryParseProtocolQualifiersEv _ZN5clang6Parser26TryParseInitDeclaratorListEv _ZN5clang6Parser18Try
ParseDeclaratorEbb _ZN5clang6Parser40isCXXConditionDeclarationOrInitStatementEb _ZN5clang6Parser40ConditionDeclarationOrInitStatementState17markNotExpressionEv... <13 total>
  Optimizing functions being tested: done.
  Checking to see if the merged program executes correctly:  nope.
Checking to see if the program is misoptimized when these functions are run through the pass: _ZN5clang6Parser26TryParseInitDeclaratorListEv _ZN5clang6Parser18TryParseDeclaratorEbb _ZN5clang6Parser40isCXXConditionDeclarationOrIn
itStatementEb _ZN5clang6Parser40ConditionDeclarationOrInitStatementState17markNotExpressionEv _ZN5clang6Parser11isCXXTypeIdENS0_25TentativeCXXTypeIdContextERb _ZN5clang6Parser25isCXX11AttributeSpecifierEbb _ZN5clang6Parser22TryP
arsePtrOperatorSeqEv
  Optimizing functions being tested: done.
  Checking to see if the merged program executes correctly:  nope.
Checking to see if the program is misoptimized when these functions are run through the pass: _ZN5clang6Parser40ConditionDeclarationOrInitStatementState17markNotExpressionEv _ZN5clang6Parser11isCXXTypeIdENS0_25TentativeCXXTypeId
ContextERb _ZN5clang6Parser25isCXX11AttributeSpecifierEbb _ZN5clang6Parser22TryParsePtrOperatorSeqEv
  Optimizing functions being tested: done.
  Checking to see if the merged program executes correctly:  yup.
Checking to see if the program is misoptimized when these functions are run through the pass: _ZN5clang6Parser26TryParseInitDeclaratorListEv _ZN5clang6Parser18TryParseDeclaratorEbb _ZN5clang6Parser40isCXXConditionDeclarationOrIn
itStatementEb
  Optimizing functions being tested: done.
  Checking to see if the merged program executes correctly:  nope.
Checking to see if the program is misoptimized when these functions are run through the pass: _ZN5clang6Parser18TryParseDeclaratorEbb _ZN5clang6Parser40isCXXConditionDeclarationOrInitStatementEb
  Optimizing functions being tested: done.
  Checking to see if the merged program executes correctly:  nope.
Checking to see if the program is misoptimized when this function is run through the pass: _ZN5clang6Parser40isCXXConditionDeclarationOrInitStatementEb
  Optimizing functions being tested: done.
  Checking to see if the merged program executes correctly:  nope.

*** The following function is being miscompiled:  _ZN5clang6Parser40isCXXConditionDeclarationOrInitStatementEb
Extracted a loop from the breaking portion of the program.
  Testing after loop extraction:
#0 0x00007fb43fd472b9 llvm::sys::PrintStackTrace(llvm::raw_ostream&) /llvm/debugbuild/../lib/Support/Unix/Signals.inc:402:11
#1 0x00007fb43fd474d9 PrintStackTraceSignalHandler(void*) /llvm/debugbuild/../lib/Support/Unix/Signals.inc:466:1
#2 0x00007fb43fd443c7 llvm::sys::RunSignalHandlers() /llvm/lib/Support/Signals.cpp:0:5
#3 0x00007fb43fd47860 SignalHandler(int) /llvm/debugbuild/../lib/Support/Unix/Signals.inc:256:1
#4 0x00007fb43f2ea8d0 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0xf8d0)
#5 0x00007fb43f88c79c llvm::Module::getContext() const /llvm/debugbuild/../include/llvm/IR/Module.h:222:44
#6 0x00007fb43f8a9551 llvm::CloneModule(llvm::Module const*, llvm::ValueMap<llvm::Value const*, llvm::WeakVH, llvm::ValueMapConfig<llvm::Value const*, llvm::sys::SmartMutex<false> > >&, llvm::function_ref<bool (llvm::GlobalValue
 const*)>) /llvm/lib/Transforms/Utils/CloneModule.cpp:53:7
#7 0x00007fb43f8a94e0 llvm::CloneModule(llvm::Module const*, llvm::ValueMap<llvm::Value const*, llvm::WeakVH, llvm::ValueMapConfig<llvm::Value const*, llvm::sys::SmartMutex<false> > >&) /llvm/lib/Transforms/Utils
/CloneModule.cpp:45:3
#8 0x00000000002d0b11 ExtractLoops(llvm::BugDriver&, llvm::Expected<bool> (*)(llvm::BugDriver&, std::__1::unique_ptr<llvm::Module, std::__1::default_delete<llvm::Module> >, std::__1::unique_ptr<llvm::Module, std::__1::default_de
lete<llvm::Module> >), std::__1::vector<llvm::Function*, std::__1::allocator<llvm::Function*> >&) /llvm/tools/bugpoint/Miscompilation.cpp:0:9
#9 0x00000000002c70e6 DebugAMiscompilation(llvm::BugDriver&, llvm::Expected<bool> (*)(llvm::BugDriver&, std::__1::unique_ptr<llvm::Module, std::__1::default_delete<llvm::Module> >, std::__1::unique_ptr<llvm::Module, std::__1::de
fault_delete<llvm::Module> >)) /llvm/tools/bugpoint/Miscompilation.cpp:0:26
#10 0x00000000002c5dfd llvm::BugDriver::debugMiscompilation() /llvm/tools/bugpoint/Miscompilation.cpp:0:7
#11 0x000000000024c734 llvm::BugDriver::run() /llvm/tools/bugpoint/BugDriver.cpp:0:19
#12 0x0000000000313c11 main /llvm/tools/bugpoint/bugpoint.cpp:0:19
#13 0x00007fb43e929b45 __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b45)
#14 0x0000000000249029 _start (../debugbuild/bin/bugpoint+0x249029)
Stack dump:
0.      Program arguments: ../debugbuild/bin/bugpoint -pdse orig/bugpoint-passinput.bc -safe-run-custom -run-custom -exec-command ./rebuild-clang --opt-command ../build/bin/opt 
Segmentation fault

All calls made to testMergedProgram fully expect to retain ownership of the arguments.

Adding Rafael because of r255087.

[• rafael]

tools/bugpoint/Miscompilation.cpp

228–234

Can you pass a const ref for M1 and M2?

[mehdi_amini]

LGTM with a const ref
(and please don't use the full log as a commit message, keep it simple and descriptive).

[davide]

LGTM

[bryant]

const refs.

tools/bugpoint/Miscompilation.cpp

 } // end anonymous namespace
 
 /// Given two modules, link them together and run the program, checking to see
 /// if the program matches the diff. If there is an error, return NULL. If not,
 /// return the merged module. The Broken argument will be set to true if the
 /// output is different. If the DeleteInputs argument is set to true then this
 /// function deletes both input modules before it returns.
 ///
-static Expected<std::unique_ptr<Module>>
-testMergedProgram(const BugDriver &BD, std::unique_ptr<Module> M1,
-                  std::unique_ptr<Module> M2, bool &Broken) {
-  if (Linker::linkModules(*M1, std::move(M2)))
+static Expected<std::unique_ptr<Module>> testMergedProgram(const BugDriver &BD,
+                                                           const Module &M1,
+                                                           const Module &M2,
+                                                           bool &Broken) {
+  // Resulting merge of M1 and M2.
+  auto Merged = CloneModule(&M1);
+  if (Linker::linkModules(*Merged, CloneModule(&M2)))
     // TODO: Shouldn't we thread the error up instead of exiting?
     exit(1);
 
   // Execute the program.
-  Expected<bool> Diff = BD.diffProgram(M1.get(), "", "", false);
+  Expected<bool> Diff = BD.diffProgram(Merged.get(), "", "", false);
   if (Error E = Diff.takeError())
     return std::move(E);
   Broken = *Diff;
-  return std::move(M1);
+  return std::move(Merged);
 }
 
 /// TestFuncs - split functions in a Module into two groups: those that are
 /// under consideration for miscompilation vs. those that are not, and test
 /// accordingly. Each group of functions becomes a separate Module.
 ///
 Expected<bool>
 ReduceMiscompilingFunctions::TestFuncs(const std::vector<Function *> &Funcs) {
while (1) {
 
     // Bugpoint is intentionally not very trusting of LLVM transformations.  In
     // particular, we're not going to assume that the loop extractor works, so
     // we're going to test the newly loop extracted program to make sure nothing
     // has broken.  If something broke, then we'll inform the user and stop
     // extraction.
     AbstractInterpreter *AI = BD.switchToSafeInterpreter();
     bool Failure;
-    Expected<std::unique_ptr<Module>> New =
-        testMergedProgram(BD, std::move(ToOptimizeLoopExtracted),
-                          std::move(ToNotOptimize), Failure);
+    Expected<std::unique_ptr<Module>> New = testMergedProgram(
+        BD, *ToOptimizeLoopExtracted, *ToNotOptimize, Failure);
     if (Error E = New.takeError())
       return std::move(E);
     if (!*New)
       return false;
 
     // Delete the original and set the new program.
     Module *Old = BD.swapProgramIn(New->release());
     for (unsigned i = 0, e = MiscompiledFunctions.size(); i != e; ++i)
if (!Optimized) {
     if (Error E = BD.debugOptimizerCrash())
       return std::move(E);
     return false;
   }
   outs() << "done.\n";
 
   outs() << "  Checking to see if the merged program executes correctly: ";
   bool Broken;
-  auto Result =
-      testMergedProgram(BD, std::move(Optimized), std::move(Safe), Broken);
+  auto Result = testMergedProgram(BD, *Optimized, *Safe, Broken);
   if (Error E = Result.takeError())
     return std::move(E);
   if (auto New = std::move(*Result)) {
     outs() << (Broken ? " nope.\n" : " yup.\n");
     // Delete the original and set the new program.
     delete BD.swapProgramIn(New.release());
   }
   return Broken;