[PATCH v2 00/16] Consistently prefer sysfs/json events

[PATCH v2 00/16] Consistently prefer sysfs/json events

[Ian Rogers]

As discussed in:
https://lore.kernel.org/lkml/20240217005738.3744121-1-atishp@rivosinc.com/
preferring sysfs/json events consistently (with or without a given
PMU) will enable RISC-V's hope to customize legacy events in the perf
tool.

Some minor clean-up is performed on the way.

v2. Additional cleanup particularly adding better error messages. Fix
    some line length issues on the earlier patches.

Ian Rogers (16):
  perf parse-events: Factor out '<event_or_pmu>/.../' parsing
  perf parse-events: Directly pass PMU to parse_events_add_pmu
  perf parse-events: Avoid copying an empty list
  perf pmu: Refactor perf_pmu__match
  perf tests parse-events: Use branches rather than cache-references
  perf parse-events: Legacy cache names on all PMUs and lower priority
  perf parse-events: Handle PE_TERM_HW in name_or_raw
  perf parse-events: Constify parse_events_add_numeric
  perf parse-events: Prefer sysfs/json hardware events over legacy
  perf parse-events: Inline parse_events_update_lists
  perf parse-events: Improve error message for bad numbers
  perf parse-events: Inline parse_events_evlist_error
  perf parse-events: Improvements to modifier parsing
  perf parse-event: Constify event_symbol arrays
  perf parse-events: Minor grouping tidy up
  perf parse-events: Tidy the setting of the default event name

 tools/perf/tests/parse-events.c |   6 +-
 tools/perf/util/parse-events.c  | 482 ++++++++++++++++----------------
 tools/perf/util/parse-events.h  |  49 ++--
 tools/perf/util/parse-events.l  | 196 +++++++++----
 tools/perf/util/parse-events.y  | 261 +++++++----------
 tools/perf/util/pmu.c           |  27 +-
 tools/perf/util/pmu.h           |   2 +-
 7 files changed, 540 insertions(+), 483 deletions(-)

-- 
2.44.0.683.g7961c838ac-goog

[PATCH v2 01/16] perf parse-events: Factor out '<event_or_pmu>/.../' parsing

[Ian Rogers]

Factor out the case of an event or PMU name followed by a slash based
term list. This is with a view to sharing the code with new legacy
hardware parsing. Use early return to reduce indentation in the code.
Make parse_events_add_pmu static now it doesn't need sharing with
parse-events.y.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/parse-events.c | 70 +++++++++++++++++++++++++++++++-
 tools/perf/util/parse-events.h | 10 +++--
 tools/perf/util/parse-events.y | 73 +++-------------------------------
 3 files changed, 80 insertions(+), 73 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 6f8b0fa17689..a6f71165ee1a 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1385,7 +1385,7 @@ static bool config_term_percore(struct list_head *config_terms)
 	return false;
 }
 
-int parse_events_add_pmu(struct parse_events_state *parse_state,
+static int parse_events_add_pmu(struct parse_events_state *parse_state,
 			 struct list_head *list, const char *name,
 			 const struct parse_events_terms *const_parsed_terms,
 			 bool auto_merge_stats, void *loc_)
@@ -1618,6 +1618,74 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
 	return ok ? 0 : -1;
 }
 
+int parse_events_multi_pmu_add_or_add_pmu(struct parse_events_state *parse_state,
+					const char *event_or_pmu,
+					const struct parse_events_terms *const_parsed_terms,
+					struct list_head **listp,
+					void *loc_)
+{
+	char *pattern = NULL;
+	YYLTYPE *loc = loc_;
+	struct perf_pmu *pmu = NULL;
+	int ok = 0;
+	char *help;
+
+	*listp = malloc(sizeof(**listp));
+	if (!*listp)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(*listp);
+
+	/* Attempt to add to list assuming event_or_pmu is a PMU name. */
+	if (!parse_events_add_pmu(parse_state, *listp, event_or_pmu, const_parsed_terms,
+					/*auto_merge_stats=*/false, loc))
+		return 0;
+
+	/* Failed to add, try wildcard expansion of event_or_pmu as a PMU name. */
+	if (asprintf(&pattern, "%s*", event_or_pmu) < 0) {
+		zfree(listp);
+		return -ENOMEM;
+	}
+
+	while ((pmu = perf_pmus__scan(pmu)) != NULL) {
+		const char *name = pmu->name;
+
+		if (parse_events__filter_pmu(parse_state, pmu))
+			continue;
+
+		if (!strncmp(name, "uncore_", 7) &&
+		    strncmp(event_or_pmu, "uncore_", 7))
+			name += 7;
+		if (!perf_pmu__match(pattern, name, event_or_pmu) ||
+		    !perf_pmu__match(pattern, pmu->alias_name, event_or_pmu)) {
+			bool auto_merge_stats = perf_pmu__auto_merge_stats(pmu);
+
+			if (!parse_events_add_pmu(parse_state, *listp, pmu->name,
+						  const_parsed_terms,
+						  auto_merge_stats, loc)) {
+				ok++;
+				parse_state->wild_card_pmus = true;
+			}
+		}
+	}
+	zfree(&pattern);
+	if (ok)
+		return 0;
+
+	/* Failure to add, assume event_or_pmu is an event name. */
+	zfree(listp);
+	if (!parse_events_multi_pmu_add(parse_state, event_or_pmu, const_parsed_terms, listp, loc))
+		return 0;
+
+	if (asprintf(&help, "Unable to find PMU or event on a PMU of '%s'", event_or_pmu) < 0)
+		help = NULL;
+	parse_events_error__handle(parse_state->error, loc->first_column,
+				strdup("Bad event or PMU"),
+				help);
+	zfree(listp);
+	return -EINVAL;
+}
+
 int parse_events__modifier_group(struct list_head *list,
 				 char *event_mod)
 {
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index 809359e8544e..a331b9f0da2b 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -209,10 +209,6 @@ int parse_events_add_breakpoint(struct parse_events_state *parse_state,
 				struct list_head *list,
 				u64 addr, char *type, u64 len,
 				struct parse_events_terms *head_config);
-int parse_events_add_pmu(struct parse_events_state *parse_state,
-			 struct list_head *list, const char *name,
-			 const struct parse_events_terms *const_parsed_terms,
-			bool auto_merge_stats, void *loc);
 
 struct evsel *parse_events__add_event(int idx, struct perf_event_attr *attr,
 				      const char *name, const char *metric_id,
@@ -223,6 +219,12 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
 			       const struct parse_events_terms *const_parsed_terms,
 			       struct list_head **listp, void *loc);
 
+int parse_events_multi_pmu_add_or_add_pmu(struct parse_events_state *parse_state,
+					const char *event_or_pmu,
+					const struct parse_events_terms *const_parsed_terms,
+					struct list_head **listp,
+					void *loc_);
+
 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);
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index d70f5d84af92..7764e5895210 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -273,78 +273,15 @@ event_def: event_pmu |
 event_pmu:
 PE_NAME opt_pmu_config
 {
-	struct parse_events_state *parse_state = _parse_state;
 	/* List of created evsels. */
 	struct list_head *list = NULL;
-	char *pattern = NULL;
-
-#define CLEANUP						\
-	do {						\
-		parse_events_terms__delete($2);		\
-		free(list);				\
-		free($1);				\
-		free(pattern);				\
-	} while(0)
+	int err = parse_events_multi_pmu_add_or_add_pmu(_parse_state, $1, $2, &list, &@1);
 
-	list = alloc_list();
-	if (!list) {
-		CLEANUP;
-		YYNOMEM;
-	}
-	/* Attempt to add to list assuming $1 is a PMU name. */
-	if (parse_events_add_pmu(parse_state, list, $1, $2, /*auto_merge_stats=*/false, &@1)) {
-		struct perf_pmu *pmu = NULL;
-		int ok = 0;
-
-		/* Failure to add, try wildcard expansion of $1 as a PMU name. */
-		if (asprintf(&pattern, "%s*", $1) < 0) {
-			CLEANUP;
-			YYNOMEM;
-		}
-
-		while ((pmu = perf_pmus__scan(pmu)) != NULL) {
-			const char *name = pmu->name;
-
-			if (parse_events__filter_pmu(parse_state, pmu))
-				continue;
-
-			if (!strncmp(name, "uncore_", 7) &&
-			    strncmp($1, "uncore_", 7))
-				name += 7;
-			if (!perf_pmu__match(pattern, name, $1) ||
-			    !perf_pmu__match(pattern, pmu->alias_name, $1)) {
-				bool auto_merge_stats = perf_pmu__auto_merge_stats(pmu);
-
-				if (!parse_events_add_pmu(parse_state, list, pmu->name, $2,
-							  auto_merge_stats, &@1)) {
-					ok++;
-					parse_state->wild_card_pmus = true;
-				}
-			}
-		}
-
-		if (!ok) {
-			/* Failure to add, assume $1 is an event name. */
-			zfree(&list);
-			ok = !parse_events_multi_pmu_add(parse_state, $1, $2, &list, &@1);
-		}
-		if (!ok) {
-			struct parse_events_error *error = parse_state->error;
-			char *help;
-
-			if (asprintf(&help, "Unable to find PMU or event on a PMU of '%s'", $1) < 0)
-				help = NULL;
-			parse_events_error__handle(error, @1.first_column,
-						   strdup("Bad event or PMU"),
-						   help);
-			CLEANUP;
-			YYABORT;
-		}
-	}
+	parse_events_terms__delete($2);
+	free($1);
+	if (err)
+		PE_ABORT(err);
 	$$ = list;
-	list = NULL;
-	CLEANUP;
-#undef CLEANUP
 }
 |
 PE_NAME sep_dc
-- 
2.44.0.683.g7961c838ac-goog

[PATCH v2 02/16] perf parse-events: Directly pass PMU to parse_events_add_pmu

[Ian Rogers]

Avoid passing the name of a PMU then finding it again, just directly
pass the PMU. parse_events_multi_pmu_add_or_add_pmu is the only
version that needs to find a PMU, so move the find there. Remove the
error message as parse_events_multi_pmu_add_or_add_pmu will given an
error at the end when a name isn't either a PMU name or event
name. Without the error message being created the location in the
input parameter (loc) can be removed.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/parse-events.c | 46 +++++++++++++---------------------
 1 file changed, 17 insertions(+), 29 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index a6f71165ee1a..2d5a275dd257 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1386,32 +1386,18 @@ static bool config_term_percore(struct list_head *config_terms)
 }
 
 static int parse_events_add_pmu(struct parse_events_state *parse_state,
-			 struct list_head *list, const char *name,
-			 const struct parse_events_terms *const_parsed_terms,
-			 bool auto_merge_stats, void *loc_)
+				struct list_head *list, struct perf_pmu *pmu,
+				const struct parse_events_terms *const_parsed_terms,
+				bool auto_merge_stats)
 {
 	struct perf_event_attr attr;
 	struct perf_pmu_info info;
-	struct perf_pmu *pmu;
 	struct evsel *evsel;
 	struct parse_events_error *err = parse_state->error;
-	YYLTYPE *loc = loc_;
 	LIST_HEAD(config_terms);
 	struct parse_events_terms parsed_terms;
 	bool alias_rewrote_terms = false;
 
-	pmu = parse_state->fake_pmu ?: perf_pmus__find(name);
-
-	if (!pmu) {
-		char *err_str;
-
-		if (asprintf(&err_str,
-				"Cannot find PMU `%s'. Missing kernel support?",
-				name) >= 0)
-			parse_events_error__handle(err, loc->first_column, err_str, NULL);
-		return -EINVAL;
-	}
-
 	parse_events_terms__init(&parsed_terms);
 	if (const_parsed_terms) {
 		int ret = parse_events_terms__copy(const_parsed_terms, &parsed_terms);
@@ -1425,9 +1411,9 @@ static int parse_events_add_pmu(struct parse_events_state *parse_state,
 
 		strbuf_init(&sb, /*hint=*/ 0);
 		if (pmu->selectable && list_empty(&parsed_terms.terms)) {
-			strbuf_addf(&sb, "%s//", name);
+			strbuf_addf(&sb, "%s//", pmu->name);
 		} else {
-			strbuf_addf(&sb, "%s/", name);
+			strbuf_addf(&sb, "%s/", pmu->name);
 			parse_events_terms__to_strbuf(&parsed_terms, &sb);
 			strbuf_addch(&sb, '/');
 		}
@@ -1469,7 +1455,7 @@ static int parse_events_add_pmu(struct parse_events_state *parse_state,
 
 		strbuf_init(&sb, /*hint=*/ 0);
 		parse_events_terms__to_strbuf(&parsed_terms, &sb);
-		fprintf(stderr, "..after resolving event: %s/%s/\n", name, sb.buf);
+		fprintf(stderr, "..after resolving event: %s/%s/\n", pmu->name, sb.buf);
 		strbuf_release(&sb);
 	}
 
@@ -1583,8 +1569,8 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
 			continue;
 
 		auto_merge_stats = perf_pmu__auto_merge_stats(pmu);
-		if (!parse_events_add_pmu(parse_state, list, pmu->name,
-					  &parsed_terms, auto_merge_stats, loc)) {
+		if (!parse_events_add_pmu(parse_state, list, pmu,
+					  &parsed_terms, auto_merge_stats)) {
 			struct strbuf sb;
 
 			strbuf_init(&sb, /*hint=*/ 0);
@@ -1596,8 +1582,8 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
 	}
 
 	if (parse_state->fake_pmu) {
-		if (!parse_events_add_pmu(parse_state, list, event_name, &parsed_terms,
-					  /*auto_merge_stats=*/true, loc)) {
+		if (!parse_events_add_pmu(parse_state, list, parse_state->fake_pmu, &parsed_terms,
+					  /*auto_merge_stats=*/true)) {
 			struct strbuf sb;
 
 			strbuf_init(&sb, /*hint=*/ 0);
@@ -1626,7 +1612,7 @@ int parse_events_multi_pmu_add_or_add_pmu(struct parse_events_state *parse_state
 {
 	char *pattern = NULL;
 	YYLTYPE *loc = loc_;
-	struct perf_pmu *pmu = NULL;
+	struct perf_pmu *pmu;
 	int ok = 0;
 	char *help;
 
@@ -1637,10 +1623,12 @@ int parse_events_multi_pmu_add_or_add_pmu(struct parse_events_state *parse_state
 	INIT_LIST_HEAD(*listp);
 
 	/* Attempt to add to list assuming event_or_pmu is a PMU name. */
-	if (!parse_events_add_pmu(parse_state, *listp, event_or_pmu, const_parsed_terms,
-					/*auto_merge_stats=*/false, loc))
+	pmu = parse_state->fake_pmu ?: perf_pmus__find(event_or_pmu);
+	if (pmu && !parse_events_add_pmu(parse_state, *listp, pmu, const_parsed_terms,
+					/*auto_merge_stats=*/false))
 		return 0;
 
+	pmu = NULL;
 	/* Failed to add, try wildcard expansion of event_or_pmu as a PMU name. */
 	if (asprintf(&pattern, "%s*", event_or_pmu) < 0) {
 		zfree(listp);
@@ -1660,9 +1648,9 @@ int parse_events_multi_pmu_add_or_add_pmu(struct parse_events_state *parse_state
 		    !perf_pmu__match(pattern, pmu->alias_name, event_or_pmu)) {
 			bool auto_merge_stats = perf_pmu__auto_merge_stats(pmu);
 
-			if (!parse_events_add_pmu(parse_state, *listp, pmu->name,
+			if (!parse_events_add_pmu(parse_state, *listp, pmu,
 						  const_parsed_terms,
-						  auto_merge_stats, loc)) {
+						  auto_merge_stats)) {
 				ok++;
 				parse_state->wild_card_pmus = true;
 			}
-- 
2.44.0.683.g7961c838ac-goog

[PATCH v2 03/16] perf parse-events: Avoid copying an empty list

[Ian Rogers]

In parse_events_add_pmu, delay copying the list of terms until it is
known the list contains terms.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/parse-events.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 2d5a275dd257..3b1f767039fa 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1398,29 +1398,21 @@ static int parse_events_add_pmu(struct parse_events_state *parse_state,
 	struct parse_events_terms parsed_terms;
 	bool alias_rewrote_terms = false;
 
-	parse_events_terms__init(&parsed_terms);
-	if (const_parsed_terms) {
-		int ret = parse_events_terms__copy(const_parsed_terms, &parsed_terms);
-
-		if (ret)
-			return ret;
-	}
-
 	if (verbose > 1) {
 		struct strbuf sb;
 
 		strbuf_init(&sb, /*hint=*/ 0);
-		if (pmu->selectable && list_empty(&parsed_terms.terms)) {
+		if (pmu->selectable && const_parsed_terms &&
+		    list_empty(&const_parsed_terms->terms)) {
 			strbuf_addf(&sb, "%s//", pmu->name);
 		} else {
 			strbuf_addf(&sb, "%s/", pmu->name);
-			parse_events_terms__to_strbuf(&parsed_terms, &sb);
+			parse_events_terms__to_strbuf(const_parsed_terms, &sb);
 			strbuf_addch(&sb, '/');
 		}
 		fprintf(stderr, "Attempt to add: %s\n", sb.buf);
 		strbuf_release(&sb);
 	}
-	fix_raw(&parsed_terms, pmu);
 
 	memset(&attr, 0, sizeof(attr));
 	if (pmu->perf_event_attr_init_default)
@@ -1428,7 +1420,7 @@ static int parse_events_add_pmu(struct parse_events_state *parse_state,
 
 	attr.type = pmu->type;
 
-	if (list_empty(&parsed_terms.terms)) {
+	if (!const_parsed_terms || list_empty(&const_parsed_terms->terms)) {
 		evsel = __add_event(list, &parse_state->idx, &attr,
 				    /*init_attr=*/true, /*name=*/NULL,
 				    /*metric_id=*/NULL, pmu,
@@ -1437,6 +1429,15 @@ static int parse_events_add_pmu(struct parse_events_state *parse_state,
 		return evsel ? 0 : -ENOMEM;
 	}
 
+	parse_events_terms__init(&parsed_terms);
+	if (const_parsed_terms) {
+		int ret = parse_events_terms__copy(const_parsed_terms, &parsed_terms);
+
+		if (ret)
+			return ret;
+	}
+	fix_raw(&parsed_terms, pmu);
+
 	/* Configure attr/terms with a known PMU, this will set hardcoded terms. */
 	if (config_attr(&attr, &parsed_terms, parse_state->error, config_term_pmu)) {
 		parse_events_terms__exit(&parsed_terms);
-- 
2.44.0.683.g7961c838ac-goog

[PATCH v2 04/16] perf pmu: Refactor perf_pmu__match

[Ian Rogers]

Move all implementation to pmu code. Don't allocate a fnmatch wildcard
pattern, matching ignoring the suffix already handles this, and only
use fnmatch if the given PMU name has a '*' in it.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/parse-events.c | 19 ++-----------------
 tools/perf/util/pmu.c          | 27 +++++++++++++++++++--------
 tools/perf/util/pmu.h          |  2 +-
 3 files changed, 22 insertions(+), 26 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 3b1f767039fa..39548ec645ec 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1611,7 +1611,6 @@ int parse_events_multi_pmu_add_or_add_pmu(struct parse_events_state *parse_state
 					struct list_head **listp,
 					void *loc_)
 {
-	char *pattern = NULL;
 	YYLTYPE *loc = loc_;
 	struct perf_pmu *pmu;
 	int ok = 0;
@@ -1631,22 +1630,9 @@ int parse_events_multi_pmu_add_or_add_pmu(struct parse_events_state *parse_state
 
 	pmu = NULL;
 	/* Failed to add, try wildcard expansion of event_or_pmu as a PMU name. */
-	if (asprintf(&pattern, "%s*", event_or_pmu) < 0) {
-		zfree(listp);
-		return -ENOMEM;
-	}
-
 	while ((pmu = perf_pmus__scan(pmu)) != NULL) {
-		const char *name = pmu->name;
-
-		if (parse_events__filter_pmu(parse_state, pmu))
-			continue;
-
-		if (!strncmp(name, "uncore_", 7) &&
-		    strncmp(event_or_pmu, "uncore_", 7))
-			name += 7;
-		if (!perf_pmu__match(pattern, name, event_or_pmu) ||
-		    !perf_pmu__match(pattern, pmu->alias_name, event_or_pmu)) {
+		if (!parse_events__filter_pmu(parse_state, pmu) &&
+		    perf_pmu__match(pmu, event_or_pmu)) {
 			bool auto_merge_stats = perf_pmu__auto_merge_stats(pmu);
 
 			if (!parse_events_add_pmu(parse_state, *listp, pmu,
@@ -1657,7 +1643,6 @@ int parse_events_multi_pmu_add_or_add_pmu(struct parse_events_state *parse_state
 			}
 		}
 	}
-	zfree(&pattern);
 	if (ok)
 		return 0;
 
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index ce72c99e4f61..d7521d84fe4a 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -2073,18 +2073,29 @@ void perf_pmu__warn_invalid_config(struct perf_pmu *pmu, __u64 config,
 		   name ?: "N/A", buf, config_name, config);
 }
 
-int perf_pmu__match(const char *pattern, const char *name, const char *tok)
+bool perf_pmu__match(const struct perf_pmu *pmu, const char *tok)
 {
-	if (!name)
-		return -1;
+	const char *name = pmu->name;
+	bool need_fnmatch = strchr(tok, '*') != NULL;
 
-	if (fnmatch(pattern, name, 0))
-		return -1;
+	if (!strncmp(tok, "uncore_", 7))
+		tok += 7;
+	if (!strncmp(name, "uncore_", 7))
+		name += 7;
 
-	if (tok && !perf_pmu__match_ignoring_suffix(name, tok))
-		return -1;
+	if (perf_pmu__match_ignoring_suffix(name, tok) ||
+	    (need_fnmatch && !fnmatch(tok, name, 0)))
+		return true;
 
-	return 0;
+	name = pmu->alias_name;
+	if (!name)
+		return false;
+
+	if (!strncmp(name, "uncore_", 7))
+		name += 7;
+
+	return perf_pmu__match_ignoring_suffix(name, tok) ||
+		(need_fnmatch && !fnmatch(tok, name, 0));
 }
 
 double __weak perf_pmu__cpu_slots_per_cycle(void)
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index 152700f78455..93d03bd3ecbe 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -263,7 +263,7 @@ void perf_pmu__warn_invalid_config(struct perf_pmu *pmu, __u64 config,
 				   const char *config_name);
 void perf_pmu__warn_invalid_formats(struct perf_pmu *pmu);
 
-int perf_pmu__match(const char *pattern, const char *name, const char *tok);
+bool perf_pmu__match(const struct perf_pmu *pmu, const char *tok);
 
 double perf_pmu__cpu_slots_per_cycle(void);
 int perf_pmu__event_source_devices_scnprintf(char *pathname, size_t size);
-- 
2.44.0.683.g7961c838ac-goog

[PATCH v2 05/16] perf tests parse-events: Use branches rather than cache-references

[Ian Rogers]

Switch from cache-references to branches in test as Intel has a sysfs
event for cache-references and changing the priority for sysfs over
legacy causes the test to fail.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/parse-events.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
index 0b70451451b3..993e482f094c 100644
--- a/tools/perf/tests/parse-events.c
+++ b/tools/perf/tests/parse-events.c
@@ -942,8 +942,8 @@ static int test__group2(struct evlist *evlist)
 			continue;
 		}
 		if (evsel->core.attr.type == PERF_TYPE_HARDWARE &&
-		    test_config(evsel, PERF_COUNT_HW_CACHE_REFERENCES)) {
-			/* cache-references + :u modifier */
+		    test_config(evsel, PERF_COUNT_HW_BRANCH_INSTRUCTIONS)) {
+			/* branches + :u modifier */
 			TEST_ASSERT_VAL("wrong exclude_user", !evsel->core.attr.exclude_user);
 			TEST_ASSERT_VAL("wrong exclude_kernel", evsel->core.attr.exclude_kernel);
 			TEST_ASSERT_VAL("wrong exclude_hv", evsel->core.attr.exclude_hv);
@@ -2032,7 +2032,7 @@ static const struct evlist_test test__events[] = {
 		/* 8 */
 	},
 	{
-		.name  = "{faults:k,cache-references}:u,cycles:k",
+		.name  = "{faults:k,branches}:u,cycles:k",
 		.check = test__group2,
 		/* 9 */
 	},
-- 
2.44.0.683.g7961c838ac-goog

[PATCH v2 06/16] perf parse-events: Legacy cache names on all PMUs and lower priority

[Ian Rogers]

Prior behavior is to not look for legacy cache names in sysfs/json and
to create events on all core PMUs. New behavior is to look for
sysfs/json events first on all PMUs, for core PMUs add a legacy event
if the sysfs/json event isn't present.

This is done so that there is consistency with how event names in
terms are handled and their prioritization of sysfs/json over
legacy. It may make sense to use a legacy cache event name as an event
name on a non-core PMU so we should allow it.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/parse-events.c | 39 +++++++++++++++++++++++++++-------
 tools/perf/util/parse-events.h |  2 +-
 2 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 39548ec645ec..1440a3b4b674 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -442,17 +442,21 @@ bool parse_events__filter_pmu(const struct parse_events_state *parse_state,
 	return strcmp(parse_state->pmu_filter, pmu->name) != 0;
 }
 
+static int parse_events_add_pmu(struct parse_events_state *parse_state,
+				struct list_head *list, struct perf_pmu *pmu,
+				const struct parse_events_terms *const_parsed_terms,
+				bool auto_merge_stats);
+
 int parse_events_add_cache(struct list_head *list, int *idx, const char *name,
 			   struct parse_events_state *parse_state,
-			   struct parse_events_terms *head_config)
+			   struct parse_events_terms *parsed_terms)
 {
 	struct perf_pmu *pmu = NULL;
 	bool found_supported = false;
-	const char *config_name = get_config_name(head_config);
-	const char *metric_id = get_config_metric_id(head_config);
+	const char *config_name = get_config_name(parsed_terms);
+	const char *metric_id = get_config_metric_id(parsed_terms);
 
-	/* Legacy cache events are only supported by core PMUs. */
-	while ((pmu = perf_pmus__scan_core(pmu)) != NULL) {
+	while ((pmu = perf_pmus__scan(pmu)) != NULL) {
 		LIST_HEAD(config_terms);
 		struct perf_event_attr attr;
 		int ret;
@@ -460,6 +464,24 @@ int parse_events_add_cache(struct list_head *list, int *idx, const char *name,
 		if (parse_events__filter_pmu(parse_state, pmu))
 			continue;
 
+		if (perf_pmu__have_event(pmu, name)) {
+			/*
+			 * The PMU has the event so add as not a legacy cache
+			 * event.
+			 */
+			ret = parse_events_add_pmu(parse_state, list, pmu,
+						   parsed_terms,
+						   perf_pmu__auto_merge_stats(pmu));
+			if (ret)
+				return ret;
+			continue;
+		}
+
+		if (!pmu->is_core) {
+			/* Legacy cache events are only supported by core PMUs. */
+			continue;
+		}
+
 		memset(&attr, 0, sizeof(attr));
 		attr.type = PERF_TYPE_HW_CACHE;
 
@@ -469,11 +491,12 @@ int parse_events_add_cache(struct list_head *list, int *idx, const char *name,
 
 		found_supported = true;
 
-		if (head_config) {
-			if (config_attr(&attr, head_config, parse_state->error, config_term_common))
+		if (parsed_terms) {
+			if (config_attr(&attr, parsed_terms, parse_state->error,
+					config_term_common))
 				return -EINVAL;
 
-			if (get_config_terms(head_config, &config_terms))
+			if (get_config_terms(parsed_terms, &config_terms))
 				return -ENOMEM;
 		}
 
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index a331b9f0da2b..db47913e54bc 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -203,7 +203,7 @@ int parse_events_add_tool(struct parse_events_state *parse_state,
 			  int tool_event);
 int parse_events_add_cache(struct list_head *list, int *idx, const char *name,
 			   struct parse_events_state *parse_state,
-			   struct parse_events_terms *head_config);
+			   struct parse_events_terms *parsed_terms);
 int parse_events__decode_legacy_cache(const char *name, int pmu_type, __u64 *config);
 int parse_events_add_breakpoint(struct parse_events_state *parse_state,
 				struct list_head *list,
-- 
2.44.0.683.g7961c838ac-goog

[PATCH v2 07/16] perf parse-events: Handle PE_TERM_HW in name_or_raw

[Ian Rogers]

Avoid duplicate logic for name_or_raw and PE_TERM_HW by having a rule
to turn PE_TERM_HW into a name_or_raw.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/parse-events.y | 31 +++++--------------------------
 1 file changed, 5 insertions(+), 26 deletions(-)

diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 7764e5895210..254f8aeca461 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -603,6 +603,11 @@ event_term
 }
 
 name_or_raw: PE_RAW | PE_NAME | PE_LEGACY_CACHE
+|
+PE_TERM_HW
+{
+	$$ = $1.str;
+}
 
 event_term:
 PE_RAW
@@ -644,20 +649,6 @@ name_or_raw '=' PE_VALUE
 	$$ = term;
 }
 |
-name_or_raw '=' PE_TERM_HW
-{
-	struct parse_events_term *term;
-	int err = parse_events_term__str(&term, PARSE_EVENTS__TERM_TYPE_USER,
-					 $1, $3.str, &@1, &@3);
-
-	if (err) {
-		free($1);
-		free($3.str);
-		PE_ABORT(err);
-	}
-	$$ = term;
-}
-|
 PE_LEGACY_CACHE
 {
 	struct parse_events_term *term;
@@ -710,18 +701,6 @@ PE_TERM '=' name_or_raw
 	$$ = term;
 }
 |
-PE_TERM '=' PE_TERM_HW
-{
-	struct parse_events_term *term;
-	int err = parse_events_term__str(&term, $1, /*config=*/NULL, $3.str, &@1, &@3);
-
-	if (err) {
-		free($3.str);
-		PE_ABORT(err);
-	}
-	$$ = term;
-}
-|
 PE_TERM '=' PE_TERM
 {
 	struct parse_events_term *term;
-- 
2.44.0.683.g7961c838ac-goog

[PATCH v2 08/16] perf parse-events: Constify parse_events_add_numeric

[Ian Rogers]

Allow the term list to be const so that other functions can pass const
term lists. Add const as necessary to called functions.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/parse-events.c | 20 +++++++++++---------
 tools/perf/util/parse-events.h |  2 +-
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 1440a3b4b674..1b408e3dccc7 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -34,7 +34,8 @@
 #ifdef PARSER_DEBUG
 extern int parse_events_debug;
 #endif
-static int get_config_terms(struct parse_events_terms *head_config, struct list_head *head_terms);
+static int get_config_terms(const struct parse_events_terms *head_config,
+			    struct list_head *head_terms);
 static int parse_events_terms__copy(const struct parse_events_terms *src,
 				    struct parse_events_terms *dest);
 
@@ -154,7 +155,7 @@ const char *event_type(int type)
 	return "unknown";
 }
 
-static char *get_config_str(struct parse_events_terms *head_terms,
+static char *get_config_str(const struct parse_events_terms *head_terms,
 			    enum parse_events__term_type type_term)
 {
 	struct parse_events_term *term;
@@ -169,12 +170,12 @@ static char *get_config_str(struct parse_events_terms *head_terms,
 	return NULL;
 }
 
-static char *get_config_metric_id(struct parse_events_terms *head_terms)
+static char *get_config_metric_id(const struct parse_events_terms *head_terms)
 {
 	return get_config_str(head_terms, PARSE_EVENTS__TERM_TYPE_METRIC_ID);
 }
 
-static char *get_config_name(struct parse_events_terms *head_terms)
+static char *get_config_name(const struct parse_events_terms *head_terms)
 {
 	return get_config_str(head_terms, PARSE_EVENTS__TERM_TYPE_NAME);
 }
@@ -358,7 +359,7 @@ static int config_term_common(struct perf_event_attr *attr,
 			      struct parse_events_term *term,
 			      struct parse_events_error *err);
 static int config_attr(struct perf_event_attr *attr,
-		       struct parse_events_terms *head,
+		       const struct parse_events_terms *head,
 		       struct parse_events_error *err,
 		       config_term_func_t config_term);
 
@@ -1108,7 +1109,7 @@ static int config_term_tracepoint(struct perf_event_attr *attr,
 #endif
 
 static int config_attr(struct perf_event_attr *attr,
-		       struct parse_events_terms *head,
+		       const struct parse_events_terms *head,
 		       struct parse_events_error *err,
 		       config_term_func_t config_term)
 {
@@ -1121,7 +1122,8 @@ static int config_attr(struct perf_event_attr *attr,
 	return 0;
 }
 
-static int get_config_terms(struct parse_events_terms *head_config, struct list_head *head_terms)
+static int get_config_terms(const struct parse_events_terms *head_config,
+			    struct list_head *head_terms)
 {
 #define ADD_CONFIG_TERM(__type, __weak)				\
 	struct evsel_config_term *__t;			\
@@ -1325,7 +1327,7 @@ int parse_events_add_tracepoint(struct list_head *list, int *idx,
 static int __parse_events_add_numeric(struct parse_events_state *parse_state,
 				struct list_head *list,
 				struct perf_pmu *pmu, u32 type, u32 extended_type,
-				u64 config, struct parse_events_terms *head_config)
+				u64 config, const struct parse_events_terms *head_config)
 {
 	struct perf_event_attr attr;
 	LIST_HEAD(config_terms);
@@ -1361,7 +1363,7 @@ static int __parse_events_add_numeric(struct parse_events_state *parse_state,
 int parse_events_add_numeric(struct parse_events_state *parse_state,
 			     struct list_head *list,
 			     u32 type, u64 config,
-			     struct parse_events_terms *head_config,
+			     const struct parse_events_terms *head_config,
 			     bool wildcard)
 {
 	struct perf_pmu *pmu = NULL;
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index db47913e54bc..5005782766e9 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -196,7 +196,7 @@ int parse_events_add_tracepoint(struct list_head *list, int *idx,
 int parse_events_add_numeric(struct parse_events_state *parse_state,
 			     struct list_head *list,
 			     u32 type, u64 config,
-			     struct parse_events_terms *head_config,
+			     const struct parse_events_terms *head_config,
 			     bool wildcard);
 int parse_events_add_tool(struct parse_events_state *parse_state,
 			  struct list_head *list,
-- 
2.44.0.683.g7961c838ac-goog

[PATCH v2 09/16] perf parse-events: Prefer sysfs/json hardware events over legacy

[Ian Rogers]

It was requested that RISC-V be able to add events to the perf tool so
the PMU driver didn't need to map legacy events to config encodings:
https://lore.kernel.org/lkml/20240217005738.3744121-1-atishp@rivosinc.com/

This change makes the priority of events specified without a PMU the
same as those specified with a PMU, namely sysfs and json events are
checked first before using the legacy encoding.

The hw_term is made more generic as a hardware_event that encodes a
pair of string and int value, allowing parse_events_multi_pmu_add to
fall back on a known encoding when the sysfs/json adding fails for
core events. As this covers PE_VALUE_SYM_HW, that token is removed and
related code simplified.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/parse-events.c | 31 ++++++++++----
 tools/perf/util/parse-events.h |  2 +-
 tools/perf/util/parse-events.l | 76 +++++++++++++++++-----------------
 tools/perf/util/parse-events.y | 62 +++++++++++++++++----------
 4 files changed, 103 insertions(+), 68 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 1b408e3dccc7..805872c90a3e 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1543,7 +1543,7 @@ static int parse_events_add_pmu(struct parse_events_state *parse_state,
 }
 
 int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
-			       const char *event_name,
+			       const char *event_name, u64 hw_config,
 			       const struct parse_events_terms *const_parsed_terms,
 			       struct list_head **listp, void *loc_)
 {
@@ -1551,8 +1551,8 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
 	struct list_head *list = NULL;
 	struct perf_pmu *pmu = NULL;
 	YYLTYPE *loc = loc_;
-	int ok = 0;
-	const char *config;
+	int ok = 0, core_ok = 0;
+	const char *tmp;
 	struct parse_events_terms parsed_terms;
 
 	*listp = NULL;
@@ -1565,15 +1565,15 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
 			return ret;
 	}
 
-	config = strdup(event_name);
-	if (!config)
+	tmp = strdup(event_name);
+	if (!tmp)
 		goto out_err;
 
 	if (parse_events_term__num(&term,
 				   PARSE_EVENTS__TERM_TYPE_USER,
-				   config, /*num=*/1, /*novalue=*/true,
+				   tmp, /*num=*/1, /*novalue=*/true,
 				   loc, /*loc_val=*/NULL) < 0) {
-		zfree(&config);
+		zfree(&tmp);
 		goto out_err;
 	}
 	list_add_tail(&term->list, &parsed_terms.terms);
@@ -1604,6 +1604,8 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
 			pr_debug("%s -> %s/%s/\n", event_name, pmu->name, sb.buf);
 			strbuf_release(&sb);
 			ok++;
+			if (pmu->is_core)
+				core_ok++;
 		}
 	}
 
@@ -1620,6 +1622,18 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
 		}
 	}
 
+	if (hw_config != PERF_COUNT_HW_MAX && !core_ok) {
+		/*
+		 * The event wasn't found on core PMUs but it has a hardware
+		 * config version to try.
+		 */
+		if (!parse_events_add_numeric(parse_state, list,
+						PERF_TYPE_HARDWARE, hw_config,
+						const_parsed_terms,
+						/*wildcard=*/true))
+			ok++;
+	}
+
 out_err:
 	parse_events_terms__exit(&parsed_terms);
 	if (ok)
@@ -1673,7 +1687,8 @@ int parse_events_multi_pmu_add_or_add_pmu(struct parse_events_state *parse_state
 
 	/* Failure to add, assume event_or_pmu is an event name. */
 	zfree(listp);
-	if (!parse_events_multi_pmu_add(parse_state, event_or_pmu, const_parsed_terms, listp, loc))
+	if (!parse_events_multi_pmu_add(parse_state, event_or_pmu, PERF_COUNT_HW_MAX,
+					const_parsed_terms, listp, loc))
 		return 0;
 
 	if (asprintf(&help, "Unable to find PMU or event on a PMU of '%s'", event_or_pmu) < 0)
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index 5005782766e9..7e5afad3feb8 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -215,7 +215,7 @@ struct evsel *parse_events__add_event(int idx, struct perf_event_attr *attr,
 				      struct perf_pmu *pmu);
 
 int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
-			       const char *event_name,
+			       const char *event_name, u64 hw_config,
 			       const struct parse_events_terms *const_parsed_terms,
 			       struct list_head **listp, void *loc);
 
diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index e86c45675e1d..6fe37003ab7b 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -100,12 +100,12 @@ do {								\
 	yyless(0);						\
 } while (0)
 
-static int sym(yyscan_t scanner, int type, int config)
+static int sym(yyscan_t scanner, int config)
 {
 	YYSTYPE *yylval = parse_events_get_lval(scanner);
 
-	yylval->num = (type << 16) + config;
-	return type == PERF_TYPE_HARDWARE ? PE_VALUE_SYM_HW : PE_VALUE_SYM_SW;
+	yylval->num = config;
+	return PE_VALUE_SYM_SW;
 }
 
 static int tool(yyscan_t scanner, enum perf_tool_event event)
@@ -124,13 +124,13 @@ static int term(yyscan_t scanner, enum parse_events__term_type type)
 	return PE_TERM;
 }
 
-static int hw_term(yyscan_t scanner, int config)
+static int hw(yyscan_t scanner, int config)
 {
 	YYSTYPE *yylval = parse_events_get_lval(scanner);
 	char *text = parse_events_get_text(scanner);
 
-	yylval->hardware_term.str = strdup(text);
-	yylval->hardware_term.num = PERF_TYPE_HARDWARE + config;
+	yylval->hardware_event.str = strdup(text);
+	yylval->hardware_event.num = config;
 	return PE_TERM_HW;
 }
 
@@ -246,16 +246,16 @@ percore			{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_PERCORE); }
 aux-output		{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_AUX_OUTPUT); }
 aux-sample-size		{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_AUX_SAMPLE_SIZE); }
 metric-id		{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_METRIC_ID); }
-cpu-cycles|cycles				{ return hw_term(yyscanner, PERF_COUNT_HW_CPU_CYCLES); }
-stalled-cycles-frontend|idle-cycles-frontend	{ return hw_term(yyscanner, PERF_COUNT_HW_STALLED_CYCLES_FRONTEND); }
-stalled-cycles-backend|idle-cycles-backend	{ return hw_term(yyscanner, PERF_COUNT_HW_STALLED_CYCLES_BACKEND); }
-instructions					{ return hw_term(yyscanner, PERF_COUNT_HW_INSTRUCTIONS); }
-cache-references				{ return hw_term(yyscanner, PERF_COUNT_HW_CACHE_REFERENCES); }
-cache-misses					{ return hw_term(yyscanner, PERF_COUNT_HW_CACHE_MISSES); }
-branch-instructions|branches			{ return hw_term(yyscanner, PERF_COUNT_HW_BRANCH_INSTRUCTIONS); }
-branch-misses					{ return hw_term(yyscanner, PERF_COUNT_HW_BRANCH_MISSES); }
-bus-cycles					{ return hw_term(yyscanner, PERF_COUNT_HW_BUS_CYCLES); }
-ref-cycles					{ return hw_term(yyscanner, PERF_COUNT_HW_REF_CPU_CYCLES); }
+cpu-cycles|cycles				{ return hw(yyscanner, PERF_COUNT_HW_CPU_CYCLES); }
+stalled-cycles-frontend|idle-cycles-frontend	{ return hw(yyscanner, PERF_COUNT_HW_STALLED_CYCLES_FRONTEND); }
+stalled-cycles-backend|idle-cycles-backend	{ return hw(yyscanner, PERF_COUNT_HW_STALLED_CYCLES_BACKEND); }
+instructions					{ return hw(yyscanner, PERF_COUNT_HW_INSTRUCTIONS); }
+cache-references				{ return hw(yyscanner, PERF_COUNT_HW_CACHE_REFERENCES); }
+cache-misses					{ return hw(yyscanner, PERF_COUNT_HW_CACHE_MISSES); }
+branch-instructions|branches			{ return hw(yyscanner, PERF_COUNT_HW_BRANCH_INSTRUCTIONS); }
+branch-misses					{ return hw(yyscanner, PERF_COUNT_HW_BRANCH_MISSES); }
+bus-cycles					{ return hw(yyscanner, PERF_COUNT_HW_BUS_CYCLES); }
+ref-cycles					{ return hw(yyscanner, PERF_COUNT_HW_REF_CPU_CYCLES); }
 r{num_raw_hex}		{ return str(yyscanner, PE_RAW); }
 r0x{num_raw_hex}	{ return str(yyscanner, PE_RAW); }
 ,			{ return ','; }
@@ -299,31 +299,31 @@ r0x{num_raw_hex}	{ return str(yyscanner, PE_RAW); }
 <<EOF>>			{ BEGIN(INITIAL); }
 }
 
-cpu-cycles|cycles				{ return sym(yyscanner, PERF_TYPE_HARDWARE, PERF_COUNT_HW_CPU_CYCLES); }
-stalled-cycles-frontend|idle-cycles-frontend	{ return sym(yyscanner, PERF_TYPE_HARDWARE, PERF_COUNT_HW_STALLED_CYCLES_FRONTEND); }
-stalled-cycles-backend|idle-cycles-backend	{ return sym(yyscanner, PERF_TYPE_HARDWARE, PERF_COUNT_HW_STALLED_CYCLES_BACKEND); }
-instructions					{ return sym(yyscanner, PERF_TYPE_HARDWARE, PERF_COUNT_HW_INSTRUCTIONS); }
-cache-references				{ return sym(yyscanner, PERF_TYPE_HARDWARE, PERF_COUNT_HW_CACHE_REFERENCES); }
-cache-misses					{ return sym(yyscanner, PERF_TYPE_HARDWARE, PERF_COUNT_HW_CACHE_MISSES); }
-branch-instructions|branches			{ return sym(yyscanner, PERF_TYPE_HARDWARE, PERF_COUNT_HW_BRANCH_INSTRUCTIONS); }
-branch-misses					{ return sym(yyscanner, PERF_TYPE_HARDWARE, PERF_COUNT_HW_BRANCH_MISSES); }
-bus-cycles					{ return sym(yyscanner, PERF_TYPE_HARDWARE, PERF_COUNT_HW_BUS_CYCLES); }
-ref-cycles					{ return sym(yyscanner, PERF_TYPE_HARDWARE, PERF_COUNT_HW_REF_CPU_CYCLES); }
-cpu-clock					{ return sym(yyscanner, PERF_TYPE_SOFTWARE, PERF_COUNT_SW_CPU_CLOCK); }
-task-clock					{ return sym(yyscanner, PERF_TYPE_SOFTWARE, PERF_COUNT_SW_TASK_CLOCK); }
-page-faults|faults				{ return sym(yyscanner, PERF_TYPE_SOFTWARE, PERF_COUNT_SW_PAGE_FAULTS); }
-minor-faults					{ return sym(yyscanner, PERF_TYPE_SOFTWARE, PERF_COUNT_SW_PAGE_FAULTS_MIN); }
-major-faults					{ return sym(yyscanner, PERF_TYPE_SOFTWARE, PERF_COUNT_SW_PAGE_FAULTS_MAJ); }
-context-switches|cs				{ return sym(yyscanner, PERF_TYPE_SOFTWARE, PERF_COUNT_SW_CONTEXT_SWITCHES); }
-cpu-migrations|migrations			{ return sym(yyscanner, PERF_TYPE_SOFTWARE, PERF_COUNT_SW_CPU_MIGRATIONS); }
-alignment-faults				{ return sym(yyscanner, PERF_TYPE_SOFTWARE, PERF_COUNT_SW_ALIGNMENT_FAULTS); }
-emulation-faults				{ return sym(yyscanner, PERF_TYPE_SOFTWARE, PERF_COUNT_SW_EMULATION_FAULTS); }
-dummy						{ return sym(yyscanner, PERF_TYPE_SOFTWARE, PERF_COUNT_SW_DUMMY); }
+cpu-cycles|cycles				{ return hw(yyscanner, PERF_COUNT_HW_CPU_CYCLES); }
+stalled-cycles-frontend|idle-cycles-frontend	{ return hw(yyscanner, PERF_COUNT_HW_STALLED_CYCLES_FRONTEND); }
+stalled-cycles-backend|idle-cycles-backend	{ return hw(yyscanner, PERF_COUNT_HW_STALLED_CYCLES_BACKEND); }
+instructions					{ return hw(yyscanner, PERF_COUNT_HW_INSTRUCTIONS); }
+cache-references				{ return hw(yyscanner, PERF_COUNT_HW_CACHE_REFERENCES); }
+cache-misses					{ return hw(yyscanner, PERF_COUNT_HW_CACHE_MISSES); }
+branch-instructions|branches			{ return hw(yyscanner, PERF_COUNT_HW_BRANCH_INSTRUCTIONS); }
+branch-misses					{ return hw(yyscanner, PERF_COUNT_HW_BRANCH_MISSES); }
+bus-cycles					{ return hw(yyscanner, PERF_COUNT_HW_BUS_CYCLES); }
+ref-cycles					{ return hw(yyscanner, PERF_COUNT_HW_REF_CPU_CYCLES); }
+cpu-clock					{ return sym(yyscanner, PERF_COUNT_SW_CPU_CLOCK); }
+task-clock					{ return sym(yyscanner, PERF_COUNT_SW_TASK_CLOCK); }
+page-faults|faults				{ return sym(yyscanner, PERF_COUNT_SW_PAGE_FAULTS); }
+minor-faults					{ return sym(yyscanner, PERF_COUNT_SW_PAGE_FAULTS_MIN); }
+major-faults					{ return sym(yyscanner, PERF_COUNT_SW_PAGE_FAULTS_MAJ); }
+context-switches|cs				{ return sym(yyscanner, PERF_COUNT_SW_CONTEXT_SWITCHES); }
+cpu-migrations|migrations			{ return sym(yyscanner, PERF_COUNT_SW_CPU_MIGRATIONS); }
+alignment-faults				{ return sym(yyscanner, PERF_COUNT_SW_ALIGNMENT_FAULTS); }
+emulation-faults				{ return sym(yyscanner, PERF_COUNT_SW_EMULATION_FAULTS); }
+dummy						{ return sym(yyscanner, PERF_COUNT_SW_DUMMY); }
 duration_time					{ return tool(yyscanner, PERF_TOOL_DURATION_TIME); }
 user_time						{ return tool(yyscanner, PERF_TOOL_USER_TIME); }
 system_time						{ return tool(yyscanner, PERF_TOOL_SYSTEM_TIME); }
-bpf-output					{ return sym(yyscanner, PERF_TYPE_SOFTWARE, PERF_COUNT_SW_BPF_OUTPUT); }
-cgroup-switches					{ return sym(yyscanner, PERF_TYPE_SOFTWARE, PERF_COUNT_SW_CGROUP_SWITCHES); }
+bpf-output					{ return sym(yyscanner, PERF_COUNT_SW_BPF_OUTPUT); }
+cgroup-switches					{ return sym(yyscanner, PERF_COUNT_SW_CGROUP_SWITCHES); }
 
 {lc_type}			{ return str(yyscanner, PE_LEGACY_CACHE); }
 {lc_type}-{lc_op_result}	{ return str(yyscanner, PE_LEGACY_CACHE); }
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 254f8aeca461..31fe8cf428ff 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -55,7 +55,7 @@ static void free_list_evsel(struct list_head* list_evsel)
 %}
 
 %token PE_START_EVENTS PE_START_TERMS
-%token PE_VALUE PE_VALUE_SYM_HW PE_VALUE_SYM_SW PE_TERM
+%token PE_VALUE PE_VALUE_SYM_SW PE_TERM
 %token PE_VALUE_SYM_TOOL
 %token PE_EVENT_NAME
 %token PE_RAW PE_NAME
@@ -66,11 +66,9 @@ static void free_list_evsel(struct list_head* list_evsel)
 %token PE_DRV_CFG_TERM
 %token PE_TERM_HW
 %type <num> PE_VALUE
-%type <num> PE_VALUE_SYM_HW
 %type <num> PE_VALUE_SYM_SW
 %type <num> PE_VALUE_SYM_TOOL
 %type <term_type> PE_TERM
-%type <num> value_sym
 %type <str> PE_RAW
 %type <str> PE_NAME
 %type <str> PE_LEGACY_CACHE
@@ -87,6 +85,7 @@ static void free_list_evsel(struct list_head* list_evsel)
 %type <list_terms> opt_pmu_config
 %destructor { parse_events_terms__delete ($$); } <list_terms>
 %type <list_evsel> event_pmu
+%type <list_evsel> event_legacy_hardware
 %type <list_evsel> event_legacy_symbol
 %type <list_evsel> event_legacy_cache
 %type <list_evsel> event_legacy_mem
@@ -104,8 +103,8 @@ static void free_list_evsel(struct list_head* list_evsel)
 %destructor { free_list_evsel ($$); } <list_evsel>
 %type <tracepoint_name> tracepoint_name
 %destructor { free ($$.sys); free ($$.event); } <tracepoint_name>
-%type <hardware_term> PE_TERM_HW
-%destructor { free ($$.str); } <hardware_term>
+%type <hardware_event> PE_TERM_HW
+%destructor { free ($$.str); } <hardware_event>
 
 %union
 {
@@ -119,10 +118,10 @@ static void free_list_evsel(struct list_head* list_evsel)
 		char *sys;
 		char *event;
 	} tracepoint_name;
-	struct hardware_term {
+	struct hardware_event {
 		char *str;
 		u64 num;
-	} hardware_term;
+	} hardware_event;
 }
 %%
 
@@ -263,6 +262,7 @@ PE_EVENT_NAME event_def
 event_def
 
 event_def: event_pmu |
+	   event_legacy_hardware |
 	   event_legacy_symbol |
 	   event_legacy_cache sep_dc |
 	   event_legacy_mem sep_dc |
@@ -289,7 +289,7 @@ PE_NAME sep_dc
 	struct list_head *list;
 	int err;
 
-	err = parse_events_multi_pmu_add(_parse_state, $1, NULL, &list, &@1);
+	err = parse_events_multi_pmu_add(_parse_state, $1, PERF_COUNT_HW_MAX, NULL, &list, &@1);
 	if (err < 0) {
 		struct parse_events_state *parse_state = _parse_state;
 		struct parse_events_error *error = parse_state->error;
@@ -305,24 +305,45 @@ PE_NAME sep_dc
 	$$ = list;
 }
 
-value_sym:
-PE_VALUE_SYM_HW
+event_legacy_hardware:
+PE_TERM_HW opt_pmu_config
+{
+	/* List of created evsels. */
+	struct list_head *list = NULL;
+	int err = parse_events_multi_pmu_add(_parse_state, $1.str, $1.num, $2, &list, &@1);
+
+	free($1.str);
+	parse_events_terms__delete($2);
+	if (err)
+		PE_ABORT(err);
+
+	$$ = list;
+}
 |
-PE_VALUE_SYM_SW
+PE_TERM_HW sep_dc
+{
+	struct list_head *list;
+	int err;
+
+	err = parse_events_multi_pmu_add(_parse_state, $1.str, $1.num, NULL, &list, &@1);
+	free($1.str);
+	if (err)
+		PE_ABORT(err);
+	$$ = list;
+}
 
 event_legacy_symbol:
-value_sym '/' event_config '/'
+PE_VALUE_SYM_SW '/' event_config '/'
 {
 	struct list_head *list;
-	int type = $1 >> 16;
-	int config = $1 & 255;
 	int err;
-	bool wildcard = (type == PERF_TYPE_HARDWARE || type == PERF_TYPE_HW_CACHE);
 
 	list = alloc_list();
 	if (!list)
 		YYNOMEM;
-	err = parse_events_add_numeric(_parse_state, list, type, config, $3, wildcard);
+	err = parse_events_add_numeric(_parse_state, list,
+				/*type=*/PERF_TYPE_SOFTWARE, /*config=*/$1,
+				$3, /*wildcard=*/false);
 	parse_events_terms__delete($3);
 	if (err) {
 		free_list_evsel(list);
@@ -331,18 +352,17 @@ value_sym '/' event_config '/'
 	$$ = list;
 }
 |
-value_sym sep_slash_slash_dc
+PE_VALUE_SYM_SW sep_slash_slash_dc
 {
 	struct list_head *list;
-	int type = $1 >> 16;
-	int config = $1 & 255;
-	bool wildcard = (type == PERF_TYPE_HARDWARE || type == PERF_TYPE_HW_CACHE);
 	int err;
 
 	list = alloc_list();
 	if (!list)
 		YYNOMEM;
-	err = parse_events_add_numeric(_parse_state, list, type, config, /*head_config=*/NULL, wildcard);
+	err = parse_events_add_numeric(_parse_state, list,
+				/*type=*/PERF_TYPE_SOFTWARE, /*config=*/$1,
+				/*head_config=*/NULL, /*wildcard=*/false);
 	if (err)
 		PE_ABORT(err);
 	$$ = list;
-- 
2.44.0.683.g7961c838ac-goog

[PATCH v2 10/16] perf parse-events: Inline parse_events_update_lists

[Ian Rogers]

The helper function just wraps a splice and free. Making the free
inline removes a comment, so then it just wraps a splice which we can
make inline too.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/parse-events.c | 13 -----------
 tools/perf/util/parse-events.h |  2 --
 tools/perf/util/parse-events.y | 41 +++++++++++++++++++++-------------
 3 files changed, 25 insertions(+), 31 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 805872c90a3e..7eba714f0d73 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1720,19 +1720,6 @@ void parse_events__set_leader(char *name, struct list_head *list)
 	leader->group_name = name;
 }
 
-/* list_event is assumed to point to malloc'ed memory */
-void parse_events_update_lists(struct list_head *list_event,
-			       struct list_head *list_all)
-{
-	/*
-	 * Called for single event definition. Update the
-	 * 'all event' list, and reinit the 'single event'
-	 * list, for next event definition.
-	 */
-	list_splice_tail(list_event, list_all);
-	free(list_event);
-}
-
 struct event_modifier {
 	int eu;
 	int ek;
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index 7e5afad3feb8..e8f2aebea10f 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -226,8 +226,6 @@ int parse_events_multi_pmu_add_or_add_pmu(struct parse_events_state *parse_state
 					void *loc_);
 
 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_evlist_error(struct parse_events_state *parse_state,
 			       int idx, const char *str);
 
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 31fe8cf428ff..51490f0f8c50 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -125,6 +125,10 @@ static void free_list_evsel(struct list_head* list_evsel)
 }
 %%
 
+ /*
+  * Entry points. We are either parsing events or terminals. Just terminal
+  * parsing is used for parsing events in sysfs.
+  */
 start:
 PE_START_EVENTS start_events
 |
@@ -132,31 +136,36 @@ PE_START_TERMS  start_terms
 
 start_events: groups
 {
+	/* Take the parsed events, groups.. and place into parse_state. */
+	struct list_head *groups  = $1;
 	struct parse_events_state *parse_state = _parse_state;
 
-	/* frees $1 */
-	parse_events_update_lists($1, &parse_state->list);
+	list_splice_tail(groups, &parse_state->list);
+	free(groups);
 }
 
-groups:
+groups: /* A list of groups or events. */
 groups ',' group
 {
-	struct list_head *list  = $1;
-	struct list_head *group = $3;
+	/* Merge group into the list of events/groups. */
+	struct list_head *groups  = $1;
+	struct list_head *group  = $3;
 
-	/* frees $3 */
-	parse_events_update_lists(group, list);
-	$$ = list;
+	list_splice_tail(group, groups);
+	free(group);
+	$$ = groups;
 }
 |
 groups ',' event
 {
-	struct list_head *list  = $1;
+	/* Merge event into the list of events/groups. */
+	struct list_head *groups  = $1;
 	struct list_head *event = $3;
 
-	/* frees $3 */
-	parse_events_update_lists(event, list);
-	$$ = list;
+
+	list_splice_tail(event, groups);
+	free(event);
+	$$ = groups;
 }
 |
 group
@@ -206,12 +215,12 @@ PE_NAME '{' events '}'
 events:
 events ',' event
 {
+	struct list_head *events  = $1;
 	struct list_head *event = $3;
-	struct list_head *list  = $1;
 
-	/* frees $3 */
-	parse_events_update_lists(event, list);
-	$$ = list;
+	list_splice_tail(event, events);
+	free(event);
+	$$ = events;
 }
 |
 event
-- 
2.44.0.683.g7961c838ac-goog

[PATCH v2 11/16] perf parse-events: Improve error message for bad numbers

[Ian Rogers]

Use the error handler from the parse_state to give a more informative
error message.

Before:
```
$ perf stat -e 'cycles/period=99999999999999999999/' true
event syntax error: 'cycles/period=99999999999999999999/'
                                  \___ parser error
Run 'perf list' for a list of valid events

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

    -e, --event <event>   event selector. use 'perf list' to list available events
```

After:
```
$ perf stat -e 'cycles/period=99999999999999999999/' true
event syntax error: 'cycles/period=99999999999999999999/'
                                  \___ parser error

event syntax error: '..les/period=99999999999999999999/'
                                  \___ Bad base 10 number "99999999999999999999"
Run 'perf list' for a list of valid events

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

    -e, --event <event>   event selector. use 'perf list' to list available events
```

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/parse-events.l | 40 ++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 16 deletions(-)

diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index 6fe37003ab7b..0cd68c9f0d4f 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -18,26 +18,34 @@
 
 char *parse_events_get_text(yyscan_t yyscanner);
 YYSTYPE *parse_events_get_lval(yyscan_t yyscanner);
+int parse_events_get_column(yyscan_t yyscanner);
+int parse_events_get_leng(yyscan_t yyscanner);
 
-static int __value(YYSTYPE *yylval, char *str, int base, int token)
+static int get_column(yyscan_t scanner)
 {
-	u64 num;
-
-	errno = 0;
-	num = strtoull(str, NULL, base);
-	if (errno)
-		return PE_ERROR;
-
-	yylval->num = num;
-	return token;
+	return parse_events_get_column(scanner) - parse_events_get_leng(scanner);
 }
 
-static int value(yyscan_t scanner, int base)
+static int value(struct parse_events_state *parse_state, yyscan_t scanner, int base)
 {
 	YYSTYPE *yylval = parse_events_get_lval(scanner);
 	char *text = parse_events_get_text(scanner);
+	u64 num;
 
-	return __value(yylval, text, base, PE_VALUE);
+	errno = 0;
+	num = strtoull(text, NULL, base);
+	if (errno) {
+		struct parse_events_error *error = parse_state->error;
+		char *help = NULL;
+
+		if (asprintf(&help, "Bad base %d number \"%s\"", base, text) > 0)
+			parse_events_error__handle(error, get_column(scanner), help , NULL);
+
+		return PE_ERROR;
+	}
+
+	yylval->num = num;
+	return PE_VALUE;
 }
 
 static int str(yyscan_t scanner, int token)
@@ -283,8 +291,8 @@ r0x{num_raw_hex}	{ return str(yyscanner, PE_RAW); }
 	 */
 "/"/{digit}		{ return PE_BP_SLASH; }
 "/"/{non_digit}		{ BEGIN(config); return '/'; }
-{num_dec}		{ return value(yyscanner, 10); }
-{num_hex}		{ return value(yyscanner, 16); }
+{num_dec}		{ return value(_parse_state, yyscanner, 10); }
+{num_hex}		{ return value(_parse_state, yyscanner, 16); }
 	/*
 	 * We need to separate 'mem:' scanner part, in order to get specific
 	 * modifier bits parsed out. Otherwise we would need to handle PE_NAME
@@ -330,8 +338,8 @@ cgroup-switches					{ return sym(yyscanner, PERF_COUNT_SW_CGROUP_SWITCHES); }
 {lc_type}-{lc_op_result}-{lc_op_result}	{ return str(yyscanner, PE_LEGACY_CACHE); }
 mem:			{ BEGIN(mem); return PE_PREFIX_MEM; }
 r{num_raw_hex}		{ return str(yyscanner, PE_RAW); }
-{num_dec}		{ return value(yyscanner, 10); }
-{num_hex}		{ return value(yyscanner, 16); }
+{num_dec}		{ return value(_parse_state, yyscanner, 10); }
+{num_hex}		{ return value(_parse_state, yyscanner, 16); }
 
 {modifier_event}	{ return str(yyscanner, PE_MODIFIER_EVENT); }
 {name}			{ return str(yyscanner, PE_NAME); }
-- 
2.44.0.683.g7961c838ac-goog

Re: [PATCH v2 11/16] perf parse-events: Improve error message for bad numbers

[Liang, Kan]

On 2024-04-16 2:15 a.m., Ian Rogers wrote:
> Use the error handler from the parse_state to give a more informative
> error message.
> 
> Before:
> ```
> $ perf stat -e 'cycles/period=99999999999999999999/' true
> event syntax error: 'cycles/period=99999999999999999999/'
>                                   \___ parser error
> Run 'perf list' for a list of valid events
> 
>  Usage: perf stat [<options>] [<command>]
> 
>     -e, --event <event>   event selector. use 'perf list' to list available events
> ```
> 
> After:
> ```
> $ perf stat -e 'cycles/period=99999999999999999999/' true
> event syntax error: 'cycles/period=99999999999999999999/'
>                                   \___ parser error
> 
> event syntax error: '..les/period=99999999999999999999/'
>                                   \___ Bad base 10 number "99999999999999999999"


It seems the patch only works for decimal?

./perf stat -e 'cycles/period=0xaaaaaaaaaaaaaaaaaaaaaa/' true
event syntax error: '..les/period=0xaaaaaaaaaaaaaaaaaaaaaa/'
                                   \___ parser error
 Run 'perf list' for a list of valid events

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

     -e, --event <event>   event selector. use 'perf list' to list
available events

Thanks,
Kan

> Run 'perf list' for a list of valid events
> 
>  Usage: perf stat [<options>] [<command>]
> 
>     -e, --event <event>   event selector. use 'perf list' to list available events
> ```
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/parse-events.l | 40 ++++++++++++++++++++--------------
>  1 file changed, 24 insertions(+), 16 deletions(-)
> 
> diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
> index 6fe37003ab7b..0cd68c9f0d4f 100644
> --- a/tools/perf/util/parse-events.l
> +++ b/tools/perf/util/parse-events.l
> @@ -18,26 +18,34 @@
>  
>  char *parse_events_get_text(yyscan_t yyscanner);
>  YYSTYPE *parse_events_get_lval(yyscan_t yyscanner);
> +int parse_events_get_column(yyscan_t yyscanner);
> +int parse_events_get_leng(yyscan_t yyscanner);
>  
> -static int __value(YYSTYPE *yylval, char *str, int base, int token)
> +static int get_column(yyscan_t scanner)
>  {
> -	u64 num;
> -
> -	errno = 0;
> -	num = strtoull(str, NULL, base);
> -	if (errno)
> -		return PE_ERROR;
> -
> -	yylval->num = num;
> -	return token;
> +	return parse_events_get_column(scanner) - parse_events_get_leng(scanner);
>  }
>  
> -static int value(yyscan_t scanner, int base)
> +static int value(struct parse_events_state *parse_state, yyscan_t scanner, int base)
>  {
>  	YYSTYPE *yylval = parse_events_get_lval(scanner);
>  	char *text = parse_events_get_text(scanner);
> +	u64 num;
>  
> -	return __value(yylval, text, base, PE_VALUE);
> +	errno = 0;
> +	num = strtoull(text, NULL, base);
> +	if (errno) {
> +		struct parse_events_error *error = parse_state->error;
> +		char *help = NULL;
> +
> +		if (asprintf(&help, "Bad base %d number \"%s\"", base, text) > 0)
> +			parse_events_error__handle(error, get_column(scanner), help , NULL);
> +
> +		return PE_ERROR;
> +	}
> +
> +	yylval->num = num;
> +	return PE_VALUE;
>  }
>  
>  static int str(yyscan_t scanner, int token)
> @@ -283,8 +291,8 @@ r0x{num_raw_hex}	{ return str(yyscanner, PE_RAW); }
>  	 */
>  "/"/{digit}		{ return PE_BP_SLASH; }
>  "/"/{non_digit}		{ BEGIN(config); return '/'; }
> -{num_dec}		{ return value(yyscanner, 10); }
> -{num_hex}		{ return value(yyscanner, 16); }
> +{num_dec}		{ return value(_parse_state, yyscanner, 10); }
> +{num_hex}		{ return value(_parse_state, yyscanner, 16); }
>  	/*
>  	 * We need to separate 'mem:' scanner part, in order to get specific
>  	 * modifier bits parsed out. Otherwise we would need to handle PE_NAME
> @@ -330,8 +338,8 @@ cgroup-switches					{ return sym(yyscanner, PERF_COUNT_SW_CGROUP_SWITCHES); }
>  {lc_type}-{lc_op_result}-{lc_op_result}	{ return str(yyscanner, PE_LEGACY_CACHE); }
>  mem:			{ BEGIN(mem); return PE_PREFIX_MEM; }
>  r{num_raw_hex}		{ return str(yyscanner, PE_RAW); }
> -{num_dec}		{ return value(yyscanner, 10); }
> -{num_hex}		{ return value(yyscanner, 16); }
> +{num_dec}		{ return value(_parse_state, yyscanner, 10); }
> +{num_hex}		{ return value(_parse_state, yyscanner, 16); }
>  
>  {modifier_event}	{ return str(yyscanner, PE_MODIFIER_EVENT); }
>  {name}			{ return str(yyscanner, PE_NAME); }

Re: [PATCH v2 11/16] perf parse-events: Improve error message for bad numbers

[Ian Rogers]

On Thu, Apr 18, 2024 at 1:27 PM Liang, Kan <kan.liang@linux.intel.com> wrote:
>
>
>
> On 2024-04-16 2:15 a.m., Ian Rogers wrote:
> > Use the error handler from the parse_state to give a more informative
> > error message.
> >
> > Before:
> > ```
> > $ perf stat -e 'cycles/period=99999999999999999999/' true
> > event syntax error: 'cycles/period=99999999999999999999/'
> >                                   \___ parser error
> > Run 'perf list' for a list of valid events
> >
> >  Usage: perf stat [<options>] [<command>]
> >
> >     -e, --event <event>   event selector. use 'perf list' to list available events
> > ```
> >
> > After:
> > ```
> > $ perf stat -e 'cycles/period=99999999999999999999/' true
> > event syntax error: 'cycles/period=99999999999999999999/'
> >                                   \___ parser error
> >
> > event syntax error: '..les/period=99999999999999999999/'
> >                                   \___ Bad base 10 number "99999999999999999999"
>
>
> It seems the patch only works for decimal?
>
> ./perf stat -e 'cycles/period=0xaaaaaaaaaaaaaaaaaaaaaa/' true
> event syntax error: '..les/period=0xaaaaaaaaaaaaaaaaaaaaaa/'
>                                    \___ parser error
>  Run 'perf list' for a list of valid events
>
>   Usage: perf stat [<options>] [<command>]
>
>      -e, --event <event>   event selector. use 'perf list' to list
> available events
>
> Thanks,
> Kan

Right, for hexadecimal we say the number of digits is at most 16, so
when you exceed this the token is no longer recognized. It just
becomes input that can't be parsed, hence parser error. Doing this
means we can simplify other strtoull checks, but I agree having a
better error message for hexadecimal would be good. Let's do it as
follow up.

Thanks,
Ian

> > Run 'perf list' for a list of valid events
> >
> >  Usage: perf stat [<options>] [<command>]
> >
> >     -e, --event <event>   event selector. use 'perf list' to list available events
> > ```
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/parse-events.l | 40 ++++++++++++++++++++--------------
> >  1 file changed, 24 insertions(+), 16 deletions(-)
> >
> > diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
> > index 6fe37003ab7b..0cd68c9f0d4f 100644
> > --- a/tools/perf/util/parse-events.l
> > +++ b/tools/perf/util/parse-events.l
> > @@ -18,26 +18,34 @@
> >
> >  char *parse_events_get_text(yyscan_t yyscanner);
> >  YYSTYPE *parse_events_get_lval(yyscan_t yyscanner);
> > +int parse_events_get_column(yyscan_t yyscanner);
> > +int parse_events_get_leng(yyscan_t yyscanner);
> >
> > -static int __value(YYSTYPE *yylval, char *str, int base, int token)
> > +static int get_column(yyscan_t scanner)
> >  {
> > -     u64 num;
> > -
> > -     errno = 0;
> > -     num = strtoull(str, NULL, base);
> > -     if (errno)
> > -             return PE_ERROR;
> > -
> > -     yylval->num = num;
> > -     return token;
> > +     return parse_events_get_column(scanner) - parse_events_get_leng(scanner);
> >  }
> >
> > -static int value(yyscan_t scanner, int base)
> > +static int value(struct parse_events_state *parse_state, yyscan_t scanner, int base)
> >  {
> >       YYSTYPE *yylval = parse_events_get_lval(scanner);
> >       char *text = parse_events_get_text(scanner);
> > +     u64 num;
> >
> > -     return __value(yylval, text, base, PE_VALUE);
> > +     errno = 0;
> > +     num = strtoull(text, NULL, base);
> > +     if (errno) {
> > +             struct parse_events_error *error = parse_state->error;
> > +             char *help = NULL;
> > +
> > +             if (asprintf(&help, "Bad base %d number \"%s\"", base, text) > 0)
> > +                     parse_events_error__handle(error, get_column(scanner), help , NULL);
> > +
> > +             return PE_ERROR;
> > +     }
> > +
> > +     yylval->num = num;
> > +     return PE_VALUE;
> >  }
> >
> >  static int str(yyscan_t scanner, int token)
> > @@ -283,8 +291,8 @@ r0x{num_raw_hex}  { return str(yyscanner, PE_RAW); }
> >        */
> >  "/"/{digit}          { return PE_BP_SLASH; }
> >  "/"/{non_digit}              { BEGIN(config); return '/'; }
> > -{num_dec}            { return value(yyscanner, 10); }
> > -{num_hex}            { return value(yyscanner, 16); }
> > +{num_dec}            { return value(_parse_state, yyscanner, 10); }
> > +{num_hex}            { return value(_parse_state, yyscanner, 16); }
> >       /*
> >        * We need to separate 'mem:' scanner part, in order to get specific
> >        * modifier bits parsed out. Otherwise we would need to handle PE_NAME
> > @@ -330,8 +338,8 @@ cgroup-switches                                   { return sym(yyscanner, PERF_COUNT_SW_CGROUP_SWITCHES); }
> >  {lc_type}-{lc_op_result}-{lc_op_result}      { return str(yyscanner, PE_LEGACY_CACHE); }
> >  mem:                 { BEGIN(mem); return PE_PREFIX_MEM; }
> >  r{num_raw_hex}               { return str(yyscanner, PE_RAW); }
> > -{num_dec}            { return value(yyscanner, 10); }
> > -{num_hex}            { return value(yyscanner, 16); }
> > +{num_dec}            { return value(_parse_state, yyscanner, 10); }
> > +{num_hex}            { return value(_parse_state, yyscanner, 16); }
> >
> >  {modifier_event}     { return str(yyscanner, PE_MODIFIER_EVENT); }
> >  {name}                       { return str(yyscanner, PE_NAME); }

Re: [PATCH v2 11/16] perf parse-events: Improve error message for bad numbers

[Liang, Kan]

On 2024-04-18 5:07 p.m., Ian Rogers wrote:
> On Thu, Apr 18, 2024 at 1:27 PM Liang, Kan <kan.liang@linux.intel.com> wrote:
>>
>>
>>
>> On 2024-04-16 2:15 a.m., Ian Rogers wrote:
>>> Use the error handler from the parse_state to give a more informative
>>> error message.
>>>
>>> Before:
>>> ```
>>> $ perf stat -e 'cycles/period=99999999999999999999/' true
>>> event syntax error: 'cycles/period=99999999999999999999/'
>>>                                   \___ parser error
>>> Run 'perf list' for a list of valid events
>>>
>>>  Usage: perf stat [<options>] [<command>]
>>>
>>>     -e, --event <event>   event selector. use 'perf list' to list available events
>>> ```
>>>
>>> After:
>>> ```
>>> $ perf stat -e 'cycles/period=99999999999999999999/' true
>>> event syntax error: 'cycles/period=99999999999999999999/'
>>>                                   \___ parser error
>>>
>>> event syntax error: '..les/period=99999999999999999999/'
>>>                                   \___ Bad base 10 number "99999999999999999999"
>>
>>
>> It seems the patch only works for decimal?
>>
>> ./perf stat -e 'cycles/period=0xaaaaaaaaaaaaaaaaaaaaaa/' true
>> event syntax error: '..les/period=0xaaaaaaaaaaaaaaaaaaaaaa/'
>>                                    \___ parser error
>>  Run 'perf list' for a list of valid events
>>
>>   Usage: perf stat [<options>] [<command>]
>>
>>      -e, --event <event>   event selector. use 'perf list' to list
>> available events
>>
>> Thanks,
>> Kan
> 
> Right, for hexadecimal we say the number of digits is at most 16, so
> when you exceed this the token is no longer recognized. It just
> becomes input that can't be parsed, hence parser error. Doing this
> means we can simplify other strtoull checks, but I agree having a
> better error message for hexadecimal would be good. Let's do it as
> follow up.

OK. There is already a warning. It's fine to provide a follow-up for a
better error message later.

Thanks,
Kan

> 
> Thanks,
> Ian
> 
>>> Run 'perf list' for a list of valid events
>>>
>>>  Usage: perf stat [<options>] [<command>]
>>>
>>>     -e, --event <event>   event selector. use 'perf list' to list available events
>>> ```
>>>
>>> Signed-off-by: Ian Rogers <irogers@google.com>
>>> ---
>>>  tools/perf/util/parse-events.l | 40 ++++++++++++++++++++--------------
>>>  1 file changed, 24 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
>>> index 6fe37003ab7b..0cd68c9f0d4f 100644
>>> --- a/tools/perf/util/parse-events.l
>>> +++ b/tools/perf/util/parse-events.l
>>> @@ -18,26 +18,34 @@
>>>
>>>  char *parse_events_get_text(yyscan_t yyscanner);
>>>  YYSTYPE *parse_events_get_lval(yyscan_t yyscanner);
>>> +int parse_events_get_column(yyscan_t yyscanner);
>>> +int parse_events_get_leng(yyscan_t yyscanner);
>>>
>>> -static int __value(YYSTYPE *yylval, char *str, int base, int token)
>>> +static int get_column(yyscan_t scanner)
>>>  {
>>> -     u64 num;
>>> -
>>> -     errno = 0;
>>> -     num = strtoull(str, NULL, base);
>>> -     if (errno)
>>> -             return PE_ERROR;
>>> -
>>> -     yylval->num = num;
>>> -     return token;
>>> +     return parse_events_get_column(scanner) - parse_events_get_leng(scanner);
>>>  }
>>>
>>> -static int value(yyscan_t scanner, int base)
>>> +static int value(struct parse_events_state *parse_state, yyscan_t scanner, int base)
>>>  {
>>>       YYSTYPE *yylval = parse_events_get_lval(scanner);
>>>       char *text = parse_events_get_text(scanner);
>>> +     u64 num;
>>>
>>> -     return __value(yylval, text, base, PE_VALUE);
>>> +     errno = 0;
>>> +     num = strtoull(text, NULL, base);
>>> +     if (errno) {
>>> +             struct parse_events_error *error = parse_state->error;
>>> +             char *help = NULL;
>>> +
>>> +             if (asprintf(&help, "Bad base %d number \"%s\"", base, text) > 0)
>>> +                     parse_events_error__handle(error, get_column(scanner), help , NULL);
>>> +
>>> +             return PE_ERROR;
>>> +     }
>>> +
>>> +     yylval->num = num;
>>> +     return PE_VALUE;
>>>  }
>>>
>>>  static int str(yyscan_t scanner, int token)
>>> @@ -283,8 +291,8 @@ r0x{num_raw_hex}  { return str(yyscanner, PE_RAW); }
>>>        */
>>>  "/"/{digit}          { return PE_BP_SLASH; }
>>>  "/"/{non_digit}              { BEGIN(config); return '/'; }
>>> -{num_dec}            { return value(yyscanner, 10); }
>>> -{num_hex}            { return value(yyscanner, 16); }
>>> +{num_dec}            { return value(_parse_state, yyscanner, 10); }
>>> +{num_hex}            { return value(_parse_state, yyscanner, 16); }
>>>       /*
>>>        * We need to separate 'mem:' scanner part, in order to get specific
>>>        * modifier bits parsed out. Otherwise we would need to handle PE_NAME
>>> @@ -330,8 +338,8 @@ cgroup-switches                                   { return sym(yyscanner, PERF_COUNT_SW_CGROUP_SWITCHES); }
>>>  {lc_type}-{lc_op_result}-{lc_op_result}      { return str(yyscanner, PE_LEGACY_CACHE); }
>>>  mem:                 { BEGIN(mem); return PE_PREFIX_MEM; }
>>>  r{num_raw_hex}               { return str(yyscanner, PE_RAW); }
>>> -{num_dec}            { return value(yyscanner, 10); }
>>> -{num_hex}            { return value(yyscanner, 16); }
>>> +{num_dec}            { return value(_parse_state, yyscanner, 10); }
>>> +{num_hex}            { return value(_parse_state, yyscanner, 16); }
>>>
>>>  {modifier_event}     { return str(yyscanner, PE_MODIFIER_EVENT); }
>>>  {name}                       { return str(yyscanner, PE_NAME); }
> 

Re: [PATCH v2 11/16] perf parse-events: Improve error message for bad numbers

[Arnaldo Carvalho de Melo]

On Mon, Apr 15, 2024 at 11:15:27PM -0700, Ian Rogers wrote:
> Use the error handler from the parse_state to give a more informative
> error message.
> 
> Before:
> ```
> $ perf stat -e 'cycles/period=99999999999999999999/' true
> event syntax error: 'cycles/period=99999999999999999999/'
>                                   \___ parser error
> Run 'perf list' for a list of valid events
> 
>  Usage: perf stat [<options>] [<command>]
> 
>     -e, --event <event>   event selector. use 'perf list' to list available events
> ```
> 
> After:
> ```
> $ perf stat -e 'cycles/period=99999999999999999999/' true
> event syntax error: 'cycles/period=99999999999999999999/'
>                                   \___ parser error
> 

This ended up in perf-tools-next, will have to look at what this problem
is:

   9    11.46 amazonlinux:2                 : FAIL gcc version 7.3.1 20180712 (Red Hat 7.3.1-17) (GCC) 
     yy_size_t parse_events_get_leng (yyscan_t yyscanner );
               ^~~~~~~~~~~~~~~~~~~~~
    util/parse-events.l:22:5: note: previous declaration of 'parse_events_get_leng' was here
     int parse_events_get_leng(yyscan_t yyscanner);
         ^~~~~~~~~~~~~~~~~~~~~
     yy_size_t parse_events_get_leng  (yyscan_t yyscanner)
               ^~~~~~~~~~~~~~~~~~~~~
    util/parse-events.l:22:5: note: previous declaration of 'parse_events_get_leng' was here
     int parse_events_get_leng(yyscan_t yyscanner);
         ^~~~~~~~~~~~~~~~~~~~~
    make[3]: *** [util] Error 2


Unsure if this will appear on the radar on other distros, maybe this is
just something that pops up with older distros...

Ran out of time today...

- Arnaldo

Re: [PATCH v2 11/16] perf parse-events: Improve error message for bad numbers

[Arnaldo Carvalho de Melo]

On Fri, Apr 26, 2024 at 10:36:02PM -0300, Arnaldo Carvalho de Melo wrote:
> On Mon, Apr 15, 2024 at 11:15:27PM -0700, Ian Rogers wrote:
> > Use the error handler from the parse_state to give a more informative
> > error message.
> > 
> > Before:
> > ```
> > $ perf stat -e 'cycles/period=99999999999999999999/' true
> > event syntax error: 'cycles/period=99999999999999999999/'
> >                                   \___ parser error
> > Run 'perf list' for a list of valid events
> > 
> >  Usage: perf stat [<options>] [<command>]
> > 
> >     -e, --event <event>   event selector. use 'perf list' to list available events
> > ```
> > 
> > After:
> > ```
> > $ perf stat -e 'cycles/period=99999999999999999999/' true
> > event syntax error: 'cycles/period=99999999999999999999/'
> >                                   \___ parser error
> > 
> 
> This ended up in perf-tools-next, will have to look at what this problem
> is:
> 
>    9    11.46 amazonlinux:2                 : FAIL gcc version 7.3.1 20180712 (Red Hat 7.3.1-17) (GCC) 
>      yy_size_t parse_events_get_leng (yyscan_t yyscanner );
>                ^~~~~~~~~~~~~~~~~~~~~
>     util/parse-events.l:22:5: note: previous declaration of 'parse_events_get_leng' was here
>      int parse_events_get_leng(yyscan_t yyscanner);
>          ^~~~~~~~~~~~~~~~~~~~~
>      yy_size_t parse_events_get_leng  (yyscan_t yyscanner)
>                ^~~~~~~~~~~~~~~~~~~~~
>     util/parse-events.l:22:5: note: previous declaration of 'parse_events_get_leng' was here
>      int parse_events_get_leng(yyscan_t yyscanner);
>          ^~~~~~~~~~~~~~~~~~~~~
>     make[3]: *** [util] Error 2
> 
> 
> Unsure if this will appear on the radar on other distros, maybe this is
> just something that pops up with older distros...
> 
> Ran out of time today...

Context:

perfbuilder@number:~$ export BUILD_TARBALL=http://192.168.86.42/perf/perf-6.9.0-rc5.tar.xz
perfbuilder@number:~$ time dm
   1   102.33 almalinux:8                   : Ok   gcc (GCC) 8.5.0 20210514 (Red Hat 8.5.0-20) , clang version 16.0.6 (Red Hat 16.0.6-2.module_el8.9.0+3621+df7f7146) flex 2.6.1
   2   102.44 almalinux:9                   : Ok   gcc (GCC) 11.4.1 20230605 (Red Hat 11.4.1-2) , clang version 16.0.6 (Red Hat 16.0.6-1.el9) flex 2.6.4
   3   124.34 alpine:3.15                   : Ok   gcc (Alpine 10.3.1_git20211027) 10.3.1 20211027 , Alpine clang version 12.0.1 flex 2.6.4
   4   109.42 alpine:3.16                   : Ok   gcc (Alpine 11.2.1_git20220219) 11.2.1 20220219 , Alpine clang version 13.0.1 flex 2.6.4
   5    90.08 alpine:3.17                   : Ok   gcc (Alpine 12.2.1_git20220924-r4) 12.2.1 20220924 , Alpine clang version 15.0.7 flex 2.6.4
   6    84.85 alpine:3.18                   : Ok   gcc (Alpine 12.2.1_git20220924-r10) 12.2.1 20220924 , Alpine clang version 16.0.6 flex 2.6.4
   7    94.18 alpine:3.19                   : Ok   gcc (Alpine 13.2.1_git20231014) 13.2.1 20231014 , Alpine clang version 17.0.5 flex 2.6.4
   8    95.45 alpine:edge                   : Ok   gcc (Alpine 13.2.1_git20240309) 13.2.1 20240309 , Alpine clang version 17.0.6 flex 2.6.4
   9    11.46 amazonlinux:2                 : FAIL gcc version 7.3.1 20180712 (Red Hat 7.3.1-17) (GCC) 
     yy_size_t parse_events_get_leng (yyscan_t yyscanner );
               ^~~~~~~~~~~~~~~~~~~~~
    util/parse-events.l:22:5: note: previous declaration of 'parse_events_get_leng' was here
     int parse_events_get_leng(yyscan_t yyscanner);
         ^~~~~~~~~~~~~~~~~~~~~
     yy_size_t parse_events_get_leng  (yyscan_t yyscanner)
               ^~~~~~~~~~~~~~~~~~~~~
    util/parse-events.l:22:5: note: previous declaration of 'parse_events_get_leng' was here
     int parse_events_get_leng(yyscan_t yyscanner);
         ^~~~~~~~~~~~~~~~~~~~~
    make[3]: *** [util] Error 2
  10    88.41 amazonlinux:2023              : Ok   gcc (GCC) 11.4.1 20230605 (Red Hat 11.4.1-2) , clang version 15.0.7 (Amazon Linux 15.0.7-3.amzn2023.0.1) flex 2.6.4
  11    89.72 amazonlinux:devel             : Ok   gcc (GCC) 11.3.1 20221121 (Red Hat 11.3.1-4) , clang version 15.0.6 (Amazon Linux 15.0.6-3.amzn2023.0.2) flex 2.6.4
  12   115.65 archlinux:base                : Ok   gcc (GCC) 13.2.1 20230801 , clang version 17.0.6 flex 2.6.4
  13    93.87 centos:stream                 : Ok   gcc (GCC) 8.5.0 20210514 (Red Hat 8.5.0-21) , clang version 17.0.6 (Red Hat 17.0.6-1.module_el8+767+9fa966b8) flex 2.6.1

Re: [PATCH v2 11/16] perf parse-events: Improve error message for bad numbers

[Ian Rogers]

On Fri, Apr 26, 2024 at 6:36 PM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> On Fri, Apr 26, 2024 at 10:36:02PM -0300, Arnaldo Carvalho de Melo wrote:
> > On Mon, Apr 15, 2024 at 11:15:27PM -0700, Ian Rogers wrote:
> > > Use the error handler from the parse_state to give a more informative
> > > error message.
> > >
> > > Before:
> > > ```
> > > $ perf stat -e 'cycles/period=99999999999999999999/' true
> > > event syntax error: 'cycles/period=99999999999999999999/'
> > >                                   \___ parser error
> > > Run 'perf list' for a list of valid events
> > >
> > >  Usage: perf stat [<options>] [<command>]
> > >
> > >     -e, --event <event>   event selector. use 'perf list' to list available events
> > > ```
> > >
> > > After:
> > > ```
> > > $ perf stat -e 'cycles/period=99999999999999999999/' true
> > > event syntax error: 'cycles/period=99999999999999999999/'
> > >                                   \___ parser error
> > >
> >
> > This ended up in perf-tools-next, will have to look at what this problem
> > is:
> >
> >    9    11.46 amazonlinux:2                 : FAIL gcc version 7.3.1 20180712 (Red Hat 7.3.1-17) (GCC)
> >      yy_size_t parse_events_get_leng (yyscan_t yyscanner );
> >                ^~~~~~~~~~~~~~~~~~~~~
> >     util/parse-events.l:22:5: note: previous declaration of 'parse_events_get_leng' was here
> >      int parse_events_get_leng(yyscan_t yyscanner);
> >          ^~~~~~~~~~~~~~~~~~~~~
> >      yy_size_t parse_events_get_leng  (yyscan_t yyscanner)
> >                ^~~~~~~~~~~~~~~~~~~~~
> >     util/parse-events.l:22:5: note: previous declaration of 'parse_events_get_leng' was here
> >      int parse_events_get_leng(yyscan_t yyscanner);
> >          ^~~~~~~~~~~~~~~~~~~~~
> >     make[3]: *** [util] Error 2
> >
> >
> > Unsure if this will appear on the radar on other distros, maybe this is
> > just something that pops up with older distros...
> >
> > Ran out of time today...
>
> Context:
>
> perfbuilder@number:~$ export BUILD_TARBALL=http://192.168.86.42/perf/perf-6.9.0-rc5.tar.xz
> perfbuilder@number:~$ time dm
>    1   102.33 almalinux:8                   : Ok   gcc (GCC) 8.5.0 20210514 (Red Hat 8.5.0-20) , clang version 16.0.6 (Red Hat 16.0.6-2.module_el8.9.0+3621+df7f7146) flex 2.6.1
>    2   102.44 almalinux:9                   : Ok   gcc (GCC) 11.4.1 20230605 (Red Hat 11.4.1-2) , clang version 16.0.6 (Red Hat 16.0.6-1.el9) flex 2.6.4
>    3   124.34 alpine:3.15                   : Ok   gcc (Alpine 10.3.1_git20211027) 10.3.1 20211027 , Alpine clang version 12.0.1 flex 2.6.4
>    4   109.42 alpine:3.16                   : Ok   gcc (Alpine 11.2.1_git20220219) 11.2.1 20220219 , Alpine clang version 13.0.1 flex 2.6.4
>    5    90.08 alpine:3.17                   : Ok   gcc (Alpine 12.2.1_git20220924-r4) 12.2.1 20220924 , Alpine clang version 15.0.7 flex 2.6.4
>    6    84.85 alpine:3.18                   : Ok   gcc (Alpine 12.2.1_git20220924-r10) 12.2.1 20220924 , Alpine clang version 16.0.6 flex 2.6.4
>    7    94.18 alpine:3.19                   : Ok   gcc (Alpine 13.2.1_git20231014) 13.2.1 20231014 , Alpine clang version 17.0.5 flex 2.6.4
>    8    95.45 alpine:edge                   : Ok   gcc (Alpine 13.2.1_git20240309) 13.2.1 20240309 , Alpine clang version 17.0.6 flex 2.6.4
>    9    11.46 amazonlinux:2                 : FAIL gcc version 7.3.1 20180712 (Red Hat 7.3.1-17) (GCC)
>      yy_size_t parse_events_get_leng (yyscan_t yyscanner );
>                ^~~~~~~~~~~~~~~~~~~~~
>     util/parse-events.l:22:5: note: previous declaration of 'parse_events_get_leng' was here
>      int parse_events_get_leng(yyscan_t yyscanner);
>          ^~~~~~~~~~~~~~~~~~~~~
>      yy_size_t parse_events_get_leng  (yyscan_t yyscanner)
>                ^~~~~~~~~~~~~~~~~~~~~
>     util/parse-events.l:22:5: note: previous declaration of 'parse_events_get_leng' was here
>      int parse_events_get_leng(yyscan_t yyscanner);
>          ^~~~~~~~~~~~~~~~~~~~~
>     make[3]: *** [util] Error 2
>   10    88.41 amazonlinux:2023              : Ok   gcc (GCC) 11.4.1 20230605 (Red Hat 11.4.1-2) , clang version 15.0.7 (Amazon Linux 15.0.7-3.amzn2023.0.1) flex 2.6.4
>   11    89.72 amazonlinux:devel             : Ok   gcc (GCC) 11.3.1 20221121 (Red Hat 11.3.1-4) , clang version 15.0.6 (Amazon Linux 15.0.6-3.amzn2023.0.2) flex 2.6.4
>   12   115.65 archlinux:base                : Ok   gcc (GCC) 13.2.1 20230801 , clang version 17.0.6 flex 2.6.4
>   13    93.87 centos:stream                 : Ok   gcc (GCC) 8.5.0 20210514 (Red Hat 8.5.0-21) , clang version 17.0.6 (Red Hat 17.0.6-1.module_el8+767+9fa966b8) flex 2.6.1

Did RedHat do value add on flex output? yyget_leng is documented to
have an int return type:
https://github.com/westes/flex/blob/master/doc/flex.texi#L4613
This patch just adds a forward declaration in order to use it in a
helper function.

Thanks,
Ian

[PATCH v2 12/16] perf parse-events: Inline parse_events_evlist_error

[Ian Rogers]

Inline parse_events_evlist_error that is only used in
parse_events_error. Modify parse_events_error to not report a parser
error unless errors haven't already been reported. Make it clearer
that the latter case only happens for unrecognized input.

Before:
```
$ perf stat -e 'cycles/period=99999999999999999999/' true
event syntax error: 'cycles/period=99999999999999999999/'
                                  \___ parser error

event syntax error: '..les/period=99999999999999999999/'
                                  \___ Bad base 10 number "99999999999999999999"
Run 'perf list' for a list of valid events

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

    -e, --event <event>   event selector. use 'perf list' to list available events
$ perf stat -e 'cycles:xyz' true
event syntax error: 'cycles:xyz'
                           \___ parser error
Run 'perf list' for a list of valid events

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

    -e, --event <event>   event selector. use 'perf list' to list available events
```

After:
```
$ perf stat -e 'cycles/period=99999999999999999999/xyz' true
event syntax error: '..les/period=99999999999999999999/xyz'
                                  \___ Bad base 10 number "99999999999999999999"
Run 'perf list' for a list of valid events

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

    -e, --event <event>   event selector. use 'perf list' to list available events
$ perf stat -e 'cycles:xyz' true
event syntax error: 'cycles:xyz'
                           \___ Unrecognized input
Run 'perf list' for a list of valid events

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

    -e, --event <event>   event selector. use 'perf list' to list available events
```

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/parse-events.c |  9 ---------
 tools/perf/util/parse-events.h |  2 --
 tools/perf/util/parse-events.y | 10 ++++++++--
 3 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 7eba714f0d73..ebada37ef98a 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -2760,15 +2760,6 @@ int parse_events_terms__to_strbuf(const struct parse_events_terms *terms, struct
 	return 0;
 }
 
-void parse_events_evlist_error(struct parse_events_state *parse_state,
-			       int idx, const char *str)
-{
-	if (!parse_state->error)
-		return;
-
-	parse_events_error__handle(parse_state->error, idx, strdup(str), NULL);
-}
-
 static void config_terms_list(char *buf, size_t buf_sz)
 {
 	int i;
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index e8f2aebea10f..290ae6c72ec5 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -226,8 +226,6 @@ int parse_events_multi_pmu_add_or_add_pmu(struct parse_events_state *parse_state
 					void *loc_);
 
 void parse_events__set_leader(char *name, struct list_head *list);
-void parse_events_evlist_error(struct parse_events_state *parse_state,
-			       int idx, const char *str);
 
 struct event_symbol {
 	const char	*symbol;
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 51490f0f8c50..2c4817e126c1 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -790,9 +790,15 @@ sep_slash_slash_dc: '/' '/' | ':' |
 
 %%
 
-void parse_events_error(YYLTYPE *loc, void *parse_state,
+void parse_events_error(YYLTYPE *loc, void *_parse_state,
 			void *scanner __maybe_unused,
 			char const *msg __maybe_unused)
 {
-	parse_events_evlist_error(parse_state, loc->last_column, "parser error");
+	struct parse_events_state *parse_state = _parse_state;
+
+	if (!parse_state->error || !list_empty(&parse_state->error->list))
+		return;
+
+	parse_events_error__handle(parse_state->error, loc->last_column,
+				   strdup("Unrecognized input"), NULL);
 }
-- 
2.44.0.683.g7961c838ac-goog

[PATCH v2 13/16] perf parse-events: Improvements to modifier parsing

[Ian Rogers]

Use a struct/bitmap rather than a copied string from lexer.

In lexer give improved error message when too many precise flags are
given or repeated modifiers.

Before:
```
$ perf stat -e 'cycles:kuk' true
event syntax error: 'cycles:kuk'
                            \___ Bad modifier
...
$ perf stat -e 'cycles:pppp' true
event syntax error: 'cycles:pppp'
                            \___ Bad modifier
...
$ perf stat -e '{instructions:p,cycles:pp}:pp' -a true
event syntax error: '..cycles:pp}:pp'
                                  \___ Bad modifier
...
```
After:
```
$ perf stat -e 'cycles:kuk' true
event syntax error: 'cycles:kuk'
                              \___ Duplicate modifier 'k' (kernel)
...
$ perf stat -e 'cycles:pppp' true
event syntax error: 'cycles:pppp'
                               \___ Maximum precise value is 3
...
$ perf stat -e '{instructions:p,cycles:pp}:pp' true
event syntax error: '..cycles:pp}:pp'
                                  \___ Maximum combined precise value is 3, adding precision to "cycles:pp"
...
```

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/parse-events.c | 250 ++++++++++++---------------------
 tools/perf/util/parse-events.h |  23 ++-
 tools/perf/util/parse-events.l |  75 +++++++++-
 tools/perf/util/parse-events.y |  28 +---
 4 files changed, 194 insertions(+), 182 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index ebada37ef98a..3ab533d0e653 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1700,12 +1700,6 @@ int parse_events_multi_pmu_add_or_add_pmu(struct parse_events_state *parse_state
 	return -EINVAL;
 }
 
-int parse_events__modifier_group(struct list_head *list,
-				 char *event_mod)
-{
-	return parse_events__modifier_event(list, event_mod, true);
-}
-
 void parse_events__set_leader(char *name, struct list_head *list)
 {
 	struct evsel *leader;
@@ -1720,183 +1714,125 @@ void parse_events__set_leader(char *name, struct list_head *list)
 	leader->group_name = name;
 }
 
-struct event_modifier {
-	int eu;
-	int ek;
-	int eh;
-	int eH;
-	int eG;
-	int eI;
-	int precise;
-	int precise_max;
-	int exclude_GH;
-	int sample_read;
-	int pinned;
-	int weak;
-	int exclusive;
-	int bpf_counter;
-};
+static int parse_events__modifier_list(struct parse_events_state *parse_state,
+				       YYLTYPE *loc,
+				       struct list_head *list,
+				       struct parse_events_modifier mod,
+				       bool group)
+{
+	struct evsel *evsel;
 
-static int get_event_modifier(struct event_modifier *mod, char *str,
-			       struct evsel *evsel)
-{
-	int eu = evsel ? evsel->core.attr.exclude_user : 0;
-	int ek = evsel ? evsel->core.attr.exclude_kernel : 0;
-	int eh = evsel ? evsel->core.attr.exclude_hv : 0;
-	int eH = evsel ? evsel->core.attr.exclude_host : 0;
-	int eG = evsel ? evsel->core.attr.exclude_guest : 0;
-	int eI = evsel ? evsel->core.attr.exclude_idle : 0;
-	int precise = evsel ? evsel->core.attr.precise_ip : 0;
-	int precise_max = 0;
-	int sample_read = 0;
-	int pinned = evsel ? evsel->core.attr.pinned : 0;
-	int exclusive = evsel ? evsel->core.attr.exclusive : 0;
-
-	int exclude = eu | ek | eh;
-	int exclude_GH = evsel ? evsel->exclude_GH : 0;
-	int weak = 0;
-	int bpf_counter = 0;
-
-	memset(mod, 0, sizeof(*mod));
-
-	while (*str) {
-		if (*str == 'u') {
+	if (!group && mod.weak) {
+		parse_events_error__handle(parse_state->error, loc->first_column,
+					   strdup("Weak modifier is for use with groups"), NULL);
+		return -EINVAL;
+	}
+
+	__evlist__for_each_entry(list, evsel) {
+		/* Translate modifiers into the equivalent evsel excludes. */
+		int eu = group ? evsel->core.attr.exclude_user : 0;
+		int ek = group ? evsel->core.attr.exclude_kernel : 0;
+		int eh = group ? evsel->core.attr.exclude_hv : 0;
+		int eH = group ? evsel->core.attr.exclude_host : 0;
+		int eG = group ? evsel->core.attr.exclude_guest : 0;
+		int exclude = eu | ek | eh;
+		int exclude_GH = group ? evsel->exclude_GH : 0;
+
+		if (mod.precise) {
+			/* use of precise requires exclude_guest */
+			eG = 1;
+		}
+		if (mod.user) {
 			if (!exclude)
 				exclude = eu = ek = eh = 1;
 			if (!exclude_GH && !perf_guest)
 				eG = 1;
 			eu = 0;
-		} else if (*str == 'k') {
+		}
+		if (mod.kernel) {
 			if (!exclude)
 				exclude = eu = ek = eh = 1;
 			ek = 0;
-		} else if (*str == 'h') {
+		}
+		if (mod.hypervisor) {
 			if (!exclude)
 				exclude = eu = ek = eh = 1;
 			eh = 0;
-		} else if (*str == 'G') {
+		}
+		if (mod.guest) {
 			if (!exclude_GH)
 				exclude_GH = eG = eH = 1;
 			eG = 0;
-		} else if (*str == 'H') {
+		}
+		if (mod.host) {
 			if (!exclude_GH)
 				exclude_GH = eG = eH = 1;
 			eH = 0;
-		} else if (*str == 'I') {
-			eI = 1;
-		} else if (*str == 'p') {
-			precise++;
-			/* use of precise requires exclude_guest */
-			if (!exclude_GH)
-				eG = 1;
-		} else if (*str == 'P') {
-			precise_max = 1;
-		} else if (*str == 'S') {
-			sample_read = 1;
-		} else if (*str == 'D') {
-			pinned = 1;
-		} else if (*str == 'e') {
-			exclusive = 1;
-		} else if (*str == 'W') {
-			weak = 1;
-		} else if (*str == 'b') {
-			bpf_counter = 1;
-		} else
-			break;
-
-		++str;
+		}
+		evsel->core.attr.exclude_user   = eu;
+		evsel->core.attr.exclude_kernel = ek;
+		evsel->core.attr.exclude_hv     = eh;
+		evsel->core.attr.exclude_host   = eH;
+		evsel->core.attr.exclude_guest  = eG;
+		evsel->exclude_GH               = exclude_GH;
+
+		/* Simple modifiers copied to the evsel. */
+		if (mod.precise) {
+			u8 precise = evsel->core.attr.precise_ip + mod.precise;
+			/*
+			 * precise ip:
+			 *
+			 *  0 - SAMPLE_IP can have arbitrary skid
+			 *  1 - SAMPLE_IP must have constant skid
+			 *  2 - SAMPLE_IP requested to have 0 skid
+			 *  3 - SAMPLE_IP must have 0 skid
+			 *
+			 *  See also PERF_RECORD_MISC_EXACT_IP
+			 */
+			if (precise > 3) {
+				char *help;
+
+				if (asprintf(&help,
+					     "Maximum combined precise value is 3, adding precision to \"%s\"",
+					     evsel__name(evsel)) > 0) {
+					parse_events_error__handle(parse_state->error,
+								   loc->first_column,
+								   help, NULL);
+				}
+				return -EINVAL;
+			}
+			evsel->core.attr.precise_ip = precise;
+		}
+		if (mod.precise_max)
+			evsel->precise_max = 1;
+		if (mod.non_idle)
+			evsel->core.attr.exclude_idle = 1;
+		if (mod.sample_read)
+			evsel->sample_read = 1;
+		if (mod.pinned && evsel__is_group_leader(evsel))
+			evsel->core.attr.pinned = 1;
+		if (mod.exclusive && evsel__is_group_leader(evsel))
+			evsel->core.attr.exclusive = 1;
+		if (mod.weak)
+			evsel->weak_group = true;
+		if (mod.bpf)
+			evsel->bpf_counter = true;
 	}
-
-	/*
-	 * precise ip:
-	 *
-	 *  0 - SAMPLE_IP can have arbitrary skid
-	 *  1 - SAMPLE_IP must have constant skid
-	 *  2 - SAMPLE_IP requested to have 0 skid
-	 *  3 - SAMPLE_IP must have 0 skid
-	 *
-	 *  See also PERF_RECORD_MISC_EXACT_IP
-	 */
-	if (precise > 3)
-		return -EINVAL;
-
-	mod->eu = eu;
-	mod->ek = ek;
-	mod->eh = eh;
-	mod->eH = eH;
-	mod->eG = eG;
-	mod->eI = eI;
-	mod->precise = precise;
-	mod->precise_max = precise_max;
-	mod->exclude_GH = exclude_GH;
-	mod->sample_read = sample_read;
-	mod->pinned = pinned;
-	mod->weak = weak;
-	mod->bpf_counter = bpf_counter;
-	mod->exclusive = exclusive;
-
 	return 0;
 }
 
-/*
- * Basic modifier sanity check to validate it contains only one
- * instance of any modifier (apart from 'p') present.
- */
-static int check_modifier(char *str)
+int parse_events__modifier_group(struct parse_events_state *parse_state, void *loc,
+				 struct list_head *list,
+				 struct parse_events_modifier mod)
 {
-	char *p = str;
-
-	/* The sizeof includes 0 byte as well. */
-	if (strlen(str) > (sizeof("ukhGHpppPSDIWeb") - 1))
-		return -1;
-
-	while (*p) {
-		if (*p != 'p' && strchr(p + 1, *p))
-			return -1;
-		p++;
-	}
-
-	return 0;
+	return parse_events__modifier_list(parse_state, loc, list, mod, /*group=*/true);
 }
 
-int parse_events__modifier_event(struct list_head *list, char *str, bool add)
+int parse_events__modifier_event(struct parse_events_state *parse_state, void *loc,
+				 struct list_head *list,
+				 struct parse_events_modifier mod)
 {
-	struct evsel *evsel;
-	struct event_modifier mod;
-
-	if (str == NULL)
-		return 0;
-
-	if (check_modifier(str))
-		return -EINVAL;
-
-	if (!add && get_event_modifier(&mod, str, NULL))
-		return -EINVAL;
-
-	__evlist__for_each_entry(list, evsel) {
-		if (add && get_event_modifier(&mod, str, evsel))
-			return -EINVAL;
-
-		evsel->core.attr.exclude_user   = mod.eu;
-		evsel->core.attr.exclude_kernel = mod.ek;
-		evsel->core.attr.exclude_hv     = mod.eh;
-		evsel->core.attr.precise_ip     = mod.precise;
-		evsel->core.attr.exclude_host   = mod.eH;
-		evsel->core.attr.exclude_guest  = mod.eG;
-		evsel->core.attr.exclude_idle   = mod.eI;
-		evsel->exclude_GH          = mod.exclude_GH;
-		evsel->sample_read         = mod.sample_read;
-		evsel->precise_max         = mod.precise_max;
-		evsel->weak_group	   = mod.weak;
-		evsel->bpf_counter	   = mod.bpf_counter;
-
-		if (evsel__is_group_leader(evsel)) {
-			evsel->core.attr.pinned = mod.pinned;
-			evsel->core.attr.exclusive = mod.exclusive;
-		}
-	}
-
-	return 0;
+	return parse_events__modifier_list(parse_state, loc, list, mod, /*group=*/false);
 }
 
 int parse_events_name(struct list_head *list, const char *name)
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index 290ae6c72ec5..f104faef1a78 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -186,8 +186,27 @@ void parse_events_terms__init(struct parse_events_terms *terms);
 void parse_events_terms__exit(struct parse_events_terms *terms);
 int parse_events_terms(struct parse_events_terms *terms, const char *str, FILE *input);
 int parse_events_terms__to_strbuf(const struct parse_events_terms *terms, struct strbuf *sb);
-int parse_events__modifier_event(struct list_head *list, char *str, bool add);
-int parse_events__modifier_group(struct list_head *list, char *event_mod);
+
+struct parse_events_modifier {
+	u8 precise;	/* Number of repeated 'p' for precision. */
+	bool precise_max : 1;	/* 'P' */
+	bool non_idle : 1;	/* 'I' */
+	bool sample_read : 1;	/* 'S' */
+	bool pinned : 1;	/* 'D' */
+	bool exclusive : 1;	/* 'e' */
+	bool weak : 1;		/* 'W' */
+	bool bpf : 1;		/* 'b' */
+	bool user : 1;		/* 'u' */
+	bool kernel : 1;	/* 'k' */
+	bool hypervisor : 1;	/* 'h' */
+	bool guest : 1;		/* 'G' */
+	bool host : 1;		/* 'H' */
+};
+
+int parse_events__modifier_event(struct parse_events_state *parse_state, void *loc,
+				 struct list_head *list, struct parse_events_modifier mod);
+int parse_events__modifier_group(struct parse_events_state *parse_state, void *loc,
+				 struct list_head *list, struct parse_events_modifier mod);
 int parse_events_name(struct list_head *list, const char *name);
 int parse_events_add_tracepoint(struct list_head *list, int *idx,
 				const char *sys, const char *event,
diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index 0cd68c9f0d4f..4aaf0c53d9b6 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -142,6 +142,77 @@ static int hw(yyscan_t scanner, int config)
 	return PE_TERM_HW;
 }
 
+static void modifiers_error(struct parse_events_state *parse_state, yyscan_t scanner,
+			    int pos, char mod_char, const char *mod_name)
+{
+	struct parse_events_error *error = parse_state->error;
+	char *help = NULL;
+
+	if (asprintf(&help, "Duplicate modifier '%c' (%s)", mod_char, mod_name) > 0)
+		parse_events_error__handle(error, get_column(scanner) + pos, help , NULL);
+}
+
+static int modifiers(struct parse_events_state *parse_state, yyscan_t scanner)
+{
+	YYSTYPE *yylval = parse_events_get_lval(scanner);
+	char *text = parse_events_get_text(scanner);
+	struct parse_events_modifier mod = { .precise = 0, };
+
+	for (size_t i = 0, n = strlen(text); i < n; i++) {
+#define CASE(c, field)							\
+		case c:							\
+			if (mod.field) {				\
+				modifiers_error(parse_state, scanner, i, c, #field); \
+				return PE_ERROR;			\
+			}						\
+			mod.field = true;				\
+			break
+
+		switch (text[i]) {
+		CASE('u', user);
+		CASE('k', kernel);
+		CASE('h', hypervisor);
+		CASE('I', non_idle);
+		CASE('G', guest);
+		CASE('H', host);
+		case 'p':
+			mod.precise++;
+			/*
+			 * precise ip:
+			 *
+			 *  0 - SAMPLE_IP can have arbitrary skid
+			 *  1 - SAMPLE_IP must have constant skid
+			 *  2 - SAMPLE_IP requested to have 0 skid
+			 *  3 - SAMPLE_IP must have 0 skid
+			 *
+			 *  See also PERF_RECORD_MISC_EXACT_IP
+			 */
+			if (mod.precise > 3) {
+				struct parse_events_error *error = parse_state->error;
+				char *help = strdup("Maximum precise value is 3");
+
+				if (help) {
+					parse_events_error__handle(error, get_column(scanner) + i,
+								   help , NULL);
+				}
+				return PE_ERROR;
+			}
+			break;
+		CASE('P', precise_max);
+		CASE('S', sample_read);
+		CASE('D', pinned);
+		CASE('W', weak);
+		CASE('e', exclusive);
+		CASE('b', bpf);
+		default:
+			return PE_ERROR;
+		}
+#undef CASE
+	}
+	yylval->mod = mod;
+	return PE_MODIFIER_EVENT;
+}
+
 #define YY_USER_ACTION					\
 do {							\
 	yylloc->last_column  = yylloc->first_column;	\
@@ -174,7 +245,7 @@ drv_cfg_term	[a-zA-Z0-9_\.]+(=[a-zA-Z0-9_*?\.:]+)?
  * If you add a modifier you need to update check_modifier().
  * Also, the letters in modifier_event must not be in modifier_bp.
  */
-modifier_event	[ukhpPGHSDIWeb]+
+modifier_event	[ukhpPGHSDIWeb]{1,15}
 modifier_bp	[rwx]{1,3}
 lc_type 	(L1-dcache|l1-d|l1d|L1-data|L1-icache|l1-i|l1i|L1-instruction|LLC|L2|dTLB|d-tlb|Data-TLB|iTLB|i-tlb|Instruction-TLB|branch|branches|bpu|btb|bpc|node)
 lc_op_result	(load|loads|read|store|stores|write|prefetch|prefetches|speculative-read|speculative-load|refs|Reference|ops|access|misses|miss)
@@ -341,7 +412,7 @@ r{num_raw_hex}		{ return str(yyscanner, PE_RAW); }
 {num_dec}		{ return value(_parse_state, yyscanner, 10); }
 {num_hex}		{ return value(_parse_state, yyscanner, 16); }
 
-{modifier_event}	{ return str(yyscanner, PE_MODIFIER_EVENT); }
+{modifier_event}	{ return modifiers(_parse_state, yyscanner); }
 {name}			{ return str(yyscanner, PE_NAME); }
 {name_tag}		{ return str(yyscanner, PE_NAME); }
 "/"			{ BEGIN(config); return '/'; }
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 2c4817e126c1..79f254189be6 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -68,11 +68,11 @@ static void free_list_evsel(struct list_head* list_evsel)
 %type <num> PE_VALUE
 %type <num> PE_VALUE_SYM_SW
 %type <num> PE_VALUE_SYM_TOOL
+%type <mod> PE_MODIFIER_EVENT
 %type <term_type> PE_TERM
 %type <str> PE_RAW
 %type <str> PE_NAME
 %type <str> PE_LEGACY_CACHE
-%type <str> PE_MODIFIER_EVENT
 %type <str> PE_MODIFIER_BP
 %type <str> PE_EVENT_NAME
 %type <str> PE_DRV_CFG_TERM
@@ -110,6 +110,7 @@ static void free_list_evsel(struct list_head* list_evsel)
 {
 	char *str;
 	u64 num;
+	struct parse_events_modifier mod;
 	enum parse_events__term_type term_type;
 	struct list_head *list_evsel;
 	struct parse_events_terms *list_terms;
@@ -175,20 +176,13 @@ event
 group:
 group_def ':' PE_MODIFIER_EVENT
 {
+	/* Apply the modifier to the events in the group_def. */
 	struct list_head *list = $1;
 	int err;
 
-	err = parse_events__modifier_group(list, $3);
-	free($3);
-	if (err) {
-		struct parse_events_state *parse_state = _parse_state;
-		struct parse_events_error *error = parse_state->error;
-
-		parse_events_error__handle(error, @3.first_column,
-					   strdup("Bad modifier"), NULL);
-		free_list_evsel(list);
+	err = parse_events__modifier_group(_parse_state, &@3, list, $3);
+	if (err)
 		YYABORT;
-	}
 	$$ = list;
 }
 |
@@ -238,17 +232,9 @@ event_name PE_MODIFIER_EVENT
 	 * (there could be more events added for multiple tracepoint
 	 * definitions via '*?'.
 	 */
-	err = parse_events__modifier_event(list, $2, false);
-	free($2);
-	if (err) {
-		struct parse_events_state *parse_state = _parse_state;
-		struct parse_events_error *error = parse_state->error;
-
-		parse_events_error__handle(error, @2.first_column,
-					   strdup("Bad modifier"), NULL);
-		free_list_evsel(list);
+	err = parse_events__modifier_event(_parse_state, &@2, list, $2);
+	if (err)
 		YYABORT;
-	}
 	$$ = list;
 }
 |
-- 
2.44.0.683.g7961c838ac-goog

Re: [PATCH v2 13/16] perf parse-events: Improvements to modifier parsing

[Liang, Kan]

On 2024-04-16 2:15 a.m., Ian Rogers wrote:
> Use a struct/bitmap rather than a copied string from lexer.
> 
> In lexer give improved error message when too many precise flags are
> given or repeated modifiers.
> 
> Before:
> ```
> $ perf stat -e 'cycles:kuk' true
> event syntax error: 'cycles:kuk'
>                             \___ Bad modifier
> ...
> $ perf stat -e 'cycles:pppp' true
> event syntax error: 'cycles:pppp'
>                             \___ Bad modifier
> ...
> $ perf stat -e '{instructions:p,cycles:pp}:pp' -a true
> event syntax error: '..cycles:pp}:pp'
>                                   \___ Bad modifier
> ...
> ```
> After:
> ```
> $ perf stat -e 'cycles:kuk' true
> event syntax error: 'cycles:kuk'
>                               \___ Duplicate modifier 'k' (kernel)
> ...
> $ perf stat -e 'cycles:pppp' true
> event syntax error: 'cycles:pppp'
>                                \___ Maximum precise value is 3
> ...
> $ perf stat -e '{instructions:p,cycles:pp}:pp' true
> event syntax error: '..cycles:pp}:pp'
>                                   \___ Maximum combined precise value is 3, adding precision to "cycles:pp"
> ...
> ```
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/parse-events.c | 250 ++++++++++++---------------------
>  tools/perf/util/parse-events.h |  23 ++-
>  tools/perf/util/parse-events.l |  75 +++++++++-
>  tools/perf/util/parse-events.y |  28 +---
>  4 files changed, 194 insertions(+), 182 deletions(-)
> 
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index ebada37ef98a..3ab533d0e653 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -1700,12 +1700,6 @@ int parse_events_multi_pmu_add_or_add_pmu(struct parse_events_state *parse_state
>  	return -EINVAL;
>  }
>  
> -int parse_events__modifier_group(struct list_head *list,
> -				 char *event_mod)
> -{
> -	return parse_events__modifier_event(list, event_mod, true);
> -}
> -
>  void parse_events__set_leader(char *name, struct list_head *list)
>  {
>  	struct evsel *leader;
> @@ -1720,183 +1714,125 @@ void parse_events__set_leader(char *name, struct list_head *list)
>  	leader->group_name = name;
>  }
>  
> -struct event_modifier {
> -	int eu;
> -	int ek;
> -	int eh;
> -	int eH;
> -	int eG;
> -	int eI;
> -	int precise;
> -	int precise_max;
> -	int exclude_GH;
> -	int sample_read;
> -	int pinned;
> -	int weak;
> -	int exclusive;
> -	int bpf_counter;
> -};
> +static int parse_events__modifier_list(struct parse_events_state *parse_state,
> +				       YYLTYPE *loc,
> +				       struct list_head *list,
> +				       struct parse_events_modifier mod,
> +				       bool group)
> +{
> +	struct evsel *evsel;
>  
> -static int get_event_modifier(struct event_modifier *mod, char *str,
> -			       struct evsel *evsel)
> -{
> -	int eu = evsel ? evsel->core.attr.exclude_user : 0;
> -	int ek = evsel ? evsel->core.attr.exclude_kernel : 0;
> -	int eh = evsel ? evsel->core.attr.exclude_hv : 0;
> -	int eH = evsel ? evsel->core.attr.exclude_host : 0;
> -	int eG = evsel ? evsel->core.attr.exclude_guest : 0;
> -	int eI = evsel ? evsel->core.attr.exclude_idle : 0;
> -	int precise = evsel ? evsel->core.attr.precise_ip : 0;
> -	int precise_max = 0;
> -	int sample_read = 0;
> -	int pinned = evsel ? evsel->core.attr.pinned : 0;
> -	int exclusive = evsel ? evsel->core.attr.exclusive : 0;
> -
> -	int exclude = eu | ek | eh;
> -	int exclude_GH = evsel ? evsel->exclude_GH : 0;
> -	int weak = 0;
> -	int bpf_counter = 0;
> -
> -	memset(mod, 0, sizeof(*mod));
> -
> -	while (*str) {
> -		if (*str == 'u') {
> +	if (!group && mod.weak) {
> +		parse_events_error__handle(parse_state->error, loc->first_column,
> +					   strdup("Weak modifier is for use with groups"), NULL);
> +		return -EINVAL;
> +	}
> +
> +	__evlist__for_each_entry(list, evsel) {
> +		/* Translate modifiers into the equivalent evsel excludes. */
> +		int eu = group ? evsel->core.attr.exclude_user : 0;
> +		int ek = group ? evsel->core.attr.exclude_kernel : 0;
> +		int eh = group ? evsel->core.attr.exclude_hv : 0;
> +		int eH = group ? evsel->core.attr.exclude_host : 0;
> +		int eG = group ? evsel->core.attr.exclude_guest : 0;
> +		int exclude = eu | ek | eh;
> +		int exclude_GH = group ? evsel->exclude_GH : 0;
> +
> +		if (mod.precise) {
> +			/* use of precise requires exclude_guest */
> +			eG = 1;
> +		}
> +		if (mod.user) {
>  			if (!exclude)
>  				exclude = eu = ek = eh = 1;
>  			if (!exclude_GH && !perf_guest)
>  				eG = 1;
>  			eu = 0;
> -		} else if (*str == 'k') {
> +		}
> +		if (mod.kernel) {
>  			if (!exclude)
>  				exclude = eu = ek = eh = 1;
>  			ek = 0;
> -		} else if (*str == 'h') {
> +		}
> +		if (mod.hypervisor) {
>  			if (!exclude)
>  				exclude = eu = ek = eh = 1;
>  			eh = 0;
> -		} else if (*str == 'G') {
> +		}
> +		if (mod.guest) {
>  			if (!exclude_GH)
>  				exclude_GH = eG = eH = 1;
>  			eG = 0;
> -		} else if (*str == 'H') {
> +		}
> +		if (mod.host) {
>  			if (!exclude_GH)
>  				exclude_GH = eG = eH = 1;
>  			eH = 0;
> -		} else if (*str == 'I') {
> -			eI = 1;
> -		} else if (*str == 'p') {
> -			precise++;
> -			/* use of precise requires exclude_guest */
> -			if (!exclude_GH)
> -				eG = 1;
> -		} else if (*str == 'P') {
> -			precise_max = 1;
> -		} else if (*str == 'S') {
> -			sample_read = 1;
> -		} else if (*str == 'D') {
> -			pinned = 1;
> -		} else if (*str == 'e') {
> -			exclusive = 1;
> -		} else if (*str == 'W') {
> -			weak = 1;
> -		} else if (*str == 'b') {
> -			bpf_counter = 1;
> -		} else
> -			break;
> -
> -		++str;
> +		}
> +		evsel->core.attr.exclude_user   = eu;
> +		evsel->core.attr.exclude_kernel = ek;
> +		evsel->core.attr.exclude_hv     = eh;
> +		evsel->core.attr.exclude_host   = eH;
> +		evsel->core.attr.exclude_guest  = eG;
> +		evsel->exclude_GH               = exclude_GH;
> +
> +		/* Simple modifiers copied to the evsel. */
> +		if (mod.precise) {
> +			u8 precise = evsel->core.attr.precise_ip + mod.precise;
> +			/*
> +			 * precise ip:
> +			 *
> +			 *  0 - SAMPLE_IP can have arbitrary skid
> +			 *  1 - SAMPLE_IP must have constant skid
> +			 *  2 - SAMPLE_IP requested to have 0 skid
> +			 *  3 - SAMPLE_IP must have 0 skid
> +			 *
> +			 *  See also PERF_RECORD_MISC_EXACT_IP
> +			 */
> +			if (precise > 3) {

The pmu_max_precise() should return the max precise the current kernel
supports. It checks the /sys/devices/cpu/caps/max_precise.

I think we should use that value rather than hard code it to 3.

Thanks,
Kan

> +				char *help;
> +
> +				if (asprintf(&help,
> +					     "Maximum combined precise value is 3, adding precision to \"%s\"",
> +					     evsel__name(evsel)) > 0) {
> +					parse_events_error__handle(parse_state->error,
> +								   loc->first_column,
> +								   help, NULL);
> +				}
> +				return -EINVAL;
> +			}
> +			evsel->core.attr.precise_ip = precise;
> +		}
> +		if (mod.precise_max)
> +			evsel->precise_max = 1;
> +		if (mod.non_idle)
> +			evsel->core.attr.exclude_idle = 1;
> +		if (mod.sample_read)
> +			evsel->sample_read = 1;
> +		if (mod.pinned && evsel__is_group_leader(evsel))
> +			evsel->core.attr.pinned = 1;
> +		if (mod.exclusive && evsel__is_group_leader(evsel))
> +			evsel->core.attr.exclusive = 1;
> +		if (mod.weak)
> +			evsel->weak_group = true;
> +		if (mod.bpf)
> +			evsel->bpf_counter = true;
>  	}
> -
> -	/*
> -	 * precise ip:
> -	 *
> -	 *  0 - SAMPLE_IP can have arbitrary skid
> -	 *  1 - SAMPLE_IP must have constant skid
> -	 *  2 - SAMPLE_IP requested to have 0 skid
> -	 *  3 - SAMPLE_IP must have 0 skid
> -	 *
> -	 *  See also PERF_RECORD_MISC_EXACT_IP
> -	 */
> -	if (precise > 3)
> -		return -EINVAL;
> -
> -	mod->eu = eu;
> -	mod->ek = ek;
> -	mod->eh = eh;
> -	mod->eH = eH;
> -	mod->eG = eG;
> -	mod->eI = eI;
> -	mod->precise = precise;
> -	mod->precise_max = precise_max;
> -	mod->exclude_GH = exclude_GH;
> -	mod->sample_read = sample_read;
> -	mod->pinned = pinned;
> -	mod->weak = weak;
> -	mod->bpf_counter = bpf_counter;
> -	mod->exclusive = exclusive;
> -
>  	return 0;
>  }
>  
> -/*
> - * Basic modifier sanity check to validate it contains only one
> - * instance of any modifier (apart from 'p') present.
> - */
> -static int check_modifier(char *str)
> +int parse_events__modifier_group(struct parse_events_state *parse_state, void *loc,
> +				 struct list_head *list,
> +				 struct parse_events_modifier mod)
>  {
> -	char *p = str;
> -
> -	/* The sizeof includes 0 byte as well. */
> -	if (strlen(str) > (sizeof("ukhGHpppPSDIWeb") - 1))
> -		return -1;
> -
> -	while (*p) {
> -		if (*p != 'p' && strchr(p + 1, *p))
> -			return -1;
> -		p++;
> -	}
> -
> -	return 0;
> +	return parse_events__modifier_list(parse_state, loc, list, mod, /*group=*/true);
>  }
>  
> -int parse_events__modifier_event(struct list_head *list, char *str, bool add)
> +int parse_events__modifier_event(struct parse_events_state *parse_state, void *loc,
> +				 struct list_head *list,
> +				 struct parse_events_modifier mod)
>  {
> -	struct evsel *evsel;
> -	struct event_modifier mod;
> -
> -	if (str == NULL)
> -		return 0;
> -
> -	if (check_modifier(str))
> -		return -EINVAL;
> -
> -	if (!add && get_event_modifier(&mod, str, NULL))
> -		return -EINVAL;
> -
> -	__evlist__for_each_entry(list, evsel) {
> -		if (add && get_event_modifier(&mod, str, evsel))
> -			return -EINVAL;
> -
> -		evsel->core.attr.exclude_user   = mod.eu;
> -		evsel->core.attr.exclude_kernel = mod.ek;
> -		evsel->core.attr.exclude_hv     = mod.eh;
> -		evsel->core.attr.precise_ip     = mod.precise;
> -		evsel->core.attr.exclude_host   = mod.eH;
> -		evsel->core.attr.exclude_guest  = mod.eG;
> -		evsel->core.attr.exclude_idle   = mod.eI;
> -		evsel->exclude_GH          = mod.exclude_GH;
> -		evsel->sample_read         = mod.sample_read;
> -		evsel->precise_max         = mod.precise_max;
> -		evsel->weak_group	   = mod.weak;
> -		evsel->bpf_counter	   = mod.bpf_counter;
> -
> -		if (evsel__is_group_leader(evsel)) {
> -			evsel->core.attr.pinned = mod.pinned;
> -			evsel->core.attr.exclusive = mod.exclusive;
> -		}
> -	}
> -
> -	return 0;
> +	return parse_events__modifier_list(parse_state, loc, list, mod, /*group=*/false);
>  }
>  
>  int parse_events_name(struct list_head *list, const char *name)
> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> index 290ae6c72ec5..f104faef1a78 100644
> --- a/tools/perf/util/parse-events.h
> +++ b/tools/perf/util/parse-events.h
> @@ -186,8 +186,27 @@ void parse_events_terms__init(struct parse_events_terms *terms);
>  void parse_events_terms__exit(struct parse_events_terms *terms);
>  int parse_events_terms(struct parse_events_terms *terms, const char *str, FILE *input);
>  int parse_events_terms__to_strbuf(const struct parse_events_terms *terms, struct strbuf *sb);
> -int parse_events__modifier_event(struct list_head *list, char *str, bool add);
> -int parse_events__modifier_group(struct list_head *list, char *event_mod);
> +
> +struct parse_events_modifier {
> +	u8 precise;	/* Number of repeated 'p' for precision. */
> +	bool precise_max : 1;	/* 'P' */
> +	bool non_idle : 1;	/* 'I' */
> +	bool sample_read : 1;	/* 'S' */
> +	bool pinned : 1;	/* 'D' */
> +	bool exclusive : 1;	/* 'e' */
> +	bool weak : 1;		/* 'W' */
> +	bool bpf : 1;		/* 'b' */
> +	bool user : 1;		/* 'u' */
> +	bool kernel : 1;	/* 'k' */
> +	bool hypervisor : 1;	/* 'h' */
> +	bool guest : 1;		/* 'G' */
> +	bool host : 1;		/* 'H' */
> +};
> +
> +int parse_events__modifier_event(struct parse_events_state *parse_state, void *loc,
> +				 struct list_head *list, struct parse_events_modifier mod);
> +int parse_events__modifier_group(struct parse_events_state *parse_state, void *loc,
> +				 struct list_head *list, struct parse_events_modifier mod);
>  int parse_events_name(struct list_head *list, const char *name);
>  int parse_events_add_tracepoint(struct list_head *list, int *idx,
>  				const char *sys, const char *event,
> diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
> index 0cd68c9f0d4f..4aaf0c53d9b6 100644
> --- a/tools/perf/util/parse-events.l
> +++ b/tools/perf/util/parse-events.l
> @@ -142,6 +142,77 @@ static int hw(yyscan_t scanner, int config)
>  	return PE_TERM_HW;
>  }
>  
> +static void modifiers_error(struct parse_events_state *parse_state, yyscan_t scanner,
> +			    int pos, char mod_char, const char *mod_name)
> +{
> +	struct parse_events_error *error = parse_state->error;
> +	char *help = NULL;
> +
> +	if (asprintf(&help, "Duplicate modifier '%c' (%s)", mod_char, mod_name) > 0)
> +		parse_events_error__handle(error, get_column(scanner) + pos, help , NULL);
> +}
> +
> +static int modifiers(struct parse_events_state *parse_state, yyscan_t scanner)
> +{
> +	YYSTYPE *yylval = parse_events_get_lval(scanner);
> +	char *text = parse_events_get_text(scanner);
> +	struct parse_events_modifier mod = { .precise = 0, };
> +
> +	for (size_t i = 0, n = strlen(text); i < n; i++) {
> +#define CASE(c, field)							\
> +		case c:							\
> +			if (mod.field) {				\
> +				modifiers_error(parse_state, scanner, i, c, #field); \
> +				return PE_ERROR;			\
> +			}						\
> +			mod.field = true;				\
> +			break
> +
> +		switch (text[i]) {
> +		CASE('u', user);
> +		CASE('k', kernel);
> +		CASE('h', hypervisor);
> +		CASE('I', non_idle);
> +		CASE('G', guest);
> +		CASE('H', host);
> +		case 'p':
> +			mod.precise++;
> +			/*
> +			 * precise ip:
> +			 *
> +			 *  0 - SAMPLE_IP can have arbitrary skid
> +			 *  1 - SAMPLE_IP must have constant skid
> +			 *  2 - SAMPLE_IP requested to have 0 skid
> +			 *  3 - SAMPLE_IP must have 0 skid
> +			 *
> +			 *  See also PERF_RECORD_MISC_EXACT_IP
> +			 */
> +			if (mod.precise > 3) {
> +				struct parse_events_error *error = parse_state->error;
> +				char *help = strdup("Maximum precise value is 3");
> +
> +				if (help) {
> +					parse_events_error__handle(error, get_column(scanner) + i,
> +								   help , NULL);
> +				}
> +				return PE_ERROR;
> +			}
> +			break;
> +		CASE('P', precise_max);
> +		CASE('S', sample_read);
> +		CASE('D', pinned);
> +		CASE('W', weak);
> +		CASE('e', exclusive);
> +		CASE('b', bpf);
> +		default:
> +			return PE_ERROR;
> +		}
> +#undef CASE
> +	}
> +	yylval->mod = mod;
> +	return PE_MODIFIER_EVENT;
> +}
> +
>  #define YY_USER_ACTION					\
>  do {							\
>  	yylloc->last_column  = yylloc->first_column;	\
> @@ -174,7 +245,7 @@ drv_cfg_term	[a-zA-Z0-9_\.]+(=[a-zA-Z0-9_*?\.:]+)?
>   * If you add a modifier you need to update check_modifier().
>   * Also, the letters in modifier_event must not be in modifier_bp.
>   */
> -modifier_event	[ukhpPGHSDIWeb]+
> +modifier_event	[ukhpPGHSDIWeb]{1,15}
>  modifier_bp	[rwx]{1,3}
>  lc_type 	(L1-dcache|l1-d|l1d|L1-data|L1-icache|l1-i|l1i|L1-instruction|LLC|L2|dTLB|d-tlb|Data-TLB|iTLB|i-tlb|Instruction-TLB|branch|branches|bpu|btb|bpc|node)
>  lc_op_result	(load|loads|read|store|stores|write|prefetch|prefetches|speculative-read|speculative-load|refs|Reference|ops|access|misses|miss)
> @@ -341,7 +412,7 @@ r{num_raw_hex}		{ return str(yyscanner, PE_RAW); }
>  {num_dec}		{ return value(_parse_state, yyscanner, 10); }
>  {num_hex}		{ return value(_parse_state, yyscanner, 16); }
>  
> -{modifier_event}	{ return str(yyscanner, PE_MODIFIER_EVENT); }
> +{modifier_event}	{ return modifiers(_parse_state, yyscanner); }
>  {name}			{ return str(yyscanner, PE_NAME); }
>  {name_tag}		{ return str(yyscanner, PE_NAME); }
>  "/"			{ BEGIN(config); return '/'; }
> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> index 2c4817e126c1..79f254189be6 100644
> --- a/tools/perf/util/parse-events.y
> +++ b/tools/perf/util/parse-events.y
> @@ -68,11 +68,11 @@ static void free_list_evsel(struct list_head* list_evsel)
>  %type <num> PE_VALUE
>  %type <num> PE_VALUE_SYM_SW
>  %type <num> PE_VALUE_SYM_TOOL
> +%type <mod> PE_MODIFIER_EVENT
>  %type <term_type> PE_TERM
>  %type <str> PE_RAW
>  %type <str> PE_NAME
>  %type <str> PE_LEGACY_CACHE
> -%type <str> PE_MODIFIER_EVENT
>  %type <str> PE_MODIFIER_BP
>  %type <str> PE_EVENT_NAME
>  %type <str> PE_DRV_CFG_TERM
> @@ -110,6 +110,7 @@ static void free_list_evsel(struct list_head* list_evsel)
>  {
>  	char *str;
>  	u64 num;
> +	struct parse_events_modifier mod;
>  	enum parse_events__term_type term_type;
>  	struct list_head *list_evsel;
>  	struct parse_events_terms *list_terms;
> @@ -175,20 +176,13 @@ event
>  group:
>  group_def ':' PE_MODIFIER_EVENT
>  {
> +	/* Apply the modifier to the events in the group_def. */
>  	struct list_head *list = $1;
>  	int err;
>  
> -	err = parse_events__modifier_group(list, $3);
> -	free($3);
> -	if (err) {
> -		struct parse_events_state *parse_state = _parse_state;
> -		struct parse_events_error *error = parse_state->error;
> -
> -		parse_events_error__handle(error, @3.first_column,
> -					   strdup("Bad modifier"), NULL);
> -		free_list_evsel(list);
> +	err = parse_events__modifier_group(_parse_state, &@3, list, $3);
> +	if (err)
>  		YYABORT;
> -	}
>  	$$ = list;
>  }
>  |
> @@ -238,17 +232,9 @@ event_name PE_MODIFIER_EVENT
>  	 * (there could be more events added for multiple tracepoint
>  	 * definitions via '*?'.
>  	 */
> -	err = parse_events__modifier_event(list, $2, false);
> -	free($2);
> -	if (err) {
> -		struct parse_events_state *parse_state = _parse_state;
> -		struct parse_events_error *error = parse_state->error;
> -
> -		parse_events_error__handle(error, @2.first_column,
> -					   strdup("Bad modifier"), NULL);
> -		free_list_evsel(list);
> +	err = parse_events__modifier_event(_parse_state, &@2, list, $2);
> +	if (err)
>  		YYABORT;
> -	}
>  	$$ = list;
>  }
>  |

Re: [PATCH v2 13/16] perf parse-events: Improvements to modifier parsing

[Ian Rogers]

On Thu, Apr 18, 2024 at 1:32 PM Liang, Kan <kan.liang@linux.intel.com> wrote:
>
>
>
> On 2024-04-16 2:15 a.m., Ian Rogers wrote:
> > Use a struct/bitmap rather than a copied string from lexer.
> >
> > In lexer give improved error message when too many precise flags are
> > given or repeated modifiers.
> >
> > Before:
> > ```
> > $ perf stat -e 'cycles:kuk' true
> > event syntax error: 'cycles:kuk'
> >                             \___ Bad modifier
> > ...
> > $ perf stat -e 'cycles:pppp' true
> > event syntax error: 'cycles:pppp'
> >                             \___ Bad modifier
> > ...
> > $ perf stat -e '{instructions:p,cycles:pp}:pp' -a true
> > event syntax error: '..cycles:pp}:pp'
> >                                   \___ Bad modifier
> > ...
> > ```
> > After:
> > ```
> > $ perf stat -e 'cycles:kuk' true
> > event syntax error: 'cycles:kuk'
> >                               \___ Duplicate modifier 'k' (kernel)
> > ...
> > $ perf stat -e 'cycles:pppp' true
> > event syntax error: 'cycles:pppp'
> >                                \___ Maximum precise value is 3
> > ...
> > $ perf stat -e '{instructions:p,cycles:pp}:pp' true
> > event syntax error: '..cycles:pp}:pp'
> >                                   \___ Maximum combined precise value is 3, adding precision to "cycles:pp"
> > ...
> > ```
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/parse-events.c | 250 ++++++++++++---------------------
> >  tools/perf/util/parse-events.h |  23 ++-
> >  tools/perf/util/parse-events.l |  75 +++++++++-
> >  tools/perf/util/parse-events.y |  28 +---
> >  4 files changed, 194 insertions(+), 182 deletions(-)
> >
> > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> > index ebada37ef98a..3ab533d0e653 100644
> > --- a/tools/perf/util/parse-events.c
> > +++ b/tools/perf/util/parse-events.c
> > @@ -1700,12 +1700,6 @@ int parse_events_multi_pmu_add_or_add_pmu(struct parse_events_state *parse_state
> >       return -EINVAL;
> >  }
> >
> > -int parse_events__modifier_group(struct list_head *list,
> > -                              char *event_mod)
> > -{
> > -     return parse_events__modifier_event(list, event_mod, true);
> > -}
> > -
> >  void parse_events__set_leader(char *name, struct list_head *list)
> >  {
> >       struct evsel *leader;
> > @@ -1720,183 +1714,125 @@ void parse_events__set_leader(char *name, struct list_head *list)
> >       leader->group_name = name;
> >  }
> >
> > -struct event_modifier {
> > -     int eu;
> > -     int ek;
> > -     int eh;
> > -     int eH;
> > -     int eG;
> > -     int eI;
> > -     int precise;
> > -     int precise_max;
> > -     int exclude_GH;
> > -     int sample_read;
> > -     int pinned;
> > -     int weak;
> > -     int exclusive;
> > -     int bpf_counter;
> > -};
> > +static int parse_events__modifier_list(struct parse_events_state *parse_state,
> > +                                    YYLTYPE *loc,
> > +                                    struct list_head *list,
> > +                                    struct parse_events_modifier mod,
> > +                                    bool group)
> > +{
> > +     struct evsel *evsel;
> >
> > -static int get_event_modifier(struct event_modifier *mod, char *str,
> > -                            struct evsel *evsel)
> > -{
> > -     int eu = evsel ? evsel->core.attr.exclude_user : 0;
> > -     int ek = evsel ? evsel->core.attr.exclude_kernel : 0;
> > -     int eh = evsel ? evsel->core.attr.exclude_hv : 0;
> > -     int eH = evsel ? evsel->core.attr.exclude_host : 0;
> > -     int eG = evsel ? evsel->core.attr.exclude_guest : 0;
> > -     int eI = evsel ? evsel->core.attr.exclude_idle : 0;
> > -     int precise = evsel ? evsel->core.attr.precise_ip : 0;
> > -     int precise_max = 0;
> > -     int sample_read = 0;
> > -     int pinned = evsel ? evsel->core.attr.pinned : 0;
> > -     int exclusive = evsel ? evsel->core.attr.exclusive : 0;
> > -
> > -     int exclude = eu | ek | eh;
> > -     int exclude_GH = evsel ? evsel->exclude_GH : 0;
> > -     int weak = 0;
> > -     int bpf_counter = 0;
> > -
> > -     memset(mod, 0, sizeof(*mod));
> > -
> > -     while (*str) {
> > -             if (*str == 'u') {
> > +     if (!group && mod.weak) {
> > +             parse_events_error__handle(parse_state->error, loc->first_column,
> > +                                        strdup("Weak modifier is for use with groups"), NULL);
> > +             return -EINVAL;
> > +     }
> > +
> > +     __evlist__for_each_entry(list, evsel) {
> > +             /* Translate modifiers into the equivalent evsel excludes. */
> > +             int eu = group ? evsel->core.attr.exclude_user : 0;
> > +             int ek = group ? evsel->core.attr.exclude_kernel : 0;
> > +             int eh = group ? evsel->core.attr.exclude_hv : 0;
> > +             int eH = group ? evsel->core.attr.exclude_host : 0;
> > +             int eG = group ? evsel->core.attr.exclude_guest : 0;
> > +             int exclude = eu | ek | eh;
> > +             int exclude_GH = group ? evsel->exclude_GH : 0;
> > +
> > +             if (mod.precise) {
> > +                     /* use of precise requires exclude_guest */
> > +                     eG = 1;
> > +             }
> > +             if (mod.user) {
> >                       if (!exclude)
> >                               exclude = eu = ek = eh = 1;
> >                       if (!exclude_GH && !perf_guest)
> >                               eG = 1;
> >                       eu = 0;
> > -             } else if (*str == 'k') {
> > +             }
> > +             if (mod.kernel) {
> >                       if (!exclude)
> >                               exclude = eu = ek = eh = 1;
> >                       ek = 0;
> > -             } else if (*str == 'h') {
> > +             }
> > +             if (mod.hypervisor) {
> >                       if (!exclude)
> >                               exclude = eu = ek = eh = 1;
> >                       eh = 0;
> > -             } else if (*str == 'G') {
> > +             }
> > +             if (mod.guest) {
> >                       if (!exclude_GH)
> >                               exclude_GH = eG = eH = 1;
> >                       eG = 0;
> > -             } else if (*str == 'H') {
> > +             }
> > +             if (mod.host) {
> >                       if (!exclude_GH)
> >                               exclude_GH = eG = eH = 1;
> >                       eH = 0;
> > -             } else if (*str == 'I') {
> > -                     eI = 1;
> > -             } else if (*str == 'p') {
> > -                     precise++;
> > -                     /* use of precise requires exclude_guest */
> > -                     if (!exclude_GH)
> > -                             eG = 1;
> > -             } else if (*str == 'P') {
> > -                     precise_max = 1;
> > -             } else if (*str == 'S') {
> > -                     sample_read = 1;
> > -             } else if (*str == 'D') {
> > -                     pinned = 1;
> > -             } else if (*str == 'e') {
> > -                     exclusive = 1;
> > -             } else if (*str == 'W') {
> > -                     weak = 1;
> > -             } else if (*str == 'b') {
> > -                     bpf_counter = 1;
> > -             } else
> > -                     break;
> > -
> > -             ++str;
> > +             }
> > +             evsel->core.attr.exclude_user   = eu;
> > +             evsel->core.attr.exclude_kernel = ek;
> > +             evsel->core.attr.exclude_hv     = eh;
> > +             evsel->core.attr.exclude_host   = eH;
> > +             evsel->core.attr.exclude_guest  = eG;
> > +             evsel->exclude_GH               = exclude_GH;
> > +
> > +             /* Simple modifiers copied to the evsel. */
> > +             if (mod.precise) {
> > +                     u8 precise = evsel->core.attr.precise_ip + mod.precise;
> > +                     /*
> > +                      * precise ip:
> > +                      *
> > +                      *  0 - SAMPLE_IP can have arbitrary skid
> > +                      *  1 - SAMPLE_IP must have constant skid
> > +                      *  2 - SAMPLE_IP requested to have 0 skid
> > +                      *  3 - SAMPLE_IP must have 0 skid
> > +                      *
> > +                      *  See also PERF_RECORD_MISC_EXACT_IP
> > +                      */
> > +                     if (precise > 3) {
>
> The pmu_max_precise() should return the max precise the current kernel
> supports. It checks the /sys/devices/cpu/caps/max_precise.
>
> I think we should use that value rather than hard code it to 3.

I'll add an extra patch to do that. I'm a bit concerned it may break
event parsing on platforms not supporting max_precise of 3.

Thanks,
Ian

> Thanks,
> Kan
>
> > +                             char *help;
> > +
> > +                             if (asprintf(&help,
> > +                                          "Maximum combined precise value is 3, adding precision to \"%s\"",
> > +                                          evsel__name(evsel)) > 0) {
> > +                                     parse_events_error__handle(parse_state->error,
> > +                                                                loc->first_column,
> > +                                                                help, NULL);
> > +                             }
> > +                             return -EINVAL;
> > +                     }
> > +                     evsel->core.attr.precise_ip = precise;
> > +             }
> > +             if (mod.precise_max)
> > +                     evsel->precise_max = 1;
> > +             if (mod.non_idle)
> > +                     evsel->core.attr.exclude_idle = 1;
> > +             if (mod.sample_read)
> > +                     evsel->sample_read = 1;
> > +             if (mod.pinned && evsel__is_group_leader(evsel))
> > +                     evsel->core.attr.pinned = 1;
> > +             if (mod.exclusive && evsel__is_group_leader(evsel))
> > +                     evsel->core.attr.exclusive = 1;
> > +             if (mod.weak)
> > +                     evsel->weak_group = true;
> > +             if (mod.bpf)
> > +                     evsel->bpf_counter = true;
> >       }
> > -
> > -     /*
> > -      * precise ip:
> > -      *
> > -      *  0 - SAMPLE_IP can have arbitrary skid
> > -      *  1 - SAMPLE_IP must have constant skid
> > -      *  2 - SAMPLE_IP requested to have 0 skid
> > -      *  3 - SAMPLE_IP must have 0 skid
> > -      *
> > -      *  See also PERF_RECORD_MISC_EXACT_IP
> > -      */
> > -     if (precise > 3)
> > -             return -EINVAL;
> > -
> > -     mod->eu = eu;
> > -     mod->ek = ek;
> > -     mod->eh = eh;
> > -     mod->eH = eH;
> > -     mod->eG = eG;
> > -     mod->eI = eI;
> > -     mod->precise = precise;
> > -     mod->precise_max = precise_max;
> > -     mod->exclude_GH = exclude_GH;
> > -     mod->sample_read = sample_read;
> > -     mod->pinned = pinned;
> > -     mod->weak = weak;
> > -     mod->bpf_counter = bpf_counter;
> > -     mod->exclusive = exclusive;
> > -
> >       return 0;
> >  }
> >
> > -/*
> > - * Basic modifier sanity check to validate it contains only one
> > - * instance of any modifier (apart from 'p') present.
> > - */
> > -static int check_modifier(char *str)
> > +int parse_events__modifier_group(struct parse_events_state *parse_state, void *loc,
> > +                              struct list_head *list,
> > +                              struct parse_events_modifier mod)
> >  {
> > -     char *p = str;
> > -
> > -     /* The sizeof includes 0 byte as well. */
> > -     if (strlen(str) > (sizeof("ukhGHpppPSDIWeb") - 1))
> > -             return -1;
> > -
> > -     while (*p) {
> > -             if (*p != 'p' && strchr(p + 1, *p))
> > -                     return -1;
> > -             p++;
> > -     }
> > -
> > -     return 0;
> > +     return parse_events__modifier_list(parse_state, loc, list, mod, /*group=*/true);
> >  }
> >
> > -int parse_events__modifier_event(struct list_head *list, char *str, bool add)
> > +int parse_events__modifier_event(struct parse_events_state *parse_state, void *loc,
> > +                              struct list_head *list,
> > +                              struct parse_events_modifier mod)
> >  {
> > -     struct evsel *evsel;
> > -     struct event_modifier mod;
> > -
> > -     if (str == NULL)
> > -             return 0;
> > -
> > -     if (check_modifier(str))
> > -             return -EINVAL;
> > -
> > -     if (!add && get_event_modifier(&mod, str, NULL))
> > -             return -EINVAL;
> > -
> > -     __evlist__for_each_entry(list, evsel) {
> > -             if (add && get_event_modifier(&mod, str, evsel))
> > -                     return -EINVAL;
> > -
> > -             evsel->core.attr.exclude_user   = mod.eu;
> > -             evsel->core.attr.exclude_kernel = mod.ek;
> > -             evsel->core.attr.exclude_hv     = mod.eh;
> > -             evsel->core.attr.precise_ip     = mod.precise;
> > -             evsel->core.attr.exclude_host   = mod.eH;
> > -             evsel->core.attr.exclude_guest  = mod.eG;
> > -             evsel->core.attr.exclude_idle   = mod.eI;
> > -             evsel->exclude_GH          = mod.exclude_GH;
> > -             evsel->sample_read         = mod.sample_read;
> > -             evsel->precise_max         = mod.precise_max;
> > -             evsel->weak_group          = mod.weak;
> > -             evsel->bpf_counter         = mod.bpf_counter;
> > -
> > -             if (evsel__is_group_leader(evsel)) {
> > -                     evsel->core.attr.pinned = mod.pinned;
> > -                     evsel->core.attr.exclusive = mod.exclusive;
> > -             }
> > -     }
> > -
> > -     return 0;
> > +     return parse_events__modifier_list(parse_state, loc, list, mod, /*group=*/false);
> >  }
> >
> >  int parse_events_name(struct list_head *list, const char *name)
> > diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> > index 290ae6c72ec5..f104faef1a78 100644
> > --- a/tools/perf/util/parse-events.h
> > +++ b/tools/perf/util/parse-events.h
> > @@ -186,8 +186,27 @@ void parse_events_terms__init(struct parse_events_terms *terms);
> >  void parse_events_terms__exit(struct parse_events_terms *terms);
> >  int parse_events_terms(struct parse_events_terms *terms, const char *str, FILE *input);
> >  int parse_events_terms__to_strbuf(const struct parse_events_terms *terms, struct strbuf *sb);
> > -int parse_events__modifier_event(struct list_head *list, char *str, bool add);
> > -int parse_events__modifier_group(struct list_head *list, char *event_mod);
> > +
> > +struct parse_events_modifier {
> > +     u8 precise;     /* Number of repeated 'p' for precision. */
> > +     bool precise_max : 1;   /* 'P' */
> > +     bool non_idle : 1;      /* 'I' */
> > +     bool sample_read : 1;   /* 'S' */
> > +     bool pinned : 1;        /* 'D' */
> > +     bool exclusive : 1;     /* 'e' */
> > +     bool weak : 1;          /* 'W' */
> > +     bool bpf : 1;           /* 'b' */
> > +     bool user : 1;          /* 'u' */
> > +     bool kernel : 1;        /* 'k' */
> > +     bool hypervisor : 1;    /* 'h' */
> > +     bool guest : 1;         /* 'G' */
> > +     bool host : 1;          /* 'H' */
> > +};
> > +
> > +int parse_events__modifier_event(struct parse_events_state *parse_state, void *loc,
> > +                              struct list_head *list, struct parse_events_modifier mod);
> > +int parse_events__modifier_group(struct parse_events_state *parse_state, void *loc,
> > +                              struct list_head *list, struct parse_events_modifier mod);
> >  int parse_events_name(struct list_head *list, const char *name);
> >  int parse_events_add_tracepoint(struct list_head *list, int *idx,
> >                               const char *sys, const char *event,
> > diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
> > index 0cd68c9f0d4f..4aaf0c53d9b6 100644
> > --- a/tools/perf/util/parse-events.l
> > +++ b/tools/perf/util/parse-events.l
> > @@ -142,6 +142,77 @@ static int hw(yyscan_t scanner, int config)
> >       return PE_TERM_HW;
> >  }
> >
> > +static void modifiers_error(struct parse_events_state *parse_state, yyscan_t scanner,
> > +                         int pos, char mod_char, const char *mod_name)
> > +{
> > +     struct parse_events_error *error = parse_state->error;
> > +     char *help = NULL;
> > +
> > +     if (asprintf(&help, "Duplicate modifier '%c' (%s)", mod_char, mod_name) > 0)
> > +             parse_events_error__handle(error, get_column(scanner) + pos, help , NULL);
> > +}
> > +
> > +static int modifiers(struct parse_events_state *parse_state, yyscan_t scanner)
> > +{
> > +     YYSTYPE *yylval = parse_events_get_lval(scanner);
> > +     char *text = parse_events_get_text(scanner);
> > +     struct parse_events_modifier mod = { .precise = 0, };
> > +
> > +     for (size_t i = 0, n = strlen(text); i < n; i++) {
> > +#define CASE(c, field)                                                       \
> > +             case c:                                                 \
> > +                     if (mod.field) {                                \
> > +                             modifiers_error(parse_state, scanner, i, c, #field); \
> > +                             return PE_ERROR;                        \
> > +                     }                                               \
> > +                     mod.field = true;                               \
> > +                     break
> > +
> > +             switch (text[i]) {
> > +             CASE('u', user);
> > +             CASE('k', kernel);
> > +             CASE('h', hypervisor);
> > +             CASE('I', non_idle);
> > +             CASE('G', guest);
> > +             CASE('H', host);
> > +             case 'p':
> > +                     mod.precise++;
> > +                     /*
> > +                      * precise ip:
> > +                      *
> > +                      *  0 - SAMPLE_IP can have arbitrary skid
> > +                      *  1 - SAMPLE_IP must have constant skid
> > +                      *  2 - SAMPLE_IP requested to have 0 skid
> > +                      *  3 - SAMPLE_IP must have 0 skid
> > +                      *
> > +                      *  See also PERF_RECORD_MISC_EXACT_IP
> > +                      */
> > +                     if (mod.precise > 3) {
> > +                             struct parse_events_error *error = parse_state->error;
> > +                             char *help = strdup("Maximum precise value is 3");
> > +
> > +                             if (help) {
> > +                                     parse_events_error__handle(error, get_column(scanner) + i,
> > +                                                                help , NULL);
> > +                             }
> > +                             return PE_ERROR;
> > +                     }
> > +                     break;
> > +             CASE('P', precise_max);
> > +             CASE('S', sample_read);
> > +             CASE('D', pinned);
> > +             CASE('W', weak);
> > +             CASE('e', exclusive);
> > +             CASE('b', bpf);
> > +             default:
> > +                     return PE_ERROR;
> > +             }
> > +#undef CASE
> > +     }
> > +     yylval->mod = mod;
> > +     return PE_MODIFIER_EVENT;
> > +}
> > +
> >  #define YY_USER_ACTION                                       \
> >  do {                                                 \
> >       yylloc->last_column  = yylloc->first_column;    \
> > @@ -174,7 +245,7 @@ drv_cfg_term      [a-zA-Z0-9_\.]+(=[a-zA-Z0-9_*?\.:]+)?
> >   * If you add a modifier you need to update check_modifier().
> >   * Also, the letters in modifier_event must not be in modifier_bp.
> >   */
> > -modifier_event       [ukhpPGHSDIWeb]+
> > +modifier_event       [ukhpPGHSDIWeb]{1,15}
> >  modifier_bp  [rwx]{1,3}
> >  lc_type      (L1-dcache|l1-d|l1d|L1-data|L1-icache|l1-i|l1i|L1-instruction|LLC|L2|dTLB|d-tlb|Data-TLB|iTLB|i-tlb|Instruction-TLB|branch|branches|bpu|btb|bpc|node)
> >  lc_op_result (load|loads|read|store|stores|write|prefetch|prefetches|speculative-read|speculative-load|refs|Reference|ops|access|misses|miss)
> > @@ -341,7 +412,7 @@ r{num_raw_hex}            { return str(yyscanner, PE_RAW); }
> >  {num_dec}            { return value(_parse_state, yyscanner, 10); }
> >  {num_hex}            { return value(_parse_state, yyscanner, 16); }
> >
> > -{modifier_event}     { return str(yyscanner, PE_MODIFIER_EVENT); }
> > +{modifier_event}     { return modifiers(_parse_state, yyscanner); }
> >  {name}                       { return str(yyscanner, PE_NAME); }
> >  {name_tag}           { return str(yyscanner, PE_NAME); }
> >  "/"                  { BEGIN(config); return '/'; }
> > diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> > index 2c4817e126c1..79f254189be6 100644
> > --- a/tools/perf/util/parse-events.y
> > +++ b/tools/perf/util/parse-events.y
> > @@ -68,11 +68,11 @@ static void free_list_evsel(struct list_head* list_evsel)
> >  %type <num> PE_VALUE
> >  %type <num> PE_VALUE_SYM_SW
> >  %type <num> PE_VALUE_SYM_TOOL
> > +%type <mod> PE_MODIFIER_EVENT
> >  %type <term_type> PE_TERM
> >  %type <str> PE_RAW
> >  %type <str> PE_NAME
> >  %type <str> PE_LEGACY_CACHE
> > -%type <str> PE_MODIFIER_EVENT
> >  %type <str> PE_MODIFIER_BP
> >  %type <str> PE_EVENT_NAME
> >  %type <str> PE_DRV_CFG_TERM
> > @@ -110,6 +110,7 @@ static void free_list_evsel(struct list_head* list_evsel)
> >  {
> >       char *str;
> >       u64 num;
> > +     struct parse_events_modifier mod;
> >       enum parse_events__term_type term_type;
> >       struct list_head *list_evsel;
> >       struct parse_events_terms *list_terms;
> > @@ -175,20 +176,13 @@ event
> >  group:
> >  group_def ':' PE_MODIFIER_EVENT
> >  {
> > +     /* Apply the modifier to the events in the group_def. */
> >       struct list_head *list = $1;
> >       int err;
> >
> > -     err = parse_events__modifier_group(list, $3);
> > -     free($3);
> > -     if (err) {
> > -             struct parse_events_state *parse_state = _parse_state;
> > -             struct parse_events_error *error = parse_state->error;
> > -
> > -             parse_events_error__handle(error, @3.first_column,
> > -                                        strdup("Bad modifier"), NULL);
> > -             free_list_evsel(list);
> > +     err = parse_events__modifier_group(_parse_state, &@3, list, $3);
> > +     if (err)
> >               YYABORT;
> > -     }
> >       $$ = list;
> >  }
> >  |
> > @@ -238,17 +232,9 @@ event_name PE_MODIFIER_EVENT
> >        * (there could be more events added for multiple tracepoint
> >        * definitions via '*?'.
> >        */
> > -     err = parse_events__modifier_event(list, $2, false);
> > -     free($2);
> > -     if (err) {
> > -             struct parse_events_state *parse_state = _parse_state;
> > -             struct parse_events_error *error = parse_state->error;
> > -
> > -             parse_events_error__handle(error, @2.first_column,
> > -                                        strdup("Bad modifier"), NULL);
> > -             free_list_evsel(list);
> > +     err = parse_events__modifier_event(_parse_state, &@2, list, $2);
> > +     if (err)
> >               YYABORT;
> > -     }
> >       $$ = list;
> >  }
> >  |

Re: [PATCH v2 13/16] perf parse-events: Improvements to modifier parsing

[Liang, Kan]

On 2024-04-19 2:22 a.m., Ian Rogers wrote:
>>> +             /* Simple modifiers copied to the evsel. */
>>> +             if (mod.precise) {
>>> +                     u8 precise = evsel->core.attr.precise_ip + mod.precise;
>>> +                     /*
>>> +                      * precise ip:
>>> +                      *
>>> +                      *  0 - SAMPLE_IP can have arbitrary skid
>>> +                      *  1 - SAMPLE_IP must have constant skid
>>> +                      *  2 - SAMPLE_IP requested to have 0 skid
>>> +                      *  3 - SAMPLE_IP must have 0 skid
>>> +                      *
>>> +                      *  See also PERF_RECORD_MISC_EXACT_IP
>>> +                      */
>>> +                     if (precise > 3) {
>> The pmu_max_precise() should return the max precise the current kernel
>> supports. It checks the /sys/devices/cpu/caps/max_precise.
>>
>> I think we should use that value rather than hard code it to 3.
> I'll add an extra patch to do that. I'm a bit concerned it may break
> event parsing on platforms not supporting max_precise of 3.

The kernel already rejects the precise_ip > max_precise (using the same
x86_pmu_max_precise()). It should be fine to apply the same logic in the
tool.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/events/core.c#n566

Will the extra patch be sent separately?

Thanks,
Kan

Re: [PATCH v2 13/16] perf parse-events: Improvements to modifier parsing

[Ian Rogers]

On Fri, Apr 19, 2024 at 6:20 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>
>
>
> On 2024-04-19 2:22 a.m., Ian Rogers wrote:
> >>> +             /* Simple modifiers copied to the evsel. */
> >>> +             if (mod.precise) {
> >>> +                     u8 precise = evsel->core.attr.precise_ip + mod.precise;
> >>> +                     /*
> >>> +                      * precise ip:
> >>> +                      *
> >>> +                      *  0 - SAMPLE_IP can have arbitrary skid
> >>> +                      *  1 - SAMPLE_IP must have constant skid
> >>> +                      *  2 - SAMPLE_IP requested to have 0 skid
> >>> +                      *  3 - SAMPLE_IP must have 0 skid
> >>> +                      *
> >>> +                      *  See also PERF_RECORD_MISC_EXACT_IP
> >>> +                      */
> >>> +                     if (precise > 3) {
> >> The pmu_max_precise() should return the max precise the current kernel
> >> supports. It checks the /sys/devices/cpu/caps/max_precise.
> >>
> >> I think we should use that value rather than hard code it to 3.
> > I'll add an extra patch to do that. I'm a bit concerned it may break
> > event parsing on platforms not supporting max_precise of 3.
>
> The kernel already rejects the precise_ip > max_precise (using the same
> x86_pmu_max_precise()). It should be fine to apply the same logic in the
> tool.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/events/core.c#n566
>
> Will the extra patch be sent separately?

Let's do it separately. I'm concerned about the behavior on AMD (and
possibly similar architectures) where certain events support precision
like cycles, as they detour to the IBS PMU, but not all events support
it. The max_precise should reflect that AMD's Zen core PMU does
support precision as a consequence of detouring to IBS, but maybe
things in sysfs aren't set up correctly.

Thanks,
Ian

> Thanks,
> Kan

Re: [PATCH v2 13/16] perf parse-events: Improvements to modifier parsing

[Liang, Kan]

On 2024-04-24 11:18 a.m., Ian Rogers wrote:
> On Fri, Apr 19, 2024 at 6:20 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>>
>>
>>
>> On 2024-04-19 2:22 a.m., Ian Rogers wrote:
>>>>> +             /* Simple modifiers copied to the evsel. */
>>>>> +             if (mod.precise) {
>>>>> +                     u8 precise = evsel->core.attr.precise_ip + mod.precise;
>>>>> +                     /*
>>>>> +                      * precise ip:
>>>>> +                      *
>>>>> +                      *  0 - SAMPLE_IP can have arbitrary skid
>>>>> +                      *  1 - SAMPLE_IP must have constant skid
>>>>> +                      *  2 - SAMPLE_IP requested to have 0 skid
>>>>> +                      *  3 - SAMPLE_IP must have 0 skid
>>>>> +                      *
>>>>> +                      *  See also PERF_RECORD_MISC_EXACT_IP
>>>>> +                      */
>>>>> +                     if (precise > 3) {
>>>> The pmu_max_precise() should return the max precise the current kernel
>>>> supports. It checks the /sys/devices/cpu/caps/max_precise.
>>>>
>>>> I think we should use that value rather than hard code it to 3.
>>> I'll add an extra patch to do that. I'm a bit concerned it may break
>>> event parsing on platforms not supporting max_precise of 3.
>>
>> The kernel already rejects the precise_ip > max_precise (using the same
>> x86_pmu_max_precise()). It should be fine to apply the same logic in the
>> tool.
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/events/core.c#n566
>>
>> Will the extra patch be sent separately?
> 
> Let's do it separately. I'm concerned about the behavior on AMD (and
> possibly similar architectures) where certain events support precision
> like cycles, as they detour to the IBS PMU, but not all events support
> it. The max_precise should reflect that AMD's Zen core PMU does
> support precision as a consequence of detouring to IBS, but maybe
> things in sysfs aren't set up correctly.
> 

The x86_pmu_max_precise() is a generic function for X86. It should apply
to AMD as well.

A separate patch looks good to me.

Thanks,
Kan

[PATCH v2 14/16] perf parse-event: Constify event_symbol arrays

[Ian Rogers]

Moves 352 bytes from .data to .data.rel.ro.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/parse-events.c | 4 ++--
 tools/perf/util/parse-events.h | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 3ab533d0e653..8d3d692d219d 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -39,7 +39,7 @@ static int get_config_terms(const struct parse_events_terms *head_config,
 static int parse_events_terms__copy(const struct parse_events_terms *src,
 				    struct parse_events_terms *dest);
 
-struct event_symbol event_symbols_hw[PERF_COUNT_HW_MAX] = {
+const struct event_symbol event_symbols_hw[PERF_COUNT_HW_MAX] = {
 	[PERF_COUNT_HW_CPU_CYCLES] = {
 		.symbol = "cpu-cycles",
 		.alias  = "cycles",
@@ -82,7 +82,7 @@ struct event_symbol event_symbols_hw[PERF_COUNT_HW_MAX] = {
 	},
 };
 
-struct event_symbol event_symbols_sw[PERF_COUNT_SW_MAX] = {
+const struct event_symbol event_symbols_sw[PERF_COUNT_SW_MAX] = {
 	[PERF_COUNT_SW_CPU_CLOCK] = {
 		.symbol = "cpu-clock",
 		.alias  = "",
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index f104faef1a78..0bb5f0c80a5e 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -250,8 +250,8 @@ struct event_symbol {
 	const char	*symbol;
 	const char	*alias;
 };
-extern struct event_symbol event_symbols_hw[];
-extern struct event_symbol event_symbols_sw[];
+extern const struct event_symbol event_symbols_hw[];
+extern const struct event_symbol event_symbols_sw[];
 
 char *parse_events_formats_error_string(char *additional_terms);
 
-- 
2.44.0.683.g7961c838ac-goog

[PATCH v2 15/16] perf parse-events: Minor grouping tidy up

[Ian Rogers]

Add comments. Ensure leader->group_name is freed before overwriting
it.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/parse-events.c | 1 +
 tools/perf/util/parse-events.y | 6 +++++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 8d3d692d219d..1c1b1bcb78e8 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1711,6 +1711,7 @@ void parse_events__set_leader(char *name, struct list_head *list)
 
 	leader = list_first_entry(list, struct evsel, core.node);
 	__perf_evlist__set_leader(list, &leader->core);
+	zfree(&leader->group_name);
 	leader->group_name = name;
 }
 
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 79f254189be6..6f1042272dda 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -193,7 +193,10 @@ PE_NAME '{' events '}'
 {
 	struct list_head *list = $3;
 
-	/* Takes ownership of $1. */
+	/*
+	 * Set the first entry of list to be the leader. Set the group name on
+	 * the leader to $1 taking ownership.
+	 */
 	parse_events__set_leader($1, list);
 	$$ = list;
 }
@@ -202,6 +205,7 @@ PE_NAME '{' events '}'
 {
 	struct list_head *list = $2;
 
+	/* Set the first entry of list to be the leader clearing the group name. */
 	parse_events__set_leader(NULL, list);
 	$$ = list;
 }
-- 
2.44.0.683.g7961c838ac-goog

[PATCH v2 16/16] perf parse-events: Tidy the setting of the default event name

[Ian Rogers]

Add comments. Pass ownership of the event name to save on a strdup.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/parse-events.c |  9 ++++++---
 tools/perf/util/parse-events.h |  2 +-
 tools/perf/util/parse-events.l |  5 +++++
 tools/perf/util/parse-events.y | 10 +++++++---
 4 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 1c1b1bcb78e8..0f308b4db2b9 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1836,18 +1836,21 @@ int parse_events__modifier_event(struct parse_events_state *parse_state, void *l
 	return parse_events__modifier_list(parse_state, loc, list, mod, /*group=*/false);
 }
 
-int parse_events_name(struct list_head *list, const char *name)
+int parse_events__set_default_name(struct list_head *list, char *name)
 {
 	struct evsel *evsel;
+	bool used_name = false;
 
 	__evlist__for_each_entry(list, evsel) {
 		if (!evsel->name) {
-			evsel->name = strdup(name);
+			evsel->name = used_name ? strdup(name) : name;
+			used_name = true;
 			if (!evsel->name)
 				return -ENOMEM;
 		}
 	}
-
+	if (!used_name)
+		free(name);
 	return 0;
 }
 
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index 0bb5f0c80a5e..5695308efab9 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -207,7 +207,7 @@ int parse_events__modifier_event(struct parse_events_state *parse_state, void *l
 				 struct list_head *list, struct parse_events_modifier mod);
 int parse_events__modifier_group(struct parse_events_state *parse_state, void *loc,
 				 struct list_head *list, struct parse_events_modifier mod);
-int parse_events_name(struct list_head *list, const char *name);
+int parse_events__set_default_name(struct list_head *list, char *name);
 int parse_events_add_tracepoint(struct list_head *list, int *idx,
 				const char *sys, const char *event,
 				struct parse_events_error *error,
diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index 4aaf0c53d9b6..08ea2d845dc3 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -96,6 +96,11 @@ static int drv_str(yyscan_t scanner, int token)
 	return token;
 }
 
+/*
+ * Use yyless to return all the characaters to the input. Update the column for
+ * location debugging. If __alloc is non-zero set yylval to the text for the
+ * returned token's value.
+ */
 #define REWIND(__alloc)				\
 do {								\
 	YYSTYPE *__yylval = parse_events_get_lval(yyscanner);	\
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 6f1042272dda..68b3b06c7ff0 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -247,10 +247,14 @@ event_name
 event_name:
 PE_EVENT_NAME event_def
 {
-	int err;
+	/*
+	 * When an event is parsed the text is rewound and the entire text of
+	 * the event is set to the str of PE_EVENT_NAME token matched here. If
+	 * no name was on an event via a term, set the name to the entire text
+	 * taking ownership of the allocation.
+	 */
+	int err = parse_events__set_default_name($2, $1);
 
-	err = parse_events_name($2, $1);
-	free($1);
 	if (err) {
 		free_list_evsel($2);
 		YYNOMEM;
-- 
2.44.0.683.g7961c838ac-goog

Re: [PATCH v2 00/16] Consistently prefer sysfs/json events

[Atish Kumar Patra]

On Mon, Apr 15, 2024 at 11:15 PM Ian Rogers <irogers@google.com> wrote:
>
> As discussed in:
> https://lore.kernel.org/lkml/20240217005738.3744121-1-atishp@rivosinc.com/
> preferring sysfs/json events consistently (with or without a given
> PMU) will enable RISC-V's hope to customize legacy events in the perf
> tool.
>

Thanks for remapping legacy events in a generic way. This looks great
and got rid of my
ugly arch specific way of remapping.  Is there a good way for the
driver (e.g via sysfs) to tell the perf tool
whether to remap the legacy event or not ?

In RISC-V the legacy systems without the new ISA extension may not
want to remap if running
the latest kernel.

I described the problem in detail in the original thread as well.
https://lore.kernel.org/lkml/63d73f09-84e5-49e1-99f5-60f414b22d70@rivosinc.com/

FWIW, for the entire series.
Tested-by: Atish Patra <atishp@rivosinc.com>

> Some minor clean-up is performed on the way.
>
> v2. Additional cleanup particularly adding better error messages. Fix
>     some line length issues on the earlier patches.
>
> Ian Rogers (16):
>   perf parse-events: Factor out '<event_or_pmu>/.../' parsing
>   perf parse-events: Directly pass PMU to parse_events_add_pmu
>   perf parse-events: Avoid copying an empty list
>   perf pmu: Refactor perf_pmu__match
>   perf tests parse-events: Use branches rather than cache-references
>   perf parse-events: Legacy cache names on all PMUs and lower priority
>   perf parse-events: Handle PE_TERM_HW in name_or_raw
>   perf parse-events: Constify parse_events_add_numeric
>   perf parse-events: Prefer sysfs/json hardware events over legacy
>   perf parse-events: Inline parse_events_update_lists
>   perf parse-events: Improve error message for bad numbers
>   perf parse-events: Inline parse_events_evlist_error
>   perf parse-events: Improvements to modifier parsing
>   perf parse-event: Constify event_symbol arrays
>   perf parse-events: Minor grouping tidy up
>   perf parse-events: Tidy the setting of the default event name
>
>  tools/perf/tests/parse-events.c |   6 +-
>  tools/perf/util/parse-events.c  | 482 ++++++++++++++++----------------
>  tools/perf/util/parse-events.h  |  49 ++--
>  tools/perf/util/parse-events.l  | 196 +++++++++----
>  tools/perf/util/parse-events.y  | 261 +++++++----------
>  tools/perf/util/pmu.c           |  27 +-
>  tools/perf/util/pmu.h           |   2 +-
>  7 files changed, 540 insertions(+), 483 deletions(-)
>
> --
> 2.44.0.683.g7961c838ac-goog
>

Re: [PATCH v2 00/16] Consistently prefer sysfs/json events

[Ian Rogers]

On Tue, Apr 23, 2024 at 5:28 PM Atish Kumar Patra <atishp@rivosinc.com> wrote:
>
> On Mon, Apr 15, 2024 at 11:15 PM Ian Rogers <irogers@google.com> wrote:
> >
> > As discussed in:
> > https://lore.kernel.org/lkml/20240217005738.3744121-1-atishp@rivosinc.com/
> > preferring sysfs/json events consistently (with or without a given
> > PMU) will enable RISC-V's hope to customize legacy events in the perf
> > tool.
> >
>
> Thanks for remapping legacy events in a generic way. This looks great
> and got rid of my
> ugly arch specific way of remapping.  Is there a good way for the
> driver (e.g via sysfs) to tell the perf tool
> whether to remap the legacy event or not ?
>
> In RISC-V the legacy systems without the new ISA extension may not
> want to remap if running
> the latest kernel.
>
> I described the problem in detail in the original thread as well.
> https://lore.kernel.org/lkml/63d73f09-84e5-49e1-99f5-60f414b22d70@rivosinc.com/

So the sysfs/json events have priority over the legacy hardware events
with this patch series. I'm not clear on your question but here are
some scenarios:

1) for a vendor/model with a CPUID json files want to be used:
1.1) the driver shouldn't advertise the events /sys/devices/<pmu name>/events
1.2) the json in the perf tool needs to have a mapfile.csv entry for
the cpuid to a model directory containing the event json. In the
directory the legacy events should be defined.

2) for a vendor/model with a CPUID the driver files should be used:
2.1) the driver should advertise the events in /sys/devices/<pmu name>/events
2.2) in the json for the CPUID avoid redefining the events

3) for a vendor/model with a CPUID the legacy events should be used:
3.1) the driver shouldn't advertise the events in /sys/devices/<pmu name>/events
3.2) in the json for the CPUID avoid defining the events

Are you asking to have both sysfs and json events for a model? In this
case, which have priority over the other? It's possible in the pmu.c
code to have a prioritized lookup either from json then sysfs or
vice-versa, at the moment it is first come first served. To some
extent this can be seen on Intel uncore events where there are both
sysfs and json events with the same config, when we reverse map if the
sysfs name is loaded then it is reverse mapped in verbose log or by
perf trace, whilst typically I think the json name is reverse mapped.
Are you asking for the search order to be configurable by the driver?
In the past I've considered that the search order may be configured in
the tool and the user may want to provide their own directory
containing additional events and metrics.

> FWIW, for the entire series.
> Tested-by: Atish Patra <atishp@rivosinc.com>

Thanks, I think we can go ahead to land this. Kan's comment was to ask
for a follow up changing max_precise behavior and I'm hesitant to do
two behavior changes in 1 patch series.

Ian

> > Some minor clean-up is performed on the way.
> >
> > v2. Additional cleanup particularly adding better error messages. Fix
> >     some line length issues on the earlier patches.
> >
> > Ian Rogers (16):
> >   perf parse-events: Factor out '<event_or_pmu>/.../' parsing
> >   perf parse-events: Directly pass PMU to parse_events_add_pmu
> >   perf parse-events: Avoid copying an empty list
> >   perf pmu: Refactor perf_pmu__match
> >   perf tests parse-events: Use branches rather than cache-references
> >   perf parse-events: Legacy cache names on all PMUs and lower priority
> >   perf parse-events: Handle PE_TERM_HW in name_or_raw
> >   perf parse-events: Constify parse_events_add_numeric
> >   perf parse-events: Prefer sysfs/json hardware events over legacy
> >   perf parse-events: Inline parse_events_update_lists
> >   perf parse-events: Improve error message for bad numbers
> >   perf parse-events: Inline parse_events_evlist_error
> >   perf parse-events: Improvements to modifier parsing
> >   perf parse-event: Constify event_symbol arrays
> >   perf parse-events: Minor grouping tidy up
> >   perf parse-events: Tidy the setting of the default event name
> >
> >  tools/perf/tests/parse-events.c |   6 +-
> >  tools/perf/util/parse-events.c  | 482 ++++++++++++++++----------------
> >  tools/perf/util/parse-events.h  |  49 ++--
> >  tools/perf/util/parse-events.l  | 196 +++++++++----
> >  tools/perf/util/parse-events.y  | 261 +++++++----------
> >  tools/perf/util/pmu.c           |  27 +-
> >  tools/perf/util/pmu.h           |   2 +-
> >  7 files changed, 540 insertions(+), 483 deletions(-)
> >
> > --
> > 2.44.0.683.g7961c838ac-goog
> >

Re: [PATCH v2 00/16] Consistently prefer sysfs/json events

[Liang, Kan]

On 2024-04-16 2:15 a.m., Ian Rogers wrote:
> As discussed in:
> https://lore.kernel.org/lkml/20240217005738.3744121-1-atishp@rivosinc.com/
> preferring sysfs/json events consistently (with or without a given
> PMU) will enable RISC-V's hope to customize legacy events in the perf
> tool.
> 
> Some minor clean-up is performed on the way.
> 
> v2. Additional cleanup particularly adding better error messages. Fix
>     some line length issues on the earlier patches.
> 
> Ian Rogers (16):
>   perf parse-events: Factor out '<event_or_pmu>/.../' parsing
>   perf parse-events: Directly pass PMU to parse_events_add_pmu
>   perf parse-events: Avoid copying an empty list
>   perf pmu: Refactor perf_pmu__match
>   perf tests parse-events: Use branches rather than cache-references
>   perf parse-events: Legacy cache names on all PMUs and lower priority
>   perf parse-events: Handle PE_TERM_HW in name_or_raw
>   perf parse-events: Constify parse_events_add_numeric
>   perf parse-events: Prefer sysfs/json hardware events over legacy
>   perf parse-events: Inline parse_events_update_lists
>   perf parse-events: Improve error message for bad numbers
>   perf parse-events: Inline parse_events_evlist_error
>   perf parse-events: Improvements to modifier parsing
>   perf parse-event: Constify event_symbol arrays
>   perf parse-events: Minor grouping tidy up
>   perf parse-events: Tidy the setting of the default event name
> 
>  tools/perf/tests/parse-events.c |   6 +-
>  tools/perf/util/parse-events.c  | 482 ++++++++++++++++----------------
>  tools/perf/util/parse-events.h  |  49 ++--
>  tools/perf/util/parse-events.l  | 196 +++++++++----
>  tools/perf/util/parse-events.y  | 261 +++++++----------
>  tools/perf/util/pmu.c           |  27 +-
>  tools/perf/util/pmu.h           |   2 +-
>  7 files changed, 540 insertions(+), 483 deletions(-)
> 


The series looks good to me.

Reviewed-by: Kan Liang <kan.liang@linux.intel.com>

Thanks,
Kan