Add all-in-one disassembler API

[athre0z]

The "recent" change of splitting out operand decoding increased the complexity of our "just take this buffer and give me a disassembled string" example quite a bit. We then added ZydisDecoderDecodeFull in order to simplify it again, but in my opinion it doesn't really do a great job at that goal, for the following reasons:

• The operand array still needs to be allocated and passed manually
• The function introduces a new set of decoder flags that are exclusively needed for this particular function. The term ZydisDecodingFlags can easily be confused with ZydisDecoderMode
• Users that look into the Decoder.h header now have to read through 60+ lines of documentation in order to figure out the difference between the two functions.

Summarizing, I think the problem here is that we attempted to provide a simplifying helper that is at the same time as powerful as the advanced API.

This PR goes a different route and adds an all-in-one API that does initializing, decoding and formatting in one step. It doesn't intend to expose all settings and advanced features that Zydis can provide, but instead aims towards providing what is frequently used with as little effort as possible. This API then serves as a starting point for new users, giving us some complexity budget for the "advanced" decoder API.

If this change is well-received, I suggest that it replaces the ZydisDecoderDecodeFull API.

[flobernd]

I like this addition to Zydis! However I don't think it should ever be an replacement for ZydisDecoderDecodeFull as the intended usecase is completely different:

ZydisDisassemble* -> I want a textual representation and nothing else ZydisDecoderDecode* -> I want to inspect the instruction in order to use/modify/... it

Besides that, I agree that ZydisDecoderDecodeFull is not the best way to go for especially this reason:

The operand array still needs to be allocated and passed manually

As already discussed internally, we could replace it by the "old style" decode function that uses a single struct containing a "max-sized" operands array.

[athre0z]

As already discussed internally, we could replace it by the "old style" decode function that uses a single struct containing a "max-sized" operands array.

Well yes, that'd be an improvement and get rid of the decoder flags.

[...] as the intended usecase is completely different:

I understand that these two functions are by no means equivalent. However, they both serve the same purpose: making the "standard" use-case more accessible and preventing potential users from immediately running away once they see a basic example that involves a ton of function calls.

If we keep another helper, I'm worried that we'll just make things more confusing again. We'd now have three different structs for decoded instructions:

• ZydisDecodedInstruction (ZydisDecoderDecodeInstruction API)
• ZydisDecodedInstructionAndOperands(?) (ZydisDecoderDecodeFull API)
• ZydisDisassembledInstruction (ZydisDisassembleXXX API).

I'm not strictly opposed to that, but I'm also not sure if it's worth the complexity. If users can get started with ZydisDisassembleXXX, switching to calling ZydisDecoderDecodeInstruction and ZydisDecoderDecodeOperands manually once they are familar with Zydis' basic use is probably no longer really a usability issue.

[flobernd]

I would keep Intel/ATT variants as they are now.

I tend to like the other way a little better, as it allows to select sub-styles like MASM as well.

First of all I would get rid of ZydisDecodingFlags

Yes, I think we all agree on this change at least!

• leave things as they are now

• make ZydisDisassembledInstruction a shared type and re-use it here (with text[0] = '\0')

I don't have a strong opinion here, both are fine for me. 2nd solution wastes some space but reduces argument count and makes it easier to pass results around.

Not an easy decision. If we see the "Full" API as a starting point and we decide to accept some performance impacts anyways, I'm fine with the second solution. Just the name of the struct could be confusing when reusing it.

[athre0z]

Generally all other files use nouns, so Disassemble.(h|c) would be the first one to break consistency here. I don't know if anyone cares about this though.

Hmmmm, good point. The file name generally also matches the "namespace" in the function names though, so if we were to rename it to Disassembler.h, we should also go with ZydisDisassemblerDisassembleIntel, which doesn't look as clean as the current variant. It'd however be consistent with the rest of the API.

---

Regarding ZydisDecoderDecodeFull, I think I'd be okay with keeping the operands in a separate argument as long as we can get rid of the decoding flags. My primary issue is that the function signature didn't make it obvious how many elements should be allocated, requiring users to carefully read the documentation. If we get rid of the flags, we can change the signature to contain the element count and get rid of the operand_count argument, e.g.:

ZYDIS_EXPORT ZyanStatus ZydisDecoderDecodeFull(const ZydisDecoder* decoder,
    const void* buffer, ZyanUSize length, ZydisDecodedInstruction* instruction,
    ZydisDecodedOperand operands[ZYDIS_MAX_OPERAND_COUNT]);

[mappzor]

Hmmmm, good point. The file name generally also matches the "namespace" in the function names though, so if we were to rename it to Disassembler.h, we should also go with ZydisDisassemblerDisassembleIntel, which doesn't look as clean as the current variant. It'd however be consistent with the rest of the API.

Considering that users just #include <Zydis/Zydis.h> I'd say that those "namespaces" are internal detail that won't concern most people. That's why I'd prioritize simpler function names. Just wanted to point this out as I didn't know if there was an existing rule about this or not.

ZYDIS_EXPORT ZyanStatus ZydisDecoderDecodeFull(const ZydisDecoder* decoder, 
    const void* buffer, ZyanUSize length, ZydisDecodedInstruction* instruction, 
    ZydisDecodedOperand operands[ZYDIS_MAX_OPERAND_COUNT]);

Looks good to me :)

One more thing, ZydisDisassembledInstruction.text[128]. Do we know how much is actually needed here? I'm not saying it should be "optimal", future-proofing is good but are we even approaching 64?

[athre0z]

Considering that users just #include <Zydis/Zydis.h> I'd say that those "namespaces" are internal detail that won't concern most people. That's why I'd prioritize simpler function names. Just wanted to point this out as I didn't know if there was an existing rule about this or not.

Yeah, I agree. There's no hard rule for this, just precedent / an implied rule from existing code.

One more thing, ZydisDisassembledInstruction.text[128]. Do we know how much is actually needed here? I'm not saying it should be "optimal", future-proofing is good but are we even approaching 64?

Yeah, going >64 is unfortunately quite possible: X86 is a pretty crazy ISA, after all! :-) For example:

6462F3EDA9039CC578563412FF

>>> len('valignq ymm3 {k1} {z}, ymm2, ymmword ptr fs:[rbp+rax*8+0x12345678], 0xFF')
72

Edit:

6462F3EDA9CF8CC578563412FF

>>> len('vgf2p8affineinvqb ymm1 {k1} {z}, ymm2, ymmword ptr fs:[rbp+rax*8+0x12345678], 0xFF')
82