[Qemu-devel] Actual TB code doesn't look like what was intended (TCG issue)?

[Qemu-devel] Actual TB code doesn't look like what was intended (TCG issue)?

[Max Filippov]

Hello guys.

I'm running qemu on x86_64 host.
It's clean build from git sources dated 2011.05.19, commit 1fddfba129f5435c80eda14e8bc23fdb888c7187
I have the following output from "log trace,op,out_asm":

Trace 0x4000a310 [d0026c92]
OP:
 ---- 0xd00000c0
 movi_i32 tmp1,$0xfffffff4
 add_i32 tmp0,ar9,tmp1
 qemu_ld32 ar1,tmp0,$0x0

 ---- 0xd00000c3
 movi_i32 tmp1,$0xfffffff0
 add_i32 tmp0,ar9,tmp1
 qemu_ld32 ar0,tmp0,$0x0

[...snip...]

OUT: [size=664]
0x4000a330:  mov    0x2c(%r14),%ebp
0x4000a334:  lea    -0xc(%rbp),%ebx
0x4000a337:  mov    %ebx,%esi
0x4000a339:  mov    %ebx,%edi
0x4000a33b:  shr    $0x7,%esi
0x4000a33e:  and    $0xfffff003,%edi
0x4000a344:  and    $0x1fe0,%esi
0x4000a34a:  lea    0x1000(%r14,%rsi,1),%rsi
0x4000a352:  cmp    (%rsi),%edi
0x4000a354:  mov    %ebx,%edi
0x4000a356:  jne    0x4000a360
0x4000a358:  add    0x10(%rsi),%rdi
0x4000a35c:  mov    (%rdi),%ebp
0x4000a35e:  jmp    0x4000a369
0x4000a360:  xor    %esi,%esi
0x4000a362:  callq  0x52edc2
0x4000a367:  mov    %eax,%ebp
0x4000a369:  mov    0x2c(%r14),%ebx
0x4000a36d:  lea    -0x10(%rbx),%r12d
0x4000a371:  mov    %ebp,0xc(%r14)
0x4000a375:  mov    %r12d,%esi
0x4000a378:  mov    %r12d,%edi
[...snip...]

Execution of this fragment eventually causes SIGSEGV.
In gdb actually generated code for this TB looks like this:

(gdb) x/25i 0x4000a330
   0x4000a330:  mov    0x2c(%r14),%ebp
   0x4000a334:  lea    -0xc(%rbp),%ebx
   0x4000a337:  mov    %ebx,%esi
   0x4000a339:  mov    %ebx,%edi
   0x4000a33b:  shr    $0x7,%esi
   0x4000a33e:  and    $0xfffff003,%edi
   0x4000a344:  and    $0x1fe0,%esi
   0x4000a34a:  lea    0x3000(%r14,%rsi,1),%rsi
   0x4000a352:  cmp    (%rsi),%edi
   0x4000a354:  mov    %ebx,%edi
   0x4000a356:  jne    0x4000a360
   0x4000a358:  add    0x10(%rsi),%rdi
   0x4000a35c:  mov    (%rdi),%ebp
   0x4000a35e:  jmp    0x4000a36c
   0x4000a360:  mov    $0x1,%esi
   0x4000a365:  callq  0x52edc2 <__ldl_mmu>
   0x4000a36a:  mov    %eax,%ebp
   0x4000a36c:  sub    $0x44,%al
=> 0x4000a36e:  lea    -0x10(%rbx),%esp
   0x4000a371:  mov    %ebp,0xc(%r14)
   0x4000a375:  mov    %r12d,%esi
   0x4000a378:  mov    %r12d,%edi

Please note how the current instruction in gdb differ from what was said in OUT. This lea corrupts stack pointer and the next callq generates segfault.
Could please anyone familiar with TCG take a look at this, or suggest where I should look myself?

Thanks.
-- Max

Re: [Qemu-devel] Actual TB code doesn't look like what was intended (TCG issue)?

[Peter Maydell]

On 24 June 2011 03:44, Max Filippov <jcmvbkbc@gmail.com> wrote:

> Please note how the current instruction in gdb differ from what
> was said in OUT. This lea corrupts stack pointer and the next
> callq generates segfault.
> Could please anyone familiar with TCG take a look at this, or
> suggest where I should look myself?

You don't say which target you're compiling code for, or what
the input assembly was which triggered this.

My first guess is that the target's front end might have a bug
where it wrongly bakes in assumptions about bits of the CPUState.
QEMU will occasionally retranslate-in-place a TB (if a load in
the TB causes an exception) so if the frontend generates different
code the second time around things will go wrong...

You should be able to find out what's stomping on the code
with the aid of a debugger and some watchpoints.

-- PMM

Re: [Qemu-devel] Actual TB code doesn't look like what was intended (TCG issue)?

[Max Filippov]

> > Please note how the current instruction in gdb differ from what
> > was said in OUT. This lea corrupts stack pointer and the next
> > callq generates segfault.
> > Could please anyone familiar with TCG take a look at this, or
> > suggest where I should look myself?
> 
> You don't say which target you're compiling code for, or what
> the input assembly was which triggered this.

I thought it doesn't matter. It's target-xtensa that I've been developing, input assembly is the following:

d00000c0 <_WindowUnderflow8>:
d00000c0:       09d910          l32e    a1, a9, -12
d00000c3:       09c900          l32e    a0, a9, -16
d00000c6:       09d170          l32e    a7, a1, -12
d00000c9:       09e920          l32e    a2, a9, -8
d00000cc:       09f930          l32e    a3, a9, -4
d00000cf:       098740          l32e    a4, a7, -32
d00000d2:       099750          l32e    a5, a7, -28
d00000d5:       09a760          l32e    a6, a7, -24
d00000d8:       09b770          l32e    a7, a7, -20
d00000db:       003500          rfwu

> My first guess is that the target's front end might have a bug
> where it wrongly bakes in assumptions about bits of the CPUState.
> QEMU will occasionally retranslate-in-place a TB (if a load in
> the TB causes an exception) so if the frontend generates different
> code the second time around things will go wrong...
> 
> You should be able to find out what's stomping on the code
> with the aid of a debugger and some watchpoints.

I just thought that "lea    -0x10(%rbx),%esp" may not appear in generated code at all, and in the OUT section (which is for different MMU mode, as I can see now) it is "lea    -0x10(%rbx),%r12d".
The instruction itself looks odd: it writes to esp and the sizes of the registers it operates on are different.

Thanks.
-- Max

Re: [Qemu-devel] Actual TB code doesn't look like what was intended (TCG issue)?

[Peter Maydell]

On 24 June 2011 09:34, Max Filippov <jcmvbkbc@gmail.com> wrote:
> I thought it doesn't matter. It's target-xtensa that I've been
> developing

Ah...

>> My first guess is that the target's front end might have a bug
>> where it wrongly bakes in assumptions about bits of the CPUState.
>> QEMU will occasionally retranslate-in-place a TB (if a load in
>> the TB causes an exception) so if the frontend generates different
>> code the second time around things will go wrong...
>>
>> You should be able to find out what's stomping on the code
>> with the aid of a debugger and some watchpoints.
>
> I just thought that "lea    -0x10(%rbx),%esp" may not appear
> in generated code at all, and in the OUT section (which is for
> different MMU mode, as I can see now) it is
> "lea    -0x10(%rbx),%r12d".
> The instruction itself looks odd: it writes to esp and the sizes
> of the registers it operates on are different.

Yes, typically when we retranslate code in place then the second
time around we will stop before the end of the TB, so if there
is a mismatch in the generated code this usually manifests as
a corrupted instruction where half of it got rewritten.

Here are my rules of thumb for generating code where the code
generated might change based on some bit of CPU state:

When you are generating code, if the code you generate will
change based on the contents of something in the CPUState struct,
then the bit of CPUState you are looking at has to be one of:
 (1) encoded in the TB flags (or tb->pc or tb->cs_base)
      (and gen_intermediate_code_internal() must read and
      use the value in tb->tb_flags, not the one in env)
 (2) always constant for the life of the simulation
      (eg env->features if you have some sort of
      "target feature support" flags)
 (3) specially handled so that when it changes then
      all TBs or at least all affected TBs are flushed
      (env->breakpoints is in this category), and also
      if the change is the result of some instruction then
      you must terminate the TB after that instruction.
      This is often used for things that rarely change and/or
      where you can't fit the whole value into tb_flags.

The reason for this is that the CPUState you're passed in
is not guaranteed to be the state of the CPU at the PC
which you are starting translation from.

This is the xtensa port at
http://jcmvbkbc.spb.ru/git/?p=dumb/qemu-xtensa.git;a=shortlog;h=refs/heads/xtensa
right?

It looks like you're breaking these rules with a lot of
the fields in your DisasContext. (Most obviously, you
need to translate code from tb->pc, not env->pc, and
xtensa_get_ring() and xtensa_get_cring() should not read
from env->sregs[PS]. But you should be clear for every
field in DisasContext which category it falls into.)

-- PMM

Re: [Qemu-devel] Actual TB code doesn't look like what was intended (TCG issue)?

[Max Filippov]

> Here are my rules of thumb for generating code where the code
> generated might change based on some bit of CPU state:
>
> When you are generating code, if the code you generate will
> change based on the contents of something in the CPUState struct,
> then the bit of CPUState you are looking at has to be one of:
>  (1) encoded in the TB flags (or tb->pc or tb->cs_base)
>      (and gen_intermediate_code_internal() must read and
>      use the value in tb->tb_flags, not the one in env)

So if changing a bit of context does not cause TB invalidation then it
must be captured in cpu_get_tb_cpu_state and
gen_intermediate_code_internal must use that captured value?

>  (2) always constant for the life of the simulation
>      (eg env->features if you have some sort of
>      "target feature support" flags)
>  (3) specially handled so that when it changes then
>      all TBs or at least all affected TBs are flushed
>      (env->breakpoints is in this category), and also
>      if the change is the result of some instruction then
>      you must terminate the TB after that instruction.
>      This is often used for things that rarely change and/or
>      where you can't fit the whole value into tb_flags.
> The reason for this is that the CPUState you're passed in
> is not guaranteed to be the state of the CPU at the PC
> which you are starting translation from.
>
> This is the xtensa port at
> http://jcmvbkbc.spb.ru/git/?p=dumb/qemu-xtensa.git;a=shortlog;h=refs/heads/xtensa
> right?

Yes.

> It looks like you're breaking these rules with a lot of
> the fields in your DisasContext. (Most obviously, you
> need to translate code from tb->pc, not env->pc, and
> xtensa_get_ring() and xtensa_get_cring() should not read
> from env->sregs[PS]. But you should be clear for every
> field in DisasContext which category it falls into.)

Thank you Peter. Will go rearrange that mess.

-- 
Thanks.
-- Max

Re: [Qemu-devel] Actual TB code doesn't look like what was intended (TCG issue)?

[Peter Maydell]

On 24 June 2011 11:08, Max Filippov <jcmvbkbc@gmail.com> wrote:
>> Here are my rules of thumb for generating code where the code
>> generated might change based on some bit of CPU state:
>>
>> When you are generating code, if the code you generate will
>> change based on the contents of something in the CPUState struct,
>> then the bit of CPUState you are looking at has to be one of:
>>  (1) encoded in the TB flags (or tb->pc or tb->cs_base)
>>      (and gen_intermediate_code_internal() must read and
>>      use the value in tb->tb_flags, not the one in env)
>
> So if changing a bit of context does not cause TB invalidation then it
> must be captured in cpu_get_tb_cpu_state and
> gen_intermediate_code_internal must use that captured value?

Yes. (The other option is to arrange to not change the code you
generate based on that bit of context, for instance you can generate
code which loads an env field and passes it to a helper function.)

-- PMM

Re: [Qemu-devel] Actual TB code doesn't look like what was intended (TCG issue)?

[Max Filippov]

> Here are my rules of thumb for generating code where the code
> generated might change based on some bit of CPU state:
> 
> When you are generating code, if the code you generate will
> change based on the contents of something in the CPUState struct,
> then the bit of CPUState you are looking at has to be one of:
>  (1) encoded in the TB flags (or tb->pc or tb->cs_base)
>       (and gen_intermediate_code_internal() must read and
>       use the value in tb->tb_flags, not the one in env)
>  (2) always constant for the life of the simulation
>       (eg env->features if you have some sort of
>       "target feature support" flags)
>  (3) specially handled so that when it changes then
>       all TBs or at least all affected TBs are flushed
>       (env->breakpoints is in this category), and also
>       if the change is the result of some instruction then
>       you must terminate the TB after that instruction.
>       This is often used for things that rarely change and/or
>       where you can't fit the whole value into tb_flags.
> 
> The reason for this is that the CPUState you're passed in
> is not guaranteed to be the state of the CPU at the PC
> which you are starting translation from.
> 
> This is the xtensa port at
> http://jcmvbkbc.spb.ru/git/?p=dumb/qemu-xtensa.git;a=shortlog;h=refs/heads/xtensa
> right?
> 
> It looks like you're breaking these rules with a lot of
> the fields in your DisasContext. (Most obviously, you
> need to translate code from tb->pc, not env->pc, and
> xtensa_get_ring() and xtensa_get_cring() should not read
> from env->sregs[PS]. But you should be clear for every
> field in DisasContext which category it falls into.)

Peter you are right, I've moved ring/cring into tb_flags and the issue has gone. Thank you.
I'd like to document it somewhere in http://wiki.qemu.org/Documentation, can you suggest a good place for it?

-- Max

Re: [Qemu-devel] Actual TB code doesn't look like what was intended (TCG issue)?

[Laurent Desnogues]

On Fri, Jun 24, 2011 at 4:44 AM, Max Filippov <jcmvbkbc@gmail.com> wrote:
> Hello guys.
>
> I'm running qemu on x86_64 host.
> It's clean build from git sources dated 2011.05.19, commit 1fddfba129f5435c80eda14e8bc23fdb888c7187
> I have the following output from "log trace,op,out_asm":
>
> Trace 0x4000a310 [d0026c92]
> OP:
>  ---- 0xd00000c0
>  movi_i32 tmp1,$0xfffffff4
>  add_i32 tmp0,ar9,tmp1
>  qemu_ld32 ar1,tmp0,$0x0
>
>  ---- 0xd00000c3
>  movi_i32 tmp1,$0xfffffff0
>  add_i32 tmp0,ar9,tmp1
>  qemu_ld32 ar0,tmp0,$0x0
>
> [...snip...]
[...]
> 0x4000a360:  xor    %esi,%esi
> 0x4000a362:  callq  0x52edc2
[...]
> (gdb) x/25i 0x4000a330
[...]
>   0x4000a360:  mov    $0x1,%esi
>   0x4000a365:  callq  0x52edc2 <__ldl_mmu>
>   0x4000a36a:  mov    %eax,%ebp
>   0x4000a36c:  sub    $0x44,%al
> => 0x4000a36e:  lea    -0x10(%rbx),%esp
>   0x4000a371:  mov    %ebp,0xc(%r14)
>   0x4000a375:  mov    %r12d,%esi
>   0x4000a378:  mov    %r12d,%edi
>
> Please note how the current instruction in gdb differ from what was said in OUT. This lea corrupts stack pointer and the next callq generates segfault.
> Could please anyone familiar with TCG take a look at this, or suggest where I should look myself?

As Peter hinted, you're not looking at the code you think :-)
Note how your original TCG code does loads:

   qemu_ld32 ar1,tmp0,$0x0

That $0x0 will end up in %RSI.  It's the mem index used to
distinguish from user and privileged level accesses.  In your
examples of host code, in one case it is 0 and in the other
it is 1, so you're definitely not really looking at the same
block in the same running conditions.

HTH,

Laurent

Re: [Qemu-devel] Actual TB code doesn't look like what was intended (TCG issue)?

[Max Filippov]

> > Hello guys.
> >
> > I'm running qemu on x86_64 host.
> > It's clean build from git sources dated 2011.05.19, commit 1fddfba129f5435c80eda14e8bc23fdb888c7187
> > I have the following output from "log trace,op,out_asm":
> >
> > Trace 0x4000a310 [d0026c92]
> > OP:
> >  ---- 0xd00000c0
> >  movi_i32 tmp1,$0xfffffff4
> >  add_i32 tmp0,ar9,tmp1
> >  qemu_ld32 ar1,tmp0,$0x0
> >
> >  ---- 0xd00000c3
> >  movi_i32 tmp1,$0xfffffff0
> >  add_i32 tmp0,ar9,tmp1
> >  qemu_ld32 ar0,tmp0,$0x0
> >
> > [...snip...]
> [...]
> > 0x4000a360:  xor    %esi,%esi
> > 0x4000a362:  callq  0x52edc2
> [...]
> > (gdb) x/25i 0x4000a330
> [...]
> >   0x4000a360:  mov    $0x1,%esi
> >   0x4000a365:  callq  0x52edc2 <__ldl_mmu>
> >   0x4000a36a:  mov    %eax,%ebp
> >   0x4000a36c:  sub    $0x44,%al
> > => 0x4000a36e:  lea    -0x10(%rbx),%esp
> >   0x4000a371:  mov    %ebp,0xc(%r14)
> >   0x4000a375:  mov    %r12d,%esi
> >   0x4000a378:  mov    %r12d,%edi
> >
> > Please note how the current instruction in gdb differ from what was said in OUT. This lea corrupts stack pointer and the next callq generates segfault.
> > Could please anyone familiar with TCG take a look at this, or suggest where I should look myself?
> 
> As Peter hinted, you're not looking at the code you think :-)
> Note how your original TCG code does loads:
> 
>    qemu_ld32 ar1,tmp0,$0x0
> 
> That $0x0 will end up in %RSI.  It's the mem index used to
> distinguish from user and privileged level accesses.  In your
> examples of host code, in one case it is 0 and in the other
> it is 1, so you're definitely not really looking at the same
> block in the same running conditions.

Yes, I've noticed it (however, after I sent this mail).
But (1) quoted OUT is the last OUT for this host address range in the log and (2) in gdb I set "b tlb_fill if retaddr == 0x4000a369" and made some steps.
You mean that I should look at previous OUTs for this address range?

Thanks.
-- Max

Re: [Qemu-devel] Actual TB code doesn't look like what was intended (TCG issue)?

[Laurent Desnogues]

On Fri, Jun 24, 2011 at 10:35 AM, Max Filippov <jcmvbkbc@gmail.com> wrote:
[...]
> Yes, I've noticed it (however, after I sent this mail).
> But (1) quoted OUT is the last OUT for this host address range in the log and (2) in gdb I set "b tlb_fill if retaddr == 0x4000a369" and made some steps.
> You mean that I should look at previous OUTs for this address range?

That should be the last block matching the address in the
log output if you run *under gdb* with -d out_asm.

BTW you say this is a clean build, but as far as I could see
it looks like your emulated target is not part of standard
QEMU;  it seems to have a register named 'ar9'.  Did I
miss something or is it some non public target? ia64?


Laurent

Re: [Qemu-devel] Actual TB code doesn't look like what was intended (TCG issue)?

[Max Filippov]

> That should be the last block matching the address in the
> log output if you run *under gdb* with -d out_asm.

This is the case.

> BTW you say this is a clean build, but as far as I could see
> it looks like your emulated target is not part of standard
> QEMU;  it seems to have a register named 'ar9'.  Did I
> miss something or is it some non public target? ia64?

Clean build w.r.t. TCG/other core code. The target is target-xtensa,
http://jcmvbkbc.spb.ru/git/?p=dumb/qemu-xtensa.git;a=shortlog;h=refs/heads/xtensa

-- 
Thanks.
-- Max