| Message ID | 20191206134203.77385-1-damien.hedde@greensocs.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | gdbstub: change GDBState.last_packet to GByteArray | expand |
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?).
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
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);
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(-)