Ignore modified attribute list if it yields invalid IR

[Nirhar]

Summary

If modified attribute list is invalid, reverting the change is a
low-cost maintainence solution as compared to examples like
this.
This will ensure that the ListReducer maintains the sanctity of any
new attribute dependencies added in the future/already present.

[lattner]

Unfortunately, I haven't touched this code since about 2004. @modocache I see you've touched this somewhat more recently, can you take a look at this?

[lattner]

llvm/tools/bugpoint/CrashDebugger.cpp

386

Can you use llvm::verifyModule instead of running the pass?

[Nirhar]

llvm/tools/bugpoint/CrashDebugger.cpp

386

Other parts of the CrashDebugger run the pass to verify llvm IR, hence I kept it the same. Let me know if I should still change to llvm::verifyModule.

[modocache]

llvm/tools/bugpoint/CrashDebugger.cpp

386

You bring up a good point, @Nirhar -- there's actually a lot of duplication in this file. I count 7 spots where the verifier pass is run, and this makes 8! Here's an example: https://github.com/llvm/llvm-project/blob/b4b532a9562a1ebca347edc566363fba0531115b/llvm/tools/bugpoint/CrashDebugger.cpp#L512-L520

I agree that it would be nicer to use llvm::verifyModule here; it accomplishes the same thing, but in a much simpler way.

(And, I think it would be even better, in a separate patch, to change the other 7 spots to use verifyModule too -- and, ideally, have them print "verify failed!" and exit, like many of them seem to do.)

[modocache]

llvm/tools/bugpoint/CrashDebugger.cpp

386

ideally, have them print "verify failed!" and exit, like many of them seem to do

Sorry, typo here. I meant to write that it would be nice to 1) have these other spots use verifyModule, AND 2) unify them with a static helper function or something, such that we had one function that could verify a module and print "verify failed!" if verification failed. Right now that logic is duplicated a ton in this file.

[arsenm]

llvm/tools/bugpoint/CrashDebugger.cpp

386

It would be easier yet to just delete bugpoint altogether and switch to using llvm-reduce. The attribute reducer there takes care to avoid introducing invalid attribute combinations

[lattner]

but bugpoint has a waaay cuter name than llvm-reduce! :-)

[Nirhar]

llvm/tools/bugpoint/CrashDebugger.cpp

386

I agree with you, let me refactor the code and then update the diff to incorporate your suggestions.

[Nirhar]

Code Refactor
Used llvm::verifyModule to verify modules modified by CrashDebugger
Introduced function isValidModule to verify correctness of IR, for
the needs of CrashDebugger.

[Nirhar]

Please have a look at these changes.

Changing from running verification passes to llvm::verifyModule leads to the failure of the following test: Bugpoint/invalid-debuginfo.ll. I'm currently trying to understand why its failing.

[modocache]

Looks great!! Thank you for also updating the other call sites. I had a few additional comments, but once those are taken care of I can accept. (When I do, will you need me to merge this commit on your behalf?)

llvm/tools/bugpoint/CrashDebugger.cpp

73

Please run clang-format on this patch; I can tell from some of the spacing here that this is not typical formatting. I believe clang-format should format this as static bool isValidModule(std::unique_ptr<Module> &M, bool exitOnFailure = true) {. There may be other formatting changes that would be applied to other parts of your patch as well.

Also, there's a mix of capitalization conventions here. I believe the Coding Standards here -- https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly -- would have you name this ExitOnFailure, with a capital 'E'.

74

I believe you don't need the parentheses here.

75

I think this would be a little simpler if you checked the inverse:

if (!llvm::verifyModule(*M.get(), &llvm::errs()))
  return true;

if (ExitOnFailure) {
  llvm::errs() << "verify failed!\n";
  exit(1);
}
return false;

This removes a lot of nesting in your if statements, and I think looks nicer.

795

No need to update this comment, IMHO.

796

I believe this should use ExitOnFailure = false, right?

865

Same as above; I don't believe this comment should change. Same goes for the 2 spots below in this patch.

866

Same as above; I believe the previous behavior was to not exit. Same goes for the 2 spots below in this patch.

[Nirhar]

Refactor code with clang format

[Nirhar]

Please have a look at these changes.

@modocache Can you please merge this commit on my behalf once you are satisfied with the changes?

[modocache]

Looks great, thanks!

Can you please merge this commit on my behalf once you are satisfied with the changes?

Sure thing!

llvm/tools/bugpoint/CrashDebugger.cpp

 cl::opt<bool> NoStripDebugTypeInfo("disable-strip-debug-types",
                                cl::desc("Do not strip debug type info metadata"),
                                cl::init(false));
 cl::opt<bool> VerboseErrors("verbose-errors",
                             cl::desc("Print the output of crashing program"),
                             cl::init(false));
 }
 
+static bool isValidModule(std::unique_ptr<Module> &M,
+                          bool ExitOnFailure = true) {
+  if (!llvm::verifyModule(*M.get(), &llvm::errs()))
+    return true;
+
+  if (ExitOnFailure) {
+    llvm::errs() << "verify failed!\n";
+    exit(1);
+  }
+  return false;
+}
+
 namespace llvm {
 class ReducePassList : public ListReducer<std::string> {
   BugDriver &BD;
 
 public:
   ReducePassList(BugDriver &bd) : BD(bd) {}
 
   // Return true iff running the "removed" passes succeeds, and running the
bool ReduceCrashingFunctionAttributes::TestFuncAttrs(
   // Set this new list of attributes on the function.
   F->setAttributes(NewAttrs);
 
   // If the attribute list includes "optnone" we need to make sure it also
   // includes "noinline" otherwise we will get a verifier failure.
   if (F->hasFnAttribute(Attribute::OptimizeNone))
     F->addFnAttr(Attribute::NoInline);
 
+  // If modifying the attribute list leads to invalid IR, revert the change
+  if (!isValidModule(M, /*ExitOnFailure=*/false))
+    return false;
+
   // Try running on the hacked up program...
   if (TestFn(BD, M.get())) {
     BD.setNewProgram(std::move(M)); // It crashed, keep the trimmed version...
 
     // Pass along the set of attributes that caused the crash.
     Attrs.clear();
     for (Attribute A : NewAttrs.getFnAttrs()) {
       Attrs.push_back(A);
bool ReduceCrashingBlocks::TestBlocks(std::vector<const BasicBlock *> &BBs) {
   for (auto &F : *M) {
     for (auto &BB : F)
       if (!Blocks.count(&BB))
         ToProcess.push_back(&BB);
     simpleSimplifyCfg(F, ToProcess);
     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);
-  }
-  M = std::move(New);
+  isValidModule(M);
 
   // Try running on the hacked up program...
   if (TestFn(BD, M.get())) {
     BD.setNewProgram(std::move(M)); // It crashed, keep the trimmed version...
 
     // Make sure to use basic block pointers that point into the now-current
     // module, and that they don't include any deleted blocks.
     BBs.clear();
bool ReduceCrashingConditionals::TestBlocks(
   for (auto &F : *M) {
     for (auto &BB : F)
       if (!Blocks.count(&BB))
         ToProcess.push_back(&BB);
     simpleSimplifyCfg(F, ToProcess);
     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);
-  }
-  M = std::move(New);
+  isValidModule(M);
 
   // Try running on the hacked up program...
   if (TestFn(BD, M.get())) {
     BD.setNewProgram(std::move(M)); // It crashed, keep the trimmed version...
 
     // Make sure to use basic block pointers that point into the now-current
     // module, and that they don't include any deleted blocks.
     BBs.clear();
for (auto &F : *M)
     for (Function::iterator BBIt = F.begin(); BBIt != F.end();) {
       if (!Blocks.count(&*BBIt)) {
         ++BBIt;
         continue;
       }
       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);
-  }
-  M = std::move(New);
+  isValidModule(M);
 
   // Try running on the hacked up program...
   if (TestFn(BD, M.get())) {
     BD.setNewProgram(std::move(M)); // It crashed, keep the trimmed version...
 
     // Make sure to use basic block pointers that point into the now-current
     // module, and that they don't include any deleted blocks.
     BBs.clear();
for (Function::iterator FI = MI->begin(), FE = MI->end(); FI != FE; ++FI)
             !Inst.isSwiftError()) {
           if (!Inst.getType()->isVoidTy())
             Inst.replaceAllUsesWith(PoisonValue::get(Inst.getType()));
           Inst.eraseFromParent();
         }
       }
 
   // Verify that this is still valid.
-  legacy::PassManager Passes;
-  Passes.add(createVerifierPass(/*FatalErrors=*/false));
-  Passes.run(*M);
+  isValidModule(M, /*ExitOnFailure=*/false);
 
   // Try running on the hacked up program...
   if (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)
for (Function &F : *M)
     for (Instruction &Inst : instructions(F)) {
       if (!Instructions.count(&Inst)) {
         Inst.dropUnknownNonDebugMetadata();
         Inst.setDebugLoc({});
       }
     }
 
   // Verify that this is still valid.
-  legacy::PassManager Passes;
-  Passes.add(createVerifierPass(/*FatalErrors=*/false));
-  Passes.run(*M);
+  isValidModule(M, /*ExitOnFailure=*/false);
 
   // Try running on the hacked up program...
   if (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 *I : Instructions)
for (auto &NamedMD : M->named_metadata())
     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);
+  isValidModule(M, /*ExitOnFailure=*/false);
 
   // Try running on the hacked up program...
   if (TestFn(BD, M.get())) {
     BD.setNewProgram(std::move(M)); // It crashed, keep the trimmed version...
     return true;
   }
   return false;
 }
for (auto &NamedMD : BD.getProgram().named_metadata()) {
     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);
+  isValidModule(M, /*ExitOnFailure=*/false);
 
   // Try running on the hacked up program...
   if (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)));