On Fri, Aug 8, 2025 at 7:19 PM Pierrick Bouvier
<pierrick.bouv...@linaro.org> wrote:
>
> On 8/8/25 1:14 AM, Manos Pitsidianakis wrote:
> > On Fri, 08 Aug 2025 05:06, Pierrick Bouvier <pierrick.bouv...@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.bouv...@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.bouv...@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/
> >
>
> Yep, that's better.
>
> >> + struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, i);
> >
> > This can return NULL,
> >
>
> It will return NULL only if i is out of the tb range, which will never
> happen here, because i < n_insns.
> As you can see in all other plugins we have, there is never a NULL check
> for the return of qemu_plugin_tb_get_insn.
> It points a good thing in the API though, that maybe we should simply
> assert i is in the range, because there is no reason for a user to use a
> random index that may fall out of the tb range.
Ah thanks for pointing that out. Keep my r-b
>
> >> +
> >> + 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
> >
>
> Thanks, probably when I created the intermediate series of patches.
>
> >
> > + 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)
> >
>
> Yes, it does not hurt, I'll add it.
>
> >> +}
> >> +
> >> +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.pitsidiana...@linaro.org>
>
