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

[Peter Maydell]

On 23 October 2015 at 23:00, 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 
> ---
> Version 2 updates the language as discussed in the followup in v1.
>
> Peter, in that followup you mentioned that we ought to just use +1
> for all targets.  I was about to do that when I noticed the comment
> in the arm32 port about it being better to use the correct insn size
> in order to avoid warnings from the disassembler.  So I left my good
> estimates as-is from v1.

Reviewed-by: Peter Maydell 

though really we should shut up the disassembler some other way
somehow..."pick the insn length" doesn't work for variable-insn-length
CPUs.

thanks
-- PMM

[Qemu-devel] [PATCH v2] 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 
---
Version 2 updates the language as discussed in the followup in v1.

Peter, in that followup you mentioned that we ought to just use +1
for all targets.  I was about to do that when I noticed the comment
in the arm32 port about it being better to use the correct insn size
in order to avoid warnings from the disassembler.  So I left my good
estimates as-is from v1.


r~
---
 target-alpha/translate.c  | 5 +
 target-arm/translate-a64.c| 7 +--
 target-arm/translate.c| 7 +--
 target-cris/translate.c   | 5 +
 target-i386/translate.c   | 5 +
 target-lm32/translate.c   | 5 +
 target-m68k/translate.c   | 5 +
 target-microblaze/translate.c | 5 +
 target-mips/translate.c   | 6 --
 target-moxie/translate.c  | 5 +
 target-openrisc/translate.c   | 5 +
 target-ppc/translate.c| 5 +
 target-s390x/translate.c  | 5 +
 target-sh4/translate.c| 5 +
 target-sparc/translate.c  | 2 +-
 target-unicore32/translate.c  | 8 +---
 target-xtensa/translate.c | 5 +
 17 files changed, 80 insertions(+), 10 deletions(-)

diff --git a/target-alpha/translate.c b/target-alpha/translate.c
index f936d1b..87950c6 100644
--- a/target-alpha/translate.c
+++ b/target-alpha/translate.c
@@ -2917,6 +2917,11 @@ void gen_intermediate_code(CPUAlphaState *env, struct 
TranslationBlock *tb)
 
 if (unlikely(cpu_breakpoint_test(cs, ctx.pc, BP_ANY))) {
 gen_excp(, EXCP_DEBUG, 0);
+/* 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.  */
+ctx.pc += 4;
 break;
 }
 if (num_insns == max_insns && (tb->cflags & CF_LAST_IO)) {
diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index 19f9d8d..83b8376 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -11096,8 +11096,11 @@ void gen_intermediate_code_a64(ARMCPU *cpu, 
TranslationBlock *tb)
 dc->is_jmp = DISAS_UPDATE;
 } else {
 gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
-/* Advance PC so that clearing the breakpoint will
-   invalidate this TB.  */
+/* 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.  */
 dc->pc += 4;
 goto done_generating;
 }
diff --git a/target-arm/translate.c b/target-arm/translate.c
index 9f1d740..ee631af 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -11348,8 +11348,11 @@ void gen_intermediate_code(CPUARMState *env, 
TranslationBlock *tb)
 dc->is_jmp = DISAS_UPDATE;
 } else {
 gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
-/* Advance PC so that clearing the breakpoint will
-   invalidate this TB.  */
+/* 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.  */
 /* TODO: Advance PC by correct instruction length to
  * avoid disassembler error messages */
 dc->pc += 2;
diff --git a/target-cris/translate.c b/target-cris/translate.c
index 964845c..2d710cc 100644
--- a/target-cris/translate.c
+++ b/target-cris/translate.c
@@ -3166,6 +3166,11 @@ 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;
+/* 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.  */
+dc->pc += 2;
 break;
 }
 
diff --git a/target-i386/translate.c b/target-i386/translate.c
index ef10e68..17f6601 100644
---