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

[Richard Henderson]

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?

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?


r~