Re: [Qemu-devel] [RISU PATCH v3 04/10] risu: add simple trace and replay support

[Peter Maydell]

On 9 December 2016 at 11:48, Alex Bennée <address@hidden> wrote:
> This adds a very dumb and easily breakable trace and replay support. In
> --master mode the various risu ops trigger a write of register/memory
> state into a binary file which can be played back to an apprentice.
> Currently there is no validation of the image source so feeding the
> wrong image will fail straight away.
>
> The trace files will get very big for any appreciable sized test file
> and this will be addressed in later patches.
>
> Signed-off-by: Alex Bennée <address@hidden>

Most of this looks good.

> ---
>
> v2
>   - moved read/write functions into main risu.c
>   - cleaned up formatting
>   - report more in apprentice --trace mode
> v3
>   - fix options parsing
>   - re-factored so no need for copy & paste
> ---
>  risu.c         | 108 
> +++++++++++++++++++++++++++++++++++++++++++++++----------
>  risu_aarch64.c |   5 ++-
>  risu_arm.c     |   4 +--
>  3 files changed, 93 insertions(+), 24 deletions(-)
>
> diff --git a/risu.c b/risu.c
> index 22571cd..173dd3c 100644
> --- a/risu.c
> +++ b/risu.c
> @@ -31,6 +31,7 @@
>  void *memblock = 0;
>
>  int apprentice_socket, master_socket;
> +int trace_file = 0;
>
>  sigjmp_buf jmpbuf;
>
> @@ -40,10 +41,12 @@ int test_fp_exc = 0;
>  long executed_tests = 0;
>  void report_test_status(void *pc)
>  {
> -   executed_tests += 1;
> -   if (executed_tests % 100 == 0) {
> -      fprintf(stderr,"Executed %ld test instructions (pc=%p)\r",
> -              executed_tests, pc);
> +   if (ismaster || trace_file) {
> +      executed_tests += 1;
> +      if (executed_tests % 100 == 0) {
> +         fprintf(stderr,"Executed %ld test instructions (pc=%p)\r",
> +                 executed_tests, pc);
> +      }
>     }
>  }
>
> @@ -54,6 +57,12 @@ int read_sock(void *ptr, size_t bytes)
>     return recv_data_pkt(master_socket, ptr, bytes);
>  }
>
> +int write_trace(void *ptr, size_t bytes)
> +{
> +   size_t res = write(trace_file, ptr, bytes);
> +   return (res == bytes) ? 0 : 1;
> +}
> +
>  void respond_sock(int r)
>  {
>     send_response_byte(master_socket, r);
> @@ -66,26 +75,62 @@ int write_sock(void *ptr, size_t bytes)
>     return send_data_pkt(apprentice_socket, ptr, bytes);
>  }
>
> +int read_trace(void *ptr, size_t bytes)
> +{
> +   size_t res = read(trace_file, ptr, bytes);
> +   return (res == bytes) ? 0 : 1;
> +}
> +
> +void respond_trace(int r)
> +{
> +   switch (r) {
> +      case 0: /* test ok */
> +      case 1: /* end of test */
> +         break;
> +      default:
> +         fprintf(stderr,"%s: got response of %d\n", __func__, r);

Is this a "something is broken/can't happen" case?

Anyway, you're in a signal handler so shouldn't use fprintf.

> +         break;
> +   }
> +}
> +
>  void master_sigill(int sig, siginfo_t *si, void *uc)
>  {
> -   switch (recv_and_compare_register_info(read_sock, respond_sock, uc))
> +   int r;
> +
> +   if (trace_file) {
> +      r = send_register_info(write_trace, uc);
> +   } else {
> +      r = recv_and_compare_register_info(read_sock, respond_sock, uc);
> +   }
> +
> +   switch (r)
>     {
>        case 0:
> -         /* match OK */
> +         /* All OK */
>           advance_pc(uc);
>           return;
>        default:
>           /* mismatch, or end of test */
>           siglongjmp(jmpbuf, 1);
> +         /* never */
> +         return;

Is your compiler complaining about this? You should probably
get a better compiler (others will likely complain that the
return is dead, because siglongjmp is marked 'noreturn' in the
system headers).

>     }
>  }
>
>  void apprentice_sigill(int sig, siginfo_t *si, void *uc)
>  {
> -   switch (send_register_info(write_sock, uc))
> +   int r;
> +
> +   if (trace_file) {
> +      r = recv_and_compare_register_info(read_trace, respond_trace, uc);
> +   } else {
> +      r = send_register_info(write_sock, uc);
> +   }
> +
> +   switch (r)
>     {
>        case 0:
> -         /* match OK */
> +         /* All OK */
>           advance_pc(uc);
>           return;
>        case 1:
> @@ -94,6 +139,9 @@ void apprentice_sigill(int sig, siginfo_t *si, void *uc)
>           exit(0);
>        default:
>           /* mismatch */
> +         if (trace_file) {
> +            report_match_status();
> +         }
>           exit(1);
>     }
>  }
> @@ -155,7 +203,13 @@ int master(int sock)
>  {
>     if (sigsetjmp(jmpbuf, 1))
>     {
> -      return report_match_status();
> +      if (trace_file) {
> +         close(trace_file);
> +         fprintf(stderr,"\nDone...\n");
> +         return 0;
> +      } else {
> +         return report_match_status();
> +      }
>     }
>     master_socket = sock;
>     set_sigill_handler(&master_sigill);
> @@ -184,6 +238,7 @@ void usage (void)
>     fprintf(stderr, "between master and apprentice risu processes.\n\n");
>     fprintf(stderr, "Options:\n");
>     fprintf(stderr, "  --master          Be the master (server)\n");
> +   fprintf(stderr, "  -t, --trace=FILE  Record/playback trace file\n");
>     fprintf(stderr, "  -h, --host=HOST   Specify master host machine 
> (apprentice only)\n");
>     fprintf(stderr, "  -p, --port=PORT   Specify the port to connect 
> to/listen on (default 9191)\n");
>  }
> @@ -194,6 +249,7 @@ int main(int argc, char **argv)
>     uint16_t port = 9191;
>     char *hostname = "localhost";
>     char *imgfile;
> +   char *trace_fn = NULL;
>     int sock;
>
>     // TODO clean this up later
> @@ -204,13 +260,14 @@ int main(int argc, char **argv)
>           {
>              { "help", no_argument, 0, '?'},
>              { "master", no_argument, &ismaster, 1 },
> +            { "trace", required_argument, 0, 't' },
>              { "host", required_argument, 0, 'h' },
>              { "port", required_argument, 0, 'p' },
>              { "test-fp-exc", no_argument, &test_fp_exc, 1 },
>              { 0,0,0,0 }
>           };
>        int optidx = 0;
> -      int c = getopt_long(argc, argv, "h:p:", longopts, &optidx);
> +      int c = getopt_long(argc, argv, "h:p:t:", longopts, &optidx);
>        if (c == -1)
>        {
>           break;
> @@ -223,6 +280,11 @@ int main(int argc, char **argv)
>              /* flag set by getopt_long, do nothing */
>              break;
>           }
> +         case 't':
> +         {
> +           trace_fn = optarg;
> +           break;
> +         }
>           case 'h':
>           {
>              hostname = optarg;
> @@ -253,18 +315,28 @@ int main(int argc, char **argv)
>     }
>
>     load_image(imgfile);
> -
> +
>     if (ismaster)
>     {
> -      fprintf(stderr, "master port %d\n", port);
> -      sock = master_connect(port);
> -      return master(sock);
> +     if (trace_fn)
> +       {
> +         trace_file = open(trace_fn, O_WRONLY|O_CREAT, S_IRWXU);
> +       } else {
> +         fprintf(stderr, "master port %d\n", port);
> +         sock = master_connect(port);
> +       }

GNU coding standards indentation ? I know risu's indenting
is a bit of a mess but we have not quite sunk that low yet :-)

> +     return master(sock);
>     }
>     else
> -   {
> -      fprintf(stderr, "apprentice host %s port %d\n", hostname, port);
> -      sock = apprentice_connect(hostname, port);
> -      return apprentice(sock);
> +     {
> +     if (trace_fn)
> +       {
> +         trace_file = open(trace_fn, O_RDONLY);
> +       } else {
> +         fprintf(stderr, "apprentice host %s port %d\n", hostname, port);
> +         sock = apprentice_connect(hostname, port);
> +       }
> +     return apprentice(sock);
>     }
>  }
>
> diff --git a/risu_aarch64.c b/risu_aarch64.c
> index c4c0d4d..7f9f612 100644
> --- a/risu_aarch64.c
> +++ b/risu_aarch64.c
> @@ -13,6 +13,7 @@
>  #include <stdio.h>
>  #include <ucontext.h>
>  #include <string.h>
> +#include <unistd.h>

Why this include?

>
>  #include "risu.h"
>  #include "risu_reginfo_aarch64.h"
> @@ -28,9 +29,7 @@ void advance_pc(void *vuc)
>  {
>      ucontext_t *uc = vuc;
>      uc->uc_mcontext.pc += 4;
> -    if (ismaster) {
> -      report_test_status((void *) uc->uc_mcontext.pc);
> -    }
> +    report_test_status((void *) uc->uc_mcontext.pc);
>  }
>
>  static void set_x0(void *vuc, uint64_t x0)
> diff --git a/risu_arm.c b/risu_arm.c
> index 474729c..77bdcac 100644
> --- a/risu_arm.c
> +++ b/risu_arm.c
> @@ -50,9 +50,7 @@ void advance_pc(void *vuc)
>  {
>     ucontext_t *uc = vuc;
>     uc->uc_mcontext.arm_pc += insnsize(uc);
> -   if (ismaster) {
> -      report_test_status((void *) uc->uc_mcontext.arm_pc);
> -   }
> +   report_test_status((void *) uc->uc_mcontext.arm_pc);
>  }

These bits should just vanish if you fix or drop the
earlier patch that added report_test_status() calls here.

>
>  static void set_r0(void *vuc, uint32_t r0)
> --
> 2.11.0

thanks
-- PMM