Re: [RFC] Have insn decoder functions return success/failure

[Borislav Petkov]

On Thu, Oct 22, 2020 at 04:31:00PM +0900, Masami Hiramatsu wrote:
> No, insn_get_length() implies it decodes whole of the instruction.
> (yeah, we need an alias of that, something like insn_get_complete())

That's exactly what I'm trying to point out: the whole API is not
entirely wrong - it just needs a better naming and documentation. Now,
the implication that getting the length of the insn will give you a full
decode is a totally internal detail which users don't need and have to
know.

> I need insn.length too. Of course we can split it into 2 calls. But
> as I said, since the insn_get_length() implies it decodes all other
> parts, I just called it once.

Yes, I have noticed that and wrote about it further on. The intent was
to show that the API needs work.

> Hm, it is better to call insn_get_immediate() if it doesn't use length later.

Ok, so you see the problem. This thing wants to decode the whole insn -
that's what the function is called. But it reads like it does something
else.

> Would you mean we'd better have something like insn_get_until_immediate() ? 
> 
> Since the x86 instruction is CISC, we can not decode intermediate
> parts. The APIs follows that. If you are confused, I'm sorry about that.

No, I'm not confused - again, I'd like for the API to be properly
defined and callers should not have to care which parts of the insn they
need to decode in order to get something else they actually need.

So the main API should be: insn_decode_insn() or so and it should give
you everything you need.

If this succeeds, you can go poke at insn.<field> and you know you have
valid data there.

If there are specialized uses, you can call some of the insn_get_*
helpers if you're not interested in decoding the full insn.

But if simply calling insn_decode_insn() would give you everything and
that is not that expensive, we can do that - API simplicity.

What I don't want to have is calling insn_get_length() or so and then
inspecting the opcode bytes because that's totally non-transparent.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette