Re: [PATCH v5 3/9] contrib/plugins/uftrace: track callstack

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

On 8/8/25 1:49 AM, Manos Pitsidianakis wrote:
> On Fri, 08 Aug 2025 05:06, Pierrick Bouvier <pierrick.bouvier@linaro.org> wrote:
>> We now track callstack, based on frame pointer analysis. We can detect
>> function calls, returns, and discontinuities.
>> We implement a frame pointer based unwinding that is used for
>> discontinuities.
> 
> 
> Nit: Never heard of the "discontinuity" term for program execution
> before :D Maybe "async control flow (signals, interrupts)"?
>

Someone posted a series to detect discontinuities in program execution.
I asked the same question as you:
[1] 
https://lore.kernel.org/qemu-devel/51ac04eea17a6c5b59a240d3c57ce54a851e4989@nut.email/

Since we'll probably have this series merged in the future, I though it 
was nice to reuse it.
As well, it's not only async control flow, a setjmp/longjmp will 
generate a discontinuity too. That's why I picked this name compared to 
"exceptional/async control flow".
That said, I don't have strong opinion, and as the name appears only in 
comments and commit message, it can be changed to anything.

>>
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>> contrib/plugins/uftrace.c | 160 ++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 160 insertions(+)
>>
>> diff --git a/contrib/plugins/uftrace.c b/contrib/plugins/uftrace.c
>> index 4b1a2f38143..d51faceb344 100644
>> --- a/contrib/plugins/uftrace.c
>> +++ b/contrib/plugins/uftrace.c
>> @@ -15,6 +15,15 @@
>>
>> QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
>>
>> +typedef struct {
>> +    GArray *s;
>> +} Callstack;
>> +
>> +typedef struct {
>> +    uint64_t pc;
>> +    uint64_t frame_pointer;
>> +} CallstackEntry;
>> +
>> typedef struct Cpu Cpu;
>>
>> typedef struct {
>> @@ -25,6 +34,7 @@ typedef struct {
>> } CpuOps;
>>
>> typedef struct Cpu {
>> +    Callstack *cs;
>>      GByteArray *buf;
>>      CpuOps ops;
>>      void *arch;
>> @@ -37,6 +47,71 @@ typedef struct {
>> static struct qemu_plugin_scoreboard *score;
>> static CpuOps arch_ops;
>>
>> +static Callstack *callstack_new(void)
>> +{
>> +    Callstack *cs = g_new0(Callstack, 1);
>> +    cs->s = g_array_new(false, false, sizeof(CallstackEntry));
>> +    return cs;
>> +}
>> +
>> +static void callstack_free(Callstack *cs)
>> +{
>> +    g_array_free(cs->s, true);
>> +    cs->s = NULL;
>> +    g_free(cs);
>> +}
>> +
>> +static size_t callstack_depth(const Callstack *cs)
>> +{
>> +    return cs->s->len;
>> +}
>> +
>> +static size_t callstack_empty(const Callstack *cs)
>> +{
>> +    return callstack_depth(cs) == 0;
>> +}
>> +
>> +static void callstack_clear(Callstack *cs)
>> +{
>> +    g_array_set_size(cs->s, 0);
>> +}
>> +
>> +static const CallstackEntry *callstack_at(const Callstack *cs, size_t depth)
>> +{
>> +    g_assert(depth > 0);
>> +    g_assert(depth <= callstack_depth(cs));
>> +    return &g_array_index(cs->s, CallstackEntry, depth - 1);
>> +}
>> +
>> +static CallstackEntry callstack_top(const Callstack *cs)
>> +{
>> +    if (callstack_depth(cs) >= 1) {
>> +        return *callstack_at(cs, callstack_depth(cs));
>> +    }
>> +    return (CallstackEntry){};
>> +}
>> +
>> +static CallstackEntry callstack_caller(const Callstack *cs)
>> +{
>> +    if (callstack_depth(cs) >= 2) {
>> +        return *callstack_at(cs, callstack_depth(cs) - 1);
>> +    }
>> +    return (CallstackEntry){};
>> +}
>> +
>> +static void callstack_push(Callstack *cs, CallstackEntry e)
>> +{
>> +    g_array_append_val(cs->s, e);
>> +}
>> +
>> +static CallstackEntry callstack_pop(Callstack *cs)
>> +{
>> +    g_assert(!callstack_empty(cs));
>> +    CallstackEntry e = callstack_top(cs);
>> +    g_array_set_size(cs->s, callstack_depth(cs) - 1);
>> +    return e;
>> +}
>> +
>> static uint64_t cpu_read_register64(Cpu *cpu, struct qemu_plugin_register *reg)
>> {
>>      GByteArray *buf = cpu->buf;
>> @@ -47,6 +122,50 @@ static uint64_t cpu_read_register64(Cpu *cpu, struct qemu_plugin_register *reg)
>>      return *((uint64_t *) buf->data);
>> }
>>
>> +static uint64_t cpu_read_memory64(Cpu *cpu, uint64_t addr)
>> +{
>> +    g_assert(addr);
>> +    GByteArray *buf = cpu->buf;
>> +    g_byte_array_set_size(buf, 0);
>> +    bool read = qemu_plugin_read_memory_vaddr(addr, buf, 8);
>> +    if (!read) {
>> +        return 0;
>> +    }
>> +    g_assert(buf->len == 8);
>> +    return *((uint64_t *) buf->data);
>> +}
>> +
>> +static void cpu_unwind_stack(Cpu *cpu, uint64_t frame_pointer, uint64_t pc)
>> +{
>> +    g_assert(callstack_empty(cpu->cs));
>> +
>> +    #define UNWIND_STACK_MAX_DEPTH 1024
>> +    CallstackEntry unwind[UNWIND_STACK_MAX_DEPTH];
>> +    size_t depth = 0;
>> +    do {
>> +        /* check we don't have an infinite stack */
>> +        for (size_t i = 0; i < depth; ++i) {
>> +            if (frame_pointer == unwind[i].frame_pointer) {
>> +                break;
>> +            }
>> +        }
>> +        CallstackEntry e = {.frame_pointer = frame_pointer, .pc = pc};
>> +        unwind[depth] = e;
>> +        depth++;
>> +        if (frame_pointer) {
>> +            frame_pointer = cpu_read_memory64(cpu, frame_pointer);
>> +        }
>> +        pc = cpu_read_memory64(cpu, frame_pointer + 8); /* read previous lr */
>> +    } while (frame_pointer && pc && depth < UNWIND_STACK_MAX_DEPTH);
>> +    #undef UNWIND_STACK_MAX_DEPTH
>> +
>> +    /* push it from bottom to top */
>> +    while (depth) {
>> +        callstack_push(cpu->cs, unwind[depth - 1]);
>> +        --depth;
>> +    }
>> +}
> 
> Nice.
>

I noticed at some point, during the boot sequence, we had a chain of 
frame pointers that looked like this:
A, B, A, B, A, B...
Thus the need to check we don't have an infinite stack.
It was in code without symbols, but I wonder why we had such a situation 
happening.

>> +
>> static struct qemu_plugin_register *plugin_find_register(const char *name)
>> {
>>      g_autoptr(GArray) regs = qemu_plugin_get_registers();
>> @@ -102,6 +221,43 @@ static CpuOps aarch64_ops = {
>>
>> static void track_callstack(unsigned int cpu_index, void *udata)
>> {
>> +    uint64_t pc = (uintptr_t) udata;
>> +    Cpu *cpu = qemu_plugin_scoreboard_find(score, cpu_index);
>> +    Callstack *cs = cpu->cs;
>> +
>> +    uint64_t fp = cpu->ops.get_frame_pointer(cpu);
>> +    if (!fp && callstack_empty(cs)) {
>> +        /*
>> +         * We simply push current pc. Note that we won't detect symbol change as
>> +         * long as a proper call does not happen.
>> +         */
>> +        callstack_push(cs, (CallstackEntry){.frame_pointer = fp, .pc = pc});
>> +        return;
>> +    }
>> +
>> +    CallstackEntry top = callstack_top(cs);
>> +    if (fp == top.frame_pointer) {
>> +        /* same function */
>> +        return;
>> +    }
>> +
>> +    CallstackEntry caller = callstack_caller(cs);
>> +    if (fp == caller.frame_pointer) {
>> +        /* return */
>> +        callstack_pop(cs);
>> +        return;
>> +    }
>> +
>> +    uint64_t caller_fp = fp ? cpu_read_memory64(cpu, fp) : 0;
>> +    if (caller_fp == top.frame_pointer) {
>> +        /* call */
>> +        callstack_push(cs, (CallstackEntry){.frame_pointer = fp, .pc = pc});
>> +        return;
>> +    }
>> +
>> +    /* discontinuity, exit current stack and unwind new one */
>> +    callstack_clear(cs);
>> +    cpu_unwind_stack(cpu, fp, pc);
>> }
>>
>> static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
>> @@ -139,12 +295,16 @@ static void vcpu_init(qemu_plugin_id_t id, unsigned int vcpu_index)
>>
>>      cpu->ops.init(cpu);
>>      cpu->buf = g_byte_array_new();
>> +
>> +    cpu->cs = callstack_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);
>> +
>> +    callstack_free(cpu->cs);
>>      memset(cpu, 0, sizeof(Cpu));
>> }
>>
>> -- 
>> 2.47.2
>>
> Looks good I think,
> 
> Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>