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

["Andreas Färber" <afaerber@suse.de>]

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