[v3,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(-)

On 6/16/24 23:28, Gustavo Romero wrote:
> +static char *extended_qsupported_features;
> +void gdb_extend_qsupported_features(char *qsupported_features)
> +{
> +    extended_qsupported_features = qsupported_features;
> +}

Assert these functions aren't called twice.
That should be good enough until we need something more complicated.

Speaking of more complicated... do we have a plan for gdb when we get to the point where 
the board contains multiple cpu types?  Not yet, of course, but we're working on it...


r~

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)
> +{

  /*
   * We don't support different sets of CPU gdb features on different
   * CPUs yet so assert things are the same.
   */

   g_assert(!extended_qsupported_features ||
   g_strcmp0(extended_qsupported_features, qsupported_features) == 0)

> +    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)
> +{
> +    extended_query_table = table;
> +    extended_query_table_size = size;

Similar assert here.

> +}
> +
>  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)
> +{

ditto

> +    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);
> +
>  #endif /* GDBSTUB_COMMANDS_H */

Otherwise:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

Richard Henderson <richard.henderson@linaro.org> writes:

> On 6/16/24 23:28, Gustavo Romero wrote:
>> +static char *extended_qsupported_features;
>> +void gdb_extend_qsupported_features(char *qsupported_features)
>> +{
>> +    extended_qsupported_features = qsupported_features;
>> +}
>
> Assert these functions aren't called twice.
> That should be good enough until we need something more complicated.

They are called more than once (because we go through the vcpu realize
step for each vcpu). But we should assert that if set we are not
changing the feature set.

> Speaking of more complicated... do we have a plan for gdb when we get
> to the point where the board contains multiple cpu types?  Not yet, of
> course, but we're working on it...

I think gdb needs to be a little smarter here.

>
>
> r~

Hi Alex,

On 6/21/24 5:11 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)
>> +{
> 
>    /*
>     * We don't support different sets of CPU gdb features on different
>     * CPUs yet so assert things are the same.
>     */
> 
>     g_assert(!extended_qsupported_features ||
>     g_strcmp0(extended_qsupported_features, qsupported_features) == 0)
>
Done in v4. Thanks.


Cheers,
Gustavo

  
>> +    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)
>> +{
>> +    extended_query_table = table;
>> +    extended_query_table_size = size;
> 
> Similar assert here.
> 
>> +}
>> +
>>   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)
>> +{
> 
> ditto
> 
>> +    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);
>> +
>>   #endif /* GDBSTUB_COMMANDS_H */
> 
> Otherwise:
> 
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>