[patch] perf tools: fix dc8d6ab typo prohibiting perf top symbol annotation

[patch] perf tools: fix dc8d6ab typo prohibiting perf top symbol annotation

[Mike Galbraith]

perf tools: fix dc8d6ab typo prohibiting perf top symbol annotation

Signed-off-by: Mike Galbraith <efault@gmx.de>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
LKML-Reference: <new-submission>

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 6f30fe1..af2e224 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1669,7 +1669,7 @@ do_kallsyms:
 	free(kallsyms_allocated_filename);
 
 out_try_fixup:
-	if (err > 0) {
+	if (err <= 0) {
 out_fixup:
 		if (kallsyms_filename == NULL)
 			dso__set_long_name(self, strdup("[kernel.kallsyms]"));

Re: [patch] perf tools: fix dc8d6ab typo prohibiting perf top symbol annotation

[Arnaldo Carvalho de Melo]

Em Fri, Jan 22, 2010 at 07:39:39AM +0100, Mike Galbraith escreveu:
> 
> perf tools: fix dc8d6ab typo prohibiting perf top symbol annotation
> 
> Signed-off-by: Mike Galbraith <efault@gmx.de>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> LKML-Reference: <new-submission>
> 
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 6f30fe1..af2e224 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1669,7 +1669,7 @@ do_kallsyms:
>  	free(kallsyms_allocated_filename);
>  
>  out_try_fixup:
> -	if (err > 0) {
> +	if (err <= 0) {
>  out_fixup:
>  		if (kallsyms_filename == NULL)
>  			dso__set_long_name(self, strdup("[kernel.kallsyms]"));

Strange, we need to fixup the map only if we found some symbol, so all
the routines there return the number of symbols found or -1 for errors,
investigating perf top symbol annotation now...

- Arnaldo

Re: [patch] perf tools: fix dc8d6ab typo prohibiting perf top symbol annotation

[Arnaldo Carvalho de Melo]

Em Fri, Jan 22, 2010 at 09:22:31AM -0200, Arnaldo Carvalho de Melo escreveu:
> Em Fri, Jan 22, 2010 at 07:39:39AM +0100, Mike Galbraith escreveu:
> > 
> > perf tools: fix dc8d6ab typo prohibiting perf top symbol annotation
> > 
> > Signed-off-by: Mike Galbraith <efault@gmx.de>
> > Cc: Ingo Molnar <mingo@elte.hu>
> > Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> > LKML-Reference: <new-submission>
> > 
> > diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> > index 6f30fe1..af2e224 100644
> > --- a/tools/perf/util/symbol.c
> > +++ b/tools/perf/util/symbol.c
> > @@ -1669,7 +1669,7 @@ do_kallsyms:
> >  	free(kallsyms_allocated_filename);
> >  
> >  out_try_fixup:
> > -	if (err > 0) {
> > +	if (err <= 0) {
> >  out_fixup:
> >  		if (kallsyms_filename == NULL)
> >  			dso__set_long_name(self, strdup("[kernel.kallsyms]"));
> 
> Strange, we need to fixup the map only if we found some symbol, so all
> the routines there return the number of symbols found or -1 for errors,
> investigating perf top symbol annotation now...

So the sequence is, right now, without Kirill's patch to autofind the
right vmlinux: one needs to specify a --vmlinux /path/to/vmlinux, and
that, dc8d6ab, made the only file to be considered.

This means that if dso__load_vmlinux for this specified file fails, we
won't have kernel symbols, and in my test it failed because the buildid
for the running kernel (/sys/kernel/notesr) didn't match the one in the
specified vmlinux file...

But that is only appearing if --verbose is used and even then, because
we use such a simple interface to refresh the screen, gets lost, I only
saw this while in gdb:

build_id in /lib/modules/2.6.33-rc4-tip+/build/vmlinux is
d04f055cae38f29e0bdef3610743acac9e80738c while expected is
74f9930ee94475b6b3238caf3725a50d59cb994b, ignoring it

I'll fix this now by making perf top fail and print this, even with it
being able to still be useful to see kernel modules and userspace
symbols. Longer term we need to move it to a ncurses interface and have
a status bar where such errors can be given proper "sticky" visibility.

Just a sec...

- Arnaldo

Re: [patch] perf tools: fix dc8d6ab typo prohibiting perf top symbol annotation

[Mike Galbraith]

On Fri, 2010-01-22 at 09:45 -0200, Arnaldo Carvalho de Melo wrote:

> So the sequence is, right now, without Kirill's patch to autofind the
> right vmlinux: one needs to specify a --vmlinux /path/to/vmlinux, and
> that, dc8d6ab, made the only file to be considered.

ala perf top -k /lib/modules/2.6.33-tip-smpx/build/vmlinux

> This means that if dso__load_vmlinux for this specified file fails, we
> won't have kernel symbols, and in my test it failed because the buildid
> for the running kernel (/sys/kernel/notesr) didn't match the one in the
> specified vmlinux file...

(gdb) s
1602            if (symbol_conf.vmlinux_name != NULL) {
(gdb) s
1603                    err = dso__load_vmlinux(self, map, session,
(gdb) print symbol_conf.vmlinux_name
$1 = 0x7fffffffe21e "/lib/modules/2.6.33-tip-smpx/build/vmlinux"
(gdb) s
dso__load_vmlinux (self=0xecf480, map=0xecf590, session=0xecf2f0,
    vmlinux=0x7fffffffe21e "/lib/modules/2.6.33-tip-smpx/build/vmlinux", filter=0x4341ea <symbol_filter>)
    at util/symbol.c:1543
1543    {
(gdb) fin
Run till exit from #0  dso__load_vmlinux (self=0xecf480, map=0xecf590, session=0xecf2f0,
    vmlinux=0x7fffffffe21e "/lib/modules/2.6.33-tip-smpx/build/vmlinux", filter=0x4341ea <symbol_filter>)
    at util/symbol.c:1543
0x000000000044e61f in dso__load_kernel_sym (self=0xecf480, map=0xecf590, session=0xecf2f0, filter=0x4341ea <symbol_filter>)
    at util/symbol.c:1603
1603                    err = dso__load_vmlinux(self, map, session,
Value returned is $2 = 15008
(gdb)

Not failing here, but we jump down to out_try_fixup, and since
kallsyms_filename is NULL, change name to [kernel.kallsyms], and
TheBadThing follows, which made me think this was supposed to be an
error path.  Jumping to that conclusion made the bad thing go away :)

	-Mike