New APIs for better performance - by pre-allocting memory

[aquynh]

Here is a proposal to speed up the disassembling process by pre-allocating memory used for the output instructions.

https://github.com/aquynh/capstone/wiki/New-APIs:-cs_disasm_alloc()-&-cs_disasm_buf()

Please comment, thanks.

[bnagy]

Option 1 seems clean and simple. If people persist in disassembling only one instruction they can still do it via the first method. Hell you can even macro it with a hardcoded count ;)

[hlide]

"for count = 1, you also need to update code, size & address, no?" I need to do so but not the function itself unlike for count > 1.

[hlide]

Proposal 2 is what I need. I prefer to analyze one instruction after one to decide how to disassemble the next instruction (for instance, "jump in the middle" trick or data detection after recording the prior instructions referring the current address as being a data and not an instruction).

Proposal 1 is cumbersome to use if count > 1 and suboptimal for count == 1 as we do extra code inside the function for nothing. What to do if count == 0?

[hlide]

Proposal 2:

• cs_disasm_iter parameter doesn't need cs_insn **insn but only cs_insn *insn.
• the updating of those three parameters, I prefer to let the user to do so instead of the function. if you just want to count instructions until an invalid one (and handle the bytes count externally to set a stop condition), you don't need to update the @code_size parameter (set to the max bytes an instruction can have), @address parameter (set to 0) and the user won't use @address and will update his/her bytes count and @code by adding insn->size to them. And so on...

[aquynh]

@hlide, yes i know what you need, but talking about an API, it is good to have it useful for various situations.

on updating code/size/address, you can use CS_INSN_OFFSET macro in https://github.com/aquynh/capstone/blob/next/include/capstone.h#L241 to calculate the offset of the last instruction in array without having to iterate thru the array. this works with count = 1 or any value, so i dont see any problem here.

on your question on count = 0, i think we can make clear that these new APIs are for count != 0.
we can also make a check for count > 0 inside these API to make sure there is no violation.

on your case, i think there is no problem using proposal 1, where you can just pass count = 1, like that sample C-pseudo code. i put it down here for reference.

if (cs_disasm_malloc(h, 1, &insns) == CS_ERR_OK) {
    while(cs_disasm_buf(h, code, size, address, 1, &insns)) {
        // analyze instruction in @insns

        // then update input code/size/address for the next iteration
        length = CS_INSN_OFFSET(insns, 1);
        code += length;
        size -= length;
        address += length;
    }

    // free memory when done
    cs_free(insns, 1);
}

[aquynh]

"...the updating of those three parameters, I prefer to let the user to do so instead of the function..."

i find this very interesting. yes, this means proposal 2 is not flexible enough, because it does what you do not want.

so conclusion is that proposal 1 is better for you?

[hlide]

Proposal 1 also suffers the same lack of flexibility because it updates them internally too. Moreover, suppose that there is a data inside the instruction set you want disasm with @count > 1, you will decode it as a false instruction and the rest of instructions will be corrupted as well: you have the same issue as the current cs_disasm.

Honestly, just choose whatever you want. If I need something, I simply fork this project and add my own interface. I'm thinking to make a C++ template version of it for my own purpose.

[aquynh]

why do you say that proposal 1 update them internally? no, it does not do anything on those 3 parameters, and you have to update code/size/address yourself, as you can see in the sample code.

on my side, i would never turn on that SKIPDATA option, since that opens more issues than solving them.

your input here is important, since i hope these new API can improve the performance for you, and also for other cases.

[hlide]

They do:

cs_disasm_buf(..., uint8_t * code, size_t code_size, size_t address, size_t count, c_insn * insn)
{
    ...
    size_t res = 0;
    while (code_size > 0 && count > 0) { // issue: code_size is unsigned!
        ...
        insn->id = 0;
        bool r = handle->disasm(...);
        if (r) {
            fill_insn(..., insn);
            ...
            code_size -= insn.size; // issue: it may be negative but comparator above is unsigned!
            code += insn.size;
            address += insn.size;
            ++res;
            --count;
            ++insn;
        } else
            break;
    }
    return res;
} 

[aquynh]

yes, but that is not reflected at the output, right?

these are updated internally only, but at the output of the function, they
are not changed!

[hlide]

precisely they are wasted cpu cycles especially if you are disassembling a data which give you a false instruction and so for the rest of instructions decoded in a buffer of @count instruction.

[aquynh]

this i agreed. but those calculation is very cheap, right? i am quite sure
that if you do benchmark without those calculation, the performance gain is
very little, so it is not really worth it.

anyway, i am afraid it is impossible to have an API optimized for every
needs.
the goal is to keep thing simple, and make it work good for most popular
cases.

[hlide]

As already said, choose whatever you want. As for me, since the source is open, there is no reason why I cannot customize it to my needs. I don't want to be bound to a constraining API to satisfy a more generic demand I don't need. I'm sure we can agree that it is not something forbidden.

[hlide]

But if you want an answer: I prefer proposal 2. It's still flexible because if instruction is valid, I don't need to update those three parameters and if invalid, I can still make a decision and alter those three parameters if needed. Moreover, for the case I need to handle some valid instructions as data (because they were referred at least by one of the previous instructions as a data - trick with 'call dword ptr [rip + offset]' for instance), I can still detect them and handle them and alter those three parameters correctly BEFORE calling cs_disasm_iter since the tracking was already done by one of previous instructions referring this address as a data. So in the end, proposal 2 remains still viable and flexible.