[Qemu-devel] [PATCH v1 0/2] some gdbstub fixes for debug and vcont

[Qemu-devel] [PATCH v1 0/2] some gdbstub fixes for debug and vcont

[Alex Bennée]

Hi,

I was attempting to debug RISU via qemu-aarch64 and the gdbstub and I
was finding the stub responding with an E22 on vcont. The fix is in
the second patch. The first patch just modernises the debug output to
use a helper instead of lots of #ifdefs.

Alex Bennée (2):
  gdbstub: modernise DEBUG_GDB
  gdbstub: don't fail on vCont;C04:0;c packets

 gdbstub.c | 81 +++++++++++++++++++++++++++------------------------------------
 1 file changed, 35 insertions(+), 46 deletions(-)

-- 
2.13.0

[Qemu-devel] [PATCH v1 1/2] gdbstub: modernise DEBUG_GDB

[Alex Bennée]

Convert the a gdb_debug helper which compiles away to nothing when not
used but still ensures the format strings are checked. There is some
minor code motion for the incorrect checksum message to report it
before we attempt to send the reply.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 gdbstub.c | 77 +++++++++++++++++++++++++++------------------------------------
 1 file changed, 33 insertions(+), 44 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 86eed4f97c..a249846954 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -271,7 +271,20 @@ static int gdb_signal_to_target (int sig)
         return -1;
 }
 
-//#define DEBUG_GDB
+/* #define DEBUG_GDB */
+
+#ifdef DEBUG_GDB
+# define DEBUG_GDB_GATE 1
+#else
+# define DEBUG_GDB_GATE 0
+#endif
+
+#define gdb_debug(fmt, ...) do { \
+    if (DEBUG_GDB_GATE) { \
+        fprintf(stderr, "%s: " fmt, __func__, ## __VA_ARGS__); \
+    } \
+} while (0)
+
 
 typedef struct GDBRegisterState {
     int base_reg;
@@ -547,9 +560,7 @@ static int put_packet_binary(GDBState *s, const char *buf, int len)
 /* return -1 if error, 0 if OK */
 static int put_packet(GDBState *s, const char *buf)
 {
-#ifdef DEBUG_GDB
-    printf("reply='%s'\n", buf);
-#endif
+    gdb_debug("reply='%s'\n", buf);
 
     return put_packet_binary(s, buf, strlen(buf));
 }
@@ -955,9 +966,9 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
     uint8_t *registers;
     target_ulong addr, len;
 
-#ifdef DEBUG_GDB
-    printf("command='%s'\n", line_buf);
-#endif
+
+    gdb_debug("command='%s'\n", line_buf);
+
     p = line_buf;
     ch = *p++;
     switch(ch) {
@@ -1518,17 +1529,14 @@ static void gdb_read_byte(GDBState *s, int ch)
         /* Waiting for a response to the last packet.  If we see the start
            of a new command then abandon the previous response.  */
         if (ch == '-') {
-#ifdef DEBUG_GDB
-            printf("Got NACK, retransmitting\n");
-#endif
+            gdb_debug("Got NACK, retransmitting\n");
             put_buffer(s, (uint8_t *)s->last_packet, s->last_packet_len);
+        } else if (ch == '+') {
+            gdb_debug("Got ACK\n");
+        } else {
+            gdb_debug("Got '%c' when expecting ACK/NACK\n", ch);
         }
-#ifdef DEBUG_GDB
-        else if (ch == '+')
-            printf("Got ACK\n");
-        else
-            printf("Got '%c' when expecting ACK/NACK\n", ch);
-#endif
+
         if (ch == '+' || ch == '$')
             s->last_packet_len = 0;
         if (ch != '$')
@@ -1549,9 +1557,7 @@ static void gdb_read_byte(GDBState *s, int ch)
                 s->line_sum = 0;
                 s->state = RS_GETLINE;
             } else {
-#ifdef DEBUG_GDB
-                printf("gdbstub received garbage between packets: 0x%x\n", ch);
-#endif
+                gdb_debug("received garbage between packets: 0x%x\n", ch);
             }
             break;
         case RS_GETLINE:
@@ -1567,9 +1573,7 @@ static void gdb_read_byte(GDBState *s, int ch)
                 /* end of command, start of checksum*/
                 s->state = RS_CHKSUM1;
             } else if (s->line_buf_index >= sizeof(s->line_buf) - 1) {
-#ifdef DEBUG_GDB
-                printf("gdbstub command buffer overrun, dropping command\n");
-#endif
+                gdb_debug("command buffer overrun, dropping command\n");
                 s->state = RS_IDLE;
             } else {
                 /* unescaped command character */
@@ -1583,9 +1587,7 @@ static void gdb_read_byte(GDBState *s, int ch)
                 s->state = RS_CHKSUM1;
             } else if (s->line_buf_index >= sizeof(s->line_buf) - 1) {
                 /* command buffer overrun */
-#ifdef DEBUG_GDB
-                printf("gdbstub command buffer overrun, dropping command\n");
-#endif
+                gdb_debug("command buffer overrun, dropping command\n");
                 s->state = RS_IDLE;
             } else {
                 /* parse escaped character and leave escape state */
@@ -1597,25 +1599,18 @@ static void gdb_read_byte(GDBState *s, int ch)
         case RS_GETLINE_RLE:
             if (ch < ' ') {
                 /* invalid RLE count encoding */
-#ifdef DEBUG_GDB
-                printf("gdbstub got invalid RLE count: 0x%x\n", ch);
-#endif
+                gdb_debug("got invalid RLE count: 0x%x\n", ch);
                 s->state = RS_GETLINE;
             } else {
                 /* decode repeat length */
                 int repeat = (unsigned char)ch - ' ' + 3;
                 if (s->line_buf_index + repeat >= sizeof(s->line_buf) - 1) {
                     /* that many repeats would overrun the command buffer */
-#ifdef DEBUG_GDB
-                    printf("gdbstub command buffer overrun,"
-                           " dropping command\n");
-#endif
+                    gdb_debug("command buffer overrun, dropping command\n");
                     s->state = RS_IDLE;
                 } else if (s->line_buf_index < 1) {
                     /* got a repeat but we have nothing to repeat */
-#ifdef DEBUG_GDB
-                    printf("gdbstub got invalid RLE sequence\n");
-#endif
+                    gdb_debug("got invalid RLE sequence\n");
                     s->state = RS_GETLINE;
                 } else {
                     /* repeat the last character */
@@ -1630,9 +1625,7 @@ static void gdb_read_byte(GDBState *s, int ch)
         case RS_CHKSUM1:
             /* get high hex digit of checksum */
             if (!isxdigit(ch)) {
-#ifdef DEBUG_GDB
-                printf("gdbstub got invalid command checksum digit\n");
-#endif
+                gdb_debug("got invalid command checksum digit\n");
                 s->state = RS_GETLINE;
                 break;
             }
@@ -1643,21 +1636,17 @@ static void gdb_read_byte(GDBState *s, int ch)
         case RS_CHKSUM2:
             /* get low hex digit of checksum */
             if (!isxdigit(ch)) {
-#ifdef DEBUG_GDB
-                printf("gdbstub got invalid command checksum digit\n");
-#endif
+                gdb_debug("got invalid command checksum digit\n");
                 s->state = RS_GETLINE;
                 break;
             }
             s->line_csum |= fromhex(ch);
 
             if (s->line_csum != (s->line_sum & 0xff)) {
+                gdb_debug("got command packet with incorrect checksum\n");
                 /* send NAK reply */
                 reply = '-';
                 put_buffer(s, &reply, 1);
-#ifdef DEBUG_GDB
-                printf("gdbstub got command packet with incorrect checksum\n");
-#endif
                 s->state = RS_IDLE;
             } else {
                 /* send ACK reply */
-- 
2.13.0

Re: [Qemu-devel] [PATCH v1 1/2] gdbstub: modernise DEBUG_GDB

[Philippe Mathieu-Daudé]

On 05/31/2017 12:09 PM, Alex Bennée wrote:
> Convert the a gdb_debug helper which compiles away to nothing when not
> used but still ensures the format strings are checked. There is some
> minor code motion for the incorrect checksum message to report it
> before we attempt to send the reply.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  gdbstub.c | 77 +++++++++++++++++++++++++++------------------------------------
>  1 file changed, 33 insertions(+), 44 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 86eed4f97c..a249846954 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -271,7 +271,20 @@ static int gdb_signal_to_target (int sig)
>          return -1;
>  }
>
> -//#define DEBUG_GDB
> +/* #define DEBUG_GDB */
> +
> +#ifdef DEBUG_GDB
> +# define DEBUG_GDB_GATE 1
> +#else
> +# define DEBUG_GDB_GATE 0
> +#endif
> +
> +#define gdb_debug(fmt, ...) do { \
> +    if (DEBUG_GDB_GATE) { \
> +        fprintf(stderr, "%s: " fmt, __func__, ## __VA_ARGS__); \
> +    } \
> +} while (0)
> +
>
>  typedef struct GDBRegisterState {
>      int base_reg;
> @@ -547,9 +560,7 @@ static int put_packet_binary(GDBState *s, const char *buf, int len)
>  /* return -1 if error, 0 if OK */
>  static int put_packet(GDBState *s, const char *buf)
>  {
> -#ifdef DEBUG_GDB
> -    printf("reply='%s'\n", buf);
> -#endif
> +    gdb_debug("reply='%s'\n", buf);
>
>      return put_packet_binary(s, buf, strlen(buf));
>  }
> @@ -955,9 +966,9 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>      uint8_t *registers;
>      target_ulong addr, len;
>
> -#ifdef DEBUG_GDB
> -    printf("command='%s'\n", line_buf);
> -#endif
> +
> +    gdb_debug("command='%s'\n", line_buf);
> +
>      p = line_buf;
>      ch = *p++;
>      switch(ch) {
> @@ -1518,17 +1529,14 @@ static void gdb_read_byte(GDBState *s, int ch)
>          /* Waiting for a response to the last packet.  If we see the start
>             of a new command then abandon the previous response.  */
>          if (ch == '-') {
> -#ifdef DEBUG_GDB
> -            printf("Got NACK, retransmitting\n");
> -#endif
> +            gdb_debug("Got NACK, retransmitting\n");
>              put_buffer(s, (uint8_t *)s->last_packet, s->last_packet_len);
> +        } else if (ch == '+') {
> +            gdb_debug("Got ACK\n");
> +        } else {
> +            gdb_debug("Got '%c' when expecting ACK/NACK\n", ch);
>          }
> -#ifdef DEBUG_GDB
> -        else if (ch == '+')
> -            printf("Got ACK\n");
> -        else
> -            printf("Got '%c' when expecting ACK/NACK\n", ch);
> -#endif
> +
>          if (ch == '+' || ch == '$')
>              s->last_packet_len = 0;
>          if (ch != '$')
> @@ -1549,9 +1557,7 @@ static void gdb_read_byte(GDBState *s, int ch)
>                  s->line_sum = 0;
>                  s->state = RS_GETLINE;
>              } else {
> -#ifdef DEBUG_GDB
> -                printf("gdbstub received garbage between packets: 0x%x\n", ch);
> -#endif
> +                gdb_debug("received garbage between packets: 0x%x\n", ch);
>              }
>              break;
>          case RS_GETLINE:
> @@ -1567,9 +1573,7 @@ static void gdb_read_byte(GDBState *s, int ch)
>                  /* end of command, start of checksum*/
>                  s->state = RS_CHKSUM1;
>              } else if (s->line_buf_index >= sizeof(s->line_buf) - 1) {
> -#ifdef DEBUG_GDB
> -                printf("gdbstub command buffer overrun, dropping command\n");
> -#endif
> +                gdb_debug("command buffer overrun, dropping command\n");
>                  s->state = RS_IDLE;
>              } else {
>                  /* unescaped command character */
> @@ -1583,9 +1587,7 @@ static void gdb_read_byte(GDBState *s, int ch)
>                  s->state = RS_CHKSUM1;
>              } else if (s->line_buf_index >= sizeof(s->line_buf) - 1) {
>                  /* command buffer overrun */
> -#ifdef DEBUG_GDB
> -                printf("gdbstub command buffer overrun, dropping command\n");
> -#endif
> +                gdb_debug("command buffer overrun, dropping command\n");
>                  s->state = RS_IDLE;
>              } else {
>                  /* parse escaped character and leave escape state */
> @@ -1597,25 +1599,18 @@ static void gdb_read_byte(GDBState *s, int ch)
>          case RS_GETLINE_RLE:
>              if (ch < ' ') {
>                  /* invalid RLE count encoding */
> -#ifdef DEBUG_GDB
> -                printf("gdbstub got invalid RLE count: 0x%x\n", ch);
> -#endif
> +                gdb_debug("got invalid RLE count: 0x%x\n", ch);
>                  s->state = RS_GETLINE;
>              } else {
>                  /* decode repeat length */
>                  int repeat = (unsigned char)ch - ' ' + 3;
>                  if (s->line_buf_index + repeat >= sizeof(s->line_buf) - 1) {
>                      /* that many repeats would overrun the command buffer */
> -#ifdef DEBUG_GDB
> -                    printf("gdbstub command buffer overrun,"
> -                           " dropping command\n");
> -#endif
> +                    gdb_debug("command buffer overrun, dropping command\n");
>                      s->state = RS_IDLE;
>                  } else if (s->line_buf_index < 1) {
>                      /* got a repeat but we have nothing to repeat */
> -#ifdef DEBUG_GDB
> -                    printf("gdbstub got invalid RLE sequence\n");
> -#endif
> +                    gdb_debug("got invalid RLE sequence\n");
>                      s->state = RS_GETLINE;
>                  } else {
>                      /* repeat the last character */
> @@ -1630,9 +1625,7 @@ static void gdb_read_byte(GDBState *s, int ch)
>          case RS_CHKSUM1:
>              /* get high hex digit of checksum */
>              if (!isxdigit(ch)) {
> -#ifdef DEBUG_GDB
> -                printf("gdbstub got invalid command checksum digit\n");
> -#endif
> +                gdb_debug("got invalid command checksum digit\n");
>                  s->state = RS_GETLINE;
>                  break;
>              }
> @@ -1643,21 +1636,17 @@ static void gdb_read_byte(GDBState *s, int ch)
>          case RS_CHKSUM2:
>              /* get low hex digit of checksum */
>              if (!isxdigit(ch)) {
> -#ifdef DEBUG_GDB
> -                printf("gdbstub got invalid command checksum digit\n");
> -#endif
> +                gdb_debug("got invalid command checksum digit\n");
>                  s->state = RS_GETLINE;
>                  break;
>              }
>              s->line_csum |= fromhex(ch);
>
>              if (s->line_csum != (s->line_sum & 0xff)) {
> +                gdb_debug("got command packet with incorrect checksum\n");
>                  /* send NAK reply */
>                  reply = '-';
>                  put_buffer(s, &reply, 1);
> -#ifdef DEBUG_GDB
> -                printf("gdbstub got command packet with incorrect checksum\n");
> -#endif
>                  s->state = RS_IDLE;
>              } else {
>                  /* send ACK reply */
>

Re: [Qemu-devel] [PATCH v1 1/2] gdbstub: modernise DEBUG_GDB

[Greg Kurz]

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

On Wed, 31 May 2017 16:09:32 +0100
Alex Bennée <alex.bennee@linaro.org> wrote:

> Convert the a gdb_debug helper which compiles away to nothing when not
> used but still ensures the format strings are checked. There is some
> minor code motion for the incorrect checksum message to report it
> before we attempt to send the reply.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  gdbstub.c | 77 +++++++++++++++++++++++++++------------------------------------
>  1 file changed, 33 insertions(+), 44 deletions(-)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index 86eed4f97c..a249846954 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -271,7 +271,20 @@ static int gdb_signal_to_target (int sig)
>          return -1;
>  }
>  
> -//#define DEBUG_GDB
> +/* #define DEBUG_GDB */
> +
> +#ifdef DEBUG_GDB
> +# define DEBUG_GDB_GATE 1
> +#else
> +# define DEBUG_GDB_GATE 0
> +#endif
> +
> +#define gdb_debug(fmt, ...) do { \
> +    if (DEBUG_GDB_GATE) { \
> +        fprintf(stderr, "%s: " fmt, __func__, ## __VA_ARGS__); \
> +    } \
> +} while (0)
> +
>  
>  typedef struct GDBRegisterState {
>      int base_reg;
> @@ -547,9 +560,7 @@ static int put_packet_binary(GDBState *s, const char *buf, int len)
>  /* return -1 if error, 0 if OK */
>  static int put_packet(GDBState *s, const char *buf)
>  {
> -#ifdef DEBUG_GDB
> -    printf("reply='%s'\n", buf);
> -#endif
> +    gdb_debug("reply='%s'\n", buf);
>  
>      return put_packet_binary(s, buf, strlen(buf));
>  }
> @@ -955,9 +966,9 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>      uint8_t *registers;
>      target_ulong addr, len;
>  
> -#ifdef DEBUG_GDB
> -    printf("command='%s'\n", line_buf);
> -#endif
> +
> +    gdb_debug("command='%s'\n", line_buf);
> +
>      p = line_buf;
>      ch = *p++;
>      switch(ch) {
> @@ -1518,17 +1529,14 @@ static void gdb_read_byte(GDBState *s, int ch)
>          /* Waiting for a response to the last packet.  If we see the start
>             of a new command then abandon the previous response.  */
>          if (ch == '-') {
> -#ifdef DEBUG_GDB
> -            printf("Got NACK, retransmitting\n");
> -#endif
> +            gdb_debug("Got NACK, retransmitting\n");
>              put_buffer(s, (uint8_t *)s->last_packet, s->last_packet_len);
> +        } else if (ch == '+') {
> +            gdb_debug("Got ACK\n");
> +        } else {
> +            gdb_debug("Got '%c' when expecting ACK/NACK\n", ch);
>          }
> -#ifdef DEBUG_GDB
> -        else if (ch == '+')
> -            printf("Got ACK\n");
> -        else
> -            printf("Got '%c' when expecting ACK/NACK\n", ch);
> -#endif
> +
>          if (ch == '+' || ch == '$')
>              s->last_packet_len = 0;
>          if (ch != '$')
> @@ -1549,9 +1557,7 @@ static void gdb_read_byte(GDBState *s, int ch)
>                  s->line_sum = 0;
>                  s->state = RS_GETLINE;
>              } else {
> -#ifdef DEBUG_GDB
> -                printf("gdbstub received garbage between packets: 0x%x\n", ch);
> -#endif
> +                gdb_debug("received garbage between packets: 0x%x\n", ch);
>              }
>              break;
>          case RS_GETLINE:
> @@ -1567,9 +1573,7 @@ static void gdb_read_byte(GDBState *s, int ch)
>                  /* end of command, start of checksum*/
>                  s->state = RS_CHKSUM1;
>              } else if (s->line_buf_index >= sizeof(s->line_buf) - 1) {
> -#ifdef DEBUG_GDB
> -                printf("gdbstub command buffer overrun, dropping command\n");
> -#endif
> +                gdb_debug("command buffer overrun, dropping command\n");
>                  s->state = RS_IDLE;
>              } else {
>                  /* unescaped command character */
> @@ -1583,9 +1587,7 @@ static void gdb_read_byte(GDBState *s, int ch)
>                  s->state = RS_CHKSUM1;
>              } else if (s->line_buf_index >= sizeof(s->line_buf) - 1) {
>                  /* command buffer overrun */
> -#ifdef DEBUG_GDB
> -                printf("gdbstub command buffer overrun, dropping command\n");
> -#endif
> +                gdb_debug("command buffer overrun, dropping command\n");
>                  s->state = RS_IDLE;
>              } else {
>                  /* parse escaped character and leave escape state */
> @@ -1597,25 +1599,18 @@ static void gdb_read_byte(GDBState *s, int ch)
>          case RS_GETLINE_RLE:
>              if (ch < ' ') {
>                  /* invalid RLE count encoding */
> -#ifdef DEBUG_GDB
> -                printf("gdbstub got invalid RLE count: 0x%x\n", ch);
> -#endif
> +                gdb_debug("got invalid RLE count: 0x%x\n", ch);
>                  s->state = RS_GETLINE;
>              } else {
>                  /* decode repeat length */
>                  int repeat = (unsigned char)ch - ' ' + 3;
>                  if (s->line_buf_index + repeat >= sizeof(s->line_buf) - 1) {
>                      /* that many repeats would overrun the command buffer */
> -#ifdef DEBUG_GDB
> -                    printf("gdbstub command buffer overrun,"
> -                           " dropping command\n");
> -#endif
> +                    gdb_debug("command buffer overrun, dropping command\n");
>                      s->state = RS_IDLE;
>                  } else if (s->line_buf_index < 1) {
>                      /* got a repeat but we have nothing to repeat */
> -#ifdef DEBUG_GDB
> -                    printf("gdbstub got invalid RLE sequence\n");
> -#endif
> +                    gdb_debug("got invalid RLE sequence\n");
>                      s->state = RS_GETLINE;
>                  } else {
>                      /* repeat the last character */
> @@ -1630,9 +1625,7 @@ static void gdb_read_byte(GDBState *s, int ch)
>          case RS_CHKSUM1:
>              /* get high hex digit of checksum */
>              if (!isxdigit(ch)) {
> -#ifdef DEBUG_GDB
> -                printf("gdbstub got invalid command checksum digit\n");
> -#endif
> +                gdb_debug("got invalid command checksum digit\n");
>                  s->state = RS_GETLINE;
>                  break;
>              }
> @@ -1643,21 +1636,17 @@ static void gdb_read_byte(GDBState *s, int ch)
>          case RS_CHKSUM2:
>              /* get low hex digit of checksum */
>              if (!isxdigit(ch)) {
> -#ifdef DEBUG_GDB
> -                printf("gdbstub got invalid command checksum digit\n");
> -#endif
> +                gdb_debug("got invalid command checksum digit\n");
>                  s->state = RS_GETLINE;
>                  break;
>              }
>              s->line_csum |= fromhex(ch);
>  
>              if (s->line_csum != (s->line_sum & 0xff)) {
> +                gdb_debug("got command packet with incorrect checksum\n");
>                  /* send NAK reply */
>                  reply = '-';
>                  put_buffer(s, &reply, 1);
> -#ifdef DEBUG_GDB
> -                printf("gdbstub got command packet with incorrect checksum\n");
> -#endif
>                  s->state = RS_IDLE;
>              } else {
>                  /* send ACK reply */


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[Qemu-devel] [PATCH v1 2/2] gdbstub: don't fail on vCont; C04:0; c packets

[Alex Bennée]

The thread-id of 0 means any CPU but we then ignore the fact we find
the first_cpu in this case who can have an index of 0. Instead of
bailing out just test if we have managed to match up thread-id to a
CPU.

Otherwise you get:
  gdb_handle_packet: command='vCont;C04:0;c'
  put_packet: reply='E22'

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 gdbstub.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index a249846954..29c9ed3002 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -934,8 +934,8 @@ static int gdb_handle_vcont(GDBState *s, const char *p)
              * CPU first, and only then we can use its index.
              */
             cpu = find_cpu(idx);
-            /* invalid CPU/thread specified */
-            if (!idx || !cpu) {
+            /* invalid thread specified, cpu not found. */
+            if (!cpu) {
                 res = -EINVAL;
                 goto out;
             }
-- 
2.13.0

Re: [Qemu-devel] [PATCH v1 2/2] gdbstub: don't fail on vCont; C04:0; c packets

[Greg Kurz]

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

On Wed, 31 May 2017 16:09:33 +0100
Alex Bennée <alex.bennee@linaro.org> wrote:

> The thread-id of 0 means any CPU but we then ignore the fact we find
> the first_cpu in this case who can have an index of 0. Instead of

The index can never be 0 in system mode actually, but you're right that this
check doesn't make sense.

The code still looks a bit convoluted IMHO. What about something like the
following ?

            /* 0 means any thread, so we pick the first valid CPU */
            cpu = tmp ? find_cpu(tmp) : first_cpu;

            /* invalid CPU/thread specified */
            if (!cpu) {
                res = -EINVAL;
                goto out;
            }

Anyway, the fix looks ok.

Reviewed-by: Greg Kurz <groug@kaod.org>

> bailing out just test if we have managed to match up thread-id to a
> CPU.
> 
> Otherwise you get:
>   gdb_handle_packet: command='vCont;C04:0;c'
>   put_packet: reply='E22'
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  gdbstub.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index a249846954..29c9ed3002 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -934,8 +934,8 @@ static int gdb_handle_vcont(GDBState *s, const char *p)
>               * CPU first, and only then we can use its index.
>               */
>              cpu = find_cpu(idx);
> -            /* invalid CPU/thread specified */
> -            if (!idx || !cpu) {
> +            /* invalid thread specified, cpu not found. */
> +            if (!cpu) {
>                  res = -EINVAL;
>                  goto out;
>              }


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH v1 2/2] gdbstub: don't fail on vCont; C04:0; c packets

[Alex Bennée]

Greg Kurz <groug@kaod.org> writes:

> On Wed, 31 May 2017 16:09:33 +0100
> Alex Bennée <alex.bennee@linaro.org> wrote:
>
>> The thread-id of 0 means any CPU but we then ignore the fact we find
>> the first_cpu in this case who can have an index of 0. Instead of
>
> The index can never be 0 in system mode actually, but you're right that this
> check doesn't make sense.
>
> The code still looks a bit convoluted IMHO. What about something like the
> following ?
>
>             /* 0 means any thread, so we pick the first valid CPU */
>             cpu = tmp ? find_cpu(tmp) : first_cpu;
>
>             /* invalid CPU/thread specified */
>             if (!cpu) {
>                 res = -EINVAL;
>                 goto out;
>             }

Yeah that will make it cleaner, I'll apply to v2.

>
> Anyway, the fix looks ok.
>
> Reviewed-by: Greg Kurz <groug@kaod.org>
>
>> bailing out just test if we have managed to match up thread-id to a
>> CPU.
>>
>> Otherwise you get:
>>   gdb_handle_packet: command='vCont;C04:0;c'
>>   put_packet: reply='E22'
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  gdbstub.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/gdbstub.c b/gdbstub.c
>> index a249846954..29c9ed3002 100644
>> --- a/gdbstub.c
>> +++ b/gdbstub.c
>> @@ -934,8 +934,8 @@ static int gdb_handle_vcont(GDBState *s, const char *p)
>>               * CPU first, and only then we can use its index.
>>               */
>>              cpu = find_cpu(idx);
>> -            /* invalid CPU/thread specified */
>> -            if (!idx || !cpu) {
>> +            /* invalid thread specified, cpu not found. */
>> +            if (!cpu) {
>>                  res = -EINVAL;
>>                  goto out;
>>              }


--
Alex Bennée

Re: [Qemu-devel] [PATCH v1 2/2] gdbstub: don't fail on vCont; C04:0; c packets

[Claudio Imbrenda]

On Wed, 31 May 2017 16:09:33 +0100
Alex Bennée <alex.bennee@linaro.org> wrote:

> The thread-id of 0 means any CPU but we then ignore the fact we find
> the first_cpu in this case who can have an index of 0. Instead of
> bailing out just test if we have managed to match up thread-id to a
> CPU.
> 
> Otherwise you get:
>   gdb_handle_packet: command='vCont;C04:0;c'
>   put_packet: reply='E22'
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  gdbstub.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index a249846954..29c9ed3002 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -934,8 +934,8 @@ static int gdb_handle_vcont(GDBState *s, const
> char *p)
>               * CPU first, and only then we can use its index.
>               */
>              cpu = find_cpu(idx);
> -            /* invalid CPU/thread specified */
> -            if (!idx || !cpu) {
> +            /* invalid thread specified, cpu not found. */
> +            if (!cpu) {
>                  res = -EINVAL;
>                  goto out;
>              }

This is strange. cpu_index() is defined as:

static inline int cpu_index(CPUState *cpu)
{
#if defined(CONFIG_USER_ONLY)
    return cpu->host_tid;
#else
    return cpu->cpu_index + 1;
#endif
}

therefore it shouldn't return 0 under any circumstance, and
find_cpu(idx) should also fail if idx == 0, because internally it also
uses cpu_index()

on the other hand, you say that the patch does fix the problem for you,
which really confuses me.



(probably) completely unrelatedly, this:

res = qemu_strtoul(p + 1, &p, 16, &tmp);

should be like this instead:

res = qemu_strtoul(p, &p, 16, &tmp);

but this shouldn't impact you in any way.

Re: [Qemu-devel] [PATCH v1 2/2] gdbstub: don't fail on vCont; C04:0; c packets

[Alex Bennée]

Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> writes:

> On Wed, 31 May 2017 16:09:33 +0100
> Alex Bennée <alex.bennee@linaro.org> wrote:
>
>> The thread-id of 0 means any CPU but we then ignore the fact we find
>> the first_cpu in this case who can have an index of 0. Instead of
>> bailing out just test if we have managed to match up thread-id to a
>> CPU.
>>
>> Otherwise you get:
>>   gdb_handle_packet: command='vCont;C04:0;c'
>>   put_packet: reply='E22'
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  gdbstub.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/gdbstub.c b/gdbstub.c
>> index a249846954..29c9ed3002 100644
>> --- a/gdbstub.c
>> +++ b/gdbstub.c
>> @@ -934,8 +934,8 @@ static int gdb_handle_vcont(GDBState *s, const
>> char *p)
>>               * CPU first, and only then we can use its index.
>>               */
>>              cpu = find_cpu(idx);
>> -            /* invalid CPU/thread specified */
>> -            if (!idx || !cpu) {
>> +            /* invalid thread specified, cpu not found. */
>> +            if (!cpu) {
>>                  res = -EINVAL;
>>                  goto out;
>>              }
>
> This is strange. cpu_index() is defined as:
>
> static inline int cpu_index(CPUState *cpu)
> {
> #if defined(CONFIG_USER_ONLY)
>     return cpu->host_tid;
> #else
>     return cpu->cpu_index + 1;
> #endif
> }
>
> therefore it shouldn't return 0 under any circumstance, and
> find_cpu(idx) should also fail if idx == 0, because internally it also
> uses cpu_index()
>
> on the other hand, you say that the patch does fix the problem for you,
> which really confuses me.

Hmm that was me assuming cpu_index(cpu) was the same as cpu->cpu_index.
However in the CONFIG_USER_ONLY case we return host_tid which is only
set by clone_func() so I think will not be set for the first cpu.

>
>
>
> (probably) completely unrelatedly, this:
>
> res = qemu_strtoul(p + 1, &p, 16, &tmp);
>
> should be like this instead:
>
> res = qemu_strtoul(p, &p, 16, &tmp);
>
> but this shouldn't impact you in any way.


--
Alex Bennée

Re: [Qemu-devel] [PATCH v1 2/2] gdbstub: don't fail on vCont; C04:0; c packets

[Greg Kurz]

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

On Wed, 31 May 2017 18:17:37 +0200
Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> wrote:

> On Wed, 31 May 2017 16:09:33 +0100
> Alex Bennée <alex.bennee@linaro.org> wrote:
> 
> > The thread-id of 0 means any CPU but we then ignore the fact we find
> > the first_cpu in this case who can have an index of 0. Instead of
> > bailing out just test if we have managed to match up thread-id to a
> > CPU.
> > 
> > Otherwise you get:
> >   gdb_handle_packet: command='vCont;C04:0;c'
> >   put_packet: reply='E22'
> > 
> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> > ---
> >  gdbstub.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/gdbstub.c b/gdbstub.c
> > index a249846954..29c9ed3002 100644
> > --- a/gdbstub.c
> > +++ b/gdbstub.c
> > @@ -934,8 +934,8 @@ static int gdb_handle_vcont(GDBState *s, const
> > char *p)
> >               * CPU first, and only then we can use its index.
> >               */
> >              cpu = find_cpu(idx);
> > -            /* invalid CPU/thread specified */
> > -            if (!idx || !cpu) {
> > +            /* invalid thread specified, cpu not found. */
> > +            if (!cpu) {
> >                  res = -EINVAL;
> >                  goto out;
> >              }  
> 
> This is strange. cpu_index() is defined as:
> 
> static inline int cpu_index(CPUState *cpu)
> {
> #if defined(CONFIG_USER_ONLY)
>     return cpu->host_tid;
> #else
>     return cpu->cpu_index + 1;
> #endif
> }
> 
> therefore it shouldn't return 0 under any circumstance, and

I think it is 0 for first_cpu in user mode.

> find_cpu(idx) should also fail if idx == 0, because internally it also
> uses cpu_index()
> 
> on the other hand, you say that the patch does fix the problem for you,
> which really confuses me.
> 
> 
> 
> (probably) completely unrelatedly, this:
> 
> res = qemu_strtoul(p + 1, &p, 16, &tmp);
> 
> should be like this instead:
> 
> res = qemu_strtoul(p, &p, 16, &tmp);
> 
> but this shouldn't impact you in any way.
> 
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH v1 2/2] gdbstub: don't fail on vCont; C04:0; c packets

[Claudio Imbrenda]

On Wed, 31 May 2017 18:33:24 +0200
Greg Kurz <groug@kaod.org> wrote:

> On Wed, 31 May 2017 18:17:37 +0200
> Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> wrote:
> 
> > On Wed, 31 May 2017 16:09:33 +0100
> > Alex Bennée <alex.bennee@linaro.org> wrote:
> >   
> > > The thread-id of 0 means any CPU but we then ignore the fact we
> > > find the first_cpu in this case who can have an index of 0.
> > > Instead of bailing out just test if we have managed to match up
> > > thread-id to a CPU.
> > > 
> > > Otherwise you get:
> > >   gdb_handle_packet: command='vCont;C04:0;c'
> > >   put_packet: reply='E22'
> > > 
> > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> > > ---
> > >  gdbstub.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/gdbstub.c b/gdbstub.c
> > > index a249846954..29c9ed3002 100644
> > > --- a/gdbstub.c
> > > +++ b/gdbstub.c
> > > @@ -934,8 +934,8 @@ static int gdb_handle_vcont(GDBState *s, const
> > > char *p)
> > >               * CPU first, and only then we can use its index.
> > >               */
> > >              cpu = find_cpu(idx);
> > > -            /* invalid CPU/thread specified */
> > > -            if (!idx || !cpu) {
> > > +            /* invalid thread specified, cpu not found. */
> > > +            if (!cpu) {
> > >                  res = -EINVAL;
> > >                  goto out;
> > >              }    
> > 
> > This is strange. cpu_index() is defined as:
> > 
> > static inline int cpu_index(CPUState *cpu)
> > {
> > #if defined(CONFIG_USER_ONLY)
> >     return cpu->host_tid;
> > #else
> >     return cpu->cpu_index + 1;
> > #endif
> > }
> > 
> > therefore it shouldn't return 0 under any circumstance, and  
> 
> I think it is 0 for first_cpu in user mode.

in linux-user/syscall.c:

info->tid = gettid();
cpu->host_tid = info->tid;

kernel thread-ids are system-wide unique and can't be 0
 
> > find_cpu(idx) should also fail if idx == 0, because internally it
> > also uses cpu_index()
> > 
> > on the other hand, you say that the patch does fix the problem for
> > you, which really confuses me.
> > 
> > 
> > 
> > (probably) completely unrelatedly, this:
> > 
> > res = qemu_strtoul(p + 1, &p, 16, &tmp);
> > 
> > should be like this instead:
> > 
> > res = qemu_strtoul(p, &p, 16, &tmp);
> > 
> > but this shouldn't impact you in any way.
> > 
> > 
> >   
> 

Re: [Qemu-devel] [PATCH v1 2/2] gdbstub: don't fail on vCont; C04:0; c packets

[Greg Kurz]

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

On Wed, 31 May 2017 18:51:06 +0200
Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> wrote:
[...]
> > > 
> > > This is strange. cpu_index() is defined as:
> > > 
> > > static inline int cpu_index(CPUState *cpu)
> > > {
> > > #if defined(CONFIG_USER_ONLY)
> > >     return cpu->host_tid;
> > > #else
> > >     return cpu->cpu_index + 1;
> > > #endif
> > > }
> > > 
> > > therefore it shouldn't return 0 under any circumstance, and    
> > 
> > I think it is 0 for first_cpu in user mode.  
> 
> in linux-user/syscall.c:
> 
> info->tid = gettid();
> cpu->host_tid = info->tid;
> 
> kernel thread-ids are system-wide unique and can't be 0
>  

This is correct but these lines are in clone_func(). This gets called for
all threads but the "main" thread which I believe to be associated to
first_cpu.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH v1 2/2] gdbstub: don't fail on vCont; C04:0; c packets

[Claudio Imbrenda]

On Wed, 31 May 2017 19:06:29 +0200
Greg Kurz <groug@kaod.org> wrote:

> On Wed, 31 May 2017 18:51:06 +0200
> Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> wrote:
> [...]
> > > > 
> > > > This is strange. cpu_index() is defined as:
> > > > 
> > > > static inline int cpu_index(CPUState *cpu)
> > > > {
> > > > #if defined(CONFIG_USER_ONLY)
> > > >     return cpu->host_tid;
> > > > #else
> > > >     return cpu->cpu_index + 1;
> > > > #endif
> > > > }
> > > > 
> > > > therefore it shouldn't return 0 under any circumstance,
> > > > and      
> > > 
> > > I think it is 0 for first_cpu in user mode.    
> > 
> > in linux-user/syscall.c:
> > 
> > info->tid = gettid();
> > cpu->host_tid = info->tid;
> > 
> > kernel thread-ids are system-wide unique and can't be 0
> >    
> 
> This is correct but these lines are in clone_func(). This gets called
> for all threads but the "main" thread which I believe to be
> associated to first_cpu.

then IMHO that is a bug and it needs to be corrected. the host_tid
should be, well, the host tid, and not 0, which is never a valid
tid for Linux.

the current behaviour is simply the easiest for the "any CPU" case.
Picking the last CPU or a random one would still be correct, and in
that case there would be no way to explicitly address the first CPU.

Re: [Qemu-devel] [PATCH v1 2/2] gdbstub: don't fail on vCont; C04:0; c packets

[Alex Bennée]

Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> writes:

> On Wed, 31 May 2017 19:06:29 +0200
> Greg Kurz <groug@kaod.org> wrote:
>
>> On Wed, 31 May 2017 18:51:06 +0200
>> Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> wrote:
>> [...]
>> > > >
>> > > > This is strange. cpu_index() is defined as:
>> > > >
>> > > > static inline int cpu_index(CPUState *cpu)
>> > > > {
>> > > > #if defined(CONFIG_USER_ONLY)
>> > > >     return cpu->host_tid;
>> > > > #else
>> > > >     return cpu->cpu_index + 1;
>> > > > #endif
>> > > > }
>> > > >
>> > > > therefore it shouldn't return 0 under any circumstance,
>> > > > and
>> > >
>> > > I think it is 0 for first_cpu in user mode.
>> >
>> > in linux-user/syscall.c:
>> >
>> > info->tid = gettid();
>> > cpu->host_tid = info->tid;
>> >
>> > kernel thread-ids are system-wide unique and can't be 0
>> >
>>
>> This is correct but these lines are in clone_func(). This gets called
>> for all threads but the "main" thread which I believe to be
>> associated to first_cpu.
>
> then IMHO that is a bug and it needs to be corrected. the host_tid
> should be, well, the host tid, and not 0, which is never a valid
> tid for Linux.
>
> the current behaviour is simply the easiest for the "any CPU" case.
> Picking the last CPU or a random one would still be correct, and in
> that case there would be no way to explicitly address the first CPU.

OK I'll include a fix in the next iteration.

--
Alex Bennée

Re: [Qemu-devel] [PATCH v1 2/2] gdbstub: don't fail on vCont; C04:0; c packets

[Greg Kurz]

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

On Wed, 31 May 2017 19:40:46 +0200
Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> wrote:

> On Wed, 31 May 2017 19:06:29 +0200
> Greg Kurz <groug@kaod.org> wrote:
> 
> > On Wed, 31 May 2017 18:51:06 +0200
> > Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> wrote:
> > [...]  
> > > > > 
> > > > > This is strange. cpu_index() is defined as:
> > > > > 
> > > > > static inline int cpu_index(CPUState *cpu)
> > > > > {
> > > > > #if defined(CONFIG_USER_ONLY)
> > > > >     return cpu->host_tid;
> > > > > #else
> > > > >     return cpu->cpu_index + 1;
> > > > > #endif
> > > > > }
> > > > > 
> > > > > therefore it shouldn't return 0 under any circumstance,
> > > > > and        
> > > > 
> > > > I think it is 0 for first_cpu in user mode.      
> > > 
> > > in linux-user/syscall.c:
> > > 
> > > info->tid = gettid();
> > > cpu->host_tid = info->tid;
> > > 
> > > kernel thread-ids are system-wide unique and can't be 0
> > >      
> > 
> > This is correct but these lines are in clone_func(). This gets called
> > for all threads but the "main" thread which I believe to be
> > associated to first_cpu.  
> 
> then IMHO that is a bug and it needs to be corrected. the host_tid
> should be, well, the host tid, and not 0, which is never a valid
> tid for Linux.
> 

I tend to agree indeed. It isn't a problem for user mode though since it
doesn't use @host_tid. Only gdbstub does.

$ git grep host_tid
include/exec/gdbstub.h:    return cpu->host_tid;
include/qom/cpu.h: * @host_tid: Host thread ID.
include/qom/cpu.h:    uint32_t host_tid;
linux-user/syscall.c:    cpu->host_tid = info->tid;

> the current behaviour is simply the easiest for the "any CPU" case.
> Picking the last CPU or a random one would still be correct, and in
> that case there would be no way to explicitly address the first CPU.
> 

I'm not familiar enough with gdbstub to know if this is a real problem.
But I guess it is possible to add a "first_cpu->host_tid = gettid();" line
somewhere in linux-user/main.c.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH v1 2/2] gdbstub: don't fail on vCont; C04:0; c packets

[Alex Bennée]

Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> writes:

> On Wed, 31 May 2017 18:33:24 +0200
> Greg Kurz <groug@kaod.org> wrote:
>
>> On Wed, 31 May 2017 18:17:37 +0200
>> Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> wrote:
>>
>> > On Wed, 31 May 2017 16:09:33 +0100
>> > Alex Bennée <alex.bennee@linaro.org> wrote:
>> >
>> > > The thread-id of 0 means any CPU but we then ignore the fact we
>> > > find the first_cpu in this case who can have an index of 0.
>> > > Instead of bailing out just test if we have managed to match up
>> > > thread-id to a CPU.
>> > >
>> > > Otherwise you get:
>> > >   gdb_handle_packet: command='vCont;C04:0;c'
>> > >   put_packet: reply='E22'
>> > >
>> > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> > > ---
>> > >  gdbstub.c | 4 ++--
>> > >  1 file changed, 2 insertions(+), 2 deletions(-)
>> > >
>> > > diff --git a/gdbstub.c b/gdbstub.c
>> > > index a249846954..29c9ed3002 100644
>> > > --- a/gdbstub.c
>> > > +++ b/gdbstub.c
>> > > @@ -934,8 +934,8 @@ static int gdb_handle_vcont(GDBState *s, const
>> > > char *p)
>> > >               * CPU first, and only then we can use its index.
>> > >               */
>> > >              cpu = find_cpu(idx);
>> > > -            /* invalid CPU/thread specified */
>> > > -            if (!idx || !cpu) {
>> > > +            /* invalid thread specified, cpu not found. */
>> > > +            if (!cpu) {
>> > >                  res = -EINVAL;
>> > >                  goto out;
>> > >              }
>> >
>> > This is strange. cpu_index() is defined as:
>> >
>> > static inline int cpu_index(CPUState *cpu)
>> > {
>> > #if defined(CONFIG_USER_ONLY)
>> >     return cpu->host_tid;
>> > #else
>> >     return cpu->cpu_index + 1;
>> > #endif
>> > }
>> >
>> > therefore it shouldn't return 0 under any circumstance, and
>>
>> I think it is 0 for first_cpu in user mode.
>
> in linux-user/syscall.c:
>
> info->tid = gettid();
> cpu->host_tid = info->tid;
>
> kernel thread-ids are system-wide unique and can't be 0

This only applies to newly cloned threads. The first is unset.

>
>> > find_cpu(idx) should also fail if idx == 0, because internally it
>> > also uses cpu_index()
>> >
>> > on the other hand, you say that the patch does fix the problem for
>> > you, which really confuses me.
>> >
>> >
>> >
>> > (probably) completely unrelatedly, this:
>> >
>> > res = qemu_strtoul(p + 1, &p, 16, &tmp);
>> >
>> > should be like this instead:
>> >
>> > res = qemu_strtoul(p, &p, 16, &tmp);
>> >
>> > but this shouldn't impact you in any way.
>> >
>> >
>> >
>>


--
Alex Bennée