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

[Gustavo Romero <gustavo.romero@linaro.org>]

Hi Phil, Alex,

On 6/27/24 9:26 AM, Philippe Mathieu-Daudé wrote:
> 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.

Thanks for the suggestion. I added it to v6.


Cheers,
Gustavo