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

[Richard Henderson]

On 9/21/22 06:31, Paolo Bonzini wrote:
On Tue, Sep 6, 2022 at 12:10 PM Richard Henderson
<richard.hender...@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;
}

I don't see what I'd do with the return values.  But I see your point about gen_eip_cur 
only updating eip sometimes.  I have changed the name to eip_cur_tl, as suggested, and it 
writes to a temporary, like eip_next_tl.


r~