Re: [Qemu-devel] [PATCH v9 17/27] gdbstub: Implement v commands with new infra

[Alex Bennée]

Jon Doron <address@hidden> writes:

> Signed-off-by: Jon Doron <address@hidden>
> ---
>  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