perf lock: fix segfault with info subcommand following move to libtraceevent
PATCH 1/1 perf lock: fix segfault with info subcommand following move to libtraceevent
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:137910 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
--- tools/perf/builtin-lock.c | 3 +++ 1 file changed, 3 insertions(+)
tools/perf/builtin-lock.c +3 -0
@@ -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); }
Signature · 1 line
1.7.10.1
Em Fri, Jul 06, 2012 at 10:02:18AM -0600, David Ahern escreveu:
tools/perf/builtin-lock.c static int read_events(void) · line 884
Quoted 10 lines
+++
b/tools/perf/builtin-lock.ctools/perf/builtin-lock.c +3
-0@@ -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
Quoted 15 lines · Arnaldo Carvalho de Melo
Em Fri, Jul 06, 2012 at 10:02:18AM -0600, David Ahern escreveu:
tools/perf/builtin-lock.c static int read_events(void) · line 884
Quoted 10 lines
+++
b/tools/perf/builtin-lock.ctools/perf/builtin-lock.c +3
-0@@ -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.
Quoted 5 lines
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....
Quoted 2 lines
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
Em Fri, Jul 06, 2012 at 11:17:41AM -0600, David Ahern escreveu:
Quoted 37 lines · Arnaldo Carvalho de Melo
Quoted 15 lines
Em Fri, Jul 06, 2012 at 10:02:18AM -0600, David Ahern escreveu:
tools/perf/builtin-lock.c static int read_events(void) · line 884
Quoted 10 lines
+++
b/tools/perf/builtin-lock.ctools/perf/builtin-lock.c +3
-0@@ -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.Quoted 5 lines
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....
Quoted 2 lines
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();
Quoted 6 lines
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