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

[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 <address@hidden>
>> ---
>>
>> 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