[Qemu-devel] Regression: segfault on ARM host

[Qemu-devel] Regression: segfault on ARM host

[Aurelien Jarno]

While trying to implement setcond on TCG ARM, I have discovered it does
not work anymore. I have bisected this regression to:

commit 6113d6d3169393c323ac4c82d756a850145a5e7a
Author: Paolo Bonzini <pbonzini@redhat.com>
Date:   Fri Jan 15 09:42:09 2010 +0100

    change while to if

    The while loop will be executed exactly 0 or 1 times, depending on
    env->exit_request.

    Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
    Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

The assertion is actually triggered. When the next patch removing the
assertion is also applied it segfaults instead.

The patch does not seem to be wrong. Moreover I have tried to debug the
problem, but I failed to as it looks like an Heisenbug. Compiling this
file with -O0 also fixes the problem.

Does someone have an idea?

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

[Qemu-devel] Re: Regression: segfault on ARM host

[Paolo Bonzini]

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

On 03/01/2010 10:33 PM, Aurelien Jarno wrote:
> While trying to implement setcond on TCG ARM, I have discovered it does
> not work anymore. I have bisected this regression to:
>
> commit 6113d6d3169393c323ac4c82d756a850145a5e7a
> Author: Paolo Bonzini<pbonzini@redhat.com>
> Date:   Fri Jan 15 09:42:09 2010 +0100
>
>      change while to if
>
>      The while loop will be executed exactly 0 or 1 times, depending on
>      env->exit_request.
>
>      Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
>      Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>
> The assertion is actually triggered. When the next patch removing the
> assertion is also applied it segfaults instead.

Looks like a race.  The only piece of logic that is changed by that 
commit is reverted in the attached patch, can you try it?  If it passes, 
I can resubmit with S-o-b.

If it doesn't pass, I wonder whether the while loop was there to trick 
the compiler into not optimizing something.  Seems a bit too clever though.

Paolo

[-- Attachment #2: qemu-race.patch --]
[-- Type: text/plain, Size: 1030 bytes --]

diff --git a/cpu-exec.c b/cpu-exec.c
index 5d6dd51..61b1c59 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -602,9 +602,15 @@ int cpu_exec(CPUState *env1)
                 /* cpu_interrupt might be called while translating the
                    TB, but before it is linked into a potentially
                    infinite loop and becomes env->current_tb. Avoid
-                   starting execution if there is a pending interrupt. */
-                if (!unlikely (env->exit_request)) {
-                    env->current_tb = tb;
+                   starting execution if there is a pending interrupt. 
+		   Doing it this way is necessary to avoid races with
+		   cpu_unlink_tb (called by cpu_exit).  */
+                env->current_tb = tb;
+                if (unlikely (env->exit_request)) {
+                    env->current_tb = NULL;
+		}
+
+		if (likely (env->current_tb)) {
                     tc_ptr = tb->tc_ptr;
                 /* execute the generated code */
 #if defined(__sparc__) && !defined(CONFIG_SOLARIS)

[Qemu-devel] Re: Regression: segfault on ARM host

[Aurelien Jarno]

On Mon, Mar 01, 2010 at 11:33:23PM +0100, Paolo Bonzini wrote:
> On 03/01/2010 10:33 PM, Aurelien Jarno wrote:
>> While trying to implement setcond on TCG ARM, I have discovered it does
>> not work anymore. I have bisected this regression to:
>>
>> commit 6113d6d3169393c323ac4c82d756a850145a5e7a
>> Author: Paolo Bonzini<pbonzini@redhat.com>
>> Date:   Fri Jan 15 09:42:09 2010 +0100
>>
>>      change while to if
>>
>>      The while loop will be executed exactly 0 or 1 times, depending on
>>      env->exit_request.
>>
>>      Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
>>      Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>
>> The assertion is actually triggered. When the next patch removing the
>> assertion is also applied it segfaults instead.
>
> Looks like a race.  The only piece of logic that is changed by that  
> commit is reverted in the attached patch, can you try it?  If it passes,  
> I can resubmit with S-o-b.

Unfortunately it doesn't work.

> If it doesn't pass, I wonder whether the while loop was there to trick  
> the compiler into not optimizing something.  Seems a bit too clever 
> though.
>

It looks like it is the case. Just replacing the if by a while in your
patch make it working again. But I do wonder what this trick is actually
preventing, as there is probably a better way to prevent that.

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] Re: Regression: segfault on ARM host

[Aurelien Jarno]

On Tue, Mar 02, 2010 at 01:05:29AM +0100, Aurelien Jarno wrote:
> On Mon, Mar 01, 2010 at 11:33:23PM +0100, Paolo Bonzini wrote:
> > On 03/01/2010 10:33 PM, Aurelien Jarno wrote:
> >> While trying to implement setcond on TCG ARM, I have discovered it does
> >> not work anymore. I have bisected this regression to:
> >>
> >> commit 6113d6d3169393c323ac4c82d756a850145a5e7a
> >> Author: Paolo Bonzini<pbonzini@redhat.com>
> >> Date:   Fri Jan 15 09:42:09 2010 +0100
> >>
> >>      change while to if
> >>
> >>      The while loop will be executed exactly 0 or 1 times, depending on
> >>      env->exit_request.
> >>
> >>      Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> >>      Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
> >>
> >> The assertion is actually triggered. When the next patch removing the
> >> assertion is also applied it segfaults instead.
> >
> > Looks like a race.  The only piece of logic that is changed by that  
> > commit is reverted in the attached patch, can you try it?  If it passes,  
> > I can resubmit with S-o-b.
> 
> Unfortunately it doesn't work.
> 

The bug is actually in the ARM backend, I have just send a patch on the
mailing list to fix it.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net