[RFC 0/6] perf tools: Report event parsing errors

[RFC 0/6] perf tools: Report event parsing errors

[Jiri Olsa]

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(-)

[PATCH 1/6] perf tools: Add parse_events_error interface

[Jiri Olsa]

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/tip-rqil5ug9qe4b6odr90g19umt@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 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(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index f7b8218785f6..3dbd8c59efc5 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -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;
diff --git a/tools/perf/tests/code-reading.c b/tools/perf/tests/code-reading.c
index f671ec37a7c4..ca0e480e741b 100644
--- a/tools/perf/tests/code-reading.c
+++ b/tools/perf/tests/code-reading.c
@@ -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;
diff --git a/tools/perf/tests/evsel-roundtrip-name.c b/tools/perf/tests/evsel-roundtrip-name.c
index b8d8341b383e..3fa715987a5e 100644
--- a/tools/perf/tests/evsel-roundtrip-name.c
+++ b/tools/perf/tests/evsel-roundtrip-name.c
@@ -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);
diff --git a/tools/perf/tests/hists_cumulate.c b/tools/perf/tests/hists_cumulate.c
index 18619966454c..b08a95a5ca1a 100644
--- a/tools/perf/tests/hists_cumulate.c
+++ b/tools/perf/tests/hists_cumulate.c
@@ -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;
 
diff --git a/tools/perf/tests/hists_filter.c b/tools/perf/tests/hists_filter.c
index 59e53db7914c..108488cd71fa 100644
--- a/tools/perf/tests/hists_filter.c
+++ b/tools/perf/tests/hists_filter.c
@@ -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;
 
diff --git a/tools/perf/tests/hists_link.c b/tools/perf/tests/hists_link.c
index 278ba8344c23..34c61e4d3352 100644
--- a/tools/perf/tests/hists_link.c
+++ b/tools/perf/tests/hists_link.c
@@ -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;
 
diff --git a/tools/perf/tests/hists_output.c b/tools/perf/tests/hists_output.c
index b52c9faea224..d8a23db80094 100644
--- a/tools/perf/tests/hists_output.c
+++ b/tools/perf/tests/hists_output.c
@@ -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;
 
diff --git a/tools/perf/tests/keep-tracking.c b/tools/perf/tests/keep-tracking.c
index 7a5ab7b0b8f6..5b171d1e338b 100644
--- a/tools/perf/tests/keep-tracking.c
+++ b/tools/perf/tests/keep-tracking.c
@@ -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);
 
diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
index 3de744961739..82d2a1636f7f 100644
--- a/tools/perf/tests/parse-events.c
+++ b/tools/perf/tests/parse-events.c
@@ -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);
diff --git a/tools/perf/tests/perf-time-to-tsc.c b/tools/perf/tests/perf-time-to-tsc.c
index f238442b238a..5f49484f1abc 100644
--- a/tools/perf/tests/perf-time-to-tsc.c
+++ b/tools/perf/tests/perf-time-to-tsc.c
@@ -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);
 
diff --git a/tools/perf/tests/switch-tracking.c b/tools/perf/tests/switch-tracking.c
index cc68648c7c55..0d31403ea593 100644
--- a/tools/perf/tests/switch-tracking.c
+++ b/tools/perf/tests/switch-tracking.c
@@ -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;
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index be0655388b38..87c72dbc2cd5 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -17,6 +17,7 @@
 #include "parse-events-flex.h"
 #include "pmu.h"
 #include "thread_map.h"
+#include "asm/bug.h"
 
 #define MAX_NAME_LEN 100
 
@@ -1019,11 +1020,13 @@ int parse_events_terms(struct list_head *terms, const char *str)
 	return ret;
 }
 
-int parse_events(struct perf_evlist *evlist, const char *str)
+int parse_events(struct perf_evlist *evlist, const char *str,
+		 struct parse_events_error *error)
 {
 	struct parse_events_evlist data = {
-		.list = LIST_HEAD_INIT(data.list),
-		.idx  = evlist->nr_entries,
+		.list  = LIST_HEAD_INIT(data.list),
+		.idx   = evlist->nr_entries,
+		.error = error,
 	};
 	int ret;
 
@@ -1044,16 +1047,77 @@ int parse_events(struct perf_evlist *evlist, const char *str)
 	return ret;
 }
 
+#define MAX_WIDTH 1000
+static int get_term_width(void)
+{
+	struct winsize ws;
+
+	get_term_dimensions(&ws);
+	return ws.ws_col > MAX_WIDTH ? MAX_WIDTH : ws.ws_col;
+}
+
+static void parse_events_print_error(struct parse_events_error *error,
+				     const char *event)
+{
+	const char *str = "invalid or unsupported event: ";
+	char _buf[MAX_WIDTH];
+	char *buf = (char *) event;
+	int idx = 0;
+
+	if (error->str) {
+		/* -2 for extra '' in the final fprintf */
+		int width       = get_term_width() - 2;
+		int len_event   = strlen(event);
+		int len_str     = strlen(str);
+		int max_len     = width - len_str;
+		int cut         = 0;
+		/*
+		 * Maximum error index indent, we will cut
+		 * the event string if it's bigger.
+		 */
+		int max_err_idx = 10;
+
+		buf = _buf;
+
+		/* we're cutting from the beggining */
+		if (error->idx > max_err_idx)
+			cut = error->idx - max_err_idx;
+
+		strncpy(buf, event + cut, max_len);
+
+		/* mark cut parts with '..' on both sides */
+		if (cut)
+			buf[0] = buf[1] = '.';
+
+		if ((len_event - cut) > max_len) {
+			buf[max_len - 1] = buf[max_len - 2] = '.';
+			buf[max_len] = 0;
+		}
+
+		idx = len_str + error->idx - cut;
+	}
+
+	fprintf(stderr, "%s'%s'\n", str, buf);
+	if (idx) {
+		fprintf(stderr, "%*s\\___ %s\n", idx + 1, "", error->str);
+		free(error->str);
+	}
+
+	fprintf(stderr, "Run 'perf list' for a list of valid events\n");
+}
+
+#undef MAX_WIDTH
+
 int parse_events_option(const struct option *opt, const char *str,
 			int unset __maybe_unused)
 {
 	struct perf_evlist *evlist = *(struct perf_evlist **)opt->value;
-	int ret = parse_events(evlist, str);
+	struct parse_events_error error = { .idx = 0, };
+	int ret = parse_events(evlist, str, &error);
+
+	if (ret)
+		parse_events_print_error(&error, str);
 
-	if (ret) {
-		fprintf(stderr, "invalid or unsupported event: '%s'\n", str);
-		fprintf(stderr, "Run 'perf list' for a list of valid events\n");
-	}
 	return ret;
 }
 
@@ -1535,3 +1599,13 @@ void parse_events__free_terms(struct list_head *terms)
 	list_for_each_entry_safe(term, h, terms, list)
 		free(term);
 }
+
+void parse_events_evlist_error(struct parse_events_evlist *data,
+			       int idx, const char *str)
+{
+	struct parse_events_error *error = data->error;
+
+	error->idx = idx;
+	error->str = strdup(str);
+	WARN_ONCE(!error->str, "WARNING: failed to allocate error string");
+}
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index 52a2dda4f954..90bdd877c62e 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -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);
 
diff --git a/tools/perf/util/record.c b/tools/perf/util/record.c
index 8acd0df88b5c..c7259a5ad185 100644
--- a/tools/perf/util/record.c
+++ b/tools/perf/util/record.c
@@ -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;
 
-- 
1.9.3

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

[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/tip-j2ek52is8nt846pmyiib05ux@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 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(-)

diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index 90bdd877c62e..5c0def908b5e 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -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);
 
diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index 8895cf3132ab..330dd2d35f5a 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -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);
 		}
 
 }
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 72def077dbbf..14521ce534d9 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -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");
 }
-- 
1.9.3

[PATCH 3/6] perf tools: Change parse_events_add_pmu interface

[Jiri Olsa]

Changing parse_events_add_pmu interface to allow
propagating of the parse_events_error info.

Link: http://lkml.kernel.org/n/tip-lhjgvd0h5e9i890nprnrnzew@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 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(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 87c72dbc2cd5..73e6618da6d1 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -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;
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index 5c0def908b5e..b3adb8abc1a8 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -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);
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 14521ce534d9..84596617b355 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -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;
 }
-- 
1.9.3

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

[Jiri Olsa]

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

Link: http://lkml.kernel.org/n/tip-0usk1luyaeeguh8qmxwmmedr@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 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(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 73e6618da6d1..3caa6b02d4e5 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -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)
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index b3adb8abc1a8..62fafbbf48c1 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -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,
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 84596617b355..2dea32458536 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -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;
 }
 
-- 
1.9.3

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

[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/tip-xkbpn3j5of376ag7utcirt8w@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 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(-)

diff --git a/tools/perf/tests/pmu.c b/tools/perf/tests/pmu.c
index eeb68bb1972d..f2655947e37f 100644
--- a/tools/perf/tests/pmu.c
+++ b/tools/perf/tests/pmu.c
@@ -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;
 
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 3caa6b02d4e5..3c5e658386a1 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -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,
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 48411674da0f..11cdfd37c4e0 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -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,
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index 6b1249fbdb5f..7b9c8cf8ae3e 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -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,
-- 
1.9.3

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

[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/tip-xkbpn3j5of376ag7utcirt8w@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/parse-events.y | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 2dea32458536..d3f963d99e99 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -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;
 }
 
-- 
1.9.3

Re: [RFC 0/6] perf tools: Report event parsing errors

[Ingo Molnar]

* Jiri Olsa <jolsa@kernel.org> wrote:

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

Very nice!

> 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 ;-)

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

Re: [RFC 0/6] perf tools: Report event parsing errors

[Jiri Olsa]

On Sat, Apr 18, 2015 at 07:39:27PM +0200, Ingo Molnar wrote:
> 
> * Jiri Olsa <jolsa@kernel.org> wrote:
> 
> > hi,
> > adding support to report error from event string parsing.
> 
> Very nice!
> 
> > 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 ;-)
> 
> 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

> 
> 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

Re: [RFC 0/6] perf tools: Report event parsing errors

[Arnaldo Carvalho de Melo]

Em Sat, Apr 18, 2015 at 10:42:58PM +0200, Jiri Olsa escreveu:
> On Sat, Apr 18, 2015 at 07:39:27PM +0200, Ingo Molnar wrote:
> > 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
> 
> > 
> > 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

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

Re: [RFC 0/6] perf tools: Report event parsing errors

[Jiri Olsa]

On Mon, Apr 20, 2015 at 05:09:36PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Sat, Apr 18, 2015 at 10:42:58PM +0200, Jiri Olsa escreveu:
> > On Sat, Apr 18, 2015 at 07:39:27PM +0200, Ingo Molnar wrote:
> > > 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
> > 
> > > 
> > > 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
> 
> 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

> 
> 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=".

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

jirka

Re: [RFC 0/6] perf tools: Report event parsing errors

[Arnaldo Carvalho de Melo]

Em Mon, Apr 20, 2015 at 10:15:39PM +0200, Jiri Olsa escreveu:
> On Mon, Apr 20, 2015 at 05:09:36PM -0300, Arnaldo Carvalho de Melo wrote:
> > 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
> 
> > 
> > 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=".
> 
> 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

Re: [RFC 0/6] perf tools: Report event parsing errors

[Jiri Olsa]

On Mon, Apr 20, 2015 at 05:38:52PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Apr 20, 2015 at 10:15:39PM +0200, Jiri Olsa escreveu:
> > On Mon, Apr 20, 2015 at 05:09:36PM -0300, Arnaldo Carvalho de Melo wrote:
> > > 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
> > 
> > > 
> > > 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=".
> > 
> > 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 :-)

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

thanks,
jirka

Re: [RFC 0/6] perf tools: Report event parsing errors

[Arnaldo Carvalho de Melo]

Em Wed, Apr 22, 2015 at 04:08:50PM +0200, Jiri Olsa escreveu:
> On Mon, Apr 20, 2015 at 05:38:52PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Mon, Apr 20, 2015 at 10:15:39PM +0200, Jiri Olsa escreveu:
> > > On Mon, Apr 20, 2015 at 05:09:36PM -0300, Arnaldo Carvalho de Melo wrote:
> > > > 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 :-)

> 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