Re: [Qemu-devel] [PATCH] Add single stepping option for all targets

[Aurelien Jarno]

On Fri, Mar 13, 2009 at 05:35:05PM +0100, Stefan Weil wrote:
> This is an update of my patch which adds a new command line
> option -singlestep, now for both system and user mode.
> 
> So it now respects the feedback from Stuart, Laurent and
> Aurelien who all wanted user mode support, too.
> 
> There remains one open feedback point which I did not
> understand - see below.
> 
> I hope the patch will be added to trunk finally...
> 
> Regards
> Stefan Weil
> 
> 
> 
> Aurelien Jarno schrieb:
> > On Sun, Feb 01, 2009 at 08:51:13PM +0100, Stefan Weil wrote:
> >> Stefan Weil schrieb:
> >>> This patch replaces the compile time options SH4_SINGLE_STEP,
> >>> DO_SINGLE_STEP and MIPS_SINGLE_STEP
> >>> by a command line option -singlestep.
> >>>
> >>> It also adds single step mode for targets which did not have a compile
> >>> time option,
> >>> so all system emulations can be used with -singlestep. Please note that
> >>> I did only run a short test for i386 and mips targets.
> >>>
> >>> A new monitor command is provided to enable or disable single step mode.
> >>> The monitor command "info status" was modified to display single step
> >>> mode when activated.
> >>>
> >>> Single stepping in Qemu's system emulation mode is useful to see the cpu
> >>> state
> >>> for each cpu instruction when used with -d in_asm,cpu. It is also a
> >>> simple way to slow down the emulation.
> >>>
> >>>
> >
> > Please find my comments below.
> >
> >> Signed-off-by: Stefan Weil <address@hidden>
> >> ...
> >> Index: trunk/vl.c
> >> ===================================================================
> >> --- trunk.orig/vl.c 2009-02-01 19:12:41.000000000 +0100
> >> +++ trunk/vl.c 2009-02-01 19:18:33.000000000 +0100
> >> @@ -193,6 +193,7 @@
> >> int nb_nics;
> >> NICInfo nd_table[MAX_NICS];
> >> int vm_running;
> >> +int vm_singlestep;
> >> static int rtc_utc = 1;
> >> static int rtc_date_offset = -1; /* -1 means no change */
> >> int cirrus_vga_enabled = 1;
> >> @@ -3984,6 +3985,7 @@
> >> "-parallel dev redirect the parallel port to char device 'dev'\n"
> >> "-monitor dev redirect the monitor to char device 'dev'\n"
> >> "-pidfile file write PID to 'file'\n"
> >> + "-singlestep always run in singlestep mode\n"
> >> "-S freeze CPU at startup (use 'c' to start execution)\n"
> >> "-s wait gdb connection to port\n"
> >> "-p port set gdb connection port [default=%s]\n"
> >> @@ -4119,6 +4121,7 @@
> >> QEMU_OPTION_parallel,
> >> QEMU_OPTION_monitor,
> >> QEMU_OPTION_pidfile,
> >> + QEMU_OPTION_singlestep,
> >> QEMU_OPTION_S,
> >> QEMU_OPTION_s,
> >> QEMU_OPTION_p,
> >> @@ -4238,6 +4241,7 @@
> >> { "parallel", HAS_ARG, QEMU_OPTION_parallel },
> >> { "monitor", HAS_ARG, QEMU_OPTION_monitor },
> >> { "pidfile", HAS_ARG, QEMU_OPTION_pidfile },
> >> + { "singlestep", 0, QEMU_OPTION_singlestep },
> >> { "S", 0, QEMU_OPTION_S },
> >> { "s", 0, QEMU_OPTION_s },
> >> { "p", HAS_ARG, QEMU_OPTION_p },
> >
> > It might be a good idea to provide that as a subset of the -d option, as
> > it is clearly something only useful for debugging.
> 
> 
> Today -d does not take suboptions.
> Could you please explain your proposal with more details?
> 
> Stefan
> 

Given those explanations and the comments from other people, I am fine
with this option. I still have some comments though (see below).


> Add new command line option -singlestep for tcg single stepping.
> 
> This replaces a compile time option for some targets and adds
> this feature to targets which did not have a compile time option.
> 
> Add monitor command to enable or disable single step mode.
> 
> Modify monitor command "info status" to display single step mode.
> 
> Signed-off-by: Stefan Weil <address@hidden>
> 
> Index: trunk/target-sh4/translate.c
> ===================================================================
> --- trunk.orig/target-sh4/translate.c 2009-03-13 10:13:39.000000000 +0100
> +++ trunk/target-sh4/translate.c      2009-03-13 17:08:01.000000000 +0100
> @@ -1929,9 +1929,8 @@
>           break;
>          if (num_insns >= max_insns)
>              break;
> -#ifdef SH4_SINGLE_STEP
> -     break;
> -#endif
> +        if (vm_singlestep)
> +            break;
>      }
>      if (tb->cflags & CF_LAST_IO)
>          gen_io_end();
> Index: trunk/target-cris/translate.c
> ===================================================================
> --- trunk.orig/target-cris/translate.c        2009-03-13 10:13:39.000000000 
> +0100
> +++ trunk/target-cris/translate.c     2009-03-13 17:08:01.000000000 +0100
> @@ -3272,6 +3272,7 @@
>                       break;
>       } while (!dc->is_jmp && !dc->cpustate_changed
>                && gen_opc_ptr < gen_opc_end
> +              && !vm_singlestep
>                && (dc->pc < next_page_start)
>                   && num_insns < max_insns);
>  
> Index: trunk/target-alpha/translate.c
> ===================================================================
> --- trunk.orig/target-alpha/translate.c       2009-03-13 10:13:39.000000000 
> +0100
> +++ trunk/target-alpha/translate.c    2009-03-13 17:08:01.000000000 +0100
> @@ -2413,11 +2413,10 @@
>          if (env->singlestep_enabled) {
>              gen_excp(&ctx, EXCP_DEBUG, 0);
>              break;
> -     }
> +        }
>  
> -#if defined (DO_SINGLE_STEP)
> -        break;
> -#endif
> +        if (vm_singlestep)
> +            break;
>      }
>      if (ret != 1 && ret != 3) {
>          tcg_gen_movi_i64(cpu_pc, ctx.pc);
> Index: trunk/vl.c
> ===================================================================
> --- trunk.orig/vl.c   2009-03-13 17:07:47.000000000 +0100
> +++ trunk/vl.c        2009-03-13 17:08:01.000000000 +0100
> @@ -211,6 +211,7 @@
>  int nb_nics;
>  NICInfo nd_table[MAX_NICS];
>  int vm_running;
> +int vm_singlestep;

You create a new variable. By the way, I think that calling it
singlestep is better, and matches the naming of other options
variable (like daemonize, graphic_rotate). You should define it
to a default value of 0.

>  static int autostart;
>  static int rtc_utc = 1;
>  static int rtc_date_offset = -1; /* -1 means no change */
> @@ -4081,6 +4082,7 @@
>             "-parallel dev   redirect the parallel port to char device 
> 'dev'\n"
>             "-monitor dev    redirect the monitor to char device 'dev'\n"
>             "-pidfile file   write PID to 'file'\n"
> +           "-singlestep     always run in singlestep mode\n"
>             "-S              freeze CPU at startup (use 'c' to start 
> execution)\n"
>             "-s              wait gdb connection to port\n"
>             "-p port         set gdb connection port [default=%s]\n"
> @@ -4221,6 +4223,7 @@
>      QEMU_OPTION_parallel,
>      QEMU_OPTION_monitor,
>      QEMU_OPTION_pidfile,
> +    QEMU_OPTION_singlestep,
>      QEMU_OPTION_S,
>      QEMU_OPTION_s,
>      QEMU_OPTION_p,
> @@ -4345,6 +4348,7 @@
>      { "parallel", HAS_ARG, QEMU_OPTION_parallel },
>      { "monitor", HAS_ARG, QEMU_OPTION_monitor },
>      { "pidfile", HAS_ARG, QEMU_OPTION_pidfile },
> +    { "singlestep", 0, QEMU_OPTION_singlestep },
>      { "S", 0, QEMU_OPTION_S },
>      { "s", 0, QEMU_OPTION_s },
>      { "p", HAS_ARG, QEMU_OPTION_p },

This option is never parsed, so the -singlestep option doesn't work.

> Index: trunk/target-ppc/translate.c
> ===================================================================
> --- trunk.orig/target-ppc/translate.c 2009-03-13 17:06:19.000000000 +0100
> +++ trunk/target-ppc/translate.c      2009-03-13 17:08:01.000000000 +0100
> @@ -39,7 +39,6 @@
>  #define GDBSTUB_SINGLE_STEP 0x4
>  
>  /* Include definitions for instructions classes and implementations flags */
> -//#define DO_SINGLE_STEP
>  //#define PPC_DEBUG_DISAS
>  //#define DO_PPC_STATISTICS
>  
> @@ -8294,9 +8293,9 @@
>               */
>              break;
>          }
> -#if defined (DO_SINGLE_STEP)
> -        break;
> -#endif
> +
> +        if (vm_singlestep)
> +            break;
>      }
>      if (tb->cflags & CF_LAST_IO)
>          gen_io_end();
> Index: trunk/target-mips/translate.c
> ===================================================================
> --- trunk.orig/target-mips/translate.c        2009-03-13 10:13:39.000000000 
> +0100
> +++ trunk/target-mips/translate.c     2009-03-13 17:08:01.000000000 +0100
> @@ -38,7 +38,6 @@
>  
>  //#define MIPS_DEBUG_DISAS
>  //#define MIPS_DEBUG_SIGN_EXTENSIONS
> -//#define MIPS_SINGLE_STEP
>  
>  /* MIPS major opcodes */
>  #define MASK_OP_MAJOR(op)  (op & (0x3F << 26))
> @@ -8245,9 +8244,9 @@
>  
>          if (num_insns >= max_insns)
>              break;
> -#if defined (MIPS_SINGLE_STEP)
> -        break;
> -#endif
> +
> +        if (vm_singlestep)
> +            break;
>      }
>      if (tb->cflags & CF_LAST_IO)
>          gen_io_end();
> Index: trunk/monitor.c
> ===================================================================
> --- trunk.orig/monitor.c      2009-03-13 10:13:39.000000000 +0100
> +++ trunk/monitor.c   2009-03-13 17:08:01.000000000 +0100
> @@ -527,6 +527,18 @@
>      cpu_set_log(mask);
>  }
>  
> +static void do_singlestep(Monitor *mon, const char *option)
> +{
> +    qemu_printf("setting vm_singlestep to %s\n", option);
> +    if (!option) {
> +        vm_singlestep = 1;
> +    } else if (!strcmp(option, "off")) {
> +        vm_singlestep = 0;
> +    } else {
> +        monitor_printf(mon, "unexpected option %s\n", option);
> +    }
> +}
> +
>  static void do_stop(Monitor *mon)
>  {
>      vm_stop(EXCP_INTERRUPT);
> @@ -1508,9 +1520,13 @@
>  
>  static void do_info_status(Monitor *mon)
>  {
> -    if (vm_running)
> -       monitor_printf(mon, "VM status: running\n");
> -    else
> +    if (vm_running) {
> +        if (vm_singlestep) {
> +            monitor_printf(mon, "VM status: running (single step mode)\n");
> +        } else {
> +            monitor_printf(mon, "VM status: running\n");
> +        }
> +    } else
>         monitor_printf(mon, "VM status: paused\n");
>  }
>  
> @@ -1641,6 +1657,8 @@
>        "tag|id", "restore a VM snapshot from its tag or id" },
>      { "delvm", "s", do_delvm,
>        "tag|id", "delete a VM snapshot from its tag or id" },
> +    { "singlestep", "s?", do_singlestep,
> +      "[off]", "run emulation in singlestep mode or switch to normal mode", 
> },
>      { "stop", "", do_stop,
>        "", "stop emulation", },
>      { "c|cont", "", do_cont,
> Index: trunk/target-i386/translate.c
> ===================================================================
> --- trunk.orig/target-i386/translate.c        2009-03-13 17:06:23.000000000 
> +0100
> +++ trunk/target-i386/translate.c     2009-03-13 17:08:01.000000000 +0100
> @@ -7651,6 +7651,11 @@
>              gen_eob(dc);
>              break;
>          }
> +        if (vm_singlestep) {
> +            gen_jmp_im(pc_ptr - dc->cs_base);
> +            gen_eob(dc);
> +            break;
> +        }
>      }
>      if (tb->cflags & CF_LAST_IO)
>          gen_io_end();
> Index: trunk/target-arm/translate.c
> ===================================================================
> --- trunk.orig/target-arm/translate.c 2009-03-13 17:06:23.000000000 +0100
> +++ trunk/target-arm/translate.c      2009-03-13 17:08:01.000000000 +0100
> @@ -8788,7 +8788,7 @@
>           * ensures prefetch aborts occur at the right place.  */
>          num_insns ++;
>      } while (!dc->is_jmp && gen_opc_ptr < gen_opc_end &&
> -             !env->singlestep_enabled &&
> +             !env->singlestep_enabled && !vm_singlestep &&
>               dc->pc < next_page_start &&
>               num_insns < max_insns);
>  
> Index: trunk/target-m68k/translate.c
> ===================================================================
> --- trunk.orig/target-m68k/translate.c        2009-03-13 17:06:18.000000000 
> +0100
> +++ trunk/target-m68k/translate.c     2009-03-13 17:08:01.000000000 +0100
> @@ -3030,7 +3030,7 @@
>       disas_m68k_insn(env, dc);
>          num_insns++;
>      } while (!dc->is_jmp && gen_opc_ptr < gen_opc_end &&
> -             !env->singlestep_enabled &&
> +             !env->singlestep_enabled && !vm_singlestep &&
>               (pc_offset) < (TARGET_PAGE_SIZE - 32) &&
>               num_insns < max_insns);
>  
> Index: trunk/target-sparc/translate.c
> ===================================================================
> --- trunk.orig/target-sparc/translate.c       2009-03-13 10:13:39.000000000 
> +0100
> +++ trunk/target-sparc/translate.c    2009-03-13 17:08:01.000000000 +0100
> @@ -4858,7 +4858,7 @@
>              break;
>          /* if single step mode, we generate only one instruction and
>             generate an exception */
> -        if (env->singlestep_enabled) {
> +        if (env->singlestep_enabled || vm_singlestep) {
>              tcg_gen_movi_tl(cpu_pc, dc->pc);
>              tcg_gen_exit_tb(0);
>              break;
> Index: trunk/qemu-doc.texi
> ===================================================================
> --- trunk.orig/qemu-doc.texi  2009-03-13 10:13:39.000000000 +0100
> +++ trunk/qemu-doc.texi       2009-03-13 17:08:01.000000000 +0100
> @@ -1100,6 +1100,9 @@
>  @item -s
>  Wait gdb connection to port 1234 (@pxref{gdb_usage}).
>  
> address@hidden -singlestep
> +Run the emulation in single step mode.
> +
>  @item -p @var{port}
>  Change gdb connection port.  @var{port} can be either a decimal number
>  to specify a TCP port, or a host device (same devices as the serial port).
> @@ -1463,6 +1466,10 @@
>  @item delvm @var{tag}|@var{id}
>  Delete the snapshot identified by @var{tag} or @var{id}.
>  
> address@hidden singlestep [off]
> +Run the emulation in single step mode.
> +If called with option off, the emulation returns to normal mode.
> +
>  @item stop
>  Stop emulation.
>  
> Index: trunk/exec-all.h
> ===================================================================
> --- trunk.orig/exec-all.h     2009-03-13 17:07:56.000000000 +0100
> +++ trunk/exec-all.h  2009-03-13 17:08:01.000000000 +0100
> @@ -381,6 +381,8 @@
>  
>  #endif
>  
> +extern int vm_singlestep;
> +
>  typedef void (CPUDebugExcpHandler)(CPUState *env);
>  
>  CPUDebugExcpHandler *cpu_set_debug_excp_handler(CPUDebugExcpHandler 
> *handler);
> Index: trunk/bsd-user/main.c
> ===================================================================
> --- trunk.orig/bsd-user/main.c        2009-03-13 10:13:39.000000000 +0100
> +++ trunk/bsd-user/main.c     2009-03-13 17:08:01.000000000 +0100
> @@ -33,6 +33,8 @@
>  
>  #define DEBUG_LOGFILE "/tmp/qemu.log"
>  
> +int vm_singlestep;
> +
>  static const char *interp_prefix = CONFIG_QEMU_PREFIX;
>  const char *qemu_uname_release = CONFIG_UNAME_RELEASE;
>  extern char **environ;
> @@ -378,6 +380,7 @@
>             "Debug options:\n"
>             "-d options   activate log (logfile=%s)\n"
>             "-p pagesize  set the host page size to 'pagesize'\n"
> +           "-singlestep  always run in singlestep mode\n"
>             "-strace      log system calls\n"
>             "\n"
>             "Environment variables:\n"
> @@ -500,6 +503,8 @@
>                  usage();
>              }
>              optind++;
> +        } else if (!strcmp(r, "singlestep")) {
> +            vm_singlestep = 1;
>          } else if (!strcmp(r, "strace")) {
>              do_strace = 1;
>          } else
> Index: trunk/darwin-user/main.c
> ===================================================================
> --- trunk.orig/darwin-user/main.c     2009-03-13 17:07:43.000000000 +0100
> +++ trunk/darwin-user/main.c  2009-03-13 17:08:01.000000000 +0100
> @@ -41,6 +41,8 @@
>  #include <mach/mach_init.h>
>  #include <mach/vm_map.h>
>  
> +int vm_singlestep;
> +
>  const char *interp_prefix = "";
>  
>  asm(".zerofill __STD_PROG_ZONE, __STD_PROG_ZONE, __std_prog_zone, 
> 0x0dfff000");
> @@ -751,6 +753,7 @@
>             "-d options   activate log (logfile='%s')\n"
>             "-g wait for gdb on port 1234\n"
>             "-p pagesize  set the host page size to 'pagesize'\n",
> +           "-singlestep  always run in singlestep mode\n"
>             TARGET_ARCH,
>             TARGET_ARCH,
>             interp_prefix,
> @@ -842,6 +845,8 @@
>  #endif
>                  exit(1);
>              }
> +        } else if (!strcmp(r, "singlestep")) {
> +            vm_singlestep = 1;
>          } else
>          {
>              usage();
> Index: trunk/linux-user/main.c
> ===================================================================
> --- trunk.orig/linux-user/main.c      2009-03-13 17:07:43.000000000 +0100
> +++ trunk/linux-user/main.c   2009-03-13 17:08:01.000000000 +0100
> @@ -39,6 +39,8 @@
>  
>  char *exec_path;
>  
> +int vm_singlestep;
> +
>  static const char *interp_prefix = CONFIG_QEMU_PREFIX;
>  const char *qemu_uname_release = CONFIG_UNAME_RELEASE;
>  
> @@ -2217,6 +2219,7 @@
>             "Debug options:\n"
>             "-d options   activate log (logfile=%s)\n"
>             "-p pagesize  set the host page size to 'pagesize'\n"
> +           "-singlestep  always run in singlestep mode\n"
>             "-strace      log system calls\n"
>             "\n"
>             "Environment variables:\n"
> @@ -2359,6 +2362,8 @@
>              }
>          } else if (!strcmp(r, "drop-ld-preload")) {
>              (void) envlist_unsetenv(envlist, "LD_PRELOAD");
> +        } else if (!strcmp(r, "singlestep")) {
> +            vm_singlestep = 1;
>          } else if (!strcmp(r, "strace")) {
>              do_strace = 1;
>          } else


-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
address@hidden                 http://www.aurel32.net