This patch changes GDBState's line_buf from a character
array to a GString. This allows line_buf to be dynamically
re-sizeable.

Signed-off-by: Vacha Bhavsar <vacha.bhavsar@oss.qualcomm.com>
---
Changes since v4:
- this patch was not present in v4, it has been added as
suggested during review of v4
---
 gdbstub/gdbstub.c   | 25 +++++++++++++------------
 gdbstub/internals.h |  3 +--
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index dd5fb5667c..0634ff9e91 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -64,6 +64,7 @@ void gdb_init_gdbserver_state(void)
     memset(&gdbserver_state, 0, sizeof(GDBState));
     gdbserver_state.init = true;
     gdbserver_state.str_buf = g_string_new(NULL);
+    gdbserver_state.line_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);
 
@@ -2369,7 +2370,7 @@ void gdb_read_byte(uint8_t ch)
         case RS_IDLE:
             if (ch == '$') {
                 /* start of command packet */
-                gdbserver_state.line_buf_index = 0;
+                g_string_set_size(gdbserver_state.line_buf, 0);
                 gdbserver_state.line_sum = 0;
                 gdbserver_state.state = RS_GETLINE;
             } else if (ch == '+') {
@@ -2393,12 +2394,12 @@ void gdb_read_byte(uint8_t ch)
             } else if (ch == '#') {
                 /* end of command, start of checksum*/
                 gdbserver_state.state = RS_CHKSUM1;
-            } else if (gdbserver_state.line_buf_index >= sizeof(gdbserver_state.line_buf) - 1) {
+            } else if (gdbserver_state.line_buf->len >= MAX_PACKET_LENGTH) {
                 trace_gdbstub_err_overrun();
                 gdbserver_state.state = RS_IDLE;
             } else {
                 /* unescaped command character */
-                gdbserver_state.line_buf[gdbserver_state.line_buf_index++] = ch;
+                g_string_append_c(gdbserver_state.line_buf, (gchar) ch);
                 gdbserver_state.line_sum += ch;
             }
             break;
@@ -2406,13 +2407,13 @@ void gdb_read_byte(uint8_t ch)
             if (ch == '#') {
                 /* unexpected end of command in escape sequence */
                 gdbserver_state.state = RS_CHKSUM1;
-            } else if (gdbserver_state.line_buf_index >= sizeof(gdbserver_state.line_buf) - 1) {
+            } else if (gdbserver_state.line_buf->len >= MAX_PACKET_LENGTH) {
                 /* command buffer overrun */
                 trace_gdbstub_err_overrun();
                 gdbserver_state.state = RS_IDLE;
             } else {
                 /* parse escaped character and leave escape state */
-                gdbserver_state.line_buf[gdbserver_state.line_buf_index++] = ch ^ 0x20;
+                g_string_append_c(gdbserver_state.line_buf, (gchar) ch ^ 0x20);
                 gdbserver_state.line_sum += ch;
                 gdbserver_state.state = RS_GETLINE;
             }
@@ -2429,19 +2430,20 @@ void gdb_read_byte(uint8_t ch)
             } else {
                 /* decode repeat length */
                 int repeat = ch - ' ' + 3;
-                if (gdbserver_state.line_buf_index + repeat >= sizeof(gdbserver_state.line_buf) - 1) {
+                if (gdbserver_state.line_buf->len + repeat >= MAX_PACKET_LENGTH) {
                     /* that many repeats would overrun the command buffer */
                     trace_gdbstub_err_overrun();
                     gdbserver_state.state = RS_IDLE;
-                } else if (gdbserver_state.line_buf_index < 1) {
+                } else if (gdbserver_state.line_buf->len < 1) {
                     /* got a repeat but we have nothing to repeat */
                     trace_gdbstub_err_invalid_rle();
                     gdbserver_state.state = RS_GETLINE;
                 } else {
                     /* repeat the last character */
-                    memset(gdbserver_state.line_buf + gdbserver_state.line_buf_index,
-                           gdbserver_state.line_buf[gdbserver_state.line_buf_index - 1], repeat);
-                    gdbserver_state.line_buf_index += repeat;
+                    for (int i = 0; i < repeat; i ++){
+                        g_string_append_c(gdbserver_state.line_buf, 
+                            gdbserver_state.line_buf->str[gdbserver_state.line_buf->len - 1]);
+                    }
                     gdbserver_state.line_sum += ch;
                     gdbserver_state.state = RS_GETLINE;
                 }
@@ -2454,7 +2456,6 @@ void gdb_read_byte(uint8_t ch)
                 gdbserver_state.state = RS_GETLINE;
                 break;
             }
-            gdbserver_state.line_buf[gdbserver_state.line_buf_index] = '\0';
             gdbserver_state.line_csum = fromhex(ch) << 4;
             gdbserver_state.state = RS_CHKSUM2;
             break;
@@ -2477,7 +2478,7 @@ void gdb_read_byte(uint8_t ch)
                 /* send ACK reply */
                 reply = '+';
                 gdb_put_buffer(&reply, 1);
-                gdbserver_state.state = gdb_handle_packet(gdbserver_state.line_buf);
+                gdbserver_state.state = gdb_handle_packet((char *) gdbserver_state.line_buf->str);
             }
             break;
         default:
diff --git a/gdbstub/internals.h b/gdbstub/internals.h
index 87f64b6318..27afbef4f5 100644
--- a/gdbstub/internals.h
+++ b/gdbstub/internals.h
@@ -72,8 +72,7 @@ typedef struct GDBState {
     CPUState *g_cpu; /* current CPU for other ops */
     CPUState *query_cpu; /* for q{f|s}ThreadInfo */
     enum RSState state; /* parsing state */
-    char line_buf[MAX_PACKET_LENGTH];
-    int line_buf_index;
+    GString *line_buf;
     int line_sum; /* running checksum */
     int line_csum; /* checksum at the end of the packet */
     GByteArray *last_packet;
-- 
2.34.1

On Mon, 11 Aug 2025 at 20:37, Vacha Bhavsar
<vacha.bhavsar@oss.qualcomm.com> wrote:
>
> This patch changes GDBState's line_buf from a character
> array to a GString. This allows line_buf to be dynamically
> re-sizeable.
>
> Signed-off-by: Vacha Bhavsar <vacha.bhavsar@oss.qualcomm.com>
> ---
> Changes since v4:
> - this patch was not present in v4, it has been added as
> suggested during review of v4
> ---

Note that since the GDBState is a file-local variable,
not allocated on the stack, it's not an issue that
line_buf[MAX_PACKET_LENGTH] is large. So whether we
make this change I think should be based on whether
it makes the code easier to read and less likely to
contain string buffer manipulation bugs.


> -                if (gdbserver_state.line_buf_index + repeat >= > sizeof(gdbserver_state.line_buf) - 1) {
> +                if (gdbserver_state.line_buf->len + repeat >= MAX_PACKET_LENGTH) {
>                      /* that many repeats would overrun the command buffer */

This comment no longer makes sense if we don't have a
fixed command buffer. In general, if we're moving away
from a fixed-sized buffer we should consider what we
want to do in all the various places that are currently
doing checks against MAX_PACKET_LENGTH.

thanks
-- PMM

Hi,



Since this change does not directly impact the original concern

for adding SME register exposure to remote GDB (for which

the resizing of MAX_PACKET_LENGTH is sufficient), may we leave

this patch open to further discussion while the others go through

with review?



Thanks,

Vacha

On Tue, Aug 12, 2025 at 7:33 AM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Mon, 11 Aug 2025 at 20:37, Vacha Bhavsar
> <vacha.bhavsar@oss.qualcomm.com> wrote:
> >
> > This patch changes GDBState's line_buf from a character
> > array to a GString. This allows line_buf to be dynamically
> > re-sizeable.
> >
> > Signed-off-by: Vacha Bhavsar <vacha.bhavsar@oss.qualcomm.com>
> > ---
> > Changes since v4:
> > - this patch was not present in v4, it has been added as
> > suggested during review of v4
> > ---
>
> Note that since the GDBState is a file-local variable,
> not allocated on the stack, it's not an issue that
> line_buf[MAX_PACKET_LENGTH] is large. So whether we
> make this change I think should be based on whether
> it makes the code easier to read and less likely to
> contain string buffer manipulation bugs.
>
>
> > -                if (gdbserver_state.line_buf_index + repeat >= >
> sizeof(gdbserver_state.line_buf) - 1) {
> > +                if (gdbserver_state.line_buf->len + repeat >=
> MAX_PACKET_LENGTH) {
> >                      /* that many repeats would overrun the command
> buffer */
>
> This comment no longer makes sense if we don't have a
> fixed command buffer. In general, if we're moving away
> from a fixed-sized buffer we should consider what we
> want to do in all the various places that are currently
> doing checks against MAX_PACKET_LENGTH.
>
> thanks
> -- PMM
>