On 10/4/22 12:27, Richard Henderson wrote:
On 10/4/22 09:23, Peter Maydell wrote:
  void arm_cpu_synchronize_from_tb(CPUState *cs,
                                   const TranslationBlock *tb)
  {
-    ARMCPU *cpu = ARM_CPU(cs);
-    CPUARMState *env = &cpu->env;
-
-    /*
-     * It's OK to look at env for the current mode here, because it's
-     * never possible for an AArch64 TB to chain to an AArch32 TB.
-     */
-    if (is_a64(env)) {
-        env->pc = tb_pc(tb);
-    } else {
-        env->regs[15] = tb_pc(tb);
+    /* The program counter is always up to date with TARGET_TB_PCREL. */

I was confused for a bit about this, but it works because
although the synchronize_from_tb hook has a name that implies
it's comparatively general purpose, in fact we use it only
in the special case of "we abandoned execution at the start of
this TB without executing any of it".

Correct.

@@ -347,16 +354,22 @@ static void gen_exception_internal(int excp)

  static void gen_exception_internal_insn(DisasContext *s, int excp)
  {
+    target_ulong pc_save = s->pc_save;
+
      gen_a64_update_pc(s, 0);
      gen_exception_internal(excp);
      s->base.is_jmp = DISAS_NORETURN;
+    s->pc_save = pc_save;

What is trashing s->pc_save that we have to work around like this,
here and in the other similar changes ?

gen_a64_update_pc trashes pc_save.

Off of the top of my head, I can't remember what conditionally uses exceptions (single step?).

Oh, duh, any conditional a32 instruction.

To some extent this instance duplicates s->pc_cond_save, but the usage pattern 
there is

    brcond(..., s->condlabel);
    s->pc_cond_save = s->pc_save;

    gen_update_pc(s, 0);  /* pc_save = pc_curr */
    raise_exception;

    if (s->pc_cond_save != s->pc_save) {
        gen_update_pc(s->pc_save - s->pc_cond_save);
    }
    /* s->pc_save now matches the state at brcond */

condlabel:


So, we have exited the TB via exception, and the second gen_update_pc would be deleted as dead code, it's just as easy to keep s->pc_save unchanged so that the second gen_update_pc is not emitted. We certainly *must* update s->pc_save around indirect branches, so that we don't wind up with an assert on s->pc_save != -1.


r~

Reply via email to