* [Qemu-devel] [PATCH 0/2] linux-user: Don't call gdb_handlesig unnecessarily
@ 2018-10-19 17:49 Peter Maydell
2018-10-19 17:49 ` [Qemu-devel] [PATCH 1/2] linux-user: Don't call gdb_handlesig() before queue_signal() Peter Maydell
` (3 more replies)
0 siblings, 4 replies; 8+ messages in thread
From: Peter Maydell @ 2018-10-19 17:49 UTC (permalink / raw)
To: qemu-devel; +Cc: patches, Riku Voipio, Laurent Vivier, Chris Wulff, Marek Vasut
This patchset fixes a minor bug in our handling of SIGTRAP
in linux-user.
The CPU main-loop routines for linux-user generally call
gdb_handlesig() when they're about to queue a SIGTRAP signal. This
is wrong, because queue_signal() will cause us to pend a signal, and
process_pending_signals() will then call gdb_handlesig() itself. So
the effect is that we notify gdb of the SIGTRAP, and then if gdb says
"OK, continue with signal X" we will incorrectly notify gdb of the
signal X as well. We don't do this double-notify for anything else,
only SIGTRAP.
This bug only manifests if the user responds to the reported SIGTRAP
using "signal SIGFOO" rather than "continue"; since the latter is the
overwhelmingly common thing to do after a breakpoint most people
won't have hit this.
Patch 1 fixes this bug for every target except nios2, by
deleting the incorrect code.
Patch 2 fixes nios2 separately, because it was doing some odd
things with gdb_handlesig(). This also fixes in passing a Coverity
issue.
Tested with "make check-tcg", and with some by-hand stepping
around with an attached gdb. NB that the nios2 patch is only
compile tested as I don't have a nios2 linux-user environment
and check-tcg doesn't cover it.
thanks
-- PMM
Peter Maydell (2):
linux-user: Don't call gdb_handlesig() before queue_signal()
linux-user: Clean up nios2 main loop signal handling
linux-user/aarch64/cpu_loop.c | 13 +++++--------
linux-user/alpha/cpu_loop.c | 12 ++++--------
linux-user/arm/cpu_loop.c | 16 ++++------------
linux-user/cris/cpu_loop.c | 16 ++++------------
linux-user/hppa/cpu_loop.c | 11 ++++-------
linux-user/i386/cpu_loop.c | 16 ++++------------
linux-user/m68k/cpu_loop.c | 16 ++++------------
linux-user/microblaze/cpu_loop.c | 16 ++++------------
linux-user/mips/cpu_loop.c | 16 ++++------------
linux-user/nios2/cpu_loop.c | 14 +++++---------
linux-user/openrisc/cpu_loop.c | 11 ++++-------
linux-user/ppc/cpu_loop.c | 15 +++++----------
linux-user/riscv/cpu_loop.c | 2 +-
linux-user/s390x/cpu_loop.c | 9 +++------
linux-user/sh4/cpu_loop.c | 17 ++++-------------
linux-user/sparc/cpu_loop.c | 16 ++++------------
linux-user/xtensa/cpu_loop.c | 11 ++++-------
17 files changed, 67 insertions(+), 160 deletions(-)
--
2.19.1
^ permalink raw reply [flat|nested] 8+ messages in thread* [Qemu-devel] [PATCH 1/2] linux-user: Don't call gdb_handlesig() before queue_signal() 2018-10-19 17:49 [Qemu-devel] [PATCH 0/2] linux-user: Don't call gdb_handlesig unnecessarily Peter Maydell @ 2018-10-19 17:49 ` Peter Maydell 2018-10-19 17:49 ` [Qemu-devel] [PATCH 2/2] linux-user: Clean up nios2 main loop signal handling Peter Maydell ` (2 subsequent siblings) 3 siblings, 0 replies; 8+ messages in thread From: Peter Maydell @ 2018-10-19 17:49 UTC (permalink / raw) To: qemu-devel; +Cc: patches, Riku Voipio, Laurent Vivier, Chris Wulff, Marek Vasut The CPU main-loop routines for linux-user generally call gdb_handlesig() when they're about to queue a SIGTRAP signal. This is wrong, because queue_signal() will cause us to pend a signal, and process_pending_signals() will then call gdb_handlesig() itself. So the effect is that we notify gdb of the SIGTRAP, and then if gdb says "OK, continue with signal X" we will incorrectly notify gdb of the signal X as well. We don't do this double-notify for anything else, only SIGTRAP. Remove this unnecessary and incorrect code from all the targets except for nios2 (whose main loop is doing something different and broken, and will be handled in a separate patch). This bug only manifests if the user responds to the reported SIGTRAP using "signal SIGFOO" rather than "continue"; since the latter is the overwhelmingly common thing to do after a breakpoint most people won't have hit this. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- linux-user/aarch64/cpu_loop.c | 13 +++++-------- linux-user/alpha/cpu_loop.c | 12 ++++-------- linux-user/arm/cpu_loop.c | 16 ++++------------ linux-user/cris/cpu_loop.c | 16 ++++------------ linux-user/hppa/cpu_loop.c | 11 ++++------- linux-user/i386/cpu_loop.c | 16 ++++------------ linux-user/m68k/cpu_loop.c | 16 ++++------------ linux-user/microblaze/cpu_loop.c | 16 ++++------------ linux-user/mips/cpu_loop.c | 16 ++++------------ linux-user/openrisc/cpu_loop.c | 11 ++++------- linux-user/ppc/cpu_loop.c | 15 +++++---------- linux-user/riscv/cpu_loop.c | 2 +- linux-user/s390x/cpu_loop.c | 9 +++------ linux-user/sh4/cpu_loop.c | 17 ++++------------- linux-user/sparc/cpu_loop.c | 16 ++++------------ linux-user/xtensa/cpu_loop.c | 11 ++++------- 16 files changed, 62 insertions(+), 151 deletions(-) diff --git a/linux-user/aarch64/cpu_loop.c b/linux-user/aarch64/cpu_loop.c index c97a6465464..65d815f0300 100644 --- a/linux-user/aarch64/cpu_loop.c +++ b/linux-user/aarch64/cpu_loop.c @@ -73,7 +73,7 @@ void cpu_loop(CPUARMState *env) { CPUState *cs = CPU(arm_env_get_cpu(env)); - int trapnr, sig; + int trapnr; abi_long ret; target_siginfo_t info; @@ -121,13 +121,10 @@ void cpu_loop(CPUARMState *env) break; case EXCP_DEBUG: case EXCP_BKPT: - sig = gdb_handlesig(cs, TARGET_SIGTRAP); - if (sig) { - info.si_signo = sig; - info.si_errno = 0; - info.si_code = TARGET_TRAP_BRKPT; - queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info); - } + info.si_signo = TARGET_SIGTRAP; + info.si_errno = 0; + info.si_code = TARGET_TRAP_BRKPT; + queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info); break; case EXCP_SEMIHOST: env->xregs[0] = do_arm_semihosting(env); diff --git a/linux-user/alpha/cpu_loop.c b/linux-user/alpha/cpu_loop.c index c1a98c8cbfb..824b6d66588 100644 --- a/linux-user/alpha/cpu_loop.c +++ b/linux-user/alpha/cpu_loop.c @@ -179,14 +179,10 @@ void cpu_loop(CPUAlphaState *env) } break; case EXCP_DEBUG: - info.si_signo = gdb_handlesig(cs, TARGET_SIGTRAP); - if (info.si_signo) { - info.si_errno = 0; - info.si_code = TARGET_TRAP_BRKPT; - queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info); - } else { - arch_interrupt = false; - } + info.si_signo = TARGET_SIGTRAP; + info.si_errno = 0; + info.si_code = TARGET_TRAP_BRKPT; + queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info); break; case EXCP_INTERRUPT: /* Just indicate that signals should be handled asap. */ diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c index 26928fbbb2c..ee68aa60bf3 100644 --- a/linux-user/arm/cpu_loop.c +++ b/linux-user/arm/cpu_loop.c @@ -397,18 +397,10 @@ void cpu_loop(CPUARMState *env) break; case EXCP_DEBUG: excp_debug: - { - int sig; - - sig = gdb_handlesig(cs, TARGET_SIGTRAP); - if (sig) - { - info.si_signo = sig; - info.si_errno = 0; - info.si_code = TARGET_TRAP_BRKPT; - queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info); - } - } + info.si_signo = TARGET_SIGTRAP; + info.si_errno = 0; + info.si_code = TARGET_TRAP_BRKPT; + queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info); break; case EXCP_KERNEL_TRAP: if (do_kernel_trap(env)) diff --git a/linux-user/cris/cpu_loop.c b/linux-user/cris/cpu_loop.c index 37bdcfa8cc3..dacf604c7df 100644 --- a/linux-user/cris/cpu_loop.c +++ b/linux-user/cris/cpu_loop.c @@ -64,18 +64,10 @@ void cpu_loop(CPUCRISState *env) } break; case EXCP_DEBUG: - { - int sig; - - sig = gdb_handlesig(cs, TARGET_SIGTRAP); - if (sig) - { - info.si_signo = sig; - info.si_errno = 0; - info.si_code = TARGET_TRAP_BRKPT; - queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info); - } - } + info.si_signo = TARGET_SIGTRAP; + info.si_errno = 0; + info.si_code = TARGET_TRAP_BRKPT; + queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info); break; case EXCP_ATOMIC: cpu_exec_step_atomic(cs); diff --git a/linux-user/hppa/cpu_loop.c b/linux-user/hppa/cpu_loop.c index 0301c766c6f..880955fdefd 100644 --- a/linux-user/hppa/cpu_loop.c +++ b/linux-user/hppa/cpu_loop.c @@ -182,13 +182,10 @@ void cpu_loop(CPUHPPAState *env) queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info); break; case EXCP_DEBUG: - trapnr = gdb_handlesig(cs, TARGET_SIGTRAP); - if (trapnr) { - info.si_signo = trapnr; - info.si_errno = 0; - info.si_code = TARGET_TRAP_BRKPT; - queue_signal(env, trapnr, QEMU_SI_FAULT, &info); - } + info.si_signo = TARGET_SIGTRAP; + info.si_errno = 0; + info.si_code = TARGET_TRAP_BRKPT; + queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info); break; case EXCP_INTERRUPT: /* just indicate that signals should be handled asap */ diff --git a/linux-user/i386/cpu_loop.c b/linux-user/i386/cpu_loop.c index 2374abfd0bd..51cfa006c97 100644 --- a/linux-user/i386/cpu_loop.c +++ b/linux-user/i386/cpu_loop.c @@ -225,18 +225,10 @@ void cpu_loop(CPUX86State *env) /* just indicate that signals should be handled asap */ break; case EXCP_DEBUG: - { - int sig; - - sig = gdb_handlesig(cs, TARGET_SIGTRAP); - if (sig) - { - info.si_signo = sig; - info.si_errno = 0; - info.si_code = TARGET_TRAP_BRKPT; - queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info); - } - } + info.si_signo = TARGET_SIGTRAP; + info.si_errno = 0; + info.si_code = TARGET_TRAP_BRKPT; + queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info); break; case EXCP_ATOMIC: cpu_exec_step_atomic(cs); diff --git a/linux-user/m68k/cpu_loop.c b/linux-user/m68k/cpu_loop.c index b4d3d8af3dc..2ab5b9c8f8c 100644 --- a/linux-user/m68k/cpu_loop.c +++ b/linux-user/m68k/cpu_loop.c @@ -113,18 +113,10 @@ void cpu_loop(CPUM68KState *env) } break; case EXCP_DEBUG: - { - int sig; - - sig = gdb_handlesig(cs, TARGET_SIGTRAP); - if (sig) - { - info.si_signo = sig; - info.si_errno = 0; - info.si_code = TARGET_TRAP_BRKPT; - queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info); - } - } + info.si_signo = TARGET_SIGTRAP; + info.si_errno = 0; + info.si_code = TARGET_TRAP_BRKPT; + queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info); break; case EXCP_ATOMIC: cpu_exec_step_atomic(cs); diff --git a/linux-user/microblaze/cpu_loop.c b/linux-user/microblaze/cpu_loop.c index 2af93eb39a6..c2190e15fdc 100644 --- a/linux-user/microblaze/cpu_loop.c +++ b/linux-user/microblaze/cpu_loop.c @@ -113,18 +113,10 @@ void cpu_loop(CPUMBState *env) } break; case EXCP_DEBUG: - { - int sig; - - sig = gdb_handlesig(cs, TARGET_SIGTRAP); - if (sig) - { - info.si_signo = sig; - info.si_errno = 0; - info.si_code = TARGET_TRAP_BRKPT; - queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info); - } - } + info.si_signo = TARGET_SIGTRAP; + info.si_errno = 0; + info.si_code = TARGET_TRAP_BRKPT; + queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info); break; case EXCP_ATOMIC: cpu_exec_step_atomic(cs); diff --git a/linux-user/mips/cpu_loop.c b/linux-user/mips/cpu_loop.c index c9c20cf8b7d..417f33c6ba8 100644 --- a/linux-user/mips/cpu_loop.c +++ b/linux-user/mips/cpu_loop.c @@ -592,18 +592,10 @@ done_syscall: /* just indicate that signals should be handled asap */ break; case EXCP_DEBUG: - { - int sig; - - sig = gdb_handlesig(cs, TARGET_SIGTRAP); - if (sig) - { - info.si_signo = sig; - info.si_errno = 0; - info.si_code = TARGET_TRAP_BRKPT; - queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info); - } - } + info.si_signo = TARGET_SIGTRAP; + info.si_errno = 0; + info.si_code = TARGET_TRAP_BRKPT; + queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info); break; case EXCP_SC: if (do_store_exclusive(env)) { diff --git a/linux-user/openrisc/cpu_loop.c b/linux-user/openrisc/cpu_loop.c index 6c6ea871e16..f496e4b48ad 100644 --- a/linux-user/openrisc/cpu_loop.c +++ b/linux-user/openrisc/cpu_loop.c @@ -85,13 +85,10 @@ void cpu_loop(CPUOpenRISCState *env) /* We processed the pending cpu work above. */ break; case EXCP_DEBUG: - trapnr = gdb_handlesig(cs, TARGET_SIGTRAP); - if (trapnr) { - info.si_signo = trapnr; - info.si_errno = 0; - info.si_code = TARGET_TRAP_BRKPT; - queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info); - } + info.si_signo = TARGET_SIGTRAP; + info.si_errno = 0; + info.si_code = TARGET_TRAP_BRKPT; + queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info); break; case EXCP_ATOMIC: cpu_exec_step_atomic(cs); diff --git a/linux-user/ppc/cpu_loop.c b/linux-user/ppc/cpu_loop.c index 133a87f349b..801f5ace29f 100644 --- a/linux-user/ppc/cpu_loop.c +++ b/linux-user/ppc/cpu_loop.c @@ -69,7 +69,7 @@ void cpu_loop(CPUPPCState *env) { CPUState *cs = CPU(ppc_env_get_cpu(env)); target_siginfo_t info; - int trapnr, sig; + int trapnr; target_ulong ret; for(;;) { @@ -449,15 +449,10 @@ void cpu_loop(CPUPPCState *env) env->gpr[3] = ret; break; case EXCP_DEBUG: - sig = gdb_handlesig(cs, TARGET_SIGTRAP); - if (sig) { - info.si_signo = sig; - info.si_errno = 0; - info.si_code = TARGET_TRAP_BRKPT; - queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info); - } else { - arch_interrupt = false; - } + info.si_signo = TARGET_SIGTRAP; + info.si_errno = 0; + info.si_code = TARGET_TRAP_BRKPT; + queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info); break; case EXCP_INTERRUPT: /* just indicate that signals should be handled asap */ diff --git a/linux-user/riscv/cpu_loop.c b/linux-user/riscv/cpu_loop.c index f137d39d7e8..4cf3e946324 100644 --- a/linux-user/riscv/cpu_loop.c +++ b/linux-user/riscv/cpu_loop.c @@ -88,7 +88,7 @@ void cpu_loop(CPURISCVState *env) break; case EXCP_DEBUG: gdbstep: - signum = gdb_handlesig(cs, TARGET_SIGTRAP); + signum = TARGET_SIGTRAP; sigcode = TARGET_TRAP_BRKPT; break; default: diff --git a/linux-user/s390x/cpu_loop.c b/linux-user/s390x/cpu_loop.c index 99f5f1594f4..51b5412ea27 100644 --- a/linux-user/s390x/cpu_loop.c +++ b/linux-user/s390x/cpu_loop.c @@ -61,12 +61,9 @@ void cpu_loop(CPUS390XState *env) break; case EXCP_DEBUG: - sig = gdb_handlesig(cs, TARGET_SIGTRAP); - if (sig) { - n = TARGET_TRAP_BRKPT; - goto do_signal_pc; - } - break; + sig = TARGET_SIGTRAP; + n = TARGET_TRAP_BRKPT; + goto do_signal_pc; case EXCP_PGM: n = env->int_pgm_code; switch (n) { diff --git a/linux-user/sh4/cpu_loop.c b/linux-user/sh4/cpu_loop.c index fdd348170b9..47e54b9b61a 100644 --- a/linux-user/sh4/cpu_loop.c +++ b/linux-user/sh4/cpu_loop.c @@ -57,19 +57,10 @@ void cpu_loop(CPUSH4State *env) /* just indicate that signals should be handled asap */ break; case EXCP_DEBUG: - { - int sig; - - sig = gdb_handlesig(cs, TARGET_SIGTRAP); - if (sig) { - info.si_signo = sig; - info.si_errno = 0; - info.si_code = TARGET_TRAP_BRKPT; - queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info); - } else { - arch_interrupt = false; - } - } + info.si_signo = TARGET_SIGTRAP; + info.si_errno = 0; + info.si_code = TARGET_TRAP_BRKPT; + queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info); break; case 0xa0: case 0xc0: diff --git a/linux-user/sparc/cpu_loop.c b/linux-user/sparc/cpu_loop.c index 91f714afc6c..7d5b337b972 100644 --- a/linux-user/sparc/cpu_loop.c +++ b/linux-user/sparc/cpu_loop.c @@ -268,18 +268,10 @@ void cpu_loop (CPUSPARCState *env) } break; case EXCP_DEBUG: - { - int sig; - - sig = gdb_handlesig(cs, TARGET_SIGTRAP); - if (sig) - { - info.si_signo = sig; - info.si_errno = 0; - info.si_code = TARGET_TRAP_BRKPT; - queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info); - } - } + info.si_signo = TARGET_SIGTRAP; + info.si_errno = 0; + info.si_code = TARGET_TRAP_BRKPT; + queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info); break; case EXCP_ATOMIC: cpu_exec_step_atomic(cs); diff --git a/linux-user/xtensa/cpu_loop.c b/linux-user/xtensa/cpu_loop.c index d142988ebe1..bee78edb8a9 100644 --- a/linux-user/xtensa/cpu_loop.c +++ b/linux-user/xtensa/cpu_loop.c @@ -239,13 +239,10 @@ void cpu_loop(CPUXtensaState *env) } break; case EXCP_DEBUG: - trapnr = gdb_handlesig(cs, TARGET_SIGTRAP); - if (trapnr) { - info.si_signo = trapnr; - info.si_errno = 0; - info.si_code = TARGET_TRAP_BRKPT; - queue_signal(env, trapnr, QEMU_SI_FAULT, &info); - } + info.si_signo = TARGET_SIGTRAP; + info.si_errno = 0; + info.si_code = TARGET_TRAP_BRKPT; + queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info); break; case EXC_DEBUG: default: -- 2.19.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH 2/2] linux-user: Clean up nios2 main loop signal handling 2018-10-19 17:49 [Qemu-devel] [PATCH 0/2] linux-user: Don't call gdb_handlesig unnecessarily Peter Maydell 2018-10-19 17:49 ` [Qemu-devel] [PATCH 1/2] linux-user: Don't call gdb_handlesig() before queue_signal() Peter Maydell @ 2018-10-19 17:49 ` Peter Maydell 2018-11-12 16:12 ` Laurent Vivier 2018-10-20 19:49 ` [Qemu-devel] [PATCH 0/2] linux-user: Don't call gdb_handlesig unnecessarily Richard Henderson 2018-11-12 14:39 ` Peter Maydell 3 siblings, 1 reply; 8+ messages in thread From: Peter Maydell @ 2018-10-19 17:49 UTC (permalink / raw) To: qemu-devel; +Cc: patches, Riku Voipio, Laurent Vivier, Chris Wulff, Marek Vasut The nios2 main loop code's code does some odd things with gdb_handlesig() that no other target CPU does: it has some signals that are delivered to gdb and only to gdb. Stop doing this, and instead behave like all the other targets: * a trap instruction becomes a SIGTRAP * an unhandled exception type returned from cpu_exec() causes us to abort(), not to try to hand gdb a SIGILL This fixes in passing Coverity issue CID 1390853, which was a complaint that the old code failed to check the return value from gdb_handlesig(). Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- Disclaimer: compile tested, and the change makes conceptual sense, but I have no nios2 test environment. --- linux-user/nios2/cpu_loop.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/linux-user/nios2/cpu_loop.c b/linux-user/nios2/cpu_loop.c index dac7a061813..973dd54d791 100644 --- a/linux-user/nios2/cpu_loop.c +++ b/linux-user/nios2/cpu_loop.c @@ -68,7 +68,10 @@ void cpu_loop(CPUNios2State *env) env->regs[R_EA] = env->regs[R_PC] + 4; env->regs[R_PC] = cpu->exception_addr; - gdbsig = TARGET_SIGTRAP; + info.si_signo = TARGET_SIGTRAP; + info.si_errno = 0; + info.si_code = TARGET_TRAP_BRKPT; + queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info); break; } case 0xaa: @@ -106,14 +109,7 @@ kuser_fail: default: EXCP_DUMP(env, "\nqemu: unhandled CPU exception %#x - aborting\n", trapnr); - gdbsig = TARGET_SIGILL; - break; - } - if (gdbsig) { - gdb_handlesig(cs, gdbsig); - if (gdbsig != TARGET_SIGTRAP) { - exit(EXIT_FAILURE); - } + abort(); } process_pending_signals(env); -- 2.19.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] linux-user: Clean up nios2 main loop signal handling 2018-10-19 17:49 ` [Qemu-devel] [PATCH 2/2] linux-user: Clean up nios2 main loop signal handling Peter Maydell @ 2018-11-12 16:12 ` Laurent Vivier 0 siblings, 0 replies; 8+ messages in thread From: Laurent Vivier @ 2018-11-12 16:12 UTC (permalink / raw) To: Peter Maydell, qemu-devel; +Cc: Marek Vasut, Riku Voipio, Chris Wulff, patches On 19/10/2018 19:49, Peter Maydell wrote: > The nios2 main loop code's code does some odd > things with gdb_handlesig() that no other target > CPU does: it has some signals that are delivered > to gdb and only to gdb. Stop doing this, and instead > behave like all the other targets: > * a trap instruction becomes a SIGTRAP > * an unhandled exception type returned from cpu_exec() > causes us to abort(), not to try to hand gdb a SIGILL > > This fixes in passing Coverity issue CID 1390853, > which was a complaint that the old code failed to > check the return value from gdb_handlesig(). > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > Disclaimer: compile tested, and the change makes conceptual > sense, but I have no nios2 test environment. I'll push a slightly modified version of this patch: gdbsig is now unused and my compiler complains about that. Thanks, Laurent > --- > linux-user/nios2/cpu_loop.c | 14 +++++--------- > 1 file changed, 5 insertions(+), 9 deletions(-) > > diff --git a/linux-user/nios2/cpu_loop.c b/linux-user/nios2/cpu_loop.c > index dac7a061813..973dd54d791 100644 > --- a/linux-user/nios2/cpu_loop.c > +++ b/linux-user/nios2/cpu_loop.c > @@ -68,7 +68,10 @@ void cpu_loop(CPUNios2State *env) > env->regs[R_EA] = env->regs[R_PC] + 4; > env->regs[R_PC] = cpu->exception_addr; > > - gdbsig = TARGET_SIGTRAP; > + info.si_signo = TARGET_SIGTRAP; > + info.si_errno = 0; > + info.si_code = TARGET_TRAP_BRKPT; > + queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info); > break; > } > case 0xaa: > @@ -106,14 +109,7 @@ kuser_fail: > default: > EXCP_DUMP(env, "\nqemu: unhandled CPU exception %#x - aborting\n", > trapnr); > - gdbsig = TARGET_SIGILL; > - break; > - } > - if (gdbsig) { > - gdb_handlesig(cs, gdbsig); > - if (gdbsig != TARGET_SIGTRAP) { > - exit(EXIT_FAILURE); > - } > + abort(); > } > > process_pending_signals(env); > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] linux-user: Don't call gdb_handlesig unnecessarily 2018-10-19 17:49 [Qemu-devel] [PATCH 0/2] linux-user: Don't call gdb_handlesig unnecessarily Peter Maydell 2018-10-19 17:49 ` [Qemu-devel] [PATCH 1/2] linux-user: Don't call gdb_handlesig() before queue_signal() Peter Maydell 2018-10-19 17:49 ` [Qemu-devel] [PATCH 2/2] linux-user: Clean up nios2 main loop signal handling Peter Maydell @ 2018-10-20 19:49 ` Richard Henderson 2018-11-12 14:39 ` Peter Maydell 3 siblings, 0 replies; 8+ messages in thread From: Richard Henderson @ 2018-10-20 19:49 UTC (permalink / raw) To: Peter Maydell, qemu-devel Cc: Marek Vasut, Riku Voipio, Laurent Vivier, Chris Wulff, patches On 10/19/18 10:49 AM, Peter Maydell wrote: > This patchset fixes a minor bug in our handling of SIGTRAP > in linux-user. > > The CPU main-loop routines for linux-user generally call > gdb_handlesig() when they're about to queue a SIGTRAP signal. This > is wrong, because queue_signal() will cause us to pend a signal, and > process_pending_signals() will then call gdb_handlesig() itself. So > the effect is that we notify gdb of the SIGTRAP, and then if gdb says > "OK, continue with signal X" we will incorrectly notify gdb of the > signal X as well. We don't do this double-notify for anything else, > only SIGTRAP. > > This bug only manifests if the user responds to the reported SIGTRAP > using "signal SIGFOO" rather than "continue"; since the latter is the > overwhelmingly common thing to do after a breakpoint most people > won't have hit this. > > Patch 1 fixes this bug for every target except nios2, by > deleting the incorrect code. > > Patch 2 fixes nios2 separately, because it was doing some odd > things with gdb_handlesig(). This also fixes in passing a Coverity > issue. > > Tested with "make check-tcg", and with some by-hand stepping > around with an attached gdb. NB that the nios2 patch is only > compile tested as I don't have a nios2 linux-user environment > and check-tcg doesn't cover it. > > thanks > -- PMM > > Peter Maydell (2): > linux-user: Don't call gdb_handlesig() before queue_signal() > linux-user: Clean up nios2 main loop signal handling Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] linux-user: Don't call gdb_handlesig unnecessarily 2018-10-19 17:49 [Qemu-devel] [PATCH 0/2] linux-user: Don't call gdb_handlesig unnecessarily Peter Maydell ` (2 preceding siblings ...) 2018-10-20 19:49 ` [Qemu-devel] [PATCH 0/2] linux-user: Don't call gdb_handlesig unnecessarily Richard Henderson @ 2018-11-12 14:39 ` Peter Maydell 2018-11-12 14:46 ` Laurent Vivier 3 siblings, 1 reply; 8+ messages in thread From: Peter Maydell @ 2018-11-12 14:39 UTC (permalink / raw) To: QEMU Developers Cc: Marek Vasut, Riku Voipio, Laurent Vivier, Chris Wulff, patches@linaro.org Ping? This got reviewed but hasn't made it to master... thanks -- PMM On 19 October 2018 at 18:49, Peter Maydell <peter.maydell@linaro.org> wrote: > This patchset fixes a minor bug in our handling of SIGTRAP > in linux-user. > > The CPU main-loop routines for linux-user generally call > gdb_handlesig() when they're about to queue a SIGTRAP signal. This > is wrong, because queue_signal() will cause us to pend a signal, and > process_pending_signals() will then call gdb_handlesig() itself. So > the effect is that we notify gdb of the SIGTRAP, and then if gdb says > "OK, continue with signal X" we will incorrectly notify gdb of the > signal X as well. We don't do this double-notify for anything else, > only SIGTRAP. > > This bug only manifests if the user responds to the reported SIGTRAP > using "signal SIGFOO" rather than "continue"; since the latter is the > overwhelmingly common thing to do after a breakpoint most people > won't have hit this. > > Patch 1 fixes this bug for every target except nios2, by > deleting the incorrect code. > > Patch 2 fixes nios2 separately, because it was doing some odd > things with gdb_handlesig(). This also fixes in passing a Coverity > issue. > > Tested with "make check-tcg", and with some by-hand stepping > around with an attached gdb. NB that the nios2 patch is only > compile tested as I don't have a nios2 linux-user environment > and check-tcg doesn't cover it. > > thanks > -- PMM > > Peter Maydell (2): > linux-user: Don't call gdb_handlesig() before queue_signal() > linux-user: Clean up nios2 main loop signal handling > > linux-user/aarch64/cpu_loop.c | 13 +++++-------- > linux-user/alpha/cpu_loop.c | 12 ++++-------- > linux-user/arm/cpu_loop.c | 16 ++++------------ > linux-user/cris/cpu_loop.c | 16 ++++------------ > linux-user/hppa/cpu_loop.c | 11 ++++------- > linux-user/i386/cpu_loop.c | 16 ++++------------ > linux-user/m68k/cpu_loop.c | 16 ++++------------ > linux-user/microblaze/cpu_loop.c | 16 ++++------------ > linux-user/mips/cpu_loop.c | 16 ++++------------ > linux-user/nios2/cpu_loop.c | 14 +++++--------- > linux-user/openrisc/cpu_loop.c | 11 ++++------- > linux-user/ppc/cpu_loop.c | 15 +++++---------- > linux-user/riscv/cpu_loop.c | 2 +- > linux-user/s390x/cpu_loop.c | 9 +++------ > linux-user/sh4/cpu_loop.c | 17 ++++------------- > linux-user/sparc/cpu_loop.c | 16 ++++------------ > linux-user/xtensa/cpu_loop.c | 11 ++++------- > 17 files changed, 67 insertions(+), 160 deletions(-) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] linux-user: Don't call gdb_handlesig unnecessarily 2018-11-12 14:39 ` Peter Maydell @ 2018-11-12 14:46 ` Laurent Vivier 2018-11-12 14:48 ` Peter Maydell 0 siblings, 1 reply; 8+ messages in thread From: Laurent Vivier @ 2018-11-12 14:46 UTC (permalink / raw) To: Peter Maydell, QEMU Developers Cc: Marek Vasut, Riku Voipio, Chris Wulff, patches@linaro.org On 12/11/2018 15:39, Peter Maydell wrote: > Ping? This got reviewed but hasn't made it to master... Do you want it in 3.1 too? Thanks, Laurent ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] linux-user: Don't call gdb_handlesig unnecessarily 2018-11-12 14:46 ` Laurent Vivier @ 2018-11-12 14:48 ` Peter Maydell 0 siblings, 0 replies; 8+ messages in thread From: Peter Maydell @ 2018-11-12 14:48 UTC (permalink / raw) To: Laurent Vivier Cc: QEMU Developers, Marek Vasut, Riku Voipio, Chris Wulff, patches@linaro.org On 12 November 2018 at 14:46, Laurent Vivier <laurent@vivier.eu> wrote: > On 12/11/2018 15:39, Peter Maydell wrote: >> Ping? This got reviewed but hasn't made it to master... > > Do you want it in 3.1 too? Well, it is a bugfix, and we haven't rolled rc1 yet, so I would default to yes, unless you'd rather defer it to 4.0. thanks -- PMM ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-11-12 16:12 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-10-19 17:49 [Qemu-devel] [PATCH 0/2] linux-user: Don't call gdb_handlesig unnecessarily Peter Maydell 2018-10-19 17:49 ` [Qemu-devel] [PATCH 1/2] linux-user: Don't call gdb_handlesig() before queue_signal() Peter Maydell 2018-10-19 17:49 ` [Qemu-devel] [PATCH 2/2] linux-user: Clean up nios2 main loop signal handling Peter Maydell 2018-11-12 16:12 ` Laurent Vivier 2018-10-20 19:49 ` [Qemu-devel] [PATCH 0/2] linux-user: Don't call gdb_handlesig unnecessarily Richard Henderson 2018-11-12 14:39 ` Peter Maydell 2018-11-12 14:46 ` Laurent Vivier 2018-11-12 14:48 ` Peter Maydell
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).