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

Pierrick Bouvier posted 9 patches 4 months, 1 week ago
Maintainers: "Alex Bennée" <alex.bennee@linaro.org>, Alexandre Iooss <erdnaxe@crans.org>, Mahmoud Mandour <ma.mandourr@gmail.com>, Pierrick Bouvier <pierrick.bouvier@linaro.org>
There is a newer version of this series
[PATCH v5 1/9] contrib/plugins/uftrace: skeleton file
Posted by Pierrick Bouvier 4 months, 1 week ago
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++) {
+        struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, i);
+
+        uintptr_t pc = qemu_plugin_insn_vaddr(insn);
+        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));
+}
+
+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
Re: [PATCH v5 1/9] contrib/plugins/uftrace: skeleton file
Posted by Manos Pitsidianakis 4 months, 1 week ago
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>
Re: [PATCH v5 1/9] contrib/plugins/uftrace: skeleton file
Posted by Pierrick Bouvier 4 months, 1 week ago
On 8/8/25 1:14 AM, Manos Pitsidianakis wrote:
> 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)
>

Looking at this twice, it does not scale with other commits, which will 
free other members. That's why there was a memset in the first place.
Sorry, I rebuilt intermediate commits by reverting various parts, so it 
may result in suck quirks.

>> +}
>> +
>> +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>
Re: [PATCH v5 1/9] contrib/plugins/uftrace: skeleton file
Posted by Pierrick Bouvier 4 months, 1 week ago
On 8/8/25 1:14 AM, Manos Pitsidianakis wrote:
> 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/
>

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.

>> +
>> +        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.pitsidianakis@linaro.org>
Re: [PATCH v5 1/9] contrib/plugins/uftrace: skeleton file
Posted by Manos Pitsidianakis 4 months, 1 week ago
On Fri, Aug 8, 2025 at 7:19 PM Pierrick Bouvier
<pierrick.bouvier@linaro.org> wrote:
>
> On 8/8/25 1:14 AM, Manos Pitsidianakis wrote:
> > 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/
> >
>
> 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.pitsidianakis@linaro.org>
>