Skip to contentSkip to conversation

perf tools: Report event parsing errors

RFC In review Jiri Olsa 4 participants 14 messages Apr 18, 2015 – Apr 22, 2015 perf events

lore
Share & tools
lore

Jiri Olsa

author reviewer
#0 lore raw

cover perf tools: Report event parsing errors

hi,
adding support to report error from event string parsing.

This patchset contains support for standard parsing errors and more logic to recognize tracepoint and 'pmu//' terms, like:

  $ perf record -e 'sched:krava' ls
  invalid or unsupported event: 'sched:krava'
                                 \___ unknown tracepoint
  $ ./perf record -e 'cpu/even=0x1/' ls
  invalid or unsupported event: 'cpu/even=0x1/'
                                     \___ unknown term
  $ perf record -e cycles,cache-mises ls
  invalid or unsupported event: '..es,cache-mises'
                                           \___ parser error

any feedback about the error string shape would be great ;-)

Changes are also reachable in here:
git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
perf/event_parse_error

thanks
jirka

---
Jiri Olsa (6):
perf tools: Add parse_events_error interface perf tools: Add flex support for parse_events_error perf tools: Change parse_events_add_pmu interface perf tools: Add location to pmu event terms perf tools: Add term support for parse_events_error perf tools: Add tracepoint support for parse_events_error

 tools/perf/builtin-stat.c               |   2 +-
 tools/perf/tests/code-reading.c         |   2 +-
 tools/perf/tests/evsel-roundtrip-name.c |   4 +--
 tools/perf/tests/hists_cumulate.c       |   2 +-
 tools/perf/tests/hists_filter.c         |   4 +--
 tools/perf/tests/hists_link.c           |   4 +--
 tools/perf/tests/hists_output.c         |   2 +-
 tools/perf/tests/keep-tracking.c        |   4 +--
 tools/perf/tests/parse-events.c         |   2 +-
 tools/perf/tests/perf-time-to-tsc.c     |   2 +-
 tools/perf/tests/pmu.c                  |   2 +-
 tools/perf/tests/switch-tracking.c      |   8 +++---
 tools/perf/util/parse-events.c          | 135 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------
 tools/perf/util/parse-events.h          |  32 ++++++++++++++--------
 tools/perf/util/parse-events.l          |  37 ++++++++++++++++++++++---
 tools/perf/util/parse-events.y          |  35 ++++++++++++++----------
 tools/perf/util/pmu.c                   |  23 +++++++++++-----
 tools/perf/util/pmu.h                   |   6 +++--
 tools/perf/util/record.c                |   4 +--
 19 files changed, 230 insertions(+), 80 deletions(-)
author reviewer
#1 lore raw

PATCH 1/6 perf tools: Add parse_events_error interface

Adding support to return error information from parse_events function. Following struct will be populated by parse_events function on return:

  struct parse_events_error {
    int   idx;
    char *str;
  };

where 'idx' is the position in the string where the parsing failed, and 'str' contains dynamically allocated error string describing the error.

The change contains reporting function, which currently does not display anything. The code changes to supply error data for specific event types are coming in next patches. However this is what the expected output is:

  $ perf record -e 'sched:krava' ls
  invalid or unsupported event: 'sched:krava'
                                 \___ unknown tracepoint
  $ ./perf record -e 'cpu/even=0x1/' ls
  invalid or unsupported event: 'cpu/even=0x1/'
                                     \___ unknown term
  $ perf record -e cycles,cache-mises ls
  invalid or unsupported event: '..es,cache-mises'
                                           \___ parser error

The output functions cut the beginning of the event string so the error starts up to 10th character and cut the end of the string of it crosses the terminal width.

Link http://lkml.kernel.org/n/[email protected] open here
Signed-off-by Jiri Olsa <[email protected]>
---
 tools/perf/builtin-stat.c               |  2 +-
 tools/perf/tests/code-reading.c         |  2 +-
 tools/perf/tests/evsel-roundtrip-name.c |  4 +-
 tools/perf/tests/hists_cumulate.c       |  2 +-
 tools/perf/tests/hists_filter.c         |  4 +-
 tools/perf/tests/hists_link.c           |  4 +-
 tools/perf/tests/hists_output.c         |  2 +-
 tools/perf/tests/keep-tracking.c        |  4 +-
 tools/perf/tests/parse-events.c         |  2 +-
 tools/perf/tests/perf-time-to-tsc.c     |  2 +-
 tools/perf/tests/switch-tracking.c      |  8 +--
 tools/perf/util/parse-events.c          | 90 ++++++++++++++++++++++++++++++---
 tools/perf/util/parse-events.h          | 18 +++++--
 tools/perf/util/record.c                |  4 +-
 14 files changed, 116 insertions(+), 32 deletions(-)
tools/perf/builtin-stat.c +1 -1
index f7b8218785f6..3dbd8c59efc5 100644
@@ -1541,7 +1541,7 @@ static int setup_events(const char * const *attrs, unsigned len)
 	unsigned i;
 
 	for (i = 0; i < len; i++) {
-		if (parse_events(evsel_list, attrs[i]))
+		if (parse_events(evsel_list, attrs[i], NULL))
 			return -1;
 	}
 	return 0;
tools/perf/tests/code-reading.c +1 -1
index f671ec37a7c4..ca0e480e741b 100644
@@ -482,7 +482,7 @@ static int do_test_code_reading(bool try_kcore)
 		else
 			str = "cycles";
 		pr_debug("Parsing event '%s'\n", str);
-		ret = parse_events(evlist, str);
+		ret = parse_events(evlist, str, NULL);
 		if (ret < 0) {
 			pr_debug("parse_events failed\n");
 			goto out_err;
tools/perf/tests/evsel-roundtrip-name.c +2 -2
index b8d8341b383e..3fa715987a5e 100644
@@ -23,7 +23,7 @@ static int perf_evsel__roundtrip_cache_name_test(void)
 			for (i = 0; i < PERF_COUNT_HW_CACHE_RESULT_MAX; i++) {
 				__perf_evsel__hw_cache_type_op_res_name(type, op, i,
 									name, sizeof(name));
-				err = parse_events(evlist, name);
+				err = parse_events(evlist, name, NULL);
 				if (err)
 					ret = err;
 			}

@@ -71,7 +71,7 @@ static int __perf_evsel__name_array_test(const char *names[], int nr_names)
                 return -ENOMEM;
 
 	for (i = 0; i < nr_names; ++i) {
-		err = parse_events(evlist, names[i]);
+		err = parse_events(evlist, names[i], NULL);
 		if (err) {
 			pr_debug("failed to parse event '%s', err %d\n",
 				 names[i], err);
tools/perf/tests/hists_cumulate.c +1 -1
index 18619966454c..b08a95a5ca1a 100644
@@ -695,7 +695,7 @@ int test__hists_cumulate(void)
 
 	TEST_ASSERT_VAL("No memory", evlist);
 
-	err = parse_events(evlist, "cpu-clock");
+	err = parse_events(evlist, "cpu-clock", NULL);
 	if (err)
 		goto out;
tools/perf/tests/hists_filter.c +2 -2
index 59e53db7914c..108488cd71fa 100644
@@ -108,10 +108,10 @@ int test__hists_filter(void)
 
 	TEST_ASSERT_VAL("No memory", evlist);
 
-	err = parse_events(evlist, "cpu-clock");
+	err = parse_events(evlist, "cpu-clock", NULL);
 	if (err)
 		goto out;
-	err = parse_events(evlist, "task-clock");
+	err = parse_events(evlist, "task-clock", NULL);
 	if (err)
 		goto out;
tools/perf/tests/hists_link.c +2 -2
index 278ba8344c23..34c61e4d3352 100644
@@ -282,10 +282,10 @@ int test__hists_link(void)
 	if (evlist == NULL)
                 return -ENOMEM;
 
-	err = parse_events(evlist, "cpu-clock");
+	err = parse_events(evlist, "cpu-clock", NULL);
 	if (err)
 		goto out;
-	err = parse_events(evlist, "task-clock");
+	err = parse_events(evlist, "task-clock", NULL);
 	if (err)
 		goto out;
tools/perf/tests/hists_output.c +1 -1
index b52c9faea224..d8a23db80094 100644
@@ -590,7 +590,7 @@ int test__hists_output(void)
 
 	TEST_ASSERT_VAL("No memory", evlist);
 
-	err = parse_events(evlist, "cpu-clock");
+	err = parse_events(evlist, "cpu-clock", NULL);
 	if (err)
 		goto out;
tools/perf/tests/keep-tracking.c +2 -2
index 7a5ab7b0b8f6..5b171d1e338b 100644
@@ -78,8 +78,8 @@ int test__keep_tracking(void)
 
 	perf_evlist__set_maps(evlist, cpus, threads);
 
-	CHECK__(parse_events(evlist, "dummy:u"));
-	CHECK__(parse_events(evlist, "cycles:u"));
+	CHECK__(parse_events(evlist, "dummy:u", NULL));
+	CHECK__(parse_events(evlist, "cycles:u", NULL));
 
 	perf_evlist__config(evlist, &opts);
tools/perf/tests/parse-events.c +1 -1
index 3de744961739..82d2a1636f7f 100644
@@ -1571,7 +1571,7 @@ static int test_event(struct evlist_test *e)
 	if (evlist == NULL)
 		return -ENOMEM;
 
-	ret = parse_events(evlist, e->name);
+	ret = parse_events(evlist, e->name, NULL);
 	if (ret) {
 		pr_debug("failed to parse event '%s', err %d\n",
 			 e->name, ret);
tools/perf/tests/perf-time-to-tsc.c +1 -1
index f238442b238a..5f49484f1abc 100644
@@ -68,7 +68,7 @@ int test__perf_time_to_tsc(void)
 
 	perf_evlist__set_maps(evlist, cpus, threads);
 
-	CHECK__(parse_events(evlist, "cycles:u"));
+	CHECK__(parse_events(evlist, "cycles:u", NULL));
 
 	perf_evlist__config(evlist, &opts);
tools/perf/tests/switch-tracking.c +4 -4
index cc68648c7c55..0d31403ea593 100644
@@ -347,7 +347,7 @@ int test__switch_tracking(void)
 	perf_evlist__set_maps(evlist, cpus, threads);
 
 	/* First event */
-	err = parse_events(evlist, "cpu-clock:u");
+	err = parse_events(evlist, "cpu-clock:u", NULL);
 	if (err) {
 		pr_debug("Failed to parse event dummy:u\n");
 		goto out_err;

@@ -356,7 +356,7 @@ int test__switch_tracking(void)
 	cpu_clocks_evsel = perf_evlist__last(evlist);
 
 	/* Second event */
-	err = parse_events(evlist, "cycles:u");
+	err = parse_events(evlist, "cycles:u", NULL);
 	if (err) {
 		pr_debug("Failed to parse event cycles:u\n");
 		goto out_err;

@@ -371,7 +371,7 @@ int test__switch_tracking(void)
 		goto out;
 	}
 
-	err = parse_events(evlist, sched_switch);
+	err = parse_events(evlist, sched_switch, NULL);
 	if (err) {
 		pr_debug("Failed to parse event %s\n", sched_switch);
 		goto out_err;

@@ -401,7 +401,7 @@ int test__switch_tracking(void)
 	perf_evsel__set_sample_bit(cycles_evsel, TIME);
 
 	/* Fourth event */
-	err = parse_events(evlist, "dummy:u");
+	err = parse_events(evlist, "dummy:u", NULL);
 	if (err) {
 		pr_debug("Failed to parse event dummy:u\n");
 		goto out_err;
tools/perf/util/parse-events.c +82 -8

118 lines not on this page · raw message

tools/perf/util/parse-events.h +14 -4
index 52a2dda4f954..90bdd877c62e 100644
@@ -12,6 +12,7 @@
 struct list_head;
 struct perf_evsel;
 struct perf_evlist;
+struct parse_events_error;
 
 struct option;
 

@@ -29,7 +30,8 @@ const char *event_type(int type);
 
 extern int parse_events_option(const struct option *opt, const char *str,
 			       int unset);
-extern int parse_events(struct perf_evlist *evlist, const char *str);
+extern int parse_events(struct perf_evlist *evlist, const char *str,
+			struct parse_events_error *error);
 extern int parse_events_terms(struct list_head *terms, const char *str);
 extern int parse_filter(const struct option *opt, const char *str, int unset);
 

@@ -74,10 +76,16 @@ struct parse_events_term {
 	bool used;
 };
 
+struct parse_events_error {
+	int   idx;
+	char *str;
+};
+
 struct parse_events_evlist {
-	struct list_head list;
-	int idx;
-	int nr_groups;
+	struct list_head	   list;
+	int			   idx;
+	int			   nr_groups;
+	struct parse_events_error *error;
 };
 
 struct parse_events_terms {

@@ -114,6 +122,8 @@ void parse_events__set_leader(char *name, struct list_head *list);
 void parse_events_update_lists(struct list_head *list_event,
 			       struct list_head *list_all);
 void parse_events_error(void *data, void *scanner, char const *msg);
+void parse_events_evlist_error(struct parse_events_evlist *data,
+			       int idx, const char *str);
 
 void print_events(const char *event_glob, bool name_only);
tools/perf/util/record.c +2 -2
index 8acd0df88b5c..c7259a5ad185 100644
@@ -20,7 +20,7 @@ static int perf_do_probe_api(setup_probe_fn_t fn, int cpu, const char *str)
 	if (!evlist)
 		return -ENOMEM;
 
-	if (parse_events(evlist, str))
+	if (parse_events(evlist, str, NULL))
 		goto out_delete;
 
 	evsel = perf_evlist__first(evlist);

@@ -207,7 +207,7 @@ bool perf_evlist__can_select_event(struct perf_evlist *evlist, const char *str)
 	if (!temp_evlist)
 		return false;
 
-	err = parse_events(temp_evlist, str);
+	err = parse_events(temp_evlist, str, NULL);
 	if (err)
 		goto out_delete;
Signature · 1 line
1.9.3
author reviewer
#2 lore raw

PATCH 2/6 perf tools: Add flex support for parse_events_error

In reply to Jiri Olsa

Allowing flex parser to report back event parsing error, like:

  $ perf record -e cycles,cache-mises ls
  invalid or unsupported event: '..es,cache-mises'
                                           \___ parser error
Link http://lkml.kernel.org/n/[email protected] open here
Signed-off-by Jiri Olsa <[email protected]>
---
 tools/perf/util/parse-events.h |  1 -
 tools/perf/util/parse-events.l | 37 +++++++++++++++++++++++++++++++++----
 tools/perf/util/parse-events.y |  7 ++++---
 3 files changed, 37 insertions(+), 8 deletions(-)
tools/perf/util/parse-events.h +0 -1
index 90bdd877c62e..5c0def908b5e 100644
@@ -121,7 +121,6 @@ perf_pmu__parse_check(const char *name);
 void parse_events__set_leader(char *name, struct list_head *list);
 void parse_events_update_lists(struct list_head *list_event,
 			       struct list_head *list_all);
-void parse_events_error(void *data, void *scanner, char const *msg);
 void parse_events_evlist_error(struct parse_events_evlist *data,
 			       int idx, const char *str);
tools/perf/util/parse-events.l +33 -4
index 8895cf3132ab..330dd2d35f5a 100644
@@ -3,6 +3,8 @@
 %option bison-bridge
 %option prefix="parse_events_"
 %option stack
+%option bison-locations
+%option yylineno
 
 %{
 #include <errno.h>

@@ -51,6 +53,18 @@ static int str(yyscan_t scanner, int token)
 	return token;
 }
 
+#define REWIND(__alloc)				\
+do {								\
+	YYSTYPE *__yylval = parse_events_get_lval(yyscanner);	\
+	char *text = parse_events_get_text(yyscanner);		\
+								\
+	if (__alloc)						\
+		__yylval->str = strdup(text);			\
+								\
+	yycolumn -= strlen(text);				\
+	yyless(0);						\
+} while (0)
+
 static int pmu_str_check(yyscan_t scanner)
 {
 	YYSTYPE *yylval = parse_events_get_lval(scanner);

@@ -85,6 +99,13 @@ static int term(yyscan_t scanner, int type)
 	return PE_TERM;
 }
 
+#define YY_USER_ACTION					\
+do {							\
+	yylloc->last_column  = yylloc->first_column;	\
+	yylloc->first_column = yycolumn;		\
+	yycolumn += yyleng;				\
+} while (0);
+
 %}
 
 %x mem

@@ -119,6 +140,12 @@ modifier_bp	[rwx]{1,3}
 
 		if (start_token) {
 			parse_events_set_extra(NULL, yyscanner);
+			/*
+			 * The flex parser does not init locations variable
+			 * via the scan_string interface, so we need do the
+			 * init in here.
+			 */
+			yycolumn = 0;
 			return start_token;
 		}
          }

@@ -127,19 +154,21 @@ modifier_bp	[rwx]{1,3}
 <event>{
 
 {group}		{
-			BEGIN(INITIAL); yyless(0);
+			BEGIN(INITIAL);
+			REWIND(0);
 		}
 
 {event_pmu}	|
 {event}		{
-			str(yyscanner, PE_EVENT_NAME);
-			BEGIN(INITIAL); yyless(0);
+			BEGIN(INITIAL);
+			REWIND(1);
 			return PE_EVENT_NAME;
 		}
 
 .		|
 <<EOF>>		{
-			BEGIN(INITIAL); yyless(0);
+			BEGIN(INITIAL);
+			REWIND(0);
 		}
 
 }
tools/perf/util/parse-events.y +4 -3
index 72def077dbbf..14521ce534d9 100644
@@ -2,6 +2,7 @@
 %parse-param {void *_data}
 %parse-param {void *scanner}
 %lex-param {void* scanner}
+%locations
 
 %{
 

@@ -14,8 +15,6 @@
 #include "parse-events.h"
 #include "parse-events-bison.h"
 
-extern int parse_events_lex (YYSTYPE* lvalp, void* scanner);
-
 #define ABORT_ON(val) \
 do { \
 	if (val) \

@@ -520,7 +519,9 @@ sep_slash_dc: '/' | ':' |
 
 %%
 
-void parse_events_error(void *data __maybe_unused, void *scanner __maybe_unused,
+void parse_events_error(YYLTYPE *loc, void *data,
+			void *scanner __maybe_unused,
 			char const *msg __maybe_unused)
 {
+	parse_events_evlist_error(data, loc->last_column, "parser error");
 }
Signature · 1 line
1.9.3
author reviewer
#3 lore raw

PATCH 3/6 perf tools: Change parse_events_add_pmu interface

In reply to Jiri Olsa

Changing parse_events_add_pmu interface to allow
propagating of the parse_events_error info.

Link http://lkml.kernel.org/n/[email protected] open here
Signed-off-by Jiri Olsa <[email protected]>
---
 tools/perf/util/parse-events.c | 11 ++++++-----
 tools/perf/util/parse-events.h |  5 +++--
 tools/perf/util/parse-events.y |  6 +++---
 3 files changed, 12 insertions(+), 10 deletions(-)
tools/perf/util/parse-events.c +6 -5
index 87c72dbc2cd5..73e6618da6d1 100644
@@ -627,8 +627,9 @@ static char *pmu_event_name(struct list_head *head_terms)
 	return NULL;
 }
 
-int parse_events_add_pmu(struct list_head *list, int *idx,
-			 char *name, struct list_head *head_config)
+int parse_events_add_pmu(struct parse_events_evlist *data,
+			 struct list_head *list, char *name,
+			 struct list_head *head_config)
 {
 	struct perf_event_attr attr;
 	struct perf_pmu_info info;

@@ -648,7 +649,7 @@ int parse_events_add_pmu(struct list_head *list, int *idx,
 
 	if (!head_config) {
 		attr.type = pmu->type;
-		evsel = __add_event(list, idx, &attr, NULL, pmu->cpus);
+		evsel = __add_event(list, &data->idx, &attr, NULL, pmu->cpus);
 		return evsel ? 0 : -ENOMEM;
 	}
 

@@ -664,8 +665,8 @@ int parse_events_add_pmu(struct list_head *list, int *idx,
 	if (perf_pmu__config(pmu, &attr, head_config))
 		return -EINVAL;
 
-	evsel = __add_event(list, idx, &attr, pmu_event_name(head_config),
-			    pmu->cpus);
+	evsel = __add_event(list, &data->idx, &attr,
+			    pmu_event_name(head_config), pmu->cpus);
 	if (evsel) {
 		evsel->unit = info.unit;
 		evsel->scale = info.scale;
tools/perf/util/parse-events.h +3 -2
index 5c0def908b5e..b3adb8abc1a8 100644
@@ -114,8 +114,9 @@ int parse_events_add_cache(struct list_head *list, int *idx,
 			   char *type, char *op_result1, char *op_result2);
 int parse_events_add_breakpoint(struct list_head *list, int *idx,
 				void *ptr, char *type, u64 len);
-int parse_events_add_pmu(struct list_head *list, int *idx,
-			 char *pmu , struct list_head *head_config);
+int parse_events_add_pmu(struct parse_events_evlist *data,
+			 struct list_head *list, char *name,
+			 struct list_head *head_config);
 enum perf_pmu_event_symbol_type
 perf_pmu__parse_check(const char *name);
 void parse_events__set_leader(char *name, struct list_head *list);
tools/perf/util/parse-events.y +3 -3
index 14521ce534d9..84596617b355 100644
@@ -207,7 +207,7 @@ PE_NAME '/' event_config '/'
 	struct list_head *list;
 
 	ALLOC_LIST(list);
-	ABORT_ON(parse_events_add_pmu(list, &data->idx, $1, $3));
+	ABORT_ON(parse_events_add_pmu(data, list, $1, $3));
 	parse_events__free_terms($3);
 	$$ = list;
 }

@@ -218,7 +218,7 @@ PE_NAME '/' '/'
 	struct list_head *list;
 
 	ALLOC_LIST(list);
-	ABORT_ON(parse_events_add_pmu(list, &data->idx, $1, NULL));
+	ABORT_ON(parse_events_add_pmu(data, list, $1, NULL));
 	$$ = list;
 }
 |

@@ -235,7 +235,7 @@ PE_KERNEL_PMU_EVENT sep_dc
 	list_add_tail(&term->list, head);
 
 	ALLOC_LIST(list);
-	ABORT_ON(parse_events_add_pmu(list, &data->idx, "cpu", head));
+	ABORT_ON(parse_events_add_pmu(data, list, "cpu", head));
 	parse_events__free_terms(head);
 	$$ = list;
 }
Signature · 1 line
1.9.3
author reviewer
#4 lore raw

PATCH 4/6 perf tools: Add location to pmu event terms

In reply to Jiri Olsa

Saving the terms location within term struct,
so it could be used later for report.

Link http://lkml.kernel.org/n/[email protected] open here
Signed-off-by Jiri Olsa <[email protected]>
---
 tools/perf/util/parse-events.c | 32 ++++++++++++++++++++++++--------
 tools/perf/util/parse-events.h |  8 ++++----
 tools/perf/util/parse-events.y | 14 +++++++-------
 3 files changed, 35 insertions(+), 19 deletions(-)
tools/perf/util/parse-events.c +24 -8
index 73e6618da6d1..3caa6b02d4e5 100644
@@ -25,6 +25,12 @@
 extern int parse_events_debug;
 #endif
 int parse_events_parse(void *data, void *scanner);
+int parse_events_term__num(struct parse_events_term **term,
+			   int type_term, char *config, u64 num,
+			   YYLTYPE *loc_term, YYLTYPE *loc_val);
+int parse_events_term__str(struct parse_events_term **term,
+			   int type_term, char *config, char *str,
+			   YYLTYPE *loc_term, YYLTYPE *loc_val);
 
 static struct perf_pmu_event_symbol *perf_pmu_events_list;
 /*

@@ -1525,7 +1531,7 @@ int parse_events__is_hardcoded_term(struct parse_events_term *term)
 
 static int new_term(struct parse_events_term **_term, int type_val,
 		    int type_term, char *config,
-		    char *str, u64 num)
+		    char *str, u64 num, int err_term, int err_val)
 {
 	struct parse_events_term *term;
 

@@ -1537,6 +1543,8 @@ static int new_term(struct parse_events_term **_term, int type_val,
 	term->type_val  = type_val;
 	term->type_term = type_term;
 	term->config = config;
+	term->err_term = err_term;
+	term->err_val  = err_val;
 
 	switch (type_val) {
 	case PARSE_EVENTS__TERM_TYPE_NUM:

@@ -1555,17 +1563,23 @@ static int new_term(struct parse_events_term **_term, int type_val,
 }
 
 int parse_events_term__num(struct parse_events_term **term,
-			   int type_term, char *config, u64 num)
+			   int type_term, char *config, u64 num,
+			   YYLTYPE *loc_term, YYLTYPE *loc_val)
 {
 	return new_term(term, PARSE_EVENTS__TERM_TYPE_NUM, type_term,
-			config, NULL, num);
+			config, NULL, num,
+			loc_term ? loc_term->first_column : 0,
+			loc_val ? loc_val->first_column : 0);
 }
 
 int parse_events_term__str(struct parse_events_term **term,
-			   int type_term, char *config, char *str)
+			   int type_term, char *config, char *str,
+			   YYLTYPE *loc_term, YYLTYPE *loc_val)
 {
 	return new_term(term, PARSE_EVENTS__TERM_TYPE_STR, type_term,
-			config, str, 0);
+			config, str, 0,
+			loc_term ? loc_term->first_column : 0,
+			loc_val ? loc_val->first_column : 0);
 }
 
 int parse_events_term__sym_hw(struct parse_events_term **term,

@@ -1579,18 +1593,20 @@ int parse_events_term__sym_hw(struct parse_events_term **term,
 	if (config)
 		return new_term(term, PARSE_EVENTS__TERM_TYPE_STR,
 				PARSE_EVENTS__TERM_TYPE_USER, config,
-				(char *) sym->symbol, 0);
+				(char *) sym->symbol, 0, 0, 0);
 	else
 		return new_term(term, PARSE_EVENTS__TERM_TYPE_STR,
 				PARSE_EVENTS__TERM_TYPE_USER,
-				(char *) "event", (char *) sym->symbol, 0);
+				(char *) "event", (char *) sym->symbol,
+				0, 0, 0);
 }
 
 int parse_events_term__clone(struct parse_events_term **new,
 			     struct parse_events_term *term)
 {
 	return new_term(new, term->type_val, term->type_term, term->config,
-			term->val.str, term->val.num);
+			term->val.str, term->val.num,
+			term->err_term, term->err_val);
 }
 
 void parse_events__free_terms(struct list_head *terms)
tools/perf/util/parse-events.h +4 -4
index b3adb8abc1a8..62fafbbf48c1 100644
@@ -74,6 +74,10 @@ struct parse_events_term {
 	int type_term;
 	struct list_head list;
 	bool used;
+
+	/* error string indexes for within parsed string */
+	int err_term;
+	int err_val;
 };
 
 struct parse_events_error {

@@ -93,10 +97,6 @@ struct parse_events_terms {
 };
 
 int parse_events__is_hardcoded_term(struct parse_events_term *term);
-int parse_events_term__num(struct parse_events_term **_term,
-			   int type_term, char *config, u64 num);
-int parse_events_term__str(struct parse_events_term **_term,
-			   int type_term, char *config, char *str);
 int parse_events_term__sym_hw(struct parse_events_term **term,
 			      char *config, unsigned idx);
 int parse_events_term__clone(struct parse_events_term **new,
tools/perf/util/parse-events.y +7 -7
index 84596617b355..2dea32458536 100644
@@ -231,7 +231,7 @@ PE_KERNEL_PMU_EVENT sep_dc
 
 	ALLOC_LIST(head);
 	ABORT_ON(parse_events_term__num(&term, PARSE_EVENTS__TERM_TYPE_USER,
-					$1, 1));
+					$1, 1, &@1, NULL));
 	list_add_tail(&term->list, head);
 
 	ALLOC_LIST(list);

@@ -251,7 +251,7 @@ PE_PMU_EVENT_PRE '-' PE_PMU_EVENT_SUF sep_dc
 
 	ALLOC_LIST(head);
 	ABORT_ON(parse_events_term__num(&term, PARSE_EVENTS__TERM_TYPE_USER,
-					&pmu_name, 1));
+					&pmu_name, 1,  &@1, NULL));
 	list_add_tail(&term->list, head);
 
 	ALLOC_LIST(list);

@@ -449,7 +449,7 @@ PE_NAME '=' PE_NAME
 	struct parse_events_term *term;
 
 	ABORT_ON(parse_events_term__str(&term, PARSE_EVENTS__TERM_TYPE_USER,
-					$1, $3));
+					$1, $3, &@1, &@3));
 	$$ = term;
 }
 |

@@ -458,7 +458,7 @@ PE_NAME '=' PE_VALUE
 	struct parse_events_term *term;
 
 	ABORT_ON(parse_events_term__num(&term, PARSE_EVENTS__TERM_TYPE_USER,
-					$1, $3));
+					$1, $3, &@1, &@3));
 	$$ = term;
 }
 |

@@ -476,7 +476,7 @@ PE_NAME
 	struct parse_events_term *term;
 
 	ABORT_ON(parse_events_term__num(&term, PARSE_EVENTS__TERM_TYPE_USER,
-					$1, 1));
+					$1, 1, &@1, NULL));
 	$$ = term;
 }
 |

@@ -501,7 +501,7 @@ PE_TERM '=' PE_VALUE
 {
 	struct parse_events_term *term;
 
-	ABORT_ON(parse_events_term__num(&term, (int)$1, NULL, $3));
+	ABORT_ON(parse_events_term__num(&term, (int)$1, NULL, $3, &@1, &@3));
 	$$ = term;
 }
 |

@@ -509,7 +509,7 @@ PE_TERM
 {
 	struct parse_events_term *term;
 
-	ABORT_ON(parse_events_term__num(&term, (int)$1, NULL, 1));
+	ABORT_ON(parse_events_term__num(&term, (int)$1, NULL, 1, &@1, NULL));
 	$$ = term;
 }
Signature · 1 line
1.9.3
author reviewer
#5 lore raw

PATCH 5/6 perf tools: Add term support for parse_events_error

In reply to Jiri Olsa

Allowing event's term processing to report back error, like:

  $ ./perf record -e 'cpu/even=0x1/' ls
  invalid or unsupported event: 'cpu/even=0x1/'
                                     \___ unknown term
Link http://lkml.kernel.org/n/[email protected] open here
Signed-off-by Jiri Olsa <[email protected]>
---
 tools/perf/tests/pmu.c         |  2 +-
 tools/perf/util/parse-events.c |  2 +-
 tools/perf/util/pmu.c          | 23 +++++++++++++++++------
 tools/perf/util/pmu.h          |  6 ++++--
 4 files changed, 23 insertions(+), 10 deletions(-)
tools/perf/tests/pmu.c +1 -1
index eeb68bb1972d..f2655947e37f 100644
@@ -152,7 +152,7 @@ int test__pmu(void)
 		if (ret)
 			break;
 
-		ret = perf_pmu__config_terms(&formats, &attr, terms, false);
+		ret = perf_pmu__config_terms(&formats, &attr, terms, false, NULL);
 		if (ret)
 			break;
tools/perf/util/parse-events.c +1 -1
index 3caa6b02d4e5..3c5e658386a1 100644
@@ -668,7 +668,7 @@ int parse_events_add_pmu(struct parse_events_evlist *data,
 	 */
 	config_attr(&attr, head_config, 0);
 
-	if (perf_pmu__config(pmu, &attr, head_config))
+	if (perf_pmu__config(pmu, &attr, head_config, data->error))
 		return -EINVAL;
 
 	evsel = __add_event(list, &data->idx, &attr,
tools/perf/util/pmu.c +17 -6
index 48411674da0f..11cdfd37c4e0 100644
@@ -587,7 +587,7 @@ static int pmu_config_term(struct list_head *formats,
 			   struct perf_event_attr *attr,
 			   struct parse_events_term *term,
 			   struct list_head *head_terms,
-			   bool zero)
+			   bool zero, struct parse_events_error *error)
 {
 	struct perf_pmu_format *format;
 	__u64 *vp;

@@ -611,6 +611,10 @@ static int pmu_config_term(struct list_head *formats,
 	if (!format) {
 		if (verbose)
 			printf("Invalid event/parameter '%s'\n", term->config);
+		if (error) {
+			error->idx = term->err_term;
+			error->str = strdup("unknown term");
+		}
 		return -EINVAL;
 	}
 

@@ -636,9 +640,14 @@ static int pmu_config_term(struct list_head *formats,
 		val = term->val.num;
 	else if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR) {
 		if (strcmp(term->val.str, "?")) {
-			if (verbose)
+			if (verbose) {
 				pr_info("Invalid sysfs entry %s=%s\n",
 						term->config, term->val.str);
+			}
+			if (error) {
+				error->idx = term->err_val;
+				error->str = strdup("expected numeric value");
+			}
 			return -EINVAL;
 		}
 

@@ -654,12 +663,13 @@ static int pmu_config_term(struct list_head *formats,
 int perf_pmu__config_terms(struct list_head *formats,
 			   struct perf_event_attr *attr,
 			   struct list_head *head_terms,
-			   bool zero)
+			   bool zero, struct parse_events_error *error)
 {
 	struct parse_events_term *term;
 
 	list_for_each_entry(term, head_terms, list) {
-		if (pmu_config_term(formats, attr, term, head_terms, zero))
+		if (pmu_config_term(formats, attr, term, head_terms,
+				    zero, error))
 			return -EINVAL;
 	}
 

@@ -672,12 +682,13 @@ int perf_pmu__config_terms(struct list_head *formats,
  * 2) pmu format definitions - specified by pmu parameter
  */
 int perf_pmu__config(struct perf_pmu *pmu, struct perf_event_attr *attr,
-		     struct list_head *head_terms)
+		     struct list_head *head_terms,
+		     struct parse_events_error *error)
 {
 	bool zero = !!pmu->default_config;
 
 	attr->type = pmu->type;
-	return perf_pmu__config_terms(&pmu->format, attr, head_terms, zero);
+	return perf_pmu__config_terms(&pmu->format, attr, head_terms, zero, error);
 }
 
 static struct perf_pmu_alias *pmu_find_alias(struct perf_pmu *pmu,
tools/perf/util/pmu.h +4 -2
index 6b1249fbdb5f..7b9c8cf8ae3e 100644
@@ -4,6 +4,7 @@
 #include <linux/bitmap.h>
 #include <linux/perf_event.h>
 #include <stdbool.h>
+#include "parse-events.h"
 
 enum {
 	PERF_PMU_FORMAT_VALUE_CONFIG,

@@ -47,11 +48,12 @@ struct perf_pmu_alias {
 
 struct perf_pmu *perf_pmu__find(const char *name);
 int perf_pmu__config(struct perf_pmu *pmu, struct perf_event_attr *attr,
-		     struct list_head *head_terms);
+		     struct list_head *head_terms,
+		     struct parse_events_error *error);
 int perf_pmu__config_terms(struct list_head *formats,
 			   struct perf_event_attr *attr,
 			   struct list_head *head_terms,
-			   bool zero);
+			   bool zero, struct parse_events_error *error);
 int perf_pmu__check_alias(struct perf_pmu *pmu, struct list_head *head_terms,
 			  struct perf_pmu_info *info);
 struct list_head *perf_pmu__alias(struct perf_pmu *pmu,
Signature · 1 line
1.9.3
author reviewer
#6 lore raw

PATCH 6/6 perf tools: Add tracepoint support for parse_events_error

In reply to Jiri Olsa

Allowing tracepoint events processing to report back error.

  $ perf record -e 'sched:krava' ls
  invalid or unsupported event: 'sched:krava'
                                 \___ unknown tracepoint
Link http://lkml.kernel.org/n/[email protected] open here
Signed-off-by Jiri Olsa <[email protected]>
---
 tools/perf/util/parse-events.y | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
tools/perf/util/parse-events.y +7 -1
index 2dea32458536..d3f963d99e99 100644
@@ -388,7 +388,13 @@ PE_NAME ':' PE_NAME
 	struct list_head *list;
 
 	ALLOC_LIST(list);
-	ABORT_ON(parse_events_add_tracepoint(list, &data->idx, $1, $3));
+	if (parse_events_add_tracepoint(list, &data->idx, $1, $3)) {
+		struct parse_events_error *error = data->error;
+
+		error->idx = @1.first_column;
+		error->str = strdup("unknown tracepoint");
+		return -1;
+	}
 	$$ = list;
 }
Signature · 1 line
1.9.3
#7 lore raw
In reply to Jiri Olsa
Quoted 2 lines · Jiri Olsa

hi,
adding support to report error from event string parsing.

Very nice!

Quoted 11 lines

This patchset contains support for standard parsing errors and more logic to recognize tracepoint and 'pmu//' terms, like:

  $ perf record -e 'sched:krava' ls
  invalid or unsupported event: 'sched:krava'
                                 \___ unknown tracepoint
  $ ./perf record -e 'cpu/even=0x1/' ls
  invalid or unsupported event: 'cpu/even=0x1/'
                                     \___ unknown term
Quoted 5 lines
  $ perf record -e cycles,cache-mises ls
  invalid or unsupported event: '..es,cache-mises'
                                           \___ parser error

any feedback about the error string shape would be great ;-)

So since we now know exactly what's going on, we might want to drop the 'invalid or unsupported event' language as well, and make it specific:

   $ ./perf record -e 'cpu/even=0x1/' ls
   event syntax error: 'cpu/even=0x1/'
                            \___ unknown term

?

Also, for the above error, could we easily list the valid terms? An error like:

   $ ./perf record -e 'cpu/even=0x1/' ls
   event syntax error: 'cpu/even=0x1/'
                            \___ unknown term
   valid terms: "event", "raw".

or so?

Thanks,

Ingo

#8 lore raw
Quoted 29 lines · Ingo Molnar
Quoted 2 lines · Jiri Olsa

hi,
adding support to report error from event string parsing.

Very nice!

Quoted 11 lines

This patchset contains support for standard parsing errors and more logic to recognize tracepoint and 'pmu//' terms, like:

  $ perf record -e 'sched:krava' ls
  invalid or unsupported event: 'sched:krava'
                                 \___ unknown tracepoint
  $ ./perf record -e 'cpu/even=0x1/' ls
  invalid or unsupported event: 'cpu/even=0x1/'
                                     \___ unknown term
Quoted 5 lines
  $ perf record -e cycles,cache-mises ls
  invalid or unsupported event: '..es,cache-mises'
                                           \___ parser error

any feedback about the error string shape would be great ;-)

So since we now know exactly what's going on, we might want to drop the 'invalid or unsupported event' language as well, and make it specific:

Quoted 6 lines
   $ ./perf record -e 'cpu/even=0x1/' ls
   event syntax error: 'cpu/even=0x1/'
                            \___ unknown term

?

ok

Quoted 10 lines

Also, for the above error, could we easily list the valid terms? An error like:

   $ ./perf record -e 'cpu/even=0x1/' ls
   event syntax error: 'cpu/even=0x1/'
                            \___ unknown term
   valid terms: "event", "raw".

or so?

we already carry list of all terms for given pmu,
so it shouldn't be a problem

jirka

maintainer
#9 lore raw

Em Sat, Apr 18, 2015 at 10:42:58PM +0200, Jiri Olsa escreveu:

Quoted 23 lines · Ingo Molnar
Quoted 10 lines

So since we now know exactly what's going on, we might want to drop the 'invalid or unsupported event' language as well, and make it specific:

   $ ./perf record -e 'cpu/even=0x1/' ls
   event syntax error: 'cpu/even=0x1/'
                            \___ unknown term

?

ok

Quoted 9 lines

Also, for the above error, could we easily list the valid terms? An error like:

   $ ./perf record -e 'cpu/even=0x1/' ls
   event syntax error: 'cpu/even=0x1/'
                            \___ unknown term
   valid terms: "event", "raw".
Quoted 4 lines
Quoted 1 line

or so?

we already carry list of all terms for given pmu,
so it shouldn't be a problem

So, do you want me to apply this series and you work on top of it or should I wait for another that implements what Ingo suggested?

Also, we have:

[root@zoo ~]# perf stat --all -e cycles usleep 1

Performance counter stats for 'system wide':

         3,382,595      cycles                                                      

       0.001739102 seconds time elapsed

[root@zoo ~]# perf stat --all-cpus -e cycles usleep 1

Performance counter stats for 'system wide':

         2,435,672      cycles                                                      

       0.001191689 seconds time elapsed

[root@zoo ~]# perf stat --a -e cycles usleep 1
Error: Ambiguous option: a (could be --no-no-aggr or --append)

usage: perf stat [<options>] [<command>]

[root@zoo ~]#

Which is nice, wouldn't it be good to have it for the that as well, i.e. for your example "ev=" would be a valid, and shorter form of "even=" or "event=".

- Arnaldo

#10 lore raw
Quoted 24 lines · Arnaldo Carvalho de Melo

Em Sat, Apr 18, 2015 at 10:42:58PM +0200, Jiri Olsa escreveu:

Quoted 23 lines · Ingo Molnar
Quoted 10 lines

So since we now know exactly what's going on, we might want to drop the 'invalid or unsupported event' language as well, and make it specific:

   $ ./perf record -e 'cpu/even=0x1/' ls
   event syntax error: 'cpu/even=0x1/'
                            \___ unknown term

?

ok

Quoted 9 lines

Also, for the above error, could we easily list the valid terms? An error like:

   $ ./perf record -e 'cpu/even=0x1/' ls
   event syntax error: 'cpu/even=0x1/'
                            \___ unknown term
   valid terms: "event", "raw".
Quoted 7 lines
Quoted 4 lines
Quoted 1 line

or so?

we already carry list of all terms for given pmu,
so it shouldn't be a problem

So, do you want me to apply this series and you work on top of it or should I wait for another that implements what Ingo suggested?

I'll resend v2 with the update

Quoted 22 lines

Also, we have:

[root@zoo ~]# perf stat --all -e cycles usleep 1

Performance counter stats for 'system wide':

         3,382,595      cycles                                                      

       0.001739102 seconds time elapsed

[root@zoo ~]# perf stat --all-cpus -e cycles usleep 1

Performance counter stats for 'system wide':

         2,435,672      cycles                                                      

       0.001191689 seconds time elapsed

[root@zoo ~]# perf stat --a -e cycles usleep 1
Error: Ambiguous option: a (could be --no-no-aggr or --append)

Quoted 7 lines

usage: perf stat [<options>] [<command>]

[root@zoo ~]#

Which is nice, wouldn't it be good to have it for the that as well, i.e. for your example "ev=" would be a valid, and shorter form of "even=" or "event=".

hum, not sure I follow.. could you please explain in more details?

jirka

maintainer
#11 lore raw

Em Mon, Apr 20, 2015 at 10:15:39PM +0200, Jiri Olsa escreveu:

Quoted 31 lines · Arnaldo Carvalho de Melo
Quoted 3 lines

Em Sat, Apr 18, 2015 at 10:42:58PM +0200, Jiri Olsa escreveu:
So, do you want me to apply this series and you work on top of it or should I wait for another that implements what Ingo suggested?

I'll resend v2 with the update

Quoted 24 lines

Also, we have:

[root@zoo ~]# perf stat --all -e cycles usleep 1

Performance counter stats for 'system wide':

         3,382,595      cycles                                                      

       0.001739102 seconds time elapsed

[root@zoo ~]# perf stat --all-cpus -e cycles usleep 1

Performance counter stats for 'system wide':

         2,435,672      cycles                                                      

       0.001191689 seconds time elapsed

[root@zoo ~]# perf stat --a -e cycles usleep 1
Error: Ambiguous option: a (could be --no-no-aggr or --append)

usage: perf stat [<options>] [<command>]

Quoted 7 lines
Quoted 5 lines

[root@zoo ~]#

Which is nice, wouldn't it be good to have it for the that as well, i.e. for your example "ev=" would be a valid, and shorter form of "even=" or "event=".

hum, not sure I follow.. could you please explain in more details?

This would be considered valid:

    $ ./perf record -e 'cpu/even=0x1/' ls
    event syntax error: 'cpu/even=0x1/'
                             \___ unknown term
    valid terms: "event", "raw".

"even=" would be recognized as a shorter form of "event=", "evint=" would be invalid :-)

- Arnaldo

#12 lore raw
Quoted 42 lines · Arnaldo Carvalho de Melo

Em Mon, Apr 20, 2015 at 10:15:39PM +0200, Jiri Olsa escreveu:

Quoted 31 lines · Arnaldo Carvalho de Melo
Quoted 3 lines

Em Sat, Apr 18, 2015 at 10:42:58PM +0200, Jiri Olsa escreveu:
So, do you want me to apply this series and you work on top of it or should I wait for another that implements what Ingo suggested?

I'll resend v2 with the update

Quoted 24 lines

Also, we have:

[root@zoo ~]# perf stat --all -e cycles usleep 1

Performance counter stats for 'system wide':

         3,382,595      cycles                                                      

       0.001739102 seconds time elapsed

[root@zoo ~]# perf stat --all-cpus -e cycles usleep 1

Performance counter stats for 'system wide':

         2,435,672      cycles                                                      

       0.001191689 seconds time elapsed

[root@zoo ~]# perf stat --a -e cycles usleep 1
Error: Ambiguous option: a (could be --no-no-aggr or --append)

usage: perf stat [<options>] [<command>]

Quoted 7 lines
Quoted 5 lines

[root@zoo ~]#

Which is nice, wouldn't it be good to have it for the that as well, i.e. for your example "ev=" would be a valid, and shorter form of "even=" or "event=".

hum, not sure I follow.. could you please explain in more details?

This would be considered valid:

Quoted 7 lines
    $ ./perf record -e 'cpu/even=0x1/' ls
    event syntax error: 'cpu/even=0x1/'
                             \___ unknown term
    valid terms: "event", "raw".

"even=" would be recognized as a shorter form of "event=", "evint=" would be invalid :-)

hum I see.. seem to be touching the bison/flex core part, might not be that easy I'll think about it.. ;-)

thanks,
jirka

maintainer
#13 lore raw

Em Wed, Apr 22, 2015 at 04:08:50PM +0200, Jiri Olsa escreveu:

Quoted 16 lines · Arnaldo Carvalho de Melo
Quoted 14 lines

Em Mon, Apr 20, 2015 at 10:15:39PM +0200, Jiri Olsa escreveu:

Quoted 6 lines · Arnaldo Carvalho de Melo
Quoted 3 lines

Which is nice, wouldn't it be good to have it for the that as well, i.e. for your example "ev=" would be a valid, and shorter form of "even=" or "event=".

hum, not sure I follow.. could you please explain in more details?

This would be considered valid:

    $ ./perf record -e 'cpu/even=0x1/' ls
    event syntax error: 'cpu/even=0x1/'
                             \___ unknown term
    valid terms: "event", "raw".
Quoted 5 lines
Quoted 2 lines

"even=" would be recognized as a shorter form of "event=", "evint=" would be invalid :-)

hum I see.. seem to be touching the bison/flex core part, might not be that easy I'll think about it.. ;-)

This is just a "nice to have" feature, that would make it consistent accross the tools, i.e. it works for long option processing, it would work for event parsing, etc, but I guess you can leave it for when there is a blizzard and you get stuck at home or something like that ;-)

- Arnaldo