| Message ID | 1406263073-635-1-git-send-email-jmiao@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 07/24/2014 06:37 PM, Jincheng Miao wrote: > '-singlestep' option will make TB contains only one instruction, > so that the qemu_log could output trace log when CPU_LOG_EXEC sets, > and it could help developers to debug control flow. > > But currently, in cpu_exec(), it doesn't check singlestep when > tb_add_jump(), so the TB linked is executed siliently. > Therefore, this patch adds singlestep check before tb_add_jump(). > > Signed-off-by: Jincheng Miao <jmiao@redhat.com> Reasonable. I've been thinking that we simply shoudn't emit goto_tb under single-step. That does require fixes to all but 2 or 3 of the backends though, and this patch attacks the problem all in one place. Reviewed-by: Richard Henderson <rth@twiddle.net> r~
On 25 July 2014 07:58, Richard Henderson <rth@twiddle.net> wrote: > On 07/24/2014 06:37 PM, Jincheng Miao wrote: >> '-singlestep' option will make TB contains only one instruction, >> so that the qemu_log could output trace log when CPU_LOG_EXEC sets, >> and it could help developers to debug control flow. >> >> But currently, in cpu_exec(), it doesn't check singlestep when >> tb_add_jump(), so the TB linked is executed siliently. >> Therefore, this patch adds singlestep check before tb_add_jump(). >> >> Signed-off-by: Jincheng Miao <jmiao@redhat.com> > > Reasonable. I've been thinking that we simply shoudn't emit goto_tb under > single-step. That does require fixes to all but 2 or 3 of the backends though, > and this patch attacks the problem all in one place. Huh? We already don't emit goto_tb if single-stepping, surely? (Well, I guess some of the backends might well be broken, but in that case they probably don't get the other bits of singlestep support right either...) -- PMM
On 07/24/2014 09:37 PM, Peter Maydell wrote: > Huh? We already don't emit goto_tb if single-stepping, surely? > (Well, I guess some of the backends might well be broken, but > in that case they probably don't get the other bits of singlestep > support right either...) Indeed. I noticed this a month or so ago. Almost all backends check the gdb env->single_step to prevent goto_tb, but forget about the tcg debugging singlestep. r~
On 25 July 2014 08:41, Richard Henderson <rth@twiddle.net> wrote: > On 07/24/2014 09:37 PM, Peter Maydell wrote: >> Huh? We already don't emit goto_tb if single-stepping, surely? >> (Well, I guess some of the backends might well be broken, but >> in that case they probably don't get the other bits of singlestep >> support right either...) > > Indeed. I noticed this a month or so ago. > > Almost all backends check the gdb env->single_step to prevent goto_tb, but > forget about the tcg debugging singlestep. Oh, we have two flavours of singlestep? That's confusing... (I'm currently working on the ARMv8 architectural singlestep, which will make 3 for target-arm.) thanks -- PMM
On 07/25/2014 03:45 PM, Peter Maydell wrote: > On 25 July 2014 08:41, Richard Henderson <rth@twiddle.net> wrote: >> On 07/24/2014 09:37 PM, Peter Maydell wrote: >>> Huh? We already don't emit goto_tb if single-stepping, surely? >>> (Well, I guess some of the backends might well be broken, but >>> in that case they probably don't get the other bits of singlestep >>> support right either...) >> Indeed. I noticed this a month or so ago. >> >> Almost all backends check the gdb env->single_step to prevent goto_tb, but >> forget about the tcg debugging singlestep. > Oh, we have two flavours of singlestep? That's confusing... IMHO, CPUState->singlestep_enabled is a cpu execute mode, for emulating it, an exception should be raised. But '-singlestep' from command line rules qemu how to generate TBs and their generated codes. In this situation, a TB only contains one instruction, and should be unlinked. Am I right? > (I'm currently working on the ARMv8 architectural singlestep, > which will make 3 for target-arm.) > > thanks > -- PMM
Hello, On Fri, Jul 25, 2014 at 6:37 AM, Jincheng Miao <jmiao@redhat.com> wrote: > '-singlestep' option will make TB contains only one instruction, > so that the qemu_log could output trace log when CPU_LOG_EXEC sets, > and it could help developers to debug control flow. > > But currently, in cpu_exec(), it doesn't check singlestep when > tb_add_jump(), so the TB linked is executed siliently. > Therefore, this patch adds singlestep check before tb_add_jump(). > > Signed-off-by: Jincheng Miao <jmiao@redhat.com> I tested your patch in an environment generating run time traces and it works fine. Tested-by: Laurent Desnogues <laurent.desnogues@gmail.com> Thanks, Laurent > --- > cpu-exec.c | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/cpu-exec.c b/cpu-exec.c > index 38e5f02..64b7289 100644 > --- a/cpu-exec.c > +++ b/cpu-exec.c > @@ -622,8 +622,8 @@ int cpu_exec(CPUArchState *env) > } > /* see if we can patch the calling TB. When the TB > spans two pages, we cannot safely do a direct > - jump. */ > - if (next_tb != 0 && tb->page_addr[1] == -1) { > + jump. So as when singlestep is enabled. */ > + if (next_tb != 0 && tb->page_addr[1] == -1 && !singlestep) { > tb_add_jump((TranslationBlock *)(next_tb & ~TB_EXIT_MASK), > next_tb & TB_EXIT_MASK, tb); > } > -- > 1.7.1 > >
diff --git a/cpu-exec.c b/cpu-exec.c index 38e5f02..64b7289 100644 --- a/cpu-exec.c +++ b/cpu-exec.c @@ -622,8 +622,8 @@ int cpu_exec(CPUArchState *env) } /* see if we can patch the calling TB. When the TB spans two pages, we cannot safely do a direct - jump. */ - if (next_tb != 0 && tb->page_addr[1] == -1) { + jump. So as when singlestep is enabled. */ + if (next_tb != 0 && tb->page_addr[1] == -1 && !singlestep) { tb_add_jump((TranslationBlock *)(next_tb & ~TB_EXIT_MASK), next_tb & TB_EXIT_MASK, tb); }
'-singlestep' option will make TB contains only one instruction, so that the qemu_log could output trace log when CPU_LOG_EXEC sets, and it could help developers to debug control flow. But currently, in cpu_exec(), it doesn't check singlestep when tb_add_jump(), so the TB linked is executed siliently. Therefore, this patch adds singlestep check before tb_add_jump(). Signed-off-by: Jincheng Miao <jmiao@redhat.com> --- cpu-exec.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)