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

[Edgar E. Iglesias]

On Mon, May 19, 2008 at 08:27:42AM -0500, Jason Wessel wrote:
> 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.
>
> > 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.


Thanks for fixing it up, I think this version looks good.

Best regards





> From: Jason Wessel <address@hidden>
> 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 <address@hidden>
> 
> ---
>  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
> address@hidden maintenance packet qqemu.sstepbits
> address@hidden 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
> address@hidden maintenance packet qqemu.sstep
> address@hidden 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
> address@hidden maintenance packet Qqemu.sstep=HEX_VALUE
> address@hidden 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:
> address@hidden
> -(gdb) maintenance packet Qqemu.sstep=0x5
> -sending: "qemu.sstep=0x5"
> -received: "OK"
> address@hidden example
> +Turn off or on the the irq processing when single stepping, which is 
> defaulted to off.
> address@hidden 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


-- 
Edgar E. Iglesias
Axis Communications AB