[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] Replace all setjmp()/longjmp() with sigsetjmp()
From: |
Blue Swirl |
Subject: |
Re: [Qemu-devel] [PATCH] Replace all setjmp()/longjmp() with sigsetjmp()/siglongjmp() |
Date: |
Sun, 17 Feb 2013 20:13:37 +0000 |
On Sun, Feb 17, 2013 at 2:44 PM, Peter Maydell <address@hidden> 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 <address@hidden>
> ---
> 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
>