Re: [RFC PATCH] gdbstub: Re-factor gdb command extensions

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

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

> On 7/16/24 21:42, Alex Bennée wrote:
>>   void gdb_extend_qsupported_features(char *qsupported_features)
>>   {
>> -    /*
>> -     * We don't support different sets of CPU gdb features on different CPUs yet
>> -     * so assert the feature strings are the same on all CPUs, or is set only
>> -     * once (1 CPU).
>> -     */
>> -    g_assert(extended_qsupported_features == NULL ||
>> -             g_strcmp0(extended_qsupported_features, qsupported_features) == 0);
>> -
>> -    extended_qsupported_features = qsupported_features;
>> +    if (!extended_qsupported_features) {
>> +        extended_qsupported_features = g_strdup(qsupported_features);
>> +    } else if (!g_strrstr(extended_qsupported_features, qsupported_features)) {
>
> Did you really need the last instance of the substring?

Not really - I just want to check the string hasn't been added before.

>
> I'll note that g_strrstr is quite simplistic, whereas strstr has a
> much more scalable algorithm.
>
>
>> +        char *old = extended_qsupported_features;
>> +        extended_qsupported_features = g_strdup_printf("%s%s", old, qsupported_features);
>
> Right tool for the right job, please: g_strconcat().
>
> That said, did you *really* want to concatenate now, and have to
> search through the middle, as opposed to storing N strings separately?
> You could defer the concat until the actual negotiation with gdb.
> That would reduce strstr above to a loop over strcmp.
>
>> +    for (int i = 0; i < extensions->len; i++) {
>> +        gpointer entry = g_ptr_array_index(extensions, i);
>> +        if (!g_ptr_array_find(table, entry, NULL)) {
>> +            g_ptr_array_add(table, entry);
>
> Are you expecting the same GdbCmdParseEntry object to be registered
> multiple times?  Can we fix that at a higher level?

Its basically a hack to deal with the fact everything is tied to the
CPUObject so we register everything multiple times. We could do a if
(!registerd) register() dance but I guess I'm thinking forward to a
hydrogenous future but I guess we'd need to do more work then anyway.

>
>
> r~

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro