On Mon, 2021-07-05 at 21:18 +0200, David Hildenbrand wrote: > On 05.07.21 19:24, Ilya Leoshkevich wrote: > > On Mon, 2021-07-05 at 11:36 +0200, David Hildenbrand wrote: > > > On 23.06.21 04:32, Ilya Leoshkevich wrote: > > > > For SIGILL, SIGFPE and SIGTRAP the PSW must point after the > > > > instruction, and at the instruction for other signals. Currently > > > > under > > > > qemu-user it always points at the instruction. > > > > > > > > Fix by advancing psw.addr for these signals. > > > > > > > > Buglink: https://gitlab.com/qemu-project/qemu/-/issues/319 > > > > Signed-off-by: Ilya Leoshkevich <i...@linux.ibm.com> > > > > Co-developed-by: Ulrich Weigand <ulrich.weig...@de.ibm.com> > > > > --- > > > > linux-user/s390x/cpu_loop.c | 5 +++++ > > > > 1 file changed, 5 insertions(+) > > > > > > > > diff --git a/linux-user/s390x/cpu_loop.c b/linux- > > > > user/s390x/cpu_loop.c > > > > index 30568139df..230217feeb 100644 > > > > --- a/linux-user/s390x/cpu_loop.c > > > > +++ b/linux-user/s390x/cpu_loop.c > > > > @@ -133,6 +133,11 @@ void cpu_loop(CPUS390XState *env) > > > > > > > > do_signal_pc: > > > > addr = env->psw.addr; > > > > + /* > > > > + * For SIGILL, SIGFPE and SIGTRAP the PSW must point > > > > after the > > > > + * instruction. > > > > + */ > > > > + env->psw.addr += env->int_pgm_ilen; > > > > > > We also reach this path via EXCP_DEBUG. How can we expect > > > env->int_pgm_ilen to contain something sensible in that case? > > > > You are right, this breaks breakpoints after getting any PGM > > exception > > (they turn into "Program received signal SIGTRAP, Trace/breakpoint > > trap." instead of the usual "Breakpoint N"). > > > > We don't need a PSW rewind here, since it's already incremented > > throught the following sequence: > > > > 1) GDB asks QEMU to set a breakpoint using a $Z0 packet. > > 2) translator_loop() notices the breakpoint and calls > > s390x_tr_breakpoint_check(). > > 3) s390x_tr_breakpoint_check() sets DisasContextBase.is_jmp to > > DISAS_PC_STALE and increments DisasContextBase.pc_next by 2. > > 4) translator_loop() calls s390x_tr_tb_stop(). > > 5) s390x_tr_tb_stop() calls update_psw_addr(), so the JITed code > > increments the PSWA by 2 as well. > > 6) s390x_tr_tb_stop() calls gen_exception(EXCP_DEBUG). > > > > What do you think about the following amend? > > > > --- a/linux-user/s390x/cpu_loop.c > > +++ b/linux-user/s390x/cpu_loop.c > > @@ -64,7 +64,13 @@ void cpu_loop(CPUS390XState *env) > > case EXCP_DEBUG: > > sig = TARGET_SIGTRAP; > > n = TARGET_TRAP_BRKPT; > > - goto do_signal_pc; > > + /* > > + * For SIGTRAP the PSW must point after the instruction, > > which it > > + * already does thanks to s390x_tr_tb_stop(). si_addr > > doesn't need > > + * to be filled. > > + */ > > + addr = 0; > > + goto do_signal; > > Looks better to me, but I'm not an expert on signals, so I cannot tell > what si_addr is supposed to contain in that case. >
Thanks, I'll send a v6 then. I used rt_sigaction(2) man here: When SIGTRAP is delivered in response to a ptrace(2) event (PTRACE_EVENT_foo), si_addr is not populated I think EXCP_DEBUG corresponds only to this case - there doesn't seem to be a way to generate it without attaching gdb.