[Qemu-devel] gcc 4.4 miscompiling cpu_exec() ?

[Qemu-devel] gcc 4.4 miscompiling cpu_exec() ?

[Jay Foad]

I'm building QEMU mipsel-linux-user with Ubuntu's GCC 4.4 on an x86
host. Whenever I try to run a trivial MIPS executable, QEMU segfaults
in cpu_loop() shortly after the call to cpu_mips_exec().

The problem seems to be that cpu_exec() doesn't preserve ebp. It tries to:

    saved_env_reg = (host_reg_t) env;

where env is a global variable decorated with asm("ebp"). This saves
ebp to the stack, but later on, in some function inlined into
cpu_exec(), the value on the stack gets overwritten with something
else.

Has anyone else seen this?

The full GCC version string is:

gcc (Ubuntu 4.4.1-4ubuntu9) 4.4.1

The following versions of GCC don't seem to suffer from the same problem:

gcc-4.1 (GCC) 4.1.3 20080704 (prerelease) (Ubuntu 4.1.2-27ubuntu1)
gcc-4.2 (GCC) 4.2.4 (Ubuntu 4.2.4-5ubuntu1)
gcc-4.3 (Ubuntu 4.3.4-5ubuntu1) 4.3.4

Thanks,
Jay.

Re: [Qemu-devel] gcc 4.4 miscompiling cpu_exec() ?

[Jay Foad]

On 23 February 2010 17:03, Aurelien Jarno <aurelien@aurel32.net> wrote:
> Jay Foad a écrit :
>> I'm building QEMU mipsel-linux-user with Ubuntu's GCC 4.4 on an x86
>> host. Whenever I try to run a trivial MIPS executable, QEMU segfaults
>> in cpu_loop() shortly after the call to cpu_mips_exec().
>>
>> The problem seems to be that cpu_exec() doesn't preserve ebp. It tries to:
>>
>>     saved_env_reg = (host_reg_t) env;
>>
>> where env is a global variable decorated with asm("ebp"). This saves
>> ebp to the stack, but later on, in some function inlined into
>> cpu_exec(), the value on the stack gets overwritten with something
>> else.
>>
>> Has anyone else seen this?
>>
>
> Yes, but only in qemu 0.12.0 to 0.12.1. The issue should be fixed in the
> stable branch and in head.

I'm seeing it today, with sources from git:

git://git.qemu.org/qemu.git
commit 724c689357211cb929c9b957e1556f211d2b56db

Thanks,
Jay.

[Qemu-devel] Re: gcc 4.4 miscompiling cpu_exec() ?

[Paolo Bonzini]

On 02/23/2010 03:50 PM, Jay Foad wrote:
> I'm building QEMU mipsel-linux-user with Ubuntu's GCC 4.4 on an x86
> host. Whenever I try to run a trivial MIPS executable, QEMU segfaults
> in cpu_loop() shortly after the call to cpu_mips_exec().
>
> The problem seems to be that cpu_exec() doesn't preserve ebp. It tries to:
>
>      saved_env_reg = (host_reg_t) env;
>
> where env is a global variable decorated with asm("ebp"). This saves
> ebp to the stack, but later on, in some function inlined into
> cpu_exec(), the value on the stack gets overwritten with something
> else.

Can you try this patch:

diff --git a/cpu-exec.c b/cpu-exec.c
index 51aa416..bfaf908 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -215,7 +215,7 @@ static void cpu_handle_debug_exception(CPUState *env)

  int cpu_exec(CPUState *env1)
  {
-    host_reg_t saved_env_reg;
+    volatile host_reg_t saved_env_reg;
      int ret, interrupt_request;
      TranslationBlock *tb;
      uint8_t *tc_ptr;
@@ -230,8 +230,8 @@ int cpu_exec(CPUState *env1)
         value, so that files not including target-xyz/exec.h are free to
         use it.  */
      QEMU_BUILD_BUG_ON (sizeof (saved_env_reg) != sizeof (env));
-    saved_env_reg = (host_reg_t) env;
      asm("");
+    saved_env_reg = (host_reg_t) env;
      env = env1;

  #if defined(TARGET_I386)

and if it works, possibly only each hunk of it?

Paolo

[Qemu-devel] Re: gcc 4.4 miscompiling cpu_exec() ?

[Jay Foad]

> Can you try this patch:

It works! Thanks.

> and if it works, possibly only each hunk of it?

Just the first hunk: works!
Just the second hunk: doesn't work

Can you explain why the volatile is necessary? Or is it working around
a problem with the compiler?

Thanks,
Jay.

[Qemu-devel] Re: gcc 4.4 miscompiling cpu_exec() ?

[Paolo Bonzini]

On 02/23/2010 07:17 PM, Jay Foad wrote:
>> Can you try this patch:
>
> It works! Thanks.
>
>> and if it works, possibly only each hunk of it?
>
> Just the first hunk: works!
> Just the second hunk: doesn't work
>
> Can you explain why the volatile is necessary? Or is it working around
> a problem with the compiler?

I don't know exactly without checking, but these were the main changes 
introduced by my 24ebf5f31a178051cff1a4aab5ba621037191577 patch.  I'll 
send it out shortly.

Paolo

[Qemu-devel] [PATCH] declare saved_env_reg as volatile

[Paolo Bonzini]

This ensures that the compiler does not move it away from
the "env = env1;" assignment.  Fixes a miscompilation
on gcc 4.4, reported by Jay Foad.

Cc: <jay.foad@gmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpu-exec.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index 51aa416..8721684 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -215,7 +215,7 @@ static void cpu_handle_debug_exception(CPUState *env)
 
 int cpu_exec(CPUState *env1)
 {
-    host_reg_t saved_env_reg;
+    volatile host_reg_t saved_env_reg;
     int ret, interrupt_request;
     TranslationBlock *tb;
     uint8_t *tc_ptr;
-- 
1.6.6