[PATCH 1/2] perf tools: Compare dso's also when comparing symbols

[PATCH 1/2] perf tools: Compare dso's also when comparing symbols

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

Linus reported that sometimes 'perf report -s symbol' exits without
any message on TUI.  David and Jiri found that it's because it failed
to add a hist entry due to an invalid symbol length.

It turns out that sorting by symbol (address) was broken since it only
compares symbol addresses.  The symbol address is a relative address
within a dso thus just checking its address can result in merging
unrelated symbols together.  Fix it by checking dso before comparing
symbol address.

Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/sort.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 32c56377e008..1f9821db9e77 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -182,9 +182,19 @@ static int64_t _sort__sym_cmp(struct symbol *sym_l, struct symbol *sym_r)
 static int64_t
 sort__sym_cmp(struct hist_entry *left, struct hist_entry *right)
 {
+	int64_t ret;
+
 	if (!left->ms.sym && !right->ms.sym)
 		return right->level - left->level;
 
+	/*
+	 * comparing symbol address alone is not enough since it's a
+	 * relative address within a dso.
+	 */
+	ret = sort__dso_cmp(left, right);
+	if (ret != 0)
+		return ret;
+
 	return _sort__sym_cmp(left->ms.sym, right->ms.sym);
 }
 
-- 
1.7.11.7

[RFC/PATCH 2/2] perf tools: Sort dso using pointers

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

The dso's in a perf session are maintained machine-wide so same dso is
shared by hist entries.  Thus just checking pointer should be enough
to comparing dso's.

It may change behavior of 'perf report -s dso' when there're two or
more dso's that have same basename - the dso's which have same name
used to merged to one unless -v option was given.  But I don't think
it's a big problem. ;)

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/sort.c | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 1f9821db9e77..70658a0834e3 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -114,20 +114,8 @@ static int64_t _sort__dso_cmp(struct map *map_l, struct map *map_r)
 {
 	struct dso *dso_l = map_l ? map_l->dso : NULL;
 	struct dso *dso_r = map_r ? map_r->dso : NULL;
-	const char *dso_name_l, *dso_name_r;
 
-	if (!dso_l || !dso_r)
-		return cmp_null(dso_l, dso_r);
-
-	if (verbose) {
-		dso_name_l = dso_l->long_name;
-		dso_name_r = dso_r->long_name;
-	} else {
-		dso_name_l = dso_l->short_name;
-		dso_name_r = dso_r->short_name;
-	}
-
-	return strcmp(dso_name_l, dso_name_r);
+	return (int64_t)(dso_l - dso_r);
 }
 
 static int64_t
-- 
1.7.11.7

Re: [PATCH 1/2] perf tools: Compare dso's also when comparing symbols

[Ingo Molnar]

* Namhyung Kim <namhyung@kernel.org> wrote:

> From: Namhyung Kim <namhyung.kim@lge.com>
> 
> Linus reported that sometimes 'perf report -s symbol' exits without any 
> message on TUI.  David and Jiri found that it's because it failed to add 
> a hist entry due to an invalid symbol length.

Btw., 'exit without any messages' is something that should be fixed 
separately as well I suspect.

>  static int64_t
>  sort__sym_cmp(struct hist_entry *left, struct hist_entry *right)
>  {
> +	int64_t ret;
> +

Btw., this file should go back to u64/s64 like the kernel and most of perf 
does. (A few int64_t uses slipped into other places as well, I suspect 
they should be fixed.)

Thanks,

	Ingo

Re: [PATCH 1/2] perf tools: Compare dso's also when comparing symbols

[Namhyung Kim]

On Tue, 15 Oct 2013 08:08:29 +0200, Ingo Molnar wrote:
> * Namhyung Kim <namhyung@kernel.org> wrote:
>
>> From: Namhyung Kim <namhyung.kim@lge.com>
>> 
>> Linus reported that sometimes 'perf report -s symbol' exits without any 
>> message on TUI.  David and Jiri found that it's because it failed to add 
>> a hist entry due to an invalid symbol length.
>
> Btw., 'exit without any messages' is something that should be fixed 
> separately as well I suspect.

Sure.  I believe acme is gonna work on it. :)

>
>>  static int64_t
>>  sort__sym_cmp(struct hist_entry *left, struct hist_entry *right)
>>  {
>> +	int64_t ret;
>> +
>
> Btw., this file should go back to u64/s64 like the kernel and most of perf 
> does. (A few int64_t uses slipped into other places as well, I suspect 
> they should be fixed.)

Okay, will send a separate patch for the fix.

Thanks,
Namhyung

[tip:perf/core] perf tools: Compare dso' s also when comparing symbols

[tip-bot for Namhyung Kim]

Commit-ID:  09600e0f9ebb06235b852a646a3644b7d4a71aca
Gitweb:     http://git.kernel.org/tip/09600e0f9ebb06235b852a646a3644b7d4a71aca
Author:     Namhyung Kim <namhyung.kim@lge.com>
AuthorDate: Tue, 15 Oct 2013 11:01:56 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 21 Oct 2013 17:33:23 -0300

perf tools: Compare dso's also when comparing symbols

Linus reported that sometimes 'perf report -s symbol' exits without any
message on TUI.  David and Jiri found that it's because it failed to add
a hist entry due to an invalid symbol length.

It turns out that sorting by symbol (address) was broken since it only
compares symbol addresses.  The symbol address is a relative address
within a dso thus just checking its address can result in merging
unrelated symbols together.  Fix it by checking dso before comparing
symbol address.

Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1381802517-18812-1-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/sort.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 32c5637..1f9821d 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -182,9 +182,19 @@ static int64_t _sort__sym_cmp(struct symbol *sym_l, struct symbol *sym_r)
 static int64_t
 sort__sym_cmp(struct hist_entry *left, struct hist_entry *right)
 {
+	int64_t ret;
+
 	if (!left->ms.sym && !right->ms.sym)
 		return right->level - left->level;
 
+	/*
+	 * comparing symbol address alone is not enough since it's a
+	 * relative address within a dso.
+	 */
+	ret = sort__dso_cmp(left, right);
+	if (ret != 0)
+		return ret;
+
 	return _sort__sym_cmp(left->ms.sym, right->ms.sym);
 }