Re: [Qemu-devel] [PATCH v9 17/27] gdbstub: Implement v commands with new infra
On Wed, May 15, 2019 at 8:06 PM Alex Bennée wrote:
>
>
> Jon Doron writes:
>
> > Signed-off-by: Jon Doron
> > ---
> > 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);
> > -
> > -
Re: [Qemu-devel] [PATCH v9 17/27] gdbstub: Implement v commands with new infra
Jon Doron writes:
> Signed-off-by: Jon Doron
> ---
> 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, , 16, )) {
> -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 ==
