https://bugs.kde.org/show_bug.cgi?id=423195
--- Comment #25 from Julian Seward <jsew...@acm.org> ---
(In reply to Carl Love from comment #14)
> Created attachment 130631 [details]
> Instruction prefix support
ok to land provided at least that the dis_nop_prefix return type is fixed
properly.
> + int ret;
> + ret = dis_nop_prefix( prefixInstr, theInstr);
> + if (ret == True)
> + goto decode_success;
> + else if (ret == PREFIX_NOP_INVALID)
> + goto decode_failure;
>
> Related problem here; pls fix (`ret` being sometimes True and sometimes
> PREFIX_NOP_INVALID given
> that they have different types)
>
> The function needs to return type Int. We need to know True,
> False or invalid. Changed the code to:
>
> static Int dis_nop_prefix ( UInt prefixInstr, UInt theInstr )
> {
> ...
> }
We shouldn't be mixing up integral types like this. Please use a custom enum
instead:
typedef enum { Invalid, No, Yes } PrefixStatus;
Some other comments to consider:
> +/*-----------------------------------------------------------*/
> +/*--- Prefix instruction helpers ---*/
> +/*-----------------------------------------------------------*/
> +/* ENABLE_PREFIX_CHECK is for development purposes. Turn off for production
> + releases to improve performance. */
> +#define ENABLE_PREFIX_CHECK 0
It won't make much perf difference in practice -- the insn decoders are not a
hot path. If it would generate better test coverage, and if it will *only*
fail if there's a bug in the decode logic (as opposed to an unknown insn),
feel free to turn it on in production.
> -static Bool dis_int_mult_add ( UInt theInstr )
> +static Bool dis_int_mult_add ( UInt prefixInstr, UInt theInstr )
Nit: isn't it a big misleading to call the first 32 bits `prefixInstr`?
Really, it's just *part* of the instruction. Would you consider calling it
instead just `prefix` ?
> + prefixInstr = 0; /* Reset the prefix so instruction flag */
Comment doesn't make sense; are there words missing?
> + A prefix instruction basically consists of a 4-byte pre-emble followed
nit: "preamble"
--
You are receiving this mail because:
You are watching all bug changes.
