Re: [PATCH v2 3/9] gdbstub: Add support for target-specific stubs

["Alex Bennée" <alex.bennee@linaro.org>]

Gustavo Romero <gustavo.romero@linaro.org> writes:

> Hi Alex,
>
> On 6/14/24 8:27 AM, Alex Bennée wrote:
>> Gustavo Romero <gustavo.romero@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.romero@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