Re: [PATCH v5 1/9] contrib/plugins/uftrace: skeleton file

[Manos Pitsidianakis <manos.pitsidianakis@linaro.org>]

On Fri, 08 Aug 2025 05:06, Pierrick Bouvier <pierrick.bouvier@linaro.org> wrote:
>We define a scoreboard that will hold our data per cpu. As well, we
>define a buffer per cpu that will be used to read registers and memories
>in a thread-safe way.
>
>For now, we just instrument all instructions with an empty callback.
>
>Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>---
> contrib/plugins/uftrace.c   | 74 +++++++++++++++++++++++++++++++++++++
> contrib/plugins/meson.build |  3 +-
> 2 files changed, 76 insertions(+), 1 deletion(-)
> create mode 100644 contrib/plugins/uftrace.c
>
>diff --git a/contrib/plugins/uftrace.c b/contrib/plugins/uftrace.c
>new file mode 100644
>index 00000000000..d60c1077496
>--- /dev/null
>+++ b/contrib/plugins/uftrace.c
>@@ -0,0 +1,74 @@
>+/*
>+ * Copyright (C) 2025, Pierrick Bouvier <pierrick.bouvier@linaro.org>
>+ *
>+ * Generates a trace compatible with uftrace (similar to uftrace record).
>+ * https://github.com/namhyung/uftrace
>+ *
>+ * See docs/about/emulation.rst|Uftrace for details and examples.
>+ *
>+ * SPDX-License-Identifier: GPL-2.0-or-later
>+ */
>+
>+#include <qemu-plugin.h>
>+#include <glib.h>
>+
>+QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
>+
>+typedef struct Cpu {
>+    GByteArray *buf;
>+} Cpu;
>+
>+static struct qemu_plugin_scoreboard *score;
>+
>+static void track_callstack(unsigned int cpu_index, void *udata)
>+{
>+}
>+
>+static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
>+{
>+    size_t n_insns = qemu_plugin_tb_n_insns(tb);
>+
>+    for (int i = 0; i < n_insns; i++) {

s/int i/size_t i/

>+        struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, i);

This can return NULL,

>+
>+        uintptr_t pc = qemu_plugin_insn_vaddr(insn);

And this would lead to a NULL dereference (it performs insn->vaddr 
access)

>+        qemu_plugin_register_vcpu_insn_exec_cb(insn, track_callstack,
>+                QEMU_PLUGIN_CB_R_REGS,
>+                (void *) pc);

Hm indentation got broken here, should be 


+        qemu_plugin_register_vcpu_insn_exec_cb(insn, track_callstack,
+                                               QEMU_PLUGIN_CB_R_REGS,
+                                               (void *)pc);

>+
>+    }
>+}
>+
>+static void vcpu_init(qemu_plugin_id_t id, unsigned int vcpu_index)
>+{
>+    Cpu *cpu = qemu_plugin_scoreboard_find(score, vcpu_index);
>+    cpu->buf = g_byte_array_new();
>+}
>+
>+static void vcpu_end(unsigned int vcpu_index)
>+{
>+    Cpu *cpu = qemu_plugin_scoreboard_find(score, vcpu_index);
>+    g_byte_array_free(cpu->buf, true);
>+    memset(cpu, 0, sizeof(Cpu));

Nitpick, cpu->buf = NULL; is easier to understand (suggestion)

>+}
>+
>+static void at_exit(qemu_plugin_id_t id, void *data)
>+{
>+    for (size_t i = 0; i < qemu_plugin_num_vcpus(); ++i) {
>+        vcpu_end(i);
>+    }
>+
>+    qemu_plugin_scoreboard_free(score);
>+}
>+
>+QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
>+                                           const qemu_info_t *info,
>+                                           int argc, char **argv)
>+{
>+    score = qemu_plugin_scoreboard_new(sizeof(Cpu));
>+    qemu_plugin_register_vcpu_init_cb(id, vcpu_init);
>+    qemu_plugin_register_atexit_cb(id, at_exit, NULL);
>+    qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans);
>+
>+    return 0;
>+}
>diff --git a/contrib/plugins/meson.build b/contrib/plugins/meson.build
>index 1876bc78438..7eb3629c95d 100644
>--- a/contrib/plugins/meson.build
>+++ b/contrib/plugins/meson.build
>@@ -1,5 +1,6 @@
> contrib_plugins = ['bbv', 'cache', 'cflow', 'drcov', 'execlog', 'hotblocks',
>-                   'hotpages', 'howvec', 'hwprofile', 'ips', 'stoptrigger']
>+                   'hotpages', 'howvec', 'hwprofile', 'ips', 'stoptrigger',
>+                   'uftrace']
> if host_os != 'windows'
>   # lockstep uses socket.h
>   contrib_plugins += 'lockstep'
>-- 
>2.47.2
>

If no other comments rise for this patch, you can add my r-b after 
fixing the NULL check:

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>