[Qemu-devel] [PATCH v2] cpu-exec: Fix compiler warning (-Werror=clobbered)

[Qemu-devel] [PATCH v2] cpu-exec: Fix compiler warning (-Werror=clobbered)

[Stefan Weil]

Reloading of local variables after sigsetjmp is only needed for some
buggy compilers.

The code which should reload these variables causes compiler warnings
with gcc 4.7 when compiler optimizations are enabled:

cpu-exec.c:204:15: error:
 variable ‘cpu’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered]
cpu-exec.c:207:15: error:
 variable ‘cc’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered]
cpu-exec.c:202:28: error:
 argument ‘env’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered]

Now this code is only used for compilers which need it
(and gcc 4.5.x, x > 0 which does not need it but won't give warnings).

There were bug reports for clang and gcc 4.5.0, while gcc 4.5.1
was reported to work fine without the reload code.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---

v2: Don't remove the code which causes the warnings, but use it
    only with clang or gcc < 4.6.

 cpu-exec.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index 30cfa2a..fec20c3 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -677,14 +677,18 @@ int cpu_exec(CPUArchState *env)
                    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'. */
+#if defined(__clang__) || !QEMU_GNUC_PREREQ(4, 6)
+            /* Some compilers wrongly smash all local variables after
+             * siglongjmp. There were bug reports for gcc 4.5.0 and clang.
+             * Reload essential local variables here for those compilers.
+             * gcc 4.7 would complain about this code (-Wclobbered). */
             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
+#endif /* __clang__ or old gcc */
         }
     } /* for(;;) */
 
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH v2] cpu-exec: Fix compiler warning (-Werror=clobbered)

[Jan Kiszka]

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

On 2013-10-31 20:31, Stefan Weil wrote:
> Reloading of local variables after sigsetjmp is only needed for some
> buggy compilers.
> 
> The code which should reload these variables causes compiler warnings
> with gcc 4.7 when compiler optimizations are enabled:
> 
> cpu-exec.c:204:15: error:
>  variable ‘cpu’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered]
> cpu-exec.c:207:15: error:
>  variable ‘cc’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered]
> cpu-exec.c:202:28: error:
>  argument ‘env’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered]
> 
> Now this code is only used for compilers which need it
> (and gcc 4.5.x, x > 0 which does not need it but won't give warnings).
> 
> There were bug reports for clang and gcc 4.5.0, while gcc 4.5.1
> was reported to work fine without the reload code.
> 
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
> 
> v2: Don't remove the code which causes the warnings, but use it
>     only with clang or gcc < 4.6.
> 
>  cpu-exec.c |    8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/cpu-exec.c b/cpu-exec.c
> index 30cfa2a..fec20c3 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -677,14 +677,18 @@ int cpu_exec(CPUArchState *env)
>                     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'. */
> +#if defined(__clang__) || !QEMU_GNUC_PREREQ(4, 6)
> +            /* Some compilers wrongly smash all local variables after
> +             * siglongjmp. There were bug reports for gcc 4.5.0 and clang.
> +             * Reload essential local variables here for those compilers.
> +             * gcc 4.7 would complain about this code (-Wclobbered). */
>              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
> +#endif /* __clang__ or old gcc */
>          }
>      } /* for(;;) */
>  
> 

Are all clang versions affected? Then this looks reasonable.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [Qemu-devel] [PATCH v2] cpu-exec: Fix compiler warning (-Werror=clobbered)

[Stefan Weil]

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

Am 31.10.2013 20:41, schrieb Jan Kiszka:
> On 2013-10-31 20:31, Stefan Weil wrote:
>> Reloading of local variables after sigsetjmp is only needed for some
>> buggy compilers.
>>
>> The code which should reload these variables causes compiler warnings
>> with gcc 4.7 when compiler optimizations are enabled:
>>
>> cpu-exec.c:204:15: error:
>>  variable ‘cpu’ might be clobbered by ‘longjmp’ or ‘vfork’
[-Werror=clobbered]
>> cpu-exec.c:207:15: error:
>>  variable ‘cc’ might be clobbered by ‘longjmp’ or ‘vfork’
[-Werror=clobbered]
>> cpu-exec.c:202:28: error:
>>  argument ‘env’ might be clobbered by ‘longjmp’ or ‘vfork’
[-Werror=clobbered]
>>
>> Now this code is only used for compilers which need it
>> (and gcc 4.5.x, x > 0 which does not need it but won't give warnings).
>>
>> There were bug reports for clang and gcc 4.5.0, while gcc 4.5.1
>> was reported to work fine without the reload code.
>>
>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>> ---
>>
>> v2: Don't remove the code which causes the warnings, but use it
>>     only with clang or gcc < 4.6.
>>
>>  cpu-exec.c |    8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/cpu-exec.c b/cpu-exec.c
>> index 30cfa2a..fec20c3 100644
>> --- a/cpu-exec.c
>> +++ b/cpu-exec.c
>> @@ -677,14 +677,18 @@ int cpu_exec(CPUArchState *env)
>>                     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'. */
>> +#if defined(__clang__) || !QEMU_GNUC_PREREQ(4, 6)
>> +            /* Some compilers wrongly smash all local variables after
>> +             * siglongjmp. There were bug reports for gcc 4.5.0 and
clang.
>> +             * Reload essential local variables here for those
compilers.
>> +             * gcc 4.7 would complain about this code (-Wclobbered). */
>>              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
>> +#endif /* __clang__ or old gcc */
>>          }
>>      } /* for(;;) */
>> 
>>
>
> Are all clang versions affected? Then this looks reasonable.
>
> Jan

Commit 6c78f29a2424622bfc9c30dfbbc13404481eacb6only says that
there was a "crash observed on FreeBSD when QEMU is built with clang",
so I don't know which versions are affected.

For compilers which don't show the "clobbered" warning, the reload code
does not harm even if it is unneeded, and including that code for all
versions of clang
is compatible with the status quo.

Stefan





[-- Attachment #2: Type: text/html, Size: 4902 bytes --]

Re: [Qemu-devel] [PATCH v2] cpu-exec: Fix compiler warning (-Werror=clobbered)

[Stefan Weil]

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

Am 31.10.2013 20:41, schrieb Jan Kiszka:
> On 2013-10-31 20:31, Stefan Weil wrote:
>> Reloading of local variables after sigsetjmp is only needed for some
>> buggy compilers.
>>
>> The code which should reload these variables causes compiler warnings
>> with gcc 4.7 when compiler optimizations are enabled:
>>
>> cpu-exec.c:204:15: error:
>>  variable ‘cpu’ might be clobbered by ‘longjmp’ or ‘vfork’
[-Werror=clobbered]
>> cpu-exec.c:207:15: error:
>>  variable ‘cc’ might be clobbered by ‘longjmp’ or ‘vfork’
[-Werror=clobbered]
>> cpu-exec.c:202:28: error:
>>  argument ‘env’ might be clobbered by ‘longjmp’ or ‘vfork’
[-Werror=clobbered]
>>
>> Now this code is only used for compilers which need it
>> (and gcc 4.5.x, x > 0 which does not need it but won't give warnings).
>>
>> There were bug reports for clang and gcc 4.5.0, while gcc 4.5.1
>> was reported to work fine without the reload code.
>>
>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>> ---
>>
>> v2: Don't remove the code which causes the warnings, but use it
>>     only with clang or gcc < 4.6.
>>
>>  cpu-exec.c |    8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/cpu-exec.c b/cpu-exec.c
>> index 30cfa2a..fec20c3 100644
>> --- a/cpu-exec.c
>> +++ b/cpu-exec.c
>> @@ -677,14 +677,18 @@ int cpu_exec(CPUArchState *env)
>>                     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'. */
>> +#if defined(__clang__) || !QEMU_GNUC_PREREQ(4, 6)
>> +            /* Some compilers wrongly smash all local variables after
>> +             * siglongjmp. There were bug reports for gcc 4.5.0 and
clang.
>> +             * Reload essential local variables here for those
compilers.
>> +             * gcc 4.7 would complain about this code (-Wclobbered). */
>>              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
>> +#endif /* __clang__ or old gcc */
>>          }
>>      } /* for(;;) */
>> 
>>
>
> Are all clang versions affected? Then this looks reasonable.
>
> Jan

Ping?

As cpu-exec.c has no explicit maintainer, I'd add this patch to my next
pull request, if nobody minds, but I'd appreciate more comments or a
Reviewed-by of course.

Stefan



[-- Attachment #2: Type: text/html, Size: 4237 bytes --]

Re: [Qemu-devel] [PATCH v2] cpu-exec: Fix compiler warning (-Werror=clobbered)

[Peter Maydell]

On 5 November 2013 17:52, Stefan Weil <sw@weilnetz.de> wrote:
> Am 31.10.2013 20:41, schrieb Jan Kiszka:
>> On 2013-10-31 20:31, Stefan Weil wrote:
>>> Reloading of local variables after sigsetjmp is only needed for some
>>> buggy compilers.
>>>
>>> The code which should reload these variables causes compiler warnings
>>> with gcc 4.7 when compiler optimizations are enabled:
>>>
>>> cpu-exec.c:204:15: error:
>>>  variable ‘cpu’ might be clobbered by ‘longjmp’ or ‘vfork’
>>> [-Werror=clobbered]
>>> cpu-exec.c:207:15: error:
>>>  variable ‘cc’ might be clobbered by ‘longjmp’ or ‘vfork’
>>> [-Werror=clobbered]
>>> cpu-exec.c:202:28: error:
>>>  argument ‘env’ might be clobbered by ‘longjmp’ or ‘vfork’
>>> [-Werror=clobbered]
>>>
>>> Now this code is only used for compilers which need it
>>> (and gcc 4.5.x, x > 0 which does not need it but won't give warnings).
>>>
>>> There were bug reports for clang and gcc 4.5.0, while gcc 4.5.1
>>> was reported to work fine without the reload code.
>>>
>>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>>> ---
>>>
>>> v2: Don't remove the code which causes the warnings, but use it
>>>     only with clang or gcc < 4.6.
>>>
>>>  cpu-exec.c |    8 ++++++--
>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/cpu-exec.c b/cpu-exec.c
>>> index 30cfa2a..fec20c3 100644
>>> --- a/cpu-exec.c
>>> +++ b/cpu-exec.c
>>> @@ -677,14 +677,18 @@ int cpu_exec(CPUArchState *env)
>>>                     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'. */
>>> +#if defined(__clang__) || !QEMU_GNUC_PREREQ(4, 6)
>>> +            /* Some compilers wrongly smash all local variables after
>>> +             * siglongjmp. There were bug reports for gcc 4.5.0 and
>>> clang.
>>> +             * Reload essential local variables here for those
>>> compilers.
>>> +             * gcc 4.7 would complain about this code (-Wclobbered). */
>>>              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
>>> +#endif /* __clang__ or old gcc */
>>>          }
>>>      } /* for(;;) */
>>>
>>>
>>
>> Are all clang versions affected? Then this looks reasonable.
>>
>> Jan
>
> Ping?
>
> As cpu-exec.c has no explicit maintainer, I'd add this patch to my next pull
> request, if nobody minds, but I'd appreciate more comments or a Reviewed-by
> of course.

Not a blocking of this patch, but it occured to me that maybe
we could have an assert in the #else path here to catch other
buggy compilers.

-- PMM

Re: [Qemu-devel] [PATCH v2] cpu-exec: Fix compiler warning (-Werror=clobbered)

[Andreas Färber]

Am 05.11.2013 18:52, schrieb Stefan Weil:
> Am 31.10.2013 20:41, schrieb Jan Kiszka:
>> On 2013-10-31 20:31, Stefan Weil wrote:
>>> Reloading of local variables after sigsetjmp is only needed for some
>>> buggy compilers.
>>>
>>> The code which should reload these variables causes compiler warnings
>>> with gcc 4.7 when compiler optimizations are enabled:
>>>
>>> cpu-exec.c:204:15: error:
>>>  variable ‘cpu’ might be clobbered by ‘longjmp’ or ‘vfork’
> [-Werror=clobbered]
>>> cpu-exec.c:207:15: error:
>>>  variable ‘cc’ might be clobbered by ‘longjmp’ or ‘vfork’
> [-Werror=clobbered]
>>> cpu-exec.c:202:28: error:
>>>  argument ‘env’ might be clobbered by ‘longjmp’ or ‘vfork’
> [-Werror=clobbered]
>>>
>>> Now this code is only used for compilers which need it
>>> (and gcc 4.5.x, x > 0 which does not need it but won't give warnings).
>>>
>>> There were bug reports for clang and gcc 4.5.0, while gcc 4.5.1
>>> was reported to work fine without the reload code.
>>>
>>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>>> ---
>>>
>>> v2: Don't remove the code which causes the warnings, but use it
>>>     only with clang or gcc < 4.6.
>>>
>>>  cpu-exec.c |    8 ++++++--
>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/cpu-exec.c b/cpu-exec.c
>>> index 30cfa2a..fec20c3 100644
>>> --- a/cpu-exec.c
>>> +++ b/cpu-exec.c
>>> @@ -677,14 +677,18 @@ int cpu_exec(CPUArchState *env)
>>>                     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'. */
>>> +#if defined(__clang__) || !QEMU_GNUC_PREREQ(4, 6)
>>> +            /* Some compilers wrongly smash all local variables after
>>> +             * siglongjmp. There were bug reports for gcc 4.5.0 and
> clang.
>>> +             * Reload essential local variables here for those
> compilers.
>>> +             * gcc 4.7 would complain about this code (-Wclobbered). */
>>>              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
>>> +#endif /* __clang__ or old gcc */
>>>          }
>>>      } /* for(;;) */
>>> 
>>>
>>
>> Are all clang versions affected? Then this looks reasonable.
>>
>> Jan
> 
> Ping?
> 
> As cpu-exec.c has no explicit maintainer, I'd add this patch to my next
> pull request, if nobody minds, but I'd appreciate more comments or a
> Reviewed-by of course.

I feel kind of responsable for this as CPU maintainer, but I wasn't
CC'ed and have not been following the list so closely lately. ;)

It seems the person reporting this for FreeBSD hasn't been CC'ed either?

Having applied the previous patch complementing the reload, I'm
generally okay with #ifdef'ing it out. But I'd be happier if Jan and/or
Peter or anyone else would provide some *-by, including Tested-by.

Regards,
Andreas

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

Re: [Qemu-devel] [PATCH v2] cpu-exec: Fix compiler warning (-Werror=clobbered)

[Stefan Weil]

Am 05.11.2013 19:03, schrieb Andreas Färber:
> Am 05.11.2013 18:52, schrieb Stefan Weil:
>> Am 31.10.2013 20:41, schrieb Jan Kiszka:
>>> On 2013-10-31 20:31, Stefan Weil wrote:
>>>> Reloading of local variables after sigsetjmp is only needed for some
>>>> buggy compilers.
>>>>
>>>> The code which should reload these variables causes compiler warnings
>>>> with gcc 4.7 when compiler optimizations are enabled:
>>>>
>>>> cpu-exec.c:204:15: error:
>>>>  variable ‘cpu’ might be clobbered by ‘longjmp’ or ‘vfork’
>> [-Werror=clobbered]
>>>> cpu-exec.c:207:15: error:
>>>>  variable ‘cc’ might be clobbered by ‘longjmp’ or ‘vfork’
>> [-Werror=clobbered]
>>>> cpu-exec.c:202:28: error:
>>>>  argument ‘env’ might be clobbered by ‘longjmp’ or ‘vfork’
>> [-Werror=clobbered]
>>>> Now this code is only used for compilers which need it
>>>> (and gcc 4.5.x, x > 0 which does not need it but won't give warnings).
>>>>
>>>> There were bug reports for clang and gcc 4.5.0, while gcc 4.5.1
>>>> was reported to work fine without the reload code.
>>>>
>>>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>>>> ---
>>>>
>>>> v2: Don't remove the code which causes the warnings, but use it
>>>>     only with clang or gcc < 4.6.
>>>>
>>>>  cpu-exec.c |    8 ++++++--
>>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/cpu-exec.c b/cpu-exec.c
>>>> index 30cfa2a..fec20c3 100644
>>>> --- a/cpu-exec.c
>>>> +++ b/cpu-exec.c
>>>> @@ -677,14 +677,18 @@ int cpu_exec(CPUArchState *env)
>>>>                     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'. */
>>>> +#if defined(__clang__) || !QEMU_GNUC_PREREQ(4, 6)
>>>> +            /* Some compilers wrongly smash all local variables after
>>>> +             * siglongjmp. There were bug reports for gcc 4.5.0 and
>> clang.
>>>> +             * Reload essential local variables here for those
>> compilers.
>>>> +             * gcc 4.7 would complain about this code (-Wclobbered). */
>>>>              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
>>>> +#endif /* __clang__ or old gcc */
>>>>          }
>>>>      } /* for(;;) */
>>>>
>>>>
>>> Are all clang versions affected? Then this looks reasonable.
>>>
>>> Jan
>> Ping?
>>
>> As cpu-exec.c has no explicit maintainer, I'd add this patch to my next
>> pull request, if nobody minds, but I'd appreciate more comments or a
>> Reviewed-by of course.
> I feel kind of responsable for this as CPU maintainer, but I wasn't
> CC'ed and have not been following the list so closely lately. ;)
>
> It seems the person reporting this for FreeBSD hasn't been CC'ed either?
>
> Having applied the previous patch complementing the reload, I'm
> generally okay with #ifdef'ing it out. But I'd be happier if Jan and/or
> Peter or anyone else would provide some *-by, including Tested-by.
>
> Regards,
> Andreas

Tested-by is not really useful here because the code change is trivial:

Either you have a clang compiler (which sets __clang__, already used
in other parts of QEMU), or you have an old gcc- in both cases the
compilation result remains unchanged.

Or you have a compiler which never needed the additional code.
Then QEMU will work like it did before the reload code was added.

Testing might be useful if anybody had the time to install and test lots of
gcc and clang (and maybe others) compiler versions, but I am rather
sure that nobody wants to do this :-)

Adding assertions in an #else part as Peter suggested is possible.
I have them in my QEMU repository, too. Feel free to add them, or
I can also send a v3 if this is needed.

I cc this mail to Dimitry who reported the clang problem and to Jürgen
who fixed it.

Regards,
Stefan