Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1410166 > unrolled thread

[PATCH v7 10/14] perf tools: Check the target platform before assigning unwind methods

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v7 10/14] perf tools: Check the target platform before assigning unwind methods He Kuang <hekuang@huawei.com> - 2016-05-31 13:30 +0200
    Re: [PATCH v7 10/14] perf tools: Check the target platform before  assigning unwind methods Jiri Olsa <jolsa@redhat.com> - 2016-06-01 10:50 +0200
      Re: [PATCH v7 10/14] perf tools: Check the target platform before  assigning unwind methods Hekuang <hekuang@huawei.com> - 2016-06-01 12:10 +0200
    Re: [PATCH v7 10/14] perf tools: Check the target platform before  assigning unwind methods Jiri Olsa <jolsa@redhat.com> - 2016-06-01 10:50 +0200

#1410166 — [PATCH v7 10/14] perf tools: Check the target platform before assigning unwind methods

Currently, perf script uses host unwind methods to parse perf.data
callchain info regardless of the target architecture. So we get wrong
result without any warnings when unwinding callchains of x86(32-bit)
on x86(64-bit) machine.

This patch adds extra step that check the target platform before
assigning unwind methods. In the latter patches in this series, we can
use this info to assign the right unwind methods for supported
platforms.

Signed-off-by: He Kuang <hekuang@huawei.com>
---
 tools/perf/util/thread.c           |  2 +-
 tools/perf/util/unwind-libunwind.c | 30 +++++++++++++++++++++++++++---
 tools/perf/util/unwind.h           |  5 +++--
 3 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
index 0615ef3..6f55c4d3 100644
--- a/tools/perf/util/thread.c
+++ b/tools/perf/util/thread.c
@@ -203,7 +203,7 @@ int thread__insert_map(struct thread *thread, struct map *map)
 	map_groups__fixup_overlappings(thread->mg, map, stderr);
 	map_groups__insert(thread->mg, map);
 
-	return unwind__prepare_access(thread);
+	return unwind__prepare_access(thread, map);
 }
 
 static int thread__clone_map_groups(struct thread *thread,
diff --git a/tools/perf/util/unwind-libunwind.c b/tools/perf/util/unwind-libunwind.c
index 787adbb..e183390 100644
--- a/tools/perf/util/unwind-libunwind.c
+++ b/tools/perf/util/unwind-libunwind.c
@@ -1,5 +1,8 @@
 #include "unwind.h"
 #include "thread.h"
+#include "session.h"
+#include "debug.h"
+#include "arch/common.h"
 
 struct unwind_libunwind_ops __weak *local_unwind_libunwind_ops;
 
@@ -9,11 +12,32 @@ void unwind__register_ops(struct thread *thread,
 	thread->unwind_libunwind_ops = ops;
 }
 
-int unwind__prepare_access(struct thread *thread)
+int unwind__prepare_access(struct thread *thread, struct map *map)
 {
-	unwind__register_ops(thread, local_unwind_libunwind_ops);
+	const char *arch;
+	enum dso_type dso_type;
+	struct unwind_libunwind_ops *ops = local_unwind_libunwind_ops;
 
-	return thread->unwind_libunwind_ops->prepare_access(thread);
+	if (!thread->mg->machine->env)
+		return 0;
+
+	dso_type = dso__type(map->dso, thread->mg->machine);
+	if (dso_type == DSO__TYPE_UNKNOWN)
+		return 0;
+
+	if (thread->addr_space)
+		pr_debug("unwind: thread map already set, 64bit is %d, dso=%s\n",
+			 dso_type == DSO__TYPE_64BIT, map->dso->name);
+
+	arch = normalize_arch(thread->mg->machine->env->arch);
+	pr_debug("unwind: target platform=%s\n", arch);
+
+	unwind__register_ops(thread, ops);
+
+	if (thread->unwind_libunwind_ops)
+		return thread->unwind_libunwind_ops->prepare_access(thread);
+	else
+		return 0;
 }
 
 void unwind__flush_access(struct thread *thread)
diff --git a/tools/perf/util/unwind.h b/tools/perf/util/unwind.h
index 25001be..dda2156 100644
--- a/tools/perf/util/unwind.h
+++ b/tools/perf/util/unwind.h
@@ -30,13 +30,14 @@ int unwind__get_entries(unwind_entry_cb_t cb, void *arg,
 /* libunwind specific */
 #ifdef HAVE_LIBUNWIND_SUPPORT
 int libunwind__arch_reg_id(int regnum);
-int unwind__prepare_access(struct thread *thread);
+int unwind__prepare_access(struct thread *thread, struct map *map);
 void unwind__flush_access(struct thread *thread);
 void unwind__finish_access(struct thread *thread);
 void unwind__register_ops(struct thread *thread,
 			  struct unwind_libunwind_ops *ops);
 #else
-static inline int unwind__prepare_access(struct thread *thread __maybe_unused)
+static inline int unwind__prepare_access(struct thread *thread __maybe_unused,
+					 struct map *map __maybe_unused)
 {
 	return 0;
 }
-- 
1.8.5.2

[toc] | [next] | [standalone]

#1410965 — Re: [PATCH v7 10/14] perf tools: Check the target platform before assigning unwind methods

On Tue, May 31, 2016 at 11:19:08AM +0000, He Kuang wrote:

SNIP

> -int unwind__prepare_access(struct thread *thread)
> +int unwind__prepare_access(struct thread *thread, struct map *map)
>  {
> -	unwind__register_ops(thread, local_unwind_libunwind_ops);
> +	const char *arch;
> +	enum dso_type dso_type;
> +	struct unwind_libunwind_ops *ops = local_unwind_libunwind_ops;
>  
> -	return thread->unwind_libunwind_ops->prepare_access(thread);
> +	if (!thread->mg->machine->env)
> +		return 0;
> +
> +	dso_type = dso__type(map->dso, thread->mg->machine);
> +	if (dso_type == DSO__TYPE_UNKNOWN)
> +		return 0;
> +
> +	if (thread->addr_space)
> +		pr_debug("unwind: thread map already set, 64bit is %d, dso=%s\n",
> +			 dso_type == DSO__TYPE_64BIT, map->dso->name);

should we leave once the address space is set? resseting it over
again seems like memory leak unless I'm missing something...

also this check should be probably the first thing we do in here

thanks,
jirka

> +
> +	arch = normalize_arch(thread->mg->machine->env->arch);
> +	pr_debug("unwind: target platform=%s\n", arch);
> +
> +	unwind__register_ops(thread, ops);
> +
> +	if (thread->unwind_libunwind_ops)
> +		return thread->unwind_libunwind_ops->prepare_access(thread);
> +	else
> +		return 0;
>  }

SNIP

[toc] | [prev] | [next] | [standalone]

#1411056 — Re: [PATCH v7 10/14] perf tools: Check the target platform before assigning unwind methods

在 2016/6/1 16:40, Jiri Olsa 写道:
> On Tue, May 31, 2016 at 11:19:08AM +0000, He Kuang wrote:
>
> SNIP
>
>> -int unwind__prepare_access(struct thread *thread)
>> +int unwind__prepare_access(struct thread *thread, struct map *map)
>>   {
>> -	unwind__register_ops(thread, local_unwind_libunwind_ops);
>> +	const char *arch;
>> +	enum dso_type dso_type;
>> +	struct unwind_libunwind_ops *ops = local_unwind_libunwind_ops;
>>   
>> -	return thread->unwind_libunwind_ops->prepare_access(thread);
>> +	if (!thread->mg->machine->env)
>> +		return 0;
>> +
>> +	dso_type = dso__type(map->dso, thread->mg->machine);
>> +	if (dso_type == DSO__TYPE_UNKNOWN)
>> +		return 0;
>> +
>> +	if (thread->addr_space)
>> +		pr_debug("unwind: thread map already set, 64bit is %d, dso=%s\n",
>> +			 dso_type == DSO__TYPE_64BIT, map->dso->name);
> should we leave once the address space is set? resseting it over
> again seems like memory leak unless I'm missing something...
>
> also this check should be probably the first thing we do in here

Sure, I must miss the return statement here.
> thanks,
> jirka
>
>> +
>> +	arch = normalize_arch(thread->mg->machine->env->arch);
>> +	pr_debug("unwind: target platform=%s\n", arch);
>> +
>> +	unwind__register_ops(thread, ops);
>> +
>> +	if (thread->unwind_libunwind_ops)
>> +		return thread->unwind_libunwind_ops->prepare_access(thread);
>> +	else
>> +		return 0;
>>   }
> SNIP
>

[toc] | [prev] | [next] | [standalone]

#1410973 — Re: [PATCH v7 10/14] perf tools: Check the target platform before assigning unwind methods

On Tue, May 31, 2016 at 11:19:08AM +0000, He Kuang wrote:

SNIP

> -int unwind__prepare_access(struct thread *thread)
> +int unwind__prepare_access(struct thread *thread, struct map *map)
>  {
> -	unwind__register_ops(thread, local_unwind_libunwind_ops);
> +	const char *arch;
> +	enum dso_type dso_type;
> +	struct unwind_libunwind_ops *ops = local_unwind_libunwind_ops;
>  
> -	return thread->unwind_libunwind_ops->prepare_access(thread);
> +	if (!thread->mg->machine->env)
> +		return 0;
> +
> +	dso_type = dso__type(map->dso, thread->mg->machine);
> +	if (dso_type == DSO__TYPE_UNKNOWN)
> +		return 0;
> +
> +	if (thread->addr_space)
> +		pr_debug("unwind: thread map already set, 64bit is %d, dso=%s\n",
> +			 dso_type == DSO__TYPE_64BIT, map->dso->name);
> +
> +	arch = normalize_arch(thread->mg->machine->env->arch);
> +	pr_debug("unwind: target platform=%s\n", arch);
> +
> +	unwind__register_ops(thread, ops);
> +
> +	if (thread->unwind_libunwind_ops)
> +		return thread->unwind_libunwind_ops->prepare_access(thread);
> +	else
> +		return 0;

why do you need to check for thread->unwind_libunwind_ops != NULL?
it's all set at this point right?

jirka

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web