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

[Jon Doron]

On Wed, May 15, 2019 at 8:06 PM Alex Bennée <address@hidden> wrote:
>
>
> 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.
>

>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