[v3] cpu-exec: Fix compiler warning (-Werror=clobbered)

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. For clang it
is not clear which versions are affected, so simply keep the status quo
for all clang compilations. This can be improved later.

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.

v3: Add assertions for compilers which hopefully don't smash variables
    (suggested by Peter Maydell).

I started v1 of this patch two years ago to prepare support for
builds with compiler option -Wextra.

See http://patchwork.ozlabs.org/patch/287593/ for the latest
discussion on this issue.


 cpu-exec.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

On 26 Sep 2015, at 13:23, Stefan Weil <sw@weilnetz.de> wrote:
> 
> Reloading of local variables after sigsetjmp is only needed for some
> buggy compilers.

I don't think the compilers are buggy; any non-volatile local variable that is changed between setjmp() and longjmp() is indeterminate.  Quoting C99 7.13.2.1:

	• All accessible objects have values, and all other components of the abstract machine(209) have state, as of the time the longjmp function was called, except that the values of objects of automatic storage duration that are local to the function containing the invocation of the corresponding setjmp macro that do not have volatile-qualified type and have been changed between the setjmp invocation and longjmp call are indeterminate.

Can you be 100% sure these variables are never changed anywhere after the setjmp() call?


> 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. For clang it
> is not clear which versions are affected, so simply keep the status quo
> for all clang compilations. This can be improved later.

Maybe you can just mark the variables in question volatile.  That will ensure they will not be indeterminate.

-Dimitry

On 26 September 2015 at 08:33, Dimitry Andric <dim@freebsd.org> wrote:
> On 26 Sep 2015, at 13:23, Stefan Weil <sw@weilnetz.de> wrote:
>>
>> Reloading of local variables after sigsetjmp is only needed for some
>> buggy compilers.
>
> I don't think the compilers are buggy; any non-volatile local variable that is changed between setjmp() and longjmp() is indeterminate.  Quoting C99 7.13.2.1:
>
>         • All accessible objects have values, and all other components of the abstract machine(209) have state, as of the time the longjmp function was called, except that the values of objects of automatic storage duration that are local to the function containing the invocation of the corresponding setjmp macro that do not have volatile-qualified type and have been changed between the setjmp invocation and longjmp call are indeterminate.
>
> Can you be 100% sure these variables are never changed anywhere after the setjmp() call?

Yes; they're only set on entry to the function before setjmp().
This is why the compilers in question are buggy -- the variables
haven't been changed, so they should be preserved, but some
compilers seemed to trash them. Thus the "restore values"
code which is working around the compiler bug.

Unfortunately now newer compilers are complaining via Wclobbered
about the "restore values" code (they're not smart enough to
be able to tell that that bit of code is only executed after
a longjmp and before we go back around to do the next setjmp).

thanks
-- PMM

Am 26.09.2015 um 13:23 schrieb 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. For clang it
> is not clear which versions are affected, so simply keep the status quo
> for all clang compilations. This can be improved later.
> 
> 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.
> 
> v3: Add assertions for compilers which hopefully don't smash variables
>     (suggested by Peter Maydell).
> 
> I started v1 of this patch two years ago to prepare support for
> builds with compiler option -Wextra.
> 
> See http://patchwork.ozlabs.org/patch/287593/ for the latest
> discussion on this issue.
> 
> 
>  cpu-exec.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/cpu-exec.c b/cpu-exec.c
> index 8fd56a6..7dab85a 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -538,15 +538,27 @@ int cpu_exec(CPUState *cpu)
>                     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.
> +             * Newer versions of gcc would complain about this code (-Wclobbered). */
>              cpu = current_cpu;
>              cc = CPU_GET_CLASS(cpu);
> -            cpu->can_do_io = 1;
>  #ifdef TARGET_I386
>              x86_cpu = X86_CPU(cpu);
>              env = &x86_cpu->env;
>  #endif
> +#else /* buggy compiler */
> +            /* Assert that the compiler does not smash local variables. */
> +            g_assert(cpu == current_cpu);
> +            g_assert(cc == CPU_GET_CLASS(cpu));
> +#ifdef TARGET_I386
> +            g_assert(x86_cpu == X86_CPU(cpu));
> +            g_assert(env == &x86_cpu->env);
> +#endif
> +#endif /* buggy compiler */
> +            cpu->can_do_io = 1;
>              tb_lock_reset();
>          }
>      } /* for(;;) */
> 


Ping. Is there any chance to get this patch into version 2.5?
I'd be happy to remove this 2 year old issue from my list of
open patches.

Regards,

Stefan

On 27/10/2015 19:31, Stefan Weil wrote:
> Am 26.09.2015 um 13:23 schrieb 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. For clang it
>> is not clear which versions are affected, so simply keep the status quo
>> for all clang compilations. This can be improved later.
>>
>> 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.
>>
>> v3: Add assertions for compilers which hopefully don't smash variables
>>     (suggested by Peter Maydell).
>>
>> I started v1 of this patch two years ago to prepare support for
>> builds with compiler option -Wextra.
>>
>> See http://patchwork.ozlabs.org/patch/287593/ for the latest
>> discussion on this issue.
>>
>>
>>  cpu-exec.c | 18 +++++++++++++++---
>>  1 file changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/cpu-exec.c b/cpu-exec.c
>> index 8fd56a6..7dab85a 100644
>> --- a/cpu-exec.c
>> +++ b/cpu-exec.c
>> @@ -538,15 +538,27 @@ int cpu_exec(CPUState *cpu)
>>                     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.
>> +             * Newer versions of gcc would complain about this code (-Wclobbered). */
>>              cpu = current_cpu;
>>              cc = CPU_GET_CLASS(cpu);
>> -            cpu->can_do_io = 1;
>>  #ifdef TARGET_I386
>>              x86_cpu = X86_CPU(cpu);
>>              env = &x86_cpu->env;
>>  #endif
>> +#else /* buggy compiler */
>> +            /* Assert that the compiler does not smash local variables. */
>> +            g_assert(cpu == current_cpu);
>> +            g_assert(cc == CPU_GET_CLASS(cpu));
>> +#ifdef TARGET_I386
>> +            g_assert(x86_cpu == X86_CPU(cpu));
>> +            g_assert(env == &x86_cpu->env);
>> +#endif
>> +#endif /* buggy compiler */
>> +            cpu->can_do_io = 1;
>>              tb_lock_reset();
>>          }
>>      } /* for(;;) */
>>
> 
> 
> Ping. Is there any chance to get this patch into version 2.5?
> I'd be happy to remove this 2 year old issue from my list of
> open patches.

Yes, I'll send a pull request next week.

Paolo