[PATCH v5 6/6] perf arm64: inject missing frames if perf-record used "--call-graph=fp"
Arnaldo Carvalho de Melo
acme at kernel.org
Sat Dec 18 03:35:05 PST 2021
Em Fri, Dec 17, 2021 at 04:01:38PM +0000, James Clark escreveu:
>
>
> On 17/12/2021 15:45, German Gomez wrote:
> > From: Alexandre Truong <alexandre.truong at arm.com>
> >
> > When unwinding using frame pointers on ARM64, the return address of the
> > current function may not have been pushed into the stack when a function
> > was interrupted, which makes perf show an incorrect call graph to the
> > user.
> >
> > Consider the following example program:
> >
> > void leaf() {
> > /* long computation */
> > }
> >
> > void parent() {
> > // (1)
> > leaf();
> > // (2)
> > }
> >
> > ... could be compiled into (using gcc -fno-inline -fno-omit-frame-pointer):
> >
> > leaf:
> > /* long computation */
> > nop
> > ret
> > parent:
> > // (1)
> > stp x29, x30, [sp, -16]!
> > mov x29, sp
> > bl parent
> > nop
> > ldp x29, x30, [sp], 16
> > // (2)
> > ret
> >
> > If the program is interrupted at (1), (2), or any point in "leaf:", the
> > call graph will skip the callers of the current function. We can unwind
> > using the dwarf info and check if the return addr is the same as the LR
> > register, and inject the missing frame into the call graph.
> >
> > Before this patch, the above example shows the following call-graph when
> > recording using "--call-graph fp" mode in ARM64:
> >
> > # Children Self Command Shared Object Symbol
> > # ........ ........ ........ ................ ......................
> > #
> > 99.86% 99.86% program3 program3 [.] leaf
> > |
> > ---_start
> > __libc_start_main
> > main
> > leaf
> >
> > As can be seen, the "parent" function is missing. This is specially
> > problematic in "leaf" because for leaf functions the compiler may always
> > omit pushing the return addr into the stack. After this patch, it shows
> > the correct graph:
> >
> > # Children Self Command Shared Object Symbol
> > # ........ ........ ........ ................ ......................
> > #
> > 99.86% 99.86% program3 program3 [.] leaf
> > |
> > ---_start
> > __libc_start_main
> > main
> > parent
> > leaf
> >
> > Signed-off-by: Alexandre Truong <alexandre.truong at arm.com>
> > Signed-off-by: German Gomez <german.gomez at arm.com>
> > ---
> > tools/perf/util/Build | 1 +
> > .../util/arm64-frame-pointer-unwind-support.c | 63 +++++++++++++++++++
> > .../util/arm64-frame-pointer-unwind-support.h | 10 +++
> > tools/perf/util/machine.c | 19 ++++--
> > tools/perf/util/machine.h | 1 +
> > 5 files changed, 89 insertions(+), 5 deletions(-)
> > create mode 100644 tools/perf/util/arm64-frame-pointer-unwind-support.c
> > create mode 100644 tools/perf/util/arm64-frame-pointer-unwind-support.h
> >
> > diff --git a/tools/perf/util/Build b/tools/perf/util/Build
> > index 2e5bfbb69960..03d4c647bd86 100644
> > --- a/tools/perf/util/Build
> > +++ b/tools/perf/util/Build
> > @@ -1,3 +1,4 @@
> > +perf-y += arm64-frame-pointer-unwind-support.o
> > perf-y += annotate.o
> > perf-y += block-info.o
> > perf-y += block-range.o
> > diff --git a/tools/perf/util/arm64-frame-pointer-unwind-support.c b/tools/perf/util/arm64-frame-pointer-unwind-support.c
> > new file mode 100644
> > index 000000000000..4f5ecf51ed38
> > --- /dev/null
> > +++ b/tools/perf/util/arm64-frame-pointer-unwind-support.c
> > @@ -0,0 +1,63 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include "arm64-frame-pointer-unwind-support.h"
> > +#include "callchain.h"
> > +#include "event.h"
> > +#include "perf_regs.h" // SMPL_REG_MASK
> > +#include "unwind.h"
> > +
> > +#define perf_event_arm_regs perf_event_arm64_regs
> > +#include "../arch/arm64/include/uapi/asm/perf_regs.h"
> > +#undef perf_event_arm_regs
> > +
> > +struct entries {
> > + u64 stack[2];
> > + size_t length;
> > +};
> > +
> > +static bool get_leaf_frame_caller_enabled(struct perf_sample *sample)
> > +{
> > + return callchain_param.record_mode == CALLCHAIN_FP && sample->user_regs.regs
> > + && sample->user_regs.mask & SMPL_REG_MASK(PERF_REG_ARM64_LR);
> > +}
> > +
> > +static int add_entry(struct unwind_entry *entry, void *arg)
> > +{
> > + struct entries *entries = arg;
> > +
> > + entries->stack[entries->length++] = entry->ip;
> > + return 0;
> > +}
> > +
> > +u64 get_leaf_frame_caller_aarch64(struct perf_sample *sample, struct thread *thread, int usr_idx)
> > +{
> > + int ret;
> > + struct entries entries = {};
> > + struct regs_dump old_regs = sample->user_regs;
> > +
> > + if (!get_leaf_frame_caller_enabled(sample))
> > + return 0;
> > +
> > + /*
> > + * If PC and SP are not recorded, get the value of PC from the stack
> > + * and set its mask. SP is not used when doing the unwinding but it
> > + * still needs to be set to prevent failures.
> > + */
> > +
> > + if (!(sample->user_regs.mask & SMPL_REG_MASK(PERF_REG_ARM64_PC))) {
> > + sample->user_regs.cache_mask |= SMPL_REG_MASK(PERF_REG_ARM64_PC);
> > + sample->user_regs.cache_regs[PERF_REG_ARM64_PC] = sample->callchain->ips[usr_idx+1];
> > + }
> > +
> > + if (!(sample->user_regs.mask & SMPL_REG_MASK(PERF_REG_ARM64_SP))) {
> > + sample->user_regs.cache_mask |= SMPL_REG_MASK(PERF_REG_ARM64_SP);
> > + sample->user_regs.cache_regs[PERF_REG_ARM64_SP] = 0;
> > + }
> > +
> > + ret = unwind__get_entries(add_entry, &entries, thread, sample, 2);
> > + sample->user_regs = old_regs;
> > +
> > + if (ret || entries.length != 2)
> > + return ret;
> > +
> > + return callchain_param.order == ORDER_CALLER ? entries.stack[0] : entries.stack[1];
> > +}
> > diff --git a/tools/perf/util/arm64-frame-pointer-unwind-support.h b/tools/perf/util/arm64-frame-pointer-unwind-support.h
> > new file mode 100644
> > index 000000000000..32af9ce94398
> > --- /dev/null
> > +++ b/tools/perf/util/arm64-frame-pointer-unwind-support.h
> > @@ -0,0 +1,10 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __PERF_ARM_FRAME_POINTER_UNWIND_SUPPORT_H
> > +#define __PERF_ARM_FRAME_POINTER_UNWIND_SUPPORT_H
> > +
> > +#include "event.h"
> > +#include "thread.h"
> > +
> > +u64 get_leaf_frame_caller_aarch64(struct perf_sample *sample, struct thread *thread, int user_idx);
> > +
> > +#endif /* __PERF_ARM_FRAME_POINTER_UNWIND_SUPPORT_H */
> > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> > index 3eddad009f78..a00fd6796b35 100644
> > --- a/tools/perf/util/machine.c
> > +++ b/tools/perf/util/machine.c
> > @@ -34,6 +34,7 @@
> > #include "bpf-event.h"
> > #include <internal/lib.h> // page_size
> > #include "cgroup.h"
> > +#include "arm64-frame-pointer-unwind-support.h"
> >
> > #include <linux/ctype.h>
> > #include <symbol/kallsyms.h>
> > @@ -2710,10 +2711,13 @@ static int find_prev_cpumode(struct ip_callchain *chain, struct thread *thread,
> > return err;
> > }
> >
> > -static u64 get_leaf_frame_caller(struct perf_sample *sample __maybe_unused,
> > - struct thread *thread __maybe_unused, int usr_idx __maybe_unused)
> > +static u64 get_leaf_frame_caller(struct perf_sample *sample,
> > + struct thread *thread, int usr_idx)
> > {
> > - return 0;
> > + if (machine__normalize_is(thread->maps->machine, "arm64"))
> > + return get_leaf_frame_caller_aarch64(sample, thread, usr_idx);
> > + else
> > + return 0;
> > }
> >
> > static int thread__resolve_callchain_sample(struct thread *thread,
> > @@ -3114,14 +3118,19 @@ int machine__set_current_tid(struct machine *machine, int cpu, pid_t pid,
> > }
> >
> > /*
> > - * Compares the raw arch string. N.B. see instead perf_env__arch() if a
> > - * normalized arch is needed.
> > + * Compares the raw arch string. N.B. see instead perf_env__arch() or
> > + * machine__normalize_is() if a normalized arch is needed.
> > */
> > bool machine__is(struct machine *machine, const char *arch)
> > {
> > return machine && !strcmp(perf_env__raw_arch(machine->env), arch);
> > }
> >
> > +bool machine__normalize_is(struct machine *machine, const char *arch)
> > +{
> > + return machine && !strcmp(perf_env__arch(machine->env), arch);
> > +}
> > +
>
> I think this function name would be clearer as something like "machine__normalized_is" or
> "machine__normalized_arch_is". The tense is slightly off because it's a test rather than a
> verb.
Agreed, its a question, not a command.
- Arnaldo
> With that change, for the whole set:
>
> Reviewed-by: James Clark <james.clark at arm.com>
>
>
> > int machine__nr_cpus_avail(struct machine *machine)
> > {
> > return machine ? perf_env__nr_cpus_avail(machine->env) : 0;
> > diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h
> > index a143087eeb47..665535153411 100644
> > --- a/tools/perf/util/machine.h
> > +++ b/tools/perf/util/machine.h
> > @@ -208,6 +208,7 @@ static inline bool machine__is_host(struct machine *machine)
> > }
> >
> > bool machine__is(struct machine *machine, const char *arch);
> > +bool machine__normalize_is(struct machine *machine, const char *arch);
> > int machine__nr_cpus_avail(struct machine *machine);
> >
> > struct thread *__machine__findnew_thread(struct machine *machine, pid_t pid, pid_t tid);
> >
--
- Arnaldo
More information about the linux-arm-kernel
mailing list