Re: [PATCH] perf record: Fix crash in pipe mode

[Arnaldo Carvalho de Melo]

Em Fri, Mar 02, 2018 at 05:13:54PM +0100, Jiri Olsa escreveu:
> Currently we can crash perf record when running in pipe mode, like:
> 
>   $ perf record ls | perf report
>   # To display the perf.data header info, please use --header/--header-only 
> options.
>   #
>   perf: Segmentation fault
>   Error:
>   The - file has no samples!
> 
> The callstack of the crash is:

Thanks, reproduced the problem, applied the patch, tested, fixed, simple
enough, applied.

- Arnaldo
 
> 0x00515242 in perf_event__synthesize_event_update_name
>   3513ev = event_update_event__new(len + 1, 
> PERF_EVENT_UPDATE__NAME, evsel->id[0]);
>   (gdb) bt
>   #0  0x00515242 in perf_event__synthesize_event_update_name
>   #1  0x005158a4 in perf_event__synthesize_extra_attr
>   #2  0x00443347 in record__synthesize
>   #3  0x004438e3 in __cmd_record
>   #4  0x0044514e in cmd_record
>   #5  0x004cbc95 in run_builtin
>   #6  0x004cbf02 in handle_internal_command
>   #7  0x004cc054 in run_argv
>   #8  0x004cc422 in main
> 
> The reason of the crash is that the evsel does not have ids array
> allocated and the pipe's synthesize code tries to access it.
> 
> We don't force evsel ids allocation when we have single event,
> because it's not needed. However we need it when we are in pipe
> mode even for single event as a key for evsel update event.
> 
> Fixing this by forcing evsel ids allocation event for single
> event, when we are in pipe mode.
> 
> Link: http://lkml.kernel.org/n/tip-xbh5ca1azdcdqyep653vb...@git.kernel.org
> Signed-off-by: Jiri Olsa 
> ---
>  tools/perf/builtin-record.c | 9 +
>  tools/perf/perf.h   | 1 +
>  tools/perf/util/record.c| 8 ++--
>  3 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index a60f398cbc24..eeef1143bae3 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -881,6 +881,15 @@ static int __cmd_record(struct record *rec, int argc, 
> const char **argv)
>   }
>   }
>  
> + /*
> +  * If we have just single event and are sending data
> +  * through pipe, we need to force the ids allocation,
> +  * because we synthesize event name through the pipe
> +  * and need the id for that.
> +  */
> + if (data->is_pipe && rec->evlist->nr_entries == 1)
> + rec->opts.sample_id = true;
> +
>   if (record__open(rec) != 0) {
>   err = -1;
>   goto out_child;
> diff --git a/tools/perf/perf.h b/tools/perf/perf.h
> index 007e0dfd5ce3..8fec1abd0f1f 100644
> --- a/tools/perf/perf.h
> +++ b/tools/perf/perf.h
> @@ -62,6 +62,7 @@ struct record_opts {
>   bool overwrite;
>   bool ignore_missing_thread;
>   bool strict_freq;
> + bool sample_id;
>   unsigned int freq;
>   unsigned int mmap_pages;
>   unsigned int auxtrace_mmap_pages;
> diff --git a/tools/perf/util/record.c b/tools/perf/util/record.c
> index 3878c503f464..867f5937ebb7 100644
> --- a/tools/perf/util/record.c
> +++ b/tools/perf/util/record.c
> @@ -138,6 +138,7 @@ void perf_evlist__config(struct perf_evlist *evlist, 
> struct record_opts *opts,
>   struct perf_evsel *evsel;
>   bool use_sample_identifier = false;
>   bool use_comm_exec;
> + bool sample_id = opts->sample_id;
>  
>   /*
>* Set the evsel leader links before we configure attributes,
> @@ -164,8 +165,7 @@ void perf_evlist__config(struct perf_evlist *evlist, 
> struct record_opts *opts,
>* match the id.
>*/
>   use_sample_identifier = perf_can_sample_identifier();
> - evlist__for_each_entry(evlist, evsel)
> - perf_evsel__set_sample_id(evsel, use_sample_identifier);
> + sample_id = true;
>   } else if (evlist->nr_entries > 1) {
>   struct perf_evsel *first = perf_evlist__first(evlist);
>  
> @@ -175,6 +175,10 @@ void perf_evlist__config(struct perf_evlist *evlist, 
> struct record_opts *opts,
>   use_sample_identifier = perf_can_sample_identifier();
>   break;
>   }
> + sample_id = true;
> + }
> +
> + if (sample_id) {
>   evlist__for_each_entry(evlist, evsel)
>   perf_evsel__set_sample_id(evsel, use_sample_identifier);
>   }
> -- 
> 2.13.6

Re: [PATCH] perf record: Fix crash in pipe mode

[Arnaldo Carvalho de Melo]

Em Fri, Mar 02, 2018 at 05:13:54PM +0100, Jiri Olsa escreveu:
> Currently we can crash perf record when running in pipe mode, like:
> 
>   $ perf record ls | perf report
>   # To display the perf.data header info, please use --header/--header-only 
> options.
>   #
>   perf: Segmentation fault
>   Error:
>   The - file has no samples!
> 
> The callstack of the crash is:

Thanks, reproduced the problem, applied the patch, tested, fixed, simple
enough, applied.

- Arnaldo
 
> 0x00515242 in perf_event__synthesize_event_update_name
>   3513ev = event_update_event__new(len + 1, 
> PERF_EVENT_UPDATE__NAME, evsel->id[0]);
>   (gdb) bt
>   #0  0x00515242 in perf_event__synthesize_event_update_name
>   #1  0x005158a4 in perf_event__synthesize_extra_attr
>   #2  0x00443347 in record__synthesize
>   #3  0x004438e3 in __cmd_record
>   #4  0x0044514e in cmd_record
>   #5  0x004cbc95 in run_builtin
>   #6  0x004cbf02 in handle_internal_command
>   #7  0x004cc054 in run_argv
>   #8  0x004cc422 in main
> 
> The reason of the crash is that the evsel does not have ids array
> allocated and the pipe's synthesize code tries to access it.
> 
> We don't force evsel ids allocation when we have single event,
> because it's not needed. However we need it when we are in pipe
> mode even for single event as a key for evsel update event.
> 
> Fixing this by forcing evsel ids allocation event for single
> event, when we are in pipe mode.
> 
> Link: http://lkml.kernel.org/n/tip-xbh5ca1azdcdqyep653vb...@git.kernel.org
> Signed-off-by: Jiri Olsa 
> ---
>  tools/perf/builtin-record.c | 9 +
>  tools/perf/perf.h   | 1 +
>  tools/perf/util/record.c| 8 ++--
>  3 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index a60f398cbc24..eeef1143bae3 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -881,6 +881,15 @@ static int __cmd_record(struct record *rec, int argc, 
> const char **argv)
>   }
>   }
>  
> + /*
> +  * If we have just single event and are sending data
> +  * through pipe, we need to force the ids allocation,
> +  * because we synthesize event name through the pipe
> +  * and need the id for that.
> +  */
> + if (data->is_pipe && rec->evlist->nr_entries == 1)
> + rec->opts.sample_id = true;
> +
>   if (record__open(rec) != 0) {
>   err = -1;
>   goto out_child;
> diff --git a/tools/perf/perf.h b/tools/perf/perf.h
> index 007e0dfd5ce3..8fec1abd0f1f 100644
> --- a/tools/perf/perf.h
> +++ b/tools/perf/perf.h
> @@ -62,6 +62,7 @@ struct record_opts {
>   bool overwrite;
>   bool ignore_missing_thread;
>   bool strict_freq;
> + bool sample_id;
>   unsigned int freq;
>   unsigned int mmap_pages;
>   unsigned int auxtrace_mmap_pages;
> diff --git a/tools/perf/util/record.c b/tools/perf/util/record.c
> index 3878c503f464..867f5937ebb7 100644
> --- a/tools/perf/util/record.c
> +++ b/tools/perf/util/record.c
> @@ -138,6 +138,7 @@ void perf_evlist__config(struct perf_evlist *evlist, 
> struct record_opts *opts,
>   struct perf_evsel *evsel;
>   bool use_sample_identifier = false;
>   bool use_comm_exec;
> + bool sample_id = opts->sample_id;
>  
>   /*
>* Set the evsel leader links before we configure attributes,
> @@ -164,8 +165,7 @@ void perf_evlist__config(struct perf_evlist *evlist, 
> struct record_opts *opts,
>* match the id.
>*/
>   use_sample_identifier = perf_can_sample_identifier();
> - evlist__for_each_entry(evlist, evsel)
> - perf_evsel__set_sample_id(evsel, use_sample_identifier);
> + sample_id = true;
>   } else if (evlist->nr_entries > 1) {
>   struct perf_evsel *first = perf_evlist__first(evlist);
>  
> @@ -175,6 +175,10 @@ void perf_evlist__config(struct perf_evlist *evlist, 
> struct record_opts *opts,
>   use_sample_identifier = perf_can_sample_identifier();
>   break;
>   }
> + sample_id = true;
> + }
> +
> + if (sample_id) {
>   evlist__for_each_entry(evlist, evsel)
>   perf_evsel__set_sample_id(evsel, use_sample_identifier);
>   }
> -- 
> 2.13.6