Gustavo Romero <gustavo.rom...@linaro.org> writes: > Currently, it's not possible to have stubs specific to a given target, > even though there are GDB features which are target-specific, like, for > instance, memory tagging. > > This commit introduces set_query_supported_arch, > set_gdb_gen_query_table_arch, and set_gdb_gen_set_table_arch functions > as interfaces to extend the qSupported string, the query handler table, > and set handler table per target, so allowing target-specific stub > implementation.
Subsystem functions exposed to the wider QEMU should be prefixed by the subsystem name (e.g. gdb_*). > Besides that, it moves GdbCmdParseEntry struct, its related types, and > gdb_put_packet to include/exec/gdbstub.h so they are also available in > the target-specific stubs. Generally I'm trying to reduce the amount of stuff that gets dumped in here because it is included widely and if your not careful you start winding yourself into knots. Could we put the command specific stuff into include/gdbstub/commands.h and only include it when needed? In general I think we could make the series a little more granular: - move GdbCmdParseEntry into headers - clean-up process_string_cmd - introduce new gdb_ APIs That will reduce the size of the individual patches and make review a bit easier. > > Signed-off-by: Gustavo Romero <gustavo.rom...@linaro.org> > --- > gdbstub/gdbstub.c | 108 +++++++++++++++++++++++------------------ > gdbstub/internals.h | 22 --------- > gdbstub/syscalls.c | 1 + > include/exec/gdbstub.h | 86 +++++++++++++++++++++++++++++++- > 4 files changed, 147 insertions(+), 70 deletions(-) > > diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c > index b3574997ea..4996530fde 100644 > --- a/gdbstub/gdbstub.c > +++ b/gdbstub/gdbstub.c > @@ -920,43 +920,6 @@ static int cmd_parse_params(const char *data, const char > *schema, > return 0; > } > > -typedef void (*GdbCmdHandler)(GArray *params, void *user_ctx); > - > -/* > - * cmd_startswith -> cmd is compared using startswith > - * > - * allow_stop_reply -> true iff the gdbstub can respond to this command with > a > - * "stop reply" packet. The list of commands that accept such response is > - * defined at the GDB Remote Serial Protocol documentation. see: > - * > https://sourceware.org/gdb/onlinedocs/gdb/Stop-Reply-Packets.html#Stop-Reply-Packets. > - * > - * schema definitions: > - * Each schema parameter entry consists of 2 chars, > - * the first char represents the parameter type handling > - * the second char represents the delimiter for the next parameter > - * > - * Currently supported schema types: > - * 'l' -> unsigned long (stored in .val_ul) > - * 'L' -> unsigned long long (stored in .val_ull) > - * 's' -> string (stored in .data) > - * 'o' -> single char (stored in .opcode) > - * 't' -> thread id (stored in .thread_id) > - * '?' -> skip according to delimiter > - * > - * Currently supported delimiters: > - * '?' -> Stop at any delimiter (",;:=\0") > - * '0' -> Stop at "\0" > - * '.' -> Skip 1 char unless reached "\0" > - * Any other value is treated as the delimiter value itself > - */ > -typedef struct GdbCmdParseEntry { > - GdbCmdHandler handler; > - const char *cmd; > - bool cmd_startswith; > - const char *schema; > - bool allow_stop_reply; > -} GdbCmdParseEntry; > - > static inline int startswith(const char *string, const char *pattern) > { > return !strncmp(string, pattern, strlen(pattern)); > @@ -1645,6 +1608,13 @@ static void handle_query_thread_extra(GArray *params, > void *user_ctx) > gdb_put_strbuf(); > } > > +/* Arch-specific qSupported */ > +char *query_supported_arch = NULL; This should be static. > +void set_query_supported_arch(char *query_supported) > +{ > + query_supported_arch = query_supported; > +} > + Maybe gdb_extended_qsupported_features? > static void handle_query_supported(GArray *params, void *user_ctx) > { > CPUClass *cc; > @@ -1684,6 +1654,11 @@ static void handle_query_supported(GArray *params, > void *user_ctx) > } > > g_string_append(gdbserver_state.str_buf, > ";vContSupported+;multiprocess+"); > + > + if (query_supported_arch) { > + g_string_append(gdbserver_state.str_buf, query_supported_arch); > + } > + > gdb_put_strbuf(); > } > > @@ -1765,6 +1740,16 @@ static const GdbCmdParseEntry > gdb_gen_query_set_common_table[] = { > }, > }; > > + > +/* Arch-specific query table */ > +static GdbCmdParseEntry *gdb_gen_query_table_arch = NULL ; > +static int gdb_gen_query_table_arch_size = 0; For local statics you don't need prefixes if this saves on variables getting too large. Also you don't need init to NULL/0 for statics. > +void set_gdb_gen_query_table_arch(GdbCmdParseEntry *table, int size) > +{ > + gdb_gen_query_table_arch = table; > + gdb_gen_query_table_arch_size = size; > +} > + > static const GdbCmdParseEntry gdb_gen_query_table[] = { > { > .handler = handle_query_curr_tid, > @@ -1857,6 +1842,15 @@ static const GdbCmdParseEntry gdb_gen_query_table[] = { > #endif > }; > > +/* Arch-specific set table */ > +static GdbCmdParseEntry *gdb_gen_set_table_arch = NULL; > +static int gdb_gen_set_table_arch_size = 0; > +void set_gdb_gen_set_table_arch(GdbCmdParseEntry *table, int size) > +{ > + gdb_gen_set_table_arch = table; > + gdb_gen_set_table_arch_size = size; > +} > + > static const GdbCmdParseEntry gdb_gen_set_table[] = { > /* Order is important if has same prefix */ > { > @@ -1889,17 +1883,27 @@ static void handle_gen_query(GArray *params, void > *user_ctx) > return; > } > > - if (!process_string_cmd(get_param(params, 0)->data, > - gdb_gen_query_set_common_table, > - ARRAY_SIZE(gdb_gen_query_set_common_table))) { > + if (process_string_cmd(get_param(params, 0)->data, > + gdb_gen_query_set_common_table, > + ARRAY_SIZE(gdb_gen_query_set_common_table)) == 0) > { > return; > } I think changing the process_string_cmd to return bool on success could be a pre-cursor patch. > > if (process_string_cmd(get_param(params, 0)->data, > gdb_gen_query_table, > - ARRAY_SIZE(gdb_gen_query_table))) { > - gdb_put_packet(""); > + ARRAY_SIZE(gdb_gen_query_table)) == 0) { > + return; > } > + > + if (gdb_gen_query_table_arch && > + process_string_cmd(get_param(params, 0)->data, > + gdb_gen_query_table_arch, > + gdb_gen_query_table_arch_size) == 0) { > + return; > + } > + > + /* Can't handle query, return Empty response. */ > + gdb_put_packet(""); > } > <snip> > diff --git a/gdbstub/syscalls.c b/gdbstub/syscalls.c > index 02e3a8f74c..ee9a16495e 100644 > --- a/gdbstub/syscalls.c > +++ b/gdbstub/syscalls.c > @@ -18,6 +18,7 @@ > #include "gdbstub/syscalls.h" > #include "trace.h" > #include "internals.h" > +#include "exec/gdbstub.h" This looks like a stray - it would be clearer once split out anyway. > > /* Syscall specific state */ > typedef struct { > diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h > index eb14b91139..7bf189d7f3 100644 > --- a/include/exec/gdbstub.h > +++ b/include/exec/gdbstub.h > @@ -144,4 +144,88 @@ void gdb_set_stop_cpu(CPUState *cpu); > /* in gdbstub-xml.c, generated by scripts/feature_to_c.py */ > extern const GDBFeature gdb_static_features[]; > > -#endif > +typedef void (*GdbCmdHandler)(GArray *params, void *user_ctx); > + > +typedef enum GDBThreadIdKind { > + GDB_ONE_THREAD = 0, > + GDB_ALL_THREADS, /* One process, all threads */ > + GDB_ALL_PROCESSES, > + GDB_READ_THREAD_ERR > +} GDBThreadIdKind; > + > +typedef union GdbCmdVariant { > + const char *data; > + uint8_t opcode; > + unsigned long val_ul; > + unsigned long long val_ull; > + struct { > + GDBThreadIdKind kind; > + uint32_t pid; > + uint32_t tid; > + } thread_id; > +} GdbCmdVariant; > + > +#define get_param(p, i) (&g_array_index(p, GdbCmdVariant, i)) This will need renaming if its public outside of gdbstub. gdb_get_cmd_param? > + > +/* If this is moving to a public API could you kdoc the description when you move it please? > + * cmd_startswith -> cmd is compared using startswith > + * > + * allow_stop_reply -> true iff the gdbstub can respond to this command with > a > + * "stop reply" packet. The list of commands that accept such response is > + * defined at the GDB Remote Serial Protocol documentation. see: > + * > https://sourceware.org/gdb/onlinedocs/gdb/Stop-Reply-Packets.html#Stop-Reply-Packets. > + * > + * schema definitions: > + * Each schema parameter entry consists of 2 chars, > + * the first char represents the parameter type handling > + * the second char represents the delimiter for the next parameter > + * > + * Currently supported schema types: > + * 'l' -> unsigned long (stored in .val_ul) > + * 'L' -> unsigned long long (stored in .val_ull) > + * 's' -> string (stored in .data) > + * 'o' -> single char (stored in .opcode) > + * 't' -> thread id (stored in .thread_id) > + * '?' -> skip according to delimiter > + * > + * Currently supported delimiters: > + * '?' -> Stop at any delimiter (",;:=\0") > + * '0' -> Stop at "\0" > + * '.' -> Skip 1 char unless reached "\0" > + * Any other value is treated as the delimiter value itself > + */ > +typedef struct GdbCmdParseEntry { > + GdbCmdHandler handler; > + const char *cmd; > + bool cmd_startswith; > + const char *schema; > + bool allow_stop_reply; > +} GdbCmdParseEntry; > + > +#define get_cmd_parsers(p) (&g_array_index(p, GdbCmdParseEntry, 0)) > + > +/** > + * set_gdb_gen_query_table_arch() - set a table to handle arch-specific query > + * packets > + */ > +void set_gdb_gen_query_table_arch(GdbCmdParseEntry *table, int size); > + > +/** > + * set_gdb_gen_set_table_arch() - set a table to handle arch-specific set > + * packets > + */ > +void set_gdb_gen_set_table_arch(GdbCmdParseEntry *, int size); > + > +/** > + * set_query_supported_arch() - set arch-specific features in qSupported > + * features > + */ > +void set_query_supported_arch(char *); > + > +/** > + * gdb_put_packet() - put string into gdb server's buffer so it is sent > + * to the client > + */ > +int gdb_put_packet(const char *buf); > + > +#endif /* GDBSTUB_H */ -- Alex Bennée Virtualisation Tech Lead @ Linaro
