[Qemu-devel] [PATCH] cpu-exec: make TBs generated codes unlinked when -singlestep

[Qemu-devel] [PATCH] cpu-exec: make TBs generated codes unlinked when -singlestep

[Jincheng Miao]

'-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(-)

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

Re: [Qemu-devel] [PATCH] cpu-exec: make TBs generated codes unlinked when -singlestep

[Richard Henderson]

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~

Re: [Qemu-devel] [PATCH] cpu-exec: make TBs generated codes unlinked when -singlestep

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] cpu-exec: make TBs generated codes unlinked when -singlestep

[Richard Henderson]

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~

Re: [Qemu-devel] [PATCH] cpu-exec: make TBs generated codes unlinked when -singlestep

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] cpu-exec: make TBs generated codes unlinked when -singlestep

[Jincheng Miao]

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

Re: [Qemu-devel] [PATCH] cpu-exec: make TBs generated codes unlinked when -singlestep

[Laurent Desnogues]

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
>
>