[v2] 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.
We want to have consistent behaviour across platforms, so we should
always use "don't save/restore signal mask" (this is also generally
going to be faster). This also works around a bug in MacOSX where the
signal-restoration on longjmp() affects the signal mask for a completely
different thread, not just the mask for the thread which did the longjmp.
The most visible effect of this was that 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 siglongjmp() to a sigjmp_buf which was created by calling
sigsetjmp() with a zero savemask parameter, so change all uses of
setjmp()/longjmp() accordingly. [Technically POSIX allows sigsetjmp(buf, 0)
to save the signal mask; however the following siglongjmp() must not
restore the signal mask, so the pair can be effectively considered as
"sigjmp/longjmp which don't touch the mask".]

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.

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>
Tested-by: Stefan Weil <sw@weilnetz.de>
---
v1->v2 changes:
 * rebased on master (resolved a trivial textual conflict in cpu-exec.c)
 * updated the commit message following Laszlo's comments and my
   further investigation of the POSIX specs

 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 02/20/13 16:21, Peter Maydell wrote:

> +#define sigjmp_buf jmp_buf
> +#define sigsetjmp(env, savemask) setjmp(env)
> +#define siglongjmp(env, val) longjmp(env, val)

Apologies for not really reviewing v1; do you think it would be useful
to add parens around "env" and "val" in the siglongjmp() replacement text?

Also, maybe it would be cleaner to replace these windows-oriented
#defines with a commonly used, new set of macros that would expand to
sig* and ", 0" on non-windows, and to the current text on windows.

That said,

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Cheers
Laszlo

Il 20/02/2013 16:46, Laszlo Ersek ha scritto:
> 
>> > +#define sigjmp_buf jmp_buf
>> > +#define sigsetjmp(env, savemask) setjmp(env)
>> > +#define siglongjmp(env, val) longjmp(env, val)
> Apologies for not really reviewing v1; do you think it would be useful
> to add parens around "env" and "val" in the siglongjmp() replacement text?

It is not necessary, I guess we do one way or the other 50% of the time.

> Also, maybe it would be cleaner to replace these windows-oriented
> #defines with a commonly used, new set of macros that would expand to
> sig* and ", 0" on non-windows, and to the current text on windows.

Ugh no. :)

Paolo

On 20 February 2013 15:46, Laszlo Ersek <lersek@redhat.com> wrote:
> On 02/20/13 16:21, Peter Maydell wrote:
>
>> +#define sigjmp_buf jmp_buf
>> +#define sigsetjmp(env, savemask) setjmp(env)
>> +#define siglongjmp(env, val) longjmp(env, val)
>
> Apologies for not really reviewing v1; do you think it would be useful
> to add parens around "env" and "val" in the siglongjmp() replacement text?

Fractional increase in clarity perhaps, but it isn't necessary
AIUI.

> Also, maybe it would be cleaner to replace these windows-oriented
> #defines with a commonly used, new set of macros that would expand to
> sig* and ", 0" on non-windows, and to the current text on windows.

I thought about that but I think in general we should prefer
to just use the POSIX interface when one is available, with
fixups for non-POSIX platforms. (I don't feel hugely strongly
either way, though.)

-- PMM

On 02/20/13 16:49, Peter Maydell wrote:
> On 20 February 2013 15:46, Laszlo Ersek <lersek@redhat.com> wrote:

>> Also, maybe it would be cleaner to replace these windows-oriented
>> #defines with a commonly used, new set of macros that would expand to
>> sig* and ", 0" on non-windows, and to the current text on windows.
> 
> I thought about that but I think in general we should prefer
> to just use the POSIX interface when one is available, with
> fixups for non-POSIX platforms. (I don't feel hugely strongly
> either way, though.)

Now that you put it this way, I actually agree whole-heartedly :)

L.

Actually I applied this and not v1, sorry for the confusion.

On Wed, Feb 20, 2013 at 3:21 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.
> We want to have consistent behaviour across platforms, so we should
> always use "don't save/restore signal mask" (this is also generally
> going to be faster). This also works around a bug in MacOSX where the
> signal-restoration on longjmp() affects the signal mask for a completely
> different thread, not just the mask for the thread which did the longjmp.
> The most visible effect of this was that 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 siglongjmp() to a sigjmp_buf which was created by calling
> sigsetjmp() with a zero savemask parameter, so change all uses of
> setjmp()/longjmp() accordingly. [Technically POSIX allows sigsetjmp(buf, 0)
> to save the signal mask; however the following siglongjmp() must not
> restore the signal mask, so the pair can be effectively considered as
> "sigjmp/longjmp which don't touch the mask".]
>
> 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.
>
> 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>
> Tested-by: Stefan Weil <sw@weilnetz.de>
> ---
> v1->v2 changes:
>  * rebased on master (resolved a trivial textual conflict in cpu-exec.c)
>  * updated the commit message following Laszlo's comments and my
>    further investigation of the POSIX specs
>
>  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 9fcfe9e..afbe497 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -35,7 +35,7 @@ void cpu_loop_exit(CPUArchState *env)
>      CPUState *cpu = ENV_GET_CPU(env);
>
>      cpu->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
> @@ -47,7 +47,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
>
> @@ -234,7 +234,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 ae64590..3dc9656 100644
> --- a/include/exec/cpu-defs.h
> +++ b/include/exec/cpu-defs.h
> @@ -184,7 +184,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
>