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

[Aurelien Jarno]

On Mon, Mar 30, 2009 at 12:18:28PM +0200, Stefan Weil wrote:
> Aurelien Jarno schrieb:
> >
> > Given those explanations and the comments from other people, I am fine
> > with this option. I still have some comments though (see below).
> 
> Please see my comments and an updated patch below.

Thanks, applied.

> >
> >
> >> ...
> >> 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.
> >
> 
> Calling those option variables option_singlestep, option_daemonize
> might even be a better solution...
> 
> I called it vm_singlestep because it is somehow similar to vm_running.
> It is a variable which indicates a certain state of QEMU's VM.
> Nevertheless I renamed it to singlestep in my new patch.
> 
> The default value is already 0 because all globals in C/C++ have this
> default value (BSS segment). An explicit value just increases the size
> of the executable (only by 4 (or 8) bytes in this case, I admit).
> 
> Today, there is no consistent usage of global default values. Some
> globals are explicitly set to zero, others not. I personally use
> explicit default values only when they are needed (!= 0).
> 
> Please feel free to add a "= 0" if you think this should be QEMU's standard.
> 
> 
> >> @@ -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.
> 
> True. The parser code got lost somewhere from my first to this
> incomplete patch
> because of many, many merges in the meantime.
> 
> The new patch fixes this.
> 
> So I hope this will be the last iteration and the patch will finally
> find its
> way to QEMU trunk.
> 
> 
> Regards
> 
> Stefan
> 
> 
> 
> 

> 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/bsd-user/main.c
> ===================================================================
> --- trunk.orig/bsd-user/main.c        2009-03-30 11:29:04.000000000 +0200
> +++ trunk/bsd-user/main.c     2009-03-30 11:30:43.000000000 +0200
> @@ -33,6 +33,8 @@
>  
>  #define DEBUG_LOGFILE "/tmp/qemu.log"
>  
> +int 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")) {
> +            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-30 11:29:04.000000000 +0200
> +++ trunk/darwin-user/main.c  2009-03-30 11:30:43.000000000 +0200
> @@ -41,6 +41,8 @@
>  #include <mach/mach_init.h>
>  #include <mach/vm_map.h>
>  
> +int 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")) {
> +            singlestep = 1;
>          } else
>          {
>              usage();
> Index: trunk/exec-all.h
> ===================================================================
> --- trunk.orig/exec-all.h     2009-03-30 11:29:04.000000000 +0200
> +++ trunk/exec-all.h  2009-03-30 11:30:43.000000000 +0200
> @@ -381,6 +381,8 @@
>  
>  #endif
>  
> +extern int singlestep;
> +
>  typedef void (CPUDebugExcpHandler)(CPUState *env);
>  
>  CPUDebugExcpHandler *cpu_set_debug_excp_handler(CPUDebugExcpHandler 
> *handler);
> Index: trunk/linux-user/main.c
> ===================================================================
> --- trunk.orig/linux-user/main.c      2009-03-30 11:29:04.000000000 +0200
> +++ trunk/linux-user/main.c   2009-03-30 11:30:43.000000000 +0200
> @@ -39,6 +39,8 @@
>  
>  char *exec_path;
>  
> +int 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")) {
> +            singlestep = 1;
>          } else if (!strcmp(r, "strace")) {
>              do_strace = 1;
>          } else
> Index: trunk/monitor.c
> ===================================================================
> --- trunk.orig/monitor.c      2009-03-30 11:29:04.000000000 +0200
> +++ trunk/monitor.c   2009-03-30 11:43:16.000000000 +0200
> @@ -527,6 +527,17 @@
>      cpu_set_log(mask);
>  }
>  
> +static void do_singlestep(Monitor *mon, const char *option)
> +{
> +    if (!option || !strcmp(option, "on")) {
> +        singlestep = 1;
> +    } else if (!strcmp(option, "off")) {
> +        singlestep = 0;
> +    } else {
> +        monitor_printf(mon, "unexpected option %s\n", option);
> +    }
> +}
> +
>  static void do_stop(Monitor *mon)
>  {
>      vm_stop(EXCP_INTERRUPT);
> @@ -1510,9 +1521,13 @@
>  
>  static void do_info_status(Monitor *mon)
>  {
> -    if (vm_running)
> -       monitor_printf(mon, "VM status: running\n");
> -    else
> +    if (vm_running) {
> +        if (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");
>  }
>  
> @@ -1643,6 +1658,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,
> +      "[on|off]", "run emulation in singlestep mode or switch to normal 
> mode", },
>      { "stop", "", do_stop,
>        "", "stop emulation", },
>      { "c|cont", "", do_cont,
> Index: trunk/qemu-doc.texi
> ===================================================================
> --- trunk.orig/qemu-doc.texi  2009-03-30 11:29:04.000000000 +0200
> +++ trunk/qemu-doc.texi       2009-03-30 11:30:43.000000000 +0200
> @@ -490,6 +490,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.
>  
> @@ -2370,6 +2374,8 @@
>  Act as if the host page size was 'pagesize' bytes
>  @item -g port
>  Wait gdb connection to port
> address@hidden -singlestep
> +Run the emulation in single step mode.
>  @end table
>  
>  Environment variables:
> @@ -2488,6 +2494,8 @@
>  Activate log (logfile=/tmp/qemu.log)
>  @item -p pagesize
>  Act as if the host page size was 'pagesize' bytes
> address@hidden -singlestep
> +Run the emulation in single step mode.
>  @end table
>  
>  @node BSD User space emulator
> @@ -2550,6 +2558,8 @@
>  Activate log (logfile=/tmp/qemu.log)
>  @item -p pagesize
>  Act as if the host page size was 'pagesize' bytes
> address@hidden -singlestep
> +Run the emulation in single step mode.
>  @end table
>  
>  @node compilation
> Index: trunk/qemu-options.hx
> ===================================================================
> --- trunk.orig/qemu-options.hx        2009-03-30 11:29:03.000000000 +0200
> +++ trunk/qemu-options.hx     2009-03-30 11:30:43.000000000 +0200
> @@ -1209,6 +1209,13 @@
>  from a script.
>  ETEXI
>  
> +DEF("singlestep", 0, QEMU_OPTION_singlestep, \
> +    "-singlestep   always run in singlestep mode\n")
> +STEXI
> address@hidden -singlestep
> +Run the emulation in single step mode.
> +ETEXI
> +
>  DEF("S", 0, QEMU_OPTION_S, \
>      "-S              freeze CPU at startup (use 'c' to start execution)\n")
>  STEXI
> Index: trunk/target-alpha/translate.c
> ===================================================================
> --- trunk.orig/target-alpha/translate.c       2009-03-30 11:29:04.000000000 
> +0200
> +++ trunk/target-alpha/translate.c    2009-03-30 11:59:18.000000000 +0200
> @@ -2412,11 +2412,11 @@
>          if (env->singlestep_enabled) {
>              gen_excp(&ctx, EXCP_DEBUG, 0);
>              break;
> -     }
> +        }
>  
> -#if defined (DO_SINGLE_STEP)
> -        break;
> -#endif
> +        if (singlestep) {
> +            break;
> +        }
>      }
>      if (ret != 1 && ret != 3) {
>          tcg_gen_movi_i64(cpu_pc, ctx.pc);
> Index: trunk/target-arm/translate.c
> ===================================================================
> --- trunk.orig/target-arm/translate.c 2009-03-30 11:29:04.000000000 +0200
> +++ trunk/target-arm/translate.c      2009-03-30 11:53:46.000000000 +0200
> @@ -8791,6 +8791,7 @@
>          num_insns ++;
>      } while (!dc->is_jmp && gen_opc_ptr < gen_opc_end &&
>               !env->singlestep_enabled &&
> +             !singlestep &&
>               dc->pc < next_page_start &&
>               num_insns < max_insns);
>  
> Index: trunk/target-cris/translate.c
> ===================================================================
> --- trunk.orig/target-cris/translate.c        2009-03-30 11:29:04.000000000 
> +0200
> +++ trunk/target-cris/translate.c     2009-03-30 11:52:58.000000000 +0200
> @@ -3272,6 +3272,7 @@
>                       break;
>       } while (!dc->is_jmp && !dc->cpustate_changed
>                && gen_opc_ptr < gen_opc_end
> +                 && !singlestep
>                && (dc->pc < next_page_start)
>                   && num_insns < max_insns);
>  
> Index: trunk/target-i386/translate.c
> ===================================================================
> --- trunk.orig/target-i386/translate.c        2009-03-30 11:29:04.000000000 
> +0200
> +++ trunk/target-i386/translate.c     2009-03-30 11:30:43.000000000 +0200
> @@ -7651,6 +7651,11 @@
>              gen_eob(dc);
>              break;
>          }
> +        if (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-m68k/translate.c
> ===================================================================
> --- trunk.orig/target-m68k/translate.c        2009-03-30 11:29:03.000000000 
> +0200
> +++ trunk/target-m68k/translate.c     2009-03-30 11:54:26.000000000 +0200
> @@ -3031,6 +3031,7 @@
>          num_insns++;
>      } while (!dc->is_jmp && gen_opc_ptr < gen_opc_end &&
>               !env->singlestep_enabled &&
> +             !singlestep &&
>               (pc_offset) < (TARGET_PAGE_SIZE - 32) &&
>               num_insns < max_insns);
>  
> Index: trunk/target-mips/translate.c
> ===================================================================
> --- trunk.orig/target-mips/translate.c        2009-03-30 11:29:04.000000000 
> +0200
> +++ trunk/target-mips/translate.c     2009-03-30 11:30:42.000000000 +0200
> @@ -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))
> @@ -8140,9 +8139,9 @@
>  
>          if (num_insns >= max_insns)
>              break;
> -#if defined (MIPS_SINGLE_STEP)
> -        break;
> -#endif
> +
> +        if (singlestep)
> +            break;
>      }
>      if (tb->cflags & CF_LAST_IO)
>          gen_io_end();
> Index: trunk/target-ppc/translate.c
> ===================================================================
> --- trunk.orig/target-ppc/translate.c 2009-03-30 11:29:04.000000000 +0200
> +++ trunk/target-ppc/translate.c      2009-03-30 11:57:29.000000000 +0200
> @@ -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
>  
> @@ -8288,15 +8287,13 @@
>              gen_exception(ctxp, POWERPC_EXCP_TRACE);
>          } else if (unlikely(((ctx.nip & (TARGET_PAGE_SIZE - 1)) == 0) ||
>                              (env->singlestep_enabled) ||
> +                            singlestep ||
>                              num_insns >= max_insns)) {
>              /* if we reach a page boundary or are single stepping, stop
>               * generation
>               */
>              break;
>          }
> -#if defined (DO_SINGLE_STEP)
> -        break;
> -#endif
>      }
>      if (tb->cflags & CF_LAST_IO)
>          gen_io_end();
> Index: trunk/target-sh4/translate.c
> ===================================================================
> --- trunk.orig/target-sh4/translate.c 2009-03-30 11:29:03.000000000 +0200
> +++ trunk/target-sh4/translate.c      2009-03-30 11:30:42.000000000 +0200
> @@ -1929,9 +1929,8 @@
>           break;
>          if (num_insns >= max_insns)
>              break;
> -#ifdef SH4_SINGLE_STEP
> -     break;
> -#endif
> +        if (singlestep)
> +            break;
>      }
>      if (tb->cflags & CF_LAST_IO)
>          gen_io_end();
> Index: trunk/target-sparc/translate.c
> ===================================================================
> --- trunk.orig/target-sparc/translate.c       2009-03-30 11:29:04.000000000 
> +0200
> +++ trunk/target-sparc/translate.c    2009-03-30 11:30:43.000000000 +0200
> @@ -4838,7 +4838,7 @@
>              break;
>          /* if single step mode, we generate only one instruction and
>             generate an exception */
> -        if (env->singlestep_enabled) {
> +        if (env->singlestep_enabled || singlestep) {
>              tcg_gen_movi_tl(cpu_pc, dc->pc);
>              tcg_gen_exit_tb(0);
>              break;
> Index: trunk/vl.c
> ===================================================================
> --- trunk.orig/vl.c   2009-03-30 11:29:04.000000000 +0200
> +++ trunk/vl.c        2009-03-30 11:30:42.000000000 +0200
> @@ -212,6 +212,7 @@
>  int nb_nics;
>  NICInfo nd_table[MAX_NICS];
>  int vm_running;
> +int singlestep;
>  static int autostart;
>  static int rtc_utc = 1;
>  static int rtc_date_offset = -1; /* -1 means no change */
> @@ -4669,6 +4670,9 @@
>              case QEMU_OPTION_bios:
>                  bios_name = optarg;
>                  break;
> +            case QEMU_OPTION_singlestep:
> +                singlestep = 1;
> +                break;
>              case QEMU_OPTION_S:
>                  autostart = 0;
>                  break;


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