perf tools: fix dc8d6ab typo prohibiting perf top symbol annotation

PATCH In review Mike Galbraith 2 participants 4 messages Jan 22, 2010 perf events

lore

Share & tools

lore

Mike Galbraith

author

2010-01-22 06:39 UTC #0 lore raw

PATCH 1/1 perf tools: fix dc8d6ab typo prohibiting perf top symbol annotation

perf tools: fix dc8d6ab typo prohibiting perf top symbol annotation

Signed-off-by Mike Galbraith <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Arnaldo Carvalho de Melo <[email protected]>

LKML-Reference: <new-submission>

tools/perf/util/symbol.c +1 -1

index 6f30fe1..af2e224 100644
@@ -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]"));

Arnaldo Carvalho de Melo

2010-01-22 11:22 UTC #1 lore raw

Em Fri, Jan 22, 2010 at 07:39:39AM +0100, Mike Galbraith escreveu:

Quoted 14 lines

perf tools: fix dc8d6ab typo prohibiting perf top symbol annotation
Signed-off-by Mike Galbraith <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
LKML-Reference: <new-submission>
tools/perf/util/symbol.c +0 -0
index 6f30fe1..af2e224 100644

tools/perf/util/symbol.c do_kallsyms: · line 1675

Quoted 8 lines

tools/perf/util/symbol.c +1 -1

@@ -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

Arnaldo Carvalho de Melo

2010-01-22 11:45 UTC #2 lore raw

Em Fri, Jan 22, 2010 at 09:22:31AM -0200, Arnaldo Carvalho de Melo escreveu:

Quoted 17 lines

Em Fri, Jan 22, 2010 at 07:39:39AM +0100, Mike Galbraith escreveu:
Quoted 16 lines
perf tools: fix dc8d6ab typo prohibiting perf top symbol annotation
Signed-off-by Mike Galbraith <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
LKML-Reference: <new-submission>
tools/perf/util/symbol.c +0 -0
index 6f30fe1..af2e224 100644
@@ -1669,7 +1669,7 @@ do_kallsyms:
 	free(kallsyms_allocated_filename);
 

Quoted 10 lines

tools/perf/util/symbol.c do_kallsyms: · line 1675
Quoted 6 lines
tools/perf/util/symbol.c +1 -1
 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

Mike Galbraith

author

2010-01-22 14:13 UTC #3 lore raw

Quoted 3 lines · Arnaldo Carvalho de Melo

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

Quoted 4 lines

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