Re: [PATCH v2 17/23] target/i386: Create gen_jmp_rel

[Paolo Bonzini]

On Tue, Sep 6, 2022 at 12:09 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Create a common helper for pc-relative branches.
> The jmp jb insn was missing a mask for CODE32.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

(Oops, my remark the previous patch should still have pointed to gen_jmp_tb).

In gen_jz_ecx_string, in the translation for LOOPNZ/LOOPZ/LOOP/JECXZ
and in i386_tr_tb_stop there is:

> -    gen_jmp_tb(s, s->pc - s->cs_base, 1);
> +    gen_jmp_rel(s, MO_32, 0, 1);

What happens if the instruction's last byte is at 0xffff? Wraparound
in the middle of an instruction is generally undefined, but I think it
should work if the instruction does not cross the 64K/4G limit (and on
real hardware, which obeys segment limits unlike TCG, said limit must
be 64K/4G of course).

In other words, why MO_32 and not "CODE32(s) ? MO_32 : MO_16"?
Likewise, if you change that you need to change gen_repz/gen_repz2
too.

Paolo


>      gen_set_label(l1);
>      return l2;
>  }
> @@ -2756,6 +2757,18 @@ static void gen_jmp_tb(DisasContext *s, target_ulong 
> eip, int tb_num)
>      }
>  }
>
> +static void gen_jmp_rel(DisasContext *s, MemOp ot, int diff, int tb_num)
> +{
> +    target_ulong dest = s->pc - s->cs_base + diff;
> +
> +    if (ot == MO_16) {
> +        dest &= 0xffff;
> +    } else if (!CODE64(s)) {
> +        dest &= 0xffffffff;
> +    }
> +    gen_jmp_tb(s, dest, tb_num);
> +}
> +
>  static void gen_jmp(DisasContext *s, target_ulong eip)
>  {
>      gen_jmp_tb(s, eip, 0);
> @@ -6816,20 +6829,12 @@ static bool disas_insn(DisasContext *s, CPUState *cpu)
>          break;
>      case 0xe8: /* call im */
>          {
> -            if (dflag != MO_16) {
> -                tval = (int32_t)insn_get(env, s, MO_32);
> -            } else {
> -                tval = (int16_t)insn_get(env, s, MO_16);
> -            }
> -            tval += s->pc - s->cs_base;
> -            if (dflag == MO_16) {
> -                tval &= 0xffff;
> -            } else if (!CODE64(s)) {
> -                tval &= 0xffffffff;
> -            }
> +            int diff = (dflag != MO_16
> +                        ? (int32_t)insn_get(env, s, MO_32)
> +                        : (int16_t)insn_get(env, s, MO_16));
>              gen_push_v(s, eip_next_tl(s));
>              gen_bnd_jmp(s);
> -            gen_jmp(s, tval);
> +            gen_jmp_rel(s, dflag, diff, 0);
>          }
>          break;
>      case 0x9a: /* lcall im */
> @@ -6847,19 +6852,13 @@ static bool disas_insn(DisasContext *s, CPUState *cpu)
>          }
>          goto do_lcall;
>      case 0xe9: /* jmp im */
> -        if (dflag != MO_16) {
> -            tval = (int32_t)insn_get(env, s, MO_32);
> -        } else {
> -            tval = (int16_t)insn_get(env, s, MO_16);
> +        {
> +            int diff = (dflag != MO_16
> +                        ? (int32_t)insn_get(env, s, MO_32)
> +                        : (int16_t)insn_get(env, s, MO_16));
> +            gen_bnd_jmp(s);
> +            gen_jmp_rel(s, dflag, diff, 0);
>          }
> -        tval += s->pc - s->cs_base;
> -        if (dflag == MO_16) {
> -            tval &= 0xffff;
> -        } else if (!CODE64(s)) {
> -            tval &= 0xffffffff;
> -        }
> -        gen_bnd_jmp(s);
> -        gen_jmp(s, tval);
>          break;
>      case 0xea: /* ljmp im */
>          {
> @@ -6876,12 +6875,10 @@ static bool disas_insn(DisasContext *s, CPUState *cpu)
>          }
>          goto do_ljmp;
>      case 0xeb: /* jmp Jb */
> -        tval = (int8_t)insn_get(env, s, MO_8);
> -        tval += s->pc - s->cs_base;
> -        if (dflag == MO_16) {
> -            tval &= 0xffff;
> +        {
> +            int diff = (int8_t)insn_get(env, s, MO_8);
> +            gen_jmp_rel(s, dflag, diff, 0);
>          }
> -        gen_jmp(s, tval);
>          break;
>      case 0x70 ... 0x7f: /* jcc Jb */
>          tval = (int8_t)insn_get(env, s, MO_8);
> --
> 2.34.1
>