On Sun, Feb 17, 2013 at 2:44 PM, Peter Maydell <peter.mayd...@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.mayd...@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 >