From: Kirill Smelkov <[email protected]>
First, for programs and prelinked libraries, annotate code was fooled by
objdump output IPs (src->eip in the code) being wrongly converted to
absolute IPs. In such case there were no conversion needed, but in
src->eip = strtoull(src->line, NULL, 16);
src->eip = map->unmap_ip(map, src->eip); // = eip + map->start - map->pgoff
we were reading absolute address from objdump (e.g. 8048604) and then
almost doubling it, because eip & map->start are approximately close for
small programs.
Needless to say, that later, in record_precise_ip() there was no
matching with real runtime IPs.
And second, like with `perf annotate` the problem with non-prelinked
*.so was that we were doing rip -> objdump address conversion wrong.
Also, because unlike `perf annotate`, `perf top` code does annotation based on
absolute IPs for performance reasons(*), new helper for mapping objdump
addresse to IP is introduced.
(*) we get samples info in absolute IPs, and since we do lots of
hit-testing on absolute IPs at runtime in record_precise_ip(), it's
better to convert objdump addresses to IPs once and do no conversion
at runtime.
I also had to fix how objdump output is parsed (with hardcoded 8/16
characters format, which was inappropriate for ET_DYN dsos with small
addresses like '4ac')
Also note, that not all objdump output lines has associtated IPs, e.g.
look at source lines here:
000004ac <my_strlen>:
extern "C"
int my_strlen(const char *s)
4ac: 55 push %ebp
4ad: 89 e5 mov %esp,%ebp
4af: 83 ec 10 sub $0x10,%esp
{
int len = 0;
4b2: c7 45 fc 00 00 00 00 movl $0x0,-0x4(%ebp)
4b9: eb 08 jmp 4c3 <my_strlen+0x17>
while (*s) {
++len;
4bb: 83 45 fc 01 addl $0x1,-0x4(%ebp)
++s;
4bf: 83 45 08 01 addl $0x1,0x8(%ebp)
So we mark them with eip=0, and ignore such lines in annotate lookup
code.
Commiter note: one hunk of this patch was applied by Mike in 57d8188.
Cc: Mike Galbraith <[email protected]>
Signed-off-by: Kirill Smelkov <[email protected]>
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/builtin-top.c | 18 +++++++++---------
tools/perf/util/map.c | 8 ++++++++
tools/perf/util/map.h | 4 ++--
3 files changed, 19 insertions(+), 11 deletions(-)
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index e4156bc..befa57e 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -216,7 +216,7 @@ static void parse_source(struct sym_entry *syme)
while (!feof(file)) {
struct source_line *src;
size_t dummy = 0;
- char *c;
+ char *c, *sep;
src = malloc(sizeof(struct source_line));
assert(src != NULL);
@@ -235,14 +235,11 @@ static void parse_source(struct sym_entry *syme)
*source->lines_tail = src;
source->lines_tail = &src->next;
- if (strlen(src->line)>8 && src->line[8] == ':') {
- src->eip = strtoull(src->line, NULL, 16);
- src->eip = map->unmap_ip(map, src->eip);
- }
- if (strlen(src->line)>8 && src->line[16] == ':') {
- src->eip = strtoull(src->line, NULL, 16);
- src->eip = map->unmap_ip(map, src->eip);
- }
+ src->eip = strtoull(src->line, &sep, 16);
+ if (*sep == ':')
+ src->eip = map__objdump_2ip(map, src->eip);
+ else /* this line has no ip info (e.g. source line) */
+ src->eip = 0;
}
pclose(file);
out_assign:
@@ -277,6 +274,9 @@ static void record_precise_ip(struct sym_entry *syme, int counter, u64 ip)
goto out_unlock;
for (line = syme->src->lines; line; line = line->next) {
+ /* skip lines without IP info */
+ if (line->eip == 0)
+ continue;
if (line->eip == ip) {
line->count[counter]++;
break;
diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index af5805f..138e3cb 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -222,3 +222,11 @@ u64 map__rip_2objdump(struct map *map, u64 rip)
rip;
return addr;
}
+
+u64 map__objdump_2ip(struct map *map, u64 addr)
+{
+ u64 ip = map->dso->adjust_symbols ?
+ addr :
+ map->unmap_ip(map, addr); /* RIP -> IP */
+ return ip;
+}
diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h
index 9cee9c7..86f77cb 100644
--- a/tools/perf/util/map.h
+++ b/tools/perf/util/map.h
@@ -61,9 +61,9 @@ static inline u64 identity__map_ip(struct map *map __used, u64 ip)
}
-/* rip -> addr suitable for passing to `objdump --start-address=` */
+/* rip/ip <-> addr suitable for passing to `objdump --start-address=` */
u64 map__rip_2objdump(struct map *map, u64 rip);
-
+u64 map__objdump_2ip(struct map *map, u64 addr);
struct symbol;
struct mmap_event;
--
1.6.2.5
From: Arnaldo Carvalho de Melo <[email protected]>
Because we may have aliases, like __GI___strcoll_l in
/lib64/libc-2.10.2.so that appears in objdump as:
$ objdump --start-address=0x0000003715a86420 \
--stop-address=0x0000003715a872dc -dS /lib64/libc-2.10.2.so
0000003715a86420 <__strcoll_l>:
3715a86420: 55 push %rbp
3715a86421: 48 89 e5 mov %rsp,%rbp
3715a86424: 41 57 push %r15
[root@doppio linux-2.6-tip]#
So look for the address exactly at the start of the line instead so that
annotation can work for in these cases.
Cc: Frédéric Weisbecker <[email protected]>
Cc: Kirill Smelkov <[email protected]>
Cc: Mike Galbraith <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Paul Mackerras <[email protected]>
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/builtin-top.c | 15 ++++++++-------
1 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index befa57e..c72ab50 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -202,10 +202,9 @@ static void parse_source(struct sym_entry *syme)
len = sym->end - sym->start;
sprintf(command,
- "objdump --start-address=0x%016Lx "
- "--stop-address=0x%016Lx -dS %s",
- map__rip_2objdump(map, sym->start),
- map__rip_2objdump(map, sym->end), path);
+ "objdump --start-address=%#0*Lx --stop-address=%#0*Lx -dS %s",
+ BITS_PER_LONG / 4, map__rip_2objdump(map, sym->start),
+ BITS_PER_LONG / 4, map__rip_2objdump(map, sym->end), path);
file = popen(command, "r");
if (!file)
@@ -292,13 +291,15 @@ static void lookup_sym_source(struct sym_entry *syme)
{
struct symbol *symbol = sym_entry__symbol(syme);
struct source_line *line;
- char pattern[PATH_MAX];
+ const size_t pattern_len = BITS_PER_LONG / 4 + 2;
+ char pattern[pattern_len + 1];
- sprintf(pattern, "<%s>:", symbol->name);
+ sprintf(pattern, "%0*Lx <", BITS_PER_LONG / 4,
+ map__rip_2objdump(syme->map, symbol->start));
pthread_mutex_lock(&syme->src->lock);
for (line = syme->src->lines; line; line = line->next) {
- if (strstr(line->line, pattern)) {
+ if (memcmp(line->line, pattern, pattern_len) == 0) {
syme->src->source = line;
break;
}
--
1.6.2.5
Commit-ID: ee11b90b12eb1ec25e1044bac861e90bfd19ec9e
Gitweb: http://git.kernel.org/tip/ee11b90b12eb1ec25e1044bac861e90bfd19ec9e
Author: Kirill Smelkov <[email protected]>
AuthorDate: Sun, 7 Feb 2010 11:46:15 -0200
Committer: Ingo Molnar <[email protected]>
CommitDate: Sun, 7 Feb 2010 17:30:20 +0100
perf top: Fix annotate for userspace
First, for programs and prelinked libraries, annotate code was
fooled by objdump output IPs (src->eip in the code) being
wrongly converted to absolute IPs. In such case there were no
conversion needed, but in
src->eip = strtoull(src->line, NULL, 16);
src->eip = map->unmap_ip(map, src->eip); // = eip + map->start - map->pgoff
we were reading absolute address from objdump (e.g. 8048604) and
then almost doubling it, because eip & map->start are
approximately close for small programs.
Needless to say, that later, in record_precise_ip() there was no
matching with real runtime IPs.
And second, like with `perf annotate` the problem with
non-prelinked *.so was that we were doing rip -> objdump address
conversion wrong.
Also, because unlike `perf annotate`, `perf top` code does
annotation based on absolute IPs for performance reasons(*), new
helper for mapping objdump addresse to IP is introduced.
(*) we get samples info in absolute IPs, and since we do lots of
hit-testing on absolute IPs at runtime in record_precise_ip(), it's
better to convert objdump addresses to IPs once and do no conversion
at runtime.
I also had to fix how objdump output is parsed (with hardcoded
8/16 characters format, which was inappropriate for ET_DYN dsos
with small addresses like '4ac')
Also note, that not all objdump output lines has associtated
IPs, e.g. look at source lines here:
000004ac <my_strlen>:
extern "C"
int my_strlen(const char *s)
4ac: 55 push %ebp
4ad: 89 e5 mov %esp,%ebp
4af: 83 ec 10 sub $0x10,%esp
{
int len = 0;
4b2: c7 45 fc 00 00 00 00 movl $0x0,-0x4(%ebp)
4b9: eb 08 jmp 4c3 <my_strlen+0x17>
while (*s) {
++len;
4bb: 83 45 fc 01 addl $0x1,-0x4(%ebp)
++s;
4bf: 83 45 08 01 addl $0x1,0x8(%ebp)
So we mark them with eip=0, and ignore such lines in annotate
lookup code.
Signed-off-by: Kirill Smelkov <[email protected]>
[ Note: one hunk of this patch was applied by Mike in 57d8188 ]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
Cc: Mike Galbraith <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
tools/perf/builtin-top.c | 18 +++++++++---------
tools/perf/util/map.c | 8 ++++++++
tools/perf/util/map.h | 4 ++--
3 files changed, 19 insertions(+), 11 deletions(-)
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index e4156bc..befa57e 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -216,7 +216,7 @@ static void parse_source(struct sym_entry *syme)
while (!feof(file)) {
struct source_line *src;
size_t dummy = 0;
- char *c;
+ char *c, *sep;
src = malloc(sizeof(struct source_line));
assert(src != NULL);
@@ -235,14 +235,11 @@ static void parse_source(struct sym_entry *syme)
*source->lines_tail = src;
source->lines_tail = &src->next;
- if (strlen(src->line)>8 && src->line[8] == ':') {
- src->eip = strtoull(src->line, NULL, 16);
- src->eip = map->unmap_ip(map, src->eip);
- }
- if (strlen(src->line)>8 && src->line[16] == ':') {
- src->eip = strtoull(src->line, NULL, 16);
- src->eip = map->unmap_ip(map, src->eip);
- }
+ src->eip = strtoull(src->line, &sep, 16);
+ if (*sep == ':')
+ src->eip = map__objdump_2ip(map, src->eip);
+ else /* this line has no ip info (e.g. source line) */
+ src->eip = 0;
}
pclose(file);
out_assign:
@@ -277,6 +274,9 @@ static void record_precise_ip(struct sym_entry *syme, int counter, u64 ip)
goto out_unlock;
for (line = syme->src->lines; line; line = line->next) {
+ /* skip lines without IP info */
+ if (line->eip == 0)
+ continue;
if (line->eip == ip) {
line->count[counter]++;
break;
diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index af5805f..138e3cb 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -222,3 +222,11 @@ u64 map__rip_2objdump(struct map *map, u64 rip)
rip;
return addr;
}
+
+u64 map__objdump_2ip(struct map *map, u64 addr)
+{
+ u64 ip = map->dso->adjust_symbols ?
+ addr :
+ map->unmap_ip(map, addr); /* RIP -> IP */
+ return ip;
+}
diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h
index 9cee9c7..86f77cb 100644
--- a/tools/perf/util/map.h
+++ b/tools/perf/util/map.h
@@ -61,9 +61,9 @@ static inline u64 identity__map_ip(struct map *map __used, u64 ip)
}
-/* rip -> addr suitable for passing to `objdump --start-address=` */
+/* rip/ip <-> addr suitable for passing to `objdump --start-address=` */
u64 map__rip_2objdump(struct map *map, u64 rip);
-
+u64 map__objdump_2ip(struct map *map, u64 addr);
struct symbol;
struct mmap_event;
On Sun, Feb 07, 2010 at 04:33:40PM +0000, tip-bot for Kirill Smelkov wrote:
> Commit-ID: ee11b90b12eb1ec25e1044bac861e90bfd19ec9e
> Gitweb: http://git.kernel.org/tip/ee11b90b12eb1ec25e1044bac861e90bfd19ec9e
> Author: Kirill Smelkov <[email protected]>
> AuthorDate: Sun, 7 Feb 2010 11:46:15 -0200
> Committer: Ingo Molnar <[email protected]>
> CommitDate: Sun, 7 Feb 2010 17:30:20 +0100
>
> perf top: Fix annotate for userspace
[...]
Arnaldo, Ingo, thanks for finally applying it
( though shouldn't, AuthorDate be ``Fri, 8 Jan 2010 15:23:08 +0300'',
just like in my post for this patch
http://marc.info/?l=linux-kernel&m=126295508002536&w=2 ?)
----
Also, could you please apply one last cosmetic patch from my original
series
http://marc.info/?l=linux-kernel&m=126295502702502&w=2
That would logically complete my holiday journey to perf land.
For convenience I'm providing this one last patch here (applies to
tip/master cleanly).
Thanks beforehand,
Kirill
From: Kirill Smelkov <[email protected]>
Subject: [PATCH 2/6] perf top: align help text on keys
Date: Fri, 8 Jan 2010 15:23:05 +0300
Mapped keys:
[d] display refresh delay. (2)
[e] display entries (lines). (46)
[f] profile display filter (count). (5)
[F] annotate display filter (percent). (5%)
[s] annotate symbol. (NULL)
[S] stop annotation.
[K] hide kernel_symbols symbols. (no)
[U] hide user symbols. (no)
[z] toggle sample zeroing. (0)
[qQ] quit.
instead of
Mapped keys:
[d] display refresh delay. (2)
[e] display entries (lines). (46)
[f] profile display filter (count). (5)
[F] annotate display filter (percent). (5%)
[s] annotate symbol. (NULL)
[S] stop annotation.
[K] hide kernel_symbols symbols. (no)
[U] hide user symbols. (no)
[z] toggle sample zeroing. (0)
[qQ] quit.
Signed-off-by: Kirill Smelkov <[email protected]>
---
tools/perf/builtin-top.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 9c7de93..9dc8070 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -704,7 +704,7 @@ static void print_mapped_keys(void)
fprintf(stdout, "\t[w] toggle display weighted/count[E]r. \t(%d)\n", display_weighted ? 1 : 0);
fprintf(stdout,
- "\t[K] hide kernel_symbols symbols. \t(%s)\n",
+ "\t[K] hide kernel_symbols symbols. \t(%s)\n",
hide_kernel_symbols ? "yes" : "no");
fprintf(stdout,
"\t[U] hide user symbols. \t(%s)\n",
--
1.6.6.79.gd514e.dirty
Em Fri, Feb 12, 2010 at 07:20:59PM +0300, Kirill Smelkov escreveu:
> On Sun, Feb 07, 2010 at 04:33:40PM +0000, tip-bot for Kirill Smelkov wrote:
> > Commit-ID: ee11b90b12eb1ec25e1044bac861e90bfd19ec9e
> > Gitweb: http://git.kernel.org/tip/ee11b90b12eb1ec25e1044bac861e90bfd19ec9e
> > Author: Kirill Smelkov <[email protected]>
> > AuthorDate: Sun, 7 Feb 2010 11:46:15 -0200
> > Committer: Ingo Molnar <[email protected]>
> > CommitDate: Sun, 7 Feb 2010 17:30:20 +0100
> >
> > perf top: Fix annotate for userspace
>
> [...]
>
> Arnaldo, Ingo, thanks for finally applying it
>
> ( though shouldn't, AuthorDate be ``Fri, 8 Jan 2010 15:23:08 +0300'',
> just like in my post for this patch
> http://marc.info/?l=linux-kernel&m=126295508002536&w=2 ?)
>
> ----
>
> Also, could you please apply one last cosmetic patch from my original
> series
>
> http://marc.info/?l=linux-kernel&m=126295502702502&w=2
>
>
> That would logically complete my holiday journey to perf land.
>
>
> For convenience I'm providing this one last patch here (applies to
> tip/master cleanly).
Ingo, you may take this one now or wait till my next series, your
choice,
Acked-by: Arnaldo Carvalho de Melo <[email protected]>
> Thanks beforehand,
> Kirill
>
>
> From: Kirill Smelkov <[email protected]>
> Subject: [PATCH 2/6] perf top: align help text on keys
> Date: Fri, 8 Jan 2010 15:23:05 +0300
>
> Mapped keys:
> [d] display refresh delay. (2)
> [e] display entries (lines). (46)
> [f] profile display filter (count). (5)
> [F] annotate display filter (percent). (5%)
> [s] annotate symbol. (NULL)
> [S] stop annotation.
> [K] hide kernel_symbols symbols. (no)
> [U] hide user symbols. (no)
> [z] toggle sample zeroing. (0)
> [qQ] quit.
>
> instead of
>
> Mapped keys:
> [d] display refresh delay. (2)
> [e] display entries (lines). (46)
> [f] profile display filter (count). (5)
> [F] annotate display filter (percent). (5%)
> [s] annotate symbol. (NULL)
> [S] stop annotation.
> [K] hide kernel_symbols symbols. (no)
> [U] hide user symbols. (no)
> [z] toggle sample zeroing. (0)
> [qQ] quit.
>
> Signed-off-by: Kirill Smelkov <[email protected]>
> ---
> tools/perf/builtin-top.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index 9c7de93..9dc8070 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -704,7 +704,7 @@ static void print_mapped_keys(void)
> fprintf(stdout, "\t[w] toggle display weighted/count[E]r. \t(%d)\n", display_weighted ? 1 : 0);
>
> fprintf(stdout,
> - "\t[K] hide kernel_symbols symbols. \t(%s)\n",
> + "\t[K] hide kernel_symbols symbols. \t(%s)\n",
> hide_kernel_symbols ? "yes" : "no");
> fprintf(stdout,
> "\t[U] hide user symbols. \t(%s)\n",
> --
> 1.6.6.79.gd514e.dirty
Commit-ID: 1a72cfa6856e7d58e049c42c6e6a789669478479
Gitweb: http://git.kernel.org/tip/1a72cfa6856e7d58e049c42c6e6a789669478479
Author: Kirill Smelkov <[email protected]>
AuthorDate: Fri, 12 Feb 2010 19:20:59 +0300
Committer: Ingo Molnar <[email protected]>
CommitDate: Sun, 14 Feb 2010 09:46:02 +0100
perf top: Fix help text alignment
Print this:
Mapped keys:
[d] display refresh delay. (2)
[e] display entries (lines). (46)
[f] profile display filter (count). (5)
[F] annotate display filter (percent). (5%)
[s] annotate symbol. (NULL)
[S] stop annotation.
[K] hide kernel_symbols symbols. (no)
[U] hide user symbols. (no)
[z] toggle sample zeroing. (0)
[qQ] quit.
instead of:
Mapped keys:
[d] display refresh delay. (2)
[e] display entries (lines). (46)
[f] profile display filter (count). (5)
[F] annotate display filter (percent). (5%)
[s] annotate symbol. (NULL)
[S] stop annotation.
[K] hide kernel_symbols symbols. (no)
[U] hide user symbols. (no)
[z] toggle sample zeroing. (0)
[qQ] quit.
Signed-off-by: Kirill Smelkov <[email protected]>
Acked-by: Arnaldo Carvalho de Melo <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Mike Galbraith <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
tools/perf/builtin-top.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index ddc584b..4b91d8c 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -705,7 +705,7 @@ static void print_mapped_keys(void)
fprintf(stdout, "\t[w] toggle display weighted/count[E]r. \t(%d)\n", display_weighted ? 1 : 0);
fprintf(stdout,
- "\t[K] hide kernel_symbols symbols. \t(%s)\n",
+ "\t[K] hide kernel_symbols symbols. \t(%s)\n",
hide_kernel_symbols ? "yes" : "no");
fprintf(stdout,
"\t[U] hide user symbols. \t(%s)\n",