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

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