[bugpoint] Actually skip a modules that cause verifier errors

[loladiro]

Summary

When reducing modules, it is possible for us to introduce changes
that fail the verifier. The intent was for these modules to get skipped
such that we would try a different reduction instead. However, it
seems that while we did the verification, we nevertheless proceeded
with the broken module. Make sure to actually check for a broken module.

[aprantl]

Since we regressed on this before: could you come up with a testcase for this?

[loladiro]

Yeah, I've been thinking about what the best way to test this is. It seems somewhat hard to reliably trigger
a verifier fault by removing an instruction - llvm.stackprotector is the one I see most commonly, but I don't
really want to make the test depend on that behavior. One option is to teach bugpoint to use a custom verifier
pass and then write a custom verifier pass that e.g. checks for the presence of a call. That seems like a decently
attractive option, because it would be a useful feature in its own right. Some frontends have their own invariants
and corresponding verifier passes that they want to maintain in their early passes. Would be nice if bugpoint could
support that. The only thing I'm not quite sure off is to how to do this nicely. Presumably, we could use the analysis
name as a pass and run that through the pass manager. I'm just not sure how to get the analysis result out of that.

tools/bugpoint/CrashDebugger.cpp

for (Function::iterator FI = MI->begin(), FE = MI->end(); FI != FE; ++FI)
             !Inst->isEHPad() && !Inst->getType()->isTokenTy() &&
             !Inst->isSwiftError()) {
           if (!Inst->getType()->isVoidTy())
             Inst->replaceAllUsesWith(UndefValue::get(Inst->getType()));
           Inst->eraseFromParent();
         }
       }
 
-  // Verify that this is still valid.
-  legacy::PassManager Passes;
-  Passes.add(createVerifierPass(/*FatalErrors=*/false));
-  Passes.run(*M);
-
-  // Try running on the hacked up program...
-  if (TestFn(BD, M.get())) {
+  // Verify that the Module is still valid and then
+  // try running on the hacked up program...
+  if (!verifyModule(*M, &llvm::outs()) && TestFn(BD, M.get())) {
     BD.setNewProgram(std::move(M)); // It crashed, keep the trimmed version...
 
     // Make sure to use instruction pointers that point into the now-current
     // module, and that they don't include any deleted blocks.
     Insts.clear();
     for (Instruction *Inst : Instructions)
       Insts.push_back(Inst);
     return true;
   }
-  // It didn't crash, try something else.
+
+  // It didn't crash (or the hacked up module was invalid), try something else.
   return false;
 }
 
 namespace {
 // Reduce the list of Named Metadata nodes. We keep this as a list of
 // names to avoid having to convert back and forth every time.
 class ReduceCrashingNamedMD : public ListReducer<std::string> {
   BugDriver &BD;
for (auto &NamedMD : M->named_metadata())
     // Always keep a nonempty llvm.dbg.cu because the Verifier would complain.
     if (!Names.count(NamedMD.getName()) &&
         (!(NamedMD.getName() == "llvm.dbg.cu" && NamedMD.getNumOperands() > 0)))
       ToDelete.push_back(&NamedMD);
 
   for (auto *NamedMD : ToDelete)
     NamedMD->eraseFromParent();
 
-  // Verify that this is still valid.
-  legacy::PassManager Passes;
-  Passes.add(createVerifierPass(/*FatalErrors=*/false));
-  Passes.run(*M);
-
   // Try running on the hacked up program...
-  if (TestFn(BD, M.get())) {
+  if (!verifyModule(*M, &llvm::outs()) && TestFn(BD, M.get())) {
     BD.setNewProgram(std::move(M)); // It crashed, keep the trimmed version...
     return true;
   }
   return false;
 }
 
 namespace {
 // Reduce the list of operands to named metadata nodes
for (auto &NamedMD : BD.getProgram().named_metadata()) {
     M->eraseNamedMetadata(M->getNamedMetadata(NamedMD.getName()));
     NamedMDNode *NewNamedMDNode =
         M->getOrInsertNamedMetadata(NamedMD.getName());
     for (MDNode *op : NamedMD.operands())
       if (OldMDNodeOps.count(op))
         NewNamedMDNode->addOperand(cast<MDNode>(MapMetadata(op, VMap)));
   }
 
-  // Verify that this is still valid.
-  legacy::PassManager Passes;
-  Passes.add(createVerifierPass(/*FatalErrors=*/false));
-  Passes.run(*M);
-
   // Try running on the hacked up program...
-  if (TestFn(BD, M.get())) {
+  if (!verifyModule(*M, &llvm::outs()) && TestFn(BD, M.get())) {
     // Make sure to use instruction pointers that point into the now-current
     // module, and that they don't include any deleted blocks.
     NamedMDOps.clear();
     for (const MDNode *Node : OldMDNodeOps)
       NamedMDOps.push_back(cast<MDNode>(*VMap.getMappedMD(Node)));
 
     BD.setNewProgram(std::move(M)); // It crashed, keep the trimmed version...
     return true;
for (Module::const_iterator FI = BD.getProgram().begin(),
                   I->isSwiftError())
                 continue;
 
               outs() << "Checking instruction: " << *I;
               std::unique_ptr<Module> M =
                   BD.deleteInstructionFromProgram(&*I, Simplification);
 
               // Find out if the pass still crashes on this pass...
-              if (TestFn(BD, M.get())) {
+              if (M && TestFn(BD, M.get())) {
                 // Yup, it does, we delete the old module, and continue trying
                 // to reduce the testcase...
                 BD.setNewProgram(std::move(M));
                 InstructionsToSkipBeforeDeleting = CurInstructionNum;
                 goto TryAgain; // I wish I had a multi-level break here!
               }
             }
           }

tools/bugpoint/ExtractFunction.cpp

BugDriver::deleteInstructionFromProgram(const Instruction *I,
 
   // If this instruction produces a value, replace any users with null values
   if (!TheInst->getType()->isVoidTy())
     TheInst->replaceAllUsesWith(Constant::getNullValue(TheInst->getType()));
 
   // Remove the instruction from the program.
   TheInst->getParent()->getInstList().erase(TheInst);
 
+  // Verify that removing this instruction didn't cause a verifier error
+  if (verifyModule(*Clone, &llvm::outs()))
+    return nullptr;
+
   // Spiff up the output a little bit.
   std::vector<std::string> Passes;
 
   /// Can we get rid of the -disable-* options?
   if (Simplification > 1 && !NoDCE)
     Passes.push_back("dce");
   if (Simplification && !DisableSimplifyCFG)
     Passes.push_back("simplifycfg"); // Delete dead control flow