[Qemu-devel] Crash due to invalid env->current_tb

[Qemu-devel] Crash due to invalid env->current_tb

[Adam Lackorzynski]

Hi,

I've been experiencing crashes of latest svn Qemu, host ia32 and target
arm, host gcc is 'gcc version 3.4.6 (Debian 3.4.6-7)'.
The segfault happens because of an invalid env->current_tb which seems
to be caused by generated code. The following code in cpu_exec

   tc_ptr = tb->tc_ptr;
   env->current_tb = tb;
   gen_func = (void *)tc_ptr;
   T0 = gen_func();
   env->current_tb = NULL;

is being compiled to the following

   mov    0x14(%ecx),%eax
   mov    %ecx,0x56c(%ebp)
   xor    %edi,%edi
   call   *%eax
   mov    %edi,0x56c(%ebp)

After the call edi isn't 0 anymore and gets the bogus value. As edi is
callee saved the code itself seems ok.
When I add a barrier before "env->current_tb = NULL" the xor is placed
after the call and everything works fine. So might the problem be that
generated code isn't preserving edi/registers?



Adam
-- 
Adam                 adam@os.inf.tu-dresden.de
  Lackorzynski         http://os.inf.tu-dresden.de/~adam/

Re: [Qemu-devel] Crash due to invalid env->current_tb

[Blue Swirl]

[-- Attachment #1: Type: text/plain, Size: 1126 bytes --]

On 4/29/08, Adam Lackorzynski <adam@os.inf.tu-dresden.de> wrote:
> Hi,
>
>  I've been experiencing crashes of latest svn Qemu, host ia32 and target
>  arm, host gcc is 'gcc version 3.4.6 (Debian 3.4.6-7)'.
>  The segfault happens because of an invalid env->current_tb which seems
>  to be caused by generated code. The following code in cpu_exec
>
>    tc_ptr = tb->tc_ptr;
>    env->current_tb = tb;
>    gen_func = (void *)tc_ptr;
>    T0 = gen_func();
>    env->current_tb = NULL;
>
>  is being compiled to the following
>
>    mov    0x14(%ecx),%eax
>    mov    %ecx,0x56c(%ebp)
>    xor    %edi,%edi
>    call   *%eax
>    mov    %edi,0x56c(%ebp)
>
>  After the call edi isn't 0 anymore and gets the bogus value. As edi is
>  callee saved the code itself seems ok.
>  When I add a barrier before "env->current_tb = NULL" the xor is placed
>  after the call and everything works fine. So might the problem be that
>  generated code isn't preserving edi/registers?

Right. How did you make the barrier? My version (attached) just
crashes, I'm not fluent on i386 assembly. Maybe your version could
serve as a temporary fix.

[-- Attachment #2: fix_i386.diff --]
[-- Type: plain/text, Size: 586 bytes --]

Re: [Qemu-devel] Crash due to invalid env->current_tb

[Adam Lackorzynski]

On Tue Apr 29, 2008 at 20:09:00 +0300, Blue Swirl wrote:
> On 4/29/08, Adam Lackorzynski <adam@os.inf.tu-dresden.de> wrote:
> > Hi,
> >
> >  I've been experiencing crashes of latest svn Qemu, host ia32 and target
> >  arm, host gcc is 'gcc version 3.4.6 (Debian 3.4.6-7)'.
> >  The segfault happens because of an invalid env->current_tb which seems
> >  to be caused by generated code. The following code in cpu_exec
> >
> >    tc_ptr = tb->tc_ptr;
> >    env->current_tb = tb;
> >    gen_func = (void *)tc_ptr;
> >    T0 = gen_func();
> >    env->current_tb = NULL;
> >
> >  is being compiled to the following
> >
> >    mov    0x14(%ecx),%eax
> >    mov    %ecx,0x56c(%ebp)
> >    xor    %edi,%edi
> >    call   *%eax
> >    mov    %edi,0x56c(%ebp)
> >
> >  After the call edi isn't 0 anymore and gets the bogus value. As edi is
> >  callee saved the code itself seems ok.
> >  When I add a barrier before "env->current_tb = NULL" the xor is placed
> >  after the call and everything works fine. So might the problem be that
> >  generated code isn't preserving edi/registers?
> 
> Right. How did you make the barrier? My version (attached) just
> crashes, I'm not fluent on i386 assembly. Maybe your version could
> serve as a temporary fix.

I just added an 'asm volatile("")' to stop reordering of instructions
which of course isn't enough. The following works for me:

===================================================================
--- cpu-exec.c	(revision 4276)
+++ cpu-exec.c	(working copy)
@@ -690,6 +691,11 @@
 		fp.ip = tc_ptr;
 		fp.gp = code_gen_buffer + 2 * (1 << 20);
 		(*(void (*)(void)) &fp)();
+#elif defined(__i386)
+		asm volatile ("call *%1\n"
+		              : "=a" (T0)
+			      : "r" (gen_func)
+			      : "esi", "edi");
 #else
                 T0 = gen_func();
 #endif



Adam
-- 
Adam                 adam@os.inf.tu-dresden.de
  Lackorzynski         http://os.inf.tu-dresden.de/~adam/

Re: [Qemu-devel] Crash due to invalid env->current_tb

[Alexander Graf]

On Apr 29, 2008, at 8:40 PM, Adam Lackorzynski wrote:

>
> On Tue Apr 29, 2008 at 20:09:00 +0300, Blue Swirl wrote:
>> On 4/29/08, Adam Lackorzynski <adam@os.inf.tu-dresden.de> wrote:
>>> Hi,
>>>
>>> I've been experiencing crashes of latest svn Qemu, host ia32 and  
>>> target
>>> arm, host gcc is 'gcc version 3.4.6 (Debian 3.4.6-7)'.
>>> The segfault happens because of an invalid env->current_tb which  
>>> seems
>>> to be caused by generated code. The following code in cpu_exec
>>>
>>>   tc_ptr = tb->tc_ptr;
>>>   env->current_tb = tb;
>>>   gen_func = (void *)tc_ptr;
>>>   T0 = gen_func();
>>>   env->current_tb = NULL;
>>>
>>> is being compiled to the following
>>>
>>>   mov    0x14(%ecx),%eax
>>>   mov    %ecx,0x56c(%ebp)
>>>   xor    %edi,%edi
>>>   call   *%eax
>>>   mov    %edi,0x56c(%ebp)
>>>
>>> After the call edi isn't 0 anymore and gets the bogus value. As  
>>> edi is
>>> callee saved the code itself seems ok.
>>> When I add a barrier before "env->current_tb = NULL" the xor is  
>>> placed
>>> after the call and everything works fine. So might the problem be  
>>> that
>>> generated code isn't preserving edi/registers?
>>
>> Right. How did you make the barrier? My version (attached) just
>> crashes, I'm not fluent on i386 assembly. Maybe your version could
>> serve as a temporary fix.
>
> I just added an 'asm volatile("")' to stop reordering of instructions
> which of course isn't enough. The following works for me:
>
> ===================================================================
> --- cpu-exec.c	(revision 4276)
> +++ cpu-exec.c	(working copy)
> @@ -690,6 +691,11 @@
> 		fp.ip = tc_ptr;
> 		fp.gp = code_gen_buffer + 2 * (1 << 20);
> 		(*(void (*)(void)) &fp)();
> +#elif defined(__i386)
> +		asm volatile ("call *%1\n"
> +		              : "=a" (T0)
> +			      : "r" (gen_func)
> +			      : "esi", "edi");
> #else
>                 T0 = gen_func();
> #endif

There was a comment from Fabrice on how to do prologues in TCG to  
save / restore the clobbered values. Btw, ebx gets clobbered as well.

Alex

Re: [Qemu-devel] Crash due to invalid env->current_tb

[Adam Lackorzynski]

On Wed Apr 30, 2008 at 11:08:46 +0200, Alexander Graf wrote:
>
> On Apr 29, 2008, at 8:40 PM, Adam Lackorzynski wrote:
>
>>
>> On Tue Apr 29, 2008 at 20:09:00 +0300, Blue Swirl wrote:
>>> On 4/29/08, Adam Lackorzynski <adam@os.inf.tu-dresden.de> wrote:
>>>> Hi,
>>>>
>>>> I've been experiencing crashes of latest svn Qemu, host ia32 and  
>>>> target
>>>> arm, host gcc is 'gcc version 3.4.6 (Debian 3.4.6-7)'.
>>>> The segfault happens because of an invalid env->current_tb which  
>>>> seems
>>>> to be caused by generated code. The following code in cpu_exec
>>>>
>>>>   tc_ptr = tb->tc_ptr;
>>>>   env->current_tb = tb;
>>>>   gen_func = (void *)tc_ptr;
>>>>   T0 = gen_func();
>>>>   env->current_tb = NULL;
>>>>
>>>> is being compiled to the following
>>>>
>>>>   mov    0x14(%ecx),%eax
>>>>   mov    %ecx,0x56c(%ebp)
>>>>   xor    %edi,%edi
>>>>   call   *%eax
>>>>   mov    %edi,0x56c(%ebp)
>>>>
>>>> After the call edi isn't 0 anymore and gets the bogus value. As  
>>>> edi is
>>>> callee saved the code itself seems ok.
>>>> When I add a barrier before "env->current_tb = NULL" the xor is  
>>>> placed
>>>> after the call and everything works fine. So might the problem be  
>>>> that
>>>> generated code isn't preserving edi/registers?
>>>
>>> Right. How did you make the barrier? My version (attached) just
>>> crashes, I'm not fluent on i386 assembly. Maybe your version could
>>> serve as a temporary fix.
>>
>> I just added an 'asm volatile("")' to stop reordering of instructions
>> which of course isn't enough. The following works for me:
>>
>> ===================================================================
>> --- cpu-exec.c	(revision 4276)
>> +++ cpu-exec.c	(working copy)
>> @@ -690,6 +691,11 @@
>> 		fp.ip = tc_ptr;
>> 		fp.gp = code_gen_buffer + 2 * (1 << 20);
>> 		(*(void (*)(void)) &fp)();
>> +#elif defined(__i386)
>> +		asm volatile ("call *%1\n"
>> +		              : "=a" (T0)
>> +			      : "r" (gen_func)
>> +			      : "esi", "edi");
>> #else
>>                 T0 = gen_func();
>> #endif
>
> There was a comment from Fabrice on how to do prologues in TCG to save / 
> restore the clobbered values. Btw, ebx gets clobbered as well.

tcg/README says that some registers are clobbered. So something like
this should be safe:

Index: cpu-exec.c
===================================================================
--- cpu-exec.c	(revision 4276)
+++ cpu-exec.c	(working copy)
@@ -690,6 +691,15 @@
 		fp.ip = tc_ptr;
 		fp.gp = code_gen_buffer + 2 * (1 << 20);
 		(*(void (*)(void)) &fp)();
+#elif defined(__i386)
+		asm volatile ("push %%ebp\n"
+		              "push %%ebx\n"
+		              "call *%1\n"
+			      "pop %%ebx\n"
+			      "pop %%ebp\n"
+		              : "=a" (T0)
+			      : "r" (gen_func)
+			      : "esi", "edi", "ecx", "edx");
 #else
                 T0 = gen_func();
 #endif




Adam
-- 
Adam                 adam@os.inf.tu-dresden.de
  Lackorzynski         http://os.inf.tu-dresden.de/~adam/

Re: [Qemu-devel] Crash due to invalid env->current_tb

[Adam Lackorzynski]

On Wed Apr 30, 2008 at 17:11:32 +0200, Adam Lackorzynski wrote:
> On Wed Apr 30, 2008 at 11:08:46 +0200, Alexander Graf wrote:
> > There was a comment from Fabrice on how to do prologues in TCG to save / 
> > restore the clobbered values. Btw, ebx gets clobbered as well.
> 
> tcg/README says that some registers are clobbered. So something like
> this should be safe:
> 
> Index: cpu-exec.c
> ===================================================================
> --- cpu-exec.c	(revision 4276)
> +++ cpu-exec.c	(working copy)
> @@ -690,6 +691,15 @@
>  		fp.ip = tc_ptr;
>  		fp.gp = code_gen_buffer + 2 * (1 << 20);
>  		(*(void (*)(void)) &fp)();
> +#elif defined(__i386)
> +		asm volatile ("push %%ebp\n"
> +		              "push %%ebx\n"
> +		              "call *%1\n"
> +			      "pop %%ebx\n"
> +			      "pop %%ebp\n"
> +		              : "=a" (T0)
> +			      : "r" (gen_func)
> +			      : "esi", "edi", "ecx", "edx");
>  #else
>                  T0 = gen_func();
>  #endif

I just realised that the push and pop of ebx is not needed as T0 is ebx
which gets overwritten in the output anyway.

Index: cpu-exec.c
===================================================================
--- cpu-exec.c	(revision 4276)
+++ cpu-exec.c	(working copy)
@@ -690,6 +691,13 @@
 		fp.ip = tc_ptr;
 		fp.gp = code_gen_buffer + 2 * (1 << 20);
 		(*(void (*)(void)) &fp)();
+#elif defined(__i386)
+		asm volatile ("push %%ebp\n"
+		              "call *%1\n"
+			      "pop %%ebp\n"
+		              : "=a" (T0)
+			      : "r" (gen_func)
+			      : "esi", "edi", "ecx", "edx");
 #else
                 T0 = gen_func();
 #endif


Adam
-- 
Adam                 adam@os.inf.tu-dresden.de
  Lackorzynski         http://os.inf.tu-dresden.de/~adam/

Re: [Qemu-devel] Crash due to invalid env->current_tb

[Blue Swirl]

On 4/30/08, Adam Lackorzynski <adam@os.inf.tu-dresden.de> wrote:
>
>  On Wed Apr 30, 2008 at 17:11:32 +0200, Adam Lackorzynski wrote:
>  > On Wed Apr 30, 2008 at 11:08:46 +0200, Alexander Graf wrote:
>
> > > There was a comment from Fabrice on how to do prologues in TCG to save /
>  > > restore the clobbered values. Btw, ebx gets clobbered as well.
>  >
>  > tcg/README says that some registers are clobbered. So something like
>  > this should be safe:
>  >
>  > Index: cpu-exec.c
>  > ===================================================================
>  > --- cpu-exec.c        (revision 4276)
>  > +++ cpu-exec.c        (working copy)
>  > @@ -690,6 +691,15 @@
>  >               fp.ip = tc_ptr;
>  >               fp.gp = code_gen_buffer + 2 * (1 << 20);
>  >               (*(void (*)(void)) &fp)();
>  > +#elif defined(__i386)
>  > +             asm volatile ("push %%ebp\n"
>  > +                           "push %%ebx\n"
>  > +                           "call *%1\n"
>  > +                           "pop %%ebx\n"
>  > +                           "pop %%ebp\n"
>  > +                           : "=a" (T0)
>  > +                           : "r" (gen_func)
>  > +                           : "esi", "edi", "ecx", "edx");
>  >  #else
>  >                  T0 = gen_func();
>  >  #endif
>
>
> I just realised that the push and pop of ebx is not needed as T0 is ebx
>  which gets overwritten in the output anyway.

Sparc32 compiles, but for sparc64-softmmu target, I get compiler errors:
/src/qemu/cpu-exec.c: In function `cpu_sparc_exec':
/src/qemu/cpu-exec.c:694: error: impossible register constraint in `asm'
/src/qemu/cpu-exec.c:694: error: can't find a register in class
`ALL_REGS' while reloading `asm'

Re: [Qemu-devel] Crash due to invalid env->current_tb

[Alexander Graf]

On Apr 30, 2008, at 5:21 PM, Adam Lackorzynski wrote:

>
> On Wed Apr 30, 2008 at 17:11:32 +0200, Adam Lackorzynski wrote:
>> On Wed Apr 30, 2008 at 11:08:46 +0200, Alexander Graf wrote:
>>> There was a comment from Fabrice on how to do prologues in TCG to  
>>> save /
>>> restore the clobbered values. Btw, ebx gets clobbered as well.
>>
>> tcg/README says that some registers are clobbered. So something like
>> this should be safe:
>>
>> Index: cpu-exec.c
>> ===================================================================
>> --- cpu-exec.c	(revision 4276)
>> +++ cpu-exec.c	(working copy)
>> @@ -690,6 +691,15 @@
>> 		fp.ip = tc_ptr;
>> 		fp.gp = code_gen_buffer + 2 * (1 << 20);
>> 		(*(void (*)(void)) &fp)();
>> +#elif defined(__i386)
>> +		asm volatile ("push %%ebp\n"
>> +		              "push %%ebx\n"
>> +		              "call *%1\n"
>> +			      "pop %%ebx\n"
>> +			      "pop %%ebp\n"
>> +		              : "=a" (T0)
>> +			      : "r" (gen_func)
>> +			      : "esi", "edi", "ecx", "edx");
>> #else
>>                 T0 = gen_func();
>> #endif
>
> I just realised that the push and pop of ebx is not needed as T0 is  
> ebx
> which gets overwritten in the output anyway.

Why is T0 =a then? Shouldn't =a mean "input and output on eax for T0"?

>
>
> Index: cpu-exec.c
> ===================================================================
> --- cpu-exec.c	(revision 4276)
> +++ cpu-exec.c	(working copy)
> @@ -690,6 +691,13 @@
> 		fp.ip = tc_ptr;
> 		fp.gp = code_gen_buffer + 2 * (1 << 20);
> 		(*(void (*)(void)) &fp)();
> +#elif defined(__i386)
> +		asm volatile ("push %%ebp\n"
> +		              "call *%1\n"
> +			      "pop %%ebp\n"
> +		              : "=a" (T0)
> +			      : "r" (gen_func)
> +			      : "esi", "edi", "ecx", "edx");
> #else
>                 T0 = gen_func();
> #endif
>
>
> Adam
> -- 
> Adam                 adam@os.inf.tu-dresden.de
>  Lackorzynski         http://os.inf.tu-dresden.de/~adam/
>
>

Re: [Qemu-devel] Crash due to invalid env->current_tb

[Blue Swirl]

[-- Attachment #1: Type: text/plain, Size: 1841 bytes --]

On 4/30/08, Alexander Graf <agraf@suse.de> wrote:
>
>  On Apr 30, 2008, at 5:21 PM, Adam Lackorzynski wrote:
>
>
> >
> > On Wed Apr 30, 2008 at 17:11:32 +0200, Adam Lackorzynski wrote:
> >
> > > On Wed Apr 30, 2008 at 11:08:46 +0200, Alexander Graf wrote:
> > >
> > > > There was a comment from Fabrice on how to do prologues in TCG to save
> /
> > > > restore the clobbered values. Btw, ebx gets clobbered as well.
> > > >
> > >
> > > tcg/README says that some registers are clobbered. So something like
> > > this should be safe:
> > >
> > > Index: cpu-exec.c
> > >
> ===================================================================
> > > --- cpu-exec.c  (revision 4276)
> > > +++ cpu-exec.c  (working copy)
> > > @@ -690,6 +691,15 @@
> > >                fp.ip = tc_ptr;
> > >                fp.gp = code_gen_buffer + 2 * (1 << 20);
> > >                (*(void (*)(void)) &fp)();
> > > +#elif defined(__i386)
> > > +               asm volatile ("push %%ebp\n"
> > > +                             "push %%ebx\n"
> > > +                             "call *%1\n"
> > > +                             "pop %%ebx\n"
> > > +                             "pop %%ebp\n"
> > > +                             : "=a" (T0)
> > > +                             : "r" (gen_func)
> > > +                             : "esi", "edi", "ecx", "edx");
> > > #else
> > >                T0 = gen_func();
> > > #endif
> > >
> >
> > I just realised that the push and pop of ebx is not needed as T0 is ebx
> > which gets overwritten in the output anyway.
> >
>
>  Why is T0 =a then? Shouldn't =a mean "input and output on eax for T0"?

GCC-Inline-Assembly-HOWTO:
"=" : Means that this operand is write-only for this instruction; the
previous value is discarded and replaced by output data.

The attached version survives quick tests for both Sparc32 and Sparc64.

[-- Attachment #2: fix_i386.diff --]
[-- Type: plain/text, Size: 983 bytes --]

Re: [Qemu-devel] Crash due to invalid env->current_tb

[Adam Lackorzynski]

On Wed Apr 30, 2008 at 21:21:40 +0300, Blue Swirl wrote:
> The attached version survives quick tests for both Sparc32 and Sparc64.

Ok, I did not check for 64bit targets. So what about the following,
works for me for arm, x86-32 and x86-64.

Index: cpu-exec.c
===================================================================
--- cpu-exec.c	(revision 4276)
+++ cpu-exec.c	(working copy)
@@ -690,7 +691,22 @@
 		fp.ip = tc_ptr;
 		fp.gp = code_gen_buffer + 2 * (1 << 20);
 		(*(void (*)(void)) &fp)();
+#elif defined(__i386)
+#if (TARGET_LONG_BITS == 32)
+#define CLOBBER ,"edx"
 #else
+#define CLOBBER ,"ebx"
+#endif
+		asm volatile ("push %%ebp\n"
+		              "call *%1\n"
+			      "pop %%ebp\n"
+		              : "=A" (T0)
+			      : "a" (gen_func)
+			      : "ecx", "esi", "edi", "cc" CLOBBER
+			      );
+		T0 &= 0xffffffff;
+#undef CLOBBER
+#else
                 T0 = gen_func();
 #endif
                 env->current_tb = NULL;


For 64bit target T0 is 64bits so "=a" does not work and "=A" is needed.
The strange thing is that I need to throw away the upper 32bits because
otherwise it won't work. gen_func is defined to return just long but T0
is unsigned long long, this seems inconsistent. The 'and' does not
appear in 32bit targets so it does not harm there.



Adam
-- 
Adam                 adam@os.inf.tu-dresden.de
  Lackorzynski         http://os.inf.tu-dresden.de/~adam/

Re: [Qemu-devel] Crash due to invalid env->current_tb

[Blue Swirl]

On 5/1/08, Adam Lackorzynski <adam@os.inf.tu-dresden.de> wrote:
>
>  On Wed Apr 30, 2008 at 21:21:40 +0300, Blue Swirl wrote:
>  > The attached version survives quick tests for both Sparc32 and Sparc64.
>
>
> Ok, I did not check for 64bit targets. So what about the following,
>  works for me for arm, x86-32 and x86-64.
>
>
>  Index: cpu-exec.c
>  ===================================================================
>  --- cpu-exec.c  (revision 4276)
>  +++ cpu-exec.c  (working copy)
>
> @@ -690,7 +691,22 @@
>
>                 fp.ip = tc_ptr;
>                 fp.gp = code_gen_buffer + 2 * (1 << 20);
>                 (*(void (*)(void)) &fp)();
>  +#elif defined(__i386)
>
> +#if (TARGET_LONG_BITS == 32)
>  +#define CLOBBER ,"edx"
>   #else
>  +#define CLOBBER ,"ebx"
>  +#endif
>  +               asm volatile ("push %%ebp\n"
>  +                             "call *%1\n"
>  +                             "pop %%ebp\n"
>  +                             : "=A" (T0)
>  +                             : "a" (gen_func)
>  +                             : "ecx", "esi", "edi", "cc" CLOBBER
>  +                             );
>  +               T0 &= 0xffffffff;
>  +#undef CLOBBER
>  +#else
>                  T0 = gen_func();
>   #endif
>                  env->current_tb = NULL;
>
>
>  For 64bit target T0 is 64bits so "=a" does not work and "=A" is needed.
>  The strange thing is that I need to throw away the upper 32bits because
>  otherwise it won't work. gen_func is defined to return just long but T0
>  is unsigned long long, this seems inconsistent. The 'and' does not
>  appear in 32bit targets so it does not harm there.

This is because in this special case, T0 is not used as target CPU
temporary, but instead to return next TB address. On i386 this is 32
bits, so only EAX is needed. TCG does not touch EDX, so it contains
garbage. This also means that moving EDX to high word of T0 and then
throwing the high word away may be slightly wasteful.

Re: [Qemu-devel] Crash due to invalid env->current_tb

[Paul Brook]

> >                  T0 = gen_func();
> >
> >  For 64bit target T0 is 64bits so "=a" does not work and "=A" is needed.
> >  The strange thing is that I need to throw away the upper 32bits because
> >  otherwise it won't work. gen_func is defined to return just long but T0
> >  is unsigned long long, this seems inconsistent. The 'and' does not
> >  appear in 32bit targets so it does not harm there.
>
> This is because in this special case, T0 is not used as target CPU
> temporary, but instead to return next TB address. On i386 this is 32
> bits, so only EAX is needed. TCG does not touch EDX, so it contains
> garbage. This also means that moving EDX to high word of T0 and then
> throwing the high word away may be slightly wasteful.

Do we need to use T0 at all here? Can't we just use a normal local variable?

Paul

Re: [Qemu-devel] Crash due to invalid env->current_tb

[Blue Swirl]

On 5/1/08, Paul Brook <paul@codesourcery.com> wrote:
> > >                  T0 = gen_func();
>
> > >
>  > >  For 64bit target T0 is 64bits so "=a" does not work and "=A" is needed.
>  > >  The strange thing is that I need to throw away the upper 32bits because
>  > >  otherwise it won't work. gen_func is defined to return just long but T0
>  > >  is unsigned long long, this seems inconsistent. The 'and' does not
>  > >  appear in 32bit targets so it does not harm there.
>  >
>  > This is because in this special case, T0 is not used as target CPU
>  > temporary, but instead to return next TB address. On i386 this is 32
>  > bits, so only EAX is needed. TCG does not touch EDX, so it contains
>  > garbage. This also means that moving EDX to high word of T0 and then
>  > throwing the high word away may be slightly wasteful.
>
>
> Do we need to use T0 at all here? Can't we just use a normal local variable?

I suspect T0 was used to gain extra performance, but in case of 64-bit
target on 32-bit host there is this unnecessary work.

But does cpu-exec.c need to know about T0/T1/T2 at all? Can we replace
exec.h include with cpu.h one?

Re: [Qemu-devel] Crash due to invalid env->current_tb

[Paul Brook]

On Thursday 01 May 2008, Blue Swirl wrote:
> On 5/1/08, Paul Brook <paul@codesourcery.com> wrote:
> > > >                  T0 = gen_func();
> > > >
> >  > >  For 64bit target T0 is 64bits so "=a" does not work and "=A" is
> >  > > needed. The strange thing is that I need to throw away the upper
> >  > > 32bits because otherwise it won't work. gen_func is defined to
> >  > > return just long but T0 is unsigned long long, this seems
> >  > > inconsistent. The 'and' does not appear in 32bit targets so it does
> >  > > not harm there.
> >  >
> >  > This is because in this special case, T0 is not used as target CPU
> >  > temporary, but instead to return next TB address. On i386 this is 32
> >  > bits, so only EAX is needed. TCG does not touch EDX, so it contains
> >  > garbage. This also means that moving EDX to high word of T0 and then
> >  > throwing the high word away may be slightly wasteful.
> >
> > Do we need to use T0 at all here? Can't we just use a normal local
> > variable?
>
> I suspect T0 was used to gain extra performance, 

Really? I doubt it. Especially on x86 reserving a register for a fixed purpose 
is almost always a bad idea.

It was used with dyngen because there was no way of directly passing 
information between cpu_loop and generated code. However now we have a proper 
code generator there is no need for this.

> but in case of 64-bit  target on 32-bit host there is this unnecessary work.
>
> But does cpu-exec.c need to know about T0/T1/T2 at all?

I don't think so.

> Can we replace exec.h include with cpu.h one?

Currently we still need it to setup "env". However once we have proper TCG 
prologue/epilogue we should be able to pass env as an argument to gen_func, 
and have that do the setup for us.

Paul

Re: [Qemu-devel] Crash due to invalid env->current_tb

[Adam Lackorzynski]

On Thu May 01, 2008 at 18:02:46 +0300, Blue Swirl wrote:
> On 5/1/08, Adam Lackorzynski <adam@os.inf.tu-dresden.de> wrote:
> >  For 64bit target T0 is 64bits so "=a" does not work and "=A" is needed.
> >  The strange thing is that I need to throw away the upper 32bits because
> >  otherwise it won't work. gen_func is defined to return just long but T0
> >  is unsigned long long, this seems inconsistent. The 'and' does not
> >  appear in 32bit targets so it does not harm there.
> 
> This is because in this special case, T0 is not used as target CPU
> temporary, but instead to return next TB address. On i386 this is 32
> bits, so only EAX is needed. TCG does not touch EDX, so it contains
> garbage. This also means that moving EDX to high word of T0 and then
> throwing the high word away may be slightly wasteful.

So I played a bit more with this by trying out the 'and' and the tmp
variable approaches. With the tmp variables the generated code looks ok
whereas with the 'and' approach it looks especially scary with gcc-4.3
(gcc-3.4 looks ok). I have two versions now, one condensed and ugly and
then one with separate parts for 32 and 64 targets. I think this one
should be prefered.


Index: cpu-exec.c
===================================================================
--- cpu-exec.c	(revision 4291)
+++ cpu-exec.c	(working copy)
@@ -690,7 +691,25 @@
 		fp.ip = tc_ptr;
 		fp.gp = code_gen_buffer + 2 * (1 << 20);
 		(*(void (*)(void)) &fp)();
+#elif defined(__i386)
+#if (TARGET_LONG_BITS == 32)
+		asm volatile ("push %%ebp\n"
+		              "call *%1\n"
+			      "pop %%ebp\n"
+		              : "=a" (T0)
+			      : "a" (gen_func)
+			      : "ecx", "esi", "edi", "edx", "cc");
 #else
+		unsigned long tmp;
+		asm volatile ("push %%ebp\n"
+		              "call *%1\n"
+			      "pop %%ebp\n"
+		              : "=a" (tmp)
+			      : "a" (gen_func)
+			      : "ebx", "ecx", "esi", "edi", "edx", "cc");
+		T0 = tmp;
+#endif
+#else
                 T0 = gen_func();
 #endif
                 env->current_tb = NULL;



Index: cpu-exec.c
===================================================================
--- cpu-exec.c	(revision 4291)
+++ cpu-exec.c	(working copy)
@@ -690,7 +691,31 @@
 		fp.ip = tc_ptr;
 		fp.gp = code_gen_buffer + 2 * (1 << 20);
 		(*(void (*)(void)) &fp)();
+#elif defined(__i386)
+#if (TARGET_LONG_BITS == 32)
+#define CLOBBER
+#define OUTPUT T0
+#define OP
+#define OUTPUT2
 #else
+#define CLOBBER  ,"ebx"
+#define OUTPUT   *((unsigned long *)&T0)
+#define OUTPUT2  , [upperT0] "=m" (*((unsigned long *)&T0 + 1))
+#define OP       "movl $0, %[upperT0]\n"
+#endif
+		asm volatile ("push %%ebp\n"
+		              "call *%[func]\n"
+			      "pop %%ebp\n"
+			      OP
+		              : "=a" (OUTPUT) OUTPUT2
+			      : [func] "a" (gen_func)
+			      : "ecx", "esi", "edi", "edx", "cc" CLOBBER
+			      );
+#undef CLOBBER
+#undef OUTPUT
+#undef OUTPUT2
+#undef OP
+#else
                 T0 = gen_func();
 #endif
                 env->current_tb = NULL;



Adam
-- 
Adam                 adam@os.inf.tu-dresden.de
  Lackorzynski         http://os.inf.tu-dresden.de/~adam/

Re: [Qemu-devel] Crash due to invalid env->current_tb

[Blue Swirl]

[-- Attachment #1: Type: text/plain, Size: 1478 bytes --]

On 5/2/08, Adam Lackorzynski <adam@os.inf.tu-dresden.de> wrote:
>
>  On Thu May 01, 2008 at 18:02:46 +0300, Blue Swirl wrote:
>  > On 5/1/08, Adam Lackorzynski <adam@os.inf.tu-dresden.de> wrote:
>
> > >  For 64bit target T0 is 64bits so "=a" does not work and "=A" is needed.
>  > >  The strange thing is that I need to throw away the upper 32bits because
>  > >  otherwise it won't work. gen_func is defined to return just long but T0
>  > >  is unsigned long long, this seems inconsistent. The 'and' does not
>  > >  appear in 32bit targets so it does not harm there.
>  >
>  > This is because in this special case, T0 is not used as target CPU
>  > temporary, but instead to return next TB address. On i386 this is 32
>  > bits, so only EAX is needed. TCG does not touch EDX, so it contains
>  > garbage. This also means that moving EDX to high word of T0 and then
>  > throwing the high word away may be slightly wasteful.
>
>
> So I played a bit more with this by trying out the 'and' and the tmp
>  variable approaches. With the tmp variables the generated code looks ok
>  whereas with the 'and' approach it looks especially scary with gcc-4.3
>  (gcc-3.4 looks ok). I have two versions now, one condensed and ugly and
>  then one with separate parts for 32 and 64 targets. I think this one
>  should be prefered.

I made a new version that does not use T0 at all. Tested on i386 and
AMD64, both Sparc32 and Sparc64 work. AMD64 asm version does not seem
to be necessary.

[-- Attachment #2: cpu_exec_no_T0.diff --]
[-- Type: plain/text, Size: 8920 bytes --]

Re: [Qemu-devel] Crash due to invalid env->current_tb

[Paul Brook]

> I made a new version that does not use T0 at all. Tested on i386 and
> AMD64, both Sparc32 and Sparc64 work. AMD64 asm version does not seem
> to be necessary.

I guess amd64 is currently working by luck rather than by design.

You're pushing a single word to the stack, which could cause issues with stack 
alignment. I suggest:

sub $12,%%esp
push %%ebp
call *%1
pop %%ebp
add $12,%%esp

Likewise for amd64 you want sub $8, %%rsp, etc.

> +                              :  "ebx", "ecx", "edx", "esi", "edi", "cc");

You also want to add "memory" here.

>  #else
> -                T0 = gen_func();
> +                next_tb = gen_func();

I'd just make this a #error. Other host are likely to need special 
consideration anyway.

Other than that, looks ok to me.

Paul

Re: [Qemu-devel] Crash due to invalid env->current_tb

[Laurent Vivier]

Le mercredi 30 avril 2008 à 17:11 +0200, Adam Lackorzynski a écrit :
> On Wed Apr 30, 2008 at 11:08:46 +0200, Alexander Graf wrote:
> >
> > On Apr 29, 2008, at 8:40 PM, Adam Lackorzynski wrote:
> >
> >>
> >> On Tue Apr 29, 2008 at 20:09:00 +0300, Blue Swirl wrote:
> >>> On 4/29/08, Adam Lackorzynski <adam@os.inf.tu-dresden.de> wrote:
> >>>> Hi,
> >>>>
> >>>> I've been experiencing crashes of latest svn Qemu, host ia32 and  
> >>>> target
> >>>> arm, host gcc is 'gcc version 3.4.6 (Debian 3.4.6-7)'.
> >>>> The segfault happens because of an invalid env->current_tb which  
> >>>> seems
> >>>> to be caused by generated code. The following code in cpu_exec
> >>>>
> >>>>   tc_ptr = tb->tc_ptr;
> >>>>   env->current_tb = tb;
> >>>>   gen_func = (void *)tc_ptr;
> >>>>   T0 = gen_func();
> >>>>   env->current_tb = NULL;
> >>>>
> >>>> is being compiled to the following
> >>>>
> >>>>   mov    0x14(%ecx),%eax
> >>>>   mov    %ecx,0x56c(%ebp)
> >>>>   xor    %edi,%edi
> >>>>   call   *%eax
> >>>>   mov    %edi,0x56c(%ebp)
> >>>>
> >>>> After the call edi isn't 0 anymore and gets the bogus value. As  
> >>>> edi is
> >>>> callee saved the code itself seems ok.
> >>>> When I add a barrier before "env->current_tb = NULL" the xor is  
> >>>> placed
> >>>> after the call and everything works fine. So might the problem be  
> >>>> that
> >>>> generated code isn't preserving edi/registers?
> >>>
> >>> Right. How did you make the barrier? My version (attached) just
> >>> crashes, I'm not fluent on i386 assembly. Maybe your version could
> >>> serve as a temporary fix.
> >>
> >> I just added an 'asm volatile("")' to stop reordering of instructions
> >> which of course isn't enough. The following works for me:
> >>
> >> ===================================================================
> >> --- cpu-exec.c	(revision 4276)
> >> +++ cpu-exec.c	(working copy)
> >> @@ -690,6 +691,11 @@
> >> 		fp.ip = tc_ptr;
> >> 		fp.gp = code_gen_buffer + 2 * (1 << 20);
> >> 		(*(void (*)(void)) &fp)();
> >> +#elif defined(__i386)
> >> +		asm volatile ("call *%1\n"
> >> +		              : "=a" (T0)
> >> +			      : "r" (gen_func)
> >> +			      : "esi", "edi");
> >> #else
> >>                 T0 = gen_func();
> >> #endif
> >
> > There was a comment from Fabrice on how to do prologues in TCG to save / 
> > restore the clobbered values. Btw, ebx gets clobbered as well.
> 
> tcg/README says that some registers are clobbered. So something like
> this should be safe:
> 
> Index: cpu-exec.c
> ===================================================================
> --- cpu-exec.c	(revision 4276)
> +++ cpu-exec.c	(working copy)
> @@ -690,6 +691,15 @@
>  		fp.ip = tc_ptr;
>  		fp.gp = code_gen_buffer + 2 * (1 << 20);
>  		(*(void (*)(void)) &fp)();
> +#elif defined(__i386)
> +		asm volatile ("push %%ebp\n"
> +		              "push %%ebx\n"
> +		              "call *%1\n"
> +			      "pop %%ebx\n"
> +			      "pop %%ebp\n"
> +		              : "=a" (T0)
> +			      : "r" (gen_func)
> +			      : "esi", "edi", "ecx", "edx");

Why don't you add ebp and ebx in the clobbered registers list (like
"esi", "edi", "ecx", "edx") ?

>  #else
>                  T0 = gen_func();
>  #endif
> 
> 
> 
> 
> Adam
-- 
------------- Laurent.Vivier@bull.net ---------------
"The best way to predict the future is to invent it."
- Alan Kay

Re: [Qemu-devel] Crash due to invalid env->current_tb

[Adam Lackorzynski]

On Wed Apr 30, 2008 at 17:28:04 +0200, Laurent Vivier wrote:
> 
> Le mercredi 30 avril 2008 à 17:11 +0200, Adam Lackorzynski a écrit :
> > On Wed Apr 30, 2008 at 11:08:46 +0200, Alexander Graf wrote:
> > >
> > > On Apr 29, 2008, at 8:40 PM, Adam Lackorzynski wrote:
> > >
> > >>
> > >> On Tue Apr 29, 2008 at 20:09:00 +0300, Blue Swirl wrote:
> > >>> On 4/29/08, Adam Lackorzynski <adam@os.inf.tu-dresden.de> wrote:
> > >>>> Hi,
> > >>>>
> > >>>> I've been experiencing crashes of latest svn Qemu, host ia32 and  
> > >>>> target
> > >>>> arm, host gcc is 'gcc version 3.4.6 (Debian 3.4.6-7)'.
> > >>>> The segfault happens because of an invalid env->current_tb which  
> > >>>> seems
> > >>>> to be caused by generated code. The following code in cpu_exec
> > >>>>
> > >>>>   tc_ptr = tb->tc_ptr;
> > >>>>   env->current_tb = tb;
> > >>>>   gen_func = (void *)tc_ptr;
> > >>>>   T0 = gen_func();
> > >>>>   env->current_tb = NULL;
> > >>>>
> > >>>> is being compiled to the following
> > >>>>
> > >>>>   mov    0x14(%ecx),%eax
> > >>>>   mov    %ecx,0x56c(%ebp)
> > >>>>   xor    %edi,%edi
> > >>>>   call   *%eax
> > >>>>   mov    %edi,0x56c(%ebp)
> > >>>>
> > >>>> After the call edi isn't 0 anymore and gets the bogus value. As  
> > >>>> edi is
> > >>>> callee saved the code itself seems ok.
> > >>>> When I add a barrier before "env->current_tb = NULL" the xor is  
> > >>>> placed
> > >>>> after the call and everything works fine. So might the problem be  
> > >>>> that
> > >>>> generated code isn't preserving edi/registers?
> > >>>
> > >>> Right. How did you make the barrier? My version (attached) just
> > >>> crashes, I'm not fluent on i386 assembly. Maybe your version could
> > >>> serve as a temporary fix.
> > >>
> > >> I just added an 'asm volatile("")' to stop reordering of instructions
> > >> which of course isn't enough. The following works for me:
> > >>
> > >> ===================================================================
> > >> --- cpu-exec.c	(revision 4276)
> > >> +++ cpu-exec.c	(working copy)
> > >> @@ -690,6 +691,11 @@
> > >> 		fp.ip = tc_ptr;
> > >> 		fp.gp = code_gen_buffer + 2 * (1 << 20);
> > >> 		(*(void (*)(void)) &fp)();
> > >> +#elif defined(__i386)
> > >> +		asm volatile ("call *%1\n"
> > >> +		              : "=a" (T0)
> > >> +			      : "r" (gen_func)
> > >> +			      : "esi", "edi");
> > >> #else
> > >>                 T0 = gen_func();
> > >> #endif
> > >
> > > There was a comment from Fabrice on how to do prologues in TCG to save / 
> > > restore the clobbered values. Btw, ebx gets clobbered as well.
> > 
> > tcg/README says that some registers are clobbered. So something like
> > this should be safe:
> > 
> > Index: cpu-exec.c
> > ===================================================================
> > --- cpu-exec.c	(revision 4276)
> > +++ cpu-exec.c	(working copy)
> > @@ -690,6 +691,15 @@
> >  		fp.ip = tc_ptr;
> >  		fp.gp = code_gen_buffer + 2 * (1 << 20);
> >  		(*(void (*)(void)) &fp)();
> > +#elif defined(__i386)
> > +		asm volatile ("push %%ebp\n"
> > +		              "push %%ebx\n"
> > +		              "call *%1\n"
> > +			      "pop %%ebx\n"
> > +			      "pop %%ebp\n"
> > +		              : "=a" (T0)
> > +			      : "r" (gen_func)
> > +			      : "esi", "edi", "ecx", "edx");
> 
> Why don't you add ebp and ebx in the clobbered registers list (like
> "esi", "edi", "ecx", "edx") ?

For ebp it's more safe to use push as it depends whether the binary is
compiled with frame-pointer or without. When without you can put it into
the clobber list, when with you should not, we had some bad experience
with this (also see gcc bugzilla #11807).
T0 is register defined to ebx, so it's not needed (the push/pop is also
not needed), and also it does not work, gcc complains.



Adam
-- 
Adam                 adam@os.inf.tu-dresden.de
  Lackorzynski         http://os.inf.tu-dresden.de/~adam/

Re: [Qemu-devel] Crash due to invalid env->current_tb

[Bernhard Kauer]

On Wed, Apr 30, 2008 at 10:36:36PM +0200, Adam Lackorzynski wrote:
> > Why don't you add ebp and ebx in the clobbered registers list (like
> > "esi", "edi", "ecx", "edx") ?
> 
> For ebp it's more safe to use push as it depends whether the binary is
> compiled with frame-pointer or without. When without you can put it into
> the clobber list, when with you should not, we had some bad experience
> with this (also see gcc bugzilla #11807).

Well the gcc bug is, that clobbering "ebp" is silently ignored if compiled
without frame-pointer.


	Bernhard Kauer