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

[Pavel Dovgaluk]

> From: Frederic Konrad [mailto:address@hidden
> On 23/10/2014 09:52, Pavel Dovgaluk wrote:
> >> From: Frederic Konrad [mailto:address@hidden
> >> On 23/10/2014 07:57, Pavel Dovgaluk wrote:
> >>>> From: Frederic Konrad [mailto:address@hidden
> >>>> On 22/10/2014 13:38, Pavel Dovgalyuk wrote:
> >>>>
> >>>> Hi Pavel,
> >>>>> This patch fixes instructions counting when execution is stopped on
> >>>>> breakpoint (e.g. set from gdb). Without a patch extra instruction is 
> >>>>> translated
> >>>>> and icount is incremented by invalid value (which equals to number of
> >>>>> executed instructions + 1).
> >>>>>
> >>>>> Signed-off-by: Pavel Dovgalyuk <address@hidden>
> >>>>> ---
> >>>>>     target-i386/translate.c |    3 ++-
> >>>>>     1 files changed, 2 insertions(+), 1 deletions(-)
> >>>>>
> >>>>> diff --git a/target-i386/translate.c b/target-i386/translate.c
> >>>>> index 1284173..193cf9f 100644
> >>>>> --- a/target-i386/translate.c
> >>>>> +++ b/target-i386/translate.c
> >>>>> @@ -8000,7 +8000,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);
> >>>>> -                    break;
> >>>>> +                    goto done_generating;
> >>>> This makes sense to me.
> >>>> But I don't see why you don't just "break" like the other instruction in
> >>>> this loop?
> >>> Single break will just exit the breakpoints iteration loop. I'll need an 
> >>> additional flag
> >>> to break the translation loop. ARM does the same thing, anyway :)
> >> Yes that's what I mentioned.
> >>>>>                     }
> >>>>>                 }
> >>>>>             }
> >>>>> @@ -8049,6 +8049,7 @@ static inline void 
> >>>>> gen_intermediate_code_internal(X86CPU *cpu,
> >>>>>                 break;
> >>>>>             }
> >>>>>         }
> >>>>> +done_generating:
> >>>>>         if (tb->cflags & CF_LAST_IO)
> >>>>>             gen_io_end();
> >>>> Is there any reason why you don't jump over this two lines in case of a
> >>>> breakpoint?
> >>> Shouldn't we switch off can_do_io flag if it was switched on?
> >> Yes but can we switch on can_do_io if we have a breakpoint?
> >>
> >> The code is not shown in this patch but there is:
> >>
> >>           if (num_insns + 1 == max_insns && (tb->cflags & CF_LAST_IO))
> >>               gen_io_start();
> >>
> >> I think you can't reach this code if you exit the translation loop?
> > This is not the only gen_io_start call. It is called from some of the 
> > instructions'
> > translation functions, that could precede the breakpoint.
> >
> > Pavel Dovgalyuk
> >
> >
> True, there are 8 others place where gen_io_start is called in this
> file, but they
> seems to be each time followed by a gen_io_end?

Right. Here is the updated patch:

diff --git a/target-i386/translate.c b/target-i386/translate.c
index 1284173..4d5dfb3 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -8000,7 +8000,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);
-                    break;
+                    goto done_generating;
                 }
             }
         }
@@ -8051,6 +8051,7 @@ static inline void gen_intermediate_code_internal(X86CPU 
*cpu,
     }
     if (tb->cflags & CF_LAST_IO)
         gen_io_end();
+done_generating:
     gen_tb_end(tb, num_insns);
     *tcg_ctx.gen_opc_ptr = INDEX_op_end;
     /* we don't forget to fill the last values */