qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] target/alpha: Add TCG plugin register tracking support
@ 2025-06-27 20:47 Yodel Eldar
  2025-06-27 20:47 ` [PATCH 1/2] contrib/plugins/execlog: Add tab to the separator search of insn_disas Yodel Eldar
  2025-06-27 20:47 ` [PATCH 2/2] target/alpha: Add GDB XML feature file Yodel Eldar
  0 siblings, 2 replies; 7+ messages in thread
From: Yodel Eldar @ 2025-06-27 20:47 UTC (permalink / raw)
  To: qemu-devel; +Cc: alex.bennee, richard.henderson, laurent, Yodel Eldar

This patch adds TCG plugin register tracking support for the Alpha
target and resolves gitlab issue #2569:

  https://gitlab.com/qemu-project/qemu/-/issues/2569

As mentioned in the bug report by Alex Bennée, the register list is
built using the target's corresponding GDB XML feature file, but the
Alpha target does not have one. The second patch introduces the missing
feature file and the necessary plumbing for it.

While testing the second patch, I noticed the following error:

  qemu-alpha: GLib: g_strrstr: assertion 'haystack != NULL' failed

when running:

  ./qemu-alpha -d plugin \
  -plugin ./contrib/plugins/libexeclog.so,reg=*,rdisas=on \
  ./tests/tcg/alpha-linux-user/linux-test

and discovered an issue with execlog.c that the first patch resolves:
a missing null check after execlog searches a disassembled instruction
for a space separator between the mnemonic and the operands. Execlog
assumes that disassembled instructions will contain a space, but some
disassemblers use tabs (like Alpha).

Besides adding the null check, the execlog patch also adds tab to the
separator search by replacing the g_strstr_len call with a call to
strpbrk, so that the plugin would operate as intended for Alpha.

Two pointers in the immediate area of the changed code were converted to
const pointers in keeping with the QEMU Coding Style. Also, as a trivial
optimization, I took the liberty of adding a break statement to the
register search loop that immediately follows the separator search, so
that it breaks out of the loop as soon as a relevant register is found
in the instruction; please let me know if either of these minor changes
should be moved to a separate patch file.

Lastly, this is my first submission to QEMU, and I want to thank
every past, present, and future contributor to this project that has
kept my system secure as I tinker with weird machines in the ultimate
sandbox. QEMU is truly mind-blowing technology, and I have all of
you to thank for it: Thanks!

Yodel Eldar (2):
  contrib/plugins/execlog: Add tab to the separator search of insn_disas
  target/alpha: Add GDB XML feature file

 configs/targets/alpha-linux-user.mak |   1 +
 configs/targets/alpha-softmmu.mak    |   1 +
 contrib/plugins/execlog.c            |  15 +--
 gdb-xml/alpha-core.xml               | 136 +++++++++++++++++++++++++++
 target/alpha/cpu.c                   |   1 +
 5 files changed, 148 insertions(+), 6 deletions(-)
 create mode 100644 gdb-xml/alpha-core.xml

-- 
2.50.0



^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/2] contrib/plugins/execlog: Add tab to the separator search of insn_disas
  2025-06-27 20:47 [PATCH 0/2] target/alpha: Add TCG plugin register tracking support Yodel Eldar
@ 2025-06-27 20:47 ` Yodel Eldar
  2025-06-29 18:50   ` Alex Bennée
  2025-06-27 20:47 ` [PATCH 2/2] target/alpha: Add GDB XML feature file Yodel Eldar
  1 sibling, 1 reply; 7+ messages in thread
From: Yodel Eldar @ 2025-06-27 20:47 UTC (permalink / raw)
  To: qemu-devel; +Cc: alex.bennee, richard.henderson, laurent, Yodel Eldar

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");
+            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;
+                    }
                 }
             }
         }
-- 
2.50.0



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/2] target/alpha: Add GDB XML feature file
  2025-06-27 20:47 [PATCH 0/2] target/alpha: Add TCG plugin register tracking support Yodel Eldar
  2025-06-27 20:47 ` [PATCH 1/2] contrib/plugins/execlog: Add tab to the separator search of insn_disas Yodel Eldar
@ 2025-06-27 20:47 ` Yodel Eldar
  2025-06-28 10:09   ` Richard Henderson
  1 sibling, 1 reply; 7+ messages in thread
From: Yodel Eldar @ 2025-06-27 20:47 UTC (permalink / raw)
  To: qemu-devel; +Cc: alex.bennee, richard.henderson, laurent, Yodel Eldar

This patch adds the GDB XML feature file that describes Alpha's core
registers.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2569

Signed-off-by: Yodel Eldar <yodel.eldar@gmail.com>
---
 configs/targets/alpha-linux-user.mak |   1 +
 configs/targets/alpha-softmmu.mak    |   1 +
 gdb-xml/alpha-core.xml               | 136 +++++++++++++++++++++++++++
 target/alpha/cpu.c                   |   1 +
 4 files changed, 139 insertions(+)
 create mode 100644 gdb-xml/alpha-core.xml

diff --git a/configs/targets/alpha-linux-user.mak b/configs/targets/alpha-linux-user.mak
index ef8e365b09..aa25766236 100644
--- a/configs/targets/alpha-linux-user.mak
+++ b/configs/targets/alpha-linux-user.mak
@@ -2,3 +2,4 @@ TARGET_ARCH=alpha
 TARGET_SYSTBL_ABI=common
 TARGET_SYSTBL=syscall.tbl
 TARGET_LONG_BITS=64
+TARGET_XML_FILES= gdb-xml/alpha-core.xml
diff --git a/configs/targets/alpha-softmmu.mak b/configs/targets/alpha-softmmu.mak
index 5275076e50..e31f059a52 100644
--- a/configs/targets/alpha-softmmu.mak
+++ b/configs/targets/alpha-softmmu.mak
@@ -1,2 +1,3 @@
 TARGET_ARCH=alpha
 TARGET_LONG_BITS=64
+TARGET_XML_FILES= gdb-xml/alpha-core.xml
diff --git a/gdb-xml/alpha-core.xml b/gdb-xml/alpha-core.xml
new file mode 100644
index 0000000000..c9e12f4ffd
--- /dev/null
+++ b/gdb-xml/alpha-core.xml
@@ -0,0 +1,136 @@
+<?xml version="1.0"?>
+<!-- Copyright (C) 2025 Free Software Foundation, Inc.
+
+     Copying and distribution of this file, with or without modification,
+     are permitted in any medium without royalty provided the copyright
+     notice and this notice are preserved.  -->
+
+<!DOCTYPE feature SYSTEM "gdb-target.dtd">
+<feature name="org.gnu.gdb.alpha.core">
+  <!-- IEEE rounding mode values -->
+  <enum id="dyn_rm_enum" size="8">
+    <!-- Chopped rounding mode -->
+    <evalue name="chop" value="0"/>
+    <!-- Minus infinity -->
+    <evalue name="-inf" value="1"/>
+    <!-- Normal rounding -->
+    <evalue name="norm" value="2"/>
+    <!-- Plus infinity -->
+    <evalue name="+inf" value="3"/>
+  </enum>
+
+  <!-- Floating-Point Control Register Flags -->
+  <flags id="fpcr_flags" size="8">
+    <!-- Denormal Operand Exception Disable -->
+    <field name="DNOD"   start="47" end="47"/>
+    <!-- Denormal Operands to Zero -->
+    <field name="DNZ"    start="48" end="48"/>
+    <!-- Invalid Operation Disable -->
+    <field name="INVD"   start="49" end="49"/>
+    <!-- Division by Zero Disable -->
+    <field name="DZED"   start="50" end="50"/>
+    <!-- Overflow Disable -->
+    <field name="OVFD"   start="51" end="51"/>
+    <!-- Invalid Operation -->
+    <field name="INV"    start="52" end="52"/>
+    <!-- Division by Zero -->
+    <field name="DZE"    start="53" end="53"/>
+    <!-- Overflow -->
+    <field name="OVF"    start="54" end="54"/>
+    <!-- Underflow -->
+    <field name="UNF"    start="55" end="55"/>
+    <!-- Inexact Result -->
+    <field name="INE"    start="56" end="56"/>
+    <!-- Integer Overflow -->
+    <field name="IOV"    start="57" end="57"/>
+    <!-- Dynamic Rounding Mode -->
+    <field name="DYN_RM" start="58" end="59" type="dyn_rm_enum"/>
+    <!-- Underflow to Zero -->
+    <field name="UNDZ"   start="60" end="60"/>
+    <!-- Underflow Disable -->
+    <field name="UNFD"   start="61" end="61"/>
+    <!-- Inexact Disable -->
+    <field name="INED"   start="62" end="62"/>
+    <!-- Summary Bit -->
+    <field name="SUM"    start="63" end="63"/>
+  </flags>
+
+  <!-- Integer Registers -->
+  <reg name="v0"   bitsize="64" type="int64"/>
+  <reg name="t0"   bitsize="64" type="int64"/>
+  <reg name="t1"   bitsize="64" type="int64"/>
+  <reg name="t2"   bitsize="64" type="int64"/>
+  <reg name="t3"   bitsize="64" type="int64"/>
+  <reg name="t4"   bitsize="64" type="int64"/>
+  <reg name="t5"   bitsize="64" type="int64"/>
+  <reg name="t6"   bitsize="64" type="int64"/>
+  <reg name="t7"   bitsize="64" type="int64"/>
+  <reg name="s0"   bitsize="64" type="int64"/>
+  <reg name="s1"   bitsize="64" type="int64"/>
+  <reg name="s2"   bitsize="64" type="int64"/>
+  <reg name="s3"   bitsize="64" type="int64"/>
+  <reg name="s4"   bitsize="64" type="int64"/>
+  <reg name="s5"   bitsize="64" type="int64"/>
+  <reg name="fp"   bitsize="64" type="int64"/>
+  <reg name="a0"   bitsize="64" type="int64"/>
+  <reg name="a1"   bitsize="64" type="int64"/>
+  <reg name="a2"   bitsize="64" type="int64"/>
+  <reg name="a3"   bitsize="64" type="int64"/>
+  <reg name="a4"   bitsize="64" type="int64"/>
+  <reg name="a5"   bitsize="64" type="int64"/>
+  <reg name="t8"   bitsize="64" type="int64"/>
+  <reg name="t9"   bitsize="64" type="int64"/>
+  <reg name="t10"  bitsize="64" type="int64"/>
+  <reg name="t11"  bitsize="64" type="int64"/>
+  <reg name="ra"   bitsize="64" type="int64"/>
+  <reg name="t12"  bitsize="64" type="int64"/>
+  <reg name="at"   bitsize="64" type="int64"/>
+  <reg name="gp"   bitsize="64" type="data_ptr"/>
+  <reg name="sp"   bitsize="64" type="data_ptr"/>
+  <reg name="zero" bitsize="64" type="int64" save-restore="no"/>
+
+  <!-- Floating-Point Registers -->
+  <reg name="f0"  bitsize="64" type="float" group="float"/>
+  <reg name="f1"  bitsize="64" type="float" group="float"/>
+  <reg name="f2"  bitsize="64" type="float" group="float"/>
+  <reg name="f3"  bitsize="64" type="float" group="float"/>
+  <reg name="f4"  bitsize="64" type="float" group="float"/>
+  <reg name="f5"  bitsize="64" type="float" group="float"/>
+  <reg name="f6"  bitsize="64" type="float" group="float"/>
+  <reg name="f7"  bitsize="64" type="float" group="float"/>
+  <reg name="f8"  bitsize="64" type="float" group="float"/>
+  <reg name="f9"  bitsize="64" type="float" group="float"/>
+  <reg name="f10" bitsize="64" type="float" group="float"/>
+  <reg name="f11" bitsize="64" type="float" group="float"/>
+  <reg name="f12" bitsize="64" type="float" group="float"/>
+  <reg name="f13" bitsize="64" type="float" group="float"/>
+  <reg name="f14" bitsize="64" type="float" group="float"/>
+  <reg name="f15" bitsize="64" type="float" group="float"/>
+  <reg name="f16" bitsize="64" type="float" group="float"/>
+  <reg name="f17" bitsize="64" type="float" group="float"/>
+  <reg name="f18" bitsize="64" type="float" group="float"/>
+  <reg name="f19" bitsize="64" type="float" group="float"/>
+  <reg name="f20" bitsize="64" type="float" group="float"/>
+  <reg name="f21" bitsize="64" type="float" group="float"/>
+  <reg name="f22" bitsize="64" type="float" group="float"/>
+  <reg name="f23" bitsize="64" type="float" group="float"/>
+  <reg name="f24" bitsize="64" type="float" group="float"/>
+  <reg name="f25" bitsize="64" type="float" group="float"/>
+  <reg name="f26" bitsize="64" type="float" group="float"/>
+  <reg name="f27" bitsize="64" type="float" group="float"/>
+  <reg name="f28" bitsize="64" type="float" group="float"/>
+  <reg name="f29" bitsize="64" type="float" group="float"/>
+  <reg name="f30" bitsize="64" type="float" group="float"/>
+
+  <!-- Floating-Point Control Register -->
+  <reg name="fpcr" bitsize="64" type="fpcr_flags" group="float"/>
+
+  <!-- Program Counter -->
+  <reg name="pc" bitsize="64" type="code_ptr"/>
+
+  <!-- Reserved Index for Former Virtual Register -->
+  <reg name="" bitsize="64" type="int64" save-restore="no"/>
+
+  <!-- PALcode Memory Slot -->
+  <reg name="unique" bitsize="64" type="int64" group="system"/>
+</feature>
diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c
index 2082db45ea..bf1787a69d 100644
--- a/target/alpha/cpu.c
+++ b/target/alpha/cpu.c
@@ -286,6 +286,7 @@ static void alpha_cpu_class_init(ObjectClass *oc, const void *data)
     cc->get_pc = alpha_cpu_get_pc;
     cc->gdb_read_register = alpha_cpu_gdb_read_register;
     cc->gdb_write_register = alpha_cpu_gdb_write_register;
+    cc->gdb_core_xml_file = "alpha-core.xml";
 #ifndef CONFIG_USER_ONLY
     dc->vmsd = &vmstate_alpha_cpu;
     cc->sysemu_ops = &alpha_sysemu_ops;
-- 
2.50.0



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] target/alpha: Add GDB XML feature file
  2025-06-27 20:47 ` [PATCH 2/2] target/alpha: Add GDB XML feature file Yodel Eldar
@ 2025-06-28 10:09   ` Richard Henderson
  0 siblings, 0 replies; 7+ messages in thread
From: Richard Henderson @ 2025-06-28 10:09 UTC (permalink / raw)
  To: Yodel Eldar, qemu-devel; +Cc: alex.bennee, laurent

On 6/27/25 13:47, Yodel Eldar wrote:
> This patch adds the GDB XML feature file that describes Alpha's core
> registers.
> 
> Resolves:https://gitlab.com/qemu-project/qemu/-/issues/2569
> 
> Signed-off-by: Yodel Eldar<yodel.eldar@gmail.com>
> ---
>   configs/targets/alpha-linux-user.mak |   1 +
>   configs/targets/alpha-softmmu.mak    |   1 +
>   gdb-xml/alpha-core.xml               | 136 +++++++++++++++++++++++++++
>   target/alpha/cpu.c                   |   1 +
>   4 files changed, 139 insertions(+)
>   create mode 100644 gdb-xml/alpha-core.xml

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] contrib/plugins/execlog: Add tab to the separator search of insn_disas
  2025-06-27 20:47 ` [PATCH 1/2] contrib/plugins/execlog: Add tab to the separator search of insn_disas Yodel Eldar
@ 2025-06-29 18:50   ` Alex Bennée
  2025-06-29 22:49     ` Yodel Eldar
  0 siblings, 1 reply; 7+ messages in thread
From: Alex Bennée @ 2025-06-29 18:50 UTC (permalink / raw)
  To: Yodel Eldar; +Cc: qemu-devel, richard.henderson, laurent

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;
+                    }


> +            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;
> +                    }
>                  }
>              }
>          }

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] contrib/plugins/execlog: Add tab to the separator search of insn_disas
  2025-06-29 18:50   ` Alex Bennée
@ 2025-06-29 22:49     ` Yodel Eldar
  2025-06-30  8:42       ` Alex Bennée
  0 siblings, 1 reply; 7+ messages in thread
From: Yodel Eldar @ 2025-06-29 22:49 UTC (permalink / raw)
  To: Alex Bennée; +Cc: qemu-devel, richard.henderson, laurent, Yodel Eldar


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;
>> +                    }
>>                   }
>>               }
>>           }


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] contrib/plugins/execlog: Add tab to the separator search of insn_disas
  2025-06-29 22:49     ` Yodel Eldar
@ 2025-06-30  8:42       ` Alex Bennée
  0 siblings, 0 replies; 7+ messages in thread
From: Alex Bennée @ 2025-06-30  8:42 UTC (permalink / raw)
  To: Yodel Eldar; +Cc: qemu-devel, richard.henderson, laurent

Yodel Eldar <yodel.eldar@gmail.com> writes:

> 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?

Suggested-by is fine ;-)

>
>
>>> +            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;
>>> +                    }
>>>                   }
>>>               }
>>>           }

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2025-06-30  8:43 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-27 20:47 [PATCH 0/2] target/alpha: Add TCG plugin register tracking support Yodel Eldar
2025-06-27 20:47 ` [PATCH 1/2] contrib/plugins/execlog: Add tab to the separator search of insn_disas Yodel Eldar
2025-06-29 18:50   ` Alex Bennée
2025-06-29 22:49     ` Yodel Eldar
2025-06-30  8:42       ` Alex Bennée
2025-06-27 20:47 ` [PATCH 2/2] target/alpha: Add GDB XML feature file Yodel Eldar
2025-06-28 10:09   ` Richard Henderson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).