[bugpoint] Don't stop reducing when verification fails, keep trying

[vsk]

Summary

Instead of calling exit() when a verification step fails we should keep
reducing the input. This is in line with what we do in other cases when
opt fails (see performFinalCleanups(), extractLoop(), etc).

I have a test case that provides coverage for this change, but it's
large and (for now) relies on an uncommitted assertion in SelectionDAG.

I don't think it's worthwhile to test this change in-tree because a
bugfix to a bugpoint reducer could break the test. (It would be fair to
say that the real problem I'm having is that some reducer is buggy, and
that I could just try to fix that instead, but unless we instantly fix
all reducer bugs having this "keep going" behavior sgtm. Thoughts?)

[vsk]

• Fixed up a test.

[davide]

Did you get a sense of why we can't remove the instructions? This is probably something we want anyway, but there could be another bug in llvm (or bugpoint itself, as instruction deletion is always supposed to succeed).

[davide]

marking as "requesting changes" so it gets out of my queue

[vsk]

In D43113#1003620, @davide wrote:

Did you get a sense of why we can't remove the instructions? This is probably something we want anyway, but there could be another bug in llvm (or bugpoint itself, as instruction deletion is always supposed to succeed).

Yep, it was completely my fault. I specified the wrong path to my opt binary, so runPassesOn() silently failed with 'file not found'. I added a warning: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20180205/523911.html.

But.. I haven't devoted any time to figuring out why the bugpoint invocation in unsymbolized.ll hit the "exit(1)" path before this change. I'll need to figure that out before landing this.

test/BugPoint/unsymbolized.ll

 ; REQUIRES: loadable_module
 ; RUN: echo "import sys" > %t.py
 ; RUN: echo "print('args = ' + str(sys.argv))" >> %t.py
 ; RUN: echo "exit(1)" >> %t.py
-; RUN: not bugpoint -load %llvmshlibdir/BugpointPasses%shlibext %s -output-prefix %t -bugpoint-crashcalls -opt-command="%python" -opt-args %t.py | FileCheck %s
+; RUN: bugpoint -load %llvmshlibdir/BugpointPasses%shlibext %s -output-prefix %t -bugpoint-crashcalls -opt-command="%python" -opt-args %t.py | FileCheck %s
 ; RUN: not --crash opt -load %llvmshlibdir/BugpointPasses%shlibext %s -bugpoint-crashcalls -disable-symbolication 2>&1 | FileCheck --check-prefix=CRASH %s
 
 ; Test that bugpoint disables symbolication on the opt tool to reduce runtime overhead when opt crashes
 ; CHECK: args = {{.*}}'-disable-symbolication'
 
 ; Test that opt, when it crashes & is passed -disable-symbolication, doesn't symbolicate.
 ; In theory this test should maybe be in test/tools/opt or
 ; test/Transforms, but since there doesn't seem to be another convenient way to
 ; crash opt, apart from the BugpointPasses dynamic plugin, this is the spot for
 ; now.
 ; CRASH-NOT: Signals.inc
 
 define void @f() {
   call void @f()
   ret void
 }

tools/bugpoint/BugDriver.h

public:
   /// "bugpoint-ID.bc".
   ///
   void EmitProgressBitcode(const Module *M, const std::string &ID,
                            bool NoFlyer = false) const;
 
   /// This method clones the current Program and deletes the specified
   /// instruction from the cloned module.  It then runs a series of cleanup
   /// passes (ADCE and SimplifyCFG) to eliminate any code which depends on the
-  /// value. The modified module is then returned.
+  /// value. The modified module is then returned. If deletion causes module
+  /// verification to fail, nullptr is returned.
   ///
   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.

tools/bugpoint/CrashDebugger.cpp

for (auto &F : *M) {
     ToProcess.clear();
   }
   // Verify we didn't break anything
   std::vector<std::string> Passes;
   Passes.push_back("verify");
   std::unique_ptr<Module> New = BD.runPassesOn(M.get(), Passes);
   if (!New) {
     errs() << "verify failed!\n";
-    exit(1);
+    return false;
   }
   M = std::move(New);
 
   // Try running on the hacked up program...
   if (TestFn(BD, M.get())) {
     BD.setNewProgram(M.release()); // It crashed, keep the trimmed version...
 
     // Make sure to use basic block pointers that point into the now-current
for (auto &F : *M) {
     ToProcess.clear();
   }
   // Verify we didn't break anything
   std::vector<std::string> Passes;
   Passes.push_back("verify");
   std::unique_ptr<Module> New = BD.runPassesOn(M.get(), Passes);
   if (!New) {
     errs() << "verify failed!\n";
-    exit(1);
+    return false;
   }
   M = std::move(New);
 
   // Try running on the hacked up program...
   if (TestFn(BD, M.get())) {
     BD.setNewProgram(M.release()); // It crashed, keep the trimmed version...
 
     // Make sure to use basic block pointers that point into the now-current
for (Function::iterator BBIt = F.begin(); BBIt != F.end();) {
       simplifyCFG(&*BBIt++, TTI);
     }
   // Verify we didn't break anything
   std::vector<std::string> Passes;
   Passes.push_back("verify");
   std::unique_ptr<Module> New = BD.runPassesOn(M.get(), Passes);
   if (!New) {
     errs() << "verify failed!\n";
-    exit(1);
+    return false;
   }
   M = std::move(New);
 
   // Try running on the hacked up program...
   if (TestFn(BD, M.get())) {
     BD.setNewProgram(M.release()); // It crashed, keep the trimmed version...
 
     // Make sure to use basic block pointers that point into the now-current
for (Module::const_iterator FI = BD.getProgram()->begin(),
 
               if (I->isEHPad() || I->getType()->isTokenTy() ||
                   I->isSwiftError())
                 continue;
 
               outs() << "Checking instruction: " << *I;
               std::unique_ptr<Module> M =
                   BD.deleteInstructionFromProgram(&*I, Simplification);
+              if (!M) // We couldn't delete this instruction. Keep trying.
+                continue;
 
               // Find out if the pass still crashes on this pass...
               if (TestFn(BD, M.get())) {
                 // Yup, it does, we delete the old module, and continue trying
                 // to reduce the testcase...
                 BD.setNewProgram(M.release());
                 InstructionsToSkipBeforeDeleting = CurInstructionNum;
                 goto TryAgain; // I wish I had a multi-level break here!

tools/bugpoint/ExtractFunction.cpp

if (Simplification > 1 && !NoDCE)
     Passes.push_back("dce");
   if (Simplification && !DisableSimplifyCFG)
     Passes.push_back("simplifycfg"); // Delete dead control flow
 
   Passes.push_back("verify");
   std::unique_ptr<Module> New = runPassesOn(Clone.get(), Passes);
   if (!New) {
     errs() << "Instruction removal failed.  Sorry. :(  Please report a bug!\n";
-    exit(1);
+    return nullptr;
   }
   return New;
 }
 
 std::unique_ptr<Module>
 BugDriver::performFinalCleanups(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)