Merging r277625:

------------------------------------------------------------------------
r277625 | dexonsmith | 2016-08-03 11:19:43 -0700 (Wed, 03 Aug 2016) | 42 lines

IR: Drop uniquing when an MDNode Value operand is deleted

This is a fix for PR28697.

An MDNode can indirectly refer to a GlobalValue, through a
ConstantAsMetadata.  When the GlobalValue is deleted, the MDNode operand
is reset to `nullptr`.  If the node is uniqued, this can lead to a
hard-to-detect cache invalidation in a Metadata map that's shared across
an LLVMContext.

Consider:

 1. A map from Metadata* to `T` called RemappedMDs.
 2. A node that references a global variable, `!{i1* @GV}`.
 3. Insert `!{i1* @GV} -> SomeT` in the map.
 4. Delete `@GV`, leaving behind `!{null} -> SomeT`.

Looking up the generic and uninteresting `!{null}` gives you `SomeT`,
which is likely related to `@GV`.  Worse, `SomeT`'s lifetime may be tied
to the deleted `@GV`.

This occurs in practice in the shared ValueMap used since r266579 in the
IRMover.  Other code that handles more than one Module (with different
lifetimes) in the same LLVMContext could hit it too.

The fix here is a partial revert of r225223: in the rare case that an
MDNode operand is a ConstantAsMetadata (i.e., wrapping a node from the
Value hierarchy), drop uniquing if it gets replaced with `nullptr`.
This changes step #4 above to leave behind `distinct !{null} -> SomeT`,
which can't be confused with the generic `!{null}`.

In theory, this can cause some churn in the LLVMContext's MDNode
uniquing map when Values are being deleted.  However:

  - The number of GlobalValues referenced from uniqued MDNodes is
    expected to be quite small.  E.g., the debug info metadata schema
    only references GlobalValues from distinct nodes.

  - Other Constants have the lifetime of the LLVMContext, whose teardown
    is careful to drop references before deleting the constants.

As a result, I don't expect a compile time regression from this change.
------------------------------------------------------------------------

git-svn-id: https://llvm.org/svn/llvm-project/llvm/branches/release_39@277639 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/lib/IR/Metadata.cpp b/lib/IR/Metadata.cpp
index 5201c2ecce6ae0e34d9d68c64c20d139d8a1352d..f35c64b27b5bbeba964c1217f3dec1f72e7f001c 100644 (file)
--- a/lib/IR/Metadata.cpp
+++ b/lib/IR/Metadata.cpp
@@ -675,8 +675,8 @@ void MDNode::handleChangedOperand(void *Ref, Metadata *New) {
   Metadata *Old = getOperand(Op);
   setOperand(Op, New);
 
-  // Drop uniquing for self-reference cycles.
-  if (New == this) {
+  // Drop uniquing for self-reference cycles and deleted constants.
+  if (New == this || (!New && Old && isa<ConstantAsMetadata>(Old))) {
     if (!isResolved())
       resolve();
     storeDistinctInContext();

diff --git a/test/Linker/Inputs/metadata-with-global-value-operand.ll b/test/Linker/Inputs/metadata-with-global-value-operand.ll
new file mode 100644 (file)
index 0000000..21d3e27
--- /dev/null
+++ b/test/Linker/Inputs/metadata-with-global-value-operand.ll
@@ -0,0 +1,3 @@
+!named.null = !{!0}
+
+!0 = !{null}

diff --git a/test/Linker/metadata-with-global-value-operand.ll b/test/Linker/metadata-with-global-value-operand.ll
new file mode 100644 (file)
index 0000000..fb4c01a
--- /dev/null
+++ b/test/Linker/metadata-with-global-value-operand.ll
@@ -0,0 +1,14 @@
+; RUN: llvm-link -S -o - %s %S/Inputs/metadata-with-global-value-operand.ll | FileCheck %s
+; This test confirms that the !{null} from the second module doesn't get mapped
+; onto the abandoned !{i1* @var} node from this module.
+
+; CHECK: @var = global
+@var = global i1 false
+
+; CHECK: !named.vars = !{!0}
+; CHECK: !named.null = !{!1}
+!named.vars = !{!0}
+
+; CHECK: !0 = !{i1* @var}
+; CHECK: !1 = !{null}
+!0 = !{i1* @var}

diff --git a/test/Transforms/GlobalOpt/metadata.ll b/test/Transforms/GlobalOpt/metadata.ll
index 152d58e6e3200be00dbea11acdc5ca511d43cdd6..b766349d506ba7fd304221b6bfe3510d3b3884ac 100644 (file)
--- a/test/Transforms/GlobalOpt/metadata.ll
+++ b/test/Transforms/GlobalOpt/metadata.ll
@@ -28,5 +28,5 @@ declare void @llvm.foo(metadata, metadata) nounwind readnone
 ; CHECK: !named = !{![[NULL:[0-9]+]]}
 
 !0 = !{i8*** @G}
-; CHECK-DAG: ![[NULL]] = !{null}
+; CHECK-DAG: ![[NULL]] = distinct !{null}
 ; CHECK-DAG: ![[EMPTY]] = !{}

diff --git a/unittests/IR/MetadataTest.cpp b/unittests/IR/MetadataTest.cpp
index 77a2dbaf2dfa863b3141cc2b0a2d57a8c1f99e68..15b03b3d5720e9fac4db029bb0024a86738344a2 100644 (file)
--- a/unittests/IR/MetadataTest.cpp
+++ b/unittests/IR/MetadataTest.cpp
@@ -449,6 +449,40 @@ TEST_F(MDNodeTest, DistinctOnUniquingCollision) {
   EXPECT_FALSE(Wrapped1->isDistinct());
 }
 
+TEST_F(MDNodeTest, UniquedOnDeletedOperand) {
+  // temp !{}
+  TempMDTuple T = MDTuple::getTemporary(Context, None);
+
+  // !{temp !{}}
+  Metadata *Ops[] = {T.get()};
+  MDTuple *N = MDTuple::get(Context, Ops);
+
+  // !{temp !{}} => !{null}
+  T.reset();
+  ASSERT_TRUE(N->isUniqued());
+  Metadata *NullOps[] = {nullptr};
+  ASSERT_EQ(N, MDTuple::get(Context, NullOps));
+}
+
+TEST_F(MDNodeTest, DistinctOnDeletedValueOperand) {
+  // i1* @GV
+  Type *Ty = Type::getInt1PtrTy(Context);
+  std::unique_ptr<GlobalVariable> GV(
+      new GlobalVariable(Ty, false, GlobalValue::ExternalLinkage));
+  ConstantAsMetadata *Op = ConstantAsMetadata::get(GV.get());
+
+  // !{i1* @GV}
+  Metadata *Ops[] = {Op};
+  MDTuple *N = MDTuple::get(Context, Ops);
+
+  // !{i1* @GV} => !{null}
+  GV.reset();
+  ASSERT_TRUE(N->isDistinct());
+  ASSERT_EQ(nullptr, N->getOperand(0));
+  Metadata *NullOps[] = {nullptr};
+  ASSERT_NE(N, MDTuple::get(Context, NullOps));
+}
+
 TEST_F(MDNodeTest, getDistinct) {
   // !{}
   MDNode *Empty = MDNode::get(Context, None);
@@ -669,7 +703,7 @@ TEST_F(MDNodeTest, replaceWithUniquedResolvingOperand) {
   EXPECT_TRUE(N->isResolved());
 }
 
-TEST_F(MDNodeTest, replaceWithUniquedChangingOperand) {
+TEST_F(MDNodeTest, replaceWithUniquedDeletedOperand) {
   // i1* @GV
   Type *Ty = Type::getInt1PtrTy(Context);
   std::unique_ptr<GlobalVariable> GV(
@@ -686,8 +720,33 @@ TEST_F(MDNodeTest, replaceWithUniquedChangingOperand) {
 
   // !{i1* @GV} => !{null}
   GV.reset();
-  ASSERT_TRUE(N->isUniqued());
+  ASSERT_TRUE(N->isDistinct());
+  ASSERT_EQ(nullptr, N->getOperand(0));
   Metadata *NullOps[] = {nullptr};
+  ASSERT_NE(N, MDTuple::get(Context, NullOps));
+}
+
+TEST_F(MDNodeTest, replaceWithUniquedChangedOperand) {
+  // i1* @GV
+  Type *Ty = Type::getInt1PtrTy(Context);
+  std::unique_ptr<GlobalVariable> GV(
+      new GlobalVariable(Ty, false, GlobalValue::ExternalLinkage));
+  ConstantAsMetadata *Op = ConstantAsMetadata::get(GV.get());
+
+  // temp !{i1* @GV}
+  Metadata *Ops[] = {Op};
+  MDTuple *N = MDTuple::getTemporary(Context, Ops).release();
+
+  // temp !{i1* @GV} => !{i1* @GV}
+  ASSERT_EQ(N, MDNode::replaceWithUniqued(TempMDTuple(N)));
+  ASSERT_TRUE(N->isUniqued());
+
+  // !{i1* @GV} => !{i1* @GV2}
+  std::unique_ptr<GlobalVariable> GV2(
+      new GlobalVariable(Ty, false, GlobalValue::ExternalLinkage));
+  GV->replaceAllUsesWith(GV2.get());
+  ASSERT_TRUE(N->isUniqued());
+  Metadata *NullOps[] = {ConstantAsMetadata::get(GV2.get())};
   ASSERT_EQ(N, MDTuple::get(Context, NullOps));
 }