[Qemu-devel] [PATCH] Use special code for sigsetjmp only in cpu-exec.c

[Qemu-devel] [PATCH] Use special code for sigsetjmp only in cpu-exec.c

[Stefan Weil]

The rest of the code can use longjmp with stack unwinding.

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

This is a bug fix needed for 64 bit Windows.

QEMU for Windows currently gets the wrong definition for
sigsetjmp. It uses stack unwinding for longjmp which results
in a crash when it is called from generated code.

Thanks to Andrew Baumann for his reminder that this patch was
still missing. Andrew, could you please test it with your
RPi emulation?

Regards,
Stefan

 cpu-exec.c                | 9 +++++++++
 include/sysemu/os-win32.h | 8 --------
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index fd92452..6a725e0 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -33,6 +33,15 @@
 #endif
 #include "sysemu/replay.h"
 
+#if defined(_WIN64)
+/* On w64, sigsetjmp is implemented by _setjmp which needs a second parameter.
+ * If this parameter is NULL, longjump does no stack unwinding.
+ * That is what we need for QEMU. Passing the value of register rsp (default)
+ * lets longjmp try a stack unwinding which will crash with generated code. */
+#undef sigsetjmp
+#define sigsetjmp(env, savesigs) _setjmp(env, NULL)
+#endif
+
 /* -icount align implementation. */
 
 typedef struct SyncClocks {
diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h
index fbed346..b151e74 100644
--- a/include/sysemu/os-win32.h
+++ b/include/sysemu/os-win32.h
@@ -55,14 +55,6 @@
 # define EWOULDBLOCK  WSAEWOULDBLOCK
 #endif
 
-#if defined(_WIN64)
-/* On w64, setjmp is implemented by _setjmp which needs a second parameter.
- * If this parameter is NULL, longjump does no stack unwinding.
- * That is what we need for QEMU. Passing the value of register rsp (default)
- * lets longjmp try a stack unwinding which will crash with generated code. */
-# undef setjmp
-# define setjmp(env) _setjmp(env, NULL)
-#endif
 /* QEMU uses sigsetjmp()/siglongjmp() as the portable way to specify
  * "longjmp and don't touch the signal masks". Since we know that the
  * savemask parameter will always be zero we can safely define these
-- 
2.1.4

Re: [Qemu-devel] [PATCH] Use special code for sigsetjmp only in cpu-exec.c

[Andrew Baumann]

> From: Stefan Weil [mailto:sw@weilnetz.de]
> Sent: Monday, 29 February 2016 9:08 PM
> 
> The rest of the code can use longjmp with stack unwinding.
> 
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
> 
> This is a bug fix needed for 64 bit Windows.
> 
> QEMU for Windows currently gets the wrong definition for
> sigsetjmp. It uses stack unwinding for longjmp which results
> in a crash when it is called from generated code.
> 
> Thanks to Andrew Baumann for his reminder that this patch was
> still missing. Andrew, could you please test it with your
> RPi emulation?
> 
> Regards,
> Stefan
> 
>  cpu-exec.c                | 9 +++++++++
>  include/sysemu/os-win32.h | 8 --------
>  2 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/cpu-exec.c b/cpu-exec.c
> index fd92452..6a725e0 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -33,6 +33,15 @@
>  #endif
>  #include "sysemu/replay.h"
> 
> +#if defined(_WIN64)
> +/* On w64, sigsetjmp is implemented by _setjmp which needs a second
> parameter.
> + * If this parameter is NULL, longjump does no stack unwinding.
> + * That is what we need for QEMU. Passing the value of register rsp
> (default)
> + * lets longjmp try a stack unwinding which will crash with generated code.
> */
> +#undef sigsetjmp
> +#define sigsetjmp(env, savesigs) _setjmp(env, NULL)
> +#endif
> +
>  /* -icount align implementation. */
> 
>  typedef struct SyncClocks {
> diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h
> index fbed346..b151e74 100644
> --- a/include/sysemu/os-win32.h
> +++ b/include/sysemu/os-win32.h
> @@ -55,14 +55,6 @@
>  # define EWOULDBLOCK  WSAEWOULDBLOCK
>  #endif
> 
> -#if defined(_WIN64)
> -/* On w64, setjmp is implemented by _setjmp which needs a second
> parameter.
> - * If this parameter is NULL, longjump does no stack unwinding.
> - * That is what we need for QEMU. Passing the value of register rsp (default)
> - * lets longjmp try a stack unwinding which will crash with generated code.
> */
> -# undef setjmp
> -# define setjmp(env) _setjmp(env, NULL)
> -#endif
>  /* QEMU uses sigsetjmp()/siglongjmp() as the portable way to specify
>   * "longjmp and don't touch the signal masks". Since we know that the
>   * savemask parameter will always be zero we can safely define these
> --
> 2.1.4

Thanks Stefan. This works for me with a mingw64 native build.

Tested-by: Andrew Baumann <Andrew.Baumann@microsoft.com>

Andrew

Re: [Qemu-devel] [PATCH] Use special code for sigsetjmp only in cpu-exec.c

[Peter Maydell]

On 1 March 2016 at 05:07, Stefan Weil <sw@weilnetz.de> wrote:
> The rest of the code can use longjmp with stack unwinding.
>
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>
> This is a bug fix needed for 64 bit Windows.
>
> QEMU for Windows currently gets the wrong definition for
> sigsetjmp. It uses stack unwinding for longjmp which results
> in a crash when it is called from generated code.
>
> Thanks to Andrew Baumann for his reminder that this patch was
> still missing. Andrew, could you please test it with your
> RPi emulation?

I don't understand this patch. Why doesn't it work to have
sigsetjmp() be implemented the same way for every use that
QEMU makes of it?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] Use special code for sigsetjmp only in cpu-exec.c

[Stefan Weil]

Am 01.03.2016 um 10:59 schrieb Peter Maydell:
> On 1 March 2016 at 05:07, Stefan Weil <sw@weilnetz.de> wrote:
>> The rest of the code can use longjmp with stack unwinding.
>>
>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>> ---
>>
>> This is a bug fix needed for 64 bit Windows.
>>
>> QEMU for Windows currently gets the wrong definition for
>> sigsetjmp. It uses stack unwinding for longjmp which results
>> in a crash when it is called from generated code.
>>
>> Thanks to Andrew Baumann for his reminder that this patch was
>> still missing. Andrew, could you please test it with your
>> RPi emulation?
> I don't understand this patch. Why doesn't it work to have
> sigsetjmp() be implemented the same way for every use that
> QEMU makes of it?
>
> thanks
> -- PMM

It does, as long as the "same way" is the correct one, namely
the one without stack unwinding.

The current code used to work, but re-arranged include files
broke the working code somewhere in the past:

include/sysemu/os-win32.h does the right thing at the
wrong place. Its correct definition of sigsetjmp is overwritten by
the definition from a Mingw-w64 system header file which
triggers stack unwinding. Stack unwinding is fatal for
QEMU's generated code.

My patch makes sure that the critical code in cpu-exec.c
gets the correct definition of sigsetjmp.

In addition, it removes code which might or might not
change the default definition of sigsetjmp (depending
on the order of include files). Now all other files beside
cpu-exec.c will use the default behaviour with stack
unwinding.

Regards,
Stefan

Re: [Qemu-devel] [PATCH] Use special code for sigsetjmp only in cpu-exec.c

[Peter Maydell]

On 1 March 2016 at 11:54, Stefan Weil <sw@weilnetz.de> wrote:
> Am 01.03.2016 um 10:59 schrieb Peter Maydell:
>> I don't understand this patch. Why doesn't it work to have
>> sigsetjmp() be implemented the same way for every use that
>> QEMU makes of it?

> It does, as long as the "same way" is the correct one, namely
> the one without stack unwinding.
>
> The current code used to work, but re-arranged include files
> broke the working code somewhere in the past:
>
> include/sysemu/os-win32.h does the right thing at the
> wrong place. Its correct definition of sigsetjmp is overwritten by
> the definition from a Mingw-w64 system header file which
> triggers stack unwinding. Stack unwinding is fatal for
> QEMU's generated code.
>
> My patch makes sure that the critical code in cpu-exec.c
> gets the correct definition of sigsetjmp.

I think we should fix this by making sure that osdep.h
does the right thing -- ie that it gives us the correct
definition and prevents mingw's headers from overriding it
with the wrong thing (by ensuring that the offending system
header is included before we redefine things, or however
necessary). This is what osdep.h's purpose is -- to hide
annoying system-header workarounds and hacks rather than
putting them in the rest of QEMU code.

> In addition, it removes code which might or might not
> change the default definition of sigsetjmp (depending
> on the order of include files). Now all other files beside
> cpu-exec.c will use the default behaviour with stack
> unwinding.

That seems wrong -- we should have the same behaviour for
sigsetjmp/siglongjmp everywhere we use it.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] Use special code for sigsetjmp only in cpu-exec.c

[Stefan Weil]

Am 01.03.2016 um 13:22 schrieb Peter Maydell:
> On 1 March 2016 at 11:54, Stefan Weil <sw@weilnetz.de> wrote:
>> Am 01.03.2016 um 10:59 schrieb Peter Maydell:
>>> I don't understand this patch. Why doesn't it work to have
>>> sigsetjmp() be implemented the same way for every use that
>>> QEMU makes of it?
>> It does, as long as the "same way" is the correct one, namely
>> the one without stack unwinding.
>>
>> The current code used to work, but re-arranged include files
>> broke the working code somewhere in the past:
>>
>> include/sysemu/os-win32.h does the right thing at the
>> wrong place. Its correct definition of sigsetjmp is overwritten by
>> the definition from a Mingw-w64 system header file which
>> triggers stack unwinding. Stack unwinding is fatal for
>> QEMU's generated code.
>>
>> My patch makes sure that the critical code in cpu-exec.c
>> gets the correct definition of sigsetjmp.
> I think we should fix this by making sure that osdep.h
> does the right thing -- ie that it gives us the correct
> definition and prevents mingw's headers from overriding it
> with the wrong thing (by ensuring that the offending system
> header is included before we redefine things, or however
> necessary). This is what osdep.h's purpose is -- to hide
> annoying system-header workarounds and hacks rather than
> putting them in the rest of QEMU code.
>
>> In addition, it removes code which might or might not
>> change the default definition of sigsetjmp (depending
>> on the order of include files). Now all other files beside
>> cpu-exec.c will use the default behaviour with stack
>> unwinding.
> That seems wrong -- we should have the same behaviour for
> sigsetjmp/siglongjmp everywhere we use it.
>
> thanks
> -- PMM

Technically there is nothing wrong with using different behaviour
for each setjmp or sigsetjmp.

The "best" solution would be to add any prologue / epilogue which
is needed for stack unwinding to the generated code. Like that,
no tricks with redefinitions of setjmp / sigsetjmp would be necessary.

As long as that solution is not available, I'd prefer the variant which
is implemented by my patch and keep the workaround close to
the single location where it is needed.

Your alternate solution would require
inclusion of setjmp.h in include/sysemu/os-win32.h. Then every
compilation for Windows would get that header file, resulting
in a (small) overhead. In addition, there would be no stack
unwinding for any setjmp/longjmp which is not the standard
behaviour for Windows 64 bit (but which we had until it was
broken). I simply don't know whether this has unwanted
side effects (maybe for debugging or with crash dumps) -
that's the reason why I'd minimize the non-standard behaviour.

Regards,
Stefan

Re: [Qemu-devel] [PATCH] Use special code for sigsetjmp only in cpu-exec.c

[Andrew Baumann]

> From: Stefan Weil [mailto:sw@weilnetz.de]
> Sent: Tuesday, 1 March 2016 5:16 AM
> 
> Am 01.03.2016 um 13:22 schrieb Peter Maydell:
> > On 1 March 2016 at 11:54, Stefan Weil <sw@weilnetz.de> wrote:
> >> Am 01.03.2016 um 10:59 schrieb Peter Maydell:
> >>> I don't understand this patch. Why doesn't it work to have
> >>> sigsetjmp() be implemented the same way for every use that
> >>> QEMU makes of it?
> >> It does, as long as the "same way" is the correct one, namely
> >> the one without stack unwinding.
> >>
> >> The current code used to work, but re-arranged include files
> >> broke the working code somewhere in the past:
> >>
> >> include/sysemu/os-win32.h does the right thing at the
> >> wrong place. Its correct definition of sigsetjmp is overwritten by
> >> the definition from a Mingw-w64 system header file which
> >> triggers stack unwinding. Stack unwinding is fatal for
> >> QEMU's generated code.
> >>
> >> My patch makes sure that the critical code in cpu-exec.c
> >> gets the correct definition of sigsetjmp.
> > I think we should fix this by making sure that osdep.h
> > does the right thing -- ie that it gives us the correct
> > definition and prevents mingw's headers from overriding it
> > with the wrong thing (by ensuring that the offending system
> > header is included before we redefine things, or however
> > necessary). This is what osdep.h's purpose is -- to hide
> > annoying system-header workarounds and hacks rather than
> > putting them in the rest of QEMU code.
> >
> >> In addition, it removes code which might or might not
> >> change the default definition of sigsetjmp (depending
> >> on the order of include files). Now all other files beside
> >> cpu-exec.c will use the default behaviour with stack
> >> unwinding.
> > That seems wrong -- we should have the same behaviour for
> > sigsetjmp/siglongjmp everywhere we use it.
> >
> > thanks
> > -- PMM
> 
> Technically there is nothing wrong with using different behaviour
> for each setjmp or sigsetjmp.
> 
> The "best" solution would be to add any prologue / epilogue which
> is needed for stack unwinding to the generated code. Like that,
> no tricks with redefinitions of setjmp / sigsetjmp would be necessary.
> 
> As long as that solution is not available, I'd prefer the variant which
> is implemented by my patch and keep the workaround close to
> the single location where it is needed.
> 
> Your alternate solution would require
> inclusion of setjmp.h in include/sysemu/os-win32.h. Then every
> compilation for Windows would get that header file, resulting
> in a (small) overhead. In addition, there would be no stack
> unwinding for any setjmp/longjmp which is not the standard
> behaviour for Windows 64 bit (but which we had until it was
> broken). I simply don't know whether this has unwanted
> side effects (maybe for debugging or with crash dumps) -
> that's the reason why I'd minimize the non-standard behaviour.

FWIW, I don't see a big problem including setjmp.h from os-win32.h and then modifying the definition globally. The overhead you mention is just in compilation time, and it's pretty minor compared to all the other system header files already included. The lack of stack unwinding is also probably not an issue -- AFAIK nothing in qemu uses structured exception handling, and that is the only reason I'm aware of for needing to be able to unwind from a longjmp.

I have been getting along fine with the following local fix:

--- a/include/sysemu/os-win32.h
+++ b/include/sysemu/os-win32.h
@@ -60,6 +60,7 @@
  * If this parameter is NULL, longjump does no stack unwinding.
  * That is what we need for QEMU. Passing the value of register rsp (default)
  * lets longjmp try a stack unwinding which will crash with generated code. */
+# include <setjmp.h>
 # undef setjmp
 # define setjmp(env) _setjmp(env, NULL)
 #endif

Cheers,
Andrew

Re: [Qemu-devel] [PATCH] Use special code for sigsetjmp only in cpu-exec.c

[Paolo Bonzini]

On 01/03/2016 18:46, Andrew Baumann wrote:
> --- a/include/sysemu/os-win32.h
> +++ b/include/sysemu/os-win32.h
> @@ -60,6 +60,7 @@
>   * If this parameter is NULL, longjump does no stack unwinding.
>   * That is what we need for QEMU. Passing the value of register rsp (default)
>   * lets longjmp try a stack unwinding which will crash with generated code. */
> +# include <setjmp.h>
>  # undef setjmp
>  # define setjmp(env) _setjmp(env, NULL)
>  #endif

I like this patch or the similar:

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 4538fdc..322a7da 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -77,6 +77,8 @@ extern int daemon(int, int);
 #include <sys/time.h>
 #include <assert.h>
 #include <signal.h>
+/* This is needed on Mingw-w64 where we redefine setjmp below.  */
+#include <setjmp.h>

 #ifdef __OpenBSD__
 #include <sys/signal.h>

which also includes the file on POSIX systems.

Paolo

Re: [Qemu-devel] [PATCH] Use special code for sigsetjmp only in cpu-exec.c

[Peter Maydell]

On 1 March 2016 at 17:53, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 01/03/2016 18:46, Andrew Baumann wrote:
>> --- a/include/sysemu/os-win32.h
>> +++ b/include/sysemu/os-win32.h
>> @@ -60,6 +60,7 @@
>>   * If this parameter is NULL, longjump does no stack unwinding.
>>   * That is what we need for QEMU. Passing the value of register rsp (default)
>>   * lets longjmp try a stack unwinding which will crash with generated code. */
>> +# include <setjmp.h>
>>  # undef setjmp
>>  # define setjmp(env) _setjmp(env, NULL)
>>  #endif
>
> I like this patch or the similar:
>
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 4538fdc..322a7da 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -77,6 +77,8 @@ extern int daemon(int, int);
>  #include <sys/time.h>
>  #include <assert.h>
>  #include <signal.h>
> +/* This is needed on Mingw-w64 where we redefine setjmp below.  */
> +#include <setjmp.h>
>
>  #ifdef __OpenBSD__
>  #include <sys/signal.h>
>
> which also includes the file on POSIX systems.

Yes, that would get my vote. (Followup cleanup -- remove the now
unneeded includes of setjmp.h elsewhere.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH] Use special code for sigsetjmp only in cpu-exec.c

[Stefan Weil]

Am 01.03.2016 um 18:54 schrieb Peter Maydell:
> On 1 March 2016 at 17:53, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>> On 01/03/2016 18:46, Andrew Baumann wrote:
>>> --- a/include/sysemu/os-win32.h
>>> +++ b/include/sysemu/os-win32.h
>>> @@ -60,6 +60,7 @@
>>>   * If this parameter is NULL, longjump does no stack unwinding.
>>>   * That is what we need for QEMU. Passing the value of register rsp (default)
>>>   * lets longjmp try a stack unwinding which will crash with generated code. */
>>> +# include <setjmp.h>
>>>  # undef setjmp
>>>  # define setjmp(env) _setjmp(env, NULL)
>>>  #endif
>>
>> I like this patch or the similar:
>>
>> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
>> index 4538fdc..322a7da 100644
>> --- a/include/qemu/osdep.h
>> +++ b/include/qemu/osdep.h
>> @@ -77,6 +77,8 @@ extern int daemon(int, int);
>>  #include <sys/time.h>
>>  #include <assert.h>
>>  #include <signal.h>
>> +/* This is needed on Mingw-w64 where we redefine setjmp below.  */

Maybe even better: "in os-win32.h" instead of "below".

>> +#include <setjmp.h>
>>
>>  #ifdef __OpenBSD__
>>  #include <sys/signal.h>
>>
>> which also includes the file on POSIX systems.
> 
> Yes, that would get my vote. (Followup cleanup -- remove the now
> unneeded includes of setjmp.h elsewhere.)
> 
> thanks
> -- PMM
> 

You are so convincing, so I'll have to send a new patch
with this variant. :-)

Thanks to all who wrote a comment
Stefan