Messages in this thread | | | Subject | Re: [PATCH v4 6/6] perf tools: determine if LR is the return address | From | German Gomez <> | Date | Fri, 17 Dec 2021 11:57:10 +0000 |
| |
Hi Mark,
Thanks for your review comments
On 15/12/2021 16:33, Mark Rutland wrote: > Hi, > > On Wed, Dec 15, 2021 at 03:11:38PM +0000, German Gomez wrote: >> From: Alexandre Truong <alexandre.truong@arm.com> >> >> On arm64 and frame pointer mode (e.g: perf record --callgraph fp), >> use dwarf unwind info to check if the link register is the return >> address in order to inject it to the frame pointer stack. > This series looks good overall, but as a general note the commit messages are a > bit hard to read because they jump into implementation details of the patch > (i.e. the change the patch makes) before explaining the problem (i.e. what the > patch is trying to solve). > > It would be nice to have a short introduction, e.g.
Thanks for the suggestion! I'll run through the logs to see if I can improve them.
> > When unwinding using frame pointers on arm64, the return address of the > current leaf function may be missed. The return address of a leaf function > may live in the LR and/or a frame record (and the location can change within > a function), so it is necessary to use DWARF to identify where to look for > the return address at any given point during a function. > > For example: > > unsigned long foo(unsigned long i) > { > i += 2; > i += 5; > } > > ... could be compiled as: > > foo: > // return addr in LR > add x0, x0, #2 > // return addr in LR > stp x29, x30, [SP, #-16]! > // return addr in LR > mov x29, sp > // return addr in LR *and* frame record > add x0, x0, #5 > // return addr in LR *and* frame record > ldp x29, x30, [sp], #16 > // return addr in LR > ret > >> Write the following application: >> >> int a = 10; >> >> void f2(void) >> { >> for (int i = 0; i < 1000000; i++) >> a *= a; >> } >> >> void f1() >> { >> for (int i = 0; i < 10; i++) >> f2(); >> } >> >> int main(void) >> { >> f1(); >> return 0; >> } >> >> with the following compilation flags: >> gcc -fno-omit-frame-pointer -fno-inline -O2 >> >> The compiler omits the frame pointer for f2 on arm. This is a problem >> with any leaf call, for example an application with many different >> calls to malloc() would always omit the calling frame, even if it >> can be determined. > I think the wording here is slightly misleading. For f2, the compiler *doesn't > create a frame record*, but the frame pointer (to the caller's frame record) > remains and is not omitted. > > Also, I think it's woth noting (as per the example I gave above) this applies > to *any* function which is the current leaf function, regardless of whether > that function creates a frame record at some point. For example, if `f1` is > interrupted before it creates its own frame record (or after it destroys the > frame record), the FP will point at the record created by `main` (containing > the caller of main), and `main` itself will be missing from the unwind as it > will only exist in the LR.
I see! I hadn't considered this. I guess it's not as likely to happen but it's worth noting indeed.
> >> ./perf record --call-graph fp ./a.out >> ./perf report >> >> currently gives the following stack: >> >> 0xffffea52f361 >> _start >> __libc_start_main >> main >> f2 >> >> After this change, perf report correctly shows f1() calling f2(), >> even though it was missing from the frame pointer unwind: >> >> ./perf report >> >> 0xffffea52f361 >> _start >> __libc_start_main >> main >> f1 >> f2 >> >> Signed-off-by: Alexandre Truong <alexandre.truong@arm.com> >> Signed-off-by: German Gomez <german.gomez@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. >> + */ > To prevent failures where? Is this something libunwind requires?
Admittedly I haven't look very deep into libunwind, but SP seems to go ignored when getting the last 2 entries only, so here we set it to any value.
Thanks, German
| |