"cpu-exec.c: avoid AREG0 use" breaks x86 emulation on x86-64

Hi Blue,

commit cea5f9a28f breaks here, just starting qemu without any parameters:

Starting program: qemu-system-x86_64
[Thread debugging using libthread_db enabled]

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff39ac770 in __sigsetjmp () from /lib64/libc.so.6
(gdb) bt
#0 0x00007ffff39ac770 in __sigsetjmp () from /lib64/libc.so.6
#1 0x00000000004eb96c in cpu_x86_exec (env=0x11d09a0) at cpu-exec.c:233
#2 0x000000000040f056 in tcg_cpu_exec (env=0x11d09a0) at cpus.c:1059
#3 cpu_exec_all () at cpus.c:1100
#4 0x000000000058cfcb in main_loop () at vl.c:1380
#5 main (argc=<value optimized out>, argv=<value optimized out>, envp=<value optimized out>) at vl.c:3318

Please have a look.

Jan

Hi Blue,

commit cea5f9a28f breaks here, just starting qemu without any parameters:

Starting program: qemu-system-x86_64
[Thread debugging using libthread_db enabled]

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff39ac770 in __sigsetjmp () from /lib64/libc.so.6
(gdb) bt
#0  0x00007ffff39ac770 in __sigsetjmp () from /lib64/libc.so.6
#1  0x00000000004eb96c in cpu_x86_exec (env=0x11d09a0) at cpu-exec.c:233
#2  0x000000000040f056 in tcg_cpu_exec (env=0x11d09a0) at cpus.c:1059
#3  cpu_exec_all () at cpus.c:1100
#4  0x000000000058cfcb in main_loop () at vl.c:1380
#5  main (argc=<value optimized out>, argv=<value optimized out>, envp=<value optimized out>) at vl.c:3318

Please have a look.

Jan

--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Quoted 20 lines · Jan Kiszka

Hi Blue,

commit cea5f9a28f breaks here, just starting qemu without any parameters:

Starting program: qemu-system-x86_64
[Thread debugging using libthread_db enabled]

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff39ac770 in __sigsetjmp () from /lib64/libc.so.6
(gdb) bt
#0  0x00007ffff39ac770 in __sigsetjmp () from /lib64/libc.so.6
#1  0x00000000004eb96c in cpu_x86_exec (env=0x11d09a0) at cpu-exec.c:233
#2  0x000000000040f056 in tcg_cpu_exec (env=0x11d09a0) at cpus.c:1059
#3  cpu_exec_all () at cpus.c:1100
#4  0x000000000058cfcb in main_loop () at vl.c:1380
#5  main (argc=<value optimized out>, argv=<value optimized out>, envp=<value optimized out>) at vl.c:3318

Please have a look.

Quoted 7 lines

Jan

--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

It works on gcc 4.4.5 x64. Which version is your gcc?

From: Jan Kiszka <[email protected]>

Recent compilers look deep into cpu_exec, find longjmp as a noreturn function and decide to smash some stack variables as they won't be used again. This may lead to env becoming invalid after return from setjmp, causing crashes. Fix it by reloading env from cpu_single_env in that case.

Signed-off-by Jan Kiszka <[email protected]>

---
 cpu-exec.c |    4 ++++

 1 files changed, 4 insertions(+), 0 deletions(-)

cpu-exec.c +0 -0

index 20e3ec4..de0d716 100644
@@ -587,6 +587,10 @@ int cpu_exec(CPUState *env)
                 /* reset soft MMU for next block (it can currently

                   only be set by a memory fault) */
            } /* for(;;) */
+        } else {
+            /* Reload env after longjmp - the compiler may have smashed all
+             * local variables as longjmp is marked 'noreturn'. */
+            env = cpu_single_env;
        }
    } /* for(;;) */

Quoted 7 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

Recent compilers look deep into cpu_exec, find longjmp as a noreturn function and decide to smash some stack variables as they won't be used again. This may lead to env becoming invalid after return from setjmp, causing crashes. Fix it by reloading env from cpu_single_env in that case.

Nice. Could you try if gcc flag -Wclobbered catches something using your compiler without your patch:

commit f826f0d0f5cf5dd18a0d34159c1a3bc8f2e6ddf4

Author: Blue Swirl <[email protected]>
Date:   Sun Sep 26 11:58:38 2010 +0000

Add gcc warning -Wclobbered

Signed-off-by Blue Swirl <[email protected]>

configure +0 -0

index 88159ac..2417205 100755

static const char *pch;

monitor.c +1 -0

+static char *saved_key;
  static jmp_buf expr_env;
 
  #define MD_TLONG 0

From: Jan Kiszka <[email protected]>

Recent compilers look deep into cpu_exec, find longjmp as a noreturn function and decide to smash some stack variables as they won't be used again. This may lead to env becoming invalid after return from setjmp, causing crashes. Fix it by reloading env from cpu_single_env in that case.

Signed-off-by Jan Kiszka <[email protected]>

---
 cpu-exec.c |    4 ++++

 1 files changed, 4 insertions(+), 0 deletions(-)

cpu-exec.c +0 -0

index 20e3ec4..de0d716 100644
@@ -587,6 +587,10 @@ int cpu_exec(CPUState *env)
                 /* reset soft MMU for next block (it can currently

                   only be set by a memory fault) */
            } /* for(;;) */
+        } else {
+            /* Reload env after longjmp - the compiler may have smashed all
+             * local variables as longjmp is marked 'noreturn'. */
+            env = cpu_single_env;
        }
    } /* for(;;) */

From: Jan Kiszka <[email protected]>

Recent compilers look deep into cpu_exec, find longjmp as a noreturn function and decide to smash some stack variables as they won't be used again. This may lead to env becoming invalid after return from setjmp, causing crashes. Fix it by reloading env from cpu_single_env in that case.

Quoted 5 lines

Quoted 5 lines

Recent compilers look deep into cpu_exec, find longjmp as a noreturn function and decide to smash some stack variables as they won't be used again. This may lead to env becoming invalid after return from setjmp, causing crashes. Fix it by reloading env from cpu_single_env in that case.

Can you give more details of what compiler/platform this was a problem for? My reading of the C standard is that the compiler isn't allowed to trash env across this longjmp, because it's a variable of automatic scope which isn't modified between the setjmp and the longjmp...

Quoted 5 lines · Peter Maydell

Can you give more details of what compiler/platform this was a problem for? My reading of the C standard is that the compiler isn't allowed to trash env across this longjmp, because it's a variable of automatic scope which isn't modified between the setjmp and the longjmp...

longjmp can destroy any non-volatile variable (-Wclobbered warns about this).

"All accessible objects have values [...] as of the time the longjmp function was called, except that the values of objects of automatic storage duration that are local to the function containing the invocation of the corresponding setjmp macro that do not have volatile-qualified type and have been changed between the setjmp invocation and longjmp call are indeterminate."
-- C99 section 7.13.2.1 para 3.

So variables may only be destroyed if they are all of:
* local to the function calling setjmp
* not volatile
* changed between setjmp and longjmp

We don't change env between the setjmp and longjmp so the compiler should not trash it. (Indeed according to Jan in
http://lists.gnu.org/archive/html/qemu-devel/2011-07/msg00144.html
-Wclobbered doesn't complain about this code.)

Quoted 4 lines · Peter Maydell

We don't change env between the setjmp and longjmp so the compiler should not trash it. (Indeed according to Jan in
http://lists.gnu.org/archive/html/qemu-devel/2011-07/msg00144.html
-Wclobbered doesn't complain about this code.)

Then it's a compiler bug, not smartness.  Making env volatile (or making a volatile copy if there is a performance impact) should still be enough to work around it.

Quoted 4 lines

Quoted 4 lines

Then it's a compiler bug, not smartness. Making env volatile (or making a volatile copy if there is a performance impact) should still be enough to work around it.

Yes. (It would have to be a volatile copy, I think, env is a function parameter and I don't think you can make those volatile.)
https://bugs.launchpad.net/qemu/+bug/823902 includes some discussion of the effects on the test of adding the volatile copy.

If I make cpu_single_env thread local with __thread and leave 0d101... in, then again it works reliably on 32bit Lucid, and is flaky on 64 bit Oneiric (5/10 2 hangs, 3 segs)

I've also tried using a volatile local variable in cpu_exec to hold a copy of env and restore that rather than cpu_single_env. With this it's solid on 32bit lucid and flaky on 64bit Oneirc; these failures on 64bit OO look like it running off the end of the code buffer (all 0 code), jumping to non-existent code addresses and a seg in tb_reset_jump_recursive2.

I'm not sure about what to read from there:

Quoted 10 lines

If I make cpu_single_env thread local with __thread and leave 0d101... in, then again it works reliably on 32bit Lucid, and is flaky on 64 bit Oneiric (5/10 2 hangs, 3 segs)

I've also tried using a volatile local variable in cpu_exec to hold a copy of env and restore that rather than cpu_single_env. With this it's solid on 32bit lucid and flaky on 64bit Oneirc; these failures on 64bit OO look like it running off the end of the code buffer (all 0 code), jumping to non-existent code addresses and a seg in tb_reset_jump_recursive2.

It looks like neither a thread-local cpu_single_env nor a volatile copy fix the bug?!?

Quoted 9 lines · Peter Maydell

Quoted 4 lines

Quoted 4 lines

Then it's a compiler bug, not smartness.  Making env volatile (or making a volatile copy if there is a performance impact) should still be enough to work around it.

Yes. (It would have to be a volatile copy, I think, env is a function parameter and I don't think you can make those volatile.)
https://bugs.launchpad.net/qemu/+bug/823902  includes some discussion of the effects on the test of adding the volatile copy.

I'm not sure about what to read from there:

Quoted 10 lines

If I make cpu_single_env thread local with __thread and leave 0d101... in, then again it works reliably on 32bit Lucid, and is flaky on 64 bit Oneiric (5/10 2 hangs, 3 segs)

I've also tried using a volatile local variable in cpu_exec to hold a copy of env and restore that rather than cpu_single_env. With this it's solid on 32bit lucid and flaky on 64bit Oneirc; these failures on 64bit OO look like it running off the end of the code buffer (all 0 code), jumping to non-existent code addresses and a seg in tb_reset_jump_recursive2.

It looks like neither a thread-local cpu_single_env nor a volatile copy fix the bug?!?

I cannot think off-hand of a reason why thread-local cpu_single_env should not work for iothread under Unix, BTW.  Since cpu_single_env is only set/used by a thread at a time (under the global lock), its users cannot distinguish between a thread-local variable and a global.

The only problem would be Windows, which runs cpu_signal in a thread different than the CPU thread.  But that can be fixed easily in qemu_cpu_kick_thread.

Quoted 4 lines

I cannot think off-hand of a reason why thread-local cpu_single_env should not work for iothread under Unix, BTW. Since cpu_single_env is only set/used by a thread at a time (under the global lock), its users cannot distinguish between a thread-local variable and a global.

Thanks for the clarification. As you say, as long as we don't ever try to access it from another thread we're fine...

Quoted 3 lines

The only problem would be Windows, which runs cpu_signal in a thread different than the CPU thread. But that can be fixed easily in qemu_cpu_kick_thread.

...and we just need to fix this.