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
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
* 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
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
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); }