[BUG] perf: buildid not managed properly when cmd path is relative

[BUG] perf: buildid not managed properly when cmd path is relative

[Stephane Eranian]

Hi,

I ran into another problem while running more tests with
perf record, perf buildid-list.

I do the following:

$ perf record foo/noploop 5
$ perf buildid-list
54b1e7cc3cf52e0db255aab44ce7538eb62655b8 [kernel.kallsyms]
875ae61623e89f408b425ca0486a9ec99e3ac73e
/home/eranian/perfmon/official/tip/build/tools/perf/foo/noploop

I know I have samples in noploop:
$ perf report -D
...
0x10a0 [0x20]: PERF_RECORD_SAMPLE(IP, 2): 14721/14721: 0x4006d6 period: 2351576
 ... thread: noploop:14721
 ...... dso: ./foo/noploop

But if I ask with buildid-list (like per-archive is doing) then I get:

$ perf buildid-list --with-hits
54b1e7cc3cf52e0db255aab44ce7538eb62655b8 [kernel.kallsyms]
0000000000000000000000000000000000000000 ./foo/noploop

The builid is bogus for noploop and it is relative path not full anymore.

I instrumented __dsos__fprintf_buildid() and I get:
hit=0 name=/home/eranian/perfmon/official/tip/build/tools/perf/foo/noploop
hit=1 name=./foo/noploop
0000000000000000000000000000000000000000 ./foo/noploop

So it looks like when cmd is relative there are two entries but the one
that counts the hits is the one with relative path. But is does not have
the buildid, the full path entry does.

This is an issue because perf-archive only packages the
content of .debug with hits.

The problem does not exists when cmd are found from PATH.

Re: [BUG] perf: buildid not managed properly when cmd path is relative

[Arnaldo Carvalho de Melo]

Em Thu, May 27, 2010 at 03:46:16PM +0200, Stephane Eranian escreveu:
> I ran into another problem while running more tests with
> perf record, perf buildid-list.
> 
> I do the following:
> 
> $ perf record foo/noploop 5
> $ perf buildid-list
> 54b1e7cc3cf52e0db255aab44ce7538eb62655b8 [kernel.kallsyms]
> 875ae61623e89f408b425ca0486a9ec99e3ac73e
> /home/eranian/perfmon/official/tip/build/tools/perf/foo/noploop
> 
> I know I have samples in noploop:
> $ perf report -D
> ...
> 0x10a0 [0x20]: PERF_RECORD_SAMPLE(IP, 2): 14721/14721: 0x4006d6 period: 2351576
>  ... thread: noploop:14721
>  ...... dso: ./foo/noploop
> 
> But if I ask with buildid-list (like per-archive is doing) then I get:
> 
> $ perf buildid-list --with-hits
> 54b1e7cc3cf52e0db255aab44ce7538eb62655b8 [kernel.kallsyms]
> 0000000000000000000000000000000000000000 ./foo/noploop
> 
> The builid is bogus for noploop and it is relative path not full anymore.
> 
> I instrumented __dsos__fprintf_buildid() and I get:
> hit=0 name=/home/eranian/perfmon/official/tip/build/tools/perf/foo/noploop
> hit=1 name=./foo/noploop
> 0000000000000000000000000000000000000000 ./foo/noploop
> 
> So it looks like when cmd is relative there are two entries but the one
> that counts the hits is the one with relative path. But is does not have
> the buildid, the full path entry does.
> 
> This is an issue because perf-archive only packages the
> content of .debug with hits.
> 
> The problem does not exists when cmd are found from PATH.

I reproduced the problem, and yeah, annoying, will work on it, thanks
for the report.

- Arnaldo

Re: [BUG] perf: buildid not managed properly when cmd path is relative

[Arnaldo Carvalho de Melo]

Em Thu, May 27, 2010 at 03:46:16PM +0200, Stephane Eranian escreveu:
> I ran into another problem while running more tests with perf record,
> perf buildid-list.
 
> I do the following:
 
> $ perf record foo/noploop 5 ; perf buildid-list
> 54b1e7cc3cf52e0db255aab44ce7538eb62655b8 [kernel.kallsyms]
> 875ae61623e89f408b425ca0486a9ec99e3ac73e
> /home/eranian/perfmon/official/tip/build/tools/perf/foo/noploop
> 
> I know I have samples in noploop:
> $ perf report -D
> 0x10a0 [0x20]: PERF_RECORD_SAMPLE(IP, 2): 14721/14721: 0x4006d6 period: 2351576
>  ... thread: noploop:14721
>  ...... dso: ./foo/noploop

> But if I ask with buildid-list (like per-archive is doing) then I get:
> $ perf buildid-list --with-hits
> 54b1e7cc3cf52e0db255aab44ce7538eb62655b8 [kernel.kallsyms]
> 0000000000000000000000000000000000000000 ./foo/noploop

> The builid is bogus for noploop and it is relative path not full anymore.

Hi Stephane,

	Can you please try the following patch?

Thanks in advance,

- Arnaldo

>From 9d146ff3c994634c529396442a4a96b0e58809e2 Mon Sep 17 00:00:00 2001
From: Arnaldo Carvalho de Melo <acme@redhat.com>
Date: Tue, 1 Jun 2010 12:37:05 -0300
Subject: [PATCH 1/1] perf buildid-list: Fix --with-hits event processing
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

When we use plain 'perf buildid-list' we use only what is in the buildid
table in the perf.data header. And those have absolute pathnames because
at 'perf record' time we used __perf_session__process_events and that
doesn't sets up the path shortening code in map__new() that happens if
symbol_conf.full_paths is false, the default.

On the other hand, when we use 'perf buildid-list --with-hits' we
process all the events using perf_session__process_events, adding
entries to the global DSO list _after_ removing the current directory
from the DSO name, for presentation purposes.

Because of that we end up having two entries in the DSO list when
recording events for binaries using relative pathnames.

Fix it minimally by setting symbol_conf.full_paths to true when marking
the DSOs with hits in 'perf buildid-list --with-hits', as used by 'perf
archive'

Right fix longer term is to shorten the path only at presentation time.
Will be done for 2.6.36.

Reported-by: Stephane Eranian <eranian@google.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: Frédéric Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Tom Zanussi <tzanussi@gmail.com>
LKML-Reference: <new-submission>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-buildid-list.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/tools/perf/builtin-buildid-list.c b/tools/perf/builtin-buildid-list.c
index 44a47e1..9989072 100644
--- a/tools/perf/builtin-buildid-list.c
+++ b/tools/perf/builtin-buildid-list.c
@@ -43,8 +43,10 @@ static int __cmd_buildid_list(void)
 	if (session == NULL)
 		return -1;
 
-	if (with_hits)
+	if (with_hits) {
+		symbol_conf.full_paths = true;
 		perf_session__process_events(session, &build_id__mark_dso_hit_ops);
+	}
 
 	perf_session__fprintf_dsos_buildid(session, stdout, with_hits);
 
-- 
1.6.5.2

Re: [BUG] perf: buildid not managed properly when cmd path is relative

[Stephane Eranian]

Hi,

You patch does seem to fix the problem.

However, I think there may be another issue, maybe
caused by the patch.

If you monitor a program WITHOUT buildids, and you run

$ perf record tmp/foo
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.077 MB perf.data (~3370 samples) ]

$  ./perf buildid-list
86200ab2bca285344a55156a079debbbe172bcc5 [kernel.kallsyms]

foo does not appear, yet we know it got samples.

$ ./perf buildid-list --with-hits
86200ab2bca285344a55156a079debbbe172bcc5 [kernel.kallsyms]
0000000000000000000000000000000000000000 /home/eranian/tmp/foo

With the --with-hits option, I get more output than without.Supposedly
no option means 'prints all entries', including those with hits or
without buildids.


On Tue, Jun 1, 2010 at 8:38 PM, Arnaldo Carvalho de Melo
<acme@infradead.org> wrote:
> Em Thu, May 27, 2010 at 03:46:16PM +0200, Stephane Eranian escreveu:
>> I ran into another problem while running more tests with perf record,
>> perf buildid-list.
>
>> I do the following:
>
>> $ perf record foo/noploop 5 ; perf buildid-list
>> 54b1e7cc3cf52e0db255aab44ce7538eb62655b8 [kernel.kallsyms]
>> 875ae61623e89f408b425ca0486a9ec99e3ac73e
>> /home/eranian/perfmon/official/tip/build/tools/perf/foo/noploop
>>
>> I know I have samples in noploop:
>> $ perf report -D
>> 0x10a0 [0x20]: PERF_RECORD_SAMPLE(IP, 2): 14721/14721: 0x4006d6 period: 2351576
>>  ... thread: noploop:14721
>>  ...... dso: ./foo/noploop
>
>> But if I ask with buildid-list (like per-archive is doing) then I get:
>> $ perf buildid-list --with-hits
>> 54b1e7cc3cf52e0db255aab44ce7538eb62655b8 [kernel.kallsyms]
>> 0000000000000000000000000000000000000000 ./foo/noploop
>
>> The builid is bogus for noploop and it is relative path not full anymore.
>
> Hi Stephane,
>
>        Can you please try the following patch?
>
> Thanks in advance,
>
> - Arnaldo
>
> From 9d146ff3c994634c529396442a4a96b0e58809e2 Mon Sep 17 00:00:00 2001
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> Date: Tue, 1 Jun 2010 12:37:05 -0300
> Subject: [PATCH 1/1] perf buildid-list: Fix --with-hits event processing
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> When we use plain 'perf buildid-list' we use only what is in the buildid
> table in the perf.data header. And those have absolute pathnames because
> at 'perf record' time we used __perf_session__process_events and that
> doesn't sets up the path shortening code in map__new() that happens if
> symbol_conf.full_paths is false, the default.
>
> On the other hand, when we use 'perf buildid-list --with-hits' we
> process all the events using perf_session__process_events, adding
> entries to the global DSO list _after_ removing the current directory
> from the DSO name, for presentation purposes.
>
> Because of that we end up having two entries in the DSO list when
> recording events for binaries using relative pathnames.
>
> Fix it minimally by setting symbol_conf.full_paths to true when marking
> the DSOs with hits in 'perf buildid-list --with-hits', as used by 'perf
> archive'
>
> Right fix longer term is to shorten the path only at presentation time.
> Will be done for 2.6.36.
>
> Reported-by: Stephane Eranian <eranian@google.com>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Frédéric Weisbecker <fweisbec@gmail.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Mike Galbraith <efault@gmx.de>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Stephane Eranian <eranian@google.com>
> Cc: Tom Zanussi <tzanussi@gmail.com>
> LKML-Reference: <new-submission>
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
>  tools/perf/builtin-buildid-list.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/tools/perf/builtin-buildid-list.c b/tools/perf/builtin-buildid-list.c
> index 44a47e1..9989072 100644
> --- a/tools/perf/builtin-buildid-list.c
> +++ b/tools/perf/builtin-buildid-list.c
> @@ -43,8 +43,10 @@ static int __cmd_buildid_list(void)
>        if (session == NULL)
>                return -1;
>
> -       if (with_hits)
> +       if (with_hits) {
> +               symbol_conf.full_paths = true;
>                perf_session__process_events(session, &build_id__mark_dso_hit_ops);
> +       }
>
>        perf_session__fprintf_dsos_buildid(session, stdout, with_hits);
>
> --
> 1.6.5.2
>
>



-- 
Stephane Eranian  | EMEA Software Engineering
Google France | 38 avenue de l'Opéra | 75002 Paris
Tel : +33 (0) 1 42 68 53 00
This email may be confidential or privileged. If you received this
communication by mistake, please
don't forward it to anyone else, please erase all copies and
attachments, and please let me know that
it went to the wrong person. Thanks

Re: [BUG] perf: buildid not managed properly when cmd path is relative

[Arnaldo Carvalho de Melo]

Em Tue, Jun 01, 2010 at 10:05:24PM +0200, Stephane Eranian escreveu:
> Hi,
> 
> You patch does seem to fix the problem.

thanks for confirming.
 
> However, I think there may be another issue, maybe
> caused by the patch.
> 
> If you monitor a program WITHOUT buildids, and you run
> 
> $ perf record tmp/foo
> [ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 0.077 MB perf.data (~3370 samples) ]
> 
> $  ./perf buildid-list
> 86200ab2bca285344a55156a079debbbe172bcc5 [kernel.kallsyms]
> 
> foo does not appear, yet we know it got samples.
> 
> $ ./perf buildid-list --with-hits
> 86200ab2bca285344a55156a079debbbe172bcc5 [kernel.kallsyms]
> 0000000000000000000000000000000000000000 /home/eranian/tmp/foo
> 
> With the --with-hits option, I get more output than without.Supposedly
> no option means 'prints all entries', including those with hits or
> without buildids.

Right, I noticed that too, but concentrated on fixing the problem that
was clearly a bug, will add this to my TODO list as I agree this is
inconsistent and/or incomplete behaviour.

Will add a Tested-by: you and push it to Ingo via perf/urgent.

Thanks for testing,

- Arnaldo