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

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(-)

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).

>  #endif /* GDBSTUB_COMMANDS_H */

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.


Cheers,
Gustavo

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