[PATCH v2 0/2] gdbstub: allow sending packet of arbitrary length

[PATCH v2 0/2] gdbstub: allow sending packet of arbitrary length

[Damien Hedde]

Hi All,

This series is a follow-up of Alex's series about sve registers
which introduces some GbyteArray/Gstring in the gdbstub.

It removes the remaining barrier to send big packets.

In consequence, we can slso simply gdb_monitor_write().

Based-on <20191130084602.10818-1-alex.bennee@linaro.org>

v2:
+ patch 1: fix gdb_monitor_write() max_sz error (Luc)
+ patch 2: new patch (Luc)

--
Damien

Damien Hedde (2):
  gdbstub: change GDBState.last_packet to GByteArray
  gdbstub: do not split gdb_monitor_write payload

 gdbstub.c | 60 +++++++++++++++++++++----------------------------------
 1 file changed, 23 insertions(+), 37 deletions(-)

-- 
2.24.0

[PATCH v2 1/2] gdbstub: change GDBState.last_packet to GByteArray

[Damien Hedde]

Remove the packet size upper limit by using a GByteArray
instead of a statically allocated array for last_packet.
Thus we can now send big packets.

Also remove the last_packet_len field and use last_packet->len
instead.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 gdbstub.c | 39 +++++++++++++++++++++------------------
 1 file changed, 21 insertions(+), 18 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 7b695bdebe..93b26f1b86 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -351,8 +351,7 @@ typedef struct GDBState {
     int line_buf_index;
     int line_sum; /* running checksum */
     int line_csum; /* checksum at the end of the packet */
-    uint8_t last_packet[MAX_PACKET_LENGTH + 4];
-    int last_packet_len;
+    GByteArray *last_packet;
     int signal;
 #ifdef CONFIG_USER_ONLY
     int fd;
@@ -384,6 +383,7 @@ static void init_gdbserver_state(void)
     gdbserver_state.init = true;
     gdbserver_state.str_buf = g_string_new(NULL);
     gdbserver_state.mem_buf = g_byte_array_sized_new(MAX_PACKET_LENGTH);
+    gdbserver_state.last_packet = g_byte_array_sized_new(MAX_PACKET_LENGTH + 4);
 }
 
 #ifndef CONFIG_USER_ONLY
@@ -626,28 +626,29 @@ static void hexdump(const char *buf, int len,
 static int put_packet_binary(const char *buf, int len, bool dump)
 {
     int csum, i;
-    uint8_t *p;
-    uint8_t *ps = &gdbserver_state.last_packet[0];
+    uint8_t footer[3];
 
     if (dump && trace_event_get_state_backends(TRACE_GDBSTUB_IO_BINARYREPLY)) {
         hexdump(buf, len, trace_gdbstub_io_binaryreply);
     }
 
     for(;;) {
-        p = ps;
-        *(p++) = '$';
-        memcpy(p, buf, len);
-        p += len;
+        g_byte_array_set_size(gdbserver_state.last_packet, 0);
+        g_byte_array_append(gdbserver_state.last_packet,
+                            (const uint8_t *) "$", 1);
+        g_byte_array_append(gdbserver_state.last_packet,
+                            (const uint8_t *) buf, len);
         csum = 0;
         for(i = 0; i < len; i++) {
             csum += buf[i];
         }
-        *(p++) = '#';
-        *(p++) = tohex((csum >> 4) & 0xf);
-        *(p++) = tohex((csum) & 0xf);
+        footer[0] = '#';
+        footer[1] = tohex((csum >> 4) & 0xf);
+        footer[2] = tohex((csum) & 0xf);
+        g_byte_array_append(gdbserver_state.last_packet, footer, 3);
 
-        gdbserver_state.last_packet_len = p - ps;
-        put_buffer(ps, gdbserver_state.last_packet_len);
+        put_buffer(gdbserver_state.last_packet->data,
+                   gdbserver_state.last_packet->len);
 
 #ifdef CONFIG_USER_ONLY
         i = get_char();
@@ -2812,20 +2813,22 @@ static void gdb_read_byte(GDBState *s, uint8_t ch)
     uint8_t reply;
 
 #ifndef CONFIG_USER_ONLY
-    if (gdbserver_state.last_packet_len) {
+    if (gdbserver_state.last_packet->len) {
         /* Waiting for a response to the last packet.  If we see the start
            of a new command then abandon the previous response.  */
         if (ch == '-') {
             trace_gdbstub_err_got_nack();
-            put_buffer((uint8_t *)gdbserver_state.last_packet, gdbserver_state.last_packet_len);
+            put_buffer(gdbserver_state.last_packet->data,
+                       gdbserver_state.last_packet->len);
         } else if (ch == '+') {
             trace_gdbstub_io_got_ack();
         } else {
             trace_gdbstub_io_got_unexpected(ch);
         }
 
-        if (ch == '+' || ch == '$')
-            gdbserver_state.last_packet_len = 0;
+        if (ch == '+' || ch == '$') {
+            g_byte_array_set_size(gdbserver_state.last_packet, 0);
+        }
         if (ch != '$')
             return;
     }
@@ -3209,7 +3212,7 @@ static int gdb_monitor_write(Chardev *chr, const uint8_t *buf, int len)
     const char *p = (const char *)buf;
     int max_sz;
 
-    max_sz = (sizeof(gdbserver_state.last_packet) - 2) / 2;
+    max_sz = (MAX_PACKET_LENGTH / 2) + 1;
     for (;;) {
         if (len <= max_sz) {
             gdb_monitor_output(&gdbserver_state, p, len);
-- 
2.24.0

Re: [PATCH v2 1/2] gdbstub: change GDBState.last_packet to GByteArray

[Alex Bennée]

Damien Hedde <damien.hedde@greensocs.com> writes:

> Remove the packet size upper limit by using a GByteArray
> instead of a statically allocated array for last_packet.
> Thus we can now send big packets.
>
> Also remove the last_packet_len field and use last_packet->len
> instead.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  gdbstub.c | 39 +++++++++++++++++++++------------------
>  1 file changed, 21 insertions(+), 18 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 7b695bdebe..93b26f1b86 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -351,8 +351,7 @@ typedef struct GDBState {
>      int line_buf_index;
>      int line_sum; /* running checksum */
>      int line_csum; /* checksum at the end of the packet */
> -    uint8_t last_packet[MAX_PACKET_LENGTH + 4];
> -    int last_packet_len;
> +    GByteArray *last_packet;
>      int signal;
>  #ifdef CONFIG_USER_ONLY
>      int fd;
> @@ -384,6 +383,7 @@ static void init_gdbserver_state(void)
>      gdbserver_state.init = true;
>      gdbserver_state.str_buf = g_string_new(NULL);
>      gdbserver_state.mem_buf = g_byte_array_sized_new(MAX_PACKET_LENGTH);
> +    gdbserver_state.last_packet = g_byte_array_sized_new(MAX_PACKET_LENGTH + 4);
>  }
>  
>  #ifndef CONFIG_USER_ONLY
> @@ -626,28 +626,29 @@ static void hexdump(const char *buf, int len,
>  static int put_packet_binary(const char *buf, int len, bool dump)
>  {
>      int csum, i;
> -    uint8_t *p;
> -    uint8_t *ps = &gdbserver_state.last_packet[0];
> +    uint8_t footer[3];
>  
>      if (dump && trace_event_get_state_backends(TRACE_GDBSTUB_IO_BINARYREPLY)) {
>          hexdump(buf, len, trace_gdbstub_io_binaryreply);
>      }
>  
>      for(;;) {
> -        p = ps;
> -        *(p++) = '$';
> -        memcpy(p, buf, len);
> -        p += len;
> +        g_byte_array_set_size(gdbserver_state.last_packet, 0);
> +        g_byte_array_append(gdbserver_state.last_packet,
> +                            (const uint8_t *) "$", 1);
> +        g_byte_array_append(gdbserver_state.last_packet,
> +                            (const uint8_t *) buf, len);
>          csum = 0;
>          for(i = 0; i < len; i++) {
>              csum += buf[i];
>          }
> -        *(p++) = '#';
> -        *(p++) = tohex((csum >> 4) & 0xf);
> -        *(p++) = tohex((csum) & 0xf);
> +        footer[0] = '#';
> +        footer[1] = tohex((csum >> 4) & 0xf);
> +        footer[2] = tohex((csum) & 0xf);
> +        g_byte_array_append(gdbserver_state.last_packet, footer, 3);
>  
> -        gdbserver_state.last_packet_len = p - ps;
> -        put_buffer(ps, gdbserver_state.last_packet_len);
> +        put_buffer(gdbserver_state.last_packet->data,
> +                   gdbserver_state.last_packet->len);
>  
>  #ifdef CONFIG_USER_ONLY
>          i = get_char();
> @@ -2812,20 +2813,22 @@ static void gdb_read_byte(GDBState *s, uint8_t ch)
>      uint8_t reply;
>  
>  #ifndef CONFIG_USER_ONLY
> -    if (gdbserver_state.last_packet_len) {
> +    if (gdbserver_state.last_packet->len) {
>          /* Waiting for a response to the last packet.  If we see the start
>             of a new command then abandon the previous response.  */
>          if (ch == '-') {
>              trace_gdbstub_err_got_nack();
> -            put_buffer((uint8_t *)gdbserver_state.last_packet, gdbserver_state.last_packet_len);
> +            put_buffer(gdbserver_state.last_packet->data,
> +                       gdbserver_state.last_packet->len);
>          } else if (ch == '+') {
>              trace_gdbstub_io_got_ack();
>          } else {
>              trace_gdbstub_io_got_unexpected(ch);
>          }
>  
> -        if (ch == '+' || ch == '$')
> -            gdbserver_state.last_packet_len = 0;
> +        if (ch == '+' || ch == '$') {
> +            g_byte_array_set_size(gdbserver_state.last_packet, 0);
> +        }
>          if (ch != '$')
>              return;
>      }
> @@ -3209,7 +3212,7 @@ static int gdb_monitor_write(Chardev *chr, const uint8_t *buf, int len)
>      const char *p = (const char *)buf;
>      int max_sz;
>  
> -    max_sz = (sizeof(gdbserver_state.last_packet) - 2) / 2;
> +    max_sz = (MAX_PACKET_LENGTH / 2) + 1;
>      for (;;) {
>          if (len <= max_sz) {
>              gdb_monitor_output(&gdbserver_state, p, len);


-- 
Alex Bennée

Re: [PATCH v2 1/2] gdbstub: change GDBState.last_packet to GByteArray

[Luc Michel]

On 12/11/19 5:05 PM, Damien Hedde wrote:
> Remove the packet size upper limit by using a GByteArray
> instead of a statically allocated array for last_packet.
> Thus we can now send big packets.
> 
> Also remove the last_packet_len field and use last_packet->len
> instead.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Luc Michel <luc.michel@greensocs.com>

> ---
>  gdbstub.c | 39 +++++++++++++++++++++------------------
>  1 file changed, 21 insertions(+), 18 deletions(-)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index 7b695bdebe..93b26f1b86 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -351,8 +351,7 @@ typedef struct GDBState {
>      int line_buf_index;
>      int line_sum; /* running checksum */
>      int line_csum; /* checksum at the end of the packet */
> -    uint8_t last_packet[MAX_PACKET_LENGTH + 4];
> -    int last_packet_len;
> +    GByteArray *last_packet;
>      int signal;
>  #ifdef CONFIG_USER_ONLY
>      int fd;
> @@ -384,6 +383,7 @@ static void init_gdbserver_state(void)
>      gdbserver_state.init = true;
>      gdbserver_state.str_buf = g_string_new(NULL);
>      gdbserver_state.mem_buf = g_byte_array_sized_new(MAX_PACKET_LENGTH);
> +    gdbserver_state.last_packet = g_byte_array_sized_new(MAX_PACKET_LENGTH + 4);
>  }
>  
>  #ifndef CONFIG_USER_ONLY
> @@ -626,28 +626,29 @@ static void hexdump(const char *buf, int len,
>  static int put_packet_binary(const char *buf, int len, bool dump)
>  {
>      int csum, i;
> -    uint8_t *p;
> -    uint8_t *ps = &gdbserver_state.last_packet[0];
> +    uint8_t footer[3];
>  
>      if (dump && trace_event_get_state_backends(TRACE_GDBSTUB_IO_BINARYREPLY)) {
>          hexdump(buf, len, trace_gdbstub_io_binaryreply);
>      }
>  
>      for(;;) {
> -        p = ps;
> -        *(p++) = '$';
> -        memcpy(p, buf, len);
> -        p += len;
> +        g_byte_array_set_size(gdbserver_state.last_packet, 0);
> +        g_byte_array_append(gdbserver_state.last_packet,
> +                            (const uint8_t *) "$", 1);
> +        g_byte_array_append(gdbserver_state.last_packet,
> +                            (const uint8_t *) buf, len);
>          csum = 0;
>          for(i = 0; i < len; i++) {
>              csum += buf[i];
>          }
> -        *(p++) = '#';
> -        *(p++) = tohex((csum >> 4) & 0xf);
> -        *(p++) = tohex((csum) & 0xf);
> +        footer[0] = '#';
> +        footer[1] = tohex((csum >> 4) & 0xf);
> +        footer[2] = tohex((csum) & 0xf);
> +        g_byte_array_append(gdbserver_state.last_packet, footer, 3);
>  
> -        gdbserver_state.last_packet_len = p - ps;
> -        put_buffer(ps, gdbserver_state.last_packet_len);
> +        put_buffer(gdbserver_state.last_packet->data,
> +                   gdbserver_state.last_packet->len);
>  
>  #ifdef CONFIG_USER_ONLY
>          i = get_char();
> @@ -2812,20 +2813,22 @@ static void gdb_read_byte(GDBState *s, uint8_t ch)
>      uint8_t reply;
>  
>  #ifndef CONFIG_USER_ONLY
> -    if (gdbserver_state.last_packet_len) {
> +    if (gdbserver_state.last_packet->len) {
>          /* Waiting for a response to the last packet.  If we see the start
>             of a new command then abandon the previous response.  */
>          if (ch == '-') {
>              trace_gdbstub_err_got_nack();
> -            put_buffer((uint8_t *)gdbserver_state.last_packet, gdbserver_state.last_packet_len);
> +            put_buffer(gdbserver_state.last_packet->data,
> +                       gdbserver_state.last_packet->len);
>          } else if (ch == '+') {
>              trace_gdbstub_io_got_ack();
>          } else {
>              trace_gdbstub_io_got_unexpected(ch);
>          }
>  
> -        if (ch == '+' || ch == '$')
> -            gdbserver_state.last_packet_len = 0;
> +        if (ch == '+' || ch == '$') {
> +            g_byte_array_set_size(gdbserver_state.last_packet, 0);
> +        }
>          if (ch != '$')
>              return;
>      }
> @@ -3209,7 +3212,7 @@ static int gdb_monitor_write(Chardev *chr, const uint8_t *buf, int len)
>      const char *p = (const char *)buf;
>      int max_sz;
>  
> -    max_sz = (sizeof(gdbserver_state.last_packet) - 2) / 2;
> +    max_sz = (MAX_PACKET_LENGTH / 2) + 1;
>      for (;;) {
>          if (len <= max_sz) {
>              gdb_monitor_output(&gdbserver_state, p, len);
> 

[PATCH v2 2/2] gdbstub: do not split gdb_monitor_write payload

[Damien Hedde]

Since we can now send packets of arbitrary length:
simplify gdb_monitor_write() and send the whole payload
in one packet.

Suggested-by: Luc Michel <luc.michel@greensocs.com>
Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
 gdbstub.c | 23 +++--------------------
 1 file changed, 3 insertions(+), 20 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 93b26f1b86..ef999abee2 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -3200,28 +3200,11 @@ static void gdb_chr_event(void *opaque, int event)
     }
 }
 
-static void gdb_monitor_output(GDBState *s, const char *msg, int len)
-{
-    g_autoptr(GString) buf = g_string_new("O");
-    memtohex(buf, (uint8_t *)msg, len);
-    put_packet(buf->str);
-}
-
 static int gdb_monitor_write(Chardev *chr, const uint8_t *buf, int len)
 {
-    const char *p = (const char *)buf;
-    int max_sz;
-
-    max_sz = (MAX_PACKET_LENGTH / 2) + 1;
-    for (;;) {
-        if (len <= max_sz) {
-            gdb_monitor_output(&gdbserver_state, p, len);
-            break;
-        }
-        gdb_monitor_output(&gdbserver_state, p, max_sz);
-        p += max_sz;
-        len -= max_sz;
-    }
+    g_autoptr(GString) hex_buf = g_string_new("O");
+    memtohex(hex_buf, buf, len);
+    put_packet(hex_buf->str);
     return len;
 }
 
-- 
2.24.0

Re: [PATCH v2 2/2] gdbstub: do not split gdb_monitor_write payload

[Philippe Mathieu-Daudé]

On 12/11/19 5:05 PM, Damien Hedde wrote:
> Since we can now send packets of arbitrary length:
> simplify gdb_monitor_write() and send the whole payload
> in one packet.

While we can send arbitrary length on the wire, we advertise 
PacketSize=MAX_PACKET_LENGTH in handle_query_supported().

We can raise this limit however.

> Suggested-by: Luc Michel <luc.michel@greensocs.com>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
>   gdbstub.c | 23 +++--------------------
>   1 file changed, 3 insertions(+), 20 deletions(-)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index 93b26f1b86..ef999abee2 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -3200,28 +3200,11 @@ static void gdb_chr_event(void *opaque, int event)
>       }
>   }
>   
> -static void gdb_monitor_output(GDBState *s, const char *msg, int len)
> -{
> -    g_autoptr(GString) buf = g_string_new("O");
> -    memtohex(buf, (uint8_t *)msg, len);
> -    put_packet(buf->str);
> -}
> -
>   static int gdb_monitor_write(Chardev *chr, const uint8_t *buf, int len)
>   {
> -    const char *p = (const char *)buf;
> -    int max_sz;
> -
> -    max_sz = (MAX_PACKET_LENGTH / 2) + 1;
> -    for (;;) {
> -        if (len <= max_sz) {
> -            gdb_monitor_output(&gdbserver_state, p, len);
> -            break;
> -        }
> -        gdb_monitor_output(&gdbserver_state, p, max_sz);
> -        p += max_sz;
> -        len -= max_sz;
> -    }
> +    g_autoptr(GString) hex_buf = g_string_new("O");
> +    memtohex(hex_buf, buf, len);
> +    put_packet(hex_buf->str);
>       return len;
>   }
>   
> 

Re: [PATCH v2 2/2] gdbstub: do not split gdb_monitor_write payload

[Damien Hedde]

On 12/11/19 7:58 PM, Philippe Mathieu-Daudé wrote:
> On 12/11/19 5:05 PM, Damien Hedde wrote:
>> Since we can now send packets of arbitrary length:
>> simplify gdb_monitor_write() and send the whole payload
>> in one packet.
> 
> While we can send arbitrary length on the wire, we advertise
> PacketSize=MAX_PACKET_LENGTH in handle_query_supported().
> 
> We can raise this limit however.

Hi Philippe,

This parameter is only about the packet size we can handle (packets we
can receive).

--
Damien

Re: [PATCH v2 2/2] gdbstub: do not split gdb_monitor_write payload

[Alex Bennée]

Damien Hedde <damien.hedde@greensocs.com> writes:

> Since we can now send packets of arbitrary length:
> simplify gdb_monitor_write() and send the whole payload
> in one packet.

Do we know gdb won't barf on us. Does the negotiated max packet size
only apply to data sent to the gdbserver?

>
> Suggested-by: Luc Michel <luc.michel@greensocs.com>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
>  gdbstub.c | 23 +++--------------------
>  1 file changed, 3 insertions(+), 20 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 93b26f1b86..ef999abee2 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -3200,28 +3200,11 @@ static void gdb_chr_event(void *opaque, int event)
>      }
>  }
>  
> -static void gdb_monitor_output(GDBState *s, const char *msg, int len)
> -{
> -    g_autoptr(GString) buf = g_string_new("O");
> -    memtohex(buf, (uint8_t *)msg, len);
> -    put_packet(buf->str);
> -}
> -
>  static int gdb_monitor_write(Chardev *chr, const uint8_t *buf, int len)
>  {
> -    const char *p = (const char *)buf;
> -    int max_sz;
> -
> -    max_sz = (MAX_PACKET_LENGTH / 2) + 1;
> -    for (;;) {
> -        if (len <= max_sz) {
> -            gdb_monitor_output(&gdbserver_state, p, len);
> -            break;
> -        }
> -        gdb_monitor_output(&gdbserver_state, p, max_sz);
> -        p += max_sz;
> -        len -= max_sz;
> -    }
> +    g_autoptr(GString) hex_buf = g_string_new("O");
> +    memtohex(hex_buf, buf, len);
> +    put_packet(hex_buf->str);
>      return len;
>  }


-- 
Alex Bennée

Re: [PATCH v2 2/2] gdbstub: do not split gdb_monitor_write payload

[Damien Hedde]

On 12/11/19 7:59 PM, Alex Bennée wrote:
> 
> Damien Hedde <damien.hedde@greensocs.com> writes:
> 
>> Since we can now send packets of arbitrary length:
>> simplify gdb_monitor_write() and send the whole payload
>> in one packet.
> 
> Do we know gdb won't barf on us. Does the negotiated max packet size
> only apply to data sent to the gdbserver?

Yes the negociated packet size is only about packet we can receive.
Qutoting the gdb doc:
| ‘PacketSize=bytes’
|
|    The remote stub can accept packets up to at least bytes in length.
| GDB will send packets up to this size for bulk transfers, and will
| never send larger packets.

The qSupported doc also says that "Any GDB which sends a ‘qSupported’
packet supports receiving packets of unlimited length".
I did some digging and qSupported appeared in gdb 6.6 (december 2006).
And gdb supported arbitrary sized packet even before that (6.4.9 2006 too).

> 
>>
>> Suggested-by: Luc Michel <luc.michel@greensocs.com>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> ---
>>  gdbstub.c | 23 +++--------------------
>>  1 file changed, 3 insertions(+), 20 deletions(-)
>>
>> diff --git a/gdbstub.c b/gdbstub.c
>> index 93b26f1b86..ef999abee2 100644
>> --- a/gdbstub.c
>> +++ b/gdbstub.c
>> @@ -3200,28 +3200,11 @@ static void gdb_chr_event(void *opaque, int event)
>>      }
>>  }
>>  
>> -static void gdb_monitor_output(GDBState *s, const char *msg, int len)
>> -{
>> -    g_autoptr(GString) buf = g_string_new("O");
>> -    memtohex(buf, (uint8_t *)msg, len);
>> -    put_packet(buf->str);
>> -}
>> -
>>  static int gdb_monitor_write(Chardev *chr, const uint8_t *buf, int len)
>>  {
>> -    const char *p = (const char *)buf;
>> -    int max_sz;
>> -
>> -    max_sz = (MAX_PACKET_LENGTH / 2) + 1;
>> -    for (;;) {
>> -        if (len <= max_sz) {
>> -            gdb_monitor_output(&gdbserver_state, p, len);
>> -            break;
>> -        }
>> -        gdb_monitor_output(&gdbserver_state, p, max_sz);
>> -        p += max_sz;
>> -        len -= max_sz;
>> -    }
>> +    g_autoptr(GString) hex_buf = g_string_new("O");
>> +    memtohex(hex_buf, buf, len);
>> +    put_packet(hex_buf->str);
>>      return len;
>>  }
> 
> 

Re: [PATCH v2 2/2] gdbstub: do not split gdb_monitor_write payload

[Alex Bennée]

Damien Hedde <damien.hedde@greensocs.com> writes:

> On 12/11/19 7:59 PM, Alex Bennée wrote:
>> 
>> Damien Hedde <damien.hedde@greensocs.com> writes:
>> 
>>> Since we can now send packets of arbitrary length:
>>> simplify gdb_monitor_write() and send the whole payload
>>> in one packet.
>> 
>> Do we know gdb won't barf on us. Does the negotiated max packet size
>> only apply to data sent to the gdbserver?
>
> Yes the negociated packet size is only about packet we can receive.
> Qutoting the gdb doc:
> | ‘PacketSize=bytes’
> |
> |    The remote stub can accept packets up to at least bytes in length.
> | GDB will send packets up to this size for bulk transfers, and will
> | never send larger packets.
>
> The qSupported doc also says that "Any GDB which sends a ‘qSupported’
> packet supports receiving packets of unlimited length".
> I did some digging and qSupported appeared in gdb 6.6 (december 2006).
> And gdb supported arbitrary sized packet even before that (6.4.9 2006
> too).

I think that is worth a comment for the function gdb_monitor_write
quoting the spec and the versions. With that comment:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

>
>> 
>>>
>>> Suggested-by: Luc Michel <luc.michel@greensocs.com>
>>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>>> ---
>>>  gdbstub.c | 23 +++--------------------
>>>  1 file changed, 3 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/gdbstub.c b/gdbstub.c
>>> index 93b26f1b86..ef999abee2 100644
>>> --- a/gdbstub.c
>>> +++ b/gdbstub.c
>>> @@ -3200,28 +3200,11 @@ static void gdb_chr_event(void *opaque, int event)
>>>      }
>>>  }
>>>  
>>> -static void gdb_monitor_output(GDBState *s, const char *msg, int len)
>>> -{
>>> -    g_autoptr(GString) buf = g_string_new("O");
>>> -    memtohex(buf, (uint8_t *)msg, len);
>>> -    put_packet(buf->str);
>>> -}
>>> -
>>>  static int gdb_monitor_write(Chardev *chr, const uint8_t *buf, int len)
>>>  {
>>> -    const char *p = (const char *)buf;
>>> -    int max_sz;
>>> -
>>> -    max_sz = (MAX_PACKET_LENGTH / 2) + 1;
>>> -    for (;;) {
>>> -        if (len <= max_sz) {
>>> -            gdb_monitor_output(&gdbserver_state, p, len);
>>> -            break;
>>> -        }
>>> -        gdb_monitor_output(&gdbserver_state, p, max_sz);
>>> -        p += max_sz;
>>> -        len -= max_sz;
>>> -    }
>>> +    g_autoptr(GString) hex_buf = g_string_new("O");
>>> +    memtohex(hex_buf, buf, len);
>>> +    put_packet(hex_buf->str);
>>>      return len;
>>>  }
>> 
>> 


-- 
Alex Bennée

Re: [PATCH v2 2/2] gdbstub: do not split gdb_monitor_write payload

[Damien Hedde]

On 12/12/19 11:52 AM, Alex Bennée wrote:
> 
> Damien Hedde <damien.hedde@greensocs.com> writes:
> 
>> On 12/11/19 7:59 PM, Alex Bennée wrote:
>>>
>>> Damien Hedde <damien.hedde@greensocs.com> writes:
>>>
>>>> Since we can now send packets of arbitrary length:
>>>> simplify gdb_monitor_write() and send the whole payload
>>>> in one packet.
>>>
>>> Do we know gdb won't barf on us. Does the negotiated max packet size
>>> only apply to data sent to the gdbserver?
>>
>> Yes the negociated packet size is only about packet we can receive.
>> Qutoting the gdb doc:
>> | ‘PacketSize=bytes’
>> |
>> |    The remote stub can accept packets up to at least bytes in length.
>> | GDB will send packets up to this size for bulk transfers, and will
>> | never send larger packets.
>>
>> The qSupported doc also says that "Any GDB which sends a ‘qSupported’
>> packet supports receiving packets of unlimited length".
>> I did some digging and qSupported appeared in gdb 6.6 (december 2006).
>> And gdb supported arbitrary sized packet even before that (6.4.9 2006
>> too).
> 
> I think that is worth a comment for the function gdb_monitor_write
> quoting the spec and the versions. With that comment:
> 
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> 

Good idea ! Is that ok if I add these comments in the 1st patch along
with the gdbstate.last_packet field ? it seems a more central place.
I can still add a short note for gdb_monitor_write().

Damien