| Message ID | 20141022113831.9548.36452.stgit@PASHA-ISP |
|---|---|
| State | New |
| Headers | show |
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 <pavel.dovgaluk@ispras.ru> > --- > 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? > } > } > } > @@ -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? > gen_tb_end(tb, num_insns); > > I'll give it a try later and I'll let you know. Thanks, Fred
> From: Frederic Konrad [mailto:fred.konrad@greensocs.com] > 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 <pavel.dovgaluk@ispras.ru> > > --- > > 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 :) > > } > > } > > } > > @@ -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? > > > gen_tb_end(tb, num_insns); > > > > > > I'll give it a try later and I'll let you know. Thanks. Pavel Dovgalyuk
On 23/10/2014 07:57, Pavel Dovgaluk wrote: >> From: Frederic Konrad [mailto:fred.konrad@greensocs.com] >> 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 <pavel.dovgaluk@ispras.ru> >>> --- >>> 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? Fred > >>> gen_tb_end(tb, num_insns); >>> >>> >> I'll give it a try later and I'll let you know. > Thanks. > > Pavel Dovgalyuk > >
> From: Frederic Konrad [mailto:fred.konrad@greensocs.com] > On 23/10/2014 07:57, Pavel Dovgaluk wrote: > >> From: Frederic Konrad [mailto:fred.konrad@greensocs.com] > >> 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 <pavel.dovgaluk@ispras.ru> > >>> --- > >>> 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
On 23/10/2014 09:52, Pavel Dovgaluk wrote: >> From: Frederic Konrad [mailto:fred.konrad@greensocs.com] >> On 23/10/2014 07:57, Pavel Dovgaluk wrote: >>>> From: Frederic Konrad [mailto:fred.konrad@greensocs.com] >>>> 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 <pavel.dovgaluk@ispras.ru> >>>>> --- >>>>> 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? eg: static inline void gen_ins(DisasContext *s, TCGMemOp ot) { if (use_icount) gen_io_start(); gen_string_movl_A0_EDI(s); /* Note: we must do this dummy write first to be restartable in case of page fault. */ tcg_gen_movi_tl(cpu_T[0], 0); gen_op_st_v(s, ot, cpu_T[0], cpu_A0); tcg_gen_trunc_tl_i32(cpu_tmp2_i32, cpu_regs[R_EDX]); tcg_gen_andi_i32(cpu_tmp2_i32, cpu_tmp2_i32, 0xffff); gen_helper_in_func(ot, cpu_T[0], cpu_tmp2_i32); gen_op_st_v(s, ot, cpu_T[0], cpu_A0); gen_op_movl_T0_Dshift(ot); gen_op_add_reg_T0(s->aflag, R_EDI); if (use_icount) gen_io_end(); }
On 22/10/2014 13:38, Pavel Dovgalyuk wrote: > 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 <pavel.dovgaluk@ispras.ru> > --- > 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; > } > } > } > @@ -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(); > gen_tb_end(tb, num_insns); > Applied, thanks. Paolo
On 2014-10-22 13:38, Pavel Dovgalyuk wrote: > 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 <pavel.dovgaluk@ispras.ru> > --- > 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; > } > } > } > @@ -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(); > gen_tb_end(tb, num_insns); > > > Didn't looked into why, just bisected that this patch breaks at least certain guest-originated break- or watchpoints in TCG mode. Can be triggered by booting a Linux kernel with kgdb self-tests enabled. The result is some false reporting of a host-originated debug stop to gdb_set_stop_cpu while gdbserver_state is NULL -> SEGV. Jan
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; } } } @@ -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(); gen_tb_end(tb, num_insns);
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 <pavel.dovgaluk@ispras.ru> --- target-i386/translate.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)