[Qemu-devel] [PATCH] cpu-exec(): also reload CPUClass *cc after longjmp return

[Qemu-devel] [PATCH] cpu-exec(): also reload CPUClass *cc after longjmp return

[Juergen Lock]

Local variable CPUClass *cc needs to be reloaded after return from longjmp
too.  (This fixes the mips-softmmu crash observed on FreeBSD when qemu is
built with clang.)

Signed-off-by: Juergen Lock <nox@jelal.kn-bremen.de>
Found-by: Dimitry Andric <dim@FreeBSD.org>

--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -681,6 +681,10 @@ int cpu_exec(CPUArchState *env)
              * local variables as longjmp is marked 'noreturn'. */
             cpu = current_cpu;
             env = cpu->env_ptr;
+#if !(defined(CONFIG_USER_ONLY) && \
+      (defined(TARGET_M68K) || defined(TARGET_PPC) || defined(TARGET_S390X)))
+            cc = CPU_GET_CLASS(cpu);
+#endif
         }
     } /* for(;;) */
 

Re: [Qemu-devel] [PATCH] cpu-exec(): also reload CPUClass *cc after longjmp return

[Peter Maydell]

On 3 October 2013 23:09, Juergen Lock <qemu-l@jelal.kn-bremen.de> wrote:
> Local variable CPUClass *cc needs to be reloaded after return from longjmp
> too.  (This fixes the mips-softmmu crash observed on FreeBSD when qemu is
> built with clang.)
>
> Signed-off-by: Juergen Lock <nox@jelal.kn-bremen.de>
> Found-by: Dimitry Andric <dim@FreeBSD.org>
>
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -681,6 +681,10 @@ int cpu_exec(CPUArchState *env)
>               * local variables as longjmp is marked 'noreturn'. */
>              cpu = current_cpu;
>              env = cpu->env_ptr;
> +#if !(defined(CONFIG_USER_ONLY) && \
> +      (defined(TARGET_M68K) || defined(TARGET_PPC) || defined(TARGET_S390X)))
> +            cc = CPU_GET_CLASS(cpu);
> +#endif

This is a c compiler or libc bug -- the C standard says that this
local variable should not be trashed by the longjmp. We were
actually discussing removing the current workarounds there...

-- PMM

Re: [Qemu-devel] [PATCH] cpu-exec(): also reload CPUClass *cc after longjmp return

[Jan Kiszka]

On 2013-10-03 18:05, Peter Maydell wrote:
> On 3 October 2013 23:09, Juergen Lock <qemu-l@jelal.kn-bremen.de> wrote:
>> Local variable CPUClass *cc needs to be reloaded after return from longjmp
>> too.  (This fixes the mips-softmmu crash observed on FreeBSD when qemu is
>> built with clang.)
>>
>> Signed-off-by: Juergen Lock <nox@jelal.kn-bremen.de>
>> Found-by: Dimitry Andric <dim@FreeBSD.org>
>>
>> --- a/cpu-exec.c
>> +++ b/cpu-exec.c
>> @@ -681,6 +681,10 @@ int cpu_exec(CPUArchState *env)
>>               * local variables as longjmp is marked 'noreturn'. */
>>              cpu = current_cpu;
>>              env = cpu->env_ptr;
>> +#if !(defined(CONFIG_USER_ONLY) && \
>> +      (defined(TARGET_M68K) || defined(TARGET_PPC) || defined(TARGET_S390X)))
>> +            cc = CPU_GET_CLASS(cpu);
>> +#endif
> 
> This is a c compiler or libc bug -- the C standard says that this
> local variable should not be trashed by the longjmp. We were
> actually discussing removing the current workarounds there...

But we didn't decide if we should stop supporting the affected compiler
versions.

Does this issue also exist with the latest clang version available for
your platform?

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] cpu-exec(): also reload CPUClass *cc after longjmp return

[Juergen Lock]

On Fri, Oct 04, 2013 at 09:15:37AM +0200, Jan Kiszka wrote:
> On 2013-10-03 18:05, Peter Maydell wrote:
> > On 3 October 2013 23:09, Juergen Lock <qemu-l@jelal.kn-bremen.de> wrote:
> >> Local variable CPUClass *cc needs to be reloaded after return from longjmp
> >> too.  (This fixes the mips-softmmu crash observed on FreeBSD when qemu is
> >> built with clang.)
> >>
> >> Signed-off-by: Juergen Lock <nox@jelal.kn-bremen.de>
> >> Found-by: Dimitry Andric <dim@FreeBSD.org>
> >>
> >> --- a/cpu-exec.c
> >> +++ b/cpu-exec.c
> >> @@ -681,6 +681,10 @@ int cpu_exec(CPUArchState *env)
> >>               * local variables as longjmp is marked 'noreturn'. */
> >>              cpu = current_cpu;
> >>              env = cpu->env_ptr;
> >> +#if !(defined(CONFIG_USER_ONLY) && \
> >> +      (defined(TARGET_M68K) || defined(TARGET_PPC) || defined(TARGET_S390X)))
> >> +            cc = CPU_GET_CLASS(cpu);
> >> +#endif
> > 
> > This is a c compiler or libc bug -- the C standard says that this
> > local variable should not be trashed by the longjmp. We were
> > actually discussing removing the current workarounds there...
> 
> But we didn't decide if we should stop supporting the affected compiler
> versions.
> 
> Does this issue also exist with the latest clang version available for
> your platform?
> 
It happens with up to date clang as it's in FreeBSD 10.0-current
which is due for a release soon.  I think the clang folks are looking
into this issue but I don't know if a fix will make it into the
release...  (For now I've added the workaround to the FreeBSD
qemu-devel port.)

 Thanx,
	Juergen

Re: [Qemu-devel] [PATCH] cpu-exec(): also reload CPUClass *cc after longjmp return

[Stefan Weil]

Am 05.10.2013 19:54, schrieb Juergen Lock:
> On Fri, Oct 04, 2013 at 09:15:37AM +0200, Jan Kiszka wrote:
>> On 2013-10-03 18:05, Peter Maydell wrote:
>>> On 3 October 2013 23:09, Juergen Lock <qemu-l@jelal.kn-bremen.de> wrote:
>>>> Local variable CPUClass *cc needs to be reloaded after return from longjmp
>>>> too.  (This fixes the mips-softmmu crash observed on FreeBSD when qemu is
>>>> built with clang.)
>>>>
>>>> Signed-off-by: Juergen Lock <nox@jelal.kn-bremen.de>
>>>> Found-by: Dimitry Andric <dim@FreeBSD.org>
>>>>
>>>> --- a/cpu-exec.c
>>>> +++ b/cpu-exec.c
>>>> @@ -681,6 +681,10 @@ int cpu_exec(CPUArchState *env)
>>>>               * local variables as longjmp is marked 'noreturn'. */
>>>>              cpu = current_cpu;
>>>>              env = cpu->env_ptr;
>>>> +#if !(defined(CONFIG_USER_ONLY) && \
>>>> +      (defined(TARGET_M68K) || defined(TARGET_PPC) || defined(TARGET_S390X)))
>>>> +            cc = CPU_GET_CLASS(cpu);
>>>> +#endif
>>> This is a c compiler or libc bug -- the C standard says that this
>>> local variable should not be trashed by the longjmp. We were
>>> actually discussing removing the current workarounds there...
>> But we didn't decide if we should stop supporting the affected compiler
>> versions.
>>
>> Does this issue also exist with the latest clang version available for
>> your platform?
>>
> It happens with up to date clang as it's in FreeBSD 10.0-current
> which is due for a release soon.  I think the clang folks are looking
> into this issue but I don't know if a fix will make it into the
> release...  (For now I've added the workaround to the FreeBSD
> qemu-devel port.)
>
>  Thanx,
> 	Juergen


Could you try whether QEMU crashes when it was configured with
TCG interpreter (--enable-tcg-interpreter)? If it does not crash, it
might be that TCG does not save / restore enough registers.

Which register is used for the local variable 'cc'?

Regards,
Stefan

Re: [Qemu-devel] [PATCH] cpu-exec(): also reload CPUClass *cc after longjmp return

[Juergen Lock]

On Sat, Oct 05, 2013 at 08:06:22PM +0200, Stefan Weil wrote:
> Am 05.10.2013 19:54, schrieb Juergen Lock:
> > On Fri, Oct 04, 2013 at 09:15:37AM +0200, Jan Kiszka wrote:
> >> On 2013-10-03 18:05, Peter Maydell wrote:
> >>> On 3 October 2013 23:09, Juergen Lock <qemu-l@jelal.kn-bremen.de> wrote:
> >>>> Local variable CPUClass *cc needs to be reloaded after return from longjmp
> >>>> too.  (This fixes the mips-softmmu crash observed on FreeBSD when qemu is
> >>>> built with clang.)
> >>>>
> >>>> Signed-off-by: Juergen Lock <nox@jelal.kn-bremen.de>
> >>>> Found-by: Dimitry Andric <dim@FreeBSD.org>
> >>>>
> >>>> --- a/cpu-exec.c
> >>>> +++ b/cpu-exec.c
> >>>> @@ -681,6 +681,10 @@ int cpu_exec(CPUArchState *env)
> >>>>               * local variables as longjmp is marked 'noreturn'. */
> >>>>              cpu = current_cpu;
> >>>>              env = cpu->env_ptr;
> >>>> +#if !(defined(CONFIG_USER_ONLY) && \
> >>>> +      (defined(TARGET_M68K) || defined(TARGET_PPC) || defined(TARGET_S390X)))
> >>>> +            cc = CPU_GET_CLASS(cpu);
> >>>> +#endif
> >>> This is a c compiler or libc bug -- the C standard says that this
> >>> local variable should not be trashed by the longjmp. We were
> >>> actually discussing removing the current workarounds there...
> >> But we didn't decide if we should stop supporting the affected compiler
> >> versions.
> >>
> >> Does this issue also exist with the latest clang version available for
> >> your platform?
> >>
> > It happens with up to date clang as it's in FreeBSD 10.0-current
> > which is due for a release soon.  I think the clang folks are looking
> > into this issue but I don't know if a fix will make it into the
> > release...  (For now I've added the workaround to the FreeBSD
> > qemu-devel port.)
> >
> >  Thanx,
> > 	Juergen
> 
> 
> Could you try whether QEMU crashes when it was configured with
> TCG interpreter (--enable-tcg-interpreter)? If it does not crash, it
> might be that TCG does not save / restore enough registers.
> 
Still crashes the same.

> Which register is used for the local variable 'cc'?
> 
 Here is the original debug log with part of the disassembly:

	http://people.freebsd.org/~nox/tmp/qemu-1.6.0-mips-softmmu-crash.txt

(I wrote the comment at the top before I knew cc needs to be reloaded...)

 So apparently cc gets loaded from the stack before the crash: -0x40(%rbp)

 Thanx,
	Juergen

Re: [Qemu-devel] [PATCH] cpu-exec(): also reload CPUClass *cc after longjmp return

[Andreas Färber]

Am 05.10.2013 23:45, schrieb Juergen Lock:
> On Sat, Oct 05, 2013 at 08:06:22PM +0200, Stefan Weil wrote:
>> Am 05.10.2013 19:54, schrieb Juergen Lock:
>>> On Fri, Oct 04, 2013 at 09:15:37AM +0200, Jan Kiszka wrote:
>>>> On 2013-10-03 18:05, Peter Maydell wrote:
>>>>> On 3 October 2013 23:09, Juergen Lock <qemu-l@jelal.kn-bremen.de> wrote:
>>>>>> Local variable CPUClass *cc needs to be reloaded after return from longjmp
>>>>>> too.  (This fixes the mips-softmmu crash observed on FreeBSD when qemu is
>>>>>> built with clang.)
>>>>>>
>>>>>> Signed-off-by: Juergen Lock <nox@jelal.kn-bremen.de>
>>>>>> Found-by: Dimitry Andric <dim@FreeBSD.org>
>>>>>>
>>>>>> --- a/cpu-exec.c
>>>>>> +++ b/cpu-exec.c
>>>>>> @@ -681,6 +681,10 @@ int cpu_exec(CPUArchState *env)
>>>>>>               * local variables as longjmp is marked 'noreturn'. */
>>>>>>              cpu = current_cpu;
>>>>>>              env = cpu->env_ptr;
>>>>>> +#if !(defined(CONFIG_USER_ONLY) && \
>>>>>> +      (defined(TARGET_M68K) || defined(TARGET_PPC) || defined(TARGET_S390X)))
>>>>>> +            cc = CPU_GET_CLASS(cpu);
>>>>>> +#endif
>>>>> This is a c compiler or libc bug -- the C standard says that this
>>>>> local variable should not be trashed by the longjmp. We were
>>>>> actually discussing removing the current workarounds there...
>>>> But we didn't decide if we should stop supporting the affected compiler
>>>> versions.
>>>>
>>>> Does this issue also exist with the latest clang version available for
>>>> your platform?
>>>>
>>> It happens with up to date clang as it's in FreeBSD 10.0-current
>>> which is due for a release soon.  I think the clang folks are looking
>>> into this issue but I don't know if a fix will make it into the
>>> release...  (For now I've added the workaround to the FreeBSD
>>> qemu-devel port.)
>>>
>>>  Thanx,
>>> 	Juergen
>>
>>
>> Could you try whether QEMU crashes when it was configured with
>> TCG interpreter (--enable-tcg-interpreter)? If it does not crash, it
>> might be that TCG does not save / restore enough registers.
>>
> Still crashes the same.

Practical bugfix beats theoretical optimization, so I'm queuing the
patch (w/ message tweaked) until someone comes up with a better one:
https://github.com/afaerber/qemu-cpu/commits/qom-cpu

Thanks,
Andreas

> 
>> Which register is used for the local variable 'cc'?
>>
>  Here is the original debug log with part of the disassembly:
> 
> 	http://people.freebsd.org/~nox/tmp/qemu-1.6.0-mips-softmmu-crash.txt
> 
> (I wrote the comment at the top before I knew cc needs to be reloaded...)
> 
>  So apparently cc gets loaded from the stack before the crash: -0x40(%rbp)
> 
>  Thanx,
> 	Juergen
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg