Two minor perf patches preparing for Skylake server events

Two minor perf patches preparing for Skylake server events

[Andi Kleen]

Arnaldo,

Here are two minor perf tools patches preparing for the Skylake
server events pull request.

-Andi

[PATCH 1/2] perf, tools, json: Support FCMask and PortMask

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Skylake server uncore IIO events need new FCMask/PortMask fields. Support
those in the json parser and pass it through as a filter.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 tools/perf/pmu-events/jevents.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
index 2350f6099a46..d51dc9ca8861 100644
--- a/tools/perf/pmu-events/jevents.c
+++ b/tools/perf/pmu-events/jevents.c
@@ -137,6 +137,8 @@ static struct field {
 	{ "AnyThread",	"any=" },
 	{ "EdgeDetect",	"edge=" },
 	{ "SampleAfterValue", "period=" },
+	{ "FCMask",	"fc_mask=" },
+	{ "PortMask",	"ch_mask=" },
 	{ NULL, NULL }
 };
 
-- 
2.9.4

Re: [PATCH 1/2] perf, tools, json: Support FCMask and PortMask

[Arnaldo Carvalho de Melo]

Em Wed, Aug 16, 2017 at 03:02:00PM -0700, Andi Kleen escreveu:
> From: Andi Kleen <ak@linux.intel.com>
> 
> Skylake server uncore IIO events need new FCMask/PortMask fields. Support
> those in the json parser and pass it through as a filter.

Thanks, applied.

[tip:perf/core] perf jevents: Support FCMask and PortMask

[tip-bot for Andi Kleen]

Commit-ID:  c73881eeb1a0ee9b45d8bc9f45ab02e23d81c5c5
Gitweb:     http://git.kernel.org/tip/c73881eeb1a0ee9b45d8bc9f45ab02e23d81c5c5
Author:     Andi Kleen <ak@linux.intel.com>
AuthorDate: Wed, 16 Aug 2017 15:02:00 -0700
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 17 Aug 2017 16:39:14 -0300

perf jevents: Support FCMask and PortMask

Skylake server uncore IIO events need new FCMask/PortMask fields. Support
those in the json parser and pass it through as a filter.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Link: http://lkml.kernel.org/r/20170816220201.19182-2-andi@firstfloor.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/pmu-events/jevents.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
index 2350f60..d51dc9c 100644
--- a/tools/perf/pmu-events/jevents.c
+++ b/tools/perf/pmu-events/jevents.c
@@ -137,6 +137,8 @@ static struct field {
 	{ "AnyThread",	"any=" },
 	{ "EdgeDetect",	"edge=" },
 	{ "SampleAfterValue", "period=" },
+	{ "FCMask",	"fc_mask=" },
+	{ "PortMask",	"ch_mask=" },
 	{ NULL, NULL }
 };
 

[PATCH 2/2] perf, tools: Avoid segfault on alias parse error

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

When there is a parse error on adding an alias the parser
segfaults. It thinks data is a parse_events_evlist and tries
to reference the error member. But it's really a parse_events_terms
for this call path through parse_events_terms.

Add the error member at the same location in parse_events_terms
as in *_evlist and set up a error structure to report errors correctly.

This can be only reproduced by adding errors to JSON aliases.

Segfault:

    #'0  0x00000000004d66d2 in parse_events_evlist_error (data=0x7fffffffce20, idx=6, str=0x76cda8 "parser error") at util/parse-events.c:2530
    #'1  0x0000000000516d0a in parse_events_error (loc=0x7fffffffb210, data=0x7fffffffce20, scanner=0x245b400, msg=0x76cb13 "syntax error") at util/parse-events.y:692
    #'2  0x000000000051675b in parse_events_parse (_data=0x7fffffffce20, scanner=0x245b400) at /home/ak/hle/obj-perf/util/parse-events-bison.c:2213
    #'3  0x00000000004d3fd6 in parse_events__scanner (str=0x725cff "event=0,", data=0x7fffffffce20, start_token=259) at util/parse-events.c:1646
    #'4  0x00000000004d4063 in parse_events_terms (terms=0x245b398, str=0x725cff "event=0,") at util/parse-events.c:1664
    #'5  0x00000000005179f1 in __perf_pmu__new_alias (list=0x7fffffffcf90, dir=0x0, name=0x725cec "unc_cha_clockticks", desc=0x725d08 "Uncore cache clock ticks. Unit: uncore_cha ",
    val=0x725cff "event=0,", long_desc=0x0, topic=0x725d34 "uncore other", unit=0x0, perpkg=0x6ca7c6 "1", metric_expr=0x0, metric_name=0x0) at util/pmu.c:255
    #'6  0x0000000000518789 in pmu_add_cpu_aliases (head=0x7fffffffcf90, name=0x2450903 "uncore_cha_9") at util/pmu.c:571
    #'7  0x00000000005188ac in pmu_lookup (name=0x2450903 "uncore_cha_9") at util/pmu.c:613
    #'8  0x0000000000518aff in perf_pmu__find (name=0x2450903 "uncore_cha_9") at util/pmu.c:672
    #'9  0x00000000005183d5 in pmu_read_sysfs () at util/pmu.c:467
    #'10 0x0000000000518a54 in perf_pmu__scan (pmu=0x0) at util/pmu.c:651
    #'11 0x0000000000519f26 in print_pmu_events (event_glob=0x0, name_only=false, quiet_flag=false, long_desc=false, details_flag=false) at util/pmu.c:1173
    #'12 0x00000000004d5ef0 in print_events (event_glob=0x0, name_only=false, quiet_flag=false, long_desc=false, details_flag=false) at util/parse-events.c:2343
    #'13 0x000000000043c7d4 in cmd_list (argc=0, argv=0x7fffffffeb90) at builtin-list.c:56
    #'14 0x00000000004ab2c8 in run_builtin (p=0xa281a0 <commands+192>, argc=1, argv=0x7fffffffeb90) at perf.c:296
    #15 0x00000000004ab535 in handle_internal_command (argc=1, argv=0x7fffffffeb90) at perf.c:348
    #16 0x00000000004ab687 in run_argv (argcp=0x7fffffffe9ec, argv=0x7fffffffe9e0) at perf.c:392
    #17 0x00000000004aba55 in main (argc=1, argv=0x7fffffffeb90) at perf.c:530

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 tools/perf/util/parse-events.c | 2 ++
 tools/perf/util/parse-events.h | 4 +++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 84e301073885..15b472aec767 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1656,7 +1656,9 @@ static int parse_events__scanner(const char *str, void *data, int start_token)
  */
 int parse_events_terms(struct list_head *terms, const char *str)
 {
+	struct parse_events_error err = { .idx = 0, };
 	struct parse_events_terms data = {
+		.error = &err,
 		.terms = NULL,
 	};
 	int ret;
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index a235f4d6d5e5..de217f5b37bb 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -108,15 +108,17 @@ struct parse_events_error {
 	char *help;	/* optional help string */
 };
 
+/* error field must match parse_events_terms */
 struct parse_events_evlist {
+	struct parse_events_error *error;
 	struct list_head	   list;
 	int			   idx;
 	int			   nr_groups;
-	struct parse_events_error *error;
 	struct perf_evlist	  *evlist;
 };
 
 struct parse_events_terms {
+	struct parse_events_error *error;
 	struct list_head *terms;
 };
 
-- 
2.9.4

Re: [PATCH 2/2] perf, tools: Avoid segfault on alias parse error

[Arnaldo Carvalho de Melo]

Em Wed, Aug 16, 2017 at 03:02:01PM -0700, Andi Kleen escreveu:
> From: Andi Kleen <ak@linux.intel.com>
> 
> When there is a parse error on adding an alias the parser
> segfaults. It thinks data is a parse_events_evlist and tries
> to reference the error member. But it's really a parse_events_terms
> for this call path through parse_events_terms.
> 
> Add the error member at the same location in parse_events_terms
> as in *_evlist and set up a error structure to report errors correctly.
> 
> This can be only reproduced by adding errors to JSON aliases.

Humm, but don't we have that checked?

void parse_events_evlist_error(struct parse_events_evlist *data,
                               int idx, const char *str)
{
        struct parse_events_error *err = data->error;

        if (!err)
                return;
        err->idx = idx;
        err->str = strdup(str);
        WARN_ONCE(!err->str, "WARNING: failed to allocate error string");
}

- Arnaldo
 
> Segfault:
> 
>     #'0  0x00000000004d66d2 in parse_events_evlist_error (data=0x7fffffffce20, idx=6, str=0x76cda8 "parser error") at util/parse-events.c:2530
>     #'1  0x0000000000516d0a in parse_events_error (loc=0x7fffffffb210, data=0x7fffffffce20, scanner=0x245b400, msg=0x76cb13 "syntax error") at util/parse-events.y:692
>     #'2  0x000000000051675b in parse_events_parse (_data=0x7fffffffce20, scanner=0x245b400) at /home/ak/hle/obj-perf/util/parse-events-bison.c:2213
>     #'3  0x00000000004d3fd6 in parse_events__scanner (str=0x725cff "event=0,", data=0x7fffffffce20, start_token=259) at util/parse-events.c:1646
>     #'4  0x00000000004d4063 in parse_events_terms (terms=0x245b398, str=0x725cff "event=0,") at util/parse-events.c:1664
>     #'5  0x00000000005179f1 in __perf_pmu__new_alias (list=0x7fffffffcf90, dir=0x0, name=0x725cec "unc_cha_clockticks", desc=0x725d08 "Uncore cache clock ticks. Unit: uncore_cha ",
>     val=0x725cff "event=0,", long_desc=0x0, topic=0x725d34 "uncore other", unit=0x0, perpkg=0x6ca7c6 "1", metric_expr=0x0, metric_name=0x0) at util/pmu.c:255
>     #'6  0x0000000000518789 in pmu_add_cpu_aliases (head=0x7fffffffcf90, name=0x2450903 "uncore_cha_9") at util/pmu.c:571
>     #'7  0x00000000005188ac in pmu_lookup (name=0x2450903 "uncore_cha_9") at util/pmu.c:613
>     #'8  0x0000000000518aff in perf_pmu__find (name=0x2450903 "uncore_cha_9") at util/pmu.c:672
>     #'9  0x00000000005183d5 in pmu_read_sysfs () at util/pmu.c:467
>     #'10 0x0000000000518a54 in perf_pmu__scan (pmu=0x0) at util/pmu.c:651
>     #'11 0x0000000000519f26 in print_pmu_events (event_glob=0x0, name_only=false, quiet_flag=false, long_desc=false, details_flag=false) at util/pmu.c:1173
>     #'12 0x00000000004d5ef0 in print_events (event_glob=0x0, name_only=false, quiet_flag=false, long_desc=false, details_flag=false) at util/parse-events.c:2343
>     #'13 0x000000000043c7d4 in cmd_list (argc=0, argv=0x7fffffffeb90) at builtin-list.c:56
>     #'14 0x00000000004ab2c8 in run_builtin (p=0xa281a0 <commands+192>, argc=1, argv=0x7fffffffeb90) at perf.c:296
>     #15 0x00000000004ab535 in handle_internal_command (argc=1, argv=0x7fffffffeb90) at perf.c:348
>     #16 0x00000000004ab687 in run_argv (argcp=0x7fffffffe9ec, argv=0x7fffffffe9e0) at perf.c:392
>     #17 0x00000000004aba55 in main (argc=1, argv=0x7fffffffeb90) at perf.c:530
> 
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
>  tools/perf/util/parse-events.c | 2 ++
>  tools/perf/util/parse-events.h | 4 +++-
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 84e301073885..15b472aec767 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -1656,7 +1656,9 @@ static int parse_events__scanner(const char *str, void *data, int start_token)
>   */
>  int parse_events_terms(struct list_head *terms, const char *str)
>  {
> +	struct parse_events_error err = { .idx = 0, };
>  	struct parse_events_terms data = {
> +		.error = &err,
>  		.terms = NULL,
>  	};
>  	int ret;
> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> index a235f4d6d5e5..de217f5b37bb 100644
> --- a/tools/perf/util/parse-events.h
> +++ b/tools/perf/util/parse-events.h
> @@ -108,15 +108,17 @@ struct parse_events_error {
>  	char *help;	/* optional help string */
>  };
>  
> +/* error field must match parse_events_terms */
>  struct parse_events_evlist {
> +	struct parse_events_error *error;
>  	struct list_head	   list;
>  	int			   idx;
>  	int			   nr_groups;
> -	struct parse_events_error *error;
>  	struct perf_evlist	  *evlist;
>  };
>  
>  struct parse_events_terms {
> +	struct parse_events_error *error;
>  	struct list_head *terms;
>  };
>  
> -- 
> 2.9.4

Re: [PATCH 2/2] perf, tools: Avoid segfault on alias parse error

[Andi Kleen]

> Humm, but don't we have that checked?

At least not in the case of the segfault below.

-Andi

> 
> void parse_events_evlist_error(struct parse_events_evlist *data,
>                                int idx, const char *str)
> {
>         struct parse_events_error *err = data->error;
> 
>         if (!err)
>                 return;
>         err->idx = idx;
>         err->str = strdup(str);
>         WARN_ONCE(!err->str, "WARNING: failed to allocate error string");
> }
> 
> - Arnaldo
>  
> > Segfault:
> > 
> >     #'0  0x00000000004d66d2 in parse_events_evlist_error (data=0x7fffffffce20, idx=6, str=0x76cda8 "parser error") at util/parse-events.c:2530
> >     #'1  0x0000000000516d0a in parse_events_error (loc=0x7fffffffb210, data=0x7fffffffce20, scanner=0x245b400, msg=0x76cb13 "syntax error") at util/parse-events.y:692
> >     #'2  0x000000000051675b in parse_events_parse (_data=0x7fffffffce20, scanner=0x245b400) at /home/ak/hle/obj-perf/util/parse-events-bison.c:2213
> >     #'3  0x00000000004d3fd6 in parse_events__scanner (str=0x725cff "event=0,", data=0x7fffffffce20, start_token=259) at util/parse-events.c:1646
> >     #'4  0x00000000004d4063 in parse_events_terms (terms=0x245b398, str=0x725cff "event=0,") at util/parse-events.c:1664
> >     #'5  0x00000000005179f1 in __perf_pmu__new_alias (list=0x7fffffffcf90, dir=0x0, name=0x725cec "unc_cha_clockticks", desc=0x725d08 "Uncore cache clock ticks. Unit: uncore_cha ",
> >     val=0x725cff "event=0,", long_desc=0x0, topic=0x725d34 "uncore other", unit=0x0, perpkg=0x6ca7c6 "1", metric_expr=0x0, metric_name=0x0) at util/pmu.c:255
> >     #'6  0x0000000000518789 in pmu_add_cpu_aliases (head=0x7fffffffcf90, name=0x2450903 "uncore_cha_9") at util/pmu.c:571
> >     #'7  0x00000000005188ac in pmu_lookup (name=0x2450903 "uncore_cha_9") at util/pmu.c:613
> >     #'8  0x0000000000518aff in perf_pmu__find (name=0x2450903 "uncore_cha_9") at util/pmu.c:672
> >     #'9  0x00000000005183d5 in pmu_read_sysfs () at util/pmu.c:467
> >     #'10 0x0000000000518a54 in perf_pmu__scan (pmu=0x0) at util/pmu.c:651
> >     #'11 0x0000000000519f26 in print_pmu_events (event_glob=0x0, name_only=false, quiet_flag=false, long_desc=false, details_flag=false) at util/pmu.c:1173
> >     #'12 0x00000000004d5ef0 in print_events (event_glob=0x0, name_only=false, quiet_flag=false, long_desc=false, details_flag=false) at util/parse-events.c:2343
> >     #'13 0x000000000043c7d4 in cmd_list (argc=0, argv=0x7fffffffeb90) at builtin-list.c:56
> >     #'14 0x00000000004ab2c8 in run_builtin (p=0xa281a0 <commands+192>, argc=1, argv=0x7fffffffeb90) at perf.c:296
> >     #15 0x00000000004ab535 in handle_internal_command (argc=1, argv=0x7fffffffeb90) at perf.c:348
> >     #16 0x00000000004ab687 in run_argv (argcp=0x7fffffffe9ec, argv=0x7fffffffe9e0) at perf.c:392
> >     #17 0x00000000004aba55 in main (argc=1, argv=0x7fffffffeb90) at perf.c:530
> > 

Re: [PATCH 2/2] perf, tools: Avoid segfault on alias parse error

[Arnaldo Carvalho de Melo]

Em Thu, Aug 17, 2017 at 08:34:22AM -0700, Andi Kleen escreveu:
> > Humm, but don't we have that checked?
> 
> At least not in the case of the segfault below.

Again:

tools/perf/util/parse-events.c

2523 void parse_events_evlist_error(struct parse_events_evlist *data,
2524                                int idx, const char *str)
2525 {
2526         struct parse_events_error *err = data->error;
2527 
2528         if (!err)
2529                 return;
2530         err->idx = idx;
2531         err->str = strdup(str);
2532         WARN_ONCE(!err->str, "WARNING: failed to allocate error string");
2533 }

data->error _is_ being checked for NULL, and the hunk you added would
just change data->error from NULL (some other member is being assigned,
so all the other left unassigned will be set to zero) to something that
will then be set to something to then get ignored.

Looking at the intervening functions in the .y files...

- Arnaldo
 
> > > Segfault:
> > > 
> > >     #'0  0x00000000004d66d2 in parse_events_evlist_error (data=0x7fffffffce20, idx=6, str=0x76cda8 "parser error") at util/parse-events.c:2530
> > >     #'1  0x0000000000516d0a in parse_events_error (loc=0x7fffffffb210, data=0x7fffffffce20, scanner=0x245b400, msg=0x76cb13 "syntax error") at util/parse-events.y:692
> > >     #'2  0x000000000051675b in parse_events_parse (_data=0x7fffffffce20, scanner=0x245b400) at /home/ak/hle/obj-perf/util/parse-events-bison.c:2213
> > >     #'3  0x00000000004d3fd6 in parse_events__scanner (str=0x725cff "event=0,", data=0x7fffffffce20, start_token=259) at util/parse-events.c:1646
> > >     #'4  0x00000000004d4063 in parse_events_terms (terms=0x245b398, str=0x725cff "event=0,") at util/parse-events.c:1664
> > >     #'5  0x00000000005179f1 in __perf_pmu__new_alias (list=0x7fffffffcf90, dir=0x0, name=0x725cec "unc_cha_clockticks", desc=0x725d08 "Uncore cache clock ticks. Unit: uncore_cha ",
> > >     val=0x725cff "event=0,", long_desc=0x0, topic=0x725d34 "uncore other", unit=0x0, perpkg=0x6ca7c6 "1", metric_expr=0x0, metric_name=0x0) at util/pmu.c:255
> > >     #'6  0x0000000000518789 in pmu_add_cpu_aliases (head=0x7fffffffcf90, name=0x2450903 "uncore_cha_9") at util/pmu.c:571
> > >     #'7  0x00000000005188ac in pmu_lookup (name=0x2450903 "uncore_cha_9") at util/pmu.c:613
> > >     #'8  0x0000000000518aff in perf_pmu__find (name=0x2450903 "uncore_cha_9") at util/pmu.c:672
> > >     #'9  0x00000000005183d5 in pmu_read_sysfs () at util/pmu.c:467
> > >     #'10 0x0000000000518a54 in perf_pmu__scan (pmu=0x0) at util/pmu.c:651
> > >     #'11 0x0000000000519f26 in print_pmu_events (event_glob=0x0, name_only=false, quiet_flag=false, long_desc=false, details_flag=false) at util/pmu.c:1173
> > >     #'12 0x00000000004d5ef0 in print_events (event_glob=0x0, name_only=false, quiet_flag=false, long_desc=false, details_flag=false) at util/parse-events.c:2343
> > >     #'13 0x000000000043c7d4 in cmd_list (argc=0, argv=0x7fffffffeb90) at builtin-list.c:56
> > >     #'14 0x00000000004ab2c8 in run_builtin (p=0xa281a0 <commands+192>, argc=1, argv=0x7fffffffeb90) at perf.c:296
> > >     #15 0x00000000004ab535 in handle_internal_command (argc=1, argv=0x7fffffffeb90) at perf.c:348
> > >     #16 0x00000000004ab687 in run_argv (argcp=0x7fffffffe9ec, argv=0x7fffffffe9e0) at perf.c:392
> > >     #17 0x00000000004aba55 in main (argc=1, argv=0x7fffffffeb90) at perf.c:530
> > > 

Re: [PATCH 2/2] perf, tools: Avoid segfault on alias parse error

[Andi Kleen]

On Thu, Aug 17, 2017 at 01:25:39PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Aug 17, 2017 at 08:34:22AM -0700, Andi Kleen escreveu:
> > > Humm, but don't we have that checked?
> > 
> > At least not in the case of the segfault below.
> 
> Again:
> 
> tools/perf/util/parse-events.c
> 
> 2523 void parse_events_evlist_error(struct parse_events_evlist *data,
> 2524                                int idx, const char *str)
> 2525 {
> 2526         struct parse_events_error *err = data->error;
> 2527 
> 2528         if (!err)
> 2529                 return;
> 2530         err->idx = idx;
> 2531         err->str = strdup(str);
> 2532         WARN_ONCE(!err->str, "WARNING: failed to allocate error string");
> 2533 }
> 
> data->error _is_ being checked for NULL, and the hunk you added would

In the crash, data->error is not NULL, it's random stack contents
because the data types for the data pointer do not match
(parse_events_evlist vs parse_events_terms)

-Andi

Re: [PATCH 2/2] perf, tools: Avoid segfault on alias parse error

[Arnaldo Carvalho de Melo]

Em Thu, Aug 17, 2017 at 12:28:16PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Wed, Aug 16, 2017 at 03:02:01PM -0700, Andi Kleen escreveu:
> > +++ b/tools/perf/util/parse-events.h
> > @@ -108,15 +108,17 @@ struct parse_events_error {
> >  	char *help;	/* optional help string */
> >  };
> >  
> > +/* error field must match parse_events_terms */

Argh, this seems to be the oddity that causes these problems, a void
pointer is passed around and sometimes code thinks it is a
struct_parse_events_evlist pointer, like in

#line 117 "util/parse-events.y" /* yacc.c:1646  */
    {
        struct parse_events_evlist *data = _data;

        parse_events_update_lists((yyvsp[0].head), &data->list);
}
#line 1503 "/tmp/build/perf/util/parse-events-bison.c" /* yacc.c:1646  */

while sometimes it thinks it is something else, like in:

  case 52: 
#line 496 "util/parse-events.y" /* yacc.c:1646  */ 
    { 
        struct parse_events_terms *data = _data; 
        data->terms = (yyvsp[0].head); 
} 
#line 1931 "/tmp/build/perf/util/parse-events-bison.c" /* yacc.c:1646  */

So it will _not_ set to null a member that it doesn't have, i.e. the
minimal fix is to just have the hunks below, making sure that the error
field is present in both structs. No need to set
parse_event_terms->error to anything, it will be set to null since other
fields are set to something.

Longer term we need to fix this mess wrt that "_data" void pointer...

- Arnaldo

> >  struct parse_events_evlist {
> > +	struct parse_events_error *error;
> >  	struct list_head	   list;
> >  	int			   idx;
> >  	int			   nr_groups;
> > -	struct parse_events_error *error;
> >  	struct perf_evlist	  *evlist;
> >  };
> >  
> >  struct parse_events_terms {
> > +	struct parse_events_error *error;
> >  	struct list_head *terms;
> >  };
> >  
> > -- 
> > 2.9.4

Re: [PATCH 2/2] perf, tools: Avoid segfault on alias parse error

[Andi Kleen]

> So it will _not_ set to null a member that it doesn't have, i.e. the
> minimal fix is to just have the hunks below, making sure that the error
> field is present in both structs. No need to set
> parse_event_terms->error to anything, it will be set to null since other
> fields are set to something.

Right but I want to see the error too. That is why I added a real 
error handler.

-Andi

Re: [PATCH 2/2] perf, tools: Avoid segfault on alias parse error

[Arnaldo Carvalho de Melo]

Em Thu, Aug 17, 2017 at 10:04:53AM -0700, Andi Kleen escreveu:
> > So it will _not_ set to null a member that it doesn't have, i.e. the
> > minimal fix is to just have the hunks below, making sure that the error
> > field is present in both structs. No need to set
> > parse_event_terms->error to anything, it will be set to null since other
> > fields are set to something.
> 
> Right but I want to see the error too. That is why I added a real 
> error handler.

Ok, but I was first trying to figure out the fix for that separate
problem (the void pointer being dereferenced to two different types in
the same function, yyparse, aka parse_events_parse), then we can go to
the 'too' part :-)

I'm adding the patch below, that should fix things by using just one
state struct for parsing.

This is done on top of another that renames 'struct parse_events_evlist'
to a more clearer 'struct parse_events_state'.

I'll then add what is left of your patch, run tests and we go from
there, holler if I'm now missing something.

- Arnaldo

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 272eab7f5ac8..d4fcf048aa9e 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1656,7 +1656,7 @@ static int parse_events__scanner(const char *str, void *data, int start_token)
  */
 int parse_events_terms(struct list_head *terms, const char *str)
 {
-	struct parse_events_terms data = {
+	struct parse_events_state data = {
 		.terms = NULL,
 	};
 	int ret;
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index 8fff8423469b..2a3617937894 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -114,10 +114,7 @@ struct parse_events_state {
 	int			   nr_groups;
 	struct parse_events_error *error;
 	struct perf_evlist	  *evlist;
-};
-
-struct parse_events_terms {
-	struct list_head *terms;
+	struct list_head	  *terms;
 };
 
 void parse_events__shrink_config_terms(void);
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index ffb4914142cf..8acb2c4ca711 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -494,7 +494,7 @@ opt_event_config:
 
 start_terms: event_config
 {
-	struct parse_events_terms *data = _data;
+	struct parse_events_state *data = _data;
 	data->terms = $1;
 }
 

Re: [PATCH 2/2] perf, tools: Avoid segfault on alias parse error

[Jiri Olsa]

On Thu, Aug 17, 2017 at 02:22:32PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Aug 17, 2017 at 10:04:53AM -0700, Andi Kleen escreveu:
> > > So it will _not_ set to null a member that it doesn't have, i.e. the
> > > minimal fix is to just have the hunks below, making sure that the error
> > > field is present in both structs. No need to set
> > > parse_event_terms->error to anything, it will be set to null since other
> > > fields are set to something.
> > 
> > Right but I want to see the error too. That is why I added a real 
> > error handler.
> 
> Ok, but I was first trying to figure out the fix for that separate
> problem (the void pointer being dereferenced to two different types in
> the same function, yyparse, aka parse_events_parse), then we can go to
> the 'too' part :-)
> 
> I'm adding the patch below, that should fix things by using just one
> state struct for parsing.
> 
> This is done on top of another that renames 'struct parse_events_evlist'
> to a more clearer 'struct parse_events_state'.
> 
> I'll then add what is left of your patch, run tests and we go from
> there, holler if I'm now missing something.

nice catch, I saw the fixes in your branch, look ok

thanks,
jirka