[llvm] r225657 - IR: Add test for handleChangedOperand() recursion

[Duncan P. N. Exon Smith]

> On 2015-Jan-12, at 14:44, Alexey Samsonov <vonosmas at gmail.com> wrote:
> 
> Hi Duncan,
> 
> ASan bootstrap detects memory leak in the test: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/603/steps/check-llvm%20asan/logs/stdio
> 

Sorry I missed this!

> On Mon, Jan 12, 2015 at 11:22 AM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> Author: dexonsmith
> Date: Mon Jan 12 13:22:04 2015
> New Revision: 225657
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=225657&view=rev
> Log:
> IR: Add test for handleChangedOperand() recursion
> 
> Turns out this can happen.  Remove the `FIXME` and add a testcase that
> crashes without the extra logic.
> 
> Modified:
>     llvm/trunk/lib/IR/Metadata.cpp
>     llvm/trunk/unittests/IR/MetadataTest.cpp
> 
> Modified: llvm/trunk/lib/IR/Metadata.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Metadata.cpp?rev=225657&r1=225656&r2=225657&view=diff
> ==============================================================================
> --- llvm/trunk/lib/IR/Metadata.cpp (original)
> +++ llvm/trunk/lib/IR/Metadata.cpp Mon Jan 12 13:22:04 2015
> @@ -537,8 +537,6 @@ void GenericMDNode::handleChangedOperand
>    if (InRAUW) {
>      // We just hit a recursion due to RAUW.  Set the operand and move on, since
>      // we're about to be deleted.
> -    //
> -    // FIXME: Can this cycle really happen?
>      setOperand(Op, New);
>      return;
>    }
> 
> Modified: llvm/trunk/unittests/IR/MetadataTest.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/IR/MetadataTest.cpp?rev=225657&r1=225656&r2=225657&view=diff
> ==============================================================================
> --- llvm/trunk/unittests/IR/MetadataTest.cpp (original)
> +++ llvm/trunk/unittests/IR/MetadataTest.cpp Mon Jan 12 13:22:04 2015
> @@ -293,6 +293,69 @@ TEST_F(MDNodeTest, getDistinctWithUnreso
>    EXPECT_EQ(Empty, Distinct->getOperand(0));
>  }
> 
> +TEST_F(MDNodeTest, handleChangedOperandRecursion) {
> +  // !0 = !{}
> +  MDNode *N0 = MDNode::get(Context, None);
> +
> +  // !1 = !{!3, null}
> +  MDNodeFwdDecl *Temp3 = MDNode::getTemporary(Context, None);
> +  Metadata *Ops1[] = {Temp3, nullptr};
> +  MDNode *N1 = MDNode::get(Context, Ops1);
> +
> +  // !2 = !{!3, !0}
> +  Metadata *Ops2[] = {Temp3, N0};
> +  MDNode *N2 = MDNode::get(Context, Ops2);
> +
> +  // !3 = !{!2}
> +  Metadata *Ops3[] = {N2};
> +  MDNode *N3 = MDNode::get(Context, Ops3);
> +  Temp3->replaceAllUsesWith(N3);
> 
> ^^
> Can/should you delete Temp3 now?

Yup, r225721.  Thanks!

>  
> +
> +  // !4 = !{!1}
> +  Metadata *Ops4[] = {N1};
> +  MDNode *N4 = MDNode::get(Context, Ops4);
> +
> +  // Confirm that the cycle prevented RAUW from getting dropped.
> +  EXPECT_TRUE(N0->isResolved());
> +  EXPECT_FALSE(N1->isResolved());
> +  EXPECT_FALSE(N2->isResolved());
> +  EXPECT_FALSE(N3->isResolved());
> +  EXPECT_FALSE(N4->isResolved());
> +
> +  // Create a couple of distinct nodes to observe what's going on.
> +  //
> +  // !5 = distinct !{!2}
> +  // !6 = distinct !{!3}
> +  Metadata *Ops5[] = {N2};
> +  MDNode *N5 = MDNode::getDistinct(Context, Ops5);
> +  Metadata *Ops6[] = {N3};
> +  MDNode *N6 = MDNode::getDistinct(Context, Ops6);
> +
> +  // Mutate !2 to look like !1, causing a uniquing collision (and an RAUW).
> +  // This will ripple up, with !3 colliding with !4, and RAUWing.  Since !2
> +  // references !3, this can cause a re-entry of handleChangedOperand() when !3
> +  // is not ready for it.
> +  //
> +  // !2->replaceOperandWith(1, nullptr)
> +  // !2: !{!3, !0} => !{!3, null}
> +  // !2->replaceAllUsesWith(!1)
> +  // !3: !{!2] => !{!1}
> +  // !3->replaceAllUsesWith(!4)
> +  N2->replaceOperandWith(1, nullptr);
> +
> +  // If all has gone well, N2 and N3 will have been RAUW'ed and deleted from
> +  // under us.  Just check that the other nodes are sane.
> +  //
> +  // !1 = !{!4, null}
> +  // !4 = !{!1}
> +  // !5 = distinct !{!1}
> +  // !6 = distinct !{!4}
> +  EXPECT_EQ(N4, N1->getOperand(0));
> +  EXPECT_EQ(N1, N4->getOperand(0));
> +  EXPECT_EQ(N1, N5->getOperand(0));
> +  EXPECT_EQ(N4, N6->getOperand(0));
> +}
> +
>  typedef MetadataTest MetadataAsValueTest;
> 
>  TEST_F(MetadataAsValueTest, MDNode) {
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 
> 
> 
> -- 
> Alexey Samsonov
> vonosmas at gmail.com