* [PATCH v2 0/2] gdbstub: allow sending packet of arbitrary length
@ 2019-12-11 16:05 Damien Hedde
2019-12-11 16:05 ` [PATCH v2 1/2] gdbstub: change GDBState.last_packet to GByteArray Damien Hedde
2019-12-11 16:05 ` [PATCH v2 2/2] gdbstub: do not split gdb_monitor_write payload Damien Hedde
0 siblings, 2 replies; 11+ messages in thread
From: Damien Hedde @ 2019-12-11 16:05 UTC (permalink / raw)
To: qemu-devel; +Cc: Damien Hedde, philmd, alex.bennee, luc.michel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/2] gdbstub: change GDBState.last_packet to GByteArray
2019-12-11 16:05 [PATCH v2 0/2] gdbstub: allow sending packet of arbitrary length Damien Hedde
@ 2019-12-11 16:05 ` Damien Hedde
2019-12-11 18:50 ` Alex Bennée
2019-12-12 14:15 ` Luc Michel
2019-12-11 16:05 ` [PATCH v2 2/2] gdbstub: do not split gdb_monitor_write payload Damien Hedde
1 sibling, 2 replies; 11+ messages in thread
From: Damien Hedde @ 2019-12-11 16:05 UTC (permalink / raw)
To: qemu-devel; +Cc: Damien Hedde, philmd, alex.bennee, luc.michel
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/2] gdbstub: do not split gdb_monitor_write payload
2019-12-11 16:05 [PATCH v2 0/2] gdbstub: allow sending packet of arbitrary length Damien Hedde
2019-12-11 16:05 ` [PATCH v2 1/2] gdbstub: change GDBState.last_packet to GByteArray Damien Hedde
@ 2019-12-11 16:05 ` Damien Hedde
2019-12-11 18:58 ` Philippe Mathieu-Daudé
2019-12-11 18:59 ` Alex Bennée
1 sibling, 2 replies; 11+ messages in thread
From: Damien Hedde @ 2019-12-11 16:05 UTC (permalink / raw)
To: qemu-devel; +Cc: Damien Hedde, philmd, alex.bennee, luc.michel
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] gdbstub: change GDBState.last_packet to GByteArray
2019-12-11 16:05 ` [PATCH v2 1/2] gdbstub: change GDBState.last_packet to GByteArray Damien Hedde
@ 2019-12-11 18:50 ` Alex Bennée
2019-12-12 14:15 ` Luc Michel
1 sibling, 0 replies; 11+ messages in thread
From: Alex Bennée @ 2019-12-11 18:50 UTC (permalink / raw)
To: Damien Hedde; +Cc: philmd, qemu-devel, luc.michel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] gdbstub: do not split gdb_monitor_write payload
2019-12-11 16:05 ` [PATCH v2 2/2] gdbstub: do not split gdb_monitor_write payload Damien Hedde
@ 2019-12-11 18:58 ` Philippe Mathieu-Daudé
2019-12-12 9:39 ` Damien Hedde
2019-12-11 18:59 ` Alex Bennée
1 sibling, 1 reply; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-12-11 18:58 UTC (permalink / raw)
To: Damien Hedde, qemu-devel; +Cc: alex.bennee, luc.michel
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;
> }
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] gdbstub: do not split gdb_monitor_write payload
2019-12-11 16:05 ` [PATCH v2 2/2] gdbstub: do not split gdb_monitor_write payload Damien Hedde
2019-12-11 18:58 ` Philippe Mathieu-Daudé
@ 2019-12-11 18:59 ` Alex Bennée
2019-12-12 10:14 ` Damien Hedde
1 sibling, 1 reply; 11+ messages in thread
From: Alex Bennée @ 2019-12-11 18:59 UTC (permalink / raw)
To: Damien Hedde; +Cc: philmd, qemu-devel, luc.michel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] gdbstub: do not split gdb_monitor_write payload
2019-12-11 18:58 ` Philippe Mathieu-Daudé
@ 2019-12-12 9:39 ` Damien Hedde
0 siblings, 0 replies; 11+ messages in thread
From: Damien Hedde @ 2019-12-12 9:39 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel; +Cc: alex.bennee, luc.michel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] gdbstub: do not split gdb_monitor_write payload
2019-12-11 18:59 ` Alex Bennée
@ 2019-12-12 10:14 ` Damien Hedde
2019-12-12 10:52 ` Alex Bennée
0 siblings, 1 reply; 11+ messages in thread
From: Damien Hedde @ 2019-12-12 10:14 UTC (permalink / raw)
To: Alex Bennée; +Cc: philmd, qemu-devel, luc.michel
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;
>> }
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] gdbstub: do not split gdb_monitor_write payload
2019-12-12 10:14 ` Damien Hedde
@ 2019-12-12 10:52 ` Alex Bennée
2019-12-13 12:13 ` Damien Hedde
0 siblings, 1 reply; 11+ messages in thread
From: Alex Bennée @ 2019-12-12 10:52 UTC (permalink / raw)
To: Damien Hedde; +Cc: philmd, qemu-devel, luc.michel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] gdbstub: change GDBState.last_packet to GByteArray
2019-12-11 16:05 ` [PATCH v2 1/2] gdbstub: change GDBState.last_packet to GByteArray Damien Hedde
2019-12-11 18:50 ` Alex Bennée
@ 2019-12-12 14:15 ` Luc Michel
1 sibling, 0 replies; 11+ messages in thread
From: Luc Michel @ 2019-12-12 14:15 UTC (permalink / raw)
To: Damien Hedde, qemu-devel; +Cc: philmd, alex.bennee
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);
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] gdbstub: do not split gdb_monitor_write payload
2019-12-12 10:52 ` Alex Bennée
@ 2019-12-13 12:13 ` Damien Hedde
0 siblings, 0 replies; 11+ messages in thread
From: Damien Hedde @ 2019-12-13 12:13 UTC (permalink / raw)
To: Alex Bennée; +Cc: philmd, qemu-devel, luc.michel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-12-13 21:27 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-11 16:05 [PATCH v2 0/2] gdbstub: allow sending packet of arbitrary length Damien Hedde
2019-12-11 16:05 ` [PATCH v2 1/2] gdbstub: change GDBState.last_packet to GByteArray Damien Hedde
2019-12-11 18:50 ` Alex Bennée
2019-12-12 14:15 ` Luc Michel
2019-12-11 16:05 ` [PATCH v2 2/2] gdbstub: do not split gdb_monitor_write payload Damien Hedde
2019-12-11 18:58 ` Philippe Mathieu-Daudé
2019-12-12 9:39 ` Damien Hedde
2019-12-11 18:59 ` Alex Bennée
2019-12-12 10:14 ` Damien Hedde
2019-12-12 10:52 ` Alex Bennée
2019-12-13 12:13 ` Damien Hedde
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).