diff mbox series

[v9,17/27] gdbstub: Implement v commands with new infra

Message ID 20190502081554.5521-18-arilou@gmail.com
State New
Headers show
Series gdbstub: Refactor command packets handler | expand

Commit Message

Jon Doron May 2, 2019, 8:15 a.m. UTC 
Signed-off-by: Jon Doron <arilou@gmail.com>
---
 gdbstub.c | 170 +++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 110 insertions(+), 60 deletions(-)

Comments

Alex Bennée May 15, 2019, 5:06 p.m. UTC | #1 
Jon Doron <arilou@gmail.com> writes:

> Signed-off-by: Jon Doron <arilou@gmail.com>
> ---
>  gdbstub.c | 170 +++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 110 insertions(+), 60 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 9b0556f8be..d56d0fd235 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1815,6 +1815,106 @@ static void handle_step(GdbCmdContext *gdb_ctx, void *user_ctx)
>      gdb_continue(gdb_ctx->s);
>  }
>
> +static void handle_v_cont_query(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    put_packet(gdb_ctx->s, "vCont;c;C;s;S");
> +}
> +
> +static void handle_v_cont(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    int res;
> +
> +    if (!gdb_ctx->num_params) {
> +        return;
> +    }
> +
> +    res = gdb_handle_vcont(gdb_ctx->s, gdb_ctx->params[0].data);
> +    if ((res == -EINVAL) || (res == -ERANGE)) {
> +        put_packet(gdb_ctx->s, "E22");
> +    } else if (res) {
> +        put_packet(gdb_ctx->s, "\0");

Isn't this just ""?

Either way my reading of the spec say the response needs to be a "Stop
Reply Packet" which I don't think includes empty or E codes.

> +    }
> +}
> +
> +static void handle_v_attach(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    GDBProcess *process;
> +    CPUState *cpu;
> +    char thread_id[16];
> +
> +    strcpy(gdb_ctx->str_buf, "E22");

pstrcpy (see HACKING about strncpy) but...

> +    if (!gdb_ctx->num_params) {
> +        goto cleanup;
> +    }
> +
> +    process = gdb_get_process(gdb_ctx->s, gdb_ctx->params[0].val_ul);
> +    if (!process) {
> +        goto cleanup;
> +    }
> +
> +    cpu = get_first_cpu_in_process(gdb_ctx->s, process);
> +    if (!cpu) {
> +        goto cleanup;
> +    }
> +
> +    process->attached = true;
> +    gdb_ctx->s->g_cpu = cpu;
> +    gdb_ctx->s->c_cpu = cpu;
> +
> +    gdb_fmt_thread_id(gdb_ctx->s, cpu, thread_id, sizeof(thread_id));
> +    snprintf(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf), "T%02xthread:%s;",
> +             GDB_SIGNAL_TRAP, thread_id);

again this would be an argument for using GString to build-up our reply packets.

> +cleanup:
> +    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
> +}
> +
> +static void handle_v_kill(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    /* Kill the target */
> +    put_packet(gdb_ctx->s, "OK");
> +    error_report("QEMU: Terminated via GDBstub");
> +    exit(0);
> +}
> +
> +static GdbCmdParseEntry gdb_v_commands_table[] = {
> +    /* Order is important if has same prefix */
> +    {
> +        .handler = handle_v_cont_query,
> +        .cmd = "Cont?",
> +        .cmd_startswith = 1
> +    },
> +    {
> +        .handler = handle_v_cont,
> +        .cmd = "Cont",
> +        .cmd_startswith = 1,
> +        .schema = "s0"
> +    },
> +    {
> +        .handler = handle_v_attach,
> +        .cmd = "Attach;",
> +        .cmd_startswith = 1,
> +        .schema = "l0"
> +    },
> +    {
> +        .handler = handle_v_kill,
> +        .cmd = "Kill;",
> +        .cmd_startswith = 1
> +    },
> +};
> +
> +static void handle_v_commands(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    if (!gdb_ctx->num_params) {
> +        return;
> +    }
> +
> +    if (process_string_cmd(gdb_ctx->s, NULL, gdb_ctx->params[0].data,
> +                           gdb_v_commands_table,
> +                           ARRAY_SIZE(gdb_v_commands_table))) {
> +        put_packet(gdb_ctx->s, "");
> +    }
> +}
> +
>  static int gdb_handle_packet(GDBState *s, const char *line_buf)
>  {
>      CPUState *cpu;
> @@ -1822,7 +1922,7 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>      CPUClass *cc;
>      const char *p;
>      uint32_t pid, tid;
> -    int ch, type, res;
> +    int ch, type;
>      uint8_t mem_buf[MAX_PACKET_LENGTH];
>      char buf[sizeof(mem_buf) + 1 /* trailing NUL */];
>      char thread_id[16];
> @@ -1871,66 +1971,16 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>          }
>          break;
>      case 'v':
> -        if (strncmp(p, "Cont", 4) == 0) {
> -            p += 4;
> -            if (*p == '?') {
> -                put_packet(s, "vCont;c;C;s;S");
> -                break;
> -            }
> -
> -            res = gdb_handle_vcont(s, p);
> -
> -            if (res) {
> -                if ((res == -EINVAL) || (res == -ERANGE)) {
> -                    put_packet(s, "E22");
> -                    break;
> -                }
> -                goto unknown_command;
> -            }
> -            break;
> -        } else if (strncmp(p, "Attach;", 7) == 0) {
> -            unsigned long pid;
> -
> -            p += 7;
> -
> -            if (qemu_strtoul(p, &p, 16, &pid)) {
> -                put_packet(s, "E22");
> -                break;
> -            }
> -
> -            process = gdb_get_process(s, pid);
> -
> -            if (process == NULL) {
> -                put_packet(s, "E22");
> -                break;
> -            }
> -
> -            cpu = get_first_cpu_in_process(s, process);
> -
> -            if (cpu == NULL) {
> -                /* Refuse to attach an empty process */
> -                put_packet(s, "E22");
> -                break;
> -            }
> -
> -            process->attached = true;
> -
> -            s->g_cpu = cpu;
> -            s->c_cpu = cpu;
> -
> -            snprintf(buf, sizeof(buf), "T%02xthread:%s;", GDB_SIGNAL_TRAP,
> -                     gdb_fmt_thread_id(s, cpu, thread_id, sizeof(thread_id)));
> -
> -            put_packet(s, buf);
> -            break;
> -        } else if (strncmp(p, "Kill;", 5) == 0) {
> -            /* Kill the target */
> -            put_packet(s, "OK");
> -            error_report("QEMU: Terminated via GDBstub");
> -            exit(0);
> -        } else {
> -            goto unknown_command;
> +        {
> +            static const GdbCmdParseEntry v_cmd_desc = {
> +                .handler = handle_v_commands,
> +                .cmd = "v",
> +                .cmd_startswith = 1,
> +                .schema = "s0"
> +            };
> +            cmd_parser = &v_cmd_desc;
>          }
> +        break;
>      case 'k':
>          /* Kill the target */
>          error_report("QEMU: Terminated via GDBstub");


--
Alex Bennée
Jon Doron May 20, 2019, 4:38 a.m. UTC | #2 
On Wed, May 15, 2019 at 8:06 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Jon Doron <arilou@gmail.com> writes:
>
> > Signed-off-by: Jon Doron <arilou@gmail.com>
> > ---
> >  gdbstub.c | 170 +++++++++++++++++++++++++++++++++++-------------------
> >  1 file changed, 110 insertions(+), 60 deletions(-)
> >
> > diff --git a/gdbstub.c b/gdbstub.c
> > index 9b0556f8be..d56d0fd235 100644
> > --- a/gdbstub.c
> > +++ b/gdbstub.c
> > @@ -1815,6 +1815,106 @@ static void handle_step(GdbCmdContext *gdb_ctx, void *user_ctx)
> >      gdb_continue(gdb_ctx->s);
> >  }
> >
> > +static void handle_v_cont_query(GdbCmdContext *gdb_ctx, void *user_ctx)
> > +{
> > +    put_packet(gdb_ctx->s, "vCont;c;C;s;S");
> > +}
> > +
> > +static void handle_v_cont(GdbCmdContext *gdb_ctx, void *user_ctx)
> > +{
> > +    int res;
> > +
> > +    if (!gdb_ctx->num_params) {
> > +        return;
> > +    }
> > +
> > +    res = gdb_handle_vcont(gdb_ctx->s, gdb_ctx->params[0].data);
> > +    if ((res == -EINVAL) || (res == -ERANGE)) {
> > +        put_packet(gdb_ctx->s, "E22");
> > +    } else if (res) {
> > +        put_packet(gdb_ctx->s, "\0");
>
> Isn't this just ""?
>
> Either way my reading of the spec say the response needs to be a "Stop
> Reply Packet" which I don't think includes empty or E codes.
>

From my understanding reading the spec and the gdbserver
implementation in binutils a null packet tells the client
the command is unsupported, so it makes sense to reply with this null
packet if handle_vcont replied with something
we dont know (i.e -ENOTSUP)

> > +    }
> > +}
> > +
> > +static void handle_v_attach(GdbCmdContext *gdb_ctx, void *user_ctx)
> > +{
> > +    GDBProcess *process;
> > +    CPUState *cpu;
> > +    char thread_id[16];
> > +
> > +    strcpy(gdb_ctx->str_buf, "E22");
>
> pstrcpy (see HACKING about strncpy) but...
>
> > +    if (!gdb_ctx->num_params) {
> > +        goto cleanup;
> > +    }
> > +
> > +    process = gdb_get_process(gdb_ctx->s, gdb_ctx->params[0].val_ul);
> > +    if (!process) {
> > +        goto cleanup;
> > +    }
> > +
> > +    cpu = get_first_cpu_in_process(gdb_ctx->s, process);
> > +    if (!cpu) {
> > +        goto cleanup;
> > +    }
> > +
> > +    process->attached = true;
> > +    gdb_ctx->s->g_cpu = cpu;
> > +    gdb_ctx->s->c_cpu = cpu;
> > +
> > +    gdb_fmt_thread_id(gdb_ctx->s, cpu, thread_id, sizeof(thread_id));
> > +    snprintf(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf), "T%02xthread:%s;",
> > +             GDB_SIGNAL_TRAP, thread_id);
>
> again this would be an argument for using GString to build-up our reply packets.
>
Perhaps we will need to make another patchset which fixes all the
strings/buffers stuff and move to Glib
but like you said probably too much for this patchset
> > +cleanup:
> > +    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
> > +}
> > +
> > +static void handle_v_kill(GdbCmdContext *gdb_ctx, void *user_ctx)
> > +{
> > +    /* Kill the target */
> > +    put_packet(gdb_ctx->s, "OK");
> > +    error_report("QEMU: Terminated via GDBstub");
> > +    exit(0);
> > +}
> > +
> > +static GdbCmdParseEntry gdb_v_commands_table[] = {
> > +    /* Order is important if has same prefix */
> > +    {
> > +        .handler = handle_v_cont_query,
> > +        .cmd = "Cont?",
> > +        .cmd_startswith = 1
> > +    },
> > +    {
> > +        .handler = handle_v_cont,
> > +        .cmd = "Cont",
> > +        .cmd_startswith = 1,
> > +        .schema = "s0"
> > +    },
> > +    {
> > +        .handler = handle_v_attach,
> > +        .cmd = "Attach;",
> > +        .cmd_startswith = 1,
> > +        .schema = "l0"
> > +    },
> > +    {
> > +        .handler = handle_v_kill,
> > +        .cmd = "Kill;",
> > +        .cmd_startswith = 1
> > +    },
> > +};
> > +
> > +static void handle_v_commands(GdbCmdContext *gdb_ctx, void *user_ctx)
> > +{
> > +    if (!gdb_ctx->num_params) {
> > +        return;
> > +    }
> > +
> > +    if (process_string_cmd(gdb_ctx->s, NULL, gdb_ctx->params[0].data,
> > +                           gdb_v_commands_table,
> > +                           ARRAY_SIZE(gdb_v_commands_table))) {
> > +        put_packet(gdb_ctx->s, "");
> > +    }
> > +}
> > +
> >  static int gdb_handle_packet(GDBState *s, const char *line_buf)
> >  {
> >      CPUState *cpu;
> > @@ -1822,7 +1922,7 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
> >      CPUClass *cc;
> >      const char *p;
> >      uint32_t pid, tid;
> > -    int ch, type, res;
> > +    int ch, type;
> >      uint8_t mem_buf[MAX_PACKET_LENGTH];
> >      char buf[sizeof(mem_buf) + 1 /* trailing NUL */];
> >      char thread_id[16];
> > @@ -1871,66 +1971,16 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
> >          }
> >          break;
> >      case 'v':
> > -        if (strncmp(p, "Cont", 4) == 0) {
> > -            p += 4;
> > -            if (*p == '?') {
> > -                put_packet(s, "vCont;c;C;s;S");
> > -                break;
> > -            }
> > -
> > -            res = gdb_handle_vcont(s, p);
> > -
> > -            if (res) {
> > -                if ((res == -EINVAL) || (res == -ERANGE)) {
> > -                    put_packet(s, "E22");
> > -                    break;
> > -                }
> > -                goto unknown_command;
> > -            }
> > -            break;
> > -        } else if (strncmp(p, "Attach;", 7) == 0) {
> > -            unsigned long pid;
> > -
> > -            p += 7;
> > -
> > -            if (qemu_strtoul(p, &p, 16, &pid)) {
> > -                put_packet(s, "E22");
> > -                break;
> > -            }
> > -
> > -            process = gdb_get_process(s, pid);
> > -
> > -            if (process == NULL) {
> > -                put_packet(s, "E22");
> > -                break;
> > -            }
> > -
> > -            cpu = get_first_cpu_in_process(s, process);
> > -
> > -            if (cpu == NULL) {
> > -                /* Refuse to attach an empty process */
> > -                put_packet(s, "E22");
> > -                break;
> > -            }
> > -
> > -            process->attached = true;
> > -
> > -            s->g_cpu = cpu;
> > -            s->c_cpu = cpu;
> > -
> > -            snprintf(buf, sizeof(buf), "T%02xthread:%s;", GDB_SIGNAL_TRAP,
> > -                     gdb_fmt_thread_id(s, cpu, thread_id, sizeof(thread_id)));
> > -
> > -            put_packet(s, buf);
> > -            break;
> > -        } else if (strncmp(p, "Kill;", 5) == 0) {
> > -            /* Kill the target */
> > -            put_packet(s, "OK");
> > -            error_report("QEMU: Terminated via GDBstub");
> > -            exit(0);
> > -        } else {
> > -            goto unknown_command;
> > +        {
> > +            static const GdbCmdParseEntry v_cmd_desc = {
> > +                .handler = handle_v_commands,
> > +                .cmd = "v",
> > +                .cmd_startswith = 1,
> > +                .schema = "s0"
> > +            };
> > +            cmd_parser = &v_cmd_desc;
> >          }
> > +        break;
> >      case 'k':
> >          /* Kill the target */
> >          error_report("QEMU: Terminated via GDBstub");
>
>
> --
> Alex Bennée
diff mbox series

Patch

diff --git a/gdbstub.c b/gdbstub.c
index 9b0556f8be..d56d0fd235 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1815,6 +1815,106 @@  static void handle_step(GdbCmdContext *gdb_ctx, void *user_ctx)
     gdb_continue(gdb_ctx->s);
 }
 
+static void handle_v_cont_query(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    put_packet(gdb_ctx->s, "vCont;c;C;s;S");
+}
+
+static void handle_v_cont(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    int res;
+
+    if (!gdb_ctx->num_params) {
+        return;
+    }
+
+    res = gdb_handle_vcont(gdb_ctx->s, gdb_ctx->params[0].data);
+    if ((res == -EINVAL) || (res == -ERANGE)) {
+        put_packet(gdb_ctx->s, "E22");
+    } else if (res) {
+        put_packet(gdb_ctx->s, "\0");
+    }
+}
+
+static void handle_v_attach(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    GDBProcess *process;
+    CPUState *cpu;
+    char thread_id[16];
+
+    strcpy(gdb_ctx->str_buf, "E22");
+    if (!gdb_ctx->num_params) {
+        goto cleanup;
+    }
+
+    process = gdb_get_process(gdb_ctx->s, gdb_ctx->params[0].val_ul);
+    if (!process) {
+        goto cleanup;
+    }
+
+    cpu = get_first_cpu_in_process(gdb_ctx->s, process);
+    if (!cpu) {
+        goto cleanup;
+    }
+
+    process->attached = true;
+    gdb_ctx->s->g_cpu = cpu;
+    gdb_ctx->s->c_cpu = cpu;
+
+    gdb_fmt_thread_id(gdb_ctx->s, cpu, thread_id, sizeof(thread_id));
+    snprintf(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf), "T%02xthread:%s;",
+             GDB_SIGNAL_TRAP, thread_id);
+cleanup:
+    put_packet(gdb_ctx->s, gdb_ctx->str_buf);
+}
+
+static void handle_v_kill(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    /* Kill the target */
+    put_packet(gdb_ctx->s, "OK");
+    error_report("QEMU: Terminated via GDBstub");
+    exit(0);
+}
+
+static GdbCmdParseEntry gdb_v_commands_table[] = {
+    /* Order is important if has same prefix */
+    {
+        .handler = handle_v_cont_query,
+        .cmd = "Cont?",
+        .cmd_startswith = 1
+    },
+    {
+        .handler = handle_v_cont,
+        .cmd = "Cont",
+        .cmd_startswith = 1,
+        .schema = "s0"
+    },
+    {
+        .handler = handle_v_attach,
+        .cmd = "Attach;",
+        .cmd_startswith = 1,
+        .schema = "l0"
+    },
+    {
+        .handler = handle_v_kill,
+        .cmd = "Kill;",
+        .cmd_startswith = 1
+    },
+};
+
+static void handle_v_commands(GdbCmdContext *gdb_ctx, void *user_ctx)
+{
+    if (!gdb_ctx->num_params) {
+        return;
+    }
+
+    if (process_string_cmd(gdb_ctx->s, NULL, gdb_ctx->params[0].data,
+                           gdb_v_commands_table,
+                           ARRAY_SIZE(gdb_v_commands_table))) {
+        put_packet(gdb_ctx->s, "");
+    }
+}
+
 static int gdb_handle_packet(GDBState *s, const char *line_buf)
 {
     CPUState *cpu;
@@ -1822,7 +1922,7 @@  static int gdb_handle_packet(GDBState *s, const char *line_buf)
     CPUClass *cc;
     const char *p;
     uint32_t pid, tid;
-    int ch, type, res;
+    int ch, type;
     uint8_t mem_buf[MAX_PACKET_LENGTH];
     char buf[sizeof(mem_buf) + 1 /* trailing NUL */];
     char thread_id[16];
@@ -1871,66 +1971,16 @@  static int gdb_handle_packet(GDBState *s, const char *line_buf)
         }
         break;
     case 'v':
-        if (strncmp(p, "Cont", 4) == 0) {
-            p += 4;
-            if (*p == '?') {
-                put_packet(s, "vCont;c;C;s;S");
-                break;
-            }
-
-            res = gdb_handle_vcont(s, p);
-
-            if (res) {
-                if ((res == -EINVAL) || (res == -ERANGE)) {
-                    put_packet(s, "E22");
-                    break;
-                }
-                goto unknown_command;
-            }
-            break;
-        } else if (strncmp(p, "Attach;", 7) == 0) {
-            unsigned long pid;
-
-            p += 7;
-
-            if (qemu_strtoul(p, &p, 16, &pid)) {
-                put_packet(s, "E22");
-                break;
-            }
-
-            process = gdb_get_process(s, pid);
-
-            if (process == NULL) {
-                put_packet(s, "E22");
-                break;
-            }
-
-            cpu = get_first_cpu_in_process(s, process);
-
-            if (cpu == NULL) {
-                /* Refuse to attach an empty process */
-                put_packet(s, "E22");
-                break;
-            }
-
-            process->attached = true;
-
-            s->g_cpu = cpu;
-            s->c_cpu = cpu;
-
-            snprintf(buf, sizeof(buf), "T%02xthread:%s;", GDB_SIGNAL_TRAP,
-                     gdb_fmt_thread_id(s, cpu, thread_id, sizeof(thread_id)));
-
-            put_packet(s, buf);
-            break;
-        } else if (strncmp(p, "Kill;", 5) == 0) {
-            /* Kill the target */
-            put_packet(s, "OK");
-            error_report("QEMU: Terminated via GDBstub");
-            exit(0);
-        } else {
-            goto unknown_command;
+        {
+            static const GdbCmdParseEntry v_cmd_desc = {
+                .handler = handle_v_commands,
+                .cmd = "v",
+                .cmd_startswith = 1,
+                .schema = "s0"
+            };
+            cmd_parser = &v_cmd_desc;
         }
+        break;
     case 'k':
         /* Kill the target */
         error_report("QEMU: Terminated via GDBstub");