Message ID | 20190502081554.5521-3-arilou@gmail.com |
---|---|
State | New |
Headers | show |
Series | gdbstub: Refactor command packets handler | expand |
Jon Doron <arilou@gmail.com> writes: > Signed-off-by: Jon Doron <arilou@gmail.com> > --- > gdbstub.c | 90 ++++++++++++++++++++++++++++++------------------------- > 1 file changed, 50 insertions(+), 40 deletions(-) > > diff --git a/gdbstub.c b/gdbstub.c > index d5e0f3878a..621d689868 100644 > --- a/gdbstub.c > +++ b/gdbstub.c > @@ -1418,11 +1418,6 @@ static inline int startswith(const char *string, const char *pattern) > return !strncmp(string, pattern, strlen(pattern)); > } > > -static int process_string_cmd( > - GDBState *s, void *user_ctx, const char *data, > - const GdbCmdParseEntry *cmds, int num_cmds) > - __attribute__((unused)); > - > static int process_string_cmd(GDBState *s, void *user_ctx, const char *data, > const GdbCmdParseEntry *cmds, int num_cmds) > { > @@ -1468,6 +1463,41 @@ static int process_string_cmd(GDBState *s, void *user_ctx, const char *data, > return -1; > } > > +static void handle_detach(GdbCmdContext *gdb_ctx, void *user_ctx) > +{ > + GDBProcess *process; > + GDBState *s = gdb_ctx->s; > + uint32_t pid = 1; > + > + if (s->multiprocess) { > + if (!gdb_ctx->num_params) { > + put_packet(s, "E22"); > + return; > + } > + > + pid = gdb_ctx->params[0].val_ul; > + } > + > + process = gdb_get_process(s, pid); > + gdb_process_breakpoint_remove_all(s, process); > + process->attached = false; > + > + if (pid == gdb_get_cpu_pid(s, s->c_cpu)) { > + s->c_cpu = gdb_first_attached_cpu(s); > + } > + > + if (pid == gdb_get_cpu_pid(s, s->g_cpu)) { > + s->g_cpu = gdb_first_attached_cpu(s); > + } > + > + if (!s->c_cpu) { > + /* No more process attached */ > + gdb_syscall_mode = GDB_SYS_DISABLED; > + gdb_continue(s); > + } > + put_packet(s, "OK"); > +} > + > static int gdb_handle_packet(GDBState *s, const char *line_buf) > { > CPUState *cpu; > @@ -1482,6 +1512,7 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) > uint8_t *registers; > target_ulong addr, len; > GDBThreadIdKind thread_kind; > + const GdbCmdParseEntry *cmd_parser = NULL; > > trace_gdbstub_io_command(line_buf); > > @@ -1582,42 +1613,15 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) > error_report("QEMU: Terminated via GDBstub"); > exit(0); > case 'D': > - /* Detach packet */ > - pid = 1; > - > - if (s->multiprocess) { > - unsigned long lpid; > - if (*p != ';') { > - put_packet(s, "E22"); > - break; > - } > - > - if (qemu_strtoul(p + 1, &p, 16, &lpid)) { > - put_packet(s, "E22"); > - break; > - } > - > - pid = lpid; > - } > - > - process = gdb_get_process(s, pid); > - gdb_process_breakpoint_remove_all(s, process); > - process->attached = false; > - > - if (pid == gdb_get_cpu_pid(s, s->c_cpu)) { > - s->c_cpu = gdb_first_attached_cpu(s); > - } > - > - if (pid == gdb_get_cpu_pid(s, s->g_cpu)) { > - s->g_cpu = gdb_first_attached_cpu(s); > - } > - > - if (s->c_cpu == NULL) { > - /* No more process attached */ > - gdb_syscall_mode = GDB_SYS_DISABLED; > - gdb_continue(s); > + { > + static const GdbCmdParseEntry detach_cmd_desc = { > + .handler = handle_detach, > + .cmd = "D", > + .cmd_startswith = 1, > + .schema = "?.l0" > + }; > + cmd_parser = &detach_cmd_desc; > } > - put_packet(s, "OK"); > break; > case 's': > if (*p != '\0') { > @@ -1990,6 +1994,12 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) > put_packet(s, buf); > break; > } > + > + if (cmd_parser && > + process_string_cmd(s, NULL, line_buf, cmd_parser, 1)) { > + put_packet(s, ""); Why this null put_packet at the end? You've passed the handling of the OK reply back to your handler so this seems superfluous. -- Alex Bennée
Hi Alex, I implemented this change but i'm having second guesses on this, basically a NULL packet means the command is not supported (as far as i understand from the protocol documentation and implementation of GDB) That being said I think it's correct to send back a NULL packet if process_string_cmd fails for any reason, or you would prefer ill just omit it? Snippet of the change I did according to your review: - if (cmd_parser && - process_string_cmd(s, NULL, line_buf, cmd_parser, 1)) { - put_packet(s, ""); + if (!cmd_parser) { + return RS_IDLE; } + process_string_cmd(s, NULL, line_buf, cmd_parser, 1); -- Jon. On Tue, May 14, 2019 at 9:54 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 | 90 ++++++++++++++++++++++++++++++------------------------- > > 1 file changed, 50 insertions(+), 40 deletions(-) > > > > diff --git a/gdbstub.c b/gdbstub.c > > index d5e0f3878a..621d689868 100644 > > --- a/gdbstub.c > > +++ b/gdbstub.c > > @@ -1418,11 +1418,6 @@ static inline int startswith(const char *string, const char *pattern) > > return !strncmp(string, pattern, strlen(pattern)); > > } > > > > -static int process_string_cmd( > > - GDBState *s, void *user_ctx, const char *data, > > - const GdbCmdParseEntry *cmds, int num_cmds) > > - __attribute__((unused)); > > - > > static int process_string_cmd(GDBState *s, void *user_ctx, const char *data, > > const GdbCmdParseEntry *cmds, int num_cmds) > > { > > @@ -1468,6 +1463,41 @@ static int process_string_cmd(GDBState *s, void *user_ctx, const char *data, > > return -1; > > } > > > > +static void handle_detach(GdbCmdContext *gdb_ctx, void *user_ctx) > > +{ > > + GDBProcess *process; > > + GDBState *s = gdb_ctx->s; > > + uint32_t pid = 1; > > + > > + if (s->multiprocess) { > > + if (!gdb_ctx->num_params) { > > + put_packet(s, "E22"); > > + return; > > + } > > + > > + pid = gdb_ctx->params[0].val_ul; > > + } > > + > > + process = gdb_get_process(s, pid); > > + gdb_process_breakpoint_remove_all(s, process); > > + process->attached = false; > > + > > + if (pid == gdb_get_cpu_pid(s, s->c_cpu)) { > > + s->c_cpu = gdb_first_attached_cpu(s); > > + } > > + > > + if (pid == gdb_get_cpu_pid(s, s->g_cpu)) { > > + s->g_cpu = gdb_first_attached_cpu(s); > > + } > > + > > + if (!s->c_cpu) { > > + /* No more process attached */ > > + gdb_syscall_mode = GDB_SYS_DISABLED; > > + gdb_continue(s); > > + } > > + put_packet(s, "OK"); > > +} > > + > > static int gdb_handle_packet(GDBState *s, const char *line_buf) > > { > > CPUState *cpu; > > @@ -1482,6 +1512,7 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) > > uint8_t *registers; > > target_ulong addr, len; > > GDBThreadIdKind thread_kind; > > + const GdbCmdParseEntry *cmd_parser = NULL; > > > > trace_gdbstub_io_command(line_buf); > > > > @@ -1582,42 +1613,15 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) > > error_report("QEMU: Terminated via GDBstub"); > > exit(0); > > case 'D': > > - /* Detach packet */ > > - pid = 1; > > - > > - if (s->multiprocess) { > > - unsigned long lpid; > > - if (*p != ';') { > > - put_packet(s, "E22"); > > - break; > > - } > > - > > - if (qemu_strtoul(p + 1, &p, 16, &lpid)) { > > - put_packet(s, "E22"); > > - break; > > - } > > - > > - pid = lpid; > > - } > > - > > - process = gdb_get_process(s, pid); > > - gdb_process_breakpoint_remove_all(s, process); > > - process->attached = false; > > - > > - if (pid == gdb_get_cpu_pid(s, s->c_cpu)) { > > - s->c_cpu = gdb_first_attached_cpu(s); > > - } > > - > > - if (pid == gdb_get_cpu_pid(s, s->g_cpu)) { > > - s->g_cpu = gdb_first_attached_cpu(s); > > - } > > - > > - if (s->c_cpu == NULL) { > > - /* No more process attached */ > > - gdb_syscall_mode = GDB_SYS_DISABLED; > > - gdb_continue(s); > > + { > > + static const GdbCmdParseEntry detach_cmd_desc = { > > + .handler = handle_detach, > > + .cmd = "D", > > + .cmd_startswith = 1, > > + .schema = "?.l0" > > + }; > > + cmd_parser = &detach_cmd_desc; > > } > > - put_packet(s, "OK"); > > break; > > case 's': > > if (*p != '\0') { > > @@ -1990,6 +1994,12 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) > > put_packet(s, buf); > > break; > > } > > + > > + if (cmd_parser && > > + process_string_cmd(s, NULL, line_buf, cmd_parser, 1)) { > > + put_packet(s, ""); > > Why this null put_packet at the end? You've passed the handling of the > OK reply back to your handler so this seems superfluous. > > -- > Alex Bennée
Jon Doron <arilou@gmail.com> writes: > Hi Alex, I implemented this change but i'm having second guesses on > this, basically a NULL packet means the command is not supported (as > far as i understand from the protocol documentation and implementation > of GDB) > That being said I think it's correct to send back a NULL packet if > process_string_cmd fails for any reason, or you would prefer ill just > omit it? > > Snippet of the change I did according to your review: > - if (cmd_parser && > - process_string_cmd(s, NULL, line_buf, cmd_parser, 1)) { > - put_packet(s, ""); > + if (!cmd_parser) { > + return RS_IDLE; > } > > + process_string_cmd(s, NULL, line_buf, cmd_parser, 1); OK I see your reasoning. So perhaps: if (cmd_parser) { /* helper will respond */ process_string_cmd(s, NULL, line_buf, cmd_parser, 1) } else { /* unknown command, empty response */ put_packet(s, ""); } return RS_IDLE; > > -- Jon. > > On Tue, May 14, 2019 at 9:54 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 | 90 ++++++++++++++++++++++++++++++------------------------- >> > 1 file changed, 50 insertions(+), 40 deletions(-) >> > >> > diff --git a/gdbstub.c b/gdbstub.c >> > index d5e0f3878a..621d689868 100644 >> > --- a/gdbstub.c >> > +++ b/gdbstub.c >> > @@ -1418,11 +1418,6 @@ static inline int startswith(const char *string, const char *pattern) >> > return !strncmp(string, pattern, strlen(pattern)); >> > } >> > >> > -static int process_string_cmd( >> > - GDBState *s, void *user_ctx, const char *data, >> > - const GdbCmdParseEntry *cmds, int num_cmds) >> > - __attribute__((unused)); >> > - >> > static int process_string_cmd(GDBState *s, void *user_ctx, const char *data, >> > const GdbCmdParseEntry *cmds, int num_cmds) >> > { >> > @@ -1468,6 +1463,41 @@ static int process_string_cmd(GDBState *s, void *user_ctx, const char *data, >> > return -1; >> > } >> > >> > +static void handle_detach(GdbCmdContext *gdb_ctx, void *user_ctx) >> > +{ >> > + GDBProcess *process; >> > + GDBState *s = gdb_ctx->s; >> > + uint32_t pid = 1; >> > + >> > + if (s->multiprocess) { >> > + if (!gdb_ctx->num_params) { >> > + put_packet(s, "E22"); >> > + return; >> > + } >> > + >> > + pid = gdb_ctx->params[0].val_ul; >> > + } >> > + >> > + process = gdb_get_process(s, pid); >> > + gdb_process_breakpoint_remove_all(s, process); >> > + process->attached = false; >> > + >> > + if (pid == gdb_get_cpu_pid(s, s->c_cpu)) { >> > + s->c_cpu = gdb_first_attached_cpu(s); >> > + } >> > + >> > + if (pid == gdb_get_cpu_pid(s, s->g_cpu)) { >> > + s->g_cpu = gdb_first_attached_cpu(s); >> > + } >> > + >> > + if (!s->c_cpu) { >> > + /* No more process attached */ >> > + gdb_syscall_mode = GDB_SYS_DISABLED; >> > + gdb_continue(s); >> > + } >> > + put_packet(s, "OK"); >> > +} >> > + >> > static int gdb_handle_packet(GDBState *s, const char *line_buf) >> > { >> > CPUState *cpu; >> > @@ -1482,6 +1512,7 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) >> > uint8_t *registers; >> > target_ulong addr, len; >> > GDBThreadIdKind thread_kind; >> > + const GdbCmdParseEntry *cmd_parser = NULL; >> > >> > trace_gdbstub_io_command(line_buf); >> > >> > @@ -1582,42 +1613,15 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) >> > error_report("QEMU: Terminated via GDBstub"); >> > exit(0); >> > case 'D': >> > - /* Detach packet */ >> > - pid = 1; >> > - >> > - if (s->multiprocess) { >> > - unsigned long lpid; >> > - if (*p != ';') { >> > - put_packet(s, "E22"); >> > - break; >> > - } >> > - >> > - if (qemu_strtoul(p + 1, &p, 16, &lpid)) { >> > - put_packet(s, "E22"); >> > - break; >> > - } >> > - >> > - pid = lpid; >> > - } >> > - >> > - process = gdb_get_process(s, pid); >> > - gdb_process_breakpoint_remove_all(s, process); >> > - process->attached = false; >> > - >> > - if (pid == gdb_get_cpu_pid(s, s->c_cpu)) { >> > - s->c_cpu = gdb_first_attached_cpu(s); >> > - } >> > - >> > - if (pid == gdb_get_cpu_pid(s, s->g_cpu)) { >> > - s->g_cpu = gdb_first_attached_cpu(s); >> > - } >> > - >> > - if (s->c_cpu == NULL) { >> > - /* No more process attached */ >> > - gdb_syscall_mode = GDB_SYS_DISABLED; >> > - gdb_continue(s); >> > + { >> > + static const GdbCmdParseEntry detach_cmd_desc = { >> > + .handler = handle_detach, >> > + .cmd = "D", >> > + .cmd_startswith = 1, >> > + .schema = "?.l0" >> > + }; >> > + cmd_parser = &detach_cmd_desc; >> > } >> > - put_packet(s, "OK"); >> > break; >> > case 's': >> > if (*p != '\0') { >> > @@ -1990,6 +1994,12 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) >> > put_packet(s, buf); >> > break; >> > } >> > + >> > + if (cmd_parser && >> > + process_string_cmd(s, NULL, line_buf, cmd_parser, 1)) { >> > + put_packet(s, ""); >> >> Why this null put_packet at the end? You've passed the handling of the >> OK reply back to your handler so this seems superfluous. >> >> -- >> Alex Bennée -- Alex Bennée
diff --git a/gdbstub.c b/gdbstub.c index d5e0f3878a..621d689868 100644 --- a/gdbstub.c +++ b/gdbstub.c @@ -1418,11 +1418,6 @@ static inline int startswith(const char *string, const char *pattern) return !strncmp(string, pattern, strlen(pattern)); } -static int process_string_cmd( - GDBState *s, void *user_ctx, const char *data, - const GdbCmdParseEntry *cmds, int num_cmds) - __attribute__((unused)); - static int process_string_cmd(GDBState *s, void *user_ctx, const char *data, const GdbCmdParseEntry *cmds, int num_cmds) { @@ -1468,6 +1463,41 @@ static int process_string_cmd(GDBState *s, void *user_ctx, const char *data, return -1; } +static void handle_detach(GdbCmdContext *gdb_ctx, void *user_ctx) +{ + GDBProcess *process; + GDBState *s = gdb_ctx->s; + uint32_t pid = 1; + + if (s->multiprocess) { + if (!gdb_ctx->num_params) { + put_packet(s, "E22"); + return; + } + + pid = gdb_ctx->params[0].val_ul; + } + + process = gdb_get_process(s, pid); + gdb_process_breakpoint_remove_all(s, process); + process->attached = false; + + if (pid == gdb_get_cpu_pid(s, s->c_cpu)) { + s->c_cpu = gdb_first_attached_cpu(s); + } + + if (pid == gdb_get_cpu_pid(s, s->g_cpu)) { + s->g_cpu = gdb_first_attached_cpu(s); + } + + if (!s->c_cpu) { + /* No more process attached */ + gdb_syscall_mode = GDB_SYS_DISABLED; + gdb_continue(s); + } + put_packet(s, "OK"); +} + static int gdb_handle_packet(GDBState *s, const char *line_buf) { CPUState *cpu; @@ -1482,6 +1512,7 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) uint8_t *registers; target_ulong addr, len; GDBThreadIdKind thread_kind; + const GdbCmdParseEntry *cmd_parser = NULL; trace_gdbstub_io_command(line_buf); @@ -1582,42 +1613,15 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) error_report("QEMU: Terminated via GDBstub"); exit(0); case 'D': - /* Detach packet */ - pid = 1; - - if (s->multiprocess) { - unsigned long lpid; - if (*p != ';') { - put_packet(s, "E22"); - break; - } - - if (qemu_strtoul(p + 1, &p, 16, &lpid)) { - put_packet(s, "E22"); - break; - } - - pid = lpid; - } - - process = gdb_get_process(s, pid); - gdb_process_breakpoint_remove_all(s, process); - process->attached = false; - - if (pid == gdb_get_cpu_pid(s, s->c_cpu)) { - s->c_cpu = gdb_first_attached_cpu(s); - } - - if (pid == gdb_get_cpu_pid(s, s->g_cpu)) { - s->g_cpu = gdb_first_attached_cpu(s); - } - - if (s->c_cpu == NULL) { - /* No more process attached */ - gdb_syscall_mode = GDB_SYS_DISABLED; - gdb_continue(s); + { + static const GdbCmdParseEntry detach_cmd_desc = { + .handler = handle_detach, + .cmd = "D", + .cmd_startswith = 1, + .schema = "?.l0" + }; + cmd_parser = &detach_cmd_desc; } - put_packet(s, "OK"); break; case 's': if (*p != '\0') { @@ -1990,6 +1994,12 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) put_packet(s, buf); break; } + + if (cmd_parser && + process_string_cmd(s, NULL, line_buf, cmd_parser, 1)) { + put_packet(s, ""); + } + return RS_IDLE; }
Signed-off-by: Jon Doron <arilou@gmail.com> --- gdbstub.c | 90 ++++++++++++++++++++++++++++++------------------------- 1 file changed, 50 insertions(+), 40 deletions(-)