Fix LLVMSetMetadata for MDNodes that contain a single value

[dotdash]

Summary

MetadataAsValue uses a canonical format that strips the MDNode if it
contains only a single constant value. This triggers an assertion when
trying to cast the value to a MDNode.

[dotdash]

I guess I should mention that triggering the assertion is a regression from 3.5.

[dexonsmith]

AFK, sorry for the terseness.

Looks correct; thanks for working on this.

• Testcase?
• All the ternary operators are hard to read. I'd switch to ifs and early returns.
• Unwrapping Val twice is a little confusing (and, unnecessary).

• dpnes

[dotdash]

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.

[dotdash]

Added an MDNode extraction function with proper assertions, fixed
LLVMAddNamedMetadataOperand, and wrote a test for it.

[dotdash]

Updated the header in metadata.c to mention the second command implemented in that file. Sorry for the noise.

[dexonsmith]

LGTM.

• dpnes

llvm/trunk/lib/IR/Core.cpp

 LLVMValueRef LLVMGetMetadata(LLVMValueRef Inst, unsigned KindID) {
   auto *I = unwrap<Instruction>(Inst);
   assert(I && "Expected instruction");
   if (auto *MD = I->getMetadata(KindID))
     return wrap(MetadataAsValue::get(I->getContext(), MD));
   return nullptr;
 }
 
-void LLVMSetMetadata(LLVMValueRef Inst, unsigned KindID, LLVMValueRef MD) {
-  MDNode *N =
-      MD ? cast<MDNode>(unwrap<MetadataAsValue>(MD)->getMetadata()) : nullptr;
+// MetadataAsValue uses a canonical format which strips the actual MDNode for
+// MDNode with just a single constant value, storing just a ConstantAsMetadata
+// This undoes this canonicalization, reconstructing the MDNode.
+static MDNode *extractMDNode(MetadataAsValue *MAV) {
+  Metadata *MD = MAV->getMetadata();
+  assert((isa<MDNode>(MD) || isa<ConstantAsMetadata>(MD)) &&
+      "Expected a metadata node or a canonicalized constant");
+
+  if (MDNode *N = dyn_cast<MDNode>(MD))
+    return N;
+
+  return MDNode::get(MAV->getContext(), MD);
+}
+
+void LLVMSetMetadata(LLVMValueRef Inst, unsigned KindID, LLVMValueRef Val) {
+  MDNode *N = Val ? extractMDNode(unwrap<MetadataAsValue>(Val)) : nullptr;
+
   unwrap<Instruction>(Inst)->setMetadata(KindID, N);
 }
 
 /*--.. Conversion functions ................................................--*/
 
 #define LLVM_DEFINE_VALUE_CAST(name)                                       \
   LLVMValueRef LLVMIsA##name(LLVMValueRef Val) {                           \
     return wrap(static_cast<Value*>(dyn_cast_or_null<name>(unwrap(Val)))); \

 void LLVMAddNamedMetadataOperand(LLVMModuleRef M, const char* name,
                                  LLVMValueRef Val)
 {
   NamedMDNode *N = unwrap(M)->getOrInsertNamedMetadata(name);
   if (!N)
     return;
   if (!Val)
     return;
-  N->addOperand(cast<MDNode>(unwrap<MetadataAsValue>(Val)->getMetadata()));
+  N->addOperand(extractMDNode(unwrap<MetadataAsValue>(Val)));
 }
 
 /*--.. Operations on scalar constants ......................................--*/
 
 LLVMValueRef LLVMConstInt(LLVMTypeRef IntTy, unsigned long long N,
                           LLVMBool SignExtend) {
   return wrap(ConstantInt::get(unwrap<IntegerType>(IntTy), N, SignExtend != 0));
 }

llvm/trunk/test/Bindings/llvm-c/add_named_metadata_operand.ll

+; RUN: llvm-c-test --add-named-metadata-operand < /dev/null
+; This used to trigger an assertion

llvm/trunk/test/Bindings/llvm-c/set_metadata.ll

+; RUN: llvm-c-test --set-metadata < /dev/null
+; This used to trigger an assertion

llvm/trunk/tools/llvm-c-test/CMakeLists.txt

 
 add_llvm_tool(llvm-c-test
   calc.c
   disassemble.c
   helpers.c
   include-all.c
   main.c
   module.c
+  metadata.c
   object.c
   targets.c
   )
 
 if(USE_LLVM_DYLIB)
   target_link_libraries(llvm-c-test LLVM)
 endif()

llvm/trunk/tools/llvm-c-test/llvm-c-test.h

 int module_list_globals(void);
 
 // calc.c
 int calc(void);
 
 // disassemble.c
 int disassemble(void);
 
+// metadata.c
+int add_named_metadata_operand(void);
+int set_metadata(void);
+
 // object.c
 int object_list_sections(void);
 int object_list_symbols(void);
 
 // targets.c
 int targets_list(void);
 
 #endif

llvm/trunk/tools/llvm-c-test/main.c

int main(int argc, char **argv) {
   } else if (argc == 2 && !strcmp(argv[1], "--object-list-sections")) {
     return object_list_sections();
   } else if (argc == 2 && !strcmp(argv[1], "--object-list-symbols")) {
     return object_list_symbols();
   } else if (argc == 2 && !strcmp(argv[1], "--disassemble")) {
     return disassemble();
   } else if (argc == 2 && !strcmp(argv[1], "--calc")) {
     return calc();
+  } else if (argc == 2 && !strcmp(argv[1], "--add-named-metadata-operand")) {
+    return add_named_metadata_operand();
+  } else if (argc == 2 && !strcmp(argv[1], "--set-metadata")) {
+    return set_metadata();
   } else {
     print_usage();
   }
 
   return 1;
 }

llvm/trunk/tools/llvm-c-test/metadata.c

+/*===-- object.c - tool for testing libLLVM and llvm-c API ----------------===*\
+|*                                                                            *|
+|*                     The LLVM Compiler Infrastructure                       *|
+|*                                                                            *|
+|* This file is distributed under the University of Illinois Open Source      *|
+|* License. See LICENSE.TXT for details.                                      *|
+|*                                                                            *|
+|*===----------------------------------------------------------------------===*|
+|*                                                                            *|
+|* This file implements the --add-named-metadata-operand and --set-metadata   *|
+|* commands in llvm-c-test.                                                   *|
+|*                                                                            *|
+\*===----------------------------------------------------------------------===*/
+
+#include "llvm-c-test.h"
+#include "llvm-c/Core.h"
+
+int add_named_metadata_operand(void) {
+  LLVMModuleRef m = LLVMModuleCreateWithName("Mod");
+  LLVMValueRef values[] = { LLVMConstInt(LLVMInt32Type(), 0, 0) };
+
+  // This used to trigger an assertion
+  LLVMAddNamedMetadataOperand(m, "name", LLVMMDNode(values, 1));
+
+  return 0;
+}
+
+int set_metadata(void) {
+  LLVMBuilderRef b = LLVMCreateBuilder();
+  LLVMValueRef values[] = { LLVMConstInt(LLVMInt32Type(), 0, 0) };
+
+  // This used to trigger an assertion
+  LLVMSetMetadata(
+      LLVMBuildRetVoid(b),
+      LLVMGetMDKindID("kind", 4),
+      LLVMMDNode(values, 1));
+
+  return 0;
+}