Re: [Qemu-devel] [PATCH] i386: fix breakpoints handling in icount mode

[Paolo Bonzini <pbonzini@redhat.com>]

On 24/05/2015 16:43, Jan Kiszka wrote:
> diff --git a/target-i386/translate.c b/target-i386/translate.c
> index 305ce50..57b607d 100644
> --- a/target-i386/translate.c
> +++ b/target-i386/translate.c
> @@ -8006,6 +8006,7 @@ static inline void gen_intermediate_code_internal(X86CPU *cpu,
>                  if (bp->pc == pc_ptr &&
>                      !((bp->flags & BP_CPU) && (tb->flags & HF_RF_MASK))) {
>                      gen_debug(dc, pc_ptr - dc->cs_base);
> +                    pc_ptr = disas_insn(env, dc, pc_ptr);
>                      goto done_generating;
>                  }
>              }
> 
> pc_ptr is used at the end of the function to calculate the tb size. I
> suspect that the difference prevents that the breakpoint event is
> associated with the stored location. Can someone explain this more
> properly? Then I would happily pass patch credits.

So when a breakpoint is removed at address X, you have to also remove 
translation blocks that end exactly at X?  That is:

diff --git a/translate-all.c b/translate-all.c
index 536008f..6e50b8f 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -1113,7 +1116,7 @@
             tb_start = tb->page_addr[1];
             tb_end = tb_start + ((tb->pc + tb->size) & ~TARGET_PAGE_MASK);
         }
-        if (!(tb_end <= start || tb_start >= end)) {
+        if (tb_start < end && tb_end >= start) {
 #ifdef TARGET_HAS_PRECISE_SMC
             if (current_tb_not_found) {
                 current_tb_not_found = 0;

Does this fix the bug?  Is there any other case where this
is desirable?  Should tb_invalidate_phys_page_range grow another
argument to choose between "tb_end > start" and "tb_end >= start"?

Paolo