[PATCH 1/2] perf diff: Fix segfault on perf diff -o N option

[PATCH 1/2] perf diff: Fix segfault on perf diff -o N option

[Namhyung Kim]

The -o/--order option is to select column number to sort a diff result.
It does the job by adding a hpp field at the beginning of the sort list.
But it should not be added to the output field list as it has no
callbacks required by a output field.

During the setup_sorting(), the perf_hpp__setup_output_field() appends
given sort keys to the output field if it's not there already.

Originally it was checked by the fmt->list being non-empty.  But commit
3f931f2c4274 changed it to check the ->equal callback.  Anyway we don't
need to add the pseudo hpp field to the output field list since it won't
be used for output.  So just skip fields if they have no ->color or
 ->entry callbacks.

Fixes: 3f931f2c4274 ("perf hists: Make hpp setup function generic")
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/hist.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index 37388397b5bc..4ec79b2f9416 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -560,6 +560,10 @@ void perf_hpp__setup_output_field(struct perf_hpp_list *list)
 	perf_hpp_list__for_each_sort_list(list, fmt) {
 		struct perf_hpp_fmt *pos;
 
+		/* skip sort-only fields ("sort_compute" in perf diff) */
+		if (!fmt->entry && !fmt->color)
+			continue;
+
 		perf_hpp_list__for_each_format(list, pos) {
 			if (fmt_equal(fmt, pos))
 				goto next;
-- 
2.11.0

[PATCH 2/2] perf diff: Fix -o/--order option behavior (again)

[Namhyung Kim]

The commit 21e6d8428664 changed list_add() to
perf_hpp__register_sort_field().  However this resulted in a behavior
change since the field was added to the tail instead of the head.  So
the -o option is mostly ignored due to its order in the list.

This patch fixes it by adding perf_hpp__prepend_sort_field().

Fixes: 21e6d8428664 ("perf diff: Use perf_hpp__register_sort_field interface")
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-diff.c | 2 +-
 tools/perf/ui/hist.c      | 6 ++++++
 tools/perf/util/hist.h    | 7 +++++++
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index 9ff0db4e2d0c..933aeec46f4a 100644
--- a/tools/perf/builtin-diff.c
+++ b/tools/perf/builtin-diff.c
@@ -1199,7 +1199,7 @@ static int ui_init(void)
 		BUG_ON(1);
 	}
 
-	perf_hpp__register_sort_field(fmt);
+	perf_hpp__prepend_sort_field(fmt);
 	return 0;
 }
 
diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index 4ec79b2f9416..18cfcdc90356 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -521,6 +521,12 @@ void perf_hpp_list__register_sort_field(struct perf_hpp_list *list,
 	list_add_tail(&format->sort_list, &list->sorts);
 }
 
+void perf_hpp_list__prepend_sort_field(struct perf_hpp_list *list,
+				       struct perf_hpp_fmt *format)
+{
+	list_add(&format->sort_list, &list->sorts);
+}
+
 void perf_hpp__column_unregister(struct perf_hpp_fmt *format)
 {
 	list_del(&format->list);
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index d4b6514eeef5..28c216e3d5b7 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -283,6 +283,8 @@ void perf_hpp_list__column_register(struct perf_hpp_list *list,
 				    struct perf_hpp_fmt *format);
 void perf_hpp_list__register_sort_field(struct perf_hpp_list *list,
 					struct perf_hpp_fmt *format);
+void perf_hpp_list__prepend_sort_field(struct perf_hpp_list *list,
+				       struct perf_hpp_fmt *format);
 
 static inline void perf_hpp__column_register(struct perf_hpp_fmt *format)
 {
@@ -294,6 +296,11 @@ static inline void perf_hpp__register_sort_field(struct perf_hpp_fmt *format)
 	perf_hpp_list__register_sort_field(&perf_hpp_list, format);
 }
 
+static inline void perf_hpp__prepend_sort_field(struct perf_hpp_fmt *format)
+{
+	perf_hpp_list__prepend_sort_field(&perf_hpp_list, format);
+}
+
 #define perf_hpp_list__for_each_format(_list, format) \
 	list_for_each_entry(format, &(_list)->fields, list)
 
-- 
2.11.0

Re: [PATCH 1/2] perf diff: Fix segfault on perf diff -o N option

[Jiri Olsa]

On Wed, Jan 18, 2017 at 02:14:56PM +0900, Namhyung Kim wrote:
> The -o/--order option is to select column number to sort a diff result.
> It does the job by adding a hpp field at the beginning of the sort list.
> But it should not be added to the output field list as it has no
> callbacks required by a output field.
> 
> During the setup_sorting(), the perf_hpp__setup_output_field() appends
> given sort keys to the output field if it's not there already.
> 
> Originally it was checked by the fmt->list being non-empty.  But commit
> 3f931f2c4274 changed it to check the ->equal callback.  Anyway we don't
> need to add the pseudo hpp field to the output field list since it won't
> be used for output.  So just skip fields if they have no ->color or
>  ->entry callbacks.
> 
> Fixes: 3f931f2c4274 ("perf hists: Make hpp setup function generic")
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

> ---
>  tools/perf/ui/hist.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> index 37388397b5bc..4ec79b2f9416 100644
> --- a/tools/perf/ui/hist.c
> +++ b/tools/perf/ui/hist.c
> @@ -560,6 +560,10 @@ void perf_hpp__setup_output_field(struct perf_hpp_list *list)
>  	perf_hpp_list__for_each_sort_list(list, fmt) {
>  		struct perf_hpp_fmt *pos;
>  
> +		/* skip sort-only fields ("sort_compute" in perf diff) */
> +		if (!fmt->entry && !fmt->color)
> +			continue;
> +
>  		perf_hpp_list__for_each_format(list, pos) {
>  			if (fmt_equal(fmt, pos))
>  				goto next;
> -- 
> 2.11.0
> 

Re: [PATCH 2/2] perf diff: Fix -o/--order option behavior (again)

[Jiri Olsa]

On Wed, Jan 18, 2017 at 02:14:57PM +0900, Namhyung Kim wrote:
> The commit 21e6d8428664 changed list_add() to
> perf_hpp__register_sort_field().  However this resulted in a behavior
> change since the field was added to the tail instead of the head.  So
> the -o option is mostly ignored due to its order in the list.
> 
> This patch fixes it by adding perf_hpp__prepend_sort_field().
> 
> Fixes: 21e6d8428664 ("perf diff: Use perf_hpp__register_sort_field interface")
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

Re: [PATCH 1/2] perf diff: Fix segfault on perf diff -o N option

[Namhyung Kim]

Hi Arnaldo,

Could you please consider merging it to the perf/urgent?

Thanks,
Namhyung


On Wed, Jan 18, 2017 at 02:14:56PM +0900, Namhyung Kim wrote:
> The -o/--order option is to select column number to sort a diff result.
> It does the job by adding a hpp field at the beginning of the sort list.
> But it should not be added to the output field list as it has no
> callbacks required by a output field.
> 
> During the setup_sorting(), the perf_hpp__setup_output_field() appends
> given sort keys to the output field if it's not there already.
> 
> Originally it was checked by the fmt->list being non-empty.  But commit
> 3f931f2c4274 changed it to check the ->equal callback.  Anyway we don't
> need to add the pseudo hpp field to the output field list since it won't
> be used for output.  So just skip fields if they have no ->color or
>  ->entry callbacks.
> 
> Fixes: 3f931f2c4274 ("perf hists: Make hpp setup function generic")
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/ui/hist.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> index 37388397b5bc..4ec79b2f9416 100644
> --- a/tools/perf/ui/hist.c
> +++ b/tools/perf/ui/hist.c
> @@ -560,6 +560,10 @@ void perf_hpp__setup_output_field(struct perf_hpp_list *list)
>  	perf_hpp_list__for_each_sort_list(list, fmt) {
>  		struct perf_hpp_fmt *pos;
>  
> +		/* skip sort-only fields ("sort_compute" in perf diff) */
> +		if (!fmt->entry && !fmt->color)
> +			continue;
> +
>  		perf_hpp_list__for_each_format(list, pos) {
>  			if (fmt_equal(fmt, pos))
>  				goto next;
> -- 
> 2.11.0
> 

Re: [PATCH 1/2] perf diff: Fix segfault on perf diff -o N option

[Arnaldo Carvalho de Melo]

Em Thu, Feb 02, 2017 at 11:43:03AM +0900, Namhyung Kim escreveu:
> Hi Arnaldo,
> 
> Could you please consider merging it to the perf/urgent?

Fell thru the cracks, applied to perf/urgent, thanks for the reminder!

- Arnaldo
 
> Thanks,
> Namhyung
> 
> 
> On Wed, Jan 18, 2017 at 02:14:56PM +0900, Namhyung Kim wrote:
> > The -o/--order option is to select column number to sort a diff result.
> > It does the job by adding a hpp field at the beginning of the sort list.
> > But it should not be added to the output field list as it has no
> > callbacks required by a output field.
> > 
> > During the setup_sorting(), the perf_hpp__setup_output_field() appends
> > given sort keys to the output field if it's not there already.
> > 
> > Originally it was checked by the fmt->list being non-empty.  But commit
> > 3f931f2c4274 changed it to check the ->equal callback.  Anyway we don't
> > need to add the pseudo hpp field to the output field list since it won't
> > be used for output.  So just skip fields if they have no ->color or
> >  ->entry callbacks.
> > 
> > Fixes: 3f931f2c4274 ("perf hists: Make hpp setup function generic")
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/ui/hist.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> > index 37388397b5bc..4ec79b2f9416 100644
> > --- a/tools/perf/ui/hist.c
> > +++ b/tools/perf/ui/hist.c
> > @@ -560,6 +560,10 @@ void perf_hpp__setup_output_field(struct perf_hpp_list *list)
> >  	perf_hpp_list__for_each_sort_list(list, fmt) {
> >  		struct perf_hpp_fmt *pos;
> >  
> > +		/* skip sort-only fields ("sort_compute" in perf diff) */
> > +		if (!fmt->entry && !fmt->color)
> > +			continue;
> > +
> >  		perf_hpp_list__for_each_format(list, pos) {
> >  			if (fmt_equal(fmt, pos))
> >  				goto next;
> > -- 
> > 2.11.0
> > 

[tip:perf/urgent] perf diff: Fix segfault on 'perf diff -o N' option

[tip-bot for Namhyung Kim]

Commit-ID:  8381cdd0e32dd748bd34ca3ace476949948bd793
Gitweb:     http://git.kernel.org/tip/8381cdd0e32dd748bd34ca3ace476949948bd793
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Wed, 18 Jan 2017 14:14:56 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 2 Feb 2017 11:39:04 -0300

perf diff: Fix segfault on 'perf diff -o N' option

The -o/--order option is to select column number to sort a diff result.

It does the job by adding a hpp field at the beginning of the sort list.
But it should not be added to the output field list as it has no
callbacks required by a output field.

During the setup_sorting(), the perf_hpp__setup_output_field() appends
the given sort keys to the output field if it's not there already.

Originally it was checked by fmt->list being non-empty.  But commit
3f931f2c4274 ("perf hists: Make hpp setup function generic") changed it
to check the ->equal callback.

Anyways, we don't need to add the pseudo hpp field to the output field
list since it won't be used for output.  So just skip fields if they
have no ->color or ->entry callbacks.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Fixes: 3f931f2c4274 ("perf hists: Make hpp setup function generic")
Link: http://lkml.kernel.org/r/20170118051457.30946-1-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/ui/hist.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index 3738839..4ec79b2 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -560,6 +560,10 @@ void perf_hpp__setup_output_field(struct perf_hpp_list *list)
 	perf_hpp_list__for_each_sort_list(list, fmt) {
 		struct perf_hpp_fmt *pos;
 
+		/* skip sort-only fields ("sort_compute" in perf diff) */
+		if (!fmt->entry && !fmt->color)
+			continue;
+
 		perf_hpp_list__for_each_format(list, pos) {
 			if (fmt_equal(fmt, pos))
 				goto next;

[tip:perf/urgent] perf diff: Fix -o/--order option behavior (again)

[tip-bot for Namhyung Kim]

Commit-ID:  a1c9f97f0b64e6337d9cfcc08c134450934fdd90
Gitweb:     http://git.kernel.org/tip/a1c9f97f0b64e6337d9cfcc08c134450934fdd90
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Wed, 18 Jan 2017 14:14:57 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 2 Feb 2017 11:39:09 -0300

perf diff: Fix -o/--order option behavior (again)

Commit 21e6d8428664 ("perf diff: Use perf_hpp__register_sort_field
interface") changed list_add() to perf_hpp__register_sort_field().

This resulted in a behavior change since the field was added to the tail
instead of the head.  So the -o option is mostly ignored due to its
order in the list.

This patch fixes it by adding perf_hpp__prepend_sort_field().

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Fixes: 21e6d8428664 ("perf diff: Use perf_hpp__register_sort_field interface")
Link: http://lkml.kernel.org/r/20170118051457.30946-2-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-diff.c | 2 +-
 tools/perf/ui/hist.c      | 6 ++++++
 tools/perf/util/hist.h    | 7 +++++++
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index 9ff0db4..933aeec 100644
--- a/tools/perf/builtin-diff.c
+++ b/tools/perf/builtin-diff.c
@@ -1199,7 +1199,7 @@ static int ui_init(void)
 		BUG_ON(1);
 	}
 
-	perf_hpp__register_sort_field(fmt);
+	perf_hpp__prepend_sort_field(fmt);
 	return 0;
 }
 
diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index 4ec79b2..18cfcdc9 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -521,6 +521,12 @@ void perf_hpp_list__register_sort_field(struct perf_hpp_list *list,
 	list_add_tail(&format->sort_list, &list->sorts);
 }
 
+void perf_hpp_list__prepend_sort_field(struct perf_hpp_list *list,
+				       struct perf_hpp_fmt *format)
+{
+	list_add(&format->sort_list, &list->sorts);
+}
+
 void perf_hpp__column_unregister(struct perf_hpp_fmt *format)
 {
 	list_del(&format->list);
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index d4b6514..28c216e 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -283,6 +283,8 @@ void perf_hpp_list__column_register(struct perf_hpp_list *list,
 				    struct perf_hpp_fmt *format);
 void perf_hpp_list__register_sort_field(struct perf_hpp_list *list,
 					struct perf_hpp_fmt *format);
+void perf_hpp_list__prepend_sort_field(struct perf_hpp_list *list,
+				       struct perf_hpp_fmt *format);
 
 static inline void perf_hpp__column_register(struct perf_hpp_fmt *format)
 {
@@ -294,6 +296,11 @@ static inline void perf_hpp__register_sort_field(struct perf_hpp_fmt *format)
 	perf_hpp_list__register_sort_field(&perf_hpp_list, format);
 }
 
+static inline void perf_hpp__prepend_sort_field(struct perf_hpp_fmt *format)
+{
+	perf_hpp_list__prepend_sort_field(&perf_hpp_list, format);
+}
+
 #define perf_hpp_list__for_each_format(_list, format) \
 	list_for_each_entry(format, &(_list)->fields, list)