[Qemu-devel] strange crash on FreeBSD-current/amd64 (pointer truncation?)

[Qemu-devel] strange crash on FreeBSD-current/amd64 (pointer truncation?)

[Juergen Lock]

Hi!

 I got a report of qemu segfaulting here on FreeBSD-current/amd64:

> #0  main_loop () at /usr/ports-cvs/emulators/qemu/work/qemu-snapshot-2007-01-11_05/vl.c:6125
> 6125                    env = env->next_cpu;
> [New Thread 0x801e10190 (LWP 100214)]
> (gdb) print env
> $1 = (CPUX86State *) 0xac10000
> (gdb) print first_cpu
> $2 = (CPUX86State *) 0x80ac10000

 This works on earlier FreeBSD branches, and the build logs,
	http://pointyhat.freebsd.org/errorlogs/amd64-7-latest-logs/qemu-0.8.2s.20070111.log
	http://pointyhat.freebsd.org/errorlogs/amd64-6-latest-logs/qemu-0.8.2s.20070111.log
(7 is the -current branch and 6 is the branch that the latest release,
6.2, was branched off) show no differences in compiler warnings, so
I'm a bit at a loss where to look for the problem.  So the qeustion is:
Has anyone seen something like this or has a helpful idea?

 Thanx,
	Juergen

[Qemu-devel] Re: strange crash on FreeBSD-current/amd64 (pointer truncation?)

[Juergen Lock]

On Wed, Jan 24, 2007 at 09:00:19PM +0100, Juergen Lock wrote:
> Hi!
> 
>  I got a report of qemu segfaulting here on FreeBSD-current/amd64:
> 
> > #0  main_loop () at /usr/ports-cvs/emulators/qemu/work/qemu-snapshot-2007-01-11_05/vl.c:6125
> > 6125                    env = env->next_cpu;
> > [New Thread 0x801e10190 (LWP 100214)]
> > (gdb) print env
> > $1 = (CPUX86State *) 0xac10000
> > (gdb) print first_cpu
> > $2 = (CPUX86State *) 0x80ac10000

Ok Jung-uk Kim found the following fix: (Thanx!)

--- qemu/cpu-exec.c.orig	Wed Jan 31 16:58:03 2007
+++ qemu/cpu-exec.c	Wed Jan 31 17:08:11 2007
@@ -226,9 +226,9 @@
 
 int cpu_exec(CPUState *env1)
 {
-    int saved_T0, saved_T1;
+    long saved_T0, saved_T1;
 #if defined(reg_T2)
-    int saved_T2;
+    long saved_T2;
 #endif
     CPUState *saved_env;
 #if defined(TARGET_I386)

 It's funny that the upper half of rbx (which holds env above)
apparently only gets spilled on FreeBSD-current, even tho on amd64
T0 (== AREG1 == rbx) certainly doesn't fit into an int...

	Juergen

Re: [Qemu-devel] Re: strange crash on FreeBSD-current/amd64 (pointer truncation?)

[Thiemo Seufer]

Juergen Lock wrote:
> On Wed, Jan 24, 2007 at 09:00:19PM +0100, Juergen Lock wrote:
> > Hi!
> > 
> >  I got a report of qemu segfaulting here on FreeBSD-current/amd64:
> > 
> > > #0  main_loop () at /usr/ports-cvs/emulators/qemu/work/qemu-snapshot-2007-01-11_05/vl.c:6125
> > > 6125                    env = env->next_cpu;
> > > [New Thread 0x801e10190 (LWP 100214)]
> > > (gdb) print env
> > > $1 = (CPUX86State *) 0xac10000
> > > (gdb) print first_cpu
> > > $2 = (CPUX86State *) 0x80ac10000
> 
> Ok Jung-uk Kim found the following fix: (Thanx!)
> 
> --- qemu/cpu-exec.c.orig	Wed Jan 31 16:58:03 2007
> +++ qemu/cpu-exec.c	Wed Jan 31 17:08:11 2007
> @@ -226,9 +226,9 @@
>  
>  int cpu_exec(CPUState *env1)
>  {
> -    int saved_T0, saved_T1;
> +    long saved_T0, saved_T1;
>  #if defined(reg_T2)
> -    int saved_T2;
> +    long saved_T2;

I used target_ulong instead.


Thiemo

Re: [Qemu-devel] Re: strange crash on FreeBSD-current/amd64 (pointer truncation?)

[Juergen Lock]

On Fri, Feb 02, 2007 at 04:02:00AM +0000, Thiemo Seufer wrote:
> Juergen Lock wrote:
> > On Wed, Jan 24, 2007 at 09:00:19PM +0100, Juergen Lock wrote:
> > > Hi!
> > > 
> > >  I got a report of qemu segfaulting here on FreeBSD-current/amd64:
> > > 
> > > > #0  main_loop () at /usr/ports-cvs/emulators/qemu/work/qemu-snapshot-2007-01-11_05/vl.c:6125
> > > > 6125                    env = env->next_cpu;
> > > > [New Thread 0x801e10190 (LWP 100214)]
> > > > (gdb) print env
> > > > $1 = (CPUX86State *) 0xac10000
> > > > (gdb) print first_cpu
> > > > $2 = (CPUX86State *) 0x80ac10000
> > 
> > Ok Jung-uk Kim found the following fix: (Thanx!)
> > 
> > --- qemu/cpu-exec.c.orig	Wed Jan 31 16:58:03 2007
> > +++ qemu/cpu-exec.c	Wed Jan 31 17:08:11 2007
> > @@ -226,9 +226,9 @@
> >  
> >  int cpu_exec(CPUState *env1)
> >  {
> > -    int saved_T0, saved_T1;
> > +    long saved_T0, saved_T1;
> >  #if defined(reg_T2)
> > -    int saved_T2;
> > +    long saved_T2;
> 
> I used target_ulong instead.

Isn't that 32 bit for 32 bit targets?  Then it wouldn't fix the bug,
the problem happened for 32 bit guests (i386-softmmu) on 64 bit hosts
(FreeBSD-current/amd64), there the upper half of rbx which held a
pointer in the calling function was lost...

	Juergen

Re: [Qemu-devel] Re: strange crash on FreeBSD-current/amd64 (pointer truncation?)

[Thiemo Seufer]

Juergen Lock wrote:
> > > Ok Jung-uk Kim found the following fix: (Thanx!)
> > > 
> > > --- qemu/cpu-exec.c.orig	Wed Jan 31 16:58:03 2007
> > > +++ qemu/cpu-exec.c	Wed Jan 31 17:08:11 2007
> > > @@ -226,9 +226,9 @@
> > >  
> > >  int cpu_exec(CPUState *env1)
> > >  {
> > > -    int saved_T0, saved_T1;
> > > +    long saved_T0, saved_T1;
> > >  #if defined(reg_T2)
> > > -    int saved_T2;
> > > +    long saved_T2;
> > 
> > I used target_ulong instead.
> 
> Isn't that 32 bit for 32 bit targets?  Then it wouldn't fix the bug,
> the problem happened for 32 bit guests (i386-softmmu) on 64 bit hosts
> (FreeBSD-current/amd64), there the upper half of rbx which held a
> pointer in the calling function was lost...

Apparently I miss something here. If T0 is a (32bit) target_ulong,
why does saving it to a (32bit) target_ulong and restoring it from
there cause any trouble?


Thiemo

Re: [Qemu-devel] Re: strange crash on FreeBSD-current/amd64 (pointer truncation?)

[Juergen Lock]

On Fri, Feb 02, 2007 at 10:24:28PM +0000, Thiemo Seufer wrote:
> Juergen Lock wrote:
> > > > Ok Jung-uk Kim found the following fix: (Thanx!)
> > > > 
> > > > --- qemu/cpu-exec.c.orig	Wed Jan 31 16:58:03 2007
> > > > +++ qemu/cpu-exec.c	Wed Jan 31 17:08:11 2007
> > > > @@ -226,9 +226,9 @@
> > > >  
> > > >  int cpu_exec(CPUState *env1)
> > > >  {
> > > > -    int saved_T0, saved_T1;
> > > > +    long saved_T0, saved_T1;
> > > >  #if defined(reg_T2)
> > > > -    int saved_T2;
> > > > +    long saved_T2;
> > > 
> > > I used target_ulong instead.
> > 
> > Isn't that 32 bit for 32 bit targets?  Then it wouldn't fix the bug,
> > the problem happened for 32 bit guests (i386-softmmu) on 64 bit hosts
> > (FreeBSD-current/amd64), there the upper half of rbx which held a
> > pointer in the calling function was lost...
> 
> Apparently I miss something here. If T0 is a (32bit) target_ulong,
> why does saving it to a (32bit) target_ulong and restoring it from
> there cause any trouble?

Hmm.  All I can say is the upper half of rbx (which holds T0) gets
spilled on FreeBSD-current/amd64 hosts unless saving and restoring
the full 64 bit of it...

Re: [Qemu-devel] Re: strange crash on FreeBSD-current/amd64 (pointer truncation?)

[Gwenole Beauchesne]

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

Hi,

> Hmm.  All I can say is the upper half of rbx (which holds T0) gets
> spilled on FreeBSD-current/amd64 hosts unless saving and restoring
> the full 64 bit of it...

That's also what I got with VirtualBox on x86_64. Here is an update to  
the patch I posted yesterday and that applies to current QEMU CVS  
instead.

Please note this one applies unconditionnally of the host and target  
combinations. Besides, I only tested the following combinations:  
x86_64/x86_64-softmmu, x86_64/i386-softmmu, i586/x86_64-softmmu,  
i586/i386-softmmu. And I verified that the generated code for  
cpu_exec() with both gcc 3.4 and gcc 4.1 actually does the host  
registers save/restore correctly.

BTW, for e.g. SPARC hosts with %l<R> registers, we could define  
something like DONT_PRESERVE_AREG<AR> since those would lay in a new  
register window and thus don't require an explicit save/restore.

Regards,
Gwenole Beauchesne

<http://svn.mandriva.com/cgi-bin/viewvc.cgi/packages/cooker/qemu/ 
current/SOURCES/qemu-0.8.3-preserve-host-registers.patch>


[-- Attachment #2: qemu-0.8.3-preserve-host-registers.patch --]
[-- Type: application/octet-stream, Size: 6280 bytes --]

2007-02-03  Gwenole Beauchesne  <gbeauchesne@mandriva.com>

	* Preserve callee-saved global registers.

--- qemu-0.8.3/cpu-exec.c.preserve-host-registers	2007-02-02 13:45:47.000000000 +0100
+++ qemu-0.8.3/cpu-exec.c	2007-02-03 09:51:55.000000000 +0100
@@ -226,37 +226,9 @@ static inline TranslationBlock *tb_find_
 
 int cpu_exec(CPUState *env1)
 {
-    target_ulong saved_T0, saved_T1;
-#if defined(reg_T2)
-    target_ulong saved_T2;
-#endif
-    CPUState *saved_env;
-#if defined(TARGET_I386)
-#ifdef reg_EAX
-    int saved_EAX;
-#endif
-#ifdef reg_ECX
-    int saved_ECX;
-#endif
-#ifdef reg_EDX
-    int saved_EDX;
-#endif
-#ifdef reg_EBX
-    int saved_EBX;
-#endif
-#ifdef reg_ESP
-    int saved_ESP;
-#endif
-#ifdef reg_EBP
-    int saved_EBP;
-#endif
-#ifdef reg_ESI
-    int saved_ESI;
-#endif
-#ifdef reg_EDI
-    int saved_EDI;
-#endif
-#elif defined(TARGET_SPARC)
+#define DO_HOST_REGS_DECLARE
+#include "hostregs_helper.h"
+#if defined(TARGET_SPARC)
 #if defined(reg_REGWPTR)
     uint32_t *saved_regwptr;
 #endif
@@ -325,44 +297,15 @@ int cpu_exec(CPUState *env1)
     cpu_single_env = env1; 
 
     /* first we save global registers */
-    saved_env = env;
+#define DO_HOST_REGS_SAVE
+#include "hostregs_helper.h"
     env = env1;
-    saved_T0 = T0;
-    saved_T1 = T1;
-#if defined(reg_T2)
-    saved_T2 = T2;
-#endif
 #if defined(__sparc__) && !defined(HOST_SOLARIS)
     /* we also save i7 because longjmp may not restore it */
     asm volatile ("mov %%i7, %0" : "=r" (saved_i7));
 #endif
 
 #if defined(TARGET_I386)
-#ifdef reg_EAX
-    saved_EAX = EAX;
-#endif
-#ifdef reg_ECX
-    saved_ECX = ECX;
-#endif
-#ifdef reg_EDX
-    saved_EDX = EDX;
-#endif
-#ifdef reg_EBX
-    saved_EBX = EBX;
-#endif
-#ifdef reg_ESP
-    saved_ESP = ESP;
-#endif
-#ifdef reg_EBP
-    saved_EBP = EBP;
-#endif
-#ifdef reg_ESI
-    saved_ESI = ESI;
-#endif
-#ifdef reg_EDI
-    saved_EDI = EDI;
-#endif
-
     env_to_regs();
     /* put eflags in CPU temporary format */
     CC_SRC = env->eflags & (CC_O | CC_S | CC_Z | CC_A | CC_P | CC_C);
@@ -827,32 +770,6 @@ int cpu_exec(CPUState *env1)
 #endif
     /* restore flags in standard format */
     env->eflags = env->eflags | cc_table[CC_OP].compute_all() | (DF & DF_MASK);
-
-    /* restore global registers */
-#ifdef reg_EAX
-    EAX = saved_EAX;
-#endif
-#ifdef reg_ECX
-    ECX = saved_ECX;
-#endif
-#ifdef reg_EDX
-    EDX = saved_EDX;
-#endif
-#ifdef reg_EBX
-    EBX = saved_EBX;
-#endif
-#ifdef reg_ESP
-    ESP = saved_ESP;
-#endif
-#ifdef reg_EBP
-    EBP = saved_EBP;
-#endif
-#ifdef reg_ESI
-    ESI = saved_ESI;
-#endif
-#ifdef reg_EDI
-    EDI = saved_EDI;
-#endif
 #elif defined(TARGET_ARM)
     /* XXX: Save/restore host fpu exception state?.  */
 #elif defined(TARGET_SPARC)
@@ -871,15 +788,14 @@ int cpu_exec(CPUState *env1)
 #else
 #error unsupported target CPU
 #endif
+
+    /* restore global registers */
 #if defined(__sparc__) && !defined(HOST_SOLARIS)
     asm volatile ("mov %0, %%i7" : : "r" (saved_i7));
 #endif
-    T0 = saved_T0;
-    T1 = saved_T1;
-#if defined(reg_T2)
-    T2 = saved_T2;
-#endif
-    env = saved_env;
+#define DO_HOST_REGS_RESTORE
+#include "hostregs_helper.h"
+
     /* fail safe : never use cpu_single_env outside cpu_exec() */
     cpu_single_env = NULL; 
     return ret;
--- qemu-0.8.3/hostregs_helper.h.preserve-host-registers	2007-02-03 10:02:51.000000000 +0100
+++ qemu-0.8.3/hostregs_helper.h	2007-02-03 10:02:27.000000000 +0100
@@ -0,0 +1,145 @@
+/*
+ *  host registers helpers for dyngen execution engine
+ *
+ *  Copyright (c) 2003 Fabrice Bellard
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#ifdef AREG0
+#define DO_AREG0(x) x
+#else
+#define DO_AREG0(x)
+#endif
+
+#ifdef AREG1
+#define DO_AREG1(x) x
+#else
+#define DO_AREG1(x)
+#endif
+
+#ifdef AREG2
+#define DO_AREG2(x) x
+#else
+#define DO_AREG2(x)
+#endif
+
+#ifdef AREG3
+#define DO_AREG3(x) x
+#else
+#define DO_AREG3(x)
+#endif
+
+#ifdef AREG4
+#define DO_AREG4(x) x
+#else
+#define DO_AREG4(x)
+#endif
+
+#ifdef AREG5
+#define DO_AREG5(x) x
+#else
+#define DO_AREG5(x)
+#endif
+
+#ifdef AREG6
+#define DO_AREG6(x) x
+#else
+#define DO_AREG6(x)
+#endif
+
+#ifdef AREG7
+#define DO_AREG7(x) x
+#else
+#define DO_AREG7(x)
+#endif
+
+#ifdef AREG8
+#define DO_AREG8(x) x
+#else
+#define DO_AREG8(x)
+#endif
+
+#ifdef AREG9
+#define DO_AREG9(x) x
+#else
+#define DO_AREG9(x)
+#endif
+
+#ifdef AREG10
+#define DO_AREG10(x) x
+#else
+#define DO_AREG10(x)
+#endif
+
+#ifdef AREG11
+#define DO_AREG11(x) x
+#else
+#define DO_AREG11(x)
+#endif
+
+#if defined(DO_HOST_REGS_DECLARE)
+#define DO_EXPR(REG)									\
+    register uintptr_t reg_AREG##REG asm(AREG##REG);	\
+    volatile uintptr_t saved_AREG##REG;
+#elif defined(DO_HOST_REGS_SAVE)
+#define DO_EXPR(REG) saved_AREG##REG = reg_AREG##REG;
+#elif defined(DO_HOST_REGS_RESTORE)
+#define DO_EXPR(REG) do {								\
+    reg_AREG##REG = saved_AREG##REG;					\
+    __asm__ __volatile__ ("" : : "r" (reg_AREG##REG));	\
+} while (0);
+#endif
+
+DO_AREG0(DO_EXPR(0))
+#undef DO_AREG0
+
+DO_AREG1(DO_EXPR(1))
+#undef DO_AREG1
+
+DO_AREG2(DO_EXPR(2))
+#undef DO_AREG2
+
+DO_AREG3(DO_EXPR(3))
+#undef DO_AREG3
+
+DO_AREG4(DO_EXPR(4))
+#undef DO_AREG4
+
+DO_AREG5(DO_EXPR(5))
+#undef DO_AREG5
+
+DO_AREG6(DO_EXPR(6))
+#undef DO_AREG6
+
+DO_AREG7(DO_EXPR(7))
+#undef DO_AREG7
+
+DO_AREG8(DO_EXPR(8))
+#undef DO_AREG8
+
+DO_AREG9(DO_EXPR(9))
+#undef DO_AREG9
+
+DO_AREG10(DO_EXPR(10))
+#undef DO_AREG10
+
+DO_AREG11(DO_EXPR(11))
+#undef DO_AREG11
+
+#undef DO_EXPR
+#undef DO_HOST_REGS_DECLARE
+#undef DO_HOST_REGS_SAVE
+#undef DO_HOST_REGS_RESTORE

Re: [Qemu-devel] Re: strange crash on FreeBSD-current/amd64 (pointer truncation?)

[Paul Brook]

On Saturday 03 February 2007 18:12, Gwenole Beauchesne wrote:
> Hi,
>
> > Hmm.  All I can say is the upper half of rbx (which holds T0) gets
> > spilled on FreeBSD-current/amd64 hosts unless saving and restoring
> > the full 64 bit of it...
>
> That's also what I got with VirtualBox on x86_64. Here is an update to
> the patch I posted yesterday and that applies to current QEMU CVS
> instead.

I rewrote it a bit and applied to CVS.

Paul

Re: [Qemu-devel] Re: strange crash on FreeBSD-current/amd64 (pointer truncation?)

[andrzej zaborowski]

On 02/02/07, Juergen Lock <nox@jelal.kn-bremen.de> wrote:
> On Fri, Feb 02, 2007 at 04:02:00AM +0000, Thiemo Seufer wrote:
> > Juergen Lock wrote:
> > > On Wed, Jan 24, 2007 at 09:00:19PM +0100, Juergen Lock wrote:
> > > > Hi!
> > > >
> > > >  I got a report of qemu segfaulting here on FreeBSD-current/amd64:
> > > >
> > > > > #0  main_loop () at /usr/ports-cvs/emulators/qemu/work/qemu-snapshot-2007-01-11_05/vl.c:6125
> > > > > 6125                    env = env->next_cpu;
> > > > > [New Thread 0x801e10190 (LWP 100214)]
> > > > > (gdb) print env
> > > > > $1 = (CPUX86State *) 0xac10000
> > > > > (gdb) print first_cpu
> > > > > $2 = (CPUX86State *) 0x80ac10000
> > >
> > > Ok Jung-uk Kim found the following fix: (Thanx!)
> > >
> > > --- qemu/cpu-exec.c.orig    Wed Jan 31 16:58:03 2007
> > > +++ qemu/cpu-exec.c Wed Jan 31 17:08:11 2007
> > > @@ -226,9 +226,9 @@
> > >
> > >  int cpu_exec(CPUState *env1)
> > >  {
> > > -    int saved_T0, saved_T1;
> > > +    long saved_T0, saved_T1;
> > >  #if defined(reg_T2)
> > > -    int saved_T2;
> > > +    long saved_T2;
> >
> > I used target_ulong instead.
>
> Isn't that 32 bit for 32 bit targets?  Then it wouldn't fix the bug,

Well, it just happens that target_ulong is 64 bit even for 32 bit
target on a 64 bit host (as a workaround for something I think, was
wondering about it too. I asked on IRC once but no one knew the actual
reason). This however doesn't change the fact that target_ulong
shouldn't be used here.

Regards,
Andrew

Re: [Qemu-devel] Re: strange crash on FreeBSD-current/amd64 (pointer truncation?)

[Paul Brook]

> > Isn't that 32 bit for 32 bit targets?  Then it wouldn't fix the bug,
>
> Well, it just happens that target_ulong is 64 bit even for 32 bit
> target on a 64 bit host (as a workaround for something I think, was
> wondering about it too. I asked on IRC once but no one knew the actual
> reason). This however doesn't change the fact that target_ulong
> shouldn't be used here.

Are you sure you're not confusing it with target_phys_address_t?

Paul

Re: [Qemu-devel] Re: strange crash on FreeBSD-current/amd64 (pointer truncation?)

[andrzej zaborowski]

On 02/02/07, Paul Brook <paul@codesourcery.com> wrote:
> > > Isn't that 32 bit for 32 bit targets?  Then it wouldn't fix the bug,
> >
> > Well, it just happens that target_ulong is 64 bit even for 32 bit
> > target on a 64 bit host (as a workaround for something I think, was
> > wondering about it too. I asked on IRC once but no one knew the actual
> > reason). This however doesn't change the fact that target_ulong
> > shouldn't be used here.
>
> Are you sure you're not confusing it with target_phys_address_t?

Oops, right, TARGET_PHYS_ADDR_BITS and TARGET_LONG_BITS are different.
Sorry for the confusion.

Regards,
Andrew