[PATCH] D15272: [Verifier] Verifier that a GlobalValue is only used in this Module

[Rafael Espíndola via llvm-commits]

On 23 December 2015 at 13:18, Keno Fischer <kfischer at college.harvard.edu> wrote:
> Somewhere we have to have the logic as to which users we can handle (those
> which are embedded in a module). We can have that in the callback with a
> boolean flag as to whether we could handle it, or we could have that logic
> in the foreach function. The latter seemed cleaner to me, but as I said, no
> strong opinions here.
>

What I am suggesting is that the helper should be a fully generic
"walk every user". What do you think of the attached patch?


Cheers,
Rafael
-------------- next part --------------
diff --git a/include/llvm/IR/Value.h b/include/llvm/IR/Value.h
index bb7ff27..1e08a4a 100644
--- a/include/llvm/IR/Value.h
+++ b/include/llvm/IR/Value.h
@@ -351,13 +351,19 @@ public:
     assertModuleIsMaterialized();
     return *materialized_user_begin();
   }
+  iterator_range<user_iterator> materialized_users() {
+    return make_range(materialized_user_begin(), user_end());
+  }
+  iterator_range<const_user_iterator> materialized_users() const {
+    return make_range(materialized_user_begin(), user_end());
+  }
   iterator_range<user_iterator> users() {
     assertModuleIsMaterialized();
-    return make_range(materialized_user_begin(), user_end());
+    return materialized_users();
   }
   iterator_range<const_user_iterator> users() const {
     assertModuleIsMaterialized();
-    return make_range(materialized_user_begin(), user_end());
+    return materialized_users();
   }
 
   /// \brief Return true if there is exactly one user of this value.
diff --git a/lib/IR/Verifier.cpp b/lib/IR/Verifier.cpp
index 71d2c30..8ea6914 100644
--- a/lib/IR/Verifier.cpp
+++ b/lib/IR/Verifier.cpp
@@ -448,6 +448,17 @@ void Verifier::visit(Instruction &I) {
   InstVisitor<Verifier>::visit(I);
 }
 
+// Helper to recursively iterate over indirect users. That is, the users of V,
+// the users of the users of V, etc.
+static void forEachUser(const Value *V, SmallPtrSet<const Value *, 32> &Visited,
+                        llvm::function_ref<void(const Value *)> Callback) {
+  if (!Visited.insert(V).second)
+    return;
+  for (const Value *U : V->materialized_users()) {
+    Callback(U);
+    forEachUser(U, Visited, Callback);
+  }
+}
 
 void Verifier::visitGlobalValue(const GlobalValue &GV) {
   Assert(!GV.isDeclaration() || GV.hasExternalLinkage() ||
@@ -467,6 +478,30 @@ void Verifier::visitGlobalValue(const GlobalValue &GV) {
 
   if (GV.isDeclarationForLinker())
     Assert(!GV.hasComdat(), "Declaration may not be in a Comdat!", &GV);
+
+  // 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> Visited;
+  forEachUser(&GV, Visited, [&](const Value *V) {
+    if (const auto *I = dyn_cast<Instruction>(V)) {
+      Assert(I->getParent() && I->getParent()->getParent(),
+             "Global is referenced by parentless instruction!", &GV,
+             GV.getParent(), I);
+      Assert(I->getParent()->getParent()->getParent() == GV.getParent(),
+             "Global is referenced in a different module!", &GV, GV.getParent(),
+             I, I->getParent()->getParent(),
+             I->getParent()->getParent()->getParent());
+      return;
+    }
+    if (const auto *F = dyn_cast<Function>(V)) {
+      Assert(F->getParent() == GV.getParent(),
+             "Global is used by function in a different module", &GV,
+             GV.getParent(), F, F->getParent());
+      return;
+    }
+  });
 }
 
 void Verifier::visitGlobalVariable(const GlobalVariable &GV) {
diff --git a/unittests/IR/VerifierTest.cpp b/unittests/IR/VerifierTest.cpp
index 4e94b43..7ae346d 100644
--- a/unittests/IR/VerifierTest.cpp
+++ b/unittests/IR/VerifierTest.cpp
@@ -64,7 +64,7 @@ TEST(VerifierTest, CrossModuleRef) {
   LLVMContext &C = getGlobalContext();
   Module M1("M1", C);
   Module M2("M2", C);
-  Module M3("M2", C);
+  Module M3("M3", C);
   FunctionType *FTy = FunctionType::get(Type::getInt32Ty(C), /*isVarArg=*/false);
   Function *F1 = cast<Function>(M1.getOrInsertFunction("foo1", FTy));
   Function *F2 = cast<Function>(M2.getOrInsertFunction("foo2", FTy));
@@ -86,7 +86,21 @@ TEST(VerifierTest, CrossModuleRef) {
 
   std::string Error;
   raw_string_ostream ErrorOS(Error);
-  EXPECT_FALSE(verifyModule(M2, &ErrorOS));
+  EXPECT_TRUE(verifyModule(M2, &ErrorOS));
+  EXPECT_TRUE(StringRef(ErrorOS.str())
+                  .equals("Global is used by function in a different module\n"
+                          "i32 ()* @foo2\n"
+                          "; ModuleID = 'M2'\n"
+                          "i32 ()* @foo3\n"
+                          "; ModuleID = 'M3'\n"
+                          "Global is referenced in a different module!\n"
+                          "i32 ()* @foo2\n"
+                          "; ModuleID = 'M2'\n"
+                          "  %call = call i32 @foo2()\n"
+                          "i32 ()* @foo1\n"
+                          "; ModuleID = 'M1'\n"));
+
+  Error.clear();
   EXPECT_TRUE(verifyModule(M1, &ErrorOS));
   EXPECT_TRUE(StringRef(ErrorOS.str()).equals(
       "Referencing function in another module!\n"