Tie the Verifier class to a Module; NFCI

[sanjoy]

Summary

This commit changes the Verifier class to accept a Module via the
constructor to make it obvious that a specific instance of the class is
only intended to work with a specific module. The updateModule setter
(despite being private) was making this fact less transparent.

There are fields in the Verifier class like DeoptimizeDeclarations
and GlobalValueVisited which are module specific, so a given
Verifier instance will not in fact work across multiple modules today.
This change just makes that more obvious.

The motivation is to make it easy to get to the datalayout of the
module unambiguously. That is required to verify that inttoptr and
ptrtoint constant expressions are well typed in the face of
non-integral pointer types.

I'm also tempted to change the Verifier::verify(const Module &M)
function to Verifier::verify() (as a separate change perhaps?) if
people are on-board with that.

[majnemer]

lib/IR/Verifier.cpp

88

Should this be a reference now?

256–257

Should we remove this M parameter now?

[sanjoy]

Agree with both of your points, but do you mind if I do those in the next change? That way this change stays small and obvious.

[mehdi_amini]

lib/IR/Verifier.cpp

310

I'd just remove the Module parameter as part of the same patch. It becomes a sketchy invariant to be part of the API now.

[chandlerc]

FWIW, LGTM regardless of how you sequence the changes, but I like the changes suggested by David and Mehdi. Feel free to sort out with them here or IRC how to land this.

lib/IR/Verifier.cpp

310

+1

[sanjoy]

Address review:

• Remove "false" parameters from Verifier::verify(Module) and Verify::checkAtomicMemAccessSize.
• Minor simplifications resulting from the change that are not worth the hassle to separately land.

I will change the Module and LLVMContext in VerifierSupport to be
references in a later change.

[mehdi_amini]

lib/IR/Verifier.cpp

91

Is the Context member used?

[sanjoy]

lib/IR/Verifier.cpp

91

Yes, it is used in Verifier (which inherits from VerifierSupport).

lib/IR/Verifier.cpp

 #include <cstdarg>
 using namespace llvm;
 
 static cl::opt<bool> VerifyDebugInfo("verify-debug-info", cl::init(true));
 
 namespace {
 struct VerifierSupport {
   raw_ostream *OS;
-  const Module *M = nullptr;
-  Optional<ModuleSlotTracker> MST;
+  const Module *M;
+  ModuleSlotTracker MST;
 
   /// Track the brokenness of the module while recursively visiting.
   bool Broken = false;
   /// Broken debug info can be "recovered" from by stripping the debug info.
   bool BrokenDebugInfo = false;
   /// Whether to treat broken debug info as an error.
   bool TreatBrokenDebugInfoAsError = true;
 
-  explicit VerifierSupport(raw_ostream *OS) : OS(OS) {}
+  explicit VerifierSupport(raw_ostream *OS, const Module *M)
+      : OS(OS), M(M), MST(M) {}
 
 private:
   template <class NodeTy> void Write(const ilist_iterator<NodeTy> &I) {
     Write(&*I);
   }
 
   void Write(const Module *M) {
     if (!M)
       return;
     *OS << "; ModuleID = '" << M->getModuleIdentifier() << "'\n";
   }
 
   void Write(const Value *V) {
     if (!V)
       return;
     if (isa<Instruction>(V)) {
-      V->print(*OS, *MST);
+      V->print(*OS, MST);
       *OS << '\n';
     } else {
-      V->printAsOperand(*OS, true, *MST);
+      V->printAsOperand(*OS, true, MST);
       *OS << '\n';
     }
   }
   void Write(ImmutableCallSite CS) {
     Write(CS.getInstruction());
   }
 
   void Write(const Metadata *MD) {
     if (!MD)
       return;
-    MD->print(*OS, *MST, M);
+    MD->print(*OS, MST, M);
     *OS << '\n';
   }
 
   template <class T> void Write(const MDTupleTypedArrayWrapper<T> &MD) {
     Write(MD.get());
   }
 
   void Write(const NamedMDNode *NMD) {
     if (!NMD)
       return;
-    NMD->print(*OS, *MST);
+    NMD->print(*OS, MST);
     *OS << '\n';
   }
 
   void Write(Type *T) {
     if (!T)
       return;
     *OS << ' ' << *T;
   }
class Verifier : public InstVisitor<Verifier>, VerifierSupport {
   SmallVector<const Function *, 4> DeoptimizeDeclarations;
 
   // Verify that this GlobalValue is only used in this module.
   // This map is used to avoid visiting uses twice. We can arrive at a user
   // twice, if they have multiple operands. In particular for very large
   // constant expressions, we can arrive at a particular user many times.
   SmallPtrSet<const Value *, 32> GlobalValueVisited;
 
   void checkAtomicMemAccessSize(const Module *M, Type *Ty,
                                 const Instruction *I);
 
-  void updateModule(const Module *NewM) {
-    if (M == NewM)
-      return;
-    MST.emplace(NewM);
-    M = NewM;
-  }
-
 public:
-  explicit Verifier(raw_ostream *OS, bool ShouldTreatBrokenDebugInfoAsError)
-      : VerifierSupport(OS), Context(nullptr), LandingPadResultTy(nullptr),
+  explicit Verifier(raw_ostream *OS, bool ShouldTreatBrokenDebugInfoAsError,
+                    const Module *M)
+      : VerifierSupport(OS, M), Context(nullptr), LandingPadResultTy(nullptr),
         SawFrameEscape(false) {
     TreatBrokenDebugInfoAsError = ShouldTreatBrokenDebugInfoAsError;
   }
 
   bool hasBrokenDebugInfo() const { return BrokenDebugInfo; }
 
   bool verify(const Function &F) {
-    updateModule(F.getParent());
+    assert(F.getParent() == M &&
+           "An instance of this class only works with a specific module!");
     Context = &M->getContext();
 
     // First ensure the function is well-enough formed to compute dominance
     // information, and directly compute a dominance tree. We don't rely on the
     // pass manager to provide this as it isolates us from a potentially
     // out-of-date dominator tree and makes it significantly more complex to run
     // this code outside of a pass manager.
     // FIXME: It's really gross that we have to cast away constness here.
     if (!F.empty())
       DT.recalculate(const_cast<Function &>(F));
 
     for (const BasicBlock &BB : F) {
       if (!BB.empty() && BB.back().isTerminator())
         continue;
 
       if (OS) {
         *OS << "Basic Block in function '" << F.getName()
             << "' does not have terminator!\n";
-        BB.printAsOperand(*OS, true, *MST);
+        BB.printAsOperand(*OS, true, MST);
         *OS << "\n";
       }
       return false;
     }
 
     Broken = false;
     // FIXME: We strip const here because the inst visitor strips const.
     visit(const_cast<Function &>(F));
     verifySiblingFuncletUnwinds();
     InstsInThisBlock.clear();
     LandingPadResultTy = nullptr;
     SawFrameEscape = false;
     SiblingFuncletInfo.clear();
 
     return !Broken;
   }
 
   bool verify(const Module &M) {
-    updateModule(&M);
+    assert(&M == this->M &&
+           "An instance of this class only works with a specific module!");
     Context = &M.getContext();
     Broken = false;
 
     // Collect all declarations of the llvm.experimental.deoptimize intrinsic.
     for (const Function &F : M)
       if (F.getIntrinsicID() == Intrinsic::experimental_deoptimize)
         DeoptimizeDeclarations.push_back(&F);
 

 //===----------------------------------------------------------------------===//
 //  Implement the public interfaces to this file...
 //===----------------------------------------------------------------------===//
 
 bool llvm::verifyFunction(const Function &f, raw_ostream *OS) {
   Function &F = const_cast<Function &>(f);
 
   // Don't use a raw_null_ostream.  Printing IR is expensive.
-  Verifier V(OS, /*ShouldTreatBrokenDebugInfoAsError=*/true);
+  Verifier V(OS, /*ShouldTreatBrokenDebugInfoAsError=*/true, f.getParent());
 
   // Note that this function's return value is inverted from what you would
   // expect of a function called "verify".
   return !V.verify(F);
 }
 
 bool llvm::verifyModule(const Module &M, raw_ostream *OS,
                         bool *BrokenDebugInfo) {
   // Don't use a raw_null_ostream.  Printing IR is expensive.
-  Verifier V(OS, /*ShouldTreatBrokenDebugInfoAsError=*/!BrokenDebugInfo);
+  Verifier V(OS, /*ShouldTreatBrokenDebugInfoAsError=*/!BrokenDebugInfo, &M);
 
   bool Broken = false;
   for (const Function &F : M)
     Broken |= !V.verify(F);
 
   Broken |= !V.verify(M);
   if (BrokenDebugInfo)
     *BrokenDebugInfo = V.hasBrokenDebugInfo();
   // Note that this function's return value is inverted from what you would
   // expect of a function called "verify".
   return Broken;
 }
 
 namespace {
 struct VerifierLegacyPass : public FunctionPass {
   static char ID;
 
-  Verifier V;
+  std::unique_ptr<Verifier> V;
   bool FatalErrors = true;
 
   VerifierLegacyPass()
-      : FunctionPass(ID),
-        V(&dbgs(), /*ShouldTreatBrokenDebugInfoAsError=*/false) {
+      : FunctionPass(ID) {
     initializeVerifierLegacyPassPass(*PassRegistry::getPassRegistry());
   }
   explicit VerifierLegacyPass(bool FatalErrors)
       : FunctionPass(ID),
-        V(&dbgs(), /*ShouldTreatBrokenDebugInfoAsError=*/false),
         FatalErrors(FatalErrors) {
     initializeVerifierLegacyPassPass(*PassRegistry::getPassRegistry());
   }
 
+  bool doInitialization(Module &M) override {
+    V = llvm::make_unique<Verifier>(
+        &dbgs(), /*ShouldTreatBrokenDebugInfoAsError=*/false, &M);
+    return false;
+  }
+
   bool runOnFunction(Function &F) override {
-    if (!V.verify(F) && FatalErrors)
+    if (!V->verify(F) && FatalErrors)
       report_fatal_error("Broken function found, compilation aborted!");
 
     return false;
   }
 
   bool doFinalization(Module &M) override {
     bool HasErrors = false;
     for (Function &F : M)
       if (F.isDeclaration())
-        HasErrors |= !V.verify(F);
+        HasErrors |= !V->verify(F);
 
-    HasErrors |= !V.verify(M);
+    HasErrors |= !V->verify(M);
     if (FatalErrors) {
       if (HasErrors)
         report_fatal_error("Broken module found, compilation aborted!");
-      assert(!V.hasBrokenDebugInfo() && "Module contains invalid debug info");
+      assert(!V->hasBrokenDebugInfo() && "Module contains invalid debug info");
     }
 
     // Strip broken debug info.
-    if (V.hasBrokenDebugInfo()) {
+    if (V->hasBrokenDebugInfo()) {
       DiagnosticInfoIgnoringInvalidDebugMetadata DiagInvalid(M);
       M.getContext().diagnose(DiagInvalid);
       if (!StripDebugInfo(M))
         report_fatal_error("Failed to strip malformed debug info");
     }
     return false;
   }