Gustavo Romero <gustavo.rom...@linaro.org> writes:
> Hi Alex,
>
> On 6/14/24 8:27 AM, Alex Bennée wrote:
>> 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 gdb_extend_qsupported_features,
>>> gdb_extend_query_table, and gdb_extend_set_table functions as interfaces
>>> to extend the qSupported string, the query handler table, and the set
>>> handler table, allowing target-specific stub implementations.
>>>
>>> Signed-off-by: Gustavo Romero <gustavo.rom...@linaro.org>
>>> ---
>>> gdbstub/gdbstub.c | 59 ++++++++++++++++++++++++++++++++++----
>>> include/gdbstub/commands.h | 22 ++++++++++++++
>>> 2 files changed, 75 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
>>> index 9ff2f4177d..e69cc5131e 100644
>>> --- a/gdbstub/gdbstub.c
>>> +++ b/gdbstub/gdbstub.c
>>> @@ -1609,6 +1609,12 @@ static void handle_query_thread_extra(GArray
>>> *params, void *user_ctx)
>>> gdb_put_strbuf();
>>> }
>>> +static char *extended_qsupported_features;
>>> +void gdb_extend_qsupported_features(char *qsupported_features)
>>> +{
>> Maybe g_assert(!extended_qsupported_features)?
>>
>>> + extended_qsupported_features = qsupported_features;
>>> +}
>>> +
>>> static void handle_query_supported(GArray *params, void *user_ctx)
>>> {
>>> CPUClass *cc;
>>> @@ -1648,6 +1654,11 @@ static void handle_query_supported(GArray *params,
>>> void *user_ctx)
>>> }
>>> g_string_append(gdbserver_state.str_buf,
>>> ";vContSupported+;multiprocess+");
>>> +
>>> + if (extended_qsupported_features) {
>>> + g_string_append(gdbserver_state.str_buf,
>>> extended_qsupported_features);
>>> + }
>>> +
>>> gdb_put_strbuf();
>>> }
>>> @@ -1729,6 +1740,14 @@ static const GdbCmdParseEntry
>>> gdb_gen_query_set_common_table[] = {
>>> },
>>> };
>>> +static GdbCmdParseEntry *extended_query_table;
>>> +static int extended_query_table_size;
>>> +void gdb_extend_query_table(GdbCmdParseEntry *table, int size)
>>> +{
>> g_assert(!extended_query_table)
>>
>>> + extended_query_table = table;
>>> + extended_query_table_size = size;
>>> +}
>>> +
>>> static const GdbCmdParseEntry gdb_gen_query_table[] = {
>>> {
>>> .handler = handle_query_curr_tid,
>>> @@ -1821,6 +1840,14 @@ static const GdbCmdParseEntry gdb_gen_query_table[]
>>> = {
>>> #endif
>>> };
>>> +static GdbCmdParseEntry *extended_set_table;
>>> +static int extended_set_table_size;
>>> +void gdb_extend_set_table(GdbCmdParseEntry *table, int size)
>>> +{
>>> + extended_set_table = table;
>>> + extended_set_table_size = size;
>>> +}
>>> +
>>> static const GdbCmdParseEntry gdb_gen_set_table[] = {
>>> /* Order is important if has same prefix */
>>> {
>>> @@ -1859,11 +1886,21 @@ static void handle_gen_query(GArray *params, void
>>> *user_ctx)
>>> return;
>>> }
>>> - if (!process_string_cmd(gdb_get_cmd_param(params, 0)->data,
>>> - gdb_gen_query_table,
>>> - ARRAY_SIZE(gdb_gen_query_table))) {
>>> - gdb_put_packet("");
>>> + if (process_string_cmd(gdb_get_cmd_param(params, 0)->data,
>>> + gdb_gen_query_table,
>>> + ARRAY_SIZE(gdb_gen_query_table))) {
>>> + return;
>>> + }
>>> +
>>> + if (extended_query_table &&
>>> + process_string_cmd(gdb_get_cmd_param(params, 0)->data,
>>> + extended_query_table,
>>> + extended_query_table_size)) {
>>> + return;
>>> }
>>> +
>>> + /* Can't handle query, return Empty response. */
>>> + gdb_put_packet("");
>>> }
>>> static void handle_gen_set(GArray *params, void *user_ctx)
>>> @@ -1878,11 +1915,21 @@ static void handle_gen_set(GArray *params, void
>>> *user_ctx)
>>> return;
>>> }
>>> - if (!process_string_cmd(gdb_get_cmd_param(params, 0)->data,
>>> + if (process_string_cmd(gdb_get_cmd_param(params, 0)->data,
>>> gdb_gen_set_table,
>>> ARRAY_SIZE(gdb_gen_set_table))) {
>>> - gdb_put_packet("");
>>> + return;
>>> }
>>> +
>>> + if (extended_set_table &&
>>> + process_string_cmd(gdb_get_cmd_param(params, 0)->data,
>>> + extended_set_table,
>>> + extended_set_table_size)) {
>>> + return;
>>> + }
>>> +
>>> + /* Can't handle set, return Empty response. */
>>> + gdb_put_packet("");
>>> }
>>> static void handle_target_halt(GArray *params, void *user_ctx)
>>> diff --git a/include/gdbstub/commands.h b/include/gdbstub/commands.h
>>> index dd45c38472..2204c3ddbe 100644
>>> --- a/include/gdbstub/commands.h
>>> +++ b/include/gdbstub/commands.h
>>> @@ -71,4 +71,26 @@ typedef struct GdbCmdParseEntry {
>>> */
>>> int gdb_put_packet(const char *buf);
>>> +/**
>>> + * gdb_extend_query_table() - Extend query table.
>>> + * @table: The table with the additional query packet handlers.
>>> + * @size: The number of handlers to be added.
>>> + */
>>> +void gdb_extend_query_table(GdbCmdParseEntry *table, int size);
>>> +
>>> +/**
>>> + * gdb_extend_set_table() - Extend set table.
>>> + * @table: The table with the additional set packet handlers.
>>> + * @size: The number of handlers to be added.
>>> + */
>>> +void gdb_extend_set_table(GdbCmdParseEntry *table, int size);
>>> +
>>> +/**
>>> + * gdb_extend_qsupported_features() - Extend the qSupported features
>>> string.
>>> + * @qsupported_features: The additional qSupported feature(s) string. The
>>> string
>>> + * should start with a semicolon and, if there are more than one feature,
>>> the
>>> + * features should be separate by a semiocolon.
>>> + */
>>> +void gdb_extend_qsupported_features(char *qsupported_features);
>>> +
>> We should make it clear these functions should only be called once
>> (although I guess in a heterogeneous future we might have to do something
>> more clever).
>
> There a some cases where actually the API functions are called multiple
> times it looks like. For instance, the assert is hit in the following test:
>
> $ ./build/qemu-aarch64 ./build/tests/tcg/aarch64-linux-user/munmap-pthread
>
> Probably because pthread_create is involved, but I can't explain it
> yet.
Hmm, I guess new threads still need to register with gdb. I wonder if asserts
like:
g_assert(extended_set_table || extended_set_table == table)
would be valid?
>
>
> Cheers,
> Gustavo
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
