Re: [PATCH 1/2] contrib/plugins/execlog: Add tab to the separator search of insn_disas

[Yodel Eldar <yodel.eldar@gmail.com>]

On 6/29/25 1:50 PM, Alex Bennée wrote:
> Yodel Eldar <yodel.eldar@gmail.com> writes:
>
>> Currently, execlog searches for a space separator between the
>> instruction mnemonic and operands, but some disassemblers, e.g. Alpha's,
>> use a tab separator instead; this results in a null pointer being passed
>> as the haystack in g_strstr during a subsequent register search, i.e.
>> undefined behavior, because of a missing null check.
>>
>> This patch adds tab to the separator search and a null check on the
>> result.
>>
>> Also, existing, affected pointers are changed to const.
>>
>> Lastly, a break statement was added to immediately terminate the
>> register search when a user-requested register is found in the current
>> instruction as a trivial optimization, because searching for the
>> remaining requested registers is unnecessary once one is found.
>>
>> Signed-off-by: Yodel Eldar <yodel.eldar@gmail.com>
>> ---
>>   contrib/plugins/execlog.c | 15 +++++++++------
>>   1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/contrib/plugins/execlog.c b/contrib/plugins/execlog.c
>> index d67d010761..08fc1f12d4 100644
>> --- a/contrib/plugins/execlog.c
>> +++ b/contrib/plugins/execlog.c
>> @@ -232,12 +232,15 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
>>            */
>>           if (disas_assist && rmatches) {
>>               check_regs_next = false;
>> -            gchar *args = g_strstr_len(insn_disas, -1, " ");
>> -            for (int n = 0; n < all_reg_names->len; n++) {
>> -                gchar *reg = g_ptr_array_index(all_reg_names, n);
>> -                if (g_strrstr(args, reg)) {
>> -                    check_regs_next = true;
>> -                    skip = false;
>> +            const gchar *args = strpbrk(insn_disas, " \t");
> We have a general preference for glib here, could we use g_strsplit_set?
>
> Something like:
>
> modified   contrib/plugins/execlog.c
> @@ -232,12 +232,14 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
>            */
>           if (disas_assist && rmatches) {
>               check_regs_next = false;
> -            gchar *args = g_strstr_len(insn_disas, -1, " ");
> -            for (int n = 0; n < all_reg_names->len; n++) {
> -                gchar *reg = g_ptr_array_index(all_reg_names, n);
> -                if (g_strrstr(args, reg)) {
> -                    check_regs_next = true;
> -                    skip = false;
> +            g_auto(GStrv) args = g_strsplit_set(insn_disas, " \t", 2);
> +            if (args && args[1]) {
> +                for (int n = 0; n < all_reg_names->len; n++) {
> +                    gchar *reg = g_ptr_array_index(all_reg_names, n);
> +                    if (g_strrstr(args[1], reg)) {
> +                        check_regs_next = true;
> +                        skip = false;
> +                    }
>

Certainly, and thanks for the suggestion! May I credit you with a
"Suggested-by" or "Co-authored-by" tag in v2 of the patch?


>> +            if (args) {
>> +                for (int n = 0; n < all_reg_names->len; n++) {
>> +                    const gchar *reg = g_ptr_array_index(all_reg_names, n);
>> +                    if (g_strrstr(args, reg)) {
>> +                        check_regs_next = true;
>> +                        skip = false;
>> +                        break;
>> +                    }
>>                   }
>>               }
>>           }