Re: [Qemu-devel] [PATCH 1/5] gdbstub: replace singlestep q packets with qRcmd packets

[Jason Wessel <jason.wessel@windriver.com>]

[-- Attachment #1: Type: text/plain, Size: 1859 bytes --]

Edgar E. Iglesias wrote:
> On Thu, May 15, 2008 at 09:11:29AM -0500, Jason Wessel wrote:
>   
>> +    buf = malloc(len * 2 + 2);
>> +    buf[0] = 'O';
>> +    memtohex(buf + 1, msg, len);
>> +    put_packet(s, buf);
>> +    free(buf);
>> +}
>>     
>
>
> It feels odd that the code path that ends up here has line_buf, buf
and mem_buf available for parsing the query and generating the response
and still we need to malloc for more. Isn't there a way to reuse some of
that memory?
>   


Given that put_packet is what really needed the memory and already had
its own private global, I modified put_packet to accept a length and
contain the memtohex invocation.

>
>   
>> +
>> +static void monitor_help(GDBState *s)
>> +{
>> +    monitor_output(s, "gdbstub specific monitor commands:\n");
>> +    monitor_output(s, "s_show -- Show gdbstub Single Stepping
variables\n");
>> +    monitor_output(s, "set s_step <0|1> -- Single Stepping enabled\n");
>> +    monitor_output(s, "set s_irq <0|1> --  Single Stepping with qemu
irq handlers enabled\n");
>> +    monitor_output(s, "set s_timer <0|1> -- Single Stepping with
qemu timers enabled\n");
>> +}
>>     
>
> I'd prefer if either have a show/set interface or we don't, this is
kind of a mix.
>
> Some suggestions:
> monitor sstep enable/disable
> monitor sstep irq/noirq
> monitor sstep timers/notimers
> monitor sstep show
>
> or:
> monitor set/show sstep
> monitor set/show sirq
> monitor set/show stimers
>
> What do you think?
>   

I explicitly did not use "monitor set/show" because I figured these
commands would have been used by the qemu monitor, even though they
are presently not used today to my surprise.  After you asked the
question it occurred to me that the "else if" block we have now will
take care of the problem, so long as unique variables are used.

Attached is the new version.

Jason.


[-- Attachment #2: gdb_single_step_monitor_cmd.patch --]
[-- Type: text/x-diff, Size: 8148 bytes --]

From: Jason Wessel <jason.wessel@windriver.com>
Subject: [PATCH] gdbstub: replace singlestep q packets with qRcmd packets

At the gdbserial protocol level, using the qRcmd packets allows gdb to
use the "monitor" command to access the controls for single stepping
behavior.  Now you can use a gdb "monitor" command instead of a gdb
"maintenance" command.

The qemu docs were updated to reflect this change.

Signed-off-by: Jason Wessel <jason.wessel@windriver.com>

---
 gdbstub.c     |  100 +++++++++++++++++++++++++++++++++++++++++-----------------
 qemu-doc.texi |   36 ++++++++++----------
 2 files changed, 89 insertions(+), 47 deletions(-)

--- a/gdbstub.c
+++ b/gdbstub.c
@@ -205,9 +205,9 @@ static void hextomem(uint8_t *mem, const
 }
 
 /* return -1 if error, 0 if OK */
-static int put_packet(GDBState *s, char *buf)
+static int put_packet_hex(GDBState *s, const char *buf, int len, int isHex)
 {
-    int len, csum, i;
+    int csum, i;
     uint8_t *p;
 
 #ifdef DEBUG_GDB
@@ -217,13 +217,19 @@ static int put_packet(GDBState *s, char 
     for(;;) {
         p = s->last_packet;
         *(p++) = '$';
-        len = strlen(buf);
-        memcpy(p, buf, len);
-        p += len;
+        if (isHex) {
+            p[0] = 'O';
+            memtohex(p + 1, buf, len);
+            len = strlen(p);
+        } else {
+            memcpy(p, buf, len);
+        }
+
         csum = 0;
         for(i = 0; i < len; i++) {
-            csum += buf[i];
+            csum += p[i];
         }
+        p += len;
         *(p++) = '#';
         *(p++) = tohex((csum >> 4) & 0xf);
         *(p++) = tohex((csum) & 0xf);
@@ -244,6 +250,25 @@ static int put_packet(GDBState *s, char 
     return 0;
 }
 
+/* return -1 if error, 0 if OK */
+static int put_packet(GDBState *s, const char *buf) {
+    return put_packet_hex(s, buf, strlen(buf), 0);
+}
+
+static void monitor_output(GDBState *s, const char *msg)
+{
+    put_packet_hex(s, msg, strlen(msg), 1);
+}
+
+static void monitor_help(GDBState *s)
+{
+    monitor_output(s, "gdbstub specific monitor commands:\n");
+    monitor_output(s, "show <sstep|sirq|stimer> -- Show gdbstub Single Stepping variables\n");
+    monitor_output(s, "set sstep <0|1> -- Single Stepping enabled\n");
+    monitor_output(s, "set sirq <0|1> --  Single Stepping with qemu irq handlers enabled\n");
+    monitor_output(s, "set stimers <0|1> -- Single Stepping with qemu timers enabled\n");
+}
+
 #if defined(TARGET_I386)
 
 #ifdef TARGET_X86_64
@@ -948,6 +973,44 @@ static void cpu_gdb_write_registers(CPUS
 
 #endif
 
+static void gdb_rcmd(GDBState *s, const char *p, char *buf, char *mem_buf)
+{
+    int len = strlen(p);
+
+    if ((len % 2) != 0) {
+        put_packet(s, "E01");
+        return;
+    }
+    hextomem(mem_buf, p, len);
+    mem_buf[(len >> 1)] = 0;
+
+    if (!strcmp(mem_buf, "show sstep")) {
+        sprintf(buf, "sstep == %i\n", (sstep_flags & SSTEP_ENABLE) != 0);
+        monitor_output(s, buf);
+    } else if (!strcmp(mem_buf, "show sirq")) {
+        sprintf(buf, "sirq == %i\n", (sstep_flags & SSTEP_NOIRQ) == 0);
+        monitor_output(s, buf);
+    } else if (!strcmp(mem_buf, "show stimers")) {
+        sprintf(buf, "stimers == %i\n", (sstep_flags & SSTEP_NOTIMER) == 0);
+        monitor_output(s, buf);
+    } else if (!strcmp(mem_buf, "set sstep 1")) {
+        sstep_flags |= SSTEP_ENABLE;
+    } else if (!strcmp(mem_buf, "set step 0")) {
+        sstep_flags &= ~SSTEP_ENABLE;
+    } else if (!strcmp(mem_buf, "set sirq 1")) {
+        sstep_flags &= ~SSTEP_NOIRQ;
+    } else if (!strcmp(mem_buf, "set sirq 0")) {
+        sstep_flags |= SSTEP_NOIRQ;
+    } else if (!strcmp(mem_buf, "set stimers 1")) {
+        sstep_flags &= ~SSTEP_NOTIMER;
+    } else if (!strcmp(mem_buf, "set stimers 0")) {
+        sstep_flags |= SSTEP_NOTIMER;
+    } else if (!strcmp(mem_buf, "help") || !strcmp(mem_buf, "?")) {
+        monitor_help(s);
+    }
+    put_packet(s, "OK");
+}
+
 static int gdb_handle_packet(GDBState *s, CPUState *env, const char *line_buf)
 {
     const char *p;
@@ -1139,29 +1202,8 @@ static int gdb_handle_packet(GDBState *s
         }
         break;
     case 'q':
-    case 'Q':
-        /* parse any 'q' packets here */
-        if (!strcmp(p,"qemu.sstepbits")) {
-            /* Query Breakpoint bit definitions */
-            sprintf(buf,"ENABLE=%x,NOIRQ=%x,NOTIMER=%x",
-                    SSTEP_ENABLE,
-                    SSTEP_NOIRQ,
-                    SSTEP_NOTIMER);
-            put_packet(s, buf);
-            break;
-        } else if (strncmp(p,"qemu.sstep",10) == 0) {
-            /* Display or change the sstep_flags */
-            p += 10;
-            if (*p != '=') {
-                /* Display current setting */
-                sprintf(buf,"0x%x", sstep_flags);
-                put_packet(s, buf);
-                break;
-            }
-            p++;
-            type = strtoul(p, (char **)&p, 16);
-            sstep_flags = type;
-            put_packet(s, "OK");
+        if (!strncmp(p, "Rcmd,", 5)) {
+            gdb_rcmd(s, p + 5, buf, mem_buf);
             break;
         }
 #ifdef CONFIG_LINUX_USER
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -1951,32 +1951,32 @@ Use @code{set architecture i8086} to dum
 
 Advanced debugging options:
 
-The default single stepping behavior is step with the IRQs and timer service routines off.  It is set this way because when gdb executes a single step it expects to advance beyond the current instruction.  With the IRQs and and timer service routines on, a single step might jump into the one of the interrupt or exception vectors instead of executing the current instruction. This means you may hit the same breakpoint a number of times before executing the instruction gdb wants to have executed.  Because there are rare circumstances where you want to single step into an interrupt vector the behavior can be controlled from GDB.  There are three commands you can query and set the single step behavior:
+The default single stepping behavior is to step with the IRQs and timer service routines off.  It is set this way because when gdb executes a single step it expects to advance beyond the current instruction.  With the IRQs and and timer service routines on, a single step might jump into the one of the interrupt or exception vectors instead of executing the current instruction. This means you may hit the same breakpoint a number of times before executing the instruction gdb wants execute.  Because there are rare circumstances where you want to single step into an interrupt vector the behavior can be controlled from GDB.  There are several commands you use to query and set the single step behavior while inside gdb:
 @table @code
-@item maintenance packet qqemu.sstepbits
+@item monitor show <sstep|sirq|stimers>
 
-This will display the MASK bits used to control the single stepping IE:
+This will display the values of the single stepping controls IE:
 @example
-(gdb) maintenance packet qqemu.sstepbits
-sending: "qqemu.sstepbits"
-received: "ENABLE=1,NOIRQ=2,NOTIMER=4"
+(gdb) monitor show sstep
+sstep == 1
+(gdb) monitor show sirq
+sirq == 0
+(gdb) monitor show stimers
+stimers == 0
 @end example
-@item maintenance packet qqemu.sstep
+@item monitor set sstep <0|1>
 
-This will display the current value of the mask used when single stepping IE:
+Turn off(0) or on(1) the single stepping feature all together, which is defaulted to on.
 @example
-(gdb) maintenance packet qqemu.sstep
-sending: "qqemu.sstep"
-received: "0x7"
+(gdb) monitor set sstep 0
+(gdb) monitor set sstep 1
 @end example
-@item maintenance packet Qqemu.sstep=HEX_VALUE
+@item monitor set sirq <0|1>
 
-This will change the single step mask, so if wanted to enable IRQs on the single step, but not timers, you would use:
-@example
-(gdb) maintenance packet Qqemu.sstep=0x5
-sending: "qemu.sstep=0x5"
-received: "OK"
-@end example
+Turn off or on the the irq processing when single stepping, which is defaulted to off.
+@item monitor set stimers <0|1>
+
+Turn off or on the the timer processing when single stepping, which is defaulted to off.
 @end table
 
 @node pcsys_os_specific