Add all-in-one disassembler API #379

athre0z · 2022-09-11T14:23:18Z

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.

athre0z · 2022-09-11T14:26:09Z

src/Disassemble.c

+    ZydisStackWidth stack_width;
+    switch (machine_mode)
+    {
+    case ZYDIS_MACHINE_MODE_LONG_64:
+        stack_width = ZYDIS_STACK_WIDTH_64;
+        break;
+    case ZYDIS_MACHINE_MODE_LONG_COMPAT_32:
+    case ZYDIS_MACHINE_MODE_LEGACY_32:
+        stack_width = ZYDIS_STACK_WIDTH_32;
+        break;
+    case ZYDIS_MACHINE_MODE_LONG_COMPAT_16:
+    case ZYDIS_MACHINE_MODE_LEGACY_16:
+    case ZYDIS_MACHINE_MODE_REAL_16:
+        stack_width = ZYDIS_STACK_WIDTH_16;
+        break;
+    default:
+        return ZYAN_STATUS_INVALID_ARGUMENT;
+    }


The need to specify the stack width separately tends to be confusing to new users and is rarely used.

athre0z · 2022-09-11T14:29:13Z

src/Disassemble.c

+        instruction->operands, instruction->info.operand_count));
+
+    ZydisFormatter formatter;
+    ZYAN_CHECK(ZydisFormatterInit(&formatter, style));


While it might seem a bit insane to re-initialize the decoder and formatter on each call, it significantly simplifies the API. As an alternative, we could put a pre-initialized context into .rdata: it'd cost us about 1KiB of binary size for the two variants (Intel and AT&T). On the other hand, the overhead for re-doing this on every call is roughly 3%, so I'm not sure whether that makes sense. We could also initialize a context in .data on the first call, but that'd require us to implement some sort of spin lock to keep in thread-safe.

athre0z · 2022-09-11T14:31:05Z

include/Zydis/Disassemble.h

+ZYDIS_EXPORT ZyanStatus ZydisDisassembleIntel(ZydisMachineMode machine_mode,
+    ZyanUSize runtime_address, const void* buffer, ZyanUSize length,
+    ZydisDisassembledInstruction *instruction);


I went with one function for each of the two major formatter flavors here to keep the argument count low. I don't have a strong take on this and am open towards instead making this a regular argument.

flobernd · 2022-09-11T14:34:47Z

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 · 2022-09-11T14:59:34Z

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.

mappzor

I really like this new API. I would keep Intel/ATT variants as they are now. One thing I would like to point out is file naming. 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.

About ZydisDecoderDecodeFull, I would definitely keep it. It provides an easy path for porting code from older versions. It's very helpful for use cases that don't need formatting. We can simplify it a bit though. First of all I would get rid of ZydisDecodingFlags as decoding only visible arguments is basically for performance only. I think it's fair to delegate this to advanced API. Second thing is how to handle instruction & operands. I think we should avoid adding a new structure here. This leaves us with two options:

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.

src/Disassemble.c

flobernd · 2022-09-12T06:11:15Z

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 · 2022-09-12T10:56:05Z

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]);

include/Zydis/Disassemble.h

mappzor · 2022-09-12T18:23:09Z

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 · 2022-09-12T19:46:18Z

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

flobernd

LGTM!

Still not sure if we should keep the Intel/ATT suffix instead of using an extra argument, but I'm fine with either way in the end.

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.

athre0z added C-feature Category: Addition of a new feature P-medium Priority: Medium labels Sep 11, 2022

athre0z commented Sep 11, 2022

View reviewed changes

athre0z marked this pull request as ready for review September 11, 2022 14:32

athre0z requested a review from flobernd September 11, 2022 14:32

athre0z force-pushed the all-in-one-helper branch from a52b4d0 to 4a58237 Compare September 11, 2022 15:10

mappzor reviewed Sep 11, 2022

View reviewed changes

src/Disassemble.c Outdated Show resolved Hide resolved

ZehMatt reviewed Sep 12, 2022

View reviewed changes

include/Zydis/Disassemble.h Outdated Show resolved Hide resolved

flobernd approved these changes Sep 13, 2022

View reviewed changes

athre0z force-pushed the all-in-one-helper branch from 0704742 to bd338fd Compare September 13, 2022 10:26

athre0z force-pushed the all-in-one-helper branch from bd338fd to 95041b4 Compare October 9, 2022 16:21

athre0z added 5 commits October 9, 2022 18:23

Disassemble: add function for decoding and formatting in one step

b2e3b83

Examples: add DisassembleSimple

1168a2d

Examples: add @file comment to Disassemble example

70018c3

README: switch decoder example to simplified version

565c2f3

CMake: simplify example build rules

ddf9ef3

athre0z force-pushed the all-in-one-helper branch from 95041b4 to ddf9ef3 Compare October 9, 2022 16:23

Deps: update zycore to v1.3.0

4ba0ba6

athre0z merged commit 0002161 into master Oct 9, 2022

athre0z deleted the all-in-one-helper branch October 9, 2022 16:44

athre0z mentioned this pull request Oct 9, 2022

Decoder: simplify ZydisDecoderDecodeFull API #380

Merged

oberrich mentioned this pull request Nov 26, 2022

ZydisDecodingFlags was removed prior to release of 4.0 #406

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add all-in-one disassembler API #379

Add all-in-one disassembler API #379

athre0z commented Sep 11, 2022 •

edited

Loading

athre0z Sep 11, 2022

athre0z Sep 11, 2022

athre0z Sep 11, 2022 •

edited

Loading

flobernd commented Sep 11, 2022

athre0z commented Sep 11, 2022 •

edited

Loading

mappzor left a comment

flobernd commented Sep 12, 2022

athre0z commented Sep 12, 2022 •

edited

Loading

mappzor commented Sep 12, 2022

athre0z commented Sep 12, 2022 •

edited

Loading

flobernd left a comment

Add all-in-one disassembler API #379

Add all-in-one disassembler API #379

Conversation

athre0z commented Sep 11, 2022 • edited Loading

athre0z Sep 11, 2022

Choose a reason for hiding this comment

athre0z Sep 11, 2022

Choose a reason for hiding this comment

athre0z Sep 11, 2022 • edited Loading

Choose a reason for hiding this comment

flobernd commented Sep 11, 2022

athre0z commented Sep 11, 2022 • edited Loading

mappzor left a comment

Choose a reason for hiding this comment

flobernd commented Sep 12, 2022

athre0z commented Sep 12, 2022 • edited Loading

mappzor commented Sep 12, 2022

athre0z commented Sep 12, 2022 • edited Loading

flobernd left a comment

Choose a reason for hiding this comment

athre0z commented Sep 11, 2022 •

edited

Loading

athre0z Sep 11, 2022 •

edited

Loading

athre0z commented Sep 11, 2022 •

edited

Loading

athre0z commented Sep 12, 2022 •

edited

Loading

athre0z commented Sep 12, 2022 •

edited

Loading