diff mbox

cpu-exec(): also reload CPUClass *cc after longjmp return

Message ID 20131003140937.GA59761@enceladus10.kn-bremen.de
State New
Headers show

Commit Message

Juergen Lock Oct. 3, 2013, 2:09 p.m. UTC 
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>

Comments

Peter Maydell Oct. 3, 2013, 4:05 p.m. UTC | #1 
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
Jan Kiszka Oct. 4, 2013, 7:15 a.m. UTC | #2 
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
Juergen Lock Oct. 5, 2013, 5:54 p.m. UTC | #3 
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
Stefan Weil Oct. 5, 2013, 6:06 p.m. UTC | #4 
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
Juergen Lock Oct. 5, 2013, 9:45 p.m. UTC | #5 
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
Andreas Färber Oct. 7, 2013, 7:28 a.m. UTC | #6 
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
>
diff mbox

Patch

--- 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(;;) */