Make setInitializer() assert that the entire initializer is usable.

[arnt]

Summary

Until now, it was possible to set an initilizer for e.g. a struct that
contained a global constant from some other module. The verifier would
complain, but the verifier's complaint doesn't point very accurately at
the problem.

[arnt]

I picked reviewers by taking the last two people who changed the file, hope you don't mind.

[lebedev.ri]

You probably want to wrap that entire function into #ifndef NDEBUG

llvm/lib/IR/Globals.cpp

386–387

The style is usually

for(int I = 0, N = V->getNumOperands(); I != N; ++I)
  assertThatAllGlobalsAreIn(V->getOperand(--n), Correct);

386–387

well, s/--n/n/

[arnt]

updated following comments from lebedev.ri; thanks

[arnt]

Ping?

[arnt]

Rebased to this morning's LLVM.

No changes were necessary.

[arnt]

pete: I took the liberty of adding you as reviewer since you made the most recent change to that function. 2015-06-02. Not exactly yesterday.

[efriedma]

llvm/lib/IR/Globals.cpp

386

Please clang-format this.

Can you check for ConstantAggregate here, instead of checking ConstantStruct and ConstantArray separately? Would it also make sense to check for ConstantExpr?

Can you use a for-range loop here? (for (auto *Op : V->operands()))

[arnt]

Add unit test, plus some nonfunctional changes.

[arnt]

I like colon-for.

llvm/lib/IR/Globals.cpp

386

Oooh, I can call constants() now. Someone has added that. (Might be a while ago, this isn't exactly new code. ;-)

I changed it to test ConstantAggregate, and added a passing unit test involving ConstantExpr.

[efriedma]

llvm/unittests/IR/VerifierTest.cpp

228

Are you sure this actually does what you want? ConstantExpr::get() does basic constant folding.

[arnt]

best take care of self-referential initialisers.

[arnt]

More conservative testing, as half-suggested by efriedma.

[efriedma]

I don't think you actually need a recursion guard; you only recurse on ConstantAggregates, and they can't refer to themselves circularly. But maybe it's a good idea anyway to avoid exponential compile-time in certain edge cases.

llvm/unittests/IR/VerifierTest.cpp

237

This test *still* isn't testing what you want; verification fails due to the other global variable.

[arichardson]

probably not the right person to review this

llvm/lib/IR/Globals.cpp

 void GlobalVariable::removeFromParent() {
   getParent()->getGlobalList().remove(getIterator());
 }
 
 void GlobalVariable::eraseFromParent() {
   getParent()->getGlobalList().erase(getIterator());
 }
 
+static void assertThatAllGlobalsAreIn(Constant *Value,
+                                      SmallPtrSetImpl<Constant *> &Visited,
+                                      Module *Correct) {
+#ifndef NDEBUG
+  if (Visited.count(Value))
+    return;
+
+  Visited.insert(Value);
+  if (ConstantAggregate *V = dyn_cast<ConstantAggregate>(Value)) {
+    for (auto &O : V->operands())
+      assertThatAllGlobalsAreIn(cast<Constant>(O.getUser()), Visited, Correct);
+  } else if (GlobalValue *V = dyn_cast<GlobalValue>(Value)) {
+    assert((!V->getParent() || V->getParent() == Correct) &&
+           "A Constant and its initializer cannot be in different modules");
+  }
+#endif
+}
+
 void GlobalVariable::setInitializer(Constant *InitVal) {
   if (!InitVal) {
     if (hasInitializer()) {
       // Note, the num operands is used to compute the offset of the operand, so
       // the order here matters.  Clearing the operand then clearing the num
       // operands ensures we have the correct offset to the operand.
       Op<0>().set(nullptr);
       setGlobalVariableNumOperands(0);
     }
   } else {
     assert(InitVal->getType() == getValueType() &&
            "Initializer type must match GlobalVariable type");
     // Note, the num operands is used to compute the offset of the operand, so
     // the order here matters.  We need to set num operands to 1 first so that
     // we get the correct offset to the first operand when we set it.
     if (!hasInitializer())
       setGlobalVariableNumOperands(1);
     Op<0>().set(InitVal);
+    SmallPtrSet<Constant *, 16> RecursionGuard;
+    assertThatAllGlobalsAreIn(InitVal, RecursionGuard, getParent());
   }
 }
 
 /// Copy all additional attributes (those not needed to create a GlobalVariable)
 /// from the GlobalVariable Src to this one.
 void GlobalVariable::copyAttributesFrom(const GlobalVariable *Src) {
   GlobalObject::copyAttributesFrom(Src);
   setThreadLocalMode(Src->getThreadLocalMode());

llvm/unittests/IR/VerifierTest.cpp

TEST(VerifierTest, DetectInvalidDebugInfo) {
     EXPECT_FALSE(verifyModule(M));
 
     // Now break it by not listing the CU at all.
     M.eraseNamedMetadata(M.getOrInsertNamedMetadata("llvm.dbg.cu"));
     EXPECT_TRUE(verifyModule(M));
   }
 }
 
+TEST(VerifierTest, CrossModuleInitialization) {
+  // Setup: provide a module containing a global variable that can be
+  // referenced...
+  LLVMContext C;
+  Module M1("M1", C);
+
+  StructType *ST = StructType::create(C, "selfref");
+  ST->setBody({ST->getPointerTo()}, false);
+  GlobalVariable *GV1 = new GlobalVariable(
+      M1, ST, true, GlobalVariable::ExternalLinkage, nullptr);
+
+  // ... and a module, that absolutely cannot refer to GV1 as part of any
+  // initializer.
+  Module M2("M2", C);
+
+  // No test is complete without a little plumbing.
+  std::string Error;
+  raw_string_ostream ErrorOS(Error);
+
+  // Test 1. Is it okay for another variable in M1 to refer to GV1? It should
+  // be.
+  new GlobalVariable(M1, ST, true, GlobalVariable::ExternalLinkage,
+                     ConstantStruct::get(ST, {GV1}));
+  EXPECT_FALSE(verifyModule(M1, &ErrorOS));
+
+  // Test 2. Is it okay for a variable in M2 to refer to GV1? It should not be.
+  new GlobalVariable(M2, ST, true, GlobalVariable::ExternalLinkage,
+                     ConstantStruct::get(ST, {GV1}));
+  Error.clear();
+  EXPECT_TRUE(verifyModule(M2, &ErrorOS));
+  EXPECT_TRUE(ErrorOS.str().substr(0, 36) ==
+              "Referencing global in another module");
+
+  // Test 3. Like 2, except that here a ConstantExpr is used to mask the
+  // reference.
+  PointerType *I1p = IntegerType::get(C, 1)->getPointerTo();
+  auto GV1again = ConstantExpr::getPointerCast(GV1, I1p);
+  StructType *STagain = StructType::create(C, "masked-selfref");
+  STagain->setBody({I1p}, false);
+  new GlobalVariable(M2, STagain, true, GlobalVariable::ExternalLinkage,
+                     ConstantStruct::get(STagain, {GV1again}));
+  Error.clear();
+  EXPECT_TRUE(verifyModule(M2, &ErrorOS));
+  EXPECT_TRUE(ErrorOS.str().substr(0, 36) ==
+              "Referencing global in another module");
+
+  // Minor addendum: M1 isn't made bad by the references from M2, right?
+  EXPECT_FALSE(verifyModule(M1, &ErrorOS));
+
+  // Test 4: Is it okay for GV1 to refer to itself? It should be, and that
+  // definitely should not lead to any infinite loop anywhere.
+  GV1->setInitializer(ConstantStruct::get(ST, {GV1}));
+  EXPECT_FALSE(verifyModule(M1, &ErrorOS));
+  // LLVM is built without exceptions, so if this test fails, you won't get a
+  // nice error message. Instead the stack blows up and the test segfaults.
+}
+
 } // end anonymous namespace
 } // end namespace llvm