From 9cbc69d1fe16da6d243f1ce9b4564d8e551b5372 Mon Sep 17 00:00:00 2001 From: "Duncan P. N. Exon Smith" Date: Wed, 3 Aug 2016 18:19:43 +0000 Subject: [PATCH] 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. llvm-svn: 277625 --- llvm/lib/IR/Metadata.cpp | 4 +- .../Inputs/metadata-with-global-value-operand.ll | 3 ++ .../Linker/metadata-with-global-value-operand.ll | 14 +++++ llvm/test/Transforms/GlobalOpt/metadata.ll | 2 +- llvm/unittests/IR/MetadataTest.cpp | 63 +++++++++++++++++++++- 5 files changed, 81 insertions(+), 5 deletions(-) create mode 100644 llvm/test/Linker/Inputs/metadata-with-global-value-operand.ll create mode 100644 llvm/test/Linker/metadata-with-global-value-operand.ll diff --git a/llvm/lib/IR/Metadata.cpp b/llvm/lib/IR/Metadata.cpp index 8b8e4dc..cbbd808 100644 --- a/llvm/lib/IR/Metadata.cpp +++ b/llvm/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(Old))) { if (!isResolved()) resolve(); storeDistinctInContext(); diff --git a/llvm/test/Linker/Inputs/metadata-with-global-value-operand.ll b/llvm/test/Linker/Inputs/metadata-with-global-value-operand.ll new file mode 100644 index 0000000..21d3e27 --- /dev/null +++ b/llvm/test/Linker/Inputs/metadata-with-global-value-operand.ll @@ -0,0 +1,3 @@ +!named.null = !{!0} + +!0 = !{null} diff --git a/llvm/test/Linker/metadata-with-global-value-operand.ll b/llvm/test/Linker/metadata-with-global-value-operand.ll new file mode 100644 index 0000000..fb4c01a --- /dev/null +++ b/llvm/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/llvm/test/Transforms/GlobalOpt/metadata.ll b/llvm/test/Transforms/GlobalOpt/metadata.ll index 152d58e..b766349 100644 --- a/llvm/test/Transforms/GlobalOpt/metadata.ll +++ b/llvm/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/llvm/unittests/IR/MetadataTest.cpp b/llvm/unittests/IR/MetadataTest.cpp index 77a2dba..15b03b3 100644 --- a/llvm/unittests/IR/MetadataTest.cpp +++ b/llvm/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 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 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 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 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)); } -- 2.7.4