[PATCH] perf lock: fix segfault with info subcommand following move to libtraceevent

[PATCH] perf lock: fix segfault with info subcommand following move to libtraceevent

[David Ahern]

Running 'perf lock info' on a "random" perf.data file crashes:

0  0x00000000004a5618 in __parse_common (pevent=0x0, data=0x0, size=0xa0, offset=0x9c, name=
    0x556e02 "common_type") at /opt/sw/ahern/kernels/kernel.git/tools/lib/traceevent/event-parse.c:2851
1  0x00000000004a56a7 in trace_parse_common_type (pevent=0x0, data=0x0)
    at /opt/sw/ahern/kernels/kernel.git/tools/lib/traceevent/event-parse.c:2861
2  0x00000000004a7e42 in pevent_data_type (pevent=0x0, rec=0x7fffffffddf0)
    at /opt/sw/ahern/kernels/kernel.git/tools/lib/traceevent/event-parse.c:3952
3  0x0000000000473f37 in trace_parse_common_type (data=0x0) at util/trace-event-parse.c:158
4  0x00000000004376cd in process_raw_event (data=0x0, cpu=-1, timestamp=533548521126805, thread=
    0x918450) at builtin-lock.c:727
5  0x0000000000437df4 in process_sample_event (tool=0x7ab300, event=0x7ffff7ff9dd8, sample=
    0x7fffffffdf60, evsel=0x917d90, machine=0x9161c0) at builtin-lock.c:863
6  0x0000000000470d04 in perf_session_deliver_event (session=0x916160, event=0x7ffff7ff9dd8, sample=
    0x7fffffffdf60, tool=0x7ab300, file_offset=11736) at util/session.c:977
7  0x000000000047024a in flush_sample_queue (s=0x916160, tool=0x7ab300) at util/session.c:679
8  0x0000000000471ba9 in __perf_session__process_events (session=0x916160, data_offset=400, data_size=
    11576, file_size=11976, tool=0x7ab300) at util/session.c:1363
9  0x0000000000471c5e in perf_session__process_events (self=0x916160, tool=0x7ab300)
    at util/session.c:1379
10 0x0000000000437e80 in read_events () at builtin-lock.c:880
11 0x0000000000438275 in cmd_lock (argc=0, argv=0x7fffffffe420, prefix=0x0) at builtin-lock.c:10

In this case the data file has cycles events, not traces and hence nothing
to do with lock tracepoints. Nonetheless, perf-lock should not crash. The
crash started with commit aaf045f72335653b24784d6042be8e4aee114403 - ie.,
the move to libtraceevent.

With this patch you get the more user friendly:
$ perf lock info
  Fatal: Unknown type of information

Signed-off-by: David Ahern <dsahern@gmail.com>
CC: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
---
 tools/perf/builtin-lock.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
index fd53319..55f7961 100644
--- a/tools/perf/builtin-lock.c
+++ b/tools/perf/builtin-lock.c
@@ -877,6 +877,9 @@ static int read_events(void)
 	if (!session)
 		die("Initializing perf session failed\n");
 
+	if (!perf_session__has_traces(session, "lock record"))
+		exit(1);
+
 	return perf_session__process_events(session, &eops);
 }
 
-- 
1.7.10.1

Re: [PATCH] perf lock: fix segfault with info subcommand following move to libtraceevent

[Arnaldo Carvalho de Melo]

Em Fri, Jul 06, 2012 at 10:02:18AM -0600, David Ahern escreveu:
> +++ b/tools/perf/builtin-lock.c
> @@ -877,6 +877,9 @@ static int read_events(void)
>  	if (!session)
>  		die("Initializing perf session failed\n");
>  
> +	if (!perf_session__has_traces(session, "lock record"))
> +		exit(1);
> +
>  	return perf_session__process_events(session, &eops);
>  }

This is getting out of hand, first a die(), then an exit(1) and finally
this function returns a value, ouch.

I'd rather use return to signal that something went wrong and as well
print some helpful warning to the user.

Eventually we should fix all the other offenders, but lets try not to
add even more.

Can you please resend with a pr_warning + return failure?

- Arnaldo

Re: [PATCH] perf lock: fix segfault with info subcommand following move to libtraceevent

[David Ahern]

On 7/6/12 11:14 AM, Arnaldo Carvalho de Melo wrote:
> Em Fri, Jul 06, 2012 at 10:02:18AM -0600, David Ahern escreveu:
>> +++ b/tools/perf/builtin-lock.c
>> @@ -877,6 +877,9 @@ static int read_events(void)
>>   	if (!session)
>>   		die("Initializing perf session failed\n");
>>
>> +	if (!perf_session__has_traces(session, "lock record"))
>> +		exit(1);
>> +
>>   	return perf_session__process_events(session, &eops);
>>   }
>
> This is getting out of hand, first a die(), then an exit(1) and finally
> this function returns a value, ouch.
>
> I'd rather use return to signal that something went wrong and as well
> print some helpful warning to the user.
>
> Eventually we should fix all the other offenders, but lets try not to
> add even more.

Agree. But....

>
> Can you please resend with a pr_warning + return failure?


This command needs some love. The rc is not checked and requires some 
rework. e.g., the report path:

     setup_pager();
     select_key();
     read_events();    <---- the function I changed
     sort_result();
     print_result();

and the info path:
         setup_pager();
         read_events();
         dump_info();

I figured for 3.5 at least not segfault; clean up for 3.6 or later.

David

Re: [PATCH] perf lock: fix segfault with info subcommand following move to libtraceevent

[Arnaldo Carvalho de Melo]

Em Fri, Jul 06, 2012 at 11:17:41AM -0600, David Ahern escreveu:
> On 7/6/12 11:14 AM, Arnaldo Carvalho de Melo wrote:
> >Em Fri, Jul 06, 2012 at 10:02:18AM -0600, David Ahern escreveu:
> >>+++ b/tools/perf/builtin-lock.c
> >>@@ -877,6 +877,9 @@ static int read_events(void)
> >>  	if (!session)
> >>  		die("Initializing perf session failed\n");
> >>
> >>+	if (!perf_session__has_traces(session, "lock record"))
> >>+		exit(1);
> >>+
> >>  	return perf_session__process_events(session, &eops);
> >>  }
> >
> >This is getting out of hand, first a die(), then an exit(1) and finally
> >this function returns a value, ouch.
> >
> >I'd rather use return to signal that something went wrong and as well
> >print some helpful warning to the user.
> >
> >Eventually we should fix all the other offenders, but lets try not to
> >add even more.
> 
> Agree. But....
> 
> >
> >Can you please resend with a pr_warning + return failure?
> 
> 
> This command needs some love. The rc is not checked and requires
> some rework. e.g., the report path:
> 
>     setup_pager();
>     select_key();
>     read_events();    <---- the function I changed
>     sort_result();
>     print_result();
> 
> and the info path:
>         setup_pager();
>         read_events();
>         dump_info();
> 
> I figured for 3.5 at least not segfault; clean up for 3.6 or later.

Fair enough, so answering that other question, you want this for
perf/urgent.

Ok, queing it up for that branch,

- Arnaldo