Replace all setjmp()/longjmp() with sigsetjmp()/siglongjmp()

The setjmp() function doesn't specify whether signal masks are saved and
restored; on Linux they are not, but on BSD (including MacOSX) they are.
QEMU never wants to save and restore signal masks, because it uses threads,
and the signal-restoration may restore the whole process signal mask,
not just the mask for the thread which did the longjmp. In particular,
this resulted in a bug where ctrl-C was ignored on MacOSX because the
CPU thread did a longjmp which resulted in its signal mask being applied
to every thread, so that all threads had SIGINT and SIGTERM blocked.

The POSIX-sanctioned portable way to do a jump without affecting signal
masks is to use sigsetjmp() with a zero savemask parameter, so change
all uses of setjmp()/longjmp() accordingly.

For Windows we provide a trivial sigsetjmp/siglongjmp in terms of
setjmp/longjmp -- this is OK because no user will ever pass a non-zero
savemask (it would be a bug to do so because of the process-wide effects
described above).

The setjmp() uses in tests/tcg/test-i386.c and tests/tcg/linux-test.c
are left untouched because these are self-contained singlethreaded
test programs intended to be run under QEMU's Linux emulation, so they
have neither the portability nor the multithreading issues to deal with.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
This should have no visible effects on Linux where setjmp/longjmp
never messed with the signal state in the first place; it just makes
us behave right on the BSDs.

Stefan: I've put in the os-win32.h defines which I think are needed;
they're pretty trivial but I don't have a Win32 setup to compile with;
can I ask you to run a quick test for me, please?

I didn't feel it necessary to change every last comment that said
'longjmp' to say 'siglongjmp' where I felt the comment was talking
about 'longjmp the concept' rather than 'longjmp the specific
function implementation'.

checkpatch complains about the indent in the disassembler
sources, as usual, but is otherwise happy.

 coroutine-sigaltstack.c   | 26 +++++++++++++-------------
 coroutine-ucontext.c      | 27 ++++++++++++++-------------
 cpu-exec.c                |  6 +++---
 disas/i386.c              |  6 +++---
 disas/m68k.c              | 11 ++++++-----
 include/exec/cpu-defs.h   |  2 +-
 include/sysemu/os-win32.h |  8 ++++++++
 monitor.c                 |  6 +++---
 user-exec.c               |  2 +-
 9 files changed, 52 insertions(+), 42 deletions(-)

On Sun, Feb 17, 2013 at 2:44 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> The setjmp() function doesn't specify whether signal masks are saved and
> restored; on Linux they are not, but on BSD (including MacOSX) they are.
> QEMU never wants to save and restore signal masks, because it uses threads,
> and the signal-restoration may restore the whole process signal mask,
> not just the mask for the thread which did the longjmp. In particular,
> this resulted in a bug where ctrl-C was ignored on MacOSX because the
> CPU thread did a longjmp which resulted in its signal mask being applied
> to every thread, so that all threads had SIGINT and SIGTERM blocked.
>
> The POSIX-sanctioned portable way to do a jump without affecting signal
> masks is to use sigsetjmp() with a zero savemask parameter, so change
> all uses of setjmp()/longjmp() accordingly.
>
> For Windows we provide a trivial sigsetjmp/siglongjmp in terms of
> setjmp/longjmp -- this is OK because no user will ever pass a non-zero
> savemask (it would be a bug to do so because of the process-wide effects
> described above).
>
> The setjmp() uses in tests/tcg/test-i386.c and tests/tcg/linux-test.c
> are left untouched because these are self-contained singlethreaded
> test programs intended to be run under QEMU's Linux emulation, so they
> have neither the portability nor the multithreading issues to deal with.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> This should have no visible effects on Linux where setjmp/longjmp
> never messed with the signal state in the first place; it just makes
> us behave right on the BSDs.
>
> Stefan: I've put in the os-win32.h defines which I think are needed;
> they're pretty trivial but I don't have a Win32 setup to compile with;
> can I ask you to run a quick test for me, please?

Builds fine here on a few mingw32 setups.

>
> I didn't feel it necessary to change every last comment that said
> 'longjmp' to say 'siglongjmp' where I felt the comment was talking
> about 'longjmp the concept' rather than 'longjmp the specific
> function implementation'.
>
> checkpatch complains about the indent in the disassembler
> sources, as usual, but is otherwise happy.
>
>  coroutine-sigaltstack.c   | 26 +++++++++++++-------------
>  coroutine-ucontext.c      | 27 ++++++++++++++-------------
>  cpu-exec.c                |  6 +++---
>  disas/i386.c              |  6 +++---
>  disas/m68k.c              | 11 ++++++-----
>  include/exec/cpu-defs.h   |  2 +-
>  include/sysemu/os-win32.h |  8 ++++++++
>  monitor.c                 |  6 +++---
>  user-exec.c               |  2 +-
>  9 files changed, 52 insertions(+), 42 deletions(-)
>
> diff --git a/coroutine-sigaltstack.c b/coroutine-sigaltstack.c
> index e37ebac..1fb41c9 100644
> --- a/coroutine-sigaltstack.c
> +++ b/coroutine-sigaltstack.c
> @@ -45,7 +45,7 @@ static unsigned int pool_size;
>  typedef struct {
>      Coroutine base;
>      void *stack;
> -    jmp_buf env;
> +    sigjmp_buf env;
>  } CoroutineUContext;
>
>  /**
> @@ -59,7 +59,7 @@ typedef struct {
>      CoroutineUContext leader;
>
>      /** Information for the signal handler (trampoline) */
> -    jmp_buf tr_reenter;
> +    sigjmp_buf tr_reenter;
>      volatile sig_atomic_t tr_called;
>      void *tr_handler;
>  } CoroutineThreadState;
> @@ -115,8 +115,8 @@ static void __attribute__((constructor)) coroutine_init(void)
>  static void coroutine_bootstrap(CoroutineUContext *self, Coroutine *co)
>  {
>      /* Initialize longjmp environment and switch back the caller */
> -    if (!setjmp(self->env)) {
> -        longjmp(*(jmp_buf *)co->entry_arg, 1);
> +    if (!sigsetjmp(self->env, 0)) {
> +        siglongjmp(*(sigjmp_buf *)co->entry_arg, 1);
>      }
>
>      while (true) {
> @@ -145,14 +145,14 @@ static void coroutine_trampoline(int signal)
>      /*
>       * Here we have to do a bit of a ping pong between the caller, given that
>       * this is a signal handler and we have to do a return "soon". Then the
> -     * caller can reestablish everything and do a longjmp here again.
> +     * caller can reestablish everything and do a siglongjmp here again.
>       */
> -    if (!setjmp(coTS->tr_reenter)) {
> +    if (!sigsetjmp(coTS->tr_reenter, 0)) {
>          return;
>      }
>
>      /*
> -     * Ok, the caller has longjmp'ed back to us, so now prepare
> +     * Ok, the caller has siglongjmp'ed back to us, so now prepare
>       * us for the real machine state switching. We have to jump
>       * into another function here to get a new stack context for
>       * the auto variables (which have to be auto-variables
> @@ -179,7 +179,7 @@ static Coroutine *coroutine_new(void)
>
>      /* The way to manipulate stack is with the sigaltstack function. We
>       * prepare a stack, with it delivering a signal to ourselves and then
> -     * put setjmp/longjmp where needed.
> +     * put sigsetjmp/siglongjmp where needed.
>       * This has been done keeping coroutine-ucontext as a model and with the
>       * pth ideas (GNU Portable Threads). See coroutine-ucontext for the basics
>       * of the coroutines and see pth_mctx.c (from the pth project) for the
> @@ -220,7 +220,7 @@ static Coroutine *coroutine_new(void)
>
>      /*
>       * Now transfer control onto the signal stack and set it up.
> -     * It will return immediately via "return" after the setjmp()
> +     * It will return immediately via "return" after the sigsetjmp()
>       * was performed. Be careful here with race conditions.  The
>       * signal can be delivered the first time sigsuspend() is
>       * called.
> @@ -261,8 +261,8 @@ static Coroutine *coroutine_new(void)
>       * type-conversion warnings related to the `volatile' qualifier and
>       * the fact that `jmp_buf' usually is an array type.
>       */
> -    if (!setjmp(old_env)) {
> -        longjmp(coTS->tr_reenter, 1);
> +    if (!sigsetjmp(old_env, 0)) {
> +        siglongjmp(coTS->tr_reenter, 1);
>      }
>
>      /*
> @@ -311,9 +311,9 @@ CoroutineAction qemu_coroutine_switch(Coroutine *from_, Coroutine *to_,
>
>      s->current = to_;
>
> -    ret = setjmp(from->env);
> +    ret = sigsetjmp(from->env, 0);
>      if (ret == 0) {
> -        longjmp(to->env, action);
> +        siglongjmp(to->env, action);
>      }
>      return ret;
>  }
> diff --git a/coroutine-ucontext.c b/coroutine-ucontext.c
> index a9c30e9..bd20e38 100644
> --- a/coroutine-ucontext.c
> +++ b/coroutine-ucontext.c
> @@ -46,7 +46,7 @@ static unsigned int pool_size;
>  typedef struct {
>      Coroutine base;
>      void *stack;
> -    jmp_buf env;
> +    sigjmp_buf env;
>
>  #ifdef CONFIG_VALGRIND_H
>      unsigned int valgrind_stack_id;
> @@ -130,8 +130,8 @@ static void coroutine_trampoline(int i0, int i1)
>      co = &self->base;
>
>      /* Initialize longjmp environment and switch back the caller */
> -    if (!setjmp(self->env)) {
> -        longjmp(*(jmp_buf *)co->entry_arg, 1);
> +    if (!sigsetjmp(self->env, 0)) {
> +        siglongjmp(*(sigjmp_buf *)co->entry_arg, 1);
>      }
>
>      while (true) {
> @@ -145,14 +145,15 @@ static Coroutine *coroutine_new(void)
>      const size_t stack_size = 1 << 20;
>      CoroutineUContext *co;
>      ucontext_t old_uc, uc;
> -    jmp_buf old_env;
> +    sigjmp_buf old_env;
>      union cc_arg arg = {0};
>
> -    /* The ucontext functions preserve signal masks which incurs a system call
> -     * overhead.  setjmp()/longjmp() does not preserve signal masks but only
> -     * works on the current stack.  Since we need a way to create and switch to
> -     * a new stack, use the ucontext functions for that but setjmp()/longjmp()
> -     * for everything else.
> +    /* The ucontext functions preserve signal masks which incurs a
> +     * system call overhead.  sigsetjmp(buf, 0)/siglongjmp() does not
> +     * preserve signal masks but only works on the current stack.
> +     * Since we need a way to create and switch to a new stack, use
> +     * the ucontext functions for that but sigsetjmp()/siglongjmp() for
> +     * everything else.
>       */
>
>      if (getcontext(&uc) == -1) {
> @@ -178,8 +179,8 @@ static Coroutine *coroutine_new(void)
>      makecontext(&uc, (void (*)(void))coroutine_trampoline,
>                  2, arg.i[0], arg.i[1]);
>
> -    /* swapcontext() in, longjmp() back out */
> -    if (!setjmp(old_env)) {
> +    /* swapcontext() in, siglongjmp() back out */
> +    if (!sigsetjmp(old_env, 0)) {
>          swapcontext(&old_uc, &uc);
>      }
>      return &co->base;
> @@ -242,9 +243,9 @@ CoroutineAction qemu_coroutine_switch(Coroutine *from_, Coroutine *to_,
>
>      s->current = to_;
>
> -    ret = setjmp(from->env);
> +    ret = sigsetjmp(from->env, 0);
>      if (ret == 0) {
> -        longjmp(to->env, action);
> +        siglongjmp(to->env, action);
>      }
>      return ret;
>  }
> diff --git a/cpu-exec.c b/cpu-exec.c
> index ff9a884..f257191 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -33,7 +33,7 @@ bool qemu_cpu_has_work(CPUState *cpu)
>  void cpu_loop_exit(CPUArchState *env)
>  {
>      env->current_tb = NULL;
> -    longjmp(env->jmp_env, 1);
> +    siglongjmp(env->jmp_env, 1);
>  }
>
>  /* exit the current TB from a signal handler. The host registers are
> @@ -45,7 +45,7 @@ void cpu_resume_from_signal(CPUArchState *env, void *puc)
>      /* XXX: restore cpu registers saved in host registers */
>
>      env->exception_index = -1;
> -    longjmp(env->jmp_env, 1);
> +    siglongjmp(env->jmp_env, 1);
>  }
>  #endif
>
> @@ -231,7 +231,7 @@ int cpu_exec(CPUArchState *env)
>
>      /* prepare setjmp context for exception handling */
>      for(;;) {
> -        if (setjmp(env->jmp_env) == 0) {
> +        if (sigsetjmp(env->jmp_env, 0) == 0) {
>              /* if an exception is pending, we execute it here */
>              if (env->exception_index >= 0) {
>                  if (env->exception_index >= EXCP_INTERRUPT) {
> diff --git a/disas/i386.c b/disas/i386.c
> index 3b006b1..175ea07 100644
> --- a/disas/i386.c
> +++ b/disas/i386.c
> @@ -226,7 +226,7 @@ struct dis_private {
>    bfd_byte the_buffer[MAX_MNEM_SIZE];
>    bfd_vma insn_start;
>    int orig_sizeflag;
> -  jmp_buf bailout;
> +  sigjmp_buf bailout;
>  };
>
>  enum address_mode
> @@ -303,7 +303,7 @@ fetch_data2(struct disassemble_info *info, bfd_byte *addr)
>          STATUS.  */
>        if (priv->max_fetched == priv->the_buffer)
>         (*info->memory_error_func) (status, start, info);
> -      longjmp (priv->bailout, 1);
> +      siglongjmp(priv->bailout, 1);
>      }
>    else
>      priv->max_fetched = addr;
> @@ -3661,7 +3661,7 @@ print_insn (bfd_vma pc, disassemble_info *info)
>    start_codep = priv.the_buffer;
>    codep = priv.the_buffer;
>
> -  if (setjmp (priv.bailout) != 0)
> +  if (sigsetjmp(priv.bailout, 0) != 0)
>      {
>        const char *name;
>
> diff --git a/disas/m68k.c b/disas/m68k.c
> index c950241..cc0db96 100644
> --- a/disas/m68k.c
> +++ b/disas/m68k.c
> @@ -624,7 +624,7 @@ struct private
>    bfd_byte *max_fetched;
>    bfd_byte the_buffer[MAXLEN];
>    bfd_vma insn_start;
> -  jmp_buf bailout;
> +  sigjmp_buf bailout;
>  };
>
>  /* Make sure that bytes from INFO->PRIVATE_DATA->BUFFER (inclusive)
> @@ -644,7 +644,7 @@ fetch_data2(struct disassemble_info *info, bfd_byte *addr)
>    if (status != 0)
>      {
>        (*info->memory_error_func) (status, start, info);
> -      longjmp (priv->bailout, 1);
> +      siglongjmp(priv->bailout, 1);
>      }
>    else
>      priv->max_fetched = addr;
> @@ -1912,9 +1912,10 @@ print_insn_m68k (bfd_vma memaddr, disassemble_info *info)
>    priv.max_fetched = priv.the_buffer;
>    priv.insn_start = memaddr;
>
> -  if (setjmp (priv.bailout) != 0)
> -    /* Error return.  */
> -    return -1;
> +  if (sigsetjmp(priv.bailout, 0) != 0) {
> +      /* Error return.  */
> +      return -1;
> +  }
>
>    switch (info->mach)
>      {
> diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h
> index 2911b9f..8958ed8 100644
> --- a/include/exec/cpu-defs.h
> +++ b/include/exec/cpu-defs.h
> @@ -187,7 +187,7 @@ typedef struct CPUWatchpoint {
>      struct GDBRegisterState *gdb_regs;                                  \
>                                                                          \
>      /* Core interrupt code */                                           \
> -    jmp_buf jmp_env;                                                    \
> +    sigjmp_buf jmp_env;                                                 \
>      int exception_index;                                                \
>                                                                          \
>      CPUArchState *next_cpu; /* next CPU sharing TB cache */                 \
> diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h
> index bf9edeb..71f5fa0 100644
> --- a/include/sysemu/os-win32.h
> +++ b/include/sysemu/os-win32.h
> @@ -63,6 +63,14 @@
>  # 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
> + * in terms of setjmp/longjmp on Win32.
> + */
> +#define sigjmp_buf jmp_buf
> +#define sigsetjmp(env, savemask) setjmp(env)
> +#define siglongjmp(env, val) longjmp(env, val)
>
>  /* Declaration of ffs() is missing in MinGW's strings.h. */
>  int ffs(int i);
> diff --git a/monitor.c b/monitor.c
> index 6a0f257..32a6e74 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2740,7 +2740,7 @@ static const mon_cmd_t qmp_cmds[] = {
>  /*******************************************************************/
>
>  static const char *pch;
> -static jmp_buf expr_env;
> +static sigjmp_buf expr_env;
>
>  #define MD_TLONG 0
>  #define MD_I32   1
> @@ -3135,7 +3135,7 @@ static const MonitorDef monitor_defs[] = {
>  static void expr_error(Monitor *mon, const char *msg)
>  {
>      monitor_printf(mon, "%s\n", msg);
> -    longjmp(expr_env, 1);
> +    siglongjmp(expr_env, 1);
>  }
>
>  /* return 0 if OK, -1 if not found */
> @@ -3345,7 +3345,7 @@ static int64_t expr_sum(Monitor *mon)
>  static int get_expr(Monitor *mon, int64_t *pval, const char **pp)
>  {
>      pch = *pp;
> -    if (setjmp(expr_env)) {
> +    if (sigsetjmp(expr_env, 0)) {
>          *pp = pch;
>          return -1;
>      }
> diff --git a/user-exec.c b/user-exec.c
> index c71acbc..71bd6c5 100644
> --- a/user-exec.c
> +++ b/user-exec.c
> @@ -70,7 +70,7 @@ void cpu_resume_from_signal(CPUArchState *env1, void *puc)
>  #endif
>      }
>      env1->exception_index = -1;
> -    longjmp(env1->jmp_env, 1);
> +    siglongjmp(env1->jmp_env, 1);
>  }
>
>  /* 'pc' is the host PC at which the exception was raised. 'address' is
> --
> 1.7.11.4
>

On 2013-02-17 06:44, Peter Maydell wrote:
> The setjmp() function doesn't specify whether signal masks are saved and
> restored; on Linux they are not, but on BSD (including MacOSX) they are.
> QEMU never wants to save and restore signal masks, because it uses threads,
> and the signal-restoration may restore the whole process signal mask,
> not just the mask for the thread which did the longjmp. In particular,
> this resulted in a bug where ctrl-C was ignored on MacOSX because the
> CPU thread did a longjmp which resulted in its signal mask being applied
> to every thread, so that all threads had SIGINT and SIGTERM blocked.
>
> The POSIX-sanctioned portable way to do a jump without affecting signal
> masks is to use sigsetjmp() with a zero savemask parameter, so change
> all uses of setjmp()/longjmp() accordingly.
>
> For Windows we provide a trivial sigsetjmp/siglongjmp in terms of
> setjmp/longjmp -- this is OK because no user will ever pass a non-zero
> savemask (it would be a bug to do so because of the process-wide effects
> described above).
>
> The setjmp() uses in tests/tcg/test-i386.c and tests/tcg/linux-test.c
> are left untouched because these are self-contained singlethreaded
> test programs intended to be run under QEMU's Linux emulation, so they
> have neither the portability nor the multithreading issues to deal with.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~

On 17 February 2013 14:44, Peter Maydell <peter.maydell@linaro.org> wrote:
> The setjmp() function doesn't specify whether signal masks are saved and
> restored; on Linux they are not, but on BSD (including MacOSX) they are.
> QEMU never wants to save and restore signal masks, because it uses threads,
> and the signal-restoration may restore the whole process signal mask,
> not just the mask for the thread which did the longjmp. In particular,
> this resulted in a bug where ctrl-C was ignored on MacOSX because the
> CPU thread did a longjmp which resulted in its signal mask being applied
> to every thread, so that all threads had SIGINT and SIGTERM blocked.
>
> The POSIX-sanctioned portable way to do a jump without affecting signal
> masks is to use sigsetjmp() with a zero savemask parameter, so change
> all uses of setjmp()/longjmp() accordingly.

This turns out to be not exactly accurate. POSIX says:
"if savemask is zero, it is unspecified whether the signal mask is saved"

Which isn't very helpful. It looks like _setjmp/_longjmp are
POSIX-required not to mess with the sigmask, but on the other
hand POSIX has also deprecated them. Sigh.

So I'm not sure what to do here. I guess this patch makes us
at least "portable between BSD and Linux", which are the major
use cases. I have no idea whether anybody's actually ever implemented
sigsetjmp() to ignore the savemask parameter; it would be pretty
perverse to do so.

-- PMM

On 18 February 2013 19:19, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 17 February 2013 14:44, Peter Maydell <peter.maydell@linaro.org> wrote:
>> The setjmp() function doesn't specify whether signal masks are saved and
>> restored; on Linux they are not, but on BSD (including MacOSX) they are.
>> QEMU never wants to save and restore signal masks, because it uses threads,
>> and the signal-restoration may restore the whole process signal mask,
>> not just the mask for the thread which did the longjmp. In particular,
>> this resulted in a bug where ctrl-C was ignored on MacOSX because the
>> CPU thread did a longjmp which resulted in its signal mask being applied
>> to every thread, so that all threads had SIGINT and SIGTERM blocked.
>>
>> The POSIX-sanctioned portable way to do a jump without affecting signal
>> masks is to use sigsetjmp() with a zero savemask parameter, so change
>> all uses of setjmp()/longjmp() accordingly.
>
> This turns out to be not exactly accurate. POSIX says:
> "if savemask is zero, it is unspecified whether the signal mask is saved"

This turns out to be accurate but misleading :-)

POSIX also says "The siglongjmp() function shall restore the saved
signal mask if and only if the env argument was initialized by a call
to sigsetjmp() with a non-zero savemask". So it allows the
implementation to pointlessly save the signal mask on sigsetjmp(buf, 0)
but only if it can then guarantee not to restore it on jump.

This means the patch is OK (though the commit message is slightly
misleading in a couple of areas and I guess I could reword it.)

-- PMM

Am 17.02.2013 21:13, schrieb Blue Swirl:
> On Sun, Feb 17, 2013 at 2:44 PM, Peter Maydell<peter.maydell@linaro.org>  wrote:
>    
>> The setjmp() function doesn't specify whether signal masks are saved and
>> restored; on Linux they are not, but on BSD (including MacOSX) they are.
>> QEMU never wants to save and restore signal masks, because it uses threads,
>> and the signal-restoration may restore the whole process signal mask,
>> not just the mask for the thread which did the longjmp. In particular,
>> this resulted in a bug where ctrl-C was ignored on MacOSX because the
>> CPU thread did a longjmp which resulted in its signal mask being applied
>> to every thread, so that all threads had SIGINT and SIGTERM blocked.
>>
>> The POSIX-sanctioned portable way to do a jump without affecting signal
>> masks is to use sigsetjmp() with a zero savemask parameter, so change
>> all uses of setjmp()/longjmp() accordingly.
>>
>> For Windows we provide a trivial sigsetjmp/siglongjmp in terms of
>> setjmp/longjmp -- this is OK because no user will ever pass a non-zero
>> savemask (it would be a bug to do so because of the process-wide effects
>> described above).
>>
>> The setjmp() uses in tests/tcg/test-i386.c and tests/tcg/linux-test.c
>> are left untouched because these are self-contained singlethreaded
>> test programs intended to be run under QEMU's Linux emulation, so they
>> have neither the portability nor the multithreading issues to deal with.
>>
>> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
>> ---
>> This should have no visible effects on Linux where setjmp/longjmp
>> never messed with the signal state in the first place; it just makes
>> us behave right on the BSDs.
>>
>> Stefan: I've put in the os-win32.h defines which I think are needed;
>> they're pretty trivial but I don't have a Win32 setup to compile with;
>> can I ask you to run a quick test for me, please?
>>      
> Builds fine here on a few mingw32 setups.
>    

So does it here.
The resulting binaries also work as expected (tested on w64).

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

Thanks, applied.

On Sun, Feb 17, 2013 at 2:44 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> The setjmp() function doesn't specify whether signal masks are saved and
> restored; on Linux they are not, but on BSD (including MacOSX) they are.
> QEMU never wants to save and restore signal masks, because it uses threads,
> and the signal-restoration may restore the whole process signal mask,
> not just the mask for the thread which did the longjmp. In particular,
> this resulted in a bug where ctrl-C was ignored on MacOSX because the
> CPU thread did a longjmp which resulted in its signal mask being applied
> to every thread, so that all threads had SIGINT and SIGTERM blocked.
>
> The POSIX-sanctioned portable way to do a jump without affecting signal
> masks is to use sigsetjmp() with a zero savemask parameter, so change
> all uses of setjmp()/longjmp() accordingly.
>
> For Windows we provide a trivial sigsetjmp/siglongjmp in terms of
> setjmp/longjmp -- this is OK because no user will ever pass a non-zero
> savemask (it would be a bug to do so because of the process-wide effects
> described above).
>
> The setjmp() uses in tests/tcg/test-i386.c and tests/tcg/linux-test.c
> are left untouched because these are self-contained singlethreaded
> test programs intended to be run under QEMU's Linux emulation, so they
> have neither the portability nor the multithreading issues to deal with.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> This should have no visible effects on Linux where setjmp/longjmp
> never messed with the signal state in the first place; it just makes
> us behave right on the BSDs.
>
> Stefan: I've put in the os-win32.h defines which I think are needed;
> they're pretty trivial but I don't have a Win32 setup to compile with;
> can I ask you to run a quick test for me, please?
>
> I didn't feel it necessary to change every last comment that said
> 'longjmp' to say 'siglongjmp' where I felt the comment was talking
> about 'longjmp the concept' rather than 'longjmp the specific
> function implementation'.
>
> checkpatch complains about the indent in the disassembler
> sources, as usual, but is otherwise happy.
>
>  coroutine-sigaltstack.c   | 26 +++++++++++++-------------
>  coroutine-ucontext.c      | 27 ++++++++++++++-------------
>  cpu-exec.c                |  6 +++---
>  disas/i386.c              |  6 +++---
>  disas/m68k.c              | 11 ++++++-----
>  include/exec/cpu-defs.h   |  2 +-
>  include/sysemu/os-win32.h |  8 ++++++++
>  monitor.c                 |  6 +++---
>  user-exec.c               |  2 +-
>  9 files changed, 52 insertions(+), 42 deletions(-)
>
> diff --git a/coroutine-sigaltstack.c b/coroutine-sigaltstack.c
> index e37ebac..1fb41c9 100644
> --- a/coroutine-sigaltstack.c
> +++ b/coroutine-sigaltstack.c
> @@ -45,7 +45,7 @@ static unsigned int pool_size;
>  typedef struct {
>      Coroutine base;
>      void *stack;
> -    jmp_buf env;
> +    sigjmp_buf env;
>  } CoroutineUContext;
>
>  /**
> @@ -59,7 +59,7 @@ typedef struct {
>      CoroutineUContext leader;
>
>      /** Information for the signal handler (trampoline) */
> -    jmp_buf tr_reenter;
> +    sigjmp_buf tr_reenter;
>      volatile sig_atomic_t tr_called;
>      void *tr_handler;
>  } CoroutineThreadState;
> @@ -115,8 +115,8 @@ static void __attribute__((constructor)) coroutine_init(void)
>  static void coroutine_bootstrap(CoroutineUContext *self, Coroutine *co)
>  {
>      /* Initialize longjmp environment and switch back the caller */
> -    if (!setjmp(self->env)) {
> -        longjmp(*(jmp_buf *)co->entry_arg, 1);
> +    if (!sigsetjmp(self->env, 0)) {
> +        siglongjmp(*(sigjmp_buf *)co->entry_arg, 1);
>      }
>
>      while (true) {
> @@ -145,14 +145,14 @@ static void coroutine_trampoline(int signal)
>      /*
>       * Here we have to do a bit of a ping pong between the caller, given that
>       * this is a signal handler and we have to do a return "soon". Then the
> -     * caller can reestablish everything and do a longjmp here again.
> +     * caller can reestablish everything and do a siglongjmp here again.
>       */
> -    if (!setjmp(coTS->tr_reenter)) {
> +    if (!sigsetjmp(coTS->tr_reenter, 0)) {
>          return;
>      }
>
>      /*
> -     * Ok, the caller has longjmp'ed back to us, so now prepare
> +     * Ok, the caller has siglongjmp'ed back to us, so now prepare
>       * us for the real machine state switching. We have to jump
>       * into another function here to get a new stack context for
>       * the auto variables (which have to be auto-variables
> @@ -179,7 +179,7 @@ static Coroutine *coroutine_new(void)
>
>      /* The way to manipulate stack is with the sigaltstack function. We
>       * prepare a stack, with it delivering a signal to ourselves and then
> -     * put setjmp/longjmp where needed.
> +     * put sigsetjmp/siglongjmp where needed.
>       * This has been done keeping coroutine-ucontext as a model and with the
>       * pth ideas (GNU Portable Threads). See coroutine-ucontext for the basics
>       * of the coroutines and see pth_mctx.c (from the pth project) for the
> @@ -220,7 +220,7 @@ static Coroutine *coroutine_new(void)
>
>      /*
>       * Now transfer control onto the signal stack and set it up.
> -     * It will return immediately via "return" after the setjmp()
> +     * It will return immediately via "return" after the sigsetjmp()
>       * was performed. Be careful here with race conditions.  The
>       * signal can be delivered the first time sigsuspend() is
>       * called.
> @@ -261,8 +261,8 @@ static Coroutine *coroutine_new(void)
>       * type-conversion warnings related to the `volatile' qualifier and
>       * the fact that `jmp_buf' usually is an array type.
>       */
> -    if (!setjmp(old_env)) {
> -        longjmp(coTS->tr_reenter, 1);
> +    if (!sigsetjmp(old_env, 0)) {
> +        siglongjmp(coTS->tr_reenter, 1);
>      }
>
>      /*
> @@ -311,9 +311,9 @@ CoroutineAction qemu_coroutine_switch(Coroutine *from_, Coroutine *to_,
>
>      s->current = to_;
>
> -    ret = setjmp(from->env);
> +    ret = sigsetjmp(from->env, 0);
>      if (ret == 0) {
> -        longjmp(to->env, action);
> +        siglongjmp(to->env, action);
>      }
>      return ret;
>  }
> diff --git a/coroutine-ucontext.c b/coroutine-ucontext.c
> index a9c30e9..bd20e38 100644
> --- a/coroutine-ucontext.c
> +++ b/coroutine-ucontext.c
> @@ -46,7 +46,7 @@ static unsigned int pool_size;
>  typedef struct {
>      Coroutine base;
>      void *stack;
> -    jmp_buf env;
> +    sigjmp_buf env;
>
>  #ifdef CONFIG_VALGRIND_H
>      unsigned int valgrind_stack_id;
> @@ -130,8 +130,8 @@ static void coroutine_trampoline(int i0, int i1)
>      co = &self->base;
>
>      /* Initialize longjmp environment and switch back the caller */
> -    if (!setjmp(self->env)) {
> -        longjmp(*(jmp_buf *)co->entry_arg, 1);
> +    if (!sigsetjmp(self->env, 0)) {
> +        siglongjmp(*(sigjmp_buf *)co->entry_arg, 1);
>      }
>
>      while (true) {
> @@ -145,14 +145,15 @@ static Coroutine *coroutine_new(void)
>      const size_t stack_size = 1 << 20;
>      CoroutineUContext *co;
>      ucontext_t old_uc, uc;
> -    jmp_buf old_env;
> +    sigjmp_buf old_env;
>      union cc_arg arg = {0};
>
> -    /* The ucontext functions preserve signal masks which incurs a system call
> -     * overhead.  setjmp()/longjmp() does not preserve signal masks but only
> -     * works on the current stack.  Since we need a way to create and switch to
> -     * a new stack, use the ucontext functions for that but setjmp()/longjmp()
> -     * for everything else.
> +    /* The ucontext functions preserve signal masks which incurs a
> +     * system call overhead.  sigsetjmp(buf, 0)/siglongjmp() does not
> +     * preserve signal masks but only works on the current stack.
> +     * Since we need a way to create and switch to a new stack, use
> +     * the ucontext functions for that but sigsetjmp()/siglongjmp() for
> +     * everything else.
>       */
>
>      if (getcontext(&uc) == -1) {
> @@ -178,8 +179,8 @@ static Coroutine *coroutine_new(void)
>      makecontext(&uc, (void (*)(void))coroutine_trampoline,
>                  2, arg.i[0], arg.i[1]);
>
> -    /* swapcontext() in, longjmp() back out */
> -    if (!setjmp(old_env)) {
> +    /* swapcontext() in, siglongjmp() back out */
> +    if (!sigsetjmp(old_env, 0)) {
>          swapcontext(&old_uc, &uc);
>      }
>      return &co->base;
> @@ -242,9 +243,9 @@ CoroutineAction qemu_coroutine_switch(Coroutine *from_, Coroutine *to_,
>
>      s->current = to_;
>
> -    ret = setjmp(from->env);
> +    ret = sigsetjmp(from->env, 0);
>      if (ret == 0) {
> -        longjmp(to->env, action);
> +        siglongjmp(to->env, action);
>      }
>      return ret;
>  }
> diff --git a/cpu-exec.c b/cpu-exec.c
> index ff9a884..f257191 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -33,7 +33,7 @@ bool qemu_cpu_has_work(CPUState *cpu)
>  void cpu_loop_exit(CPUArchState *env)
>  {
>      env->current_tb = NULL;
> -    longjmp(env->jmp_env, 1);
> +    siglongjmp(env->jmp_env, 1);
>  }
>
>  /* exit the current TB from a signal handler. The host registers are
> @@ -45,7 +45,7 @@ void cpu_resume_from_signal(CPUArchState *env, void *puc)
>      /* XXX: restore cpu registers saved in host registers */
>
>      env->exception_index = -1;
> -    longjmp(env->jmp_env, 1);
> +    siglongjmp(env->jmp_env, 1);
>  }
>  #endif
>
> @@ -231,7 +231,7 @@ int cpu_exec(CPUArchState *env)
>
>      /* prepare setjmp context for exception handling */
>      for(;;) {
> -        if (setjmp(env->jmp_env) == 0) {
> +        if (sigsetjmp(env->jmp_env, 0) == 0) {
>              /* if an exception is pending, we execute it here */
>              if (env->exception_index >= 0) {
>                  if (env->exception_index >= EXCP_INTERRUPT) {
> diff --git a/disas/i386.c b/disas/i386.c
> index 3b006b1..175ea07 100644
> --- a/disas/i386.c
> +++ b/disas/i386.c
> @@ -226,7 +226,7 @@ struct dis_private {
>    bfd_byte the_buffer[MAX_MNEM_SIZE];
>    bfd_vma insn_start;
>    int orig_sizeflag;
> -  jmp_buf bailout;
> +  sigjmp_buf bailout;
>  };
>
>  enum address_mode
> @@ -303,7 +303,7 @@ fetch_data2(struct disassemble_info *info, bfd_byte *addr)
>          STATUS.  */
>        if (priv->max_fetched == priv->the_buffer)
>         (*info->memory_error_func) (status, start, info);
> -      longjmp (priv->bailout, 1);
> +      siglongjmp(priv->bailout, 1);
>      }
>    else
>      priv->max_fetched = addr;
> @@ -3661,7 +3661,7 @@ print_insn (bfd_vma pc, disassemble_info *info)
>    start_codep = priv.the_buffer;
>    codep = priv.the_buffer;
>
> -  if (setjmp (priv.bailout) != 0)
> +  if (sigsetjmp(priv.bailout, 0) != 0)
>      {
>        const char *name;
>
> diff --git a/disas/m68k.c b/disas/m68k.c
> index c950241..cc0db96 100644
> --- a/disas/m68k.c
> +++ b/disas/m68k.c
> @@ -624,7 +624,7 @@ struct private
>    bfd_byte *max_fetched;
>    bfd_byte the_buffer[MAXLEN];
>    bfd_vma insn_start;
> -  jmp_buf bailout;
> +  sigjmp_buf bailout;
>  };
>
>  /* Make sure that bytes from INFO->PRIVATE_DATA->BUFFER (inclusive)
> @@ -644,7 +644,7 @@ fetch_data2(struct disassemble_info *info, bfd_byte *addr)
>    if (status != 0)
>      {
>        (*info->memory_error_func) (status, start, info);
> -      longjmp (priv->bailout, 1);
> +      siglongjmp(priv->bailout, 1);
>      }
>    else
>      priv->max_fetched = addr;
> @@ -1912,9 +1912,10 @@ print_insn_m68k (bfd_vma memaddr, disassemble_info *info)
>    priv.max_fetched = priv.the_buffer;
>    priv.insn_start = memaddr;
>
> -  if (setjmp (priv.bailout) != 0)
> -    /* Error return.  */
> -    return -1;
> +  if (sigsetjmp(priv.bailout, 0) != 0) {
> +      /* Error return.  */
> +      return -1;
> +  }
>
>    switch (info->mach)
>      {
> diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h
> index 2911b9f..8958ed8 100644
> --- a/include/exec/cpu-defs.h
> +++ b/include/exec/cpu-defs.h
> @@ -187,7 +187,7 @@ typedef struct CPUWatchpoint {
>      struct GDBRegisterState *gdb_regs;                                  \
>                                                                          \
>      /* Core interrupt code */                                           \
> -    jmp_buf jmp_env;                                                    \
> +    sigjmp_buf jmp_env;                                                 \
>      int exception_index;                                                \
>                                                                          \
>      CPUArchState *next_cpu; /* next CPU sharing TB cache */                 \
> diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h
> index bf9edeb..71f5fa0 100644
> --- a/include/sysemu/os-win32.h
> +++ b/include/sysemu/os-win32.h
> @@ -63,6 +63,14 @@
>  # 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
> + * in terms of setjmp/longjmp on Win32.
> + */
> +#define sigjmp_buf jmp_buf
> +#define sigsetjmp(env, savemask) setjmp(env)
> +#define siglongjmp(env, val) longjmp(env, val)
>
>  /* Declaration of ffs() is missing in MinGW's strings.h. */
>  int ffs(int i);
> diff --git a/monitor.c b/monitor.c
> index 6a0f257..32a6e74 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2740,7 +2740,7 @@ static const mon_cmd_t qmp_cmds[] = {
>  /*******************************************************************/
>
>  static const char *pch;
> -static jmp_buf expr_env;
> +static sigjmp_buf expr_env;
>
>  #define MD_TLONG 0
>  #define MD_I32   1
> @@ -3135,7 +3135,7 @@ static const MonitorDef monitor_defs[] = {
>  static void expr_error(Monitor *mon, const char *msg)
>  {
>      monitor_printf(mon, "%s\n", msg);
> -    longjmp(expr_env, 1);
> +    siglongjmp(expr_env, 1);
>  }
>
>  /* return 0 if OK, -1 if not found */
> @@ -3345,7 +3345,7 @@ static int64_t expr_sum(Monitor *mon)
>  static int get_expr(Monitor *mon, int64_t *pval, const char **pp)
>  {
>      pch = *pp;
> -    if (setjmp(expr_env)) {
> +    if (sigsetjmp(expr_env, 0)) {
>          *pp = pch;
>          return -1;
>      }
> diff --git a/user-exec.c b/user-exec.c
> index c71acbc..71bd6c5 100644
> --- a/user-exec.c
> +++ b/user-exec.c
> @@ -70,7 +70,7 @@ void cpu_resume_from_signal(CPUArchState *env1, void *puc)
>  #endif
>      }
>      env1->exception_index = -1;
> -    longjmp(env1->jmp_env, 1);
> +    siglongjmp(env1->jmp_env, 1);
>  }
>
>  /* 'pc' is the host PC at which the exception was raised. 'address' is
> --
> 1.7.11.4
>