[PATCH] Expose MCInst in C Disassembler API

Anders Waldenborg anders at 0x63.nu
Tue Jun 24 01:57:03 PDT 2014


I would find this to be a useful addition the the API. Especially if it could be extended with a function like LLVMMCInstIsFlowControl

================
Comment at: include/llvm-c/MCInst.h:53-61
@@ +52,11 @@
+ */
+typedef struct {
+  LLVMMCOperandType Kind;
+  union {
+    unsigned RegVal;
+    int64_t ImmVal;
+    double FPImmVal;
+    LLVMMCInstRef InstVal;
+  };
+} LLVMMCOperand;
+
----------------
Can't this struct be kept opaque, like almost every other exposed type in the llvm-c api? Instead create accessor functions.

Does the API get too clunky if you get rid of the LLVMMCOperandRef type entirely? And instead have functions like LLVMMCInstGetOperandKind, LLVMMCInstGetOperandRegVal and so on?

================
Comment at: lib/MC/MCInst.cpp:98-104
@@ +97,9 @@
+  MCOperand* OperandCPP = (MCOperand*)OperandR;
+  if (OperandCPP->isReg()) {
+    OperandC->Kind     = LLVMOTReg;
+    OperandC->RegVal   = OperandCPP->getReg();
+  } else if (OperandCPP->isImm()) {
+    OperandC->Kind     = LLVMOTImm;
+    OperandC->ImmVal   = OperandCPP->getImm();
+  } else if (OperandCPP->isReg()) {
+    OperandC->Kind     = LLVMOTFPImm;
----------------
This looks like copy-paste error of isReg here.

================
Comment at: tools/llvm-c-test/disassemble.c:46-49
@@ -45,6 +45,6 @@
 
   pos = 0;
   while (pos < siz) {
-    size_t l = LLVMDisasmInstruction(D, buf + pos, siz - pos, 0, outline,
-                                     sizeof(outline));
+    size_t l = LLVMDisasmInstruction(D, buf + pos, siz - pos, 0, NULL,
+                                     outline, sizeof(outline));
     if (!l) {
----------------
It would be nice if the tests requested an MCInst here and dumped some information from it to make the new code at least run on tests. If you could add some checks on the output even better.

http://reviews.llvm.org/D4236






More information about the llvm-commits mailing list