[4/4] perf tools: determine if LR is the return address
diff mbox series

Message ID 20210122161854.5289-4-alexandre.truong@arm.com
State New, archived
Headers show
Series
  • [1/4] perf tools: record aarch64 registers automatically
Related show

Commit Message

Alexandre Truong Jan. 22, 2021, 4:18 p.m. UTC
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.

Write the following application:

	int a = 10;

	void f2(void)
	{
		for (int i = 0; i < 1000000; i++)
			a *= a;
	}

	void f1()
	{
		f2();
	}

	int main (void)
	{
		f1();
		return 0;
	}

with the following compilation flags:
	gcc -g -fno-omit-frame-pointer -fno-inline -O1

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.

	./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>
Cc: John Garry <john.garry@huawei.com>
Cc: Will Deacon <will@kernel.org>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Leo Yan <leo.yan@linaro.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Kemeng Shi <shikemeng@huawei.com>
Cc: Ian Rogers <irogers@google.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Jin Yao <yao.jin@linux.intel.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Al Grant <al.grant@arm.com>
Cc: James Clark <james.clark@arm.com>
Cc: Wilco Dijkstra <wilco.dijkstra@arm.com>
---
 tools/perf/util/Build                         |  1 +
 .../util/arm-frame-pointer-unwind-support.c   | 43 +++++++++++++++++++
 .../util/arm-frame-pointer-unwind-support.h   |  7 +++
 tools/perf/util/machine.c                     |  9 ++--
 4 files changed, 57 insertions(+), 3 deletions(-)
 create mode 100644 tools/perf/util/arm-frame-pointer-unwind-support.c
 create mode 100644 tools/perf/util/arm-frame-pointer-unwind-support.h

Comments

Jiri Olsa Jan. 24, 2021, 12:05 a.m. UTC | #1
On Fri, Jan 22, 2021 at 04:18:54PM +0000, Alexandre Truong wrote:
> 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.
> 
> Write the following application:
> 
> 	int a = 10;
> 
> 	void f2(void)
> 	{
> 		for (int i = 0; i < 1000000; i++)
> 			a *= a;
> 	}
> 
> 	void f1()
> 	{
> 		f2();
> 	}
> 
> 	int main (void)
> 	{
> 		f1();
> 		return 0;
> 	}
> 
> with the following compilation flags:
> 	gcc -g -fno-omit-frame-pointer -fno-inline -O1
> 
> 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.
> 
> 	./perf record --call-graph fp ./a.out
> 	./perf report
> 
> currently gives the following stack:
> 
> 0xffffea52f361
> _start
> __libc_start_main
> main
> f2

reproduced on x86 as well

> +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 != PERF_REGS_MASK;
> +}
> +
> +static int add_entry(struct unwind_entry *entry, void *arg)
> +{
> +	struct entries *entries = arg;
> +
> +	entries->stack[entries->i++] = entry->ip;
> +	return 0;
> +}
> +
> +u64 get_leaf_frame_caller_aarch64(struct perf_sample *sample, struct thread *thread)
> +{
> +	u64 leaf_frame;
> +	struct entries entries = {{0, 0}, 0};
> +
> +	if (get_leaf_frame_caller_enabled(sample))

the name suggest you'd want to continue if it's true

> +		return 0;
> +
> +	unwind__get_entries(add_entry, &entries, thread, sample, 2);

I'm scratching my head how this unwinds anything, you enabled just
registers, not the stack right? so the unwind code would do just
IP -> LR + 1 shift?

thanks,
jirka

> +	leaf_frame = callchain_param.order == ORDER_CALLER ?
> +		entries.stack[0] : entries.stack[1];
> +
> +	if (leaf_frame + 1 == sample->user_regs.regs[PERF_REG_ARM64_LR])
> +		return sample->user_regs.regs[PERF_REG_ARM64_LR];
> +	return 0;
> +}

SNIP
James Clark Jan. 25, 2021, 9:39 a.m. UTC | #2
On 24/01/2021 02:05, Jiri Olsa wrote:
> On Fri, Jan 22, 2021 at 04:18:54PM +0000, Alexandre Truong wrote:
>> 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.
>>
>> Write the following application:
>>
>> 	int a = 10;
>>
>> 	void f2(void)
>> 	{
>> 		for (int i = 0; i < 1000000; i++)
>> 			a *= a;
>> 	}
>>
>> 	void f1()
>> 	{
>> 		f2();
>> 	}
>>
>> 	int main (void)
>> 	{
>> 		f1();
>> 		return 0;
>> 	}
>>
>> with the following compilation flags:
>> 	gcc -g -fno-omit-frame-pointer -fno-inline -O1
>>
>> 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.
>>
>> 	./perf record --call-graph fp ./a.out
>> 	./perf report
>>
>> currently gives the following stack:
>>
>> 0xffffea52f361
>> _start
>> __libc_start_main
>> main
>> f2
> 
> reproduced on x86 as well
> 
>> +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 != PERF_REGS_MASK;
>> +}
>> +
>> +static int add_entry(struct unwind_entry *entry, void *arg)
>> +{
>> +	struct entries *entries = arg;
>> +
>> +	entries->stack[entries->i++] = entry->ip;
>> +	return 0;
>> +}
>> +
>> +u64 get_leaf_frame_caller_aarch64(struct perf_sample *sample, struct thread *thread)
>> +{
>> +	u64 leaf_frame;
>> +	struct entries entries = {{0, 0}, 0};
>> +
>> +	if (get_leaf_frame_caller_enabled(sample))
> 
> the name suggest you'd want to continue if it's true
> 
>> +		return 0;
>> +
>> +	unwind__get_entries(add_entry, &entries, thread, sample, 2);
> 
> I'm scratching my head how this unwinds anything, you enabled just
> registers, not the stack right? so the unwind code would do just
> IP -> LR + 1 shift?

I think the idea about using libunwind is that the LR might not
be a valid return address. It could be used as a general purpose
register, or just not used at all.

Libunwind should be able to use the dwarf present in the binary to
unwind one frame, as long as nothing stored in the stack is needed.

But now I look at the disassembly for this example, I see that f2()
just has a single 'b' instruction, and not 'bl' so the link register
won't be set. And also 'f1' does store a few things on the stack.
Whether these are needed or not to unwind one frame I'm not sure.

It could be that libunwind is falling back to a frame pointer unwind
mode, which we don't want.

I think it needs further investigation.


James

> 
> thanks,
> jirka
> 
>> +	leaf_frame = callchain_param.order == ORDER_CALLER ?
>> +		entries.stack[0] : entries.stack[1];
>> +
>> +	if (leaf_frame + 1 == sample->user_regs.regs[PERF_REG_ARM64_LR])
>> +		return sample->user_regs.regs[PERF_REG_ARM64_LR];
>> +	return 0;
>> +}
> 
> SNIP
>
James Clark Feb. 8, 2021, 3:39 p.m. UTC | #3
On 22/01/2021 18:18, Alexandre Truong wrote:

> +}
> +
> +static int add_entry(struct unwind_entry *entry, void *arg)
> +{
> +	struct entries *entries = arg;
> +
> +	entries->stack[entries->i++] = entry->ip;
> +	return 0;
> +}
> +
> +u64 get_leaf_frame_caller_aarch64(struct perf_sample *sample, struct thread *thread)
> +{
> +	u64 leaf_frame;
> +	struct entries entries = {{0, 0}, 0};
> +
> +	if (get_leaf_frame_caller_enabled(sample))
> +		return 0;
> +
> +	unwind__get_entries(add_entry, &entries, thread, sample, 2);
> +	leaf_frame = callchain_param.order == ORDER_CALLER ?
> +		entries.stack[0] : entries.stack[1];
> +
> +	if (leaf_frame + 1 == sample->user_regs.regs[PERF_REG_ARM64_LR])
> +		return sample->user_regs.regs[PERF_REG_ARM64_LR];

Hi Alex,

From your other reply about your investigation it looks like the check against PERF_REG_ARM64_LR isn't
required because libunwind won't return a value if it's not correct. Whether it's equal to the LR or not.

And PERF_REG_ARM64_LR points to the instruction _after_ the call site. i.e. where to return to,
not where the call was made from. So just leaf_frame rather than leaf_frame+1 would be more accurate.

I was also looking at unwind_entry in machine.c which is similar to your add_entry function and saw that it
does some extra bits like this:

	if (symbol_conf.hide_unresolved && entry->ms.sym == NULL)
		return 0;

	if (append_inlines(cursor, &entry->ms, entry->ip) == 0)
		return 0;

	/*
	 * Convert entry->ip from a virtual address to an offset in
	 * its corresponding binary.
	 */
	if (entry->ms.map)
		addr = map__map_ip(entry->ms.map, entry->ip);

I have a feeling you will also need to do those on your values returned from libunwind to make it 100%
equivalent.

James

> +	return 0;
> +}
> diff --git a/tools/perf/util/arm-frame-pointer-unwind-support.h b/tools/perf/util/arm-frame-pointer-unwind-support.h
> new file mode 100644
> index 000000000000..16dc03fa9abe
> --- /dev/null
> +++ b/tools/perf/util/arm-frame-pointer-unwind-support.h
> @@ -0,0 +1,7 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __PERF_ARM_FRAME_POINTER_UNWIND_SUPPORT_H
> +#define __PERF_ARM_FRAME_POINTER_UNWIND_SUPPORT_H
> +
> +u64 get_leaf_frame_caller_aarch64(struct perf_sample *sample, struct thread *thread);
> +
> +#endif /* __PERF_ARM_FRAME_POINTER_UNWIND_SUPPORT_H */
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index 40082d70eec1..bc6147e46c89 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 "arm-frame-pointer-unwind-support.h"
>  
>  #include <linux/ctype.h>
>  #include <symbol/kallsyms.h>
> @@ -2671,10 +2672,12 @@ 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)
> +static u64 get_leaf_frame_caller(struct perf_sample *sample, struct thread *thread)
>  {
> -	return 0;
> +	if (strncmp(thread->maps->machine->env->arch, "aarch64", 7) == 0)
> +		return get_leaf_frame_caller_aarch64(sample, thread);
> +	else
> +		return 0;
>  }
>  
>  static int thread__resolve_callchain_sample(struct thread *thread,
>
Alexandre Truong Feb. 10, 2021, 12:05 p.m. UTC | #4
On 2/8/21 3:39 PM, James Clark wrote:
>
>
> On 22/01/2021 18:18, Alexandre Truong wrote:
>
>> +}
>> +
>> +static int add_entry(struct unwind_entry *entry, void *arg)
>> +{
>> +    struct entries *entries = arg;
>> +
>> +    entries->stack[entries->i++] = entry->ip;
>> +    return 0;
>> +}
>> +
>> +u64 get_leaf_frame_caller_aarch64(struct perf_sample *sample, struct thread *thread)
>> +{
>> +    u64 leaf_frame;
>> +    struct entries entries = {{0, 0}, 0};
>> +
>> +    if (get_leaf_frame_caller_enabled(sample))
>> +            return 0;
>> +
>> +    unwind__get_entries(add_entry, &entries, thread, sample, 2);
>> +    leaf_frame = callchain_param.order == ORDER_CALLER ?
>> +            entries.stack[0] : entries.stack[1];
>> +
>> +    if (leaf_frame + 1 == sample->user_regs.regs[PERF_REG_ARM64_LR])
>> +            return sample->user_regs.regs[PERF_REG_ARM64_LR];
>
> Hi Alex,
>
>  From your other reply about your investigation it looks like the check against PERF_REG_ARM64_LR isn't
> required because libunwind won't return a value if it's not correct. Whether it's equal to the LR or not.
>
> And PERF_REG_ARM64_LR points to the instruction _after_ the call site. i.e. where to return to,
> not where the call was made from. So just leaf_frame rather than leaf_frame+1 would be more accurate.
>
> I was also looking at unwind_entry in machine.c which is similar to your add_entry function and saw that it
> does some extra bits like this:
>
>       if (symbol_conf.hide_unresolved && entry->ms.sym == NULL)
>               return 0;
>
>       if (append_inlines(cursor, &entry->ms, entry->ip) == 0)
>               return 0;
>
>       /*
>        * Convert entry->ip from a virtual address to an offset in
>        * its corresponding binary.
>        */
>       if (entry->ms.map)
>               addr = map__map_ip(entry->ms.map, entry->ip);
>
> I have a feeling you will also need to do those on your values returned from libunwind to make it 100%
> equivalent.
>
> James
>

Hi James,

Thanks for your reply.

The check against PERF_REG_ARM64_LR is indeed not required and I can
check if libunwind goes successfully.

I am going to follow up with a v2 of the patch applying these changes.

I think the bits you mentioned don't need to be added because this check
is already done in add_callchain_ip() called afterwards in machine.c :

     if (symbol_conf.hide_unresolved && entry->ms.sym == NULL)
         return 0;

For the second one, I don't think it needs to be added either because
append_inlines() appends ip on the cursor which is also already done by
add_callchain_ip().

For the last one, the conversion from a virtual address to a binary one
isn't required.


Also for the expansion to all platforms, it doesn't work on x86 so I'll
leave it just for arm for now.

Regards,

Alexandre

>> +    return 0;
>> +}
>> diff --git a/tools/perf/util/arm-frame-pointer-unwind-support.h b/tools/perf/util/arm-frame-pointer-unwind-support.h
>> new file mode 100644
>> index 000000000000..16dc03fa9abe
>> --- /dev/null
>> +++ b/tools/perf/util/arm-frame-pointer-unwind-support.h
>> @@ -0,0 +1,7 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef __PERF_ARM_FRAME_POINTER_UNWIND_SUPPORT_H
>> +#define __PERF_ARM_FRAME_POINTER_UNWIND_SUPPORT_H
>> +
>> +u64 get_leaf_frame_caller_aarch64(struct perf_sample *sample, struct thread *thread);
>> +
>> +#endif /* __PERF_ARM_FRAME_POINTER_UNWIND_SUPPORT_H */
>> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
>> index 40082d70eec1..bc6147e46c89 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 "arm-frame-pointer-unwind-support.h"
>>
>>   #include <linux/ctype.h>
>>   #include <symbol/kallsyms.h>
>> @@ -2671,10 +2672,12 @@ 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)
>> +static u64 get_leaf_frame_caller(struct perf_sample *sample, struct thread *thread)
>>   {
>> -    return 0;
>> +    if (strncmp(thread->maps->machine->env->arch, "aarch64", 7) == 0)
>> +            return get_leaf_frame_caller_aarch64(sample, thread);
>> +    else
>> +            return 0;
>>   }
>>
>>   static int thread__resolve_callchain_sample(struct thread *thread,
>>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Patch
diff mbox series

diff --git a/tools/perf/util/Build b/tools/perf/util/Build
index e2563d0154eb..2009d5f02972 100644
--- a/tools/perf/util/Build
+++ b/tools/perf/util/Build
@@ -1,3 +1,4 @@ 
+perf-y += arm-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/arm-frame-pointer-unwind-support.c b/tools/perf/util/arm-frame-pointer-unwind-support.c
new file mode 100644
index 000000000000..2901ae2917e9
--- /dev/null
+++ b/tools/perf/util/arm-frame-pointer-unwind-support.c
@@ -0,0 +1,43 @@ 
+// SPDX-License-Identifier: GPL-2.0
+#include "../arch/arm64/include/uapi/asm/perf_regs.h"
+#include "arch/arm64/include/perf_regs.h"
+#include "event.h"
+#include "arm-frame-pointer-unwind-support.h"
+#include "callchain.h"
+#include "unwind.h"
+
+struct entries {
+	u64 stack[2];
+	int i;
+};
+
+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 != PERF_REGS_MASK;
+}
+
+static int add_entry(struct unwind_entry *entry, void *arg)
+{
+	struct entries *entries = arg;
+
+	entries->stack[entries->i++] = entry->ip;
+	return 0;
+}
+
+u64 get_leaf_frame_caller_aarch64(struct perf_sample *sample, struct thread *thread)
+{
+	u64 leaf_frame;
+	struct entries entries = {{0, 0}, 0};
+
+	if (get_leaf_frame_caller_enabled(sample))
+		return 0;
+
+	unwind__get_entries(add_entry, &entries, thread, sample, 2);
+	leaf_frame = callchain_param.order == ORDER_CALLER ?
+		entries.stack[0] : entries.stack[1];
+
+	if (leaf_frame + 1 == sample->user_regs.regs[PERF_REG_ARM64_LR])
+		return sample->user_regs.regs[PERF_REG_ARM64_LR];
+	return 0;
+}
diff --git a/tools/perf/util/arm-frame-pointer-unwind-support.h b/tools/perf/util/arm-frame-pointer-unwind-support.h
new file mode 100644
index 000000000000..16dc03fa9abe
--- /dev/null
+++ b/tools/perf/util/arm-frame-pointer-unwind-support.h
@@ -0,0 +1,7 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __PERF_ARM_FRAME_POINTER_UNWIND_SUPPORT_H
+#define __PERF_ARM_FRAME_POINTER_UNWIND_SUPPORT_H
+
+u64 get_leaf_frame_caller_aarch64(struct perf_sample *sample, struct thread *thread);
+
+#endif /* __PERF_ARM_FRAME_POINTER_UNWIND_SUPPORT_H */
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 40082d70eec1..bc6147e46c89 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 "arm-frame-pointer-unwind-support.h"
 
 #include <linux/ctype.h>
 #include <symbol/kallsyms.h>
@@ -2671,10 +2672,12 @@  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)
+static u64 get_leaf_frame_caller(struct perf_sample *sample, struct thread *thread)
 {
-	return 0;
+	if (strncmp(thread->maps->machine->env->arch, "aarch64", 7) == 0)
+		return get_leaf_frame_caller_aarch64(sample, thread);
+	else
+		return 0;
 }
 
 static int thread__resolve_callchain_sample(struct thread *thread,