Re: [PATCH v2 23/23] target/i386: Enable TARGET_TB_PCREL

[Paolo Bonzini]

On Tue, Sep 6, 2022 at 12:10 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>  static void gen_update_eip_cur(DisasContext *s)
>  {
>      gen_jmp_im(s, s->base.pc_next - s->cs_base);
> +    s->pc_save = s->base.pc_next;

s->pc_save is not valid after all gen_jmp_im() calls. Is it worth
noting after each call to gen_jmp_im() why this is not a problem?

>  }
>
>  static void gen_update_eip_next(DisasContext *s)
>  {
>      gen_jmp_im(s, s->pc - s->cs_base);
> +    s->pc_save = s->pc;
> +}
> +
> +static TCGv gen_eip_cur(DisasContext *s)
> +{
> +    if (TARGET_TB_PCREL) {
> +        gen_update_eip_cur(s);
> +        return cpu_eip;
> +    } else {
> +        return tcg_constant_tl(s->base.pc_next - s->cs_base);
> +    }

Ok, now I see why you called it gen_eip_cur(), but it's still a bit
disconcerting to see the difference in behavior between the
TARGET_TB_PCREL and !TARGET_TB_PCREL cases, one of them updating
cpu_eip and other not.

Perhaps gen_jmp_im() and gen_update_eip_cur() could be rewritten to
return the destination instead:

static TCGv gen_jmp_im(DisasContext *s, target_ulong eip)
{
    if (TARGET_TB_PCREL) {
        target_ulong eip_save = s->pc_save - s->cs_base;
        tcg_gen_addi_tl(cpu_eip, cpu_eip, eip - eip_save);
        return cpu_eip;
    } else {
        TCGv dest = tcg_constant_tl(eip);
        tcg_gen_mov_tl(cpu_eip, dest);
        return dest;
    }
}

static TCGv gen_update_eip_cur(DisasContext *s)
{
    TCGv dest = gen_jmp_im(s, s->base.pc_next - s->cs_base);
    s->pc_save = s->base.pc_next;
    return dest;
}

and the "if (update_ip)" case would use the return value?

This change would basically replace the previous patch, with just the
"if (TARGET_TB_PCREL)" added here.

Paolo