From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1LoEZs-0003b1-Td for qemu-devel@nongnu.org; Mon, 30 Mar 2009 06:18:48 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1LoEZo-0003Z7-Js for qemu-devel@nongnu.org; Mon, 30 Mar 2009 06:18:48 -0400 Received: from [199.232.76.173] (port=40500 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1LoEZo-0003Yt-BM for qemu-devel@nongnu.org; Mon, 30 Mar 2009 06:18:44 -0400 Received: from moutng.kundenserver.de ([212.227.126.177]:56032) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1LoEZn-0000mB-Lf for qemu-devel@nongnu.org; Mon, 30 Mar 2009 06:18:44 -0400 Message-ID: <49D09C74.2030307@mail.berlios.de> Date: Mon, 30 Mar 2009 12:18:28 +0200 From: Stefan Weil MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] Add single stepping option for all targets References: <494D18B1.8080900@mail.berlios.de> <4985FD31.4010000@mail.berlios.de> <20090303063841.GG5159@volta.aurel32.net> <49BA8B39.8050804@mail.berlios.de> <20090328221241.GB14328@volta.aurel32.net> In-Reply-To: <20090328221241.GB14328@volta.aurel32.net> Content-Type: multipart/mixed; boundary="------------050506080004080501020509" Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Aurelien Jarno Cc: QEMU Developers This is a multi-part message in MIME format. --------------050506080004080501020509 Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit 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. > > >> ... >> 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 --------------050506080004080501020509 Content-Type: text/x-diff; name="singlestep.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="singlestep.patch" 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 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 #include +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}. +@item 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 +@item -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 +@item -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 +@item -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 +@item -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; --------------050506080004080501020509--