[PATCH] Fix LLVMSetMetadata for MDNodes that contain a single value

[Duncan P. N. Exon Smith]

> On 2015 Jan 27, at 15:00, Björn Steinbrink <bsteinbr at gmail.com> wrote:
> 
> Thanks for the feedback!
> 
> Added a testcase and replaced the ternary operators with if statements.
> 
> I did not use an early return for the !Val case though, because that would,
> AFAICT, stop the user from removing existing metadata.
> 
> 
> http://reviews.llvm.org/D7165
> 
> Files:
>  lib/IR/Core.cpp
>  test/Bindings/llvm-c/metadata.ll
>  tools/llvm-c-test/CMakeLists.txt
>  tools/llvm-c-test/llvm-c-test.h
>  tools/llvm-c-test/main.c
>  tools/llvm-c-test/metadata.c
> 
> EMAIL PREFERENCES
>  http://reviews.llvm.org/settings/panel/emailpreferences/
> <D7165.18856.patch>

Looking closer I think this fix is too lenient..

> Index: lib/IR/Core.cpp
> ===================================================================
> --- lib/IR/Core.cpp
> +++ lib/IR/Core.cpp
> @@ -563,9 +563,15 @@
>    return nullptr;
>  }
>  
> -void LLVMSetMetadata(LLVMValueRef Inst, unsigned KindID, LLVMValueRef MD) {
> -  MDNode *N =
> -      MD ? cast<MDNode>(unwrap<MetadataAsValue>(MD)->getMetadata()) : nullptr;
> +void LLVMSetMetadata(LLVMValueRef Inst, unsigned KindID, LLVMValueRef Val) {
> +  MDNode *N = nullptr;
> +  if (Val) {
> +    MetadataAsValue *MAV = unwrap<MetadataAsValue>(Val);
> +    Metadata *MD = MAV->getMetadata();

I think you should add this assert:

    assert(isa<MDNode>(MD) || isa<ConstantAsMetadata>(MD) &&
           "Expected a metadata node or a canonicalized constant");

This only adds the `MDNode` wrapper when the canonicalization could
have stripped it, and otherwise demands correct API use.

> +    if (!(N = dyn_cast<MDNode>(MD)))
> +      N = MDNode::get(MAV->getContext(), MD);

I suspect there's a similar bug in `LLVMAddNamedMetadataOperand()`,
so you might as well fix that too while you're in there.  I'd extract
this logic into a function, something like:

    /// Some comment about reversing the canonicalization.
    static MDNode *extractMDNode(MetadataAsValue *MAV) {
      // ...
    }

and reuse it in both places.  You should probably add a test for that
too.

> +  }
> +
>    unwrap<Instruction>(Inst)->setMetadata(KindID, N);
>  }
>