Re: [PATCH v8] tests/tcg/s390x: Test SIGILL and SIGSEGV handling

[Ilya Leoshkevich]

On Thu, 2021-08-05 at 11:37 +0200, David Hildenbrand wrote:
> On 05.08.21 00:51, Ilya Leoshkevich wrote:
> > Verify that s390x-specific uc_mcontext.psw.addr is reported
> > correctly
> > and that signal handling interacts properly with debugging.
> > 
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> > 
> > v7: 
> > https://lists.nongnu.org/archive/html/qemu-devel/2021-08/msg00463.html
> > v7 -> v8: Another rebase needed due to the conflict with Jonathan's
> >            50e36dd61652.
> > 
> >   tests/tcg/s390x/Makefile.target               |  17 +-
> >   tests/tcg/s390x/gdbstub/test-signals-s390x.py |  76 ++++++++
> >   tests/tcg/s390x/signals-s390x.c               | 165
> > ++++++++++++++++++
> >   3 files changed, 257 insertions(+), 1 deletion(-)
> >   create mode 100644 tests/tcg/s390x/gdbstub/test-signals-s390x.py
> >   create mode 100644 tests/tcg/s390x/signals-s390x.c
> > 
> > diff --git a/tests/tcg/s390x/Makefile.target
> > b/tests/tcg/s390x/Makefile.target
> > index bd084c7840..cc64dd32d2 100644
> > --- a/tests/tcg/s390x/Makefile.target
> > +++ b/tests/tcg/s390x/Makefile.target
> > @@ -1,4 +1,5 @@
> > -VPATH+=$(SRC_PATH)/tests/tcg/s390x
> > +S390X_SRC=$(SRC_PATH)/tests/tcg/s390x
> > +VPATH+=$(S390X_SRC)
> >   CFLAGS+=-march=zEC12 -m64
> >   TESTS+=hello-s390x
> >   TESTS+=csst
> > @@ -9,3 +10,17 @@ TESTS+=pack
> >   TESTS+=mvo
> >   TESTS+=mvc
> >   TESTS+=trap
> > +TESTS+=signals-s390x
> > +
> > +ifneq ($(HAVE_GDB_BIN),)
> > +GDB_SCRIPT=$(SRC_PATH)/tests/guest-debug/run-test.py
> > +
> > +run-gdbstub-signals-s390x: signals-s390x
> > +       $(call run-test, $@, $(GDB_SCRIPT) \
> > +               --gdb $(HAVE_GDB_BIN) \
> > +               --qemu $(QEMU) --qargs "$(QEMU_OPTS)" \
> > +               --bin $< --test $(S390X_SRC)/gdbstub/test-signals-
> > s390x.py, \
> > +       "mixing signals and debugging on s390x")
> > +
> > +EXTRA_RUNS += run-gdbstub-signals-s390x
> > +endif
> > diff --git a/tests/tcg/s390x/gdbstub/test-signals-s390x.py
> > b/tests/tcg/s390x/gdbstub/test-signals-s390x.py
> > new file mode 100644
> > index 0000000000..80a284b475
> > --- /dev/null
> > +++ b/tests/tcg/s390x/gdbstub/test-signals-s390x.py
> > @@ -0,0 +1,76 @@
> > +from __future__ import print_function
> > +
> > +#
> > +# Test that signals and debugging mix well together on s390x.
> > +#
> > +# This is launched via tests/guest-debug/run-test.py
> > +#
> > +
> > +import gdb
> > +import sys
> > +
> > +failcount = 0
> > +
> > +
> > +def report(cond, msg):
> > +    """Report success/fail of test"""
> > +    if cond:
> > +        print("PASS: %s" % (msg))
> > +    else:
> > +        print("FAIL: %s" % (msg))
> > +        global failcount
> > +        failcount += 1
> > +
> > +
> > +def run_test():
> > +    """Run through the tests one by one"""
> > +    illegal_op = gdb.Breakpoint("illegal_op")
> > +    stg = gdb.Breakpoint("stg")
> > +    mvc_8 = gdb.Breakpoint("mvc_8")
> > +
> > +    # Expect the following events:
> > +    # 1x illegal_op breakpoint
> > +    # 2x stg breakpoint, segv, breakpoint
> > +    # 2x mvc_8 breakpoint, segv, breakpoint
> > +    for _ in range(14):
> 
> How do we come up with the value 14?

1 (initial) + 1 (illegal op) + 2 * 3 (stg) + 2 * 3 (mvc_8).

> 
> > +        gdb.execute("c")
> > +    report(illegal_op.hit_count == 1, "illegal_op.hit_count == 1")
> > +    report(stg.hit_count == 4, "stg.hit_count == 4")
> 
> The doc above says we should see this twice, why do we see it 4
> times?

With "2x stg breakpoint, segv, breakpoint" I meant: stg break, stg
segv, stg break, stg break, stg segv, stg break.

> > +    report(mvc_8.hit_count == 4, "mvc_8.hit_count == 4")
> > +
> 
> Dito
> 
> [...]
> 
> > diff --git a/tests/tcg/s390x/signals-s390x.c
> > b/tests/tcg/s390x/signals-s390x.c
> > new file mode 100644
> > index 0000000000..dc2f8ee59a
> > --- /dev/null
> > +++ b/tests/tcg/s390x/signals-s390x.c
> > @@ -0,0 +1,165 @@
> > +#include <assert.h>
> > +#include <signal.h>
> > +#include <string.h>
> > +#include <sys/mman.h>
> > +#include <ucontext.h>
> > +#include <unistd.h>
> > +
> > +/*
> > + * Various instructions that generate SIGILL and SIGSEGV. They
> > could have been
> > + * defined in a separate .s file, but this would complicate the
> > build, so the
> > + * inline asm is used instead.
> > + */
> > +
> > +void illegal_op(void);
> > +void after_illegal_op(void);
> > +asm(".globl\tillegal_op\n"
> > +    "illegal_op:\t.byte\t0x00,0x00\n"
> > +    "\t.globl\tafter_illegal_op\n"
> > +    "after_illegal_op:\tbr\t%r14");
> > +
> > +void stg(void *dst, unsigned long src);
> > +asm(".globl\tstg\n"
> > +    "stg:\tstg\t%r3,0(%r2)\n"
> > +    "\tbr\t%r14");
> > +
> > +void mvc_8(void *dst, void *src);
> > +asm(".globl\tmvc_8\n"
> > +    "mvc_8:\tmvc\t0(8,%r2),0(%r3)\n"
> > +    "\tbr\t%r14");
> 
> I was wondering if there would be any nicer way to write this,
> like (very prototype and wrong)
> 
> 
> static void stg(void *dst, unsigned long src)
> {
>      asm volatile("stg %r3,0(%r2)\n");
> }
> 
> static void mvc_8(void *dst, void *src)
> {
>      asm volatile("mvc 0(8,%r2),0(%r3)\n");
> }

The prologue would get in the way, and I don't think gcc has
__attribute__((naked)) for s390.

> Please ignore if that just doesn't make any sense.
> 
> Nothing else jumped at me :)