Verifier: Simplify and fix issue where we were not verifying unmaterialized functions.

[pcc]

Summary

• Arrange to call verify(Function &) on each function, followed by verify(Module &), whether the verifier is being used from the pass or from verifyModule(). As a side effect, this fixes an issue that caused us not to call verify(Function &) on unmaterialized functions from verifyModule().

• Remove previously unreachable code that verifies that a function definition has an entry block. By definition, a function definition has at least one block.

[pcc]

• Now that we can verify external functions, remove the assert from verifyFunction()

[aprantl]

Would it be possible to add a test for the unmaterialized functions? Sounds like it could be hard though.
The removal of the useless check for the entry block is LGTM as a separate NFC commit.

[pcc]

Add test

[pcc]

Would it be possible to add a test for the unmaterialized functions? Sounds like it could be hard though.

Yes, I added one based on function attached metadata.

The removal of the useless check for the entry block is LGTM as a separate NFC commit.

r271948

[aprantl]

lib/IR/Verifier.cpp

4481 ↗

(On Diff #59796)

Can you remind me why this change is only necessary in the LegacyPass implementation?

[pcc]

lib/IR/Verifier.cpp

4481 ↗

(On Diff #59796)

The LegacyPass implementation is the only one that calls verify(Function &) and verify(Module &) directly. The new pass manager uses the verifyModule and verifyFunction functions.

[aprantl]

Thanks, I think this change is fine, then.

llvm/trunk/lib/IR/Verifier.cpp

bool verify(const Function &F) {
     return !Broken;
   }
 
   bool verify(const Module &M) {
     updateModule(&M);
     Context = &M.getContext();
     Broken = false;
 
-    // Scan through, checking all of the external function's linkage now...
-    for (const Function &F : M) {
-      visitGlobalValue(F);
-
-      // Check to make sure function prototypes are okay.
-      if (F.isDeclaration()) {
-        visitFunction(F);
-        if (F.getIntrinsicID() == Intrinsic::experimental_deoptimize)
-          DeoptimizeDeclarations.push_back(&F);
-      }
-    }
+    // Collect all declarations of the llvm.experimental.deoptimize intrinsic.
+    for (const Function &F : M)
 
     // Now that we've visited every function, verify that we never asked to
     // recover a frame index that wasn't escaped.
     verifyFrameRecoverIndices();
     for (const GlobalVariable &GV : M.globals())
       visitGlobalVariable(GV);
 
     for (const GlobalAlias &GA : M.aliases())
for (const auto &Pair : SiblingFuncletInfo) {
     // nodes' successors.
     Active.clear();
   }
 }
 
 // visitFunction - Verify that a function is ok.
 //
 void Verifier::visitFunction(const Function &F) {
+  visitGlobalValue(F);
+
   // Check function arguments.
   FunctionType *FT = F.getFunctionType();
   unsigned NumArgs = F.arg_size();
 
   Assert(Context == &F.getContext(),
          "Function context does not match Module context!", &F);
 
   Assert(!F.hasCommonLinkage(), "Functions may not have common linkage", &F);

 }
 
 //===----------------------------------------------------------------------===//
 //  Implement the public interfaces to this file...
 //===----------------------------------------------------------------------===//
 
 bool llvm::verifyFunction(const Function &f, raw_ostream *OS) {
   Function &F = const_cast<Function &>(f);
-  assert(!F.isDeclaration() && "Cannot verify external functions");
 
   // Don't use a raw_null_ostream.  Printing IR is expensive.
   Verifier V(OS, /*ShouldTreatBrokenDebugInfoAsError=*/true);
 
   // 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);
 
   bool Broken = false;
   for (const Function &F : M)
-    if (!F.isDeclaration() && !F.isMaterializable())
-      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;
 }
struct VerifierLegacyPass : public FunctionPass {
   bool runOnFunction(Function &F) override {
     if (!V.verify(F) && FatalErrors)
       report_fatal_error("Broken function found, compilation aborted!");
 
     return false;
   }
 
   bool doFinalization(Module &M) override {
-    bool HasErrors = !V.verify(M);
+    bool HasErrors = false;
+    for (Function &F : M)
+      if (F.isDeclaration())
+        HasErrors |= !V.verify(F);
+
+    HasErrors |= !V.verify(M);
     if (FatalErrors) {
       if (HasErrors)
         report_fatal_error("Broken module found, compilation aborted!");
       assert(!V.hasBrokenDebugInfo() && "Module contains invalid debug info");
     }
 
     // Strip broken debug info.
     if (V.hasBrokenDebugInfo()) {

llvm/trunk/unittests/IR/MetadataTest.cpp

TEST_F(FunctionAttachmentTest, Verifier) {
 
   // Confirm this has no body.
   ASSERT_TRUE(F->empty());
 
   // Functions without a body cannot have metadata attachments (they also can't
   // be verified directly, so check that the module fails to verify).
   EXPECT_TRUE(verifyModule(*F->getParent()));
 
+  // Nor can materializable functions.
+  F->setIsMaterializable(true);
+  EXPECT_TRUE(verifyModule(*F->getParent()));
+
   // Functions with a body can.
+  F->setIsMaterializable(false);
   (void)new UnreachableInst(Context, BasicBlock::Create(Context, "bb", F));
   EXPECT_FALSE(verifyModule(*F->getParent()));
   EXPECT_FALSE(verifyFunction(*F));
 }
 
 TEST_F(FunctionAttachmentTest, EntryCount) {
   Function *F = getFunction("foo");
   EXPECT_FALSE(F->getEntryCount().hasValue());