Re: PING: [PATCH 2/2] tests/tcg/s390x: Test unwinding from signal handlers

[Laurent Vivier]

Le 24/05/2022 à 11:56, Thomas Huth a écrit :
> On 19/05/2022 13.34, Ilya Leoshkevich wrote:
> > On Wed, 2022-05-04 at 00:51 +0200, Ilya Leoshkevich wrote:
> > > Add a small test to prevent regressions.
> > >
> > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > > ---
> > >   tests/tcg/s390x/signals-s390x.c | 69 ++++++++++++++++++++++++++-----
> > > --
> > >   1 file changed, 55 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/tests/tcg/s390x/signals-s390x.c
> > > b/tests/tcg/s390x/signals-s390x.c
> > > index dc2f8ee59a..48c3b6cdfd 100644
> > > --- a/tests/tcg/s390x/signals-s390x.c
> > > +++ b/tests/tcg/s390x/signals-s390x.c
> > > @@ -1,4 +1,5 @@
> > >   #include <assert.h>
> > > +#include <execinfo.h>
> > >   #include <signal.h>
> > >   #include <string.h>
> > >   #include <sys/mman.h>
> > > @@ -11,22 +12,28 @@
> > >    * inline asm is used instead.
> > >    */
> > > +#define DEFINE_ASM_FUNCTION(name, body) \
> > > +    asm(".globl " #name "\n" \
> > > +        #name ":\n" \
> > > +        ".cfi_startproc\n" \
> > > +        body "\n" \
> > > +        "br %r14\n" \
> > > +        ".cfi_endproc");
> > > +
> > >   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");
> > > +extern const char after_illegal_op;
> > > +DEFINE_ASM_FUNCTION(illegal_op,
> > > +    ".byte 0x00,0x00\n"
> > > +    ".globl after_illegal_op\n"
> > > +    "after_illegal_op:")
> > >   void stg(void *dst, unsigned long src);
> > > -asm(".globl\tstg\n"
> > > -    "stg:\tstg\t%r3,0(%r2)\n"
> > > -    "\tbr\t%r14");
> > > +DEFINE_ASM_FUNCTION(stg, "stg %r3,0(%r2)")
> > >   void mvc_8(void *dst, void *src);
> > > -asm(".globl\tmvc_8\n"
> > > -    "mvc_8:\tmvc\t0(8,%r2),0(%r3)\n"
> > > -    "\tbr\t%r14");
> > > +DEFINE_ASM_FUNCTION(mvc_8, "mvc 0(8,%r2),0(%r3)")
> > > +
> > > +extern const char return_from_main_1;
> > >   static void safe_puts(const char *s)
> > >   {
> > > @@ -49,8 +56,9 @@ static struct {
> > >   static void handle_signal(int sig, siginfo_t *info, void *ucontext)
> > >   {
> > > +    int err, i, n_frames;
> > > +    void *frames[16];
> > >       void *page;
> > > -    int err;
> > >       if (sig != expected.sig) {
> > >           safe_puts("[  FAILED  ] wrong signal");
> > > @@ -86,6 +94,17 @@ static void handle_signal(int sig, siginfo_t
> > > *info, void *ucontext)
> > >       default:
> > >           break;
> > >       }
> > > +
> > > +    n_frames = backtrace(frames, sizeof(frames) /
> > > sizeof(frames[0]));
> > > +    for (i = 0; i < n_frames; i++) {
> > > +        if (frames[i] == &return_from_main_1) {
> > > +            break;
> > > +        }
> > > +    }
> > > +    if (i == n_frames) {
> > > +        safe_puts("[  FAILED  ] backtrace() is broken");
> > > +        _exit(1);
> > > +    }
> > >   }
> > >   static void check_sigsegv(void *func, enum exception exception,
> > > @@ -122,7 +141,7 @@ static void check_sigsegv(void *func, enum
> > > exception exception,
> > >       assert(err == 0);
> > >   }
> > > -int main(void)
> > > +int main_1(void)
> > >   {
> > >       struct sigaction act;
> > >       int err;
> > > @@ -138,7 +157,7 @@ int main(void)
> > >       safe_puts("[ RUN      ] Operation exception");
> > >       expected.sig = SIGILL;
> > >       expected.addr = illegal_op;
> > > -    expected.psw_addr = (unsigned long)after_illegal_op;
> > > +    expected.psw_addr = (unsigned long)&after_illegal_op;
> > >       expected.exception = exception_operation;
> > >       illegal_op();
> > >       safe_puts("[       OK ]");
> > > @@ -163,3 +182,25 @@ int main(void)
> > >       _exit(0);
> > >   }
> > > +
> > > +/*
> > > + * Define main() in assembly in order to test that unwinding from
> > > signal
> > > + * handlers until main() works. This way we can define a specific
> > > point that
> > > + * the unwinder should reach. This is also better than defining
> > > main() in C
> > > + * and using inline assembly to call main_1(), since it's not easy
> > > to get all
> > > + * the clobbers right.
> > > + */
> > > +
> > > +DEFINE_ASM_FUNCTION(main,
> > > +    "stmg %r14,%r15,112(%r15)\n"
> > > +    ".cfi_offset 14,-48\n"
> > > +    ".cfi_offset 15,-40\n"
> > > +    "lay %r15,-160(%r15)\n"
> > > +    ".cfi_def_cfa_offset 320\n"
> > > +    "brasl %r14,main_1\n"
> > > +    ".globl return_from_main_1\n"
> > > +    "return_from_main_1:\n"
> > > +    "lmg %r14,%r15,272(%r15)\n"
> > > +    ".cfi_restore 15\n"
> > > +    ".cfi_restore 14\n"
> > > +    ".cfi_def_cfa_offset 160");
> >
> > Ping.
>
> Acked-by: Thomas Huth <thuth@redhat.com>
>
> Laurent, do you want to take these two patches through your linux-user branch, or shall I take them 
> via the s390x branch?

I will take both.

Thanks,
Laurent