[PATCH] Expose MCInst in C Disassembler API

[Alp Toker]

On 22/06/2014 18:36, Filip Pizlo 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.

Yes, that's the point. I'm suggesting that the burden of stability 
should be pushed on clients in some cases. Especially like this where 
the underlying facility is fairly new and still being shaped up.

Let's focus on making the internal C++ API flexible yet readily 
consumable if people want to do that, with no guarantees, and providing 
a tidy high-level C wrapper like the existing LLVMDisasmInstruction() 
for people who need absolute stability. The C++ interface doesn't break 
every month, but when it does I think anyone tracking ToT would be 
willing to make the small tweak to their code to adapt. That gives the 
best of both worlds.

I guess you're familiar with JavaScriptCore? Note how that project is 
best known for its JIT, yet its stable C API focuses only on the primary 
interface, not exposing JSC's bytecode, JIT or developer facilities. We 
should seek to learn from and emulate the success stories -- don't do a 
libclang :-)

Alp.





>
>> 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.
>
> -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
>>

-- 
http://www.nuanti.com
the browser experts