[Qemu-devel] [PATCH] target-*: Advance pc after recognizing a breakpoint

[Qemu-devel] [PATCH] target-*: Advance pc after recognizing a breakpoint

[Richard Henderson]

Some targets already had this within their logic, but make sure
it's present for all targets.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 target-alpha/translate.c      | 3 +++
 target-cris/translate.c       | 3 +++
 target-i386/translate.c       | 3 +++
 target-lm32/translate.c       | 3 +++
 target-m68k/translate.c       | 3 +++
 target-microblaze/translate.c | 3 +++
 target-moxie/translate.c      | 3 +++
 target-openrisc/translate.c   | 3 +++
 target-ppc/translate.c        | 3 +++
 target-s390x/translate.c      | 3 +++
 target-sh4/translate.c        | 3 +++
 target-sparc/translate.c      | 2 +-
 target-unicore32/translate.c  | 2 +-
 target-xtensa/translate.c     | 3 +++
 14 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/target-alpha/translate.c b/target-alpha/translate.c
index f936d1b..1a2d284 100644
--- a/target-alpha/translate.c
+++ b/target-alpha/translate.c
@@ -2917,6 +2917,9 @@ void gen_intermediate_code(CPUAlphaState *env, struct TranslationBlock *tb)
 
         if (unlikely(cpu_breakpoint_test(cs, ctx.pc, BP_ANY))) {
             gen_excp(&ctx, EXCP_DEBUG, 0);
+            /* Advance PC so that clearing the breakpoint will
+               invalidate this TB.  */
+            ctx.pc += 4;
             break;
         }
         if (num_insns == max_insns && (tb->cflags & CF_LAST_IO)) {
diff --git a/target-cris/translate.c b/target-cris/translate.c
index 964845c..460cedd 100644
--- a/target-cris/translate.c
+++ b/target-cris/translate.c
@@ -3166,6 +3166,9 @@ void gen_intermediate_code(CPUCRISState *env, struct TranslationBlock *tb)
             tcg_gen_movi_tl(env_pc, dc->pc);
             t_gen_raise_exception(EXCP_DEBUG);
             dc->is_jmp = DISAS_UPDATE;
+            /* Advance PC so that clearing the breakpoint will
+               invalidate this TB.  */
+            dc->pc += 2;
             break;
         }
 
diff --git a/target-i386/translate.c b/target-i386/translate.c
index ef10e68..e4c3e7e 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -7942,6 +7942,9 @@ void gen_intermediate_code(CPUX86State *env, TranslationBlock *tb)
                                          tb->flags & HF_RF_MASK
                                          ? BP_GDB : BP_ANY))) {
             gen_debug(dc, pc_ptr - dc->cs_base);
+            /* Advance PC so that clearing the breakpoint will
+               invalidate this TB.  */
+            pc_ptr += 1;
             goto done_generating;
         }
         if (num_insns == max_insns && (tb->cflags & CF_LAST_IO)) {
diff --git a/target-lm32/translate.c b/target-lm32/translate.c
index c61ad0f..0ade098 100644
--- a/target-lm32/translate.c
+++ b/target-lm32/translate.c
@@ -1078,6 +1078,9 @@ void gen_intermediate_code(CPULM32State *env, struct TranslationBlock *tb)
             tcg_gen_movi_tl(cpu_pc, dc->pc);
             t_gen_raise_exception(dc, EXCP_DEBUG);
             dc->is_jmp = DISAS_UPDATE;
+            /* Advance PC so that clearing the breakpoint will
+               invalidate this TB.  */
+            dc->pc += 4;
             break;
         }
 
diff --git a/target-m68k/translate.c b/target-m68k/translate.c
index 5995cce..93b5d2c 100644
--- a/target-m68k/translate.c
+++ b/target-m68k/translate.c
@@ -3004,6 +3004,9 @@ void gen_intermediate_code(CPUM68KState *env, TranslationBlock *tb)
         if (unlikely(cpu_breakpoint_test(cs, dc->pc, BP_ANY))) {
             gen_exception(dc, dc->pc, EXCP_DEBUG);
             dc->is_jmp = DISAS_JUMP;
+            /* Advance PC so that clearing the breakpoint will
+               invalidate this TB.  */
+            dc->pc += 2;
             break;
         }
 
diff --git a/target-microblaze/translate.c b/target-microblaze/translate.c
index a9c5010..ce76e9e 100644
--- a/target-microblaze/translate.c
+++ b/target-microblaze/translate.c
@@ -1693,6 +1693,9 @@ void gen_intermediate_code(CPUMBState *env, struct TranslationBlock *tb)
         if (unlikely(cpu_breakpoint_test(cs, dc->pc, BP_ANY))) {
             t_gen_raise_exception(dc, EXCP_DEBUG);
             dc->is_jmp = DISAS_UPDATE;
+            /* Advance PC so that clearing the breakpoint will
+               invalidate this TB.  */
+            dc->pc += 4;
             break;
         }
 
diff --git a/target-moxie/translate.c b/target-moxie/translate.c
index f84841e..9fb9082 100644
--- a/target-moxie/translate.c
+++ b/target-moxie/translate.c
@@ -848,6 +848,9 @@ void gen_intermediate_code(CPUMoxieState *env, struct TranslationBlock *tb)
             tcg_gen_movi_i32(cpu_pc, ctx.pc);
             gen_helper_debug(cpu_env);
             ctx.bstate = BS_EXCP;
+            /* Advance PC so that clearing the breakpoint will
+               invalidate this TB.  */
+            ctx.pc += 2;
             goto done_generating;
         }
 
diff --git a/target-openrisc/translate.c b/target-openrisc/translate.c
index b66fde1..0932249 100644
--- a/target-openrisc/translate.c
+++ b/target-openrisc/translate.c
@@ -1665,6 +1665,9 @@ void gen_intermediate_code(CPUOpenRISCState *env, struct TranslationBlock *tb)
             tcg_gen_movi_tl(cpu_pc, dc->pc);
             gen_exception(dc, EXCP_DEBUG);
             dc->is_jmp = DISAS_UPDATE;
+            /* Advance PC so that clearing the breakpoint will
+               invalidate this TB.  */
+            dc->pc += 4;
             break;
         }
 
diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index c2bc1a7..b15606d 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -11488,6 +11488,9 @@ void gen_intermediate_code(CPUPPCState *env, struct TranslationBlock *tb)
 
         if (unlikely(cpu_breakpoint_test(cs, ctx.nip, BP_ANY))) {
             gen_debug_exception(ctxp);
+            /* Advance PC so that clearing the breakpoint will
+               invalidate this TB.  */
+            ctx.nip += 4;
             break;
         }
 
diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index 05d51fe..8dbc4fe 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -5360,6 +5360,9 @@ void gen_intermediate_code(CPUS390XState *env, struct TranslationBlock *tb)
         if (unlikely(cpu_breakpoint_test(cs, dc.pc, BP_ANY))) {
             status = EXIT_PC_STALE;
             do_debug = true;
+            /* Advance PC so that clearing the breakpoint will
+               invalidate this TB.  */
+            dc.pc += 2;
             break;
         }
 
diff --git a/target-sh4/translate.c b/target-sh4/translate.c
index f764bc2..5fc29bd 100644
--- a/target-sh4/translate.c
+++ b/target-sh4/translate.c
@@ -1855,6 +1855,9 @@ void gen_intermediate_code(CPUSH4State * env, struct TranslationBlock *tb)
             tcg_gen_movi_i32(cpu_pc, ctx.pc);
             gen_helper_debug(cpu_env);
             ctx.bstate = BS_BRANCH;
+            /* Advance PC so that clearing the breakpoint will
+               invalidate this TB.  */
+            ctx.pc += 2;
             break;
         }
 
diff --git a/target-sparc/translate.c b/target-sparc/translate.c
index b59742a..41a3319 100644
--- a/target-sparc/translate.c
+++ b/target-sparc/translate.c
@@ -5247,6 +5247,7 @@ void gen_intermediate_code(CPUSPARCState * env, TranslationBlock * tb)
             tcg_gen_insn_start(dc->pc, dc->npc);
         }
         num_insns++;
+        last_pc = dc->pc;
 
         if (unlikely(cpu_breakpoint_test(cs, dc->pc, BP_ANY))) {
             if (dc->pc != pc_start) {
@@ -5262,7 +5263,6 @@ void gen_intermediate_code(CPUSPARCState * env, TranslationBlock * tb)
             gen_io_start();
         }
 
-        last_pc = dc->pc;
         insn = cpu_ldl_code(env, dc->pc);
 
         disas_sparc_insn(dc, insn);
diff --git a/target-unicore32/translate.c b/target-unicore32/translate.c
index 48f89fb..9d9f6e5 100644
--- a/target-unicore32/translate.c
+++ b/target-unicore32/translate.c
@@ -1919,7 +1919,7 @@ void gen_intermediate_code(CPUUniCore32State *env, TranslationBlock *tb)
             dc->is_jmp = DISAS_JUMP;
             /* Advance PC so that clearing the breakpoint will
                invalidate this TB.  */
-            dc->pc += 2; /* FIXME */
+            dc->pc += 4;
             goto done_generating;
         }
 
diff --git a/target-xtensa/translate.c b/target-xtensa/translate.c
index fda91b7..dfb3d8d 100644
--- a/target-xtensa/translate.c
+++ b/target-xtensa/translate.c
@@ -3055,6 +3055,9 @@ void gen_intermediate_code(CPUXtensaState *env, TranslationBlock *tb)
             tcg_gen_movi_i32(cpu_pc, dc.pc);
             gen_exception(&dc, EXCP_DEBUG);
             dc.is_jmp = DISAS_UPDATE;
+            /* Advance PC so that clearing the breakpoint will
+               invalidate this TB.  */
+            dc.pc += 2;
             break;
         }
 
-- 
2.4.3

Re: [Qemu-devel] [PATCH] target-*: Advance pc after recognizing a breakpoint

[Sergey Fedorov]

On 14.10.2015 01:10, Richard Henderson wrote:
> Some targets already had this within their logic, but make sure
> it's present for all targets.
>
> Signed-off-by: Richard Henderson <rth@twiddle.net>

Reported-by: Sergey Fedorov <serge.fdrv@gmail.com>

Thanks,
Sergey

> ---
>  target-alpha/translate.c      | 3 +++
>  target-cris/translate.c       | 3 +++
>  target-i386/translate.c       | 3 +++
>  target-lm32/translate.c       | 3 +++
>  target-m68k/translate.c       | 3 +++
>  target-microblaze/translate.c | 3 +++
>  target-moxie/translate.c      | 3 +++
>  target-openrisc/translate.c   | 3 +++
>  target-ppc/translate.c        | 3 +++
>  target-s390x/translate.c      | 3 +++
>  target-sh4/translate.c        | 3 +++
>  target-sparc/translate.c      | 2 +-
>  target-unicore32/translate.c  | 2 +-
>  target-xtensa/translate.c     | 3 +++
>  14 files changed, 38 insertions(+), 2 deletions(-)
>
> diff --git a/target-alpha/translate.c b/target-alpha/translate.c
> index f936d1b..1a2d284 100644
> --- a/target-alpha/translate.c
> +++ b/target-alpha/translate.c
> @@ -2917,6 +2917,9 @@ void gen_intermediate_code(CPUAlphaState *env, struct TranslationBlock *tb)
>  
>          if (unlikely(cpu_breakpoint_test(cs, ctx.pc, BP_ANY))) {
>              gen_excp(&ctx, EXCP_DEBUG, 0);
> +            /* Advance PC so that clearing the breakpoint will
> +               invalidate this TB.  */
> +            ctx.pc += 4;
>              break;
>          }
>          if (num_insns == max_insns && (tb->cflags & CF_LAST_IO)) {
> diff --git a/target-cris/translate.c b/target-cris/translate.c
> index 964845c..460cedd 100644
> --- a/target-cris/translate.c
> +++ b/target-cris/translate.c
> @@ -3166,6 +3166,9 @@ void gen_intermediate_code(CPUCRISState *env, struct TranslationBlock *tb)
>              tcg_gen_movi_tl(env_pc, dc->pc);
>              t_gen_raise_exception(EXCP_DEBUG);
>              dc->is_jmp = DISAS_UPDATE;
> +            /* Advance PC so that clearing the breakpoint will
> +               invalidate this TB.  */
> +            dc->pc += 2;
>              break;
>          }
>  
> diff --git a/target-i386/translate.c b/target-i386/translate.c
> index ef10e68..e4c3e7e 100644
> --- a/target-i386/translate.c
> +++ b/target-i386/translate.c
> @@ -7942,6 +7942,9 @@ void gen_intermediate_code(CPUX86State *env, TranslationBlock *tb)
>                                           tb->flags & HF_RF_MASK
>                                           ? BP_GDB : BP_ANY))) {
>              gen_debug(dc, pc_ptr - dc->cs_base);
> +            /* Advance PC so that clearing the breakpoint will
> +               invalidate this TB.  */
> +            pc_ptr += 1;
>              goto done_generating;
>          }
>          if (num_insns == max_insns && (tb->cflags & CF_LAST_IO)) {
> diff --git a/target-lm32/translate.c b/target-lm32/translate.c
> index c61ad0f..0ade098 100644
> --- a/target-lm32/translate.c
> +++ b/target-lm32/translate.c
> @@ -1078,6 +1078,9 @@ void gen_intermediate_code(CPULM32State *env, struct TranslationBlock *tb)
>              tcg_gen_movi_tl(cpu_pc, dc->pc);
>              t_gen_raise_exception(dc, EXCP_DEBUG);
>              dc->is_jmp = DISAS_UPDATE;
> +            /* Advance PC so that clearing the breakpoint will
> +               invalidate this TB.  */
> +            dc->pc += 4;
>              break;
>          }
>  
> diff --git a/target-m68k/translate.c b/target-m68k/translate.c
> index 5995cce..93b5d2c 100644
> --- a/target-m68k/translate.c
> +++ b/target-m68k/translate.c
> @@ -3004,6 +3004,9 @@ void gen_intermediate_code(CPUM68KState *env, TranslationBlock *tb)
>          if (unlikely(cpu_breakpoint_test(cs, dc->pc, BP_ANY))) {
>              gen_exception(dc, dc->pc, EXCP_DEBUG);
>              dc->is_jmp = DISAS_JUMP;
> +            /* Advance PC so that clearing the breakpoint will
> +               invalidate this TB.  */
> +            dc->pc += 2;
>              break;
>          }
>  
> diff --git a/target-microblaze/translate.c b/target-microblaze/translate.c
> index a9c5010..ce76e9e 100644
> --- a/target-microblaze/translate.c
> +++ b/target-microblaze/translate.c
> @@ -1693,6 +1693,9 @@ void gen_intermediate_code(CPUMBState *env, struct TranslationBlock *tb)
>          if (unlikely(cpu_breakpoint_test(cs, dc->pc, BP_ANY))) {
>              t_gen_raise_exception(dc, EXCP_DEBUG);
>              dc->is_jmp = DISAS_UPDATE;
> +            /* Advance PC so that clearing the breakpoint will
> +               invalidate this TB.  */
> +            dc->pc += 4;
>              break;
>          }
>  
> diff --git a/target-moxie/translate.c b/target-moxie/translate.c
> index f84841e..9fb9082 100644
> --- a/target-moxie/translate.c
> +++ b/target-moxie/translate.c
> @@ -848,6 +848,9 @@ void gen_intermediate_code(CPUMoxieState *env, struct TranslationBlock *tb)
>              tcg_gen_movi_i32(cpu_pc, ctx.pc);
>              gen_helper_debug(cpu_env);
>              ctx.bstate = BS_EXCP;
> +            /* Advance PC so that clearing the breakpoint will
> +               invalidate this TB.  */
> +            ctx.pc += 2;
>              goto done_generating;
>          }
>  
> diff --git a/target-openrisc/translate.c b/target-openrisc/translate.c
> index b66fde1..0932249 100644
> --- a/target-openrisc/translate.c
> +++ b/target-openrisc/translate.c
> @@ -1665,6 +1665,9 @@ void gen_intermediate_code(CPUOpenRISCState *env, struct TranslationBlock *tb)
>              tcg_gen_movi_tl(cpu_pc, dc->pc);
>              gen_exception(dc, EXCP_DEBUG);
>              dc->is_jmp = DISAS_UPDATE;
> +            /* Advance PC so that clearing the breakpoint will
> +               invalidate this TB.  */
> +            dc->pc += 4;
>              break;
>          }
>  
> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> index c2bc1a7..b15606d 100644
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -11488,6 +11488,9 @@ void gen_intermediate_code(CPUPPCState *env, struct TranslationBlock *tb)
>  
>          if (unlikely(cpu_breakpoint_test(cs, ctx.nip, BP_ANY))) {
>              gen_debug_exception(ctxp);
> +            /* Advance PC so that clearing the breakpoint will
> +               invalidate this TB.  */
> +            ctx.nip += 4;
>              break;
>          }
>  
> diff --git a/target-s390x/translate.c b/target-s390x/translate.c
> index 05d51fe..8dbc4fe 100644
> --- a/target-s390x/translate.c
> +++ b/target-s390x/translate.c
> @@ -5360,6 +5360,9 @@ void gen_intermediate_code(CPUS390XState *env, struct TranslationBlock *tb)
>          if (unlikely(cpu_breakpoint_test(cs, dc.pc, BP_ANY))) {
>              status = EXIT_PC_STALE;
>              do_debug = true;
> +            /* Advance PC so that clearing the breakpoint will
> +               invalidate this TB.  */
> +            dc.pc += 2;
>              break;
>          }
>  
> diff --git a/target-sh4/translate.c b/target-sh4/translate.c
> index f764bc2..5fc29bd 100644
> --- a/target-sh4/translate.c
> +++ b/target-sh4/translate.c
> @@ -1855,6 +1855,9 @@ void gen_intermediate_code(CPUSH4State * env, struct TranslationBlock *tb)
>              tcg_gen_movi_i32(cpu_pc, ctx.pc);
>              gen_helper_debug(cpu_env);
>              ctx.bstate = BS_BRANCH;
> +            /* Advance PC so that clearing the breakpoint will
> +               invalidate this TB.  */
> +            ctx.pc += 2;
>              break;
>          }
>  
> diff --git a/target-sparc/translate.c b/target-sparc/translate.c
> index b59742a..41a3319 100644
> --- a/target-sparc/translate.c
> +++ b/target-sparc/translate.c
> @@ -5247,6 +5247,7 @@ void gen_intermediate_code(CPUSPARCState * env, TranslationBlock * tb)
>              tcg_gen_insn_start(dc->pc, dc->npc);
>          }
>          num_insns++;
> +        last_pc = dc->pc;
>  
>          if (unlikely(cpu_breakpoint_test(cs, dc->pc, BP_ANY))) {
>              if (dc->pc != pc_start) {
> @@ -5262,7 +5263,6 @@ void gen_intermediate_code(CPUSPARCState * env, TranslationBlock * tb)
>              gen_io_start();
>          }
>  
> -        last_pc = dc->pc;
>          insn = cpu_ldl_code(env, dc->pc);
>  
>          disas_sparc_insn(dc, insn);
> diff --git a/target-unicore32/translate.c b/target-unicore32/translate.c
> index 48f89fb..9d9f6e5 100644
> --- a/target-unicore32/translate.c
> +++ b/target-unicore32/translate.c
> @@ -1919,7 +1919,7 @@ void gen_intermediate_code(CPUUniCore32State *env, TranslationBlock *tb)
>              dc->is_jmp = DISAS_JUMP;
>              /* Advance PC so that clearing the breakpoint will
>                 invalidate this TB.  */
> -            dc->pc += 2; /* FIXME */
> +            dc->pc += 4;
>              goto done_generating;
>          }
>  
> diff --git a/target-xtensa/translate.c b/target-xtensa/translate.c
> index fda91b7..dfb3d8d 100644
> --- a/target-xtensa/translate.c
> +++ b/target-xtensa/translate.c
> @@ -3055,6 +3055,9 @@ void gen_intermediate_code(CPUXtensaState *env, TranslationBlock *tb)
>              tcg_gen_movi_i32(cpu_pc, dc.pc);
>              gen_exception(&dc, EXCP_DEBUG);
>              dc.is_jmp = DISAS_UPDATE;
> +            /* Advance PC so that clearing the breakpoint will
> +               invalidate this TB.  */
> +            dc.pc += 2;
>              break;
>          }
>  

Re: [Qemu-devel] [PATCH] target-*: Advance pc after recognizing a breakpoint

[Peter Maydell]

On 13 October 2015 at 23:10, Richard Henderson <rth@twiddle.net> wrote:
> Some targets already had this within their logic, but make sure
> it's present for all targets.
>
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  target-alpha/translate.c      | 3 +++
>  target-cris/translate.c       | 3 +++
>  target-i386/translate.c       | 3 +++
>  target-lm32/translate.c       | 3 +++
>  target-m68k/translate.c       | 3 +++
>  target-microblaze/translate.c | 3 +++
>  target-moxie/translate.c      | 3 +++
>  target-openrisc/translate.c   | 3 +++
>  target-ppc/translate.c        | 3 +++
>  target-s390x/translate.c      | 3 +++
>  target-sh4/translate.c        | 3 +++
>  target-sparc/translate.c      | 2 +-
>  target-unicore32/translate.c  | 2 +-
>  target-xtensa/translate.c     | 3 +++
>  14 files changed, 38 insertions(+), 2 deletions(-)
>
> diff --git a/target-alpha/translate.c b/target-alpha/translate.c
> index f936d1b..1a2d284 100644
> --- a/target-alpha/translate.c
> +++ b/target-alpha/translate.c
> @@ -2917,6 +2917,9 @@ void gen_intermediate_code(CPUAlphaState *env, struct TranslationBlock *tb)
>
>          if (unlikely(cpu_breakpoint_test(cs, ctx.pc, BP_ANY))) {
>              gen_excp(&ctx, EXCP_DEBUG, 0);
> +            /* Advance PC so that clearing the breakpoint will
> +               invalidate this TB.  */
> +            ctx.pc += 4;
>              break;
>          }

This is still the same cryptic comment we have in the
targets which do do this. Can we have something
that is a bit more explanatory about what is going on and
why we need to do this, please?
(Also explaining what the number you need to advance by
should be would be helpful for people writing new targets
in future.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target-*: Advance pc after recognizing a breakpoint

[Richard Henderson]

On 10/15/2015 06:34 AM, Peter Maydell wrote:
> This is still the same cryptic comment we have in the
> targets which do do this. Can we have something
> that is a bit more explanatory about what is going on and
> why we need to do this, please?

Suggestions?

> (Also explaining what the number you need to advance by
> should be would be helpful for people writing new targets
> in future.)

The number needed for advancing *need* only be 1, but I've continued with the 
existing tradition of advancing by the minimum instruction size.


r~

Re: [Qemu-devel] [PATCH] target-*: Advance pc after recognizing a breakpoint

[Peter Maydell]

On 14 October 2015 at 22:02, Richard Henderson <rth@twiddle.net> wrote:
> On 10/15/2015 06:34 AM, Peter Maydell wrote:
>>
>> This is still the same cryptic comment we have in the
>> targets which do do this. Can we have something
>> that is a bit more explanatory about what is going on and
>> why we need to do this, please?
>
>
> Suggestions?

...well, I don't entirely understand the problem it's
fixing, which is why I'm asking for a better comment :-)

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target-*: Advance pc after recognizing a breakpoint

[Richard Henderson]

On 10/16/2015 03:36 AM, Peter Maydell wrote:
> On 14 October 2015 at 22:02, Richard Henderson <rth@twiddle.net> wrote:
>> On 10/15/2015 06:34 AM, Peter Maydell wrote:
>>>
>>> This is still the same cryptic comment we have in the
>>> targets which do do this. Can we have something
>>> that is a bit more explanatory about what is going on and
>>> why we need to do this, please?
>>
>>
>> Suggestions?
>
> ...well, I don't entirely understand the problem it's
> fixing, which is why I'm asking for a better comment :-)

Heh.  Fair enough.  How about

   /* The address covered by the breakpoint must be included in
      [tb->pc, tb->pc + tb->size) in order to for it to be
      properly cleared -- thus we increment the PC here so that
      the logic setting tb->size below does the right thing.  */

There are two edge cases that cause the problem with clearing that could be 
described, but I think that the comment becomes too bulky, as well as confuses 
the situation for someone cutting-and-pasting the logic to a new port.


r~

Re: [Qemu-devel] [PATCH] target-*: Advance pc after recognizing a breakpoint

[Peter Maydell]

On 16 October 2015 at 02:14, Richard Henderson <rth@twiddle.net> wrote:
> Heh.  Fair enough.  How about
>
>   /* The address covered by the breakpoint must be included in
>      [tb->pc, tb->pc + tb->size) in order to for it to be
>      properly cleared -- thus we increment the PC here so that
>      the logic setting tb->size below does the right thing.  */
>
> There are two edge cases that cause the problem with clearing that could be
> described, but I think that the comment becomes too bulky, as well as
> confuses the situation for someone cutting-and-pasting the logic to a new
> port.

OK, that sounds good. (It does suggest that we should just be
advancing PC by 1 for all targets, though.)

-- PMM

Re: [Qemu-devel] [PATCH] target-*: Advance pc after recognizing a breakpoint

[Sergey Fedorov]

On 16.10.2015 04:14, Richard Henderson wrote:
> On 10/16/2015 03:36 AM, Peter Maydell wrote:
>> On 14 October 2015 at 22:02, Richard Henderson <rth@twiddle.net> wrote:
>>> On 10/15/2015 06:34 AM, Peter Maydell wrote:
>>>>
>>>> This is still the same cryptic comment we have in the
>>>> targets which do do this. Can we have something
>>>> that is a bit more explanatory about what is going on and
>>>> why we need to do this, please?
>>>
>>>
>>> Suggestions?
>>
>> ...well, I don't entirely understand the problem it's
>> fixing, which is why I'm asking for a better comment :-)
>
> Heh.  Fair enough.  How about
>
>   /* The address covered by the breakpoint must be included in
>      [tb->pc, tb->pc + tb->size) in order to for it to be
>      properly cleared -- thus we increment the PC here so that
>      the logic setting tb->size below does the right thing.  */
>
> There are two edge cases that cause the problem with clearing that
> could be described, but I think that the comment becomes too bulky, as
> well as confuses the situation for someone cutting-and-pasting the
> logic to a new port.

Maybe we could rather fix that condition in
tb_invalidate_phys_page_range()? It seems weird that it can't handle a
zero-sized TB.

Best,
Sergey

Re: [Qemu-devel] [PATCH] target-*: Advance pc after recognizing a breakpoint

[Sergey Fedorov]

On 16.10.2015 17:08, Sergey Fedorov wrote:
> On 16.10.2015 04:14, Richard Henderson wrote:
>> On 10/16/2015 03:36 AM, Peter Maydell wrote:
>>> On 14 October 2015 at 22:02, Richard Henderson <rth@twiddle.net> wrote:
>>>> On 10/15/2015 06:34 AM, Peter Maydell wrote:
>>>>> This is still the same cryptic comment we have in the
>>>>> targets which do do this. Can we have something
>>>>> that is a bit more explanatory about what is going on and
>>>>> why we need to do this, please?
>>>>
>>>> Suggestions?
>>> ...well, I don't entirely understand the problem it's
>>> fixing, which is why I'm asking for a better comment :-)
>> Heh.  Fair enough.  How about
>>
>>   /* The address covered by the breakpoint must be included in
>>      [tb->pc, tb->pc + tb->size) in order to for it to be
>>      properly cleared -- thus we increment the PC here so that
>>      the logic setting tb->size below does the right thing.  */
>>
>> There are two edge cases that cause the problem with clearing that
>> could be described, but I think that the comment becomes too bulky, as
>> well as confuses the situation for someone cutting-and-pasting the
>> logic to a new port.
> Maybe we could rather fix that condition in
> tb_invalidate_phys_page_range()? It seems weird that it can't handle a
> zero-sized TB.

I think extending "!(tb_end <= start || tb_start >= end)" condition to
"!(tb_end <= start || tb_start >= end) || tb_start == start" would work
fine. Thoughts? I could prepare a patch for that.

Best regards,
Sergey

Re: [Qemu-devel] [PATCH] target-*: Advance pc after recognizing a breakpoint

[Sergey Fedorov]

On 16.10.2015 19:36, Sergey Fedorov wrote:
> On 16.10.2015 17:08, Sergey Fedorov wrote:
>> On 16.10.2015 04:14, Richard Henderson wrote:
>>> On 10/16/2015 03:36 AM, Peter Maydell wrote:
>>>> On 14 October 2015 at 22:02, Richard Henderson <rth@twiddle.net> wrote:
>>>>> On 10/15/2015 06:34 AM, Peter Maydell wrote:
>>>>>> This is still the same cryptic comment we have in the
>>>>>> targets which do do this. Can we have something
>>>>>> that is a bit more explanatory about what is going on and
>>>>>> why we need to do this, please?
>>>>> Suggestions?
>>>> ...well, I don't entirely understand the problem it's
>>>> fixing, which is why I'm asking for a better comment :-)
>>> Heh.  Fair enough.  How about
>>>
>>>   /* The address covered by the breakpoint must be included in
>>>      [tb->pc, tb->pc + tb->size) in order to for it to be
>>>      properly cleared -- thus we increment the PC here so that
>>>      the logic setting tb->size below does the right thing.  */
>>>
>>> There are two edge cases that cause the problem with clearing that
>>> could be described, but I think that the comment becomes too bulky, as
>>> well as confuses the situation for someone cutting-and-pasting the
>>> logic to a new port.
>> Maybe we could rather fix that condition in
>> tb_invalidate_phys_page_range()? It seems weird that it can't handle a
>> zero-sized TB.
> I think extending "!(tb_end <= start || tb_start >= end)" condition to
> "!(tb_end <= start || tb_start >= end) || tb_start == start" would work
> fine. Thoughts? I could prepare a patch for that.

But there are at least two other places which doesn't seem to support a
zero-sized TB: build_page_bitmap(), and tb_gen_code() when checking if a
next page is needed.

Best,
Sergey

Re: [Qemu-devel] [PATCH] target-*: Advance pc after recognizing a breakpoint

[Richard Henderson]

On 10/16/2015 04:08 AM, Sergey Fedorov wrote:
> On 16.10.2015 04:14, Richard Henderson wrote:
>> On 10/16/2015 03:36 AM, Peter Maydell wrote:
>>> On 14 October 2015 at 22:02, Richard Henderson <rth@twiddle.net> wrote:
>>>> On 10/15/2015 06:34 AM, Peter Maydell wrote:
>>>>>
>>>>> This is still the same cryptic comment we have in the
>>>>> targets which do do this. Can we have something
>>>>> that is a bit more explanatory about what is going on and
>>>>> why we need to do this, please?
>>>>
>>>>
>>>> Suggestions?
>>>
>>> ...well, I don't entirely understand the problem it's
>>> fixing, which is why I'm asking for a better comment :-)
>>
>> Heh.  Fair enough.  How about
>>
>>    /* The address covered by the breakpoint must be included in
>>       [tb->pc, tb->pc + tb->size) in order to for it to be
>>       properly cleared -- thus we increment the PC here so that
>>       the logic setting tb->size below does the right thing.  */
>>
>> There are two edge cases that cause the problem with clearing that
>> could be described, but I think that the comment becomes too bulky, as
>> well as confuses the situation for someone cutting-and-pasting the
>> logic to a new port.
>
> Maybe we could rather fix that condition in
> tb_invalidate_phys_page_range()? It seems weird that it can't handle a
> zero-sized TB.

We also need to be able to handle a TB which crosses a page.  E.g. the 
breakpoint is at the page boundary, and we fall through into it from the top. 
This will be true on e.g. x86.  This is not simply true for breakpoint 
insertion/removal, but also page invalidation.

The same fix, adding a byte to the size, handles this as well.


r~

Re: [Qemu-devel] [PATCH] target-*: Advance pc after recognizing a breakpoint

[Sergey Fedorov]

On 19.10.2015 01:46, Richard Henderson wrote:
> On 10/16/2015 04:08 AM, Sergey Fedorov wrote:
>> On 16.10.2015 04:14, Richard Henderson wrote:
>>> On 10/16/2015 03:36 AM, Peter Maydell wrote:
>>>> On 14 October 2015 at 22:02, Richard Henderson <rth@twiddle.net>
>>>> wrote:
>>>>> On 10/15/2015 06:34 AM, Peter Maydell wrote:
>>>>>>
>>>>>> This is still the same cryptic comment we have in the
>>>>>> targets which do do this. Can we have something
>>>>>> that is a bit more explanatory about what is going on and
>>>>>> why we need to do this, please?
>>>>>
>>>>>
>>>>> Suggestions?
>>>>
>>>> ...well, I don't entirely understand the problem it's
>>>> fixing, which is why I'm asking for a better comment :-)
>>>
>>> Heh.  Fair enough.  How about
>>>
>>>    /* The address covered by the breakpoint must be included in
>>>       [tb->pc, tb->pc + tb->size) in order to for it to be
>>>       properly cleared -- thus we increment the PC here so that
>>>       the logic setting tb->size below does the right thing.  */
>>>
>>> There are two edge cases that cause the problem with clearing that
>>> could be described, but I think that the comment becomes too bulky, as
>>> well as confuses the situation for someone cutting-and-pasting the
>>> logic to a new port.
>>
>> Maybe we could rather fix that condition in
>> tb_invalidate_phys_page_range()? It seems weird that it can't handle a
>> zero-sized TB.
>
> We also need to be able to handle a TB which crosses a page.  E.g. the
> breakpoint is at the page boundary, and we fall through into it from
> the top. This will be true on e.g. x86.  This is not simply true for
> breakpoint insertion/removal, but also page invalidation.
>
> The same fix, adding a byte to the size, handles this as well.

It's clear except that instructions crossing a page boundary can be
different in size. AFAIK, x86 instructions can be up to 15-byte long.
What if only the very last byte of instruction crosses a page boundary?

Best regards,
Sergey

Re: [Qemu-devel] [PATCH] target-*: Advance pc after recognizing a breakpoint

[Richard Henderson]

On 10/19/2015 01:04 AM, Sergey Fedorov wrote:
> On 19.10.2015 01:46, Richard Henderson wrote:
>> On 10/16/2015 04:08 AM, Sergey Fedorov wrote:
>>> On 16.10.2015 04:14, Richard Henderson wrote:
>>>> On 10/16/2015 03:36 AM, Peter Maydell wrote:
>>>>> On 14 October 2015 at 22:02, Richard Henderson <rth@twiddle.net>
>>>>> wrote:
>>>>>> On 10/15/2015 06:34 AM, Peter Maydell wrote:
>>>>>>>
>>>>>>> This is still the same cryptic comment we have in the
>>>>>>> targets which do do this. Can we have something
>>>>>>> that is a bit more explanatory about what is going on and
>>>>>>> why we need to do this, please?
>>>>>>
>>>>>>
>>>>>> Suggestions?
>>>>>
>>>>> ...well, I don't entirely understand the problem it's
>>>>> fixing, which is why I'm asking for a better comment :-)
>>>>
>>>> Heh.  Fair enough.  How about
>>>>
>>>>     /* The address covered by the breakpoint must be included in
>>>>        [tb->pc, tb->pc + tb->size) in order to for it to be
>>>>        properly cleared -- thus we increment the PC here so that
>>>>        the logic setting tb->size below does the right thing.  */
>>>>
>>>> There are two edge cases that cause the problem with clearing that
>>>> could be described, but I think that the comment becomes too bulky, as
>>>> well as confuses the situation for someone cutting-and-pasting the
>>>> logic to a new port.
>>>
>>> Maybe we could rather fix that condition in
>>> tb_invalidate_phys_page_range()? It seems weird that it can't handle a
>>> zero-sized TB.
>>
>> We also need to be able to handle a TB which crosses a page.  E.g. the
>> breakpoint is at the page boundary, and we fall through into it from
>> the top. This will be true on e.g. x86.  This is not simply true for
>> breakpoint insertion/removal, but also page invalidation.
>>
>> The same fix, adding a byte to the size, handles this as well.
>
> It's clear except that instructions crossing a page boundary can be
> different in size. AFAIK, x86 instructions can be up to 15-byte long.
> What if only the very last byte of instruction crosses a page boundary?

Then only the last byte crosses?  What's your point?


r~

Re: [Qemu-devel] [PATCH] target-*: Advance pc after recognizing a breakpoint

[Sergey Fedorov]

On 19.10.2015 20:04, Richard Henderson wrote:
> On 10/19/2015 01:04 AM, Sergey Fedorov wrote:
>> On 19.10.2015 01:46, Richard Henderson wrote:
>>> On 10/16/2015 04:08 AM, Sergey Fedorov wrote:
>>>> On 16.10.2015 04:14, Richard Henderson wrote:
>>>>> On 10/16/2015 03:36 AM, Peter Maydell wrote:
>>>>>> On 14 October 2015 at 22:02, Richard Henderson <rth@twiddle.net>
>>>>>> wrote:
>>>>>>> On 10/15/2015 06:34 AM, Peter Maydell wrote:
>>>>>>>>
>>>>>>>> This is still the same cryptic comment we have in the
>>>>>>>> targets which do do this. Can we have something
>>>>>>>> that is a bit more explanatory about what is going on and
>>>>>>>> why we need to do this, please?
>>>>>>>
>>>>>>>
>>>>>>> Suggestions?
>>>>>>
>>>>>> ...well, I don't entirely understand the problem it's
>>>>>> fixing, which is why I'm asking for a better comment :-)
>>>>>
>>>>> Heh.  Fair enough.  How about
>>>>>
>>>>>     /* The address covered by the breakpoint must be included in
>>>>>        [tb->pc, tb->pc + tb->size) in order to for it to be
>>>>>        properly cleared -- thus we increment the PC here so that
>>>>>        the logic setting tb->size below does the right thing.  */
>>>>>
>>>>> There are two edge cases that cause the problem with clearing that
>>>>> could be described, but I think that the comment becomes too
>>>>> bulky, as
>>>>> well as confuses the situation for someone cutting-and-pasting the
>>>>> logic to a new port.
>>>>
>>>> Maybe we could rather fix that condition in
>>>> tb_invalidate_phys_page_range()? It seems weird that it can't handle a
>>>> zero-sized TB.
>>>
>>> We also need to be able to handle a TB which crosses a page.  E.g. the
>>> breakpoint is at the page boundary, and we fall through into it from
>>> the top. This will be true on e.g. x86.  This is not simply true for
>>> breakpoint insertion/removal, but also page invalidation.
>>>
>>> The same fix, adding a byte to the size, handles this as well.
>>
>> It's clear except that instructions crossing a page boundary can be
>> different in size. AFAIK, x86 instructions can be up to 15-byte long.
>> What if only the very last byte of instruction crosses a page boundary?
>
> Then only the last byte crosses?  What's your point?

My point is if "adding a byte to the size" handles such case as well.

Best,
Sergey