Re: [PATCH] gdbstub: change GDBState.last_packet to GByteArray

[Damien Hedde <damien.hedde@greensocs.com>]

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