gdbstub.c | 39 +++++++++++++++++++++------------------ 1 file changed, 21 insertions(+), 18 deletions(-)
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>
---
This patch 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.
Based-on <20191130084602.10818-1-alex.bennee@linaro.org>
---
gdbstub.c | 39 +++++++++++++++++++++------------------
1 file changed, 21 insertions(+), 18 deletions(-)
diff --git a/gdbstub.c b/gdbstub.c
index 7b695bdebe..022edd6bdb 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;
for (;;) {
if (len <= max_sz) {
gdb_monitor_output(&gdbserver_state, p, len);
--
2.24.0
On 12/6/19 2:42 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> > --- > > This patch 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. > > Based-on <20191130084602.10818-1-alex.bennee@linaro.org> > --- > gdbstub.c | 39 +++++++++++++++++++++------------------ > 1 file changed, 21 insertions(+), 18 deletions(-) > > diff --git a/gdbstub.c b/gdbstub.c > index 7b695bdebe..022edd6bdb 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; > for (;;) { > if (len <= max_sz) { > gdb_monitor_output(&gdbserver_state, p, len); > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
On 12/6/19 2:42 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> > --- > > This patch 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. > > Based-on <20191130084602.10818-1-alex.bennee@linaro.org> > --- > gdbstub.c | 39 +++++++++++++++++++++------------------ > 1 file changed, 21 insertions(+), 18 deletions(-) > > diff --git a/gdbstub.c b/gdbstub.c > index 7b695bdebe..022edd6bdb 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; I think max_sz is not the same as before here. Before we had (sizeof(gdbserver_state.last_packet) - 2) / 2 -> ((MAX_PACKET_LENGTH + 4) - 2) / 2 -> (MAX_PACKET_LENGTH / 2) + 1 Now we have MAX_PACKET_LENGTH / 2 But looking at the gdb_monitor_output() function, it performs the following check: if (len > (MAX_PACKET_LENGTH/2) - 1) len = (MAX_PACKET_LENGTH/2) - 1; So both versions seem actually wrong and can lead to characters being dropped. However gdb_monitor_output() is still using a MAX_PACKET_LENGTH sized local buffer. With this patch it can probably be removed entirely so that last_packet is written directly (with a g_byte_array aware version of memtohex?). -- Luc > for (;;) { > if (len <= max_sz) { > gdb_monitor_output(&gdbserver_state, p, len); >
On 12/9/19 11:15 AM, Luc Michel wrote: > > > On 12/6/19 2:42 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> >> --- >> >> This patch 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. >> >> Based-on <20191130084602.10818-1-alex.bennee@linaro.org> >> --- >> gdbstub.c | 39 +++++++++++++++++++++------------------ >> 1 file changed, 21 insertions(+), 18 deletions(-) >> >> diff --git a/gdbstub.c b/gdbstub.c >> index 7b695bdebe..022edd6bdb 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; > I think max_sz is not the same as before here. > > Before we had (sizeof(gdbserver_state.last_packet) - 2) / 2 > -> ((MAX_PACKET_LENGTH + 4) - 2) / 2 > -> (MAX_PACKET_LENGTH / 2) + 1 > > Now we have MAX_PACKET_LENGTH / 2 Oups... I'll fix that. > > But looking at the gdb_monitor_output() function, it performs the > following check: > if (len > (MAX_PACKET_LENGTH/2) - 1) > len = (MAX_PACKET_LENGTH/2) - 1; > > So both versions seem actually wrong and can lead to characters being > dropped. > > However gdb_monitor_output() is still using a MAX_PACKET_LENGTH sized > local buffer. With this patch it can probably be removed entirely so > that last_packet is written directly (with a g_byte_array aware version > of memtohex?). > Actually, Alex's patches (on which this patch is based) already do the memtohex aware version. But yes we can simplify this, I'll prepare a v2 with this. Thanks, -- Damien
© 2016 - 2024 Red Hat, Inc.