Re: [Qemu-devel] [PATCH v2] target-m68k: add rol/ror/roxl/roxr instructions

[Richard Henderson]

On 11/10/2016 11:51 PM, Laurent Vivier wrote:
> +/* Result of rotate_x() is valid if 0 < shift < (size + 1) < 32 */
> +static TCGv rotate_x(TCGv dest, TCGv src, TCGv shift, int left, int size)
> +{
> +    TCGv X, shl, shr, shx;
> +
> +    shr = tcg_temp_new();
> +    shl = tcg_temp_new();
> +    shx = tcg_temp_new();
> +    if (left) {
> +        tcg_gen_mov_i32(shl, shift);      /* shl = shift */
> +        tcg_gen_movi_i32(shr, size + 1);
> +        tcg_gen_sub_i32(shr, shr, shift); /* shr = size + 1 - shift */
> +        tcg_gen_subi_i32(shx, shift, 1);  /* shx = shift - 1 */

You're failing to bound shx properly.  If shift == 0, you produce -1 here which 
is illegal.  You could mask this to 31, but it's even better to produce size, 
as I said before, because then you don't have to have

> +        tcg_gen_movcond_i32(TCG_COND_EQ, X,
> +                            t1, zero,
> +                            QREG_CC_X, X);
> +        tcg_gen_movcond_i32(TCG_COND_EQ, DREG(insn, 0),
> +                            t1, zero,
> +                            DREG(insn, 0), res);

this at the end.  A zero shift will simply leave everything where it belongs, 
and you extract all the parts as normal.

> +DISAS_INSN(rotate_mem)
> +{
> +    TCGv src;
> +    TCGv addr;
> +    TCGv shift;
> +    int left = (insn & 0x100);
> +
> +    SRC_EA(env, src, OS_WORD, 0, &addr);
> +
> +    shift = tcg_const_i32(1);
> +    if (insn & 8) {

It's bit 9 for memory rotate.


r~