[PULL,04/26] target-*: Introduce and use cpu_breakpoint_test

Reduce the boilerplate required for each target.  At the same time,
move the test for breakpoint after calling tcg_gen_insn_start.

Note that arm and aarch64 do not use cpu_breakpoint_test, but still
move the inline test down after tcg_gen_insn_start.

Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 include/qom/cpu.h             | 16 ++++++++++++++++
 target-alpha/translate.c      | 13 ++++---------
 target-arm/translate-a64.c    | 26 +++++++++++++-------------
 target-arm/translate.c        | 31 ++++++++++++++++---------------
 target-cris/translate.c       | 27 ++++++++-------------------
 target-i386/translate.c       | 17 +++++++----------
 target-lm32/translate.c       | 25 +++++++------------------
 target-m68k/translate.c       | 18 ++++++------------
 target-microblaze/translate.c | 36 +++++++++++++-----------------------
 target-mips/translate.c       | 25 ++++++++++---------------
 target-moxie/translate.c      | 19 +++++++------------
 target-openrisc/translate.c   | 24 +++++++-----------------
 target-ppc/translate.c        | 14 +++++---------
 target-s390x/translate.c      | 16 ++++++----------
 target-sh4/translate.c        | 20 ++++++++------------
 target-sparc/translate.c      | 23 ++++++++++-------------
 target-unicore32/translate.c  | 24 ++++++++++--------------
 target-xtensa/translate.c     | 25 +++++++------------------
 18 files changed, 160 insertions(+), 239 deletions(-)

On 07.10.2015 12:43, Richard Henderson wrote:
> Reduce the boilerplate required for each target.  At the same time,
> move the test for breakpoint after calling tcg_gen_insn_start.
>
> Note that arm and aarch64 do not use cpu_breakpoint_test, but still
> move the inline test down after tcg_gen_insn_start.
>
> Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  include/qom/cpu.h             | 16 ++++++++++++++++
>  target-alpha/translate.c      | 13 ++++---------
>  target-arm/translate-a64.c    | 26 +++++++++++++-------------
>  target-arm/translate.c        | 31 ++++++++++++++++---------------
>  target-cris/translate.c       | 27 ++++++++-------------------
>  target-i386/translate.c       | 17 +++++++----------
>  target-lm32/translate.c       | 25 +++++++------------------
>  target-m68k/translate.c       | 18 ++++++------------
>  target-microblaze/translate.c | 36 +++++++++++++-----------------------
>  target-mips/translate.c       | 25 ++++++++++---------------
>  target-moxie/translate.c      | 19 +++++++------------
>  target-openrisc/translate.c   | 24 +++++++-----------------
>  target-ppc/translate.c        | 14 +++++---------
>  target-s390x/translate.c      | 16 ++++++----------
>  target-sh4/translate.c        | 20 ++++++++------------
>  target-sparc/translate.c      | 23 ++++++++++-------------
>  target-unicore32/translate.c  | 24 ++++++++++--------------
>  target-xtensa/translate.c     | 25 +++++++------------------
>  18 files changed, 160 insertions(+), 239 deletions(-)
>
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 9405554..b613ff0 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -721,6 +721,7 @@ void cpu_single_step(CPUState *cpu, int enabled);
>  /* 0x08 currently unused */
>  #define BP_GDB                0x10
>  #define BP_CPU                0x20
> +#define BP_ANY                (BP_GDB | BP_CPU)
>  #define BP_WATCHPOINT_HIT_READ 0x40
>  #define BP_WATCHPOINT_HIT_WRITE 0x80
>  #define BP_WATCHPOINT_HIT (BP_WATCHPOINT_HIT_READ | BP_WATCHPOINT_HIT_WRITE)
> @@ -731,6 +732,21 @@ int cpu_breakpoint_remove(CPUState *cpu, vaddr pc, int flags);
>  void cpu_breakpoint_remove_by_ref(CPUState *cpu, CPUBreakpoint *breakpoint);
>  void cpu_breakpoint_remove_all(CPUState *cpu, int mask);
>  
> +/* Return true if PC matches an installed breakpoint.  */
> +static inline bool cpu_breakpoint_test(CPUState *cpu, vaddr pc, int mask)
> +{
> +    CPUBreakpoint *bp;
> +
> +    if (unlikely(!QTAILQ_EMPTY(&cpu->breakpoints))) {
> +        QTAILQ_FOREACH(bp, &cpu->breakpoints, entry) {
> +            if (bp->pc == pc && (bp->flags & mask)) {
> +                return true;
> +            }
> +        }
> +    }
> +    return false;
> +}
> +
>  int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len,
>                            int flags, CPUWatchpoint **watchpoint);
>  int cpu_watchpoint_remove(CPUState *cpu, vaddr addr,
> diff --git a/target-alpha/translate.c b/target-alpha/translate.c
> index fa0ac2d..c10193e 100644
> --- a/target-alpha/translate.c
> +++ b/target-alpha/translate.c
> @@ -2868,7 +2868,6 @@ static inline void gen_intermediate_code_internal(AlphaCPU *cpu,
>      target_ulong pc_start;
>      target_ulong pc_mask;
>      uint32_t insn;
> -    CPUBreakpoint *bp;
>      int j, lj = -1;
>      ExitStatus ret;
>      int num_insns;
> @@ -2913,14 +2912,6 @@ static inline void gen_intermediate_code_internal(AlphaCPU *cpu,
>  
>      gen_tb_start(tb);
>      do {
> -        if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
> -            QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
> -                if (bp->pc == ctx.pc) {
> -                    gen_excp(&ctx, EXCP_DEBUG, 0);
> -                    break;
> -                }
> -            }
> -        }
>          if (search_pc) {
>              j = tcg_op_buf_count();
>              if (lj < j) {
> @@ -2936,6 +2927,10 @@ static inline void gen_intermediate_code_internal(AlphaCPU *cpu,
>          tcg_gen_insn_start(ctx.pc);
>          num_insns++;
>  
> +        if (unlikely(cpu_breakpoint_test(cs, ctx.pc, BP_ANY))) {
> +            gen_excp(&ctx, EXCP_DEBUG, 0);
> +            break;
> +        }

Actually, control logic has changed here. The old code used a break
statement to exit from QTAILQ_FOREACH loop and continue with instruction
translation thus translating at least one instruction. The break
statement in the new code makes exit from the translation loop itself,
effectively producing zero-length TB which won't get invalidated when
clearing the breakpoint. Seems like we should remove the break statement
here and in similar cases below, right?

Best regards,
Sergey

>          if (num_insns == max_insns && (tb->cflags & CF_LAST_IO)) {
>              gen_io_start();
>          }
> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> index 4670941..bc2040e 100644
> --- a/target-arm/translate-a64.c
> +++ b/target-arm/translate-a64.c
> @@ -11007,7 +11007,6 @@ void gen_intermediate_code_internal_a64(ARMCPU *cpu,
>      CPUState *cs = CPU(cpu);
>      CPUARMState *env = &cpu->env;
>      DisasContext dc1, *dc = &dc1;
> -    CPUBreakpoint *bp;
>      int j, lj;
>      target_ulong pc_start;
>      target_ulong next_page_start;
> @@ -11079,18 +11078,6 @@ void gen_intermediate_code_internal_a64(ARMCPU *cpu,
>      tcg_clear_temp_count();
>  
>      do {
> -        if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
> -            QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
> -                if (bp->pc == dc->pc) {
> -                    gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
> -                    /* Advance PC so that clearing the breakpoint will
> -                       invalidate this TB.  */
> -                    dc->pc += 2;
> -                    goto done_generating;
> -                }
> -            }
> -        }
> -
>          if (search_pc) {
>              j = tcg_op_buf_count();
>              if (lj < j) {
> @@ -11106,6 +11093,19 @@ void gen_intermediate_code_internal_a64(ARMCPU *cpu,
>          tcg_gen_insn_start(dc->pc);
>          num_insns++;
>  
> +        if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
> +            CPUBreakpoint *bp;
> +            QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
> +                if (bp->pc == dc->pc) {
> +                    gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
> +                    /* Advance PC so that clearing the breakpoint will
> +                       invalidate this TB.  */
> +                    dc->pc += 2;
> +                    goto done_generating;
> +                }
> +            }
> +        }
> +
>          if (num_insns == max_insns && (tb->cflags & CF_LAST_IO)) {
>              gen_io_start();
>          }
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index cd88997..44468dc 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -11177,7 +11177,6 @@ static inline void gen_intermediate_code_internal(ARMCPU *cpu,
>      CPUState *cs = CPU(cpu);
>      CPUARMState *env = &cpu->env;
>      DisasContext dc1, *dc = &dc1;
> -    CPUBreakpoint *bp;
>      int j, lj;
>      target_ulong pc_start;
>      target_ulong next_page_start;
> @@ -11306,6 +11305,21 @@ static inline void gen_intermediate_code_internal(ARMCPU *cpu,
>          store_cpu_field(tmp, condexec_bits);
>        }
>      do {
> +        if (search_pc) {
> +            j = tcg_op_buf_count();
> +            if (lj < j) {
> +                lj++;
> +                while (lj < j)
> +                    tcg_ctx.gen_opc_instr_start[lj++] = 0;
> +            }
> +            tcg_ctx.gen_opc_pc[lj] = dc->pc;
> +            gen_opc_condexec_bits[lj] = (dc->condexec_cond << 4) | (dc->condexec_mask >> 1);
> +            tcg_ctx.gen_opc_instr_start[lj] = 1;
> +            tcg_ctx.gen_opc_icount[lj] = num_insns;
> +        }
> +        tcg_gen_insn_start(dc->pc);
> +        num_insns++;
> +
>  #ifdef CONFIG_USER_ONLY
>          /* Intercept jump to the magic kernel page.  */
>          if (dc->pc >= 0xffff0000) {
> @@ -11326,6 +11340,7 @@ static inline void gen_intermediate_code_internal(ARMCPU *cpu,
>  #endif
>  
>          if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
> +            CPUBreakpoint *bp;
>              QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
>                  if (bp->pc == dc->pc) {
>                      gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
> @@ -11336,20 +11351,6 @@ static inline void gen_intermediate_code_internal(ARMCPU *cpu,
>                  }
>              }
>          }
> -        if (search_pc) {
> -            j = tcg_op_buf_count();
> -            if (lj < j) {
> -                lj++;
> -                while (lj < j)
> -                    tcg_ctx.gen_opc_instr_start[lj++] = 0;
> -            }
> -            tcg_ctx.gen_opc_pc[lj] = dc->pc;
> -            gen_opc_condexec_bits[lj] = (dc->condexec_cond << 4) | (dc->condexec_mask >> 1);
> -            tcg_ctx.gen_opc_instr_start[lj] = 1;
> -            tcg_ctx.gen_opc_icount[lj] = num_insns;
> -        }
> -        tcg_gen_insn_start(dc->pc);
> -        num_insns++;
>  
>          if (num_insns == max_insns && (tb->cflags & CF_LAST_IO)) {
>              gen_io_start();
> diff --git a/target-cris/translate.c b/target-cris/translate.c
> index bba7217..477bddc 100644
> --- a/target-cris/translate.c
> +++ b/target-cris/translate.c
> @@ -3030,23 +3030,6 @@ static unsigned int crisv32_decoder(CPUCRISState *env, DisasContext *dc)
>      return insn_len;
>  }
>  
> -static void check_breakpoint(CPUCRISState *env, DisasContext *dc)
> -{
> -    CPUState *cs = CPU(cris_env_get_cpu(env));
> -    CPUBreakpoint *bp;
> -
> -    if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
> -        QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
> -            if (bp->pc == dc->pc) {
> -                cris_evaluate_flags(dc);
> -                tcg_gen_movi_tl(env_pc, dc->pc);
> -                t_gen_raise_exception(EXCP_DEBUG);
> -                dc->is_jmp = DISAS_UPDATE;
> -            }
> -        }
> -    }
> -}
> -
>  #include "translate_v10.c"
>  
>  /*
> @@ -3175,8 +3158,6 @@ gen_intermediate_code_internal(CRISCPU *cpu, TranslationBlock *tb,
>  
>      gen_tb_start(tb);
>      do {
> -        check_breakpoint(env, dc);
> -
>          if (search_pc) {
>              j = tcg_op_buf_count();
>              if (lj < j) {
> @@ -3196,6 +3177,14 @@ gen_intermediate_code_internal(CRISCPU *cpu, TranslationBlock *tb,
>          tcg_gen_insn_start(dc->pc);
>          num_insns++;
>  
> +        if (unlikely(cpu_breakpoint_test(cs, dc->pc, BP_ANY))) {
> +            cris_evaluate_flags(dc);
> +            tcg_gen_movi_tl(env_pc, dc->pc);
> +            t_gen_raise_exception(EXCP_DEBUG);
> +            dc->is_jmp = DISAS_UPDATE;
> +            break;
> +        }
> +
>          /* Pretty disas.  */
>          LOG_DIS("%8.8x:\t", dc->pc);
>  
> diff --git a/target-i386/translate.c b/target-i386/translate.c
> index 3d0c23d..9ec9c4c 100644
> --- a/target-i386/translate.c
> +++ b/target-i386/translate.c
> @@ -7849,7 +7849,6 @@ static inline void gen_intermediate_code_internal(X86CPU *cpu,
>      CPUX86State *env = &cpu->env;
>      DisasContext dc1, *dc = &dc1;
>      target_ulong pc_ptr;
> -    CPUBreakpoint *bp;
>      int j, lj;
>      uint64_t flags;
>      target_ulong pc_start;
> @@ -7938,15 +7937,6 @@ static inline void gen_intermediate_code_internal(X86CPU *cpu,
>  
>      gen_tb_start(tb);
>      for(;;) {
> -        if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
> -            QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
> -                if (bp->pc == pc_ptr &&
> -                    !((bp->flags & BP_CPU) && (tb->flags & HF_RF_MASK))) {
> -                    gen_debug(dc, pc_ptr - dc->cs_base);
> -                    goto done_generating;
> -                }
> -            }
> -        }
>          if (search_pc) {
>              j = tcg_op_buf_count();
>              if (lj < j) {
> @@ -7962,6 +7952,13 @@ static inline void gen_intermediate_code_internal(X86CPU *cpu,
>          tcg_gen_insn_start(pc_ptr);
>          num_insns++;
>  
> +        /* If RF is set, suppress an internally generated breakpoint.  */
> +        if (unlikely(cpu_breakpoint_test(cs, pc_ptr,
> +                                         tb->flags & HF_RF_MASK
> +                                         ? BP_GDB : BP_ANY))) {
> +            gen_debug(dc, pc_ptr - dc->cs_base);
> +            goto done_generating;
> +        }
>          if (num_insns == max_insns && (tb->cflags & CF_LAST_IO)) {
>              gen_io_start();
>          }
> diff --git a/target-lm32/translate.c b/target-lm32/translate.c
> index a34914a..8ea7929 100644
> --- a/target-lm32/translate.c
> +++ b/target-lm32/translate.c
> @@ -1032,22 +1032,6 @@ static inline void decode(DisasContext *dc, uint32_t ir)
>      decinfo[dc->opcode](dc);
>  }
>  
> -static void check_breakpoint(CPULM32State *env, DisasContext *dc)
> -{
> -    CPUState *cs = CPU(lm32_env_get_cpu(env));
> -    CPUBreakpoint *bp;
> -
> -    if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
> -        QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
> -            if (bp->pc == dc->pc) {
> -                tcg_gen_movi_tl(cpu_pc, dc->pc);
> -                t_gen_raise_exception(dc, EXCP_DEBUG);
> -                dc->is_jmp = DISAS_UPDATE;
> -             }
> -        }
> -    }
> -}
> -
>  /* generate intermediate code for basic block 'tb'.  */
>  static inline
>  void gen_intermediate_code_internal(LM32CPU *cpu,
> @@ -1088,8 +1072,6 @@ void gen_intermediate_code_internal(LM32CPU *cpu,
>  
>      gen_tb_start(tb);
>      do {
> -        check_breakpoint(env, dc);
> -
>          if (search_pc) {
>              j = tcg_op_buf_count();
>              if (lj < j) {
> @@ -1105,6 +1087,13 @@ void gen_intermediate_code_internal(LM32CPU *cpu,
>          tcg_gen_insn_start(dc->pc);
>          num_insns++;
>  
> +        if (unlikely(cpu_breakpoint_test(cs, dc->pc, BP_ANY))) {
> +            tcg_gen_movi_tl(cpu_pc, dc->pc);
> +            t_gen_raise_exception(dc, EXCP_DEBUG);
> +            dc->is_jmp = DISAS_UPDATE;
> +            break;
> +        }
> +
>          /* Pretty disas.  */
>          LOG_DIS("%8.8x:\t", dc->pc);
>  
> diff --git a/target-m68k/translate.c b/target-m68k/translate.c
> index 422244e..afef37f 100644
> --- a/target-m68k/translate.c
> +++ b/target-m68k/translate.c
> @@ -2969,7 +2969,6 @@ gen_intermediate_code_internal(M68kCPU *cpu, TranslationBlock *tb,
>      CPUState *cs = CPU(cpu);
>      CPUM68KState *env = &cpu->env;
>      DisasContext dc1, *dc = &dc1;
> -    CPUBreakpoint *bp;
>      int j, lj;
>      target_ulong pc_start;
>      int pc_offset;
> @@ -2999,17 +2998,6 @@ gen_intermediate_code_internal(M68kCPU *cpu, TranslationBlock *tb,
>      do {
>          pc_offset = dc->pc - pc_start;
>          gen_throws_exception = NULL;
> -        if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
> -            QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
> -                if (bp->pc == dc->pc) {
> -                    gen_exception(dc, dc->pc, EXCP_DEBUG);
> -                    dc->is_jmp = DISAS_JUMP;
> -                    break;
> -                }
> -            }
> -            if (dc->is_jmp)
> -                break;
> -        }
>          if (search_pc) {
>              j = tcg_op_buf_count();
>              if (lj < j) {
> @@ -3024,6 +3012,12 @@ gen_intermediate_code_internal(M68kCPU *cpu, TranslationBlock *tb,
>          tcg_gen_insn_start(dc->pc);
>          num_insns++;
>  
> +        if (unlikely(cpu_breakpoint_test(cs, dc->pc, BP_ANY))) {
> +            gen_exception(dc, dc->pc, EXCP_DEBUG);
> +            dc->is_jmp = DISAS_JUMP;
> +            break;
> +        }
> +
>          if (num_insns == max_insns && (tb->cflags & CF_LAST_IO)) {
>              gen_io_start();
>          }
> diff --git a/target-microblaze/translate.c b/target-microblaze/translate.c
> index a25b042..1224456 100644
> --- a/target-microblaze/translate.c
> +++ b/target-microblaze/translate.c
> @@ -1626,21 +1626,6 @@ static inline void decode(DisasContext *dc, uint32_t ir)
>      }
>  }
>  
> -static void check_breakpoint(CPUMBState *env, DisasContext *dc)
> -{
> -    CPUState *cs = CPU(mb_env_get_cpu(env));
> -    CPUBreakpoint *bp;
> -
> -    if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
> -        QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
> -            if (bp->pc == dc->pc) {
> -                t_gen_raise_exception(dc, EXCP_DEBUG);
> -                dc->is_jmp = DISAS_UPDATE;
> -             }
> -        }
> -    }
> -}
> -
>  /* generate intermediate code for basic block 'tb'.  */
>  static inline void
>  gen_intermediate_code_internal(MicroBlazeCPU *cpu, TranslationBlock *tb,
> @@ -1695,14 +1680,6 @@ gen_intermediate_code_internal(MicroBlazeCPU *cpu, TranslationBlock *tb,
>      gen_tb_start(tb);
>      do
>      {
> -#if SIM_COMPAT
> -        if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)) {
> -            tcg_gen_movi_tl(cpu_SR[SR_PC], dc->pc);
> -            gen_helper_debug();
> -        }
> -#endif
> -        check_breakpoint(env, dc);
> -
>          if (search_pc) {
>              j = tcg_op_buf_count();
>              if (lj < j) {
> @@ -1717,6 +1694,19 @@ gen_intermediate_code_internal(MicroBlazeCPU *cpu, TranslationBlock *tb,
>          tcg_gen_insn_start(dc->pc);
>          num_insns++;
>  
> +#if SIM_COMPAT
> +        if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)) {
> +            tcg_gen_movi_tl(cpu_SR[SR_PC], dc->pc);
> +            gen_helper_debug();
> +        }
> +#endif
> +
> +        if (unlikely(cpu_breakpoint_test(cs, dc->pc, BP_ANY))) {
> +            t_gen_raise_exception(dc, EXCP_DEBUG);
> +            dc->is_jmp = DISAS_UPDATE;
> +            break;
> +        }
> +
>          /* Pretty disas.  */
>          LOG_DIS("%8.8x:\t", dc->pc);
>  
> diff --git a/target-mips/translate.c b/target-mips/translate.c
> index 66147d8..57e826d 100644
> --- a/target-mips/translate.c
> +++ b/target-mips/translate.c
> @@ -19544,7 +19544,6 @@ gen_intermediate_code_internal(MIPSCPU *cpu, TranslationBlock *tb,
>      DisasContext ctx;
>      target_ulong pc_start;
>      target_ulong next_page_start;
> -    CPUBreakpoint *bp;
>      int j, lj = -1;
>      int num_insns;
>      int max_insns;
> @@ -19591,20 +19590,6 @@ gen_intermediate_code_internal(MIPSCPU *cpu, TranslationBlock *tb,
>      LOG_DISAS("\ntb %p idx %d hflags %04x\n", tb, ctx.mem_idx, ctx.hflags);
>      gen_tb_start(tb);
>      while (ctx.bstate == BS_NONE) {
> -        if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
> -            QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
> -                if (bp->pc == ctx.pc) {
> -                    save_cpu_state(&ctx, 1);
> -                    ctx.bstate = BS_BRANCH;
> -                    gen_helper_raise_exception_debug(cpu_env);
> -                    /* Include the breakpoint location or the tb won't
> -                     * be flushed when it must be.  */
> -                    ctx.pc += 4;
> -                    goto done_generating;
> -                }
> -            }
> -        }
> -
>          if (search_pc) {
>              j = tcg_op_buf_count();
>              if (lj < j) {
> @@ -19621,6 +19606,16 @@ gen_intermediate_code_internal(MIPSCPU *cpu, TranslationBlock *tb,
>          tcg_gen_insn_start(ctx.pc);
>          num_insns++;
>  
> +        if (unlikely(cpu_breakpoint_test(cs, ctx.pc, BP_ANY))) {
> +            save_cpu_state(&ctx, 1);
> +            ctx.bstate = BS_BRANCH;
> +            gen_helper_raise_exception_debug(cpu_env);
> +            /* Include the breakpoint location or the tb won't
> +             * be flushed when it must be.  */
> +            ctx.pc += 4;
> +            goto done_generating;
> +        }
> +
>          if (num_insns == max_insns && (tb->cflags & CF_LAST_IO)) {
>              gen_io_start();
>          }
> diff --git a/target-moxie/translate.c b/target-moxie/translate.c
> index f71ed24..d71f55b 100644
> --- a/target-moxie/translate.c
> +++ b/target-moxie/translate.c
> @@ -822,7 +822,6 @@ gen_intermediate_code_internal(MoxieCPU *cpu, TranslationBlock *tb,
>      CPUState *cs = CPU(cpu);
>      DisasContext ctx;
>      target_ulong pc_start;
> -    CPUBreakpoint *bp;
>      int j, lj = -1;
>      CPUMoxieState *env = &cpu->env;
>      int num_insns;
> @@ -838,17 +837,6 @@ gen_intermediate_code_internal(MoxieCPU *cpu, TranslationBlock *tb,
>  
>      gen_tb_start(tb);
>      do {
> -        if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
> -            QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
> -                if (ctx.pc == bp->pc) {
> -                    tcg_gen_movi_i32(cpu_pc, ctx.pc);
> -                    gen_helper_debug(cpu_env);
> -                    ctx.bstate = BS_EXCP;
> -                    goto done_generating;
> -                }
> -            }
> -        }
> -
>          if (search_pc) {
>              j = tcg_op_buf_count();
>              if (lj < j) {
> @@ -864,6 +852,13 @@ gen_intermediate_code_internal(MoxieCPU *cpu, TranslationBlock *tb,
>          tcg_gen_insn_start(ctx.pc);
>          num_insns++;
>  
> +        if (unlikely(cpu_breakpoint_test(cs, ctx.pc, BP_ANY))) {
> +            tcg_gen_movi_i32(cpu_pc, ctx.pc);
> +            gen_helper_debug(cpu_env);
> +            ctx.bstate = BS_EXCP;
> +            goto done_generating;
> +        }
> +
>          ctx.opcode = cpu_lduw_code(env, ctx.pc);
>          ctx.pc += decode_opc(cpu, &ctx);
>  
> diff --git a/target-openrisc/translate.c b/target-openrisc/translate.c
> index f9b4ed5..9755850 100644
> --- a/target-openrisc/translate.c
> +++ b/target-openrisc/translate.c
> @@ -1618,22 +1618,6 @@ static void disas_openrisc_insn(DisasContext *dc, OpenRISCCPU *cpu)
>      }
>  }
>  
> -static void check_breakpoint(OpenRISCCPU *cpu, DisasContext *dc)
> -{
> -    CPUState *cs = CPU(cpu);
> -    CPUBreakpoint *bp;
> -
> -    if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
> -        QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
> -            if (bp->pc == dc->pc) {
> -                tcg_gen_movi_tl(cpu_pc, dc->pc);
> -                gen_exception(dc, EXCP_DEBUG);
> -                dc->is_jmp = DISAS_UPDATE;
> -            }
> -        }
> -    }
> -}
> -
>  static inline void gen_intermediate_code_internal(OpenRISCCPU *cpu,
>                                                    TranslationBlock *tb,
>                                                    int search_pc)
> @@ -1674,7 +1658,6 @@ static inline void gen_intermediate_code_internal(OpenRISCCPU *cpu,
>      gen_tb_start(tb);
>  
>      do {
> -        check_breakpoint(cpu, dc);
>          if (search_pc) {
>              j = tcg_op_buf_count();
>              if (k < j) {
> @@ -1690,6 +1673,13 @@ static inline void gen_intermediate_code_internal(OpenRISCCPU *cpu,
>          tcg_gen_insn_start(dc->pc);
>          num_insns++;
>  
> +        if (unlikely(cpu_breakpoint_test(cs, dc->pc, BP_ANY))) {
> +            tcg_gen_movi_tl(cpu_pc, dc->pc);
> +            gen_exception(dc, EXCP_DEBUG);
> +            dc->is_jmp = DISAS_UPDATE;
> +            break;
> +        }
> +
>          if (num_insns == max_insns && (tb->cflags & CF_LAST_IO)) {
>              gen_io_start();
>          }
> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> index 7c288aa..fc234a3 100644
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -11418,7 +11418,6 @@ static inline void gen_intermediate_code_internal(PowerPCCPU *cpu,
>      DisasContext ctx, *ctxp = &ctx;
>      opc_handler_t **table, *handler;
>      target_ulong pc_start;
> -    CPUBreakpoint *bp;
>      int j, lj = -1;
>      int num_insns;
>      int max_insns;
> @@ -11483,14 +11482,6 @@ static inline void gen_intermediate_code_internal(PowerPCCPU *cpu,
>      tcg_clear_temp_count();
>      /* Set env in case of segfault during code fetch */
>      while (ctx.exception == POWERPC_EXCP_NONE && !tcg_op_buf_full()) {
> -        if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
> -            QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
> -                if (bp->pc == ctx.nip) {
> -                    gen_debug_exception(ctxp);
> -                    break;
> -                }
> -            }
> -        }
>          if (unlikely(search_pc)) {
>              j = tcg_op_buf_count();
>              if (lj < j) {
> @@ -11505,6 +11496,11 @@ static inline void gen_intermediate_code_internal(PowerPCCPU *cpu,
>          tcg_gen_insn_start(ctx.nip);
>          num_insns++;
>  
> +        if (unlikely(cpu_breakpoint_test(cs, ctx.nip, BP_ANY))) {
> +            gen_debug_exception(ctxp);
> +            break;
> +        }
> +
>          LOG_DISAS("----------------\n");
>          LOG_DISAS("nip=" TARGET_FMT_lx " super=%d ir=%d\n",
>                    ctx.nip, ctx.mem_idx, (int)msr_ir);
> diff --git a/target-s390x/translate.c b/target-s390x/translate.c
> index 58cf365..4959828 100644
> --- a/target-s390x/translate.c
> +++ b/target-s390x/translate.c
> @@ -5330,7 +5330,6 @@ static inline void gen_intermediate_code_internal(S390CPU *cpu,
>      uint64_t next_page_start;
>      int j, lj = -1;
>      int num_insns, max_insns;
> -    CPUBreakpoint *bp;
>      ExitStatus status;
>      bool do_debug;
>  
> @@ -5373,20 +5372,17 @@ static inline void gen_intermediate_code_internal(S390CPU *cpu,
>          tcg_gen_insn_start(dc.pc);
>          num_insns++;
>  
> +        if (unlikely(cpu_breakpoint_test(cs, dc.pc, BP_ANY))) {
> +            status = EXIT_PC_STALE;
> +            do_debug = true;
> +            break;
> +        }
> +
>          if (num_insns == max_insns && (tb->cflags & CF_LAST_IO)) {
>              gen_io_start();
>          }
>  
>          status = NO_EXIT;
> -        if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
> -            QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
> -                if (bp->pc == dc.pc) {
> -                    status = EXIT_PC_STALE;
> -                    do_debug = true;
> -                    break;
> -                }
> -            }
> -        }
>          if (status == NO_EXIT) {
>              status = translate_one(env, &dc);
>          }
> diff --git a/target-sh4/translate.c b/target-sh4/translate.c
> index e0294e7..53bf9e8 100644
> --- a/target-sh4/translate.c
> +++ b/target-sh4/translate.c
> @@ -1824,7 +1824,6 @@ gen_intermediate_code_internal(SuperHCPU *cpu, TranslationBlock *tb,
>      CPUSH4State *env = &cpu->env;
>      DisasContext ctx;
>      target_ulong pc_start;
> -    CPUBreakpoint *bp;
>      int i, ii;
>      int num_insns;
>      int max_insns;
> @@ -1849,17 +1848,6 @@ gen_intermediate_code_internal(SuperHCPU *cpu, TranslationBlock *tb,
>          max_insns = CF_COUNT_MASK;
>      gen_tb_start(tb);
>      while (ctx.bstate == BS_NONE && !tcg_op_buf_full()) {
> -        if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
> -            QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
> -                if (ctx.pc == bp->pc) {
> -		    /* We have hit a breakpoint - make sure PC is up-to-date */
> -		    tcg_gen_movi_i32(cpu_pc, ctx.pc);
> -                    gen_helper_debug(cpu_env);
> -                    ctx.bstate = BS_BRANCH;
> -		    break;
> -		}
> -	    }
> -	}
>          if (search_pc) {
>              i = tcg_op_buf_count();
>              if (ii < i) {
> @@ -1875,6 +1863,14 @@ gen_intermediate_code_internal(SuperHCPU *cpu, TranslationBlock *tb,
>          tcg_gen_insn_start(ctx.pc);
>          num_insns++;
>  
> +        if (unlikely(cpu_breakpoint_test(cs, ctx.pc, BP_ANY))) {
> +            /* We have hit a breakpoint - make sure PC is up-to-date */
> +            tcg_gen_movi_i32(cpu_pc, ctx.pc);
> +            gen_helper_debug(cpu_env);
> +            ctx.bstate = BS_BRANCH;
> +            break;
> +        }
> +
>          if (num_insns == max_insns && (tb->cflags & CF_LAST_IO)) {
>              gen_io_start();
>          }
> diff --git a/target-sparc/translate.c b/target-sparc/translate.c
> index 762eb9b..f359ac9 100644
> --- a/target-sparc/translate.c
> +++ b/target-sparc/translate.c
> @@ -5217,7 +5217,6 @@ static inline void gen_intermediate_code_internal(SPARCCPU *cpu,
>      CPUSPARCState *env = &cpu->env;
>      target_ulong pc_start, last_pc;
>      DisasContext dc1, *dc = &dc1;
> -    CPUBreakpoint *bp;
>      int j, lj = -1;
>      int num_insns;
>      int max_insns;
> @@ -5242,18 +5241,6 @@ static inline void gen_intermediate_code_internal(SPARCCPU *cpu,
>          max_insns = CF_COUNT_MASK;
>      gen_tb_start(tb);
>      do {
> -        if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
> -            QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
> -                if (bp->pc == dc->pc) {
> -                    if (dc->pc != pc_start)
> -                        save_state(dc);
> -                    gen_helper_debug(cpu_env);
> -                    tcg_gen_exit_tb(0);
> -                    dc->is_br = 1;
> -                    goto exit_gen_loop;
> -                }
> -            }
> -        }
>          if (spc) {
>              qemu_log("Search PC...\n");
>              j = tcg_op_buf_count();
> @@ -5270,6 +5257,16 @@ static inline void gen_intermediate_code_internal(SPARCCPU *cpu,
>          tcg_gen_insn_start(dc->pc);
>          num_insns++;
>  
> +        if (unlikely(cpu_breakpoint_test(cs, dc->pc, BP_ANY))) {
> +            if (dc->pc != pc_start) {
> +                save_state(dc);
> +            }
> +            gen_helper_debug(cpu_env);
> +            tcg_gen_exit_tb(0);
> +            dc->is_br = 1;
> +            goto exit_gen_loop;
> +        }
> +
>          if (num_insns == max_insns && (tb->cflags & CF_LAST_IO)) {
>              gen_io_start();
>          }
> diff --git a/target-unicore32/translate.c b/target-unicore32/translate.c
> index 7aad61f..cd23c4b 100644
> --- a/target-unicore32/translate.c
> +++ b/target-unicore32/translate.c
> @@ -1872,7 +1872,6 @@ static inline void gen_intermediate_code_internal(UniCore32CPU *cpu,
>      CPUState *cs = CPU(cpu);
>      CPUUniCore32State *env = &cpu->env;
>      DisasContext dc1, *dc = &dc1;
> -    CPUBreakpoint *bp;
>      int j, lj;
>      target_ulong pc_start;
>      uint32_t next_page_start;
> @@ -1912,19 +1911,6 @@ static inline void gen_intermediate_code_internal(UniCore32CPU *cpu,
>  
>      gen_tb_start(tb);
>      do {
> -        if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
> -            QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
> -                if (bp->pc == dc->pc) {
> -                    gen_set_pc_im(dc->pc);
> -                    gen_exception(EXCP_DEBUG);
> -                    dc->is_jmp = DISAS_JUMP;
> -                    /* Advance PC so that clearing the breakpoint will
> -                       invalidate this TB.  */
> -                    dc->pc += 2; /* FIXME */
> -                    goto done_generating;
> -                }
> -            }
> -        }
>          if (search_pc) {
>              j = tcg_op_buf_count();
>              if (lj < j) {
> @@ -1940,6 +1926,16 @@ static inline void gen_intermediate_code_internal(UniCore32CPU *cpu,
>          tcg_gen_insn_start(dc->pc);
>          num_insns++;
>  
> +        if (unlikely(cpu_breakpoint_test(cs, dc->pc, BP_ANY))) {
> +            gen_set_pc_im(dc->pc);
> +            gen_exception(EXCP_DEBUG);
> +            dc->is_jmp = DISAS_JUMP;
> +            /* Advance PC so that clearing the breakpoint will
> +               invalidate this TB.  */
> +            dc->pc += 2; /* FIXME */
> +            goto done_generating;
> +        }
> +
>          if (num_insns == max_insns && (tb->cflags & CF_LAST_IO)) {
>              gen_io_start();
>          }
> diff --git a/target-xtensa/translate.c b/target-xtensa/translate.c
> index 3607e41..ea87cb5 100644
> --- a/target-xtensa/translate.c
> +++ b/target-xtensa/translate.c
> @@ -2984,22 +2984,6 @@ static inline unsigned xtensa_insn_len(CPUXtensaState *env, DisasContext *dc)
>      return xtensa_op0_insn_len(OP0);
>  }
>  
> -static void check_breakpoint(CPUXtensaState *env, DisasContext *dc)
> -{
> -    CPUState *cs = CPU(xtensa_env_get_cpu(env));
> -    CPUBreakpoint *bp;
> -
> -    if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
> -        QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
> -            if (bp->pc == dc->pc) {
> -                tcg_gen_movi_i32(cpu_pc, dc->pc);
> -                gen_exception(dc, EXCP_DEBUG);
> -                dc->is_jmp = DISAS_UPDATE;
> -             }
> -        }
> -    }
> -}
> -
>  static void gen_ibreak_check(CPUXtensaState *env, DisasContext *dc)
>  {
>      unsigned i;
> @@ -3062,8 +3046,6 @@ void gen_intermediate_code_internal(XtensaCPU *cpu,
>      }
>  
>      do {
> -        check_breakpoint(env, &dc);
> -
>          if (search_pc) {
>              j = tcg_op_buf_count();
>              if (lj < j) {
> @@ -3081,6 +3063,13 @@ void gen_intermediate_code_internal(XtensaCPU *cpu,
>  
>          ++dc.ccount_delta;
>  
> +        if (unlikely(cpu_breakpoint_test(cs, dc.pc, BP_ANY))) {
> +            tcg_gen_movi_i32(cpu_pc, dc.pc);
> +            gen_exception(&dc, EXCP_DEBUG);
> +            dc.is_jmp = DISAS_UPDATE;
> +            break;
> +        }
> +
>          if (insn_count == max_insns && (tb->cflags & CF_LAST_IO)) {
>              gen_io_start();
>          }

On 10/10/2015 12:34 AM, Sergey Fedorov wrote:
>> @@ -2936,6 +2927,10 @@ static inline void gen_intermediate_code_internal(AlphaCPU *cpu,
>>           tcg_gen_insn_start(ctx.pc);
>>           num_insns++;
>>
>> +        if (unlikely(cpu_breakpoint_test(cs, ctx.pc, BP_ANY))) {
>> +            gen_excp(&ctx, EXCP_DEBUG, 0);
>> +            break;
>> +        }
>
> Actually, control logic has changed here. The old code used a break
> statement to exit from QTAILQ_FOREACH loop and continue with instruction
> translation thus translating at least one instruction. The break
> statement in the new code makes exit from the translation loop itself,
> effectively producing zero-length TB which won't get invalidated when
> clearing the breakpoint. Seems like we should remove the break statement
> here and in similar cases below, right?

Why do you believe that a zero-length TB won't be cleared?
The TB still has a start address, which is contained within
a given page, which is invalidated.

Some target-*/translate.c takes care to advance the PC, but I believe that this 
is only required when the breakpoint instruction *itself* could span a page 
boundary.  I.e. the TB needs to be marked to span two pages.  This situation 
can never be true for many RISC targets.

We did discuss this exact situation during review of the patch set, though it's 
probably true that there are outstanding errors in some translators.


r~

On 13 October 2015 at 01:13, Richard Henderson <rth@twiddle.net> wrote:
> Why do you believe that a zero-length TB won't be cleared?
> The TB still has a start address, which is contained within
> a given page, which is invalidated.
>
> Some target-*/translate.c takes care to advance the PC, but I believe that
> this is only required when the breakpoint instruction *itself* could span a
> page boundary.  I.e. the TB needs to be marked to span two pages.  This
> situation can never be true for many RISC targets.

If this is the reason for the logic it would be good to have
a comment explaining it in the code. I've never really understood
that part...

-- PMM

On 13.10.2015 03:13, Richard Henderson wrote:
> On 10/10/2015 12:34 AM, Sergey Fedorov wrote:
>>> @@ -2936,6 +2927,10 @@ static inline void
>>> gen_intermediate_code_internal(AlphaCPU *cpu,
>>>           tcg_gen_insn_start(ctx.pc);
>>>           num_insns++;
>>>
>>> +        if (unlikely(cpu_breakpoint_test(cs, ctx.pc, BP_ANY))) {
>>> +            gen_excp(&ctx, EXCP_DEBUG, 0);
>>> +            break;
>>> +        }
>>
>> Actually, control logic has changed here. The old code used a break
>> statement to exit from QTAILQ_FOREACH loop and continue with instruction
>> translation thus translating at least one instruction. The break
>> statement in the new code makes exit from the translation loop itself,
>> effectively producing zero-length TB which won't get invalidated when
>> clearing the breakpoint. Seems like we should remove the break statement
>> here and in similar cases below, right?
>
> Why do you believe that a zero-length TB won't be cleared?
> The TB still has a start address, which is contained within
> a given page, which is invalidated.
>
> Some target-*/translate.c takes care to advance the PC, but I believe
> that this is only required when the breakpoint instruction *itself*
> could span a page boundary.  I.e. the TB needs to be marked to span
> two pages.  This situation can never be true for many RISC targets.
>
> We did discuss this exact situation during review of the patch set,
> though it's probably true that there are outstanding errors in some
> translators.

I see your point, but what I am actually concerned about is the
following scenario.

Lets suppose we are going to remove a breakpoint. Eventually we can get
the following function call stack:

#0  tb_invalidate_phys_page_range()
#1  tb_invalidate_phys_addr()
#2  breakpoint_invalidate()
#3  cpu_breakpoint_remove_by_ref()
...

Then, if we come across a zero-sized TB then 'tb_start' would be equal
to 'tb_end'. That is not a big deal unless 'start' is not equal to
'tb_start'. Otherwise, "!(tb_end <= start || tb_start >= end)" condition
check will fail and that TB won't get invalidated as it should be. As I
can see this is a case when we try to remove a breakpoint which is
happened to be set at the very first instruction of a TB. So we either
need to change the condition in tb_invalidate_phys_page_range() or do
the PC advancement trick during translation, no matter can instructions
cross a page boundary or not.

Best regards,
Sergey

On 10/14/2015 12:40 AM, Sergey Fedorov wrote:
> Otherwise, "!(tb_end <= start || tb_start >= end)" condition
> check will fail
...
> So we either
> need to change the condition in tb_invalidate_phys_page_range() or do
> the PC advancement trick during translation, no matter can instructions
> cross a page boundary or not.

I think you're right.

The reason I've not seen an issue so far is that breakpoint_invalidate expands 
the requested range to the entire page.  So the failure to invalidate would 
require the breakpoint to be at the page boundary.


r~