On Wed, May 15, 2019 at 8:06 PM Alex Bennée <alex.ben...@linaro.org> wrote:
>
>
> Jon Doron <ari...@gmail.com> writes:
>
> > Signed-off-by: Jon Doron <ari...@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
