Fix cloning GlobalValues with comdats across Modules.

[jlebar]

Summary

Previously copyAttributesFrom would copy the Comdat pointer from one
GlobalValue to another. The Comdat is owned by the Module, so if the
two GlobalValues were not in the same module, this would be Bad.

[jlebar]

Add missing null check.

[jlebar]

Preserve old behavior when a function isn't in a module.

Now all the tests pass. :)

[majnemer]

lib/IR/Globals.cpp

103–110

I find it a little weird that we are trying to predict how the global will be used.
We are not sure which module it will be in but we sorta assume it will be in the same one as the original function...

I'm starting to think that the right thing to do is move this logic out of copyAttributesFrom and into another method. Something like copyComdatFrom which asserts that the source and dest are from the same module.

What do you think?

[jlebar]

lib/IR/Globals.cpp

103–110

I find it a little weird that we are trying to predict how the global will be used.

Me too.

An alternative would be not to set the comdat at all if the function isn't in a module. That's how I originally had it, but it broke a test (which was what made me realize it was a functional change in the first place).

Something like copyComdatFrom which asserts that the source and dest are from the same module.

...or maybe it would assert that just that src and dst are both in a module, and then copy the comdat as we do here? That seems like basically what one would want, I think.

The problem with adding either of these new functions is that we'd have to audit the ~30 calls to copyAttributesFrom, and I'm not sure I'm going to be successful at that.

[jlebar]

Something like copyComdatFrom which asserts that the source and dest are from the same module.

What are the circumstances under which one would or wouldn't want to copy the comdat, when copying all the rest of the attributes? Like, if we should (almost) always do it, then moving it into a separate function doesn't make a lot of sense to me. OTOH if we almost never actually want do it...

[rnk]

I think most users of copyAttributesFrom are not copying globals across modules, they are trying to RAUW in the current module. The one user that I know of (the IR linker) goes out of its way to copy the comdat separately. Anything wrong with forcing the cross-module use case to be a little more complicated? Such users already have to rewrite the initializer to avoid cross-module references, so rewriting the comdat as well isn't that crazy.

[jlebar]

In D21255#458782, @rnk wrote:

Anything wrong with forcing the cross-module use case to be a little more complicated?

I guess the suggestion is,

• leave the implementation of copyAttributesFrom alone, and
• add a comment explaining what needs to be done if you're copying between modules?

I guess I'm fine with that, although given that we already had a nontrivial bug (that took me half a day to track down), I am a bit concerned about leaving the footgun there as-is. Even if all of the code in llvm is correct, this seems like a pretty sharp edge to expose to future authors.

What if we modified Function::setParent to either

• assert that the comdat is from the right module, or
• fix up the comdat?

I guess if cross-module copiers have to fix up the initializers, an assertion might be the most appropriate thing, but I'm fine with either.

[rnk]

In D21255#458935, @jlebar wrote:

In D21255#458782, @rnk wrote:

Anything wrong with forcing the cross-module use case to be a little more complicated?

I guess the suggestion is,

• leave the implementation of copyAttributesFrom alone, and
• add a comment explaining what needs to be done if you're copying between modules?

I guess I'm fine with that, although given that we already had a nontrivial bug (that took me half a day to track down), I am a bit concerned about leaving the footgun there as-is. Even if all of the code in llvm is correct, this seems like a pretty sharp edge to expose to future authors.

What if we modified Function::setParent to either

• assert that the comdat is from the right module, or
• fix up the comdat?

I guess if cross-module copiers have to fix up the initializers, an assertion might be the most appropriate thing, but I'm fine with either.

Yeah, it is pretty crummy. I think this is why David didn't copy the comdat with the other attributes in the first place.

I like the idea of asserting in setParent. We don't currently do the analogous check for GlobalVariable initializers, but we should consider it. :)

[jlebar]

I like the idea of asserting in setParent

Okay, this doesn't quite work. You can assert in setParent, but it doesn't make sense to assert there but not inside copyAttributesFrom. But if we assert in copyAttributesFrom, that means that the IRLinker needs to be changed so that the GlobalValue it copies into is not in a module. And changing that looks nontrivial (and contrived).

I guess we could leave the assertion in place and add a new function, copyAttributesExceptComdatFrom. We'd have to make this one virtual and make copyAttributesFrom non-virtual. Not sure how I feel about that...

[majnemer]

In D21255#459019, @jlebar wrote:

I like the idea of asserting in setParent

Okay, this doesn't quite work. You can assert in setParent, but it doesn't make sense to assert there but not inside copyAttributesFrom. But if we assert in copyAttributesFrom, that means that the IRLinker needs to be changed so that the GlobalValue it copies into is not in a module. And changing that looks nontrivial (and contrived).

I guess we could leave the assertion in place and add a new function, copyAttributesExceptComdatFrom. We'd have to make this one virtual and make copyAttributesFrom non-virtual. Not sure how I feel about that...

How about let's just remove the COMDAT machinations from copyAttributesFrom. Maybe provide another mechanism for comdats.

[rnk]

In D21255#459059, @majnemer wrote:

How about let's just remove the COMDAT machinations from copyAttributesFrom. Maybe provide another mechanism for comdats.

For non-module copying, it's only a one liner, so sure, let's revert http://reviews.llvm.org/D20631.

[jlebar]

For non-module copying, it's only a one liner, so sure, let's revert http://reviews.llvm.org/D20631.

OK, I'll send you a patch.

[jlebar]

Sent http://reviews.llvm.org/D21403.

lib/IR/Globals.cpp

void GlobalObject::setGlobalObjectSubClassData(unsigned Val) {
   assert(getGlobalObjectSubClassData() == Val && "representation error");
 }
 
 void GlobalObject::copyAttributesFrom(const GlobalValue *Src) {
   GlobalValue::copyAttributesFrom(Src);
   if (const auto *GV = dyn_cast<GlobalObject>(Src)) {
     setAlignment(GV->getAlignment());
     setSection(GV->getSection());
-    setComdat(const_cast<GlobalObject *>(GV)->getComdat());
+    // Src may not be in the same Module as this, and Modules own their Comdats,
+    // so we need to explicitly get a Comdat from our own Module.  If we're not
+    // currently in a Module, guess that we're going to be put into the same
+    // module as Src, so it's safe to share Src's comdat.
+    Comdat *SrcComdat = const_cast<GlobalObject *>(GV)->getComdat();
+    if (SrcComdat == nullptr || getParent() == nullptr)
+      setComdat(SrcComdat);
+    else if (getParent() != nullptr) {
+      Comdat *C = getParent()->getOrInsertComdat(SrcComdat->getName());
+      C->setSelectionKind(SrcComdat->getSelectionKind());
+      setComdat(C);
+    }
   }
 }
 
 std::string GlobalValue::getGlobalIdentifier(StringRef Name,
                                              GlobalValue::LinkageTypes Linkage,
                                              StringRef FileName) {
 
   // Value names may be prefixed with a binary '1' to indicate

unittests/IR/ValueTest.cpp

= new GlobalVariable(*M,
                          1);
 
   // Make sure the address space isn't dropped when returning this.
   Constant *DummyCast1 = M->getOrInsertGlobal("dummy_cast", Int8Ty);
   EXPECT_EQ(1u, DummyCast1->getType()->getPointerAddressSpace());
   EXPECT_NE(DummyCast0, DummyCast1) << *DummyCast1;
 }
 
+TEST(GlobalTest, CloneWithComdatIntoSameModule) {
+  LLVMContext Ctx;
+  Type *Int32Ty = Type::getInt32Ty(Ctx);
+  auto M = make_unique<Module>("Module", Ctx);
+
+  GlobalVariable *GV1 =
+      new GlobalVariable(*M, Int32Ty, true, GlobalValue::ExternalLinkage,
+                         Constant::getAllOnesValue(Int32Ty), "GV", nullptr,
+                         GlobalVariable::NotThreadLocal, 1);
+  Comdat *Comdat = M->getOrInsertComdat("GV");
+  Comdat->setSelectionKind(Comdat::Largest);
+  GV1->setComdat(Comdat);
+
+  GlobalVariable *GV2 =
+      new GlobalVariable(*M, Int32Ty, true, GlobalValue::ExternalLinkage,
+                         Constant::getAllOnesValue(Int32Ty), "GV", nullptr,
+                         GlobalVariable::NotThreadLocal, 1);
+  GV2->copyAttributesFrom(GV1);
+  ASSERT_EQ(Comdat, GV1->getComdat());
+  ASSERT_TRUE(GV2->getComdat() != nullptr);
+  EXPECT_EQ(Comdat::Largest, GV2->getComdat()->getSelectionKind());
+
+  // GV1 and GV2 should share the same comdat pointers, because they're in the
+  // same module.
+  EXPECT_EQ(GV1->getComdat(), GV2->getComdat());
+}
+
+TEST(GlobalTest, CloneWithComdatIntoSeparateModule) {
+  LLVMContext Ctx;
+  Type *Int32Ty = Type::getInt32Ty(Ctx);
+  auto M1 = make_unique<Module>("Module1", Ctx);
+  auto M2 = make_unique<Module>("Module2", Ctx);
+
+  GlobalVariable *GV1 =
+      new GlobalVariable(*M1, Int32Ty, true, GlobalValue::ExternalLinkage,
+                         Constant::getAllOnesValue(Int32Ty), "GV", nullptr,
+                         GlobalVariable::NotThreadLocal, 1);
+  Comdat *Comdat1 = M1->getOrInsertComdat("GV");
+  Comdat1->setSelectionKind(Comdat::Largest);
+  GV1->setComdat(Comdat1);
+
+  GlobalVariable *GV2 =
+      new GlobalVariable(*M2, Int32Ty, true, GlobalValue::ExternalLinkage,
+                         Constant::getAllOnesValue(Int32Ty), "GV", nullptr,
+                         GlobalVariable::NotThreadLocal, 1);
+  GV2->copyAttributesFrom(GV1);
+  ASSERT_EQ(Comdat1, GV1->getComdat());
+  ASSERT_TRUE(GV2->getComdat() != nullptr);
+  EXPECT_EQ(Comdat::Largest, GV2->getComdat()->getSelectionKind());
+
+  // GV1 and GV2 cannot share the same comdat pointers, because they're in
+  // different Modules.
+  EXPECT_NE(GV1->getComdat(), GV2->getComdat());
+}
+
 #ifdef GTEST_HAS_DEATH_TEST
 #ifndef NDEBUG
 TEST(GlobalTest, AlignDeath) {
   LLVMContext Ctx;
   std::unique_ptr<Module> M(new Module("TestModule", Ctx));
   Type *Int32Ty = Type::getInt32Ty(Ctx);
   GlobalVariable *Var =
       new GlobalVariable(*M, Int32Ty, true, GlobalValue::ExternalLinkage,