[llvm] r225738 - IR: Fix an inverted assertion when replacing resolved operands

[Duncan P. N. Exon Smith]

> On 2015-Jan-12, at 16:23, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Mon, Jan 12, 2015 at 4:10 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> Author: dexonsmith
> Date: Mon Jan 12 18:10:38 2015
> New Revision: 225738
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=225738&view=rev
> Log:
> IR: Fix an inverted assertion when replacing resolved operands
> 
> Add a unit test, since this bug was only exposed by clang tests.  Thanks
> to Rafael for tracking this down!
> 
> 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=225738&r1=225737&r2=225738&view=diff
> ==============================================================================
> --- llvm/trunk/lib/IR/Metadata.cpp (original)
> +++ llvm/trunk/lib/IR/Metadata.cpp Mon Jan 12 18:10:38 2015
> @@ -444,7 +444,7 @@ void UniquableMDNode::resolveAfterOperan
> 
>    // Check if an operand was resolved.
>    if (!isOperandUnresolved(Old))
> -    assert(isOperandUnresolved(New) && "Operand just became unresolved");
> +    assert(!isOperandUnresolved(New) && "Operand just became unresolved");
> 
> Could the if condition be rolled into the assertion here, or does it have side effects? (tricksy name if it does)
>  

Would have been a good idea.  See r225745 though.  This assertion was
overzealous, even when written as I'd intended.

>    else if (!isOperandUnresolved(New))
>      decrementUnresolvedOperandCount();
>  }
> 
> Modified: llvm/trunk/unittests/IR/MetadataTest.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/IR/MetadataTest.cpp?rev=225738&r1=225737&r2=225738&view=diff
> ==============================================================================
> --- llvm/trunk/unittests/IR/MetadataTest.cpp (original)
> +++ llvm/trunk/unittests/IR/MetadataTest.cpp Mon Jan 12 18:10:38 2015
> @@ -7,6 +7,7 @@
>  //
>  //===----------------------------------------------------------------------===//
> 
> +#include "llvm/ADT/STLExtras.h"
>  #include "llvm/IR/Metadata.h"
>  #include "llvm/IR/Constants.h"
>  #include "llvm/IR/Instructions.h"
> @@ -361,6 +362,28 @@ TEST_F(MDNodeTest, handleChangedOperandR
>    EXPECT_EQ(N4, N6->getOperand(0));
>  }
> 
> +TEST_F(MDNodeTest, replaceResolvedOperand) {
> +  // Check code for replacing one resolved operand with another.  If doing this
> +  // directly (via replaceOperandWith()) becomes illegal, change the operand to
> +  // a global value that gets RAUW'ed.
> +  //
> +  // Use a temporary node to keep N from being resolved.
> +  std::unique_ptr<MDNodeFwdDecl> Temp(MDNodeFwdDecl::get(Context, None));
> +  Metadata *Ops[] = {nullptr, Temp.get()};
> +
> +  MDNode *Empty = MDTuple::get(Context, {});
> +  MDNode *N = MDTuple::get(Context, Ops);
> +  EXPECT_EQ(nullptr, N->getOperand(0));
> +  ASSERT_FALSE(N->isResolved());
> +
> +  // Check code for replacing resolved nodes.
> +  N->replaceOperandWith(0, Empty);
> +  EXPECT_EQ(Empty, N->getOperand(0));
> +
> +  // Remove the reference to Temp; required for teardown.
> +  N->replaceOperandWith(1, nullptr);
> +}
> +
>  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
>