Richard Henderson <richard.hender...@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
