Re: [PATCH v5 7/9] gdbstub: Make get cpu and hex conversion functions non-internal

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 27/6/24 13:05, Alex Bennée wrote:
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> 
>> On 27/6/24 06:13, Gustavo Romero wrote:
>>> Make the gdb_first_attached_cpu and gdb_hextomem non-internal so they
>>> are not confined to use only in gdbstub.c.
>>> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>> ---
>>>    gdbstub/internals.h        | 2 --
>>>    include/exec/gdbstub.h     | 5 +++++
>>>    include/gdbstub/commands.h | 6 ++++++
>>>    3 files changed, 11 insertions(+), 2 deletions(-)
>>
>>
>>> diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
>>> index 1bd2c4ec2a..77e5ec9a5b 100644
>>> --- a/include/exec/gdbstub.h
>>> +++ b/include/exec/gdbstub.h
>>> @@ -135,4 +135,9 @@ void gdb_set_stop_cpu(CPUState *cpu);
>>>    /* in gdbstub-xml.c, generated by scripts/feature_to_c.py */
>>>    extern const GDBFeature gdb_static_features[];
>>>    +/**
>>> + * Return the first attached CPU
>>> + */
>>> +CPUState *gdb_first_attached_cpu(void);
>>
>> Alex, it seems dubious to expose the API like that.
>>
>> IMHO GdbCmdHandler should take a GDBRegisterState argument,
>> then this would become:
>>
>>    CPUState *gdb_first_attached_cpu(GDBRegisterState *s);
> 
> Maybe instead of exposing this we can use user_ctx for something? If we
> look at handle_set_reg/handle_get_reg we can see that passes down
> gdbserver_state.g_cpu down to the eventual helpers. We could define
> something like:
> 
> --8<---------------cut here---------------start------------->8---
> fixups to avoid get_first_cpu()
> 
> 5 files changed, 25 insertions(+), 18 deletions(-)
> gdbstub/internals.h        |  1 +
> include/exec/gdbstub.h     |  5 -----
> include/gdbstub/commands.h |  3 +++
> gdbstub/gdbstub.c          |  7 ++++++-
> target/arm/gdbstub64.c     | 27 +++++++++++++++------------


> @@ -54,6 +54,8 @@ typedef union GdbCmdVariant {
>    * "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.
> + *
> + * @need_cpu_context: pass current CPU to command via user_ctx.
>    */
>   typedef struct GdbCmdParseEntry {
>       GdbCmdHandler handler;
> @@ -61,6 +63,7 @@ typedef struct GdbCmdParseEntry {
>       bool cmd_startswith;
>       const char *schema;
>       bool allow_stop_reply;
> +    bool need_cpu_context;
>   } GdbCmdParseEntry;
>   
>   #define get_cmd_parsers(p) (&g_array_index(p, GdbCmdParseEntry, 0))
> modified   gdbstub/gdbstub.c
> @@ -938,6 +938,7 @@ static bool process_string_cmd(const char *data,
>   
>       for (i = 0; i < num_cmds; i++) {
>           const GdbCmdParseEntry *cmd = &cmds[i];
> +        void *user_ctx = NULL;
>           g_assert(cmd->handler && cmd->cmd);
>   
>           if ((cmd->cmd_startswith && !startswith(data, cmd->cmd)) ||
> @@ -952,8 +953,12 @@ static bool process_string_cmd(const char *data,
>               }
>           }
>   
> +        if (cmd->need_cpu_context) {
> +            user_ctx = (void *) gdbserver_state.g_cpu;

LGTM.

> +        }
> +
>           gdbserver_state.allow_stop_reply = cmd->allow_stop_reply;
> -        cmd->handler(params, NULL);
> +        cmd->handler(params, user_ctx);
>           return true;
>       }