Re: [Qemu-devel] [RISU PATCH v3 01/10] risu: a bit more verbosity when running

[Peter Maydell]

On 9 December 2016 at 11:48, Alex Bennée <address@hidden> wrote:
> Before this is could seem a little quite when running as you had no
> indication stuff was happening (or how fast). I only dump on the master
> side as I want to minimise the amount of qemu logs to sift through.

Yeah, more progress info would be good. (I used to have a patchset
that made risu print '.' every so often but I dunno where
that disappeared to.)

> Signed-off-by: Alex Bennée <address@hidden>
>
> --
> v3
>   - use portable fmt string for image_start_address
>   - include arm dumping position
> ---
>  risu.c         | 15 +++++++++++++--
>  risu.h         |  3 +++
>  risu_aarch64.c |  3 +++
>  risu_arm.c     |  3 +++
>  4 files changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/risu.c b/risu.c
> index 7e42160..bcdc219 100644
> --- a/risu.c
> +++ b/risu.c
> @@ -37,6 +37,16 @@ sigjmp_buf jmpbuf;
>  /* Should we test for FP exception status bits? */
>  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);
> +   }

I think this is too much info to be printing every 100
instructions, though. We should either just print '.'s or
print info less often.

Also, you're inside a signal handler, so trying to use
libc stdio is probably a bad idea. With '.' you can at
least stick to write() which is OK inside signal handlers.

> +}
> +
>  void master_sigill(int sig, siginfo_t *si, void *uc)
>  {
>     switch (recv_and_compare_register_info(master_socket, uc))
> @@ -61,6 +71,7 @@ void apprentice_sigill(int sig, siginfo_t *si, void *uc)
>           return;
>        case 1:
>           /* end of test */
> +         fprintf(stderr, "\nend of test\n");
>           exit(0);

This is also inside a signal handler. (If that's too annoying
you can do what master_sigill() does for this, and siglongjmp()
out to somewhere more convenient.)

>        default:
>           /* mismatch */

The default case is about to do 'exit(1);' -- should we print
something there too?

> @@ -129,7 +140,7 @@ int master(int sock)
>     }
>     master_socket = sock;
>     set_sigill_handler(&master_sigill);
> -   fprintf(stderr, "starting image\n");
> +   fprintf(stderr, "starting master image at 0x%"PRIxPTR"\n", 
> image_start_address);
>     image_start();
>     fprintf(stderr, "image returned unexpectedly\n");
>     exit(1);
> @@ -139,7 +150,7 @@ int apprentice(int sock)
>  {
>     apprentice_socket = sock;
>     set_sigill_handler(&apprentice_sigill);
> -   fprintf(stderr, "starting image\n");
> +   fprintf(stderr, "starting apprentice image at 0x%"PRIxPTR"\n", 
> image_start_address);
>     image_start();
>     fprintf(stderr, "image returned unexpectedly\n");
>     exit(1);
> diff --git a/risu.h b/risu.h
> index 26ed834..e4bb323 100644
> --- a/risu.h
> +++ b/risu.h
> @@ -26,6 +26,7 @@ extern uintptr_t image_start_address;
>  extern void *memblock;
>
>  extern int test_fp_exc;
> +extern int ismaster;
>
>  /* Ops code under test can request from risu: */
>  #define OP_COMPARE 0
> @@ -59,6 +60,8 @@ int recv_and_compare_register_info(int sock, void *uc);
>   */
>  int report_match_status(void);
>
> +void report_test_status(void *pc);
> +
>  /* Move the PC past this faulting insn by adjusting ucontext
>   */
>  void advance_pc(void *uc);
> diff --git a/risu_aarch64.c b/risu_aarch64.c
> index 547f987..1595604 100644
> --- a/risu_aarch64.c
> +++ b/risu_aarch64.c
> @@ -28,6 +28,9 @@ void advance_pc(void *vuc)
>  {
>      ucontext_t *uc = vuc;
>      uc->uc_mcontext.pc += 4;
> +    if (ismaster) {
> +      report_test_status((void *) uc->uc_mcontext.pc);
> +    }
>  }

I think it would be nicer to do the progress info
directly from master_sigill()/apprentice_sigill(). That
means that you don't have to change the advance_pc() function for
every architecture, and you don't need a global for ismaster either,
because you know by virtue of which function you were in.
(If you're dead set on printing the guest PC we should add a
uint64_t get_guest_pc(void *vuc) function for the progress info
function to use, but I'd rather not bother.)

>  static void set_x0(void *vuc, uint64_t x0)
> diff --git a/risu_arm.c b/risu_arm.c
> index bdfb59b..c3fe3d3 100644
> --- a/risu_arm.c
> +++ b/risu_arm.c
> @@ -50,6 +50,9 @@ 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);
> +   }
>  }
>
>  static void set_r0(void *vuc, uint32_t r0)
> --
> 2.11.0

thanks
-- PMM