[PATCH 0/3] perf script: Few trivial fixes

[PATCH 0/3] perf script: Few trivial fixes

[Ravi Bangoria]

First patch fixes perf output field hint. Second and third fixes
crash when used in a piped mode.

Ravi Bangoria (3):
  perf script: Add missing output fields in a hint
  perf script: Fix crash because of missing evsel->priv
  perf script: Fix crash because of missing feat_op[] entry

 tools/perf/builtin-script.c | 19 +++++++++++++++++--
 tools/perf/util/header.c    |  7 ++++++-
 2 files changed, 23 insertions(+), 3 deletions(-)

-- 
2.14.4

[PATCH 1/3] perf script: Add missing output fields in a hint

[Ravi Bangoria]

Few fields are missing in a perf script -F hint. Add them.

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
 tools/perf/builtin-script.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index b3bf35512d21..4d1cee68cfd2 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -3113,8 +3113,9 @@ int cmd_script(int argc, const char **argv)
 		     "+field to add and -field to remove."
 		     "Valid types: hw,sw,trace,raw,synth. "
 		     "Fields: comm,tid,pid,time,cpu,event,trace,ip,sym,dso,"
-		     "addr,symoff,period,iregs,uregs,brstack,brstacksym,flags,"
-		     "bpf-output,callindent,insn,insnlen,brstackinsn,synth,phys_addr",
+		     "addr,symoff,srcline,period,iregs,uregs,brstack,"
+		     "brstacksym,flags,bpf-output,brstackinsn,brstackoff,"
+		     "callindent,insn,insnlen,synth,phys_addr,metric,misc",
 		     parse_output_fields),
 	OPT_BOOLEAN('a', "all-cpus", &system_wide,
 		    "system-wide collection from all CPUs"),
-- 
2.14.4

[PATCH 2/3] perf script: Fix crash because of missing evsel->priv

[Ravi Bangoria]

perf script in pipped mode is crashing because evsel->priv is not
set properly. Fix it.

Before:
  # ./perf record -o - -- ls | ./perf script
    Segmentation fault (core dumped)

After:
  # ./perf record -o - -- ls | ./perf script
  ls 2282 1031.731974:  250000 cpu-clock:uhH:  7effe4b3d29e
  ls 2282 1031.732222:  250000 cpu-clock:uhH:  7effe4b3a650

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Fixes: a14390fde64e ("perf script: Allow creating per-event dump files")
---
 tools/perf/builtin-script.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index 4d1cee68cfd2..acee05562f5e 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -1822,6 +1822,7 @@ static int process_attr(struct perf_tool *tool, union perf_event *event,
 	struct perf_evlist *evlist;
 	struct perf_evsel *evsel, *pos;
 	int err;
+	static struct perf_evsel_script *es;
 
 	err = perf_event__process_attr(tool, event, pevlist);
 	if (err)
@@ -1830,6 +1831,19 @@ static int process_attr(struct perf_tool *tool, union perf_event *event,
 	evlist = *pevlist;
 	evsel = perf_evlist__last(*pevlist);
 
+	if (!evsel->priv) {
+		if (scr->per_event_dump) {
+			evsel->priv = perf_evsel_script__new(evsel,
+						scr->session->data);
+		} else {
+			es = zalloc(sizeof(*es));
+			if (!es)
+				return -ENOMEM;
+			es->fp = stdout;
+			evsel->priv = es;
+		}
+	}
+
 	if (evsel->attr.type >= PERF_TYPE_MAX &&
 	    evsel->attr.type != PERF_TYPE_SYNTH)
 		return 0;
-- 
2.14.4

Re: [PATCH 2/3] perf script: Fix crash because of missing evsel->priv

[Arnaldo Carvalho de Melo]

Em Wed, Jun 20, 2018 at 07:00:29PM +0530, Ravi Bangoria escreveu:
> perf script in pipped mode is crashing because evsel->priv is not
> set properly. Fix it.
> 
> Before:
>   # ./perf record -o - -- ls | ./perf script
>     Segmentation fault (core dumped)
> 
> After:
>   # ./perf record -o - -- ls | ./perf script
>   ls 2282 1031.731974:  250000 cpu-clock:uhH:  7effe4b3d29e
>   ls 2282 1031.732222:  250000 cpu-clock:uhH:  7effe4b3a650
> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> Fixes: a14390fde64e ("perf script: Allow creating per-event dump files")

Humm, this cset doesn't set evsel->priv to a 'struct perf_evsel_script'
object, will check which one does to continue review.

- Arnaldo

> ---
>  tools/perf/builtin-script.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index 4d1cee68cfd2..acee05562f5e 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -1822,6 +1822,7 @@ static int process_attr(struct perf_tool *tool, union perf_event *event,
>  	struct perf_evlist *evlist;
>  	struct perf_evsel *evsel, *pos;
>  	int err;
> +	static struct perf_evsel_script *es;
>  
>  	err = perf_event__process_attr(tool, event, pevlist);
>  	if (err)
> @@ -1830,6 +1831,19 @@ static int process_attr(struct perf_tool *tool, union perf_event *event,
>  	evlist = *pevlist;
>  	evsel = perf_evlist__last(*pevlist);
>  
> +	if (!evsel->priv) {
> +		if (scr->per_event_dump) {
> +			evsel->priv = perf_evsel_script__new(evsel,
> +						scr->session->data);
> +		} else {
> +			es = zalloc(sizeof(*es));
> +			if (!es)
> +				return -ENOMEM;
> +			es->fp = stdout;
> +			evsel->priv = es;
> +		}
> +	}
> +
>  	if (evsel->attr.type >= PERF_TYPE_MAX &&
>  	    evsel->attr.type != PERF_TYPE_SYNTH)
>  		return 0;
> -- 
> 2.14.4

Re: [PATCH 2/3] perf script: Fix crash because of missing evsel->priv

[Ravi Bangoria]

Hi Arnaldo,

On 06/20/2018 07:22 PM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Jun 20, 2018 at 07:00:29PM +0530, Ravi Bangoria escreveu:
>> perf script in pipped mode is crashing because evsel->priv is not
>> set properly. Fix it.
>>
>> Before:
>>   # ./perf record -o - -- ls | ./perf script
>>     Segmentation fault (core dumped)
>>
>> After:
>>   # ./perf record -o - -- ls | ./perf script
>>   ls 2282 1031.731974:  250000 cpu-clock:uhH:  7effe4b3d29e
>>   ls 2282 1031.732222:  250000 cpu-clock:uhH:  7effe4b3a650
>>
>> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
>> Fixes: a14390fde64e ("perf script: Allow creating per-event dump files")
> 
> Humm, this cset doesn't set evsel->priv to a 'struct perf_evsel_script'
> object, will check which one does to continue review.

Possible. Actually, git bisect led me to this commit.

Ravi

Re: [PATCH 2/3] perf script: Fix crash because of missing evsel->priv

[Ravi Bangoria]

Hi Arnaldo,

On 06/20/2018 07:22 PM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Jun 20, 2018 at 07:00:29PM +0530, Ravi Bangoria escreveu:
>> perf script in pipped mode is crashing because evsel->priv is not
>> set properly. Fix it.
>>
>> Before:
>>   # ./perf record -o - -- ls | ./perf script
>>     Segmentation fault (core dumped)
>>
>> After:
>>   # ./perf record -o - -- ls | ./perf script
>>   ls 2282 1031.731974:  250000 cpu-clock:uhH:  7effe4b3d29e
>>   ls 2282 1031.732222:  250000 cpu-clock:uhH:  7effe4b3a650
>>
>> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
>> Fixes: a14390fde64e ("perf script: Allow creating per-event dump files")
> 
> Humm, this cset doesn't set evsel->priv to a 'struct perf_evsel_script'
> object, will check which one does to continue review.

So, it's not about setting evsel->priv to 'struct perf_evsel_script', but
it's about setting proper output stream, either file or stdout. When
'struct perf_evsel_script' was not introduced, we were setting evsel->priv
to output stream. So I think, this commit missed to set evsel->priv properly
in pipped case. Ex, below patch applied _directly_ on top of a14390fde64e
fixes the issue.

------
diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index fb5e49b3bc44..66cc4b29bf4d 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -1645,6 +1645,9 @@ static int process_attr(struct perf_tool *tool, union perf_event *event,
 	evlist = *pevlist;
 	evsel = perf_evlist__last(*pevlist);
 
+	if (!evsel->priv)
+		evsel->priv = stdout;
+
 	if (evsel->attr.type >= PERF_TYPE_MAX &&
 	    evsel->attr.type != PERF_TYPE_SYNTH)
 		return 0;
------

To me this commit seems to be the bad. Please let me know if that is not
the case. I'll change the last patch as you suggested an post v2 soon.

Thanks,
Ravi

[PATCH 3/3] perf script: Fix crash because of missing feat_op[] entry

[Ravi Bangoria]

perf_event__process_feature() tries to access feat_ops[feat].process
which is not defined for feat = HEADER_LAST_FEATURE and thus perf is
crashing. Add dummy entry for HEADER_LAST_FEATURE in the feat_ops.

Before:
  # ./perf record -o - ls | ./perf script
  Segmentation fault (core dumped)

After:
  # ./perf record -o - ls | ./perf script
  ls 7031 4392.099856:  250000 cpu-clock:uhH:  7f5e0ce7cd60
  ls 7031 4392.100355:  250000 cpu-clock:uhH:  7f5e0c706ef7

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Fixes: 57b5de463925 ("perf report: Support forced leader feature in pipe mode")
---
 tools/perf/util/header.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 540cd2dcd3e7..de8e3e29d870 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -2555,14 +2555,18 @@ struct feature_ops {
 	}
 
 /* feature_ops not implemented: */
+#define write_last_feature	NULL
+
 #define print_tracing_data	NULL
 #define print_build_id		NULL
+#define print_last_feature	NULL
 
 #define process_branch_stack	NULL
 #define process_stat		NULL
+#define process_last_feature	NULL
 
 
-static const struct feature_ops feat_ops[HEADER_LAST_FEATURE] = {
+static const struct feature_ops feat_ops[HEADER_LAST_FEATURE + 1] = {
 	FEAT_OPN(TRACING_DATA,	tracing_data,	false),
 	FEAT_OPN(BUILD_ID,	build_id,	false),
 	FEAT_OPR(HOSTNAME,	hostname,	false),
@@ -2585,6 +2589,7 @@ static const struct feature_ops feat_ops[HEADER_LAST_FEATURE] = {
 	FEAT_OPN(CACHE,		cache,		true),
 	FEAT_OPR(SAMPLE_TIME,	sample_time,	false),
 	FEAT_OPR(MEM_TOPOLOGY,	mem_topology,	true),
+	FEAT_OPN(LAST_FEATURE,	last_feature,	false),
 };
 
 struct header_print_data {
-- 
2.14.4

Re: [PATCH 3/3] perf script: Fix crash because of missing feat_op[] entry

[Arnaldo Carvalho de Melo]

Em Wed, Jun 20, 2018 at 07:00:30PM +0530, Ravi Bangoria escreveu:
> perf_event__process_feature() tries to access feat_ops[feat].process
> which is not defined for feat = HEADER_LAST_FEATURE and thus perf is
> crashing. Add dummy entry for HEADER_LAST_FEATURE in the feat_ops.

Humm, first impression is that we should check for HEADER_LAST_FEATURE
and not try to access that array slot, as it is just a marker, not an
actual feature.

- Arnaldo
 
> Before:
>   # ./perf record -o - ls | ./perf script
>   Segmentation fault (core dumped)
> 
> After:
>   # ./perf record -o - ls | ./perf script
>   ls 7031 4392.099856:  250000 cpu-clock:uhH:  7f5e0ce7cd60
>   ls 7031 4392.100355:  250000 cpu-clock:uhH:  7f5e0c706ef7
> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> Fixes: 57b5de463925 ("perf report: Support forced leader feature in pipe mode")
> ---
>  tools/perf/util/header.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 540cd2dcd3e7..de8e3e29d870 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -2555,14 +2555,18 @@ struct feature_ops {
>  	}
>  
>  /* feature_ops not implemented: */
> +#define write_last_feature	NULL
> +
>  #define print_tracing_data	NULL
>  #define print_build_id		NULL
> +#define print_last_feature	NULL
>  
>  #define process_branch_stack	NULL
>  #define process_stat		NULL
> +#define process_last_feature	NULL
>  
>  
> -static const struct feature_ops feat_ops[HEADER_LAST_FEATURE] = {
> +static const struct feature_ops feat_ops[HEADER_LAST_FEATURE + 1] = {
>  	FEAT_OPN(TRACING_DATA,	tracing_data,	false),
>  	FEAT_OPN(BUILD_ID,	build_id,	false),
>  	FEAT_OPR(HOSTNAME,	hostname,	false),
> @@ -2585,6 +2589,7 @@ static const struct feature_ops feat_ops[HEADER_LAST_FEATURE] = {
>  	FEAT_OPN(CACHE,		cache,		true),
>  	FEAT_OPR(SAMPLE_TIME,	sample_time,	false),
>  	FEAT_OPR(MEM_TOPOLOGY,	mem_topology,	true),
> +	FEAT_OPN(LAST_FEATURE,	last_feature,	false),
>  };
>  
>  struct header_print_data {
> -- 
> 2.14.4

Re: [PATCH 3/3] perf script: Fix crash because of missing feat_op[] entry

[Ravi Bangoria]

Hi Arnaldo,

On 06/20/2018 07:19 PM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Jun 20, 2018 at 07:00:30PM +0530, Ravi Bangoria escreveu:
>> perf_event__process_feature() tries to access feat_ops[feat].process
>> which is not defined for feat = HEADER_LAST_FEATURE and thus perf is
>> crashing. Add dummy entry for HEADER_LAST_FEATURE in the feat_ops.
> 
> Humm, first impression is that we should check for HEADER_LAST_FEATURE
> and not try to access that array slot, as it is just a marker, not an
> actual feature.

Sure. Let me try to handle it inside perf_event__process_feature() itself.
May be something like below.

-----
diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 540cd2dcd3e7..4dc251d3b372 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -3461,6 +3461,10 @@ int perf_event__process_feature(struct perf_tool *tool,
                return -1;
        }
 
+       /* HEADER_LAST_FEATURE is just a marker. Ignore it. */
+       if (feat == HEADER_LAST_FEATURE)
+               return 0;
+
        if (!feat_ops[feat].process)
                return 0;
-----

Thanks,
Ravi