Re: [PATCH 05/12] perf pmu: Fix pmu events parsing rule

[Jiri Olsa]

On Mon, May 07, 2018 at 04:24:08PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, May 07, 2018 at 03:37:49PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Sat, May 05, 2018 at 08:43:11PM -0700, Andi Kleen escreveu:
> > > Jiri Olsa  writes:
> > > 
> > > Please fix this quickly, PT is currently totally non functional in Linus
> > > mainline.
> > 
> > Ok, so I'm reverting this patch, the previous situation was just a
> > misleading error message, so it can wait for the discussion about the
> > parser fixes to come to a conclusion and a proper patch to be submitted.
> 
> ... and I'm adding this to my perf/core branch, so that we notice this
> faster in the future:

sry, overlooked this one.. good idea, looks ok to me, ack

jirka

> 
> diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
> index 18b06444f230..6829dd416a99 100644
> --- a/tools/perf/tests/parse-events.c
> +++ b/tools/perf/tests/parse-events.c
> @@ -1309,6 +1309,14 @@ static int test__checkevent_config_cache(struct 
> perf_evlist *evlist)
>   return 0;
>  }
>  
> +static int test__intel_pt(struct perf_evlist *evlist)
> +{
> + struct perf_evsel *evsel = perf_evlist__first(evlist);
> +
> + TEST_ASSERT_VAL("wrong name setting", strcmp(evsel->name, 
> "intel_pt//u") == 0);
> + return 0;
> +}
> +
>  static int count_tracepoints(void)
>  {
>   struct dirent *events_ent;
> @@ -1637,6 +1645,11 @@ static struct evlist_test test__events[] = {
>   .check = test__checkevent_config_cache,
>   .id= 51,
>   },
> + {
> + .name  = "intel_pt//u",
> + .check = test__intel_pt,
> + .id= 52,
> + },
>  };
>  
>  static struct evlist_test test__events_pmu[] = {

Re: [PATCH 05/12] perf pmu: Fix pmu events parsing rule

[Jiri Olsa]

On Mon, May 07, 2018 at 04:24:08PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, May 07, 2018 at 03:37:49PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Sat, May 05, 2018 at 08:43:11PM -0700, Andi Kleen escreveu:
> > > Jiri Olsa  writes:
> > > 
> > > Please fix this quickly, PT is currently totally non functional in Linus
> > > mainline.
> > 
> > Ok, so I'm reverting this patch, the previous situation was just a
> > misleading error message, so it can wait for the discussion about the
> > parser fixes to come to a conclusion and a proper patch to be submitted.
> 
> ... and I'm adding this to my perf/core branch, so that we notice this
> faster in the future:

sry, overlooked this one.. good idea, looks ok to me, ack

jirka

> 
> diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
> index 18b06444f230..6829dd416a99 100644
> --- a/tools/perf/tests/parse-events.c
> +++ b/tools/perf/tests/parse-events.c
> @@ -1309,6 +1309,14 @@ static int test__checkevent_config_cache(struct 
> perf_evlist *evlist)
>   return 0;
>  }
>  
> +static int test__intel_pt(struct perf_evlist *evlist)
> +{
> + struct perf_evsel *evsel = perf_evlist__first(evlist);
> +
> + TEST_ASSERT_VAL("wrong name setting", strcmp(evsel->name, 
> "intel_pt//u") == 0);
> + return 0;
> +}
> +
>  static int count_tracepoints(void)
>  {
>   struct dirent *events_ent;
> @@ -1637,6 +1645,11 @@ static struct evlist_test test__events[] = {
>   .check = test__checkevent_config_cache,
>   .id= 51,
>   },
> + {
> + .name  = "intel_pt//u",
> + .check = test__intel_pt,
> + .id= 52,
> + },
>  };
>  
>  static struct evlist_test test__events_pmu[] = {

Re: [PATCH 05/12] perf pmu: Fix pmu events parsing rule

[Arnaldo Carvalho de Melo]

Em Mon, May 07, 2018 at 09:16:53PM +0200, Jiri Olsa escreveu:
> On Mon, May 07, 2018 at 03:37:49PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Sat, May 05, 2018 at 08:43:11PM -0700, Andi Kleen escreveu:
> > > Jiri Olsa  writes:
> > > 
> > > Please fix this quickly, PT is currently totally non functional in Linus
> > > mainline.
> > 
> > Ok, so I'm reverting this patch, the previous situation was just a
> > misleading error message, so it can wait for the discussion about the
> > parser fixes to come to a conclusion and a proper patch to be submitted.
> 
> ok, I was waiting for some feedback on the new patch.. I'll merge
> it with the one you reverted now

Ok!

- Arnaldo

Re: [PATCH 05/12] perf pmu: Fix pmu events parsing rule

[Arnaldo Carvalho de Melo]

Em Mon, May 07, 2018 at 09:16:53PM +0200, Jiri Olsa escreveu:
> On Mon, May 07, 2018 at 03:37:49PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Sat, May 05, 2018 at 08:43:11PM -0700, Andi Kleen escreveu:
> > > Jiri Olsa  writes:
> > > 
> > > Please fix this quickly, PT is currently totally non functional in Linus
> > > mainline.
> > 
> > Ok, so I'm reverting this patch, the previous situation was just a
> > misleading error message, so it can wait for the discussion about the
> > parser fixes to come to a conclusion and a proper patch to be submitted.
> 
> ok, I was waiting for some feedback on the new patch.. I'll merge
> it with the one you reverted now

Ok!

- Arnaldo

Re: [PATCH 05/12] perf pmu: Fix pmu events parsing rule

[Arnaldo Carvalho de Melo]

Em Mon, May 07, 2018 at 03:37:49PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Sat, May 05, 2018 at 08:43:11PM -0700, Andi Kleen escreveu:
> > Jiri Olsa  writes:
> > 
> > Please fix this quickly, PT is currently totally non functional in Linus
> > mainline.
> 
> Ok, so I'm reverting this patch, the previous situation was just a
> misleading error message, so it can wait for the discussion about the
> parser fixes to come to a conclusion and a proper patch to be submitted.

... and I'm adding this to my perf/core branch, so that we notice this
faster in the future:

diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
index 18b06444f230..6829dd416a99 100644
--- a/tools/perf/tests/parse-events.c
+++ b/tools/perf/tests/parse-events.c
@@ -1309,6 +1309,14 @@ static int test__checkevent_config_cache(struct 
perf_evlist *evlist)
return 0;
 }
 
+static int test__intel_pt(struct perf_evlist *evlist)
+{
+   struct perf_evsel *evsel = perf_evlist__first(evlist);
+
+   TEST_ASSERT_VAL("wrong name setting", strcmp(evsel->name, 
"intel_pt//u") == 0);
+   return 0;
+}
+
 static int count_tracepoints(void)
 {
struct dirent *events_ent;
@@ -1637,6 +1645,11 @@ static struct evlist_test test__events[] = {
.check = test__checkevent_config_cache,
.id= 51,
},
+   {
+   .name  = "intel_pt//u",
+   .check = test__intel_pt,
+   .id= 52,
+   },
 };
 
 static struct evlist_test test__events_pmu[] = {

Re: [PATCH 05/12] perf pmu: Fix pmu events parsing rule

[Arnaldo Carvalho de Melo]

Em Mon, May 07, 2018 at 03:37:49PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Sat, May 05, 2018 at 08:43:11PM -0700, Andi Kleen escreveu:
> > Jiri Olsa  writes:
> > 
> > Please fix this quickly, PT is currently totally non functional in Linus
> > mainline.
> 
> Ok, so I'm reverting this patch, the previous situation was just a
> misleading error message, so it can wait for the discussion about the
> parser fixes to come to a conclusion and a proper patch to be submitted.

... and I'm adding this to my perf/core branch, so that we notice this
faster in the future:

diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
index 18b06444f230..6829dd416a99 100644
--- a/tools/perf/tests/parse-events.c
+++ b/tools/perf/tests/parse-events.c
@@ -1309,6 +1309,14 @@ static int test__checkevent_config_cache(struct 
perf_evlist *evlist)
return 0;
 }
 
+static int test__intel_pt(struct perf_evlist *evlist)
+{
+   struct perf_evsel *evsel = perf_evlist__first(evlist);
+
+   TEST_ASSERT_VAL("wrong name setting", strcmp(evsel->name, 
"intel_pt//u") == 0);
+   return 0;
+}
+
 static int count_tracepoints(void)
 {
struct dirent *events_ent;
@@ -1637,6 +1645,11 @@ static struct evlist_test test__events[] = {
.check = test__checkevent_config_cache,
.id= 51,
},
+   {
+   .name  = "intel_pt//u",
+   .check = test__intel_pt,
+   .id= 52,
+   },
 };
 
 static struct evlist_test test__events_pmu[] = {

Re: [PATCH 05/12] perf pmu: Fix pmu events parsing rule

[Jiri Olsa]

On Mon, May 07, 2018 at 03:37:49PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Sat, May 05, 2018 at 08:43:11PM -0700, Andi Kleen escreveu:
> > Jiri Olsa  writes:
> > 
> > Please fix this quickly, PT is currently totally non functional in Linus
> > mainline.
> 
> Ok, so I'm reverting this patch, the previous situation was just a
> misleading error message, so it can wait for the discussion about the
> parser fixes to come to a conclusion and a proper patch to be submitted.

ok, I was waiting for some feedback on the new patch.. I'll merge
it with the one you reverted now

thanks,
jirka

Re: [PATCH 05/12] perf pmu: Fix pmu events parsing rule

[Jiri Olsa]

On Mon, May 07, 2018 at 03:37:49PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Sat, May 05, 2018 at 08:43:11PM -0700, Andi Kleen escreveu:
> > Jiri Olsa  writes:
> > 
> > Please fix this quickly, PT is currently totally non functional in Linus
> > mainline.
> 
> Ok, so I'm reverting this patch, the previous situation was just a
> misleading error message, so it can wait for the discussion about the
> parser fixes to come to a conclusion and a proper patch to be submitted.

ok, I was waiting for some feedback on the new patch.. I'll merge
it with the one you reverted now

thanks,
jirka

Re: [PATCH 05/12] perf pmu: Fix pmu events parsing rule

[Arnaldo Carvalho de Melo]

Em Sat, May 05, 2018 at 08:43:11PM -0700, Andi Kleen escreveu:
> Jiri Olsa  writes:
> 
> Please fix this quickly, PT is currently totally non functional in Linus
> mainline.

Ok, so I'm reverting this patch, the previous situation was just a
misleading error message, so it can wait for the discussion about the
parser fixes to come to a conclusion and a proper patch to be submitted.

- Arnaldo

Re: [PATCH 05/12] perf pmu: Fix pmu events parsing rule

[Arnaldo Carvalho de Melo]

Em Sat, May 05, 2018 at 08:43:11PM -0700, Andi Kleen escreveu:
> Jiri Olsa  writes:
> 
> Please fix this quickly, PT is currently totally non functional in Linus
> mainline.

Ok, so I'm reverting this patch, the previous situation was just a
misleading error message, so it can wait for the discussion about the
parser fixes to come to a conclusion and a proper patch to be submitted.

- Arnaldo

Re: [PATCH 05/12] perf pmu: Fix pmu events parsing rule

[Andi Kleen]

Jiri Olsa  writes:

Please fix this quickly, PT is currently totally non functional in Linus
mainline.

-Andi

Re: [PATCH 05/12] perf pmu: Fix pmu events parsing rule

[Andi Kleen]

Jiri Olsa  writes:

Please fix this quickly, PT is currently totally non functional in Linus
mainline.

-Andi

Re: [PATCH 05/12] perf pmu: Fix pmu events parsing rule

[Jiri Olsa]

On Thu, May 03, 2018 at 02:38:15PM +0300, Adrian Hunter wrote:

SNIP

> >>> index 7afeb80cc39e..d14464c42714 100644
> >>> --- a/tools/perf/util/parse-events.y
> >>> +++ b/tools/perf/util/parse-events.y
> >>> @@ -224,15 +224,15 @@ event_def: event_pmu |
> >>>  event_bpf_file
> >>>  
> >>>  event_pmu:
> >>> -PE_NAME opt_event_config
> >>> +PE_NAME '/' event_config '/'
> >>
> >> These are not equivalent because opt_event_config allows '//'
> >> but event_config cannot be an empty string.
> > 
> > yep, overlooked this one, how about patch below
> 
> Seems to work but gives build warnings:
> 
> util/parse-events.y: warning: 1 shift/reduce conflict [-Wconflicts-sr]
> util/parse-events.y: warning: 1 reduce/reduce conflict [-Wconflicts-rr]

I had to make an explicit rule, so I moved the original rule code
into separate function.. hence the patch is little bigger ;-)

cc-ing Kan Liang, who recently changed this code,

I'll still have to rebase this one his recent change,
once it gets in, but should be easy

jirka


---
 tools/perf/util/parse-events.c | 43 +++
 tools/perf/util/parse-events.h |  3 +++
 tools/perf/util/parse-events.y | 51 +-
 3 files changed, 62 insertions(+), 35 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 2fb0272146d8..33e3fc147f35 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -8,6 +8,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "term.h"
 #include "../perf.h"
 #include "evlist.h"
@@ -1287,6 +1288,48 @@ int parse_events_add_pmu(struct parse_events_state 
*parse_state,
return evsel ? 0 : -ENOMEM;
 }
 
+int parse_events_pmu(struct parse_events_state *parse_state,
+struct list_head *list, char *name,
+struct list_head *head_config)
+{
+   struct list_head *orig_terms;
+   struct perf_pmu *pmu = NULL;
+   char *pattern = NULL;
+   int ok = 0;
+
+   if (!parse_events_add_pmu(parse_state, list, name, head_config, false))
+   return 0;
+
+   if (parse_events_copy_term_list(head_config, _terms))
+   return -1;
+
+   if (asprintf(, "%s*", name) < 0)
+   goto out;
+
+   while ((pmu = perf_pmu__scan(pmu)) != NULL) {
+   struct list_head *terms;
+   char *tmp = pmu->name;
+
+   if (!strncmp(tmp, "uncore_", 7) &&
+   strncmp(tmp, "uncore_", 7))
+   tmp += 7;
+   if (!fnmatch(pattern, tmp, 0)) {
+   if (parse_events_copy_term_list(orig_terms, )) {
+   ok = 0;
+   goto out;
+   }
+   if (!parse_events_add_pmu(parse_state, list, pmu->name, 
terms, true))
+   ok++;
+   parse_events_terms__delete(terms);
+   }
+   }
+
+out:
+   parse_events_terms__delete(orig_terms);
+   free(pattern);
+   return ok ? 0 : -1;
+}
+
 int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
   char *str, struct list_head **listp)
 {
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index 5015cfd58277..743586213ee4 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -168,6 +168,9 @@ int parse_events_add_breakpoint(struct list_head *list, int 
*idx,
 int parse_events_add_pmu(struct parse_events_state *parse_state,
 struct list_head *list, char *name,
 struct list_head *head_config, bool auto_merge_stats);
+int parse_events_pmu(struct parse_events_state *parse_state,
+struct list_head *list, char *name,
+struct list_head *head_config);
 
 int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
   char *str,
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index d14464c42714..a93f8c660395 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -8,7 +8,6 @@
 
 #define YYDEBUG 1
 
-#include 
 #include 
 #include 
 #include 
@@ -226,44 +225,26 @@ event_def: event_pmu |
 event_pmu:
 PE_NAME '/' event_config '/'
 {
-   struct list_head *list, *orig_terms, *terms;
+   struct list_head *list;
+
+   ALLOC_LIST(list);
 
-   if (parse_events_copy_term_list($3, _terms))
+   if (parse_events_pmu(_parse_state, list, $1, $3))
YYABORT;
 
-   ALLOC_LIST(list);
-   if (parse_events_add_pmu(_parse_state, list, $1, $3, false)) {
-   struct perf_pmu *pmu = NULL;
-   int ok = 0;
-   char *pattern;
-
-   if (asprintf(, "%s*", $1) < 0)
-   YYABORT;
-
-   while ((pmu = perf_pmu__scan(pmu)) != NULL) 

Re: [PATCH 05/12] perf pmu: Fix pmu events parsing rule

[Jiri Olsa]

On Thu, May 03, 2018 at 02:38:15PM +0300, Adrian Hunter wrote:

SNIP

> >>> index 7afeb80cc39e..d14464c42714 100644
> >>> --- a/tools/perf/util/parse-events.y
> >>> +++ b/tools/perf/util/parse-events.y
> >>> @@ -224,15 +224,15 @@ event_def: event_pmu |
> >>>  event_bpf_file
> >>>  
> >>>  event_pmu:
> >>> -PE_NAME opt_event_config
> >>> +PE_NAME '/' event_config '/'
> >>
> >> These are not equivalent because opt_event_config allows '//'
> >> but event_config cannot be an empty string.
> > 
> > yep, overlooked this one, how about patch below
> 
> Seems to work but gives build warnings:
> 
> util/parse-events.y: warning: 1 shift/reduce conflict [-Wconflicts-sr]
> util/parse-events.y: warning: 1 reduce/reduce conflict [-Wconflicts-rr]

I had to make an explicit rule, so I moved the original rule code
into separate function.. hence the patch is little bigger ;-)

cc-ing Kan Liang, who recently changed this code,

I'll still have to rebase this one his recent change,
once it gets in, but should be easy

jirka


---
 tools/perf/util/parse-events.c | 43 +++
 tools/perf/util/parse-events.h |  3 +++
 tools/perf/util/parse-events.y | 51 +-
 3 files changed, 62 insertions(+), 35 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 2fb0272146d8..33e3fc147f35 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -8,6 +8,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "term.h"
 #include "../perf.h"
 #include "evlist.h"
@@ -1287,6 +1288,48 @@ int parse_events_add_pmu(struct parse_events_state 
*parse_state,
return evsel ? 0 : -ENOMEM;
 }
 
+int parse_events_pmu(struct parse_events_state *parse_state,
+struct list_head *list, char *name,
+struct list_head *head_config)
+{
+   struct list_head *orig_terms;
+   struct perf_pmu *pmu = NULL;
+   char *pattern = NULL;
+   int ok = 0;
+
+   if (!parse_events_add_pmu(parse_state, list, name, head_config, false))
+   return 0;
+
+   if (parse_events_copy_term_list(head_config, _terms))
+   return -1;
+
+   if (asprintf(, "%s*", name) < 0)
+   goto out;
+
+   while ((pmu = perf_pmu__scan(pmu)) != NULL) {
+   struct list_head *terms;
+   char *tmp = pmu->name;
+
+   if (!strncmp(tmp, "uncore_", 7) &&
+   strncmp(tmp, "uncore_", 7))
+   tmp += 7;
+   if (!fnmatch(pattern, tmp, 0)) {
+   if (parse_events_copy_term_list(orig_terms, )) {
+   ok = 0;
+   goto out;
+   }
+   if (!parse_events_add_pmu(parse_state, list, pmu->name, 
terms, true))
+   ok++;
+   parse_events_terms__delete(terms);
+   }
+   }
+
+out:
+   parse_events_terms__delete(orig_terms);
+   free(pattern);
+   return ok ? 0 : -1;
+}
+
 int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
   char *str, struct list_head **listp)
 {
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index 5015cfd58277..743586213ee4 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -168,6 +168,9 @@ int parse_events_add_breakpoint(struct list_head *list, int 
*idx,
 int parse_events_add_pmu(struct parse_events_state *parse_state,
 struct list_head *list, char *name,
 struct list_head *head_config, bool auto_merge_stats);
+int parse_events_pmu(struct parse_events_state *parse_state,
+struct list_head *list, char *name,
+struct list_head *head_config);
 
 int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
   char *str,
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index d14464c42714..a93f8c660395 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -8,7 +8,6 @@
 
 #define YYDEBUG 1
 
-#include 
 #include 
 #include 
 #include 
@@ -226,44 +225,26 @@ event_def: event_pmu |
 event_pmu:
 PE_NAME '/' event_config '/'
 {
-   struct list_head *list, *orig_terms, *terms;
+   struct list_head *list;
+
+   ALLOC_LIST(list);
 
-   if (parse_events_copy_term_list($3, _terms))
+   if (parse_events_pmu(_parse_state, list, $1, $3))
YYABORT;
 
-   ALLOC_LIST(list);
-   if (parse_events_add_pmu(_parse_state, list, $1, $3, false)) {
-   struct perf_pmu *pmu = NULL;
-   int ok = 0;
-   char *pattern;
-
-   if (asprintf(, "%s*", $1) < 0)
-   YYABORT;
-
-   while ((pmu = perf_pmu__scan(pmu)) != NULL) 

Re: [PATCH 05/12] perf pmu: Fix pmu events parsing rule

[Jiri Olsa]

On Thu, May 03, 2018 at 02:38:15PM +0300, Adrian Hunter wrote:
> On 03/05/18 13:37, Jiri Olsa wrote:
> > On Thu, May 03, 2018 at 11:25:16AM +0300, Adrian Hunter wrote:
> >> Hi
> >>
> >> This breaks Intel PT i.e.
> >>
> >> $ perf record -e intel_pt//u uname
> >> event syntax error: 'intel_pt//u'
> >>  \___ parser error
> >> Run 'perf list' for a list of valid events
> >>
> >>  Usage: perf record [] []
> >> or: perf record [] --  []
> >>
> >> -e, --eventevent selector. use 'perf list' to list 
> >> available events
> >>
> >> See below for cause.
> >>
> >> On 25/04/18 19:00, Arnaldo Carvalho de Melo wrote:
> >>> From: Jiri Olsa 
> >>>
> >>> Currently all the event parsing fails end up in the event_pmu rule, and
> >>> display misleading help like:
> >>>
> >>>   $ perf stat -e inst kill
> >>>   event syntax error: 'inst'
> >>>\___ Cannot find PMU `inst'. Missing kernel 
> >>> support?
> >>>   ...
> >>>
> >>> The reason is that the event_pmu is too strong and match also single
> >>> string. Changing it to force the '/' separators to be part of the rule,
> >>> and getting the proper error now:
> >>>
> >>>   $ perf stat -e inst kill
> >>>   event syntax error: 'inst'
> >>>\___ parser error
> >>>   Run 'perf list' for a list of valid events
> >>>   ...
> >>>
> >>> Signed-off-by: Jiri Olsa 
> >>> Reported-by: Ingo Molnar 
> >>> Tested-by: Arnaldo Carvalho de Melo 
> >>> Cc: Alexander Shishkin 
> >>> Cc: David Ahern 
> >>> Cc: Namhyung Kim 
> >>> Cc: Peter Zijlstra 
> >>> Link: http://lkml.kernel.org/r/20180423090823.32309-5-jo...@kernel.org
> >>> Signed-off-by: Arnaldo Carvalho de Melo 
> >>> ---
> >>>  tools/perf/util/parse-events.y | 8 
> >>>  1 file changed, 4 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/tools/perf/util/parse-events.y 
> >>> b/tools/perf/util/parse-events.y
> >>> index 7afeb80cc39e..d14464c42714 100644
> >>> --- a/tools/perf/util/parse-events.y
> >>> +++ b/tools/perf/util/parse-events.y
> >>> @@ -224,15 +224,15 @@ event_def: event_pmu |
> >>>  event_bpf_file
> >>>  
> >>>  event_pmu:
> >>> -PE_NAME opt_event_config
> >>> +PE_NAME '/' event_config '/'
> >>
> >> These are not equivalent because opt_event_config allows '//'
> >> but event_config cannot be an empty string.
> > 
> > yep, overlooked this one, how about patch below
> 
> Seems to work but gives build warnings:
> 
> util/parse-events.y: warning: 1 shift/reduce conflict [-Wconflicts-sr]
> util/parse-events.y: warning: 1 reduce/reduce conflict [-Wconflicts-rr]

hm, I wonder why my bison was silent.. there's conflict
with opt_event_config reduction to empty, I'll resend

thanks,
jirka

Re: [PATCH 05/12] perf pmu: Fix pmu events parsing rule

[Jiri Olsa]

On Thu, May 03, 2018 at 02:38:15PM +0300, Adrian Hunter wrote:
> On 03/05/18 13:37, Jiri Olsa wrote:
> > On Thu, May 03, 2018 at 11:25:16AM +0300, Adrian Hunter wrote:
> >> Hi
> >>
> >> This breaks Intel PT i.e.
> >>
> >> $ perf record -e intel_pt//u uname
> >> event syntax error: 'intel_pt//u'
> >>  \___ parser error
> >> Run 'perf list' for a list of valid events
> >>
> >>  Usage: perf record [] []
> >> or: perf record [] --  []
> >>
> >> -e, --eventevent selector. use 'perf list' to list 
> >> available events
> >>
> >> See below for cause.
> >>
> >> On 25/04/18 19:00, Arnaldo Carvalho de Melo wrote:
> >>> From: Jiri Olsa 
> >>>
> >>> Currently all the event parsing fails end up in the event_pmu rule, and
> >>> display misleading help like:
> >>>
> >>>   $ perf stat -e inst kill
> >>>   event syntax error: 'inst'
> >>>\___ Cannot find PMU `inst'. Missing kernel 
> >>> support?
> >>>   ...
> >>>
> >>> The reason is that the event_pmu is too strong and match also single
> >>> string. Changing it to force the '/' separators to be part of the rule,
> >>> and getting the proper error now:
> >>>
> >>>   $ perf stat -e inst kill
> >>>   event syntax error: 'inst'
> >>>\___ parser error
> >>>   Run 'perf list' for a list of valid events
> >>>   ...
> >>>
> >>> Signed-off-by: Jiri Olsa 
> >>> Reported-by: Ingo Molnar 
> >>> Tested-by: Arnaldo Carvalho de Melo 
> >>> Cc: Alexander Shishkin 
> >>> Cc: David Ahern 
> >>> Cc: Namhyung Kim 
> >>> Cc: Peter Zijlstra 
> >>> Link: http://lkml.kernel.org/r/20180423090823.32309-5-jo...@kernel.org
> >>> Signed-off-by: Arnaldo Carvalho de Melo 
> >>> ---
> >>>  tools/perf/util/parse-events.y | 8 
> >>>  1 file changed, 4 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/tools/perf/util/parse-events.y 
> >>> b/tools/perf/util/parse-events.y
> >>> index 7afeb80cc39e..d14464c42714 100644
> >>> --- a/tools/perf/util/parse-events.y
> >>> +++ b/tools/perf/util/parse-events.y
> >>> @@ -224,15 +224,15 @@ event_def: event_pmu |
> >>>  event_bpf_file
> >>>  
> >>>  event_pmu:
> >>> -PE_NAME opt_event_config
> >>> +PE_NAME '/' event_config '/'
> >>
> >> These are not equivalent because opt_event_config allows '//'
> >> but event_config cannot be an empty string.
> > 
> > yep, overlooked this one, how about patch below
> 
> Seems to work but gives build warnings:
> 
> util/parse-events.y: warning: 1 shift/reduce conflict [-Wconflicts-sr]
> util/parse-events.y: warning: 1 reduce/reduce conflict [-Wconflicts-rr]

hm, I wonder why my bison was silent.. there's conflict
with opt_event_config reduction to empty, I'll resend

thanks,
jirka

Re: [PATCH 05/12] perf pmu: Fix pmu events parsing rule

[Adrian Hunter]

On 03/05/18 13:37, Jiri Olsa wrote:
> On Thu, May 03, 2018 at 11:25:16AM +0300, Adrian Hunter wrote:
>> Hi
>>
>> This breaks Intel PT i.e.
>>
>> $ perf record -e intel_pt//u uname
>> event syntax error: 'intel_pt//u'
>>  \___ parser error
>> Run 'perf list' for a list of valid events
>>
>>  Usage: perf record [] []
>> or: perf record [] --  []
>>
>> -e, --eventevent selector. use 'perf list' to list available 
>> events
>>
>> See below for cause.
>>
>> On 25/04/18 19:00, Arnaldo Carvalho de Melo wrote:
>>> From: Jiri Olsa 
>>>
>>> Currently all the event parsing fails end up in the event_pmu rule, and
>>> display misleading help like:
>>>
>>>   $ perf stat -e inst kill
>>>   event syntax error: 'inst'
>>>\___ Cannot find PMU `inst'. Missing kernel support?
>>>   ...
>>>
>>> The reason is that the event_pmu is too strong and match also single
>>> string. Changing it to force the '/' separators to be part of the rule,
>>> and getting the proper error now:
>>>
>>>   $ perf stat -e inst kill
>>>   event syntax error: 'inst'
>>>\___ parser error
>>>   Run 'perf list' for a list of valid events
>>>   ...
>>>
>>> Signed-off-by: Jiri Olsa 
>>> Reported-by: Ingo Molnar 
>>> Tested-by: Arnaldo Carvalho de Melo 
>>> Cc: Alexander Shishkin 
>>> Cc: David Ahern 
>>> Cc: Namhyung Kim 
>>> Cc: Peter Zijlstra 
>>> Link: http://lkml.kernel.org/r/20180423090823.32309-5-jo...@kernel.org
>>> Signed-off-by: Arnaldo Carvalho de Melo 
>>> ---
>>>  tools/perf/util/parse-events.y | 8 
>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
>>> index 7afeb80cc39e..d14464c42714 100644
>>> --- a/tools/perf/util/parse-events.y
>>> +++ b/tools/perf/util/parse-events.y
>>> @@ -224,15 +224,15 @@ event_def: event_pmu |
>>>event_bpf_file
>>>  
>>>  event_pmu:
>>> -PE_NAME opt_event_config
>>> +PE_NAME '/' event_config '/'
>>
>> These are not equivalent because opt_event_config allows '//'
>> but event_config cannot be an empty string.
> 
> yep, overlooked this one, how about patch below

Seems to work but gives build warnings:

util/parse-events.y: warning: 1 shift/reduce conflict [-Wconflicts-sr]
util/parse-events.y: warning: 1 reduce/reduce conflict [-Wconflicts-rr]


> 
> jirka
> 
> ---
> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> index d14464c42714..1ed2befeca8a 100644
> --- a/tools/perf/util/parse-events.y
> +++ b/tools/perf/util/parse-events.y
> @@ -523,6 +523,10 @@ event_term
>   list_add_tail(>list, head);
>   $$ = head;
>  }
> +|
> +{
> + $$ = NULL;
> +}
>  
>  event_term:
>  PE_NAME '=' PE_NAME
> 

Re: [PATCH 05/12] perf pmu: Fix pmu events parsing rule

[Adrian Hunter]

On 03/05/18 13:37, Jiri Olsa wrote:
> On Thu, May 03, 2018 at 11:25:16AM +0300, Adrian Hunter wrote:
>> Hi
>>
>> This breaks Intel PT i.e.
>>
>> $ perf record -e intel_pt//u uname
>> event syntax error: 'intel_pt//u'
>>  \___ parser error
>> Run 'perf list' for a list of valid events
>>
>>  Usage: perf record [] []
>> or: perf record [] --  []
>>
>> -e, --eventevent selector. use 'perf list' to list available 
>> events
>>
>> See below for cause.
>>
>> On 25/04/18 19:00, Arnaldo Carvalho de Melo wrote:
>>> From: Jiri Olsa 
>>>
>>> Currently all the event parsing fails end up in the event_pmu rule, and
>>> display misleading help like:
>>>
>>>   $ perf stat -e inst kill
>>>   event syntax error: 'inst'
>>>\___ Cannot find PMU `inst'. Missing kernel support?
>>>   ...
>>>
>>> The reason is that the event_pmu is too strong and match also single
>>> string. Changing it to force the '/' separators to be part of the rule,
>>> and getting the proper error now:
>>>
>>>   $ perf stat -e inst kill
>>>   event syntax error: 'inst'
>>>\___ parser error
>>>   Run 'perf list' for a list of valid events
>>>   ...
>>>
>>> Signed-off-by: Jiri Olsa 
>>> Reported-by: Ingo Molnar 
>>> Tested-by: Arnaldo Carvalho de Melo 
>>> Cc: Alexander Shishkin 
>>> Cc: David Ahern 
>>> Cc: Namhyung Kim 
>>> Cc: Peter Zijlstra 
>>> Link: http://lkml.kernel.org/r/20180423090823.32309-5-jo...@kernel.org
>>> Signed-off-by: Arnaldo Carvalho de Melo 
>>> ---
>>>  tools/perf/util/parse-events.y | 8 
>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
>>> index 7afeb80cc39e..d14464c42714 100644
>>> --- a/tools/perf/util/parse-events.y
>>> +++ b/tools/perf/util/parse-events.y
>>> @@ -224,15 +224,15 @@ event_def: event_pmu |
>>>event_bpf_file
>>>  
>>>  event_pmu:
>>> -PE_NAME opt_event_config
>>> +PE_NAME '/' event_config '/'
>>
>> These are not equivalent because opt_event_config allows '//'
>> but event_config cannot be an empty string.
> 
> yep, overlooked this one, how about patch below

Seems to work but gives build warnings:

util/parse-events.y: warning: 1 shift/reduce conflict [-Wconflicts-sr]
util/parse-events.y: warning: 1 reduce/reduce conflict [-Wconflicts-rr]


> 
> jirka
> 
> ---
> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> index d14464c42714..1ed2befeca8a 100644
> --- a/tools/perf/util/parse-events.y
> +++ b/tools/perf/util/parse-events.y
> @@ -523,6 +523,10 @@ event_term
>   list_add_tail(>list, head);
>   $$ = head;
>  }
> +|
> +{
> + $$ = NULL;
> +}
>  
>  event_term:
>  PE_NAME '=' PE_NAME
> 

Re: [PATCH 05/12] perf pmu: Fix pmu events parsing rule

[Jiri Olsa]

On Thu, May 03, 2018 at 11:25:16AM +0300, Adrian Hunter wrote:
> Hi
> 
> This breaks Intel PT i.e.
> 
> $ perf record -e intel_pt//u uname
> event syntax error: 'intel_pt//u'
>  \___ parser error
> Run 'perf list' for a list of valid events
> 
>  Usage: perf record [] []
> or: perf record [] --  []
> 
> -e, --eventevent selector. use 'perf list' to list available 
> events
> 
> See below for cause.
> 
> On 25/04/18 19:00, Arnaldo Carvalho de Melo wrote:
> > From: Jiri Olsa 
> > 
> > Currently all the event parsing fails end up in the event_pmu rule, and
> > display misleading help like:
> > 
> >   $ perf stat -e inst kill
> >   event syntax error: 'inst'
> >\___ Cannot find PMU `inst'. Missing kernel support?
> >   ...
> > 
> > The reason is that the event_pmu is too strong and match also single
> > string. Changing it to force the '/' separators to be part of the rule,
> > and getting the proper error now:
> > 
> >   $ perf stat -e inst kill
> >   event syntax error: 'inst'
> >\___ parser error
> >   Run 'perf list' for a list of valid events
> >   ...
> > 
> > Signed-off-by: Jiri Olsa 
> > Reported-by: Ingo Molnar 
> > Tested-by: Arnaldo Carvalho de Melo 
> > Cc: Alexander Shishkin 
> > Cc: David Ahern 
> > Cc: Namhyung Kim 
> > Cc: Peter Zijlstra 
> > Link: http://lkml.kernel.org/r/20180423090823.32309-5-jo...@kernel.org
> > Signed-off-by: Arnaldo Carvalho de Melo 
> > ---
> >  tools/perf/util/parse-events.y | 8 
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> > index 7afeb80cc39e..d14464c42714 100644
> > --- a/tools/perf/util/parse-events.y
> > +++ b/tools/perf/util/parse-events.y
> > @@ -224,15 +224,15 @@ event_def: event_pmu |
> >event_bpf_file
> >  
> >  event_pmu:
> > -PE_NAME opt_event_config
> > +PE_NAME '/' event_config '/'
> 
> These are not equivalent because opt_event_config allows '//'
> but event_config cannot be an empty string.

yep, overlooked this one, how about patch below

jirka

---
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index d14464c42714..1ed2befeca8a 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -523,6 +523,10 @@ event_term
list_add_tail(>list, head);
$$ = head;
 }
+|
+{
+   $$ = NULL;
+}
 
 event_term:
 PE_NAME '=' PE_NAME

Re: [PATCH 05/12] perf pmu: Fix pmu events parsing rule

[Jiri Olsa]

On Thu, May 03, 2018 at 11:25:16AM +0300, Adrian Hunter wrote:
> Hi
> 
> This breaks Intel PT i.e.
> 
> $ perf record -e intel_pt//u uname
> event syntax error: 'intel_pt//u'
>  \___ parser error
> Run 'perf list' for a list of valid events
> 
>  Usage: perf record [] []
> or: perf record [] --  []
> 
> -e, --eventevent selector. use 'perf list' to list available 
> events
> 
> See below for cause.
> 
> On 25/04/18 19:00, Arnaldo Carvalho de Melo wrote:
> > From: Jiri Olsa 
> > 
> > Currently all the event parsing fails end up in the event_pmu rule, and
> > display misleading help like:
> > 
> >   $ perf stat -e inst kill
> >   event syntax error: 'inst'
> >\___ Cannot find PMU `inst'. Missing kernel support?
> >   ...
> > 
> > The reason is that the event_pmu is too strong and match also single
> > string. Changing it to force the '/' separators to be part of the rule,
> > and getting the proper error now:
> > 
> >   $ perf stat -e inst kill
> >   event syntax error: 'inst'
> >\___ parser error
> >   Run 'perf list' for a list of valid events
> >   ...
> > 
> > Signed-off-by: Jiri Olsa 
> > Reported-by: Ingo Molnar 
> > Tested-by: Arnaldo Carvalho de Melo 
> > Cc: Alexander Shishkin 
> > Cc: David Ahern 
> > Cc: Namhyung Kim 
> > Cc: Peter Zijlstra 
> > Link: http://lkml.kernel.org/r/20180423090823.32309-5-jo...@kernel.org
> > Signed-off-by: Arnaldo Carvalho de Melo 
> > ---
> >  tools/perf/util/parse-events.y | 8 
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> > index 7afeb80cc39e..d14464c42714 100644
> > --- a/tools/perf/util/parse-events.y
> > +++ b/tools/perf/util/parse-events.y
> > @@ -224,15 +224,15 @@ event_def: event_pmu |
> >event_bpf_file
> >  
> >  event_pmu:
> > -PE_NAME opt_event_config
> > +PE_NAME '/' event_config '/'
> 
> These are not equivalent because opt_event_config allows '//'
> but event_config cannot be an empty string.

yep, overlooked this one, how about patch below

jirka

---
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index d14464c42714..1ed2befeca8a 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -523,6 +523,10 @@ event_term
list_add_tail(>list, head);
$$ = head;
 }
+|
+{
+   $$ = NULL;
+}
 
 event_term:
 PE_NAME '=' PE_NAME

Re: [PATCH 05/12] perf pmu: Fix pmu events parsing rule

[Adrian Hunter]

Hi

This breaks Intel PT i.e.

$ perf record -e intel_pt//u uname
event syntax error: 'intel_pt//u'
 \___ parser error
Run 'perf list' for a list of valid events

 Usage: perf record [] []
or: perf record [] --  []

-e, --eventevent selector. use 'perf list' to list available 
events

See below for cause.

On 25/04/18 19:00, Arnaldo Carvalho de Melo wrote:
> From: Jiri Olsa 
> 
> Currently all the event parsing fails end up in the event_pmu rule, and
> display misleading help like:
> 
>   $ perf stat -e inst kill
>   event syntax error: 'inst'
>\___ Cannot find PMU `inst'. Missing kernel support?
>   ...
> 
> The reason is that the event_pmu is too strong and match also single
> string. Changing it to force the '/' separators to be part of the rule,
> and getting the proper error now:
> 
>   $ perf stat -e inst kill
>   event syntax error: 'inst'
>\___ parser error
>   Run 'perf list' for a list of valid events
>   ...
> 
> Signed-off-by: Jiri Olsa 
> Reported-by: Ingo Molnar 
> Tested-by: Arnaldo Carvalho de Melo 
> Cc: Alexander Shishkin 
> Cc: David Ahern 
> Cc: Namhyung Kim 
> Cc: Peter Zijlstra 
> Link: http://lkml.kernel.org/r/20180423090823.32309-5-jo...@kernel.org
> Signed-off-by: Arnaldo Carvalho de Melo 
> ---
>  tools/perf/util/parse-events.y | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> index 7afeb80cc39e..d14464c42714 100644
> --- a/tools/perf/util/parse-events.y
> +++ b/tools/perf/util/parse-events.y
> @@ -224,15 +224,15 @@ event_def: event_pmu |
>  event_bpf_file
>  
>  event_pmu:
> -PE_NAME opt_event_config
> +PE_NAME '/' event_config '/'

These are not equivalent because opt_event_config allows '//'
but event_config cannot be an empty string.

>  {
>   struct list_head *list, *orig_terms, *terms;
>  
> - if (parse_events_copy_term_list($2, _terms))
> + if (parse_events_copy_term_list($3, _terms))
>   YYABORT;
>  
>   ALLOC_LIST(list);
> - if (parse_events_add_pmu(_parse_state, list, $1, $2, false)) {
> + if (parse_events_add_pmu(_parse_state, list, $1, $3, false)) {
>   struct perf_pmu *pmu = NULL;
>   int ok = 0;
>   char *pattern;
> @@ -262,7 +262,7 @@ PE_NAME opt_event_config
>   if (!ok)
>   YYABORT;
>   }
> - parse_events_terms__delete($2);
> + parse_events_terms__delete($3);
>   parse_events_terms__delete(orig_terms);
>   $$ = list;
>  }
> 

Re: [PATCH 05/12] perf pmu: Fix pmu events parsing rule

[Adrian Hunter]

Hi

This breaks Intel PT i.e.

$ perf record -e intel_pt//u uname
event syntax error: 'intel_pt//u'
 \___ parser error
Run 'perf list' for a list of valid events

 Usage: perf record [] []
or: perf record [] --  []

-e, --eventevent selector. use 'perf list' to list available 
events

See below for cause.

On 25/04/18 19:00, Arnaldo Carvalho de Melo wrote:
> From: Jiri Olsa 
> 
> Currently all the event parsing fails end up in the event_pmu rule, and
> display misleading help like:
> 
>   $ perf stat -e inst kill
>   event syntax error: 'inst'
>\___ Cannot find PMU `inst'. Missing kernel support?
>   ...
> 
> The reason is that the event_pmu is too strong and match also single
> string. Changing it to force the '/' separators to be part of the rule,
> and getting the proper error now:
> 
>   $ perf stat -e inst kill
>   event syntax error: 'inst'
>\___ parser error
>   Run 'perf list' for a list of valid events
>   ...
> 
> Signed-off-by: Jiri Olsa 
> Reported-by: Ingo Molnar 
> Tested-by: Arnaldo Carvalho de Melo 
> Cc: Alexander Shishkin 
> Cc: David Ahern 
> Cc: Namhyung Kim 
> Cc: Peter Zijlstra 
> Link: http://lkml.kernel.org/r/20180423090823.32309-5-jo...@kernel.org
> Signed-off-by: Arnaldo Carvalho de Melo 
> ---
>  tools/perf/util/parse-events.y | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> index 7afeb80cc39e..d14464c42714 100644
> --- a/tools/perf/util/parse-events.y
> +++ b/tools/perf/util/parse-events.y
> @@ -224,15 +224,15 @@ event_def: event_pmu |
>  event_bpf_file
>  
>  event_pmu:
> -PE_NAME opt_event_config
> +PE_NAME '/' event_config '/'

These are not equivalent because opt_event_config allows '//'
but event_config cannot be an empty string.

>  {
>   struct list_head *list, *orig_terms, *terms;
>  
> - if (parse_events_copy_term_list($2, _terms))
> + if (parse_events_copy_term_list($3, _terms))
>   YYABORT;
>  
>   ALLOC_LIST(list);
> - if (parse_events_add_pmu(_parse_state, list, $1, $2, false)) {
> + if (parse_events_add_pmu(_parse_state, list, $1, $3, false)) {
>   struct perf_pmu *pmu = NULL;
>   int ok = 0;
>   char *pattern;
> @@ -262,7 +262,7 @@ PE_NAME opt_event_config
>   if (!ok)
>   YYABORT;
>   }
> - parse_events_terms__delete($2);
> + parse_events_terms__delete($3);
>   parse_events_terms__delete(orig_terms);
>   $$ = list;
>  }
>