[PATCH] Expose MCInst in C Disassembler API

[Eric Christopher]

On Sun, Jun 22, 2014 at 8:36 AM, Filip Pizlo <fpizlo at apple.com> wrote:
>
>
>> On Jun 22, 2014, at 2:01 AM, Alp Toker <alp at nuanti.com> wrote:
>>
>>
>>> On 20/06/2014 23:38, Filip Pizlo wrote:
>>> It’s great to see more things being exposed in the C API, and I think that this is generally the right direction.
>>
>> It makes a lot more sense to use/wrap the C++ embedding API if you need to go beyond the strings offered by LLVMDisasmInstruction().
>
> The C++ API is not meant to be stable, so it's not appropriate for some clients.
>
>>
>> libclang over-exposed internals just like that, and now clang SVN has multiple copies of entire subsystems in order to maintain stability.
>
> This is an inevitable reality for any serious software project.
>

Then I'd rather actually have a versioned API for it.

That said, Kevin's comment is the one that matters for this review thread.

-eric

> -Filip
>
>>
>> Alp.
>>
>>
>>>
>>> Some comments:
>>>
>>>>  size_t LLVMDisasmInstruction(LLVMDisasmContextRef DC, uint8_t *Bytes,
>>>>                               uint64_t BytesSize, uint64_t PC,
>>>> +                             LLVMMCInstRef* Inst,
>>>>                               char *OutString, size_t OutStringSize);
>>> Adding a new parameter to this function will break previous callers to this function.  The C API should be stable.  Hence, I think that you need to add a new version of LLVMDisasmInstruction().
>>>
>>>> +/*===-- llvm-c/MCInst.h - Disassembler Public C Interface ---*- C -*-===*\
>>> It feels like maybe this should just go into an existing file, rather than adding a new file.  It seems like the current convention in the C API is to have a smaller number of more populous headers.
>>>
>>>> +typedef struct {
>>>> +  LLVMMCOperandType Kind;
>>>> +  union {
>>>> +    unsigned RegVal;
>>>> +    int64_t ImmVal;
>>>> +    double FPImmVal;
>>>> +    LLVMMCInstRef InstVal;
>>>> +  };
>>>> +} LLVMMCOperand;
>>> I’m worried that defining the struct in this way will result in compatibility hassles in the future.  Can we be sure that we won’t ever change the types of any of these things?
>>>
>>> I would feel better if operands were represented as an OperandRef with functions to get their contents.  This would allow for forward/backward ABI compatibility if new fields were added or existing fields had their types changed.
>>>
>>> -Filip
>>>
>>>
>>>
>>>> On Jun 20, 2014, at 12:08 PM, Matthew Maurer <maurer+llvmphabricator at matthewmaurer.org> wrote:
>>>>
>>>> Partially expose the MCInst struct from the C Disassembler API to allow mechanized examination of opcodes and arguments.
>>>>
>>>> http://reviews.llvm.org/D4236
>>>>
>>>> Files:
>>>>  include/llvm-c/Disassembler.h
>>>>  include/llvm-c/MCInst.h
>>>>  lib/MC/MCDisassembler/Disassembler.cpp
>>>>  lib/MC/MCInst.cpp
>>>>  tools/llvm-c-test/disassemble.c
>>>> <D4236.10701.patch>_______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>> --
>> http://www.nuanti.com
>> the browser experts
>>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits