[PATCH v1 1/6] perf parse-events: Fixes relating to no_value terms

[PATCH v1 1/6] perf parse-events: Fixes relating to no_value terms

[Ian Rogers]

A term may have no value in which case it is assumed to have a value
of 1. It doesn't just apply to alias/event terms so change the
parse_events_term__to_strbuf assert.

Commit 99e7138eb789 ("perf tools: Fail on using multiple bits long
terms without value") made it so that no_value terms could only be for
a single bit. Prior to commit 64199ae4b8a3 ("perf parse-events: Fix
propagation of term's no_value when cloning") this missed a test case
where config1 had no_value.

Fixes: 64199ae4b8a3 ("perf parse-events: Fix propagation of term's no_value when cloning")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/parse-events.c | 2 +-
 tools/perf/util/parse-events.c  | 2 +-
 tools/perf/util/parse-events.h  | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
index d86076d575ed..d47f1f871164 100644
--- a/tools/perf/tests/parse-events.c
+++ b/tools/perf/tests/parse-events.c
@@ -2170,7 +2170,7 @@ static const struct evlist_test test__events[] = {
 
 static const struct evlist_test test__events_pmu[] = {
 	{
-		.name  = "cpu/config=10,config1,config2=3,period=1000/u",
+		.name  = "cpu/config=10,config1=1,config2=3,period=1000/u",
 		.valid = test__pmu_cpu_valid,
 		.check = test__checkevent_pmu,
 		/* 0 */
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 68fe2c4ff49f..65608a3cba81 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -2607,7 +2607,7 @@ int parse_events_term__to_strbuf(struct list_head *term_list, struct strbuf *sb)
 
 		if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM)
 			if (term->no_value) {
-				assert(term->type_term == PARSE_EVENTS__TERM_TYPE_USER);
+				assert(term->val.num == 1);
 				ret = strbuf_addf(sb, "%s", term->config);
 			} else
 				ret = strbuf_addf(sb, "%s=%#"PRIx64, term->config, term->val.num);
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index 855b0725c5d4..594e5d2dc67f 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -124,8 +124,8 @@ struct parse_events_term {
 	 */
 	bool weak;
 	/**
-	 * @no_value: Is there no value. TODO: this should really be part of
-	 * type_val.
+	 * @no_value: Is there no value. If a numeric term has no value then the
+	 * value is assumed to be 1. An event name also has no value.
 	 */
 	bool no_value;
 };
-- 
2.42.0.283.g2d96d420d3-goog

[PATCH v1 2/6] perf parse-events: Remove unnecessary __maybe_unused

[Ian Rogers]

The parameter head_terms is always used in get_config_terms.

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

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 65608a3cba81..e9e3623f3fed 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -34,8 +34,7 @@
 #ifdef PARSER_DEBUG
 extern int parse_events_debug;
 #endif
-static int get_config_terms(struct list_head *head_config,
-			    struct list_head *head_terms __maybe_unused);
+static int get_config_terms(struct list_head *head_config, struct list_head *head_terms);
 
 struct event_symbol event_symbols_hw[PERF_COUNT_HW_MAX] = {
 	[PERF_COUNT_HW_CPU_CYCLES] = {
@@ -1079,8 +1078,7 @@ static int config_attr(struct perf_event_attr *attr,
 	return 0;
 }
 
-static int get_config_terms(struct list_head *head_config,
-			    struct list_head *head_terms __maybe_unused)
+static int get_config_terms(struct list_head *head_config, struct list_head *head_terms)
 {
 #define ADD_CONFIG_TERM(__type, __weak)				\
 	struct evsel_config_term *__t;			\
-- 
2.42.0.283.g2d96d420d3-goog

[PATCH v1 3/6] perf parse-events: Tidy up str parameter

[Ian Rogers]

Add a const and rename str to event_name.

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

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index e9e3623f3fed..283c559a35b4 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1482,7 +1482,7 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
 }
 
 int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
-			       char *str, struct list_head *head,
+			       const char *event_name, struct list_head *head,
 			       struct list_head **listp, void *loc_)
 {
 	struct parse_events_term *term;
@@ -1502,7 +1502,8 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
 
 		INIT_LIST_HEAD(head);
 	}
-	config = strdup(str);
+
+	config = strdup(event_name);
 	if (!config)
 		goto out_err;
 
@@ -1528,7 +1529,7 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
 		if (parse_events__filter_pmu(parse_state, pmu))
 			continue;
 
-		if (!perf_pmu__have_event(pmu, str))
+		if (!perf_pmu__have_event(pmu, event_name))
 			continue;
 
 		auto_merge_stats = perf_pmu__auto_merge_stats(pmu);
@@ -1539,7 +1540,7 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
 
 			strbuf_init(&sb, /*hint=*/ 0);
 			parse_events_term__to_strbuf(orig_head, &sb);
-			pr_debug("%s -> %s/%s/\n", str, pmu->name, sb.buf);
+			pr_debug("%s -> %s/%s/\n", event_name, pmu->name, sb.buf);
 			strbuf_release(&sb);
 			ok++;
 		}
@@ -1547,13 +1548,13 @@ 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, str, head,
+		if (!parse_events_add_pmu(parse_state, list, event_name, head,
 					  /*auto_merge_stats=*/true, loc)) {
 			struct strbuf sb;
 
 			strbuf_init(&sb, /*hint=*/ 0);
 			parse_events_term__to_strbuf(head, &sb);
-			pr_debug("%s -> %s/%s/\n", str, "fake_pmu", sb.buf);
+			pr_debug("%s -> %s/%s/\n", event_name, "fake_pmu", sb.buf);
 			strbuf_release(&sb);
 			ok++;
 		}
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index 594e5d2dc67f..36a67ef7b35a 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -217,7 +217,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,
-			       char *str,
+			       const char *event_name,
 			       struct list_head *head_config,
 			       struct list_head **listp, void *loc);
 
-- 
2.42.0.283.g2d96d420d3-goog

[PATCH v1 4/6] perf parse-events: Avoid enum casts

[Ian Rogers]

Add term_type to union of values returned by the lexer to avoid casts
to and from an integer.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/parse-events.l |  2 +-
 tools/perf/util/parse-events.y | 25 +++++++++++--------------
 2 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index 4ef4b6f171a0..7bdf0565a92c 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -120,7 +120,7 @@ static int term(yyscan_t scanner, enum parse_events__term_type type)
 {
 	YYSTYPE *yylval = parse_events_get_lval(scanner);
 
-	yylval->num = type;
+	yylval->term_type = type;
 	return PE_TERM;
 }
 
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 4a305df61f74..4fae7847d13b 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -70,7 +70,7 @@ static void free_list_evsel(struct list_head* list_evsel)
 %type <num> PE_VALUE_SYM_HW
 %type <num> PE_VALUE_SYM_SW
 %type <num> PE_VALUE_SYM_TOOL
-%type <num> PE_TERM
+%type <term_type> PE_TERM
 %type <num> value_sym
 %type <str> PE_RAW
 %type <str> PE_NAME
@@ -111,6 +111,7 @@ static void free_list_evsel(struct list_head* list_evsel)
 {
 	char *str;
 	u64 num;
+	enum parse_events__term_type term_type;
 	struct list_head *list_evsel;
 	struct list_head *list_terms;
 	struct parse_events_term *term;
@@ -778,8 +779,7 @@ PE_TERM_HW
 PE_TERM '=' name_or_legacy
 {
 	struct parse_events_term *term;
-	int err = parse_events_term__str(&term, (enum parse_events__term_type)$1,
-					/*config=*/NULL, $3, &@1, &@3);
+	int err = parse_events_term__str(&term, $1, /*config=*/NULL, $3, &@1, &@3);
 
 	if (err) {
 		free($3);
@@ -791,8 +791,7 @@ PE_TERM '=' name_or_legacy
 PE_TERM '=' PE_TERM_HW
 {
 	struct parse_events_term *term;
-	int err = parse_events_term__str(&term, (enum parse_events__term_type)$1,
-					 /*config=*/NULL, $3.str, &@1, &@3);
+	int err = parse_events_term__str(&term, $1, /*config=*/NULL, $3.str, &@1, &@3);
 
 	if (err) {
 		free($3.str);
@@ -804,10 +803,7 @@ PE_TERM '=' PE_TERM_HW
 PE_TERM '=' PE_TERM
 {
 	struct parse_events_term *term;
-	int err = parse_events_term__term(&term,
-					  (enum parse_events__term_type)$1,
-					  (enum parse_events__term_type)$3,
-					  &@1, &@3);
+	int err = parse_events_term__term(&term, $1, $3, &@1, &@3);
 
 	if (err)
 		PE_ABORT(err);
@@ -818,8 +814,9 @@ PE_TERM '=' PE_TERM
 PE_TERM '=' PE_VALUE
 {
 	struct parse_events_term *term;
-	int err = parse_events_term__num(&term, (enum parse_events__term_type)$1,
-					 /*config=*/NULL, $3, /*novalue=*/false, &@1, &@3);
+	int err = parse_events_term__num(&term, $1,
+					 /*config=*/NULL, $3, /*novalue=*/false,
+					 &@1, &@3);
 
 	if (err)
 		PE_ABORT(err);
@@ -830,9 +827,9 @@ PE_TERM '=' PE_VALUE
 PE_TERM
 {
 	struct parse_events_term *term;
-	int err = parse_events_term__num(&term, (enum parse_events__term_type)$1,
-					/*config=*/NULL, /*num=*/1, /*novalue=*/true,
-					&@1, /*loc_val=*/NULL);
+	int err = parse_events_term__num(&term, $1,
+					 /*config=*/NULL, /*num=*/1, /*novalue=*/true,
+					 &@1, /*loc_val=*/NULL);
 
 	if (err)
 		PE_ABORT(err);
-- 
2.42.0.283.g2d96d420d3-goog

[PATCH v1 5/6] perf parse-events: Copy fewer term lists

[Ian Rogers]

When trying to add events to multiple PMUs the term list is copied
first as adding the event will rewrite the event's name term into the
sysfs and/or json encoding terms (see perf_pmu__check_alias). Change
the parse events add API so the passed in term list is const, then
copy the list when modification is necessary.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/parse-events.c | 108 ++++++++++++++++++---------------
 tools/perf/util/parse-events.h |   7 +--
 tools/perf/util/parse-events.y |  17 +-----
 3 files changed, 65 insertions(+), 67 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 283c559a35b4..06a844bcce4a 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -35,6 +35,7 @@
 extern int parse_events_debug;
 #endif
 static int get_config_terms(struct list_head *head_config, struct list_head *head_terms);
+static int parse_events_terms__copy(const struct list_head *src, struct list_head *dest);
 
 struct event_symbol event_symbols_hw[PERF_COUNT_HW_MAX] = {
 	[PERF_COUNT_HW_CPU_CYCLES] = {
@@ -1367,7 +1368,7 @@ static bool config_term_percore(struct list_head *config_terms)
 
 int parse_events_add_pmu(struct parse_events_state *parse_state,
 			 struct list_head *list, const char *name,
-			 struct list_head *head_config,
+			 const struct list_head *const_head_terms,
 			 bool auto_merge_stats, void *loc_)
 {
 	struct perf_event_attr attr;
@@ -1377,6 +1378,7 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
 	struct parse_events_error *err = parse_state->error;
 	YYLTYPE *loc = loc_;
 	LIST_HEAD(config_terms);
+	LIST_HEAD(head_terms);
 
 	pmu = parse_state->fake_pmu ?: perf_pmus__find(name);
 
@@ -1390,32 +1392,37 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
 		return -EINVAL;
 	}
 
+	if (const_head_terms) {
+		int ret = parse_events_terms__copy(const_head_terms, &head_terms);
+
+		if (ret)
+			return ret;
+	}
+
 	if (verbose > 1) {
 		struct strbuf sb;
 
 		strbuf_init(&sb, /*hint=*/ 0);
-		if (pmu->selectable && !head_config) {
+		if (pmu->selectable && list_empty(&head_terms)) {
 			strbuf_addf(&sb, "%s//", name);
 		} else {
 			strbuf_addf(&sb, "%s/", name);
-			parse_events_term__to_strbuf(head_config, &sb);
+			parse_events_term__to_strbuf(&head_terms, &sb);
 			strbuf_addch(&sb, '/');
 		}
 		fprintf(stderr, "Attempt to add: %s\n", sb.buf);
 		strbuf_release(&sb);
 	}
-	if (head_config)
-		fix_raw(head_config, pmu);
+	fix_raw(&head_terms, pmu);
 
 	if (pmu->default_config) {
-		memcpy(&attr, pmu->default_config,
-		       sizeof(struct perf_event_attr));
+		memcpy(&attr, pmu->default_config, sizeof(struct perf_event_attr));
 	} else {
 		memset(&attr, 0, sizeof(attr));
 	}
 	attr.type = pmu->type;
 
-	if (!head_config) {
+	if (list_empty(&head_terms)) {
 		evsel = __add_event(list, &parse_state->idx, &attr,
 				    /*init_attr=*/true, /*name=*/NULL,
 				    /*metric_id=*/NULL, pmu,
@@ -1424,14 +1431,16 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
 		return evsel ? 0 : -ENOMEM;
 	}
 
-	if (!parse_state->fake_pmu && perf_pmu__check_alias(pmu, head_config, &info, err))
+	if (!parse_state->fake_pmu && perf_pmu__check_alias(pmu, &head_terms, &info, err)) {
+		parse_events_terms__purge(&head_terms);
 		return -EINVAL;
+	}
 
 	if (verbose > 1) {
 		struct strbuf sb;
 
 		strbuf_init(&sb, /*hint=*/ 0);
-		parse_events_term__to_strbuf(head_config, &sb);
+		parse_events_term__to_strbuf(&head_terms, &sb);
 		fprintf(stderr, "..after resolving event: %s/%s/\n", name, sb.buf);
 		strbuf_release(&sb);
 	}
@@ -1440,39 +1449,52 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
 	 * Configure hardcoded terms first, no need to check
 	 * return value when called with fail == 0 ;)
 	 */
-	if (config_attr(&attr, head_config, parse_state->error, config_term_pmu))
+	if (config_attr(&attr, &head_terms, parse_state->error, config_term_pmu)) {
+		parse_events_terms__purge(&head_terms);
 		return -EINVAL;
+	}
 
-	if (get_config_terms(head_config, &config_terms))
+	if (get_config_terms(&head_terms, &config_terms)) {
+		parse_events_terms__purge(&head_terms);
 		return -ENOMEM;
+	}
 
 	/*
 	 * When using default config, record which bits of attr->config were
 	 * changed by the user.
 	 */
-	if (pmu->default_config && get_config_chgs(pmu, head_config, &config_terms))
+	if (pmu->default_config && get_config_chgs(pmu, &head_terms, &config_terms)) {
+		parse_events_terms__purge(&head_terms);
 		return -ENOMEM;
+	}
 
-	if (!parse_state->fake_pmu && perf_pmu__config(pmu, &attr, head_config, parse_state->error)) {
+	if (!parse_state->fake_pmu &&
+	    perf_pmu__config(pmu, &attr, &head_terms, parse_state->error)) {
 		free_config_terms(&config_terms);
+		parse_events_terms__purge(&head_terms);
 		return -EINVAL;
 	}
 
 	evsel = __add_event(list, &parse_state->idx, &attr, /*init_attr=*/true,
-			    get_config_name(head_config),
-			    get_config_metric_id(head_config), pmu,
+			    get_config_name(&head_terms),
+			    get_config_metric_id(&head_terms), pmu,
 			    &config_terms, auto_merge_stats, /*cpu_list=*/NULL);
-	if (!evsel)
+	if (!evsel) {
+		parse_events_terms__purge(&head_terms);
 		return -ENOMEM;
+	}
 
 	if (evsel->name)
 		evsel->use_config_name = true;
 
 	evsel->percore = config_term_percore(&evsel->config_terms);
 
-	if (parse_state->fake_pmu)
+	if (parse_state->fake_pmu) {
+		parse_events_terms__purge(&head_terms);
 		return 0;
+	}
 
+	parse_events_terms__purge(&head_terms);
 	free((char *)evsel->unit);
 	evsel->unit = strdup(info.unit);
 	evsel->scale = info.scale;
@@ -1482,25 +1504,25 @@ 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, struct list_head *head,
+			       const char *event_name,
+			       const struct list_head *const_head_terms,
 			       struct list_head **listp, void *loc_)
 {
 	struct parse_events_term *term;
 	struct list_head *list = NULL;
-	struct list_head *orig_head = NULL;
 	struct perf_pmu *pmu = NULL;
 	YYLTYPE *loc = loc_;
 	int ok = 0;
 	const char *config;
+	LIST_HEAD(head_terms);
 
 	*listp = NULL;
 
-	if (!head) {
-		head = malloc(sizeof(struct list_head));
-		if (!head)
-			goto out_err;
+	if (const_head_terms) {
+		int ret = parse_events_terms__copy(const_head_terms, &head_terms);
 
-		INIT_LIST_HEAD(head);
+		if (ret)
+			return ret;
 	}
 
 	config = strdup(event_name);
@@ -1514,7 +1536,7 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
 		zfree(&config);
 		goto out_err;
 	}
-	list_add_tail(&term->list, head);
+	list_add_tail(&term->list, &head_terms);
 
 	/* Add it for all PMUs that support the alias */
 	list = malloc(sizeof(struct list_head));
@@ -1533,27 +1555,25 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
 			continue;
 
 		auto_merge_stats = perf_pmu__auto_merge_stats(pmu);
-		parse_events_copy_term_list(head, &orig_head);
 		if (!parse_events_add_pmu(parse_state, list, pmu->name,
-					  orig_head, auto_merge_stats, loc)) {
+					  &head_terms, auto_merge_stats, loc)) {
 			struct strbuf sb;
 
 			strbuf_init(&sb, /*hint=*/ 0);
-			parse_events_term__to_strbuf(orig_head, &sb);
+			parse_events_term__to_strbuf(&head_terms, &sb);
 			pr_debug("%s -> %s/%s/\n", event_name, pmu->name, sb.buf);
 			strbuf_release(&sb);
 			ok++;
 		}
-		parse_events_terms__delete(orig_head);
 	}
 
 	if (parse_state->fake_pmu) {
-		if (!parse_events_add_pmu(parse_state, list, event_name, head,
+		if (!parse_events_add_pmu(parse_state, list, event_name, &head_terms,
 					  /*auto_merge_stats=*/true, loc)) {
 			struct strbuf sb;
 
 			strbuf_init(&sb, /*hint=*/ 0);
-			parse_events_term__to_strbuf(head, &sb);
+			parse_events_term__to_strbuf(&head_terms, &sb);
 			pr_debug("%s -> %s/%s/\n", event_name, "fake_pmu", sb.buf);
 			strbuf_release(&sb);
 			ok++;
@@ -1561,12 +1581,12 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
 	}
 
 out_err:
+	parse_events_terms__purge(&head_terms);
 	if (ok)
 		*listp = list;
 	else
 		free(list);
 
-	parse_events_terms__delete(head);
 	return ok ? 0 : -1;
 }
 
@@ -2543,27 +2563,19 @@ void parse_events_term__delete(struct parse_events_term *term)
 	free(term);
 }
 
-int parse_events_copy_term_list(struct list_head *old,
-				 struct list_head **new)
+static int parse_events_terms__copy(const struct list_head *src, struct list_head *dest)
 {
-	struct parse_events_term *term, *n;
-	int ret;
-
-	if (!old) {
-		*new = NULL;
-		return 0;
-	}
+	struct parse_events_term *term;
 
-	*new = malloc(sizeof(struct list_head));
-	if (!*new)
-		return -ENOMEM;
-	INIT_LIST_HEAD(*new);
+	list_for_each_entry (term, src, list) {
+		struct parse_events_term *n;
+		int ret;
 
-	list_for_each_entry (term, old, list) {
 		ret = parse_events_term__clone(&n, term);
 		if (ret)
 			return ret;
-		list_add_tail(&n->list, *new);
+
+		list_add_tail(&n->list, dest);
 	}
 	return 0;
 }
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index 36a67ef7b35a..e6612856e881 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -209,7 +209,7 @@ int parse_events_add_breakpoint(struct parse_events_state *parse_state,
 				struct list_head *head_config);
 int parse_events_add_pmu(struct parse_events_state *parse_state,
 			 struct list_head *list, const char *name,
-			 struct list_head *head_config,
+			 const struct list_head *const_head_terms,
 			bool auto_merge_stats, void *loc);
 
 struct evsel *parse_events__add_event(int idx, struct perf_event_attr *attr,
@@ -218,12 +218,9 @@ struct evsel *parse_events__add_event(int idx, struct perf_event_attr *attr,
 
 int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
 			       const char *event_name,
-			       struct list_head *head_config,
+			       const struct list_head *head_terms,
 			       struct list_head **listp, void *loc);
 
-int parse_events_copy_term_list(struct list_head *old,
-				 struct list_head **new);
-
 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 4fae7847d13b..1ee65f36762c 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -274,23 +274,18 @@ event_pmu:
 PE_NAME opt_pmu_config
 {
 	struct parse_events_state *parse_state = _parse_state;
-	struct list_head *list = NULL, *orig_terms = NULL, *terms= NULL;
+	/* List of created evsels. */
+	struct list_head *list = NULL;
 	char *pattern = NULL;
 
 #define CLEANUP						\
 	do {						\
 		parse_events_terms__delete($2);		\
-		parse_events_terms__delete(orig_terms);	\
 		free(list);				\
 		free($1);				\
 		free(pattern);				\
 	} while(0)
 
-	if (parse_events_copy_term_list($2, &orig_terms)) {
-		CLEANUP;
-		YYNOMEM;
-	}
-
 	list = alloc_list();
 	if (!list) {
 		CLEANUP;
@@ -320,16 +315,11 @@ PE_NAME opt_pmu_config
 			    !perf_pmu__match(pattern, pmu->alias_name, $1)) {
 				bool auto_merge_stats = perf_pmu__auto_merge_stats(pmu);
 
-				if (parse_events_copy_term_list(orig_terms, &terms)) {
-					CLEANUP;
-					YYNOMEM;
-				}
-				if (!parse_events_add_pmu(parse_state, list, pmu->name, terms,
+				if (!parse_events_add_pmu(parse_state, list, pmu->name, $2,
 							  auto_merge_stats, &@1)) {
 					ok++;
 					parse_state->wild_card_pmus = true;
 				}
-				parse_events_terms__delete(terms);
 			}
 		}
 
@@ -337,7 +327,6 @@ PE_NAME opt_pmu_config
 			/* Failure to add, assume $1 is an event name. */
 			zfree(&list);
 			ok = !parse_events_multi_pmu_add(parse_state, $1, $2, &list, &@1);
-			$2 = NULL;
 		}
 		if (!ok) {
 			struct parse_events_error *error = parse_state->error;
-- 
2.42.0.283.g2d96d420d3-goog

[PATCH v1 6/6] perf parse-events: Add struct parse_events_terms

[Ian Rogers]

parse_events_terms existed in function names but was passed a
list_head. As many parse_events functions take an evsel_config list as
well as a parse_event_term list, and the naming head_terms and
head_config is inconsistent, there's a potential to switch the lists
and get errors. Introduce a struct parse_events_terms, that just wraps
a list_head, to avoid this. Add the regular init/exit functions and
transition the code to use them.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/arch/x86/util/intel-pt.c |  15 +--
 tools/perf/tests/parse-events.c     |  12 +--
 tools/perf/tests/pmu.c              |  23 ++--
 tools/perf/util/parse-events.c      | 158 +++++++++++++++-------------
 tools/perf/util/parse-events.h      |  29 +++--
 tools/perf/util/parse-events.y      |  12 +--
 tools/perf/util/pmu.c               |  49 ++++-----
 tools/perf/util/pmu.h               |   6 +-
 8 files changed, 160 insertions(+), 144 deletions(-)

diff --git a/tools/perf/arch/x86/util/intel-pt.c b/tools/perf/arch/x86/util/intel-pt.c
index 31807791589e..b923bca939d9 100644
--- a/tools/perf/arch/x86/util/intel-pt.c
+++ b/tools/perf/arch/x86/util/intel-pt.c
@@ -64,28 +64,23 @@ static int intel_pt_parse_terms_with_default(struct perf_pmu *pmu,
 					     const char *str,
 					     u64 *config)
 {
-	struct list_head *terms;
+	struct parse_events_terms terms;
 	struct perf_event_attr attr = { .size = 0, };
 	int err;
 
-	terms = malloc(sizeof(struct list_head));
-	if (!terms)
-		return -ENOMEM;
-
-	INIT_LIST_HEAD(terms);
-
-	err = parse_events_terms(terms, str, /*input=*/ NULL);
+	parse_events_terms__init(&terms);
+	err = parse_events_terms(&terms, str, /*input=*/ NULL);
 	if (err)
 		goto out_free;
 
 	attr.config = *config;
-	err = perf_pmu__config_terms(pmu, &attr, terms, /*zero=*/true, /*err=*/NULL);
+	err = perf_pmu__config_terms(pmu, &attr, &terms, /*zero=*/true, /*err=*/NULL);
 	if (err)
 		goto out_free;
 
 	*config = attr.config;
 out_free:
-	parse_events_terms__delete(terms);
+	parse_events_terms__exit(&terms);
 	return err;
 }
 
diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
index d47f1f871164..93796e885cef 100644
--- a/tools/perf/tests/parse-events.c
+++ b/tools/perf/tests/parse-events.c
@@ -771,12 +771,12 @@ static int test__checkevent_pmu_events_mix(struct evlist *evlist)
 	return TEST_OK;
 }
 
-static int test__checkterms_simple(struct list_head *terms)
+static int test__checkterms_simple(struct parse_events_terms *terms)
 {
 	struct parse_events_term *term;
 
 	/* config=10 */
-	term = list_entry(terms->next, struct parse_events_term, list);
+	term = list_entry(terms->terms.next, struct parse_events_term, list);
 	TEST_ASSERT_VAL("wrong type term",
 			term->type_term == PARSE_EVENTS__TERM_TYPE_CONFIG);
 	TEST_ASSERT_VAL("wrong type val",
@@ -2363,7 +2363,7 @@ static const struct evlist_test test__events_pmu[] = {
 
 struct terms_test {
 	const char *str;
-	int (*check)(struct list_head *terms);
+	int (*check)(struct parse_events_terms *terms);
 };
 
 static const struct terms_test test__terms[] = {
@@ -2467,11 +2467,11 @@ static int test__events2(struct test_suite *test __maybe_unused, int subtest __m
 
 static int test_term(const struct terms_test *t)
 {
-	struct list_head terms;
+	struct parse_events_terms terms;
 	int ret;
 
-	INIT_LIST_HEAD(&terms);
 
+	parse_events_terms__init(&terms);
 	ret = parse_events_terms(&terms, t->str, /*input=*/ NULL);
 	if (ret) {
 		pr_debug("failed to parse terms '%s', err %d\n",
@@ -2480,7 +2480,7 @@ static int test_term(const struct terms_test *t)
 	}
 
 	ret = t->check(&terms);
-	parse_events_terms__purge(&terms);
+	parse_events_terms__exit(&terms);
 
 	return ret;
 }
diff --git a/tools/perf/tests/pmu.c b/tools/perf/tests/pmu.c
index eb60e5f66859..8f18127d876a 100644
--- a/tools/perf/tests/pmu.c
+++ b/tools/perf/tests/pmu.c
@@ -128,30 +128,35 @@ static int test_format_dir_put(char *dir)
 	return system(buf);
 }
 
-static struct list_head *test_terms_list(void)
+static void add_test_terms(struct parse_events_terms *terms)
 {
-	static LIST_HEAD(terms);
 	unsigned int i;
 
-	for (i = 0; i < ARRAY_SIZE(test_terms); i++)
-		list_add_tail(&test_terms[i].list, &terms);
+	for (i = 0; i < ARRAY_SIZE(test_terms); i++) {
+		struct parse_events_term *clone;
 
-	return &terms;
+		parse_events_term__clone(&clone, &test_terms[i]);
+		list_add_tail(&clone->list, &terms->terms);
+	}
 }
 
 static int test__pmu(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
 {
 	char dir[PATH_MAX];
 	char *format;
-	struct list_head *terms = test_terms_list();
+	struct parse_events_terms terms;
 	struct perf_event_attr attr;
 	struct perf_pmu *pmu;
 	int fd;
 	int ret;
 
+	parse_events_terms__init(&terms);
+	add_test_terms(&terms);
 	pmu = zalloc(sizeof(*pmu));
-	if (!pmu)
+	if (!pmu) {
+		parse_events_terms__exit(&terms);
 		return -ENOMEM;
+	}
 
 	INIT_LIST_HEAD(&pmu->format);
 	INIT_LIST_HEAD(&pmu->aliases);
@@ -159,6 +164,7 @@ static int test__pmu(struct test_suite *test __maybe_unused, int subtest __maybe
 	format = test_format_dir_get(dir, sizeof(dir));
 	if (!format) {
 		free(pmu);
+		parse_events_terms__exit(&terms);
 		return -EINVAL;
 	}
 
@@ -175,7 +181,7 @@ static int test__pmu(struct test_suite *test __maybe_unused, int subtest __maybe
 	if (ret)
 		goto out;
 
-	ret = perf_pmu__config_terms(pmu, &attr, terms, /*zero=*/false, /*err=*/NULL);
+	ret = perf_pmu__config_terms(pmu, &attr, &terms, /*zero=*/false, /*err=*/NULL);
 	if (ret)
 		goto out;
 
@@ -191,6 +197,7 @@ static int test__pmu(struct test_suite *test __maybe_unused, int subtest __maybe
 out:
 	test_format_dir_put(format);
 	perf_pmu__delete(pmu);
+	parse_events_terms__exit(&terms);
 	return ret;
 }
 
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 06a844bcce4a..c56e07bd7dd6 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -34,8 +34,9 @@
 #ifdef PARSER_DEBUG
 extern int parse_events_debug;
 #endif
-static int get_config_terms(struct list_head *head_config, struct list_head *head_terms);
-static int parse_events_terms__copy(const struct list_head *src, struct list_head *dest);
+static int get_config_terms(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);
 
 struct event_symbol event_symbols_hw[PERF_COUNT_HW_MAX] = {
 	[PERF_COUNT_HW_CPU_CYCLES] = {
@@ -153,26 +154,27 @@ const char *event_type(int type)
 	return "unknown";
 }
 
-static char *get_config_str(struct list_head *head_terms, enum parse_events__term_type type_term)
+static char *get_config_str(struct parse_events_terms *head_terms,
+			    enum parse_events__term_type type_term)
 {
 	struct parse_events_term *term;
 
 	if (!head_terms)
 		return NULL;
 
-	list_for_each_entry(term, head_terms, list)
+	list_for_each_entry(term, &head_terms->terms, list)
 		if (term->type_term == type_term)
 			return term->val.str;
 
 	return NULL;
 }
 
-static char *get_config_metric_id(struct list_head *head_terms)
+static char *get_config_metric_id(struct parse_events_terms *head_terms)
 {
 	return get_config_str(head_terms, PARSE_EVENTS__TERM_TYPE_METRIC_ID);
 }
 
-static char *get_config_name(struct list_head *head_terms)
+static char *get_config_name(struct parse_events_terms *head_terms)
 {
 	return get_config_str(head_terms, PARSE_EVENTS__TERM_TYPE_NAME);
 }
@@ -188,11 +190,11 @@ static char *get_config_name(struct list_head *head_terms)
  * @config_terms: the list of terms that may contain a raw term.
  * @pmu: the PMU to scan for events from.
  */
-static void fix_raw(struct list_head *config_terms, struct perf_pmu *pmu)
+static void fix_raw(struct parse_events_terms *config_terms, struct perf_pmu *pmu)
 {
 	struct parse_events_term *term;
 
-	list_for_each_entry(term, config_terms, list) {
+	list_for_each_entry(term, &config_terms->terms, list) {
 		u64 num;
 
 		if (term->type_term != PARSE_EVENTS__TERM_TYPE_RAW)
@@ -356,7 +358,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 list_head *head,
+		       struct parse_events_terms *head,
 		       struct parse_events_error *err,
 		       config_term_func_t config_term);
 
@@ -442,7 +444,7 @@ bool parse_events__filter_pmu(const struct parse_events_state *parse_state,
 
 int parse_events_add_cache(struct list_head *list, int *idx, const char *name,
 			   struct parse_events_state *parse_state,
-			   struct list_head *head_config)
+			   struct parse_events_terms *head_config)
 {
 	struct perf_pmu *pmu = NULL;
 	bool found_supported = false;
@@ -520,7 +522,7 @@ static void tracepoint_error(struct parse_events_error *e, int err,
 static int add_tracepoint(struct list_head *list, int *idx,
 			  const char *sys_name, const char *evt_name,
 			  struct parse_events_error *err,
-			  struct list_head *head_config, void *loc_)
+			  struct parse_events_terms *head_config, void *loc_)
 {
 	YYLTYPE *loc = loc_;
 	struct evsel *evsel = evsel__newtp_idx(sys_name, evt_name, (*idx)++);
@@ -545,7 +547,7 @@ static int add_tracepoint(struct list_head *list, int *idx,
 static int add_tracepoint_multi_event(struct list_head *list, int *idx,
 				      const char *sys_name, const char *evt_name,
 				      struct parse_events_error *err,
-				      struct list_head *head_config, YYLTYPE *loc)
+				      struct parse_events_terms *head_config, YYLTYPE *loc)
 {
 	char *evt_path;
 	struct dirent *evt_ent;
@@ -593,7 +595,7 @@ static int add_tracepoint_multi_event(struct list_head *list, int *idx,
 static int add_tracepoint_event(struct list_head *list, int *idx,
 				const char *sys_name, const char *evt_name,
 				struct parse_events_error *err,
-				struct list_head *head_config, YYLTYPE *loc)
+				struct parse_events_terms *head_config, YYLTYPE *loc)
 {
 	return strpbrk(evt_name, "*?") ?
 		add_tracepoint_multi_event(list, idx, sys_name, evt_name,
@@ -605,7 +607,7 @@ static int add_tracepoint_event(struct list_head *list, int *idx,
 static int add_tracepoint_multi_sys(struct list_head *list, int *idx,
 				    const char *sys_name, const char *evt_name,
 				    struct parse_events_error *err,
-				    struct list_head *head_config, YYLTYPE *loc)
+				    struct parse_events_terms *head_config, YYLTYPE *loc)
 {
 	struct dirent *events_ent;
 	DIR *events_dir;
@@ -680,7 +682,7 @@ do {					\
 int parse_events_add_breakpoint(struct parse_events_state *parse_state,
 				struct list_head *list,
 				u64 addr, char *type, u64 len,
-				struct list_head *head_config __maybe_unused)
+				struct parse_events_terms *head_config)
 {
 	struct perf_event_attr attr;
 	LIST_HEAD(config_terms);
@@ -1066,20 +1068,20 @@ static int config_term_tracepoint(struct perf_event_attr *attr,
 #endif
 
 static int config_attr(struct perf_event_attr *attr,
-		       struct list_head *head,
+		       struct parse_events_terms *head,
 		       struct parse_events_error *err,
 		       config_term_func_t config_term)
 {
 	struct parse_events_term *term;
 
-	list_for_each_entry(term, head, list)
+	list_for_each_entry(term, &head->terms, list)
 		if (config_term(attr, term, err))
 			return -EINVAL;
 
 	return 0;
 }
 
-static int get_config_terms(struct list_head *head_config, struct list_head *head_terms)
+static int get_config_terms(struct parse_events_terms *head_config, struct list_head *head_terms)
 {
 #define ADD_CONFIG_TERM(__type, __weak)				\
 	struct evsel_config_term *__t;			\
@@ -1112,7 +1114,7 @@ do {								\
 
 	struct parse_events_term *term;
 
-	list_for_each_entry(term, head_config, list) {
+	list_for_each_entry(term, &head_config->terms, list) {
 		switch (term->type_term) {
 		case PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD:
 			ADD_CONFIG_TERM_VAL(PERIOD, period, term->val.num, term->weak);
@@ -1193,14 +1195,14 @@ do {								\
  * Add EVSEL__CONFIG_TERM_CFG_CHG where cfg_chg will have a bit set for
  * each bit of attr->config that the user has changed.
  */
-static int get_config_chgs(struct perf_pmu *pmu, struct list_head *head_config,
+static int get_config_chgs(struct perf_pmu *pmu, struct parse_events_terms *head_config,
 			   struct list_head *head_terms)
 {
 	struct parse_events_term *term;
 	u64 bits = 0;
 	int type;
 
-	list_for_each_entry(term, head_config, list) {
+	list_for_each_entry(term, &head_config->terms, list) {
 		switch (term->type_term) {
 		case PARSE_EVENTS__TERM_TYPE_USER:
 			type = perf_pmu__format_type(pmu, term->config);
@@ -1250,7 +1252,7 @@ static int get_config_chgs(struct perf_pmu *pmu, struct list_head *head_config,
 int parse_events_add_tracepoint(struct list_head *list, int *idx,
 				const char *sys, const char *event,
 				struct parse_events_error *err,
-				struct list_head *head_config, void *loc_)
+				struct parse_events_terms *head_config, void *loc_)
 {
 	YYLTYPE *loc = loc_;
 #ifdef HAVE_LIBTRACEEVENT
@@ -1283,7 +1285,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 list_head *head_config)
+				u64 config, struct parse_events_terms *head_config)
 {
 	struct perf_event_attr attr;
 	LIST_HEAD(config_terms);
@@ -1319,7 +1321,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 list_head *head_config,
+			     struct parse_events_terms *head_config,
 			     bool wildcard)
 {
 	struct perf_pmu *pmu = NULL;
@@ -1368,7 +1370,7 @@ static bool config_term_percore(struct list_head *config_terms)
 
 int parse_events_add_pmu(struct parse_events_state *parse_state,
 			 struct list_head *list, const char *name,
-			 const struct list_head *const_head_terms,
+			 const struct parse_events_terms *const_parsed_terms,
 			 bool auto_merge_stats, void *loc_)
 {
 	struct perf_event_attr attr;
@@ -1378,7 +1380,7 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
 	struct parse_events_error *err = parse_state->error;
 	YYLTYPE *loc = loc_;
 	LIST_HEAD(config_terms);
-	LIST_HEAD(head_terms);
+	struct parse_events_terms parsed_terms;
 
 	pmu = parse_state->fake_pmu ?: perf_pmus__find(name);
 
@@ -1392,8 +1394,9 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
 		return -EINVAL;
 	}
 
-	if (const_head_terms) {
-		int ret = parse_events_terms__copy(const_head_terms, &head_terms);
+	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;
@@ -1403,17 +1406,17 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
 		struct strbuf sb;
 
 		strbuf_init(&sb, /*hint=*/ 0);
-		if (pmu->selectable && list_empty(&head_terms)) {
+		if (pmu->selectable && list_empty(&parsed_terms.terms)) {
 			strbuf_addf(&sb, "%s//", name);
 		} else {
 			strbuf_addf(&sb, "%s/", name);
-			parse_events_term__to_strbuf(&head_terms, &sb);
+			parse_events_terms__to_strbuf(&parsed_terms, &sb);
 			strbuf_addch(&sb, '/');
 		}
 		fprintf(stderr, "Attempt to add: %s\n", sb.buf);
 		strbuf_release(&sb);
 	}
-	fix_raw(&head_terms, pmu);
+	fix_raw(&parsed_terms, pmu);
 
 	if (pmu->default_config) {
 		memcpy(&attr, pmu->default_config, sizeof(struct perf_event_attr));
@@ -1422,7 +1425,7 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
 	}
 	attr.type = pmu->type;
 
-	if (list_empty(&head_terms)) {
+	if (list_empty(&parsed_terms.terms)) {
 		evsel = __add_event(list, &parse_state->idx, &attr,
 				    /*init_attr=*/true, /*name=*/NULL,
 				    /*metric_id=*/NULL, pmu,
@@ -1431,8 +1434,8 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
 		return evsel ? 0 : -ENOMEM;
 	}
 
-	if (!parse_state->fake_pmu && perf_pmu__check_alias(pmu, &head_terms, &info, err)) {
-		parse_events_terms__purge(&head_terms);
+	if (!parse_state->fake_pmu && perf_pmu__check_alias(pmu, &parsed_terms, &info, err)) {
+		parse_events_terms__exit(&parsed_terms);
 		return -EINVAL;
 	}
 
@@ -1440,7 +1443,7 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
 		struct strbuf sb;
 
 		strbuf_init(&sb, /*hint=*/ 0);
-		parse_events_term__to_strbuf(&head_terms, &sb);
+		parse_events_terms__to_strbuf(&parsed_terms, &sb);
 		fprintf(stderr, "..after resolving event: %s/%s/\n", name, sb.buf);
 		strbuf_release(&sb);
 	}
@@ -1449,13 +1452,13 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
 	 * Configure hardcoded terms first, no need to check
 	 * return value when called with fail == 0 ;)
 	 */
-	if (config_attr(&attr, &head_terms, parse_state->error, config_term_pmu)) {
-		parse_events_terms__purge(&head_terms);
+	if (config_attr(&attr, &parsed_terms, parse_state->error, config_term_pmu)) {
+		parse_events_terms__exit(&parsed_terms);
 		return -EINVAL;
 	}
 
-	if (get_config_terms(&head_terms, &config_terms)) {
-		parse_events_terms__purge(&head_terms);
+	if (get_config_terms(&parsed_terms, &config_terms)) {
+		parse_events_terms__exit(&parsed_terms);
 		return -ENOMEM;
 	}
 
@@ -1463,24 +1466,24 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
 	 * When using default config, record which bits of attr->config were
 	 * changed by the user.
 	 */
-	if (pmu->default_config && get_config_chgs(pmu, &head_terms, &config_terms)) {
-		parse_events_terms__purge(&head_terms);
+	if (pmu->default_config && get_config_chgs(pmu, &parsed_terms, &config_terms)) {
+		parse_events_terms__exit(&parsed_terms);
 		return -ENOMEM;
 	}
 
 	if (!parse_state->fake_pmu &&
-	    perf_pmu__config(pmu, &attr, &head_terms, parse_state->error)) {
+	    perf_pmu__config(pmu, &attr, &parsed_terms, parse_state->error)) {
 		free_config_terms(&config_terms);
-		parse_events_terms__purge(&head_terms);
+		parse_events_terms__exit(&parsed_terms);
 		return -EINVAL;
 	}
 
 	evsel = __add_event(list, &parse_state->idx, &attr, /*init_attr=*/true,
-			    get_config_name(&head_terms),
-			    get_config_metric_id(&head_terms), pmu,
+			    get_config_name(&parsed_terms),
+			    get_config_metric_id(&parsed_terms), pmu,
 			    &config_terms, auto_merge_stats, /*cpu_list=*/NULL);
 	if (!evsel) {
-		parse_events_terms__purge(&head_terms);
+		parse_events_terms__exit(&parsed_terms);
 		return -ENOMEM;
 	}
 
@@ -1490,11 +1493,11 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
 	evsel->percore = config_term_percore(&evsel->config_terms);
 
 	if (parse_state->fake_pmu) {
-		parse_events_terms__purge(&head_terms);
+		parse_events_terms__exit(&parsed_terms);
 		return 0;
 	}
 
-	parse_events_terms__purge(&head_terms);
+	parse_events_terms__exit(&parsed_terms);
 	free((char *)evsel->unit);
 	evsel->unit = strdup(info.unit);
 	evsel->scale = info.scale;
@@ -1505,7 +1508,7 @@ 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 struct list_head *const_head_terms,
+			       const struct parse_events_terms *const_parsed_terms,
 			       struct list_head **listp, void *loc_)
 {
 	struct parse_events_term *term;
@@ -1514,12 +1517,13 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
 	YYLTYPE *loc = loc_;
 	int ok = 0;
 	const char *config;
-	LIST_HEAD(head_terms);
+	struct parse_events_terms parsed_terms;
 
 	*listp = NULL;
 
-	if (const_head_terms) {
-		int ret = parse_events_terms__copy(const_head_terms, &head_terms);
+	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;
@@ -1536,7 +1540,7 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
 		zfree(&config);
 		goto out_err;
 	}
-	list_add_tail(&term->list, &head_terms);
+	list_add_tail(&term->list, &parsed_terms.terms);
 
 	/* Add it for all PMUs that support the alias */
 	list = malloc(sizeof(struct list_head));
@@ -1556,11 +1560,11 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
 
 		auto_merge_stats = perf_pmu__auto_merge_stats(pmu);
 		if (!parse_events_add_pmu(parse_state, list, pmu->name,
-					  &head_terms, auto_merge_stats, loc)) {
+					  &parsed_terms, auto_merge_stats, loc)) {
 			struct strbuf sb;
 
 			strbuf_init(&sb, /*hint=*/ 0);
-			parse_events_term__to_strbuf(&head_terms, &sb);
+			parse_events_terms__to_strbuf(&parsed_terms, &sb);
 			pr_debug("%s -> %s/%s/\n", event_name, pmu->name, sb.buf);
 			strbuf_release(&sb);
 			ok++;
@@ -1568,12 +1572,12 @@ 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, &head_terms,
+		if (!parse_events_add_pmu(parse_state, list, event_name, &parsed_terms,
 					  /*auto_merge_stats=*/true, loc)) {
 			struct strbuf sb;
 
 			strbuf_init(&sb, /*hint=*/ 0);
-			parse_events_term__to_strbuf(&head_terms, &sb);
+			parse_events_terms__to_strbuf(&parsed_terms, &sb);
 			pr_debug("%s -> %s/%s/\n", event_name, "fake_pmu", sb.buf);
 			strbuf_release(&sb);
 			ok++;
@@ -1581,7 +1585,7 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
 	}
 
 out_err:
-	parse_events_terms__purge(&head_terms);
+	parse_events_terms__exit(&parsed_terms);
 	if (ok)
 		*listp = list;
 	else
@@ -1851,7 +1855,7 @@ static int parse_events__scanner(const char *str,
 /*
  * parse event config string, return a list of event terms.
  */
-int parse_events_terms(struct list_head *terms, const char *str, FILE *input)
+int parse_events_terms(struct parse_events_terms *terms, const char *str, FILE *input)
 {
 	struct parse_events_state parse_state = {
 		.terms  = NULL,
@@ -1860,14 +1864,10 @@ int parse_events_terms(struct list_head *terms, const char *str, FILE *input)
 	int ret;
 
 	ret = parse_events__scanner(str, input, &parse_state);
+	if (!ret)
+		list_splice(&parse_state.terms->terms, &terms->terms);
 
-	if (!ret) {
-		list_splice(parse_state.terms, terms);
-		zfree(&parse_state.terms);
-		return 0;
-	}
-
-	parse_events_terms__delete(parse_state.terms);
+	zfree(&parse_state.terms);
 	return ret;
 }
 
@@ -2563,11 +2563,12 @@ void parse_events_term__delete(struct parse_events_term *term)
 	free(term);
 }
 
-static int parse_events_terms__copy(const struct list_head *src, struct list_head *dest)
+static int parse_events_terms__copy(const struct parse_events_terms *src,
+				    struct parse_events_terms *dest)
 {
 	struct parse_events_term *term;
 
-	list_for_each_entry (term, src, list) {
+	list_for_each_entry (term, &src->terms, list) {
 		struct parse_events_term *n;
 		int ret;
 
@@ -2575,38 +2576,43 @@ static int parse_events_terms__copy(const struct list_head *src, struct list_hea
 		if (ret)
 			return ret;
 
-		list_add_tail(&n->list, dest);
+		list_add_tail(&n->list, &dest->terms);
 	}
 	return 0;
 }
 
-void parse_events_terms__purge(struct list_head *terms)
+void parse_events_terms__init(struct parse_events_terms *terms)
+{
+	INIT_LIST_HEAD(&terms->terms);
+}
+
+void parse_events_terms__exit(struct parse_events_terms *terms)
 {
 	struct parse_events_term *term, *h;
 
-	list_for_each_entry_safe(term, h, terms, list) {
+	list_for_each_entry_safe(term, h, &terms->terms, list) {
 		list_del_init(&term->list);
 		parse_events_term__delete(term);
 	}
 }
 
-void parse_events_terms__delete(struct list_head *terms)
+void parse_events_terms__delete(struct parse_events_terms *terms)
 {
 	if (!terms)
 		return;
-	parse_events_terms__purge(terms);
+	parse_events_terms__exit(terms);
 	free(terms);
 }
 
-int parse_events_term__to_strbuf(struct list_head *term_list, struct strbuf *sb)
+int parse_events_terms__to_strbuf(const struct parse_events_terms *terms, struct strbuf *sb)
 {
 	struct parse_events_term *term;
 	bool first = true;
 
-	if (!term_list)
+	if (!terms)
 		return 0;
 
-	list_for_each_entry(term, term_list, list) {
+	list_for_each_entry(term, &terms->terms, list) {
 		int ret;
 
 		if (!first) {
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index e6612856e881..63c0a36a4bf1 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -44,7 +44,6 @@ static inline int parse_events(struct evlist *evlist, const char *str,
 
 int parse_event(struct evlist *evlist, const char *str);
 
-int parse_events_terms(struct list_head *terms, const char *str, FILE *input);
 int parse_filter(const struct option *opt, const char *str, int unset);
 int exclude_perf(const struct option *opt, const char *arg, int unset);
 
@@ -140,6 +139,11 @@ struct parse_events_error {
 	char *first_help;
 };
 
+/* A wrapper around a list of terms for the sake of better type safety. */
+struct parse_events_terms {
+	struct list_head terms;
+};
+
 struct parse_events_state {
 	/* The list parsed events are placed on. */
 	struct list_head	   list;
@@ -148,7 +152,7 @@ struct parse_events_state {
 	/* Error information. */
 	struct parse_events_error *error;
 	/* Holds returned terms for term parsing. */
-	struct list_head	  *terms;
+	struct parse_events_terms *terms;
 	/* Start token. */
 	int			   stoken;
 	/* Special fake PMU marker for testing. */
@@ -181,35 +185,38 @@ int parse_events_term__term(struct parse_events_term **term,
 int parse_events_term__clone(struct parse_events_term **new,
 			     struct parse_events_term *term);
 void parse_events_term__delete(struct parse_events_term *term);
-void parse_events_terms__delete(struct list_head *terms);
-void parse_events_terms__purge(struct list_head *terms);
-int parse_events_term__to_strbuf(struct list_head *term_list, struct strbuf *sb);
+
+void parse_events_terms__delete(struct parse_events_terms *terms);
+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);
 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,
 				struct parse_events_error *error,
-				struct list_head *head_config, void *loc);
+				struct parse_events_terms *head_config, void *loc);
 int parse_events_add_numeric(struct parse_events_state *parse_state,
 			     struct list_head *list,
 			     u32 type, u64 config,
-			     struct list_head *head_config,
+			     struct parse_events_terms *head_config,
 			     bool wildcard);
 int parse_events_add_tool(struct parse_events_state *parse_state,
 			  struct list_head *list,
 			  int tool_event);
 int parse_events_add_cache(struct list_head *list, int *idx, const char *name,
 			   struct parse_events_state *parse_state,
-			   struct list_head *head_config);
+			   struct parse_events_terms *head_config);
 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,
 				u64 addr, char *type, u64 len,
-				struct list_head *head_config);
+				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 list_head *const_head_terms,
+			 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,
@@ -218,7 +225,7 @@ struct evsel *parse_events__add_event(int idx, struct perf_event_attr *attr,
 
 int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
 			       const char *event_name,
-			       const struct list_head *head_terms,
+			       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);
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 1ee65f36762c..2cbdbf17f8b8 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -113,7 +113,7 @@ static void free_list_evsel(struct list_head* list_evsel)
 	u64 num;
 	enum parse_events__term_type term_type;
 	struct list_head *list_evsel;
-	struct list_head *list_terms;
+	struct parse_events_terms *list_terms;
 	struct parse_events_term *term;
 	struct tracepoint_name {
 		char *sys;
@@ -644,26 +644,26 @@ start_terms: event_config
 event_config:
 event_config ',' event_term
 {
-	struct list_head *head = $1;
+	struct parse_events_terms *head = $1;
 	struct parse_events_term *term = $3;
 
 	if (!head) {
 		parse_events_term__delete(term);
 		YYABORT;
 	}
-	list_add_tail(&term->list, head);
+	list_add_tail(&term->list, &head->terms);
 	$$ = $1;
 }
 |
 event_term
 {
-	struct list_head *head = malloc(sizeof(*head));
+	struct parse_events_terms *head = malloc(sizeof(*head));
 	struct parse_events_term *term = $1;
 
 	if (!head)
 		YYNOMEM;
-	INIT_LIST_HEAD(head);
-	list_add_tail(&term->list, head);
+	parse_events_terms__init(head);
+	list_add_tail(&term->list, &head->terms);
 	$$ = head;
 }
 
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index d85602aa4b9f..e2159854ab26 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -52,7 +52,7 @@ struct perf_pmu_alias {
 	 */
 	char *topic;
 	/** @terms: Owned list of the original parsed parameters. */
-	struct list_head terms;
+	struct parse_events_terms terms;
 	/** @list: List element of struct perf_pmu aliases. */
 	struct list_head list;
 	/**
@@ -404,7 +404,7 @@ static void perf_pmu_free_alias(struct perf_pmu_alias *newalias)
 	zfree(&newalias->long_desc);
 	zfree(&newalias->topic);
 	zfree(&newalias->pmu_name);
-	parse_events_terms__purge(&newalias->terms);
+	parse_events_terms__exit(&newalias->terms);
 	free(newalias);
 }
 
@@ -484,7 +484,7 @@ static int update_alias(const struct pmu_event *pe,
 	assign_str(pe->name, "topic", &data->alias->topic, pe->topic);
 	data->alias->per_pkg = pe->perpkg;
 	if (pe->event) {
-		parse_events_terms__purge(&data->alias->terms);
+		parse_events_terms__exit(&data->alias->terms);
 		ret = parse_events_terms(&data->alias->terms, pe->event, /*input=*/NULL);
 	}
 	if (!ret && pe->unit) {
@@ -524,7 +524,7 @@ static int perf_pmu__new_alias(struct perf_pmu *pmu, const char *name,
 	if (!alias)
 		return -ENOMEM;
 
-	INIT_LIST_HEAD(&alias->terms);
+	parse_events_terms__init(&alias->terms);
 	alias->scale = 1.0;
 	alias->unit[0] = '\0';
 	alias->per_pkg = perpkg;
@@ -656,17 +656,17 @@ static int pmu_aliases_parse(struct perf_pmu *pmu)
 	return 0;
 }
 
-static int pmu_alias_terms(struct perf_pmu_alias *alias,
-			   struct list_head *terms)
+static int pmu_alias_terms(struct perf_pmu_alias *alias, struct list_head *terms)
 {
 	struct parse_events_term *term, *cloned;
-	LIST_HEAD(list);
-	int ret;
+	struct parse_events_terms clone_terms;
+
+	parse_events_terms__init(&clone_terms);
+	list_for_each_entry(term, &alias->terms.terms, list) {
+		int ret = parse_events_term__clone(&cloned, term);
 
-	list_for_each_entry(term, &alias->terms, list) {
-		ret = parse_events_term__clone(&cloned, term);
 		if (ret) {
-			parse_events_terms__purge(&list);
+			parse_events_terms__exit(&clone_terms);
 			return ret;
 		}
 		/*
@@ -674,9 +674,10 @@ static int pmu_alias_terms(struct perf_pmu_alias *alias,
 		 * which we don't want for implicit terms in aliases.
 		 */
 		cloned->weak = true;
-		list_add_tail(&cloned->list, &list);
+		list_add_tail(&cloned->list, &clone_terms.terms);
 	}
-	list_splice(&list, terms);
+	list_splice_init(&clone_terms.terms, terms);
+	parse_events_terms__exit(&clone_terms);
 	return 0;
 }
 
@@ -1188,12 +1189,12 @@ static __u64 pmu_format_max_value(const unsigned long *format)
  *   in a config string) later on in the term list.
  */
 static int pmu_resolve_param_term(struct parse_events_term *term,
-				  struct list_head *head_terms,
+				  struct parse_events_terms *head_terms,
 				  __u64 *value)
 {
 	struct parse_events_term *t;
 
-	list_for_each_entry(t, head_terms, list) {
+	list_for_each_entry(t, &head_terms->terms, list) {
 		if (t->type_val == PARSE_EVENTS__TERM_TYPE_NUM &&
 		    t->config && !strcmp(t->config, term->config)) {
 			t->used = true;
@@ -1237,7 +1238,7 @@ static char *pmu_formats_string(struct list_head *formats)
 static int pmu_config_term(struct perf_pmu *pmu,
 			   struct perf_event_attr *attr,
 			   struct parse_events_term *term,
-			   struct list_head *head_terms,
+			   struct parse_events_terms *head_terms,
 			   bool zero, struct parse_events_error *err)
 {
 	struct perf_pmu_format *format;
@@ -1359,13 +1360,13 @@ static int pmu_config_term(struct perf_pmu *pmu,
 
 int perf_pmu__config_terms(struct perf_pmu *pmu,
 			   struct perf_event_attr *attr,
-			   struct list_head *head_terms,
+			   struct parse_events_terms *terms,
 			   bool zero, struct parse_events_error *err)
 {
 	struct parse_events_term *term;
 
-	list_for_each_entry(term, head_terms, list) {
-		if (pmu_config_term(pmu, attr, term, head_terms, zero, err))
+	list_for_each_entry(term, &terms->terms, list) {
+		if (pmu_config_term(pmu, attr, term, terms, zero, err))
 			return -EINVAL;
 	}
 
@@ -1378,7 +1379,7 @@ int perf_pmu__config_terms(struct perf_pmu *pmu,
  * 2) pmu format definitions - specified by pmu parameter
  */
 int perf_pmu__config(struct perf_pmu *pmu, struct perf_event_attr *attr,
-		     struct list_head *head_terms,
+		     struct parse_events_terms *head_terms,
 		     struct parse_events_error *err)
 {
 	bool zero = !!pmu->default_config;
@@ -1472,7 +1473,7 @@ static int check_info_data(struct perf_pmu *pmu,
  * Find alias in the terms list and replace it with the terms
  * defined for the alias
  */
-int perf_pmu__check_alias(struct perf_pmu *pmu, struct list_head *head_terms,
+int perf_pmu__check_alias(struct perf_pmu *pmu, struct parse_events_terms *head_terms,
 			  struct perf_pmu_info *info, struct parse_events_error *err)
 {
 	struct parse_events_term *term, *h;
@@ -1489,7 +1490,7 @@ int perf_pmu__check_alias(struct perf_pmu *pmu, struct list_head *head_terms,
 	info->scale    = 0.0;
 	info->snapshot = false;
 
-	list_for_each_entry_safe(term, h, head_terms, list) {
+	list_for_each_entry_safe(term, h, &head_terms->terms, list) {
 		alias = pmu_find_alias(pmu, term);
 		if (!alias)
 			continue;
@@ -1634,7 +1635,7 @@ static char *format_alias(char *buf, int len, const struct perf_pmu *pmu,
 		: (int)strlen(pmu->name);
 	int used = snprintf(buf, len, "%.*s/%s", pmu_name_len, pmu->name, alias->name);
 
-	list_for_each_entry(term, &alias->terms, list) {
+	list_for_each_entry(term, &alias->terms.terms, list) {
 		if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR)
 			used += snprintf(buf + used, sub_non_neg(len, used),
 					",%s=%s", term->config,
@@ -1693,7 +1694,7 @@ int perf_pmu__for_each_event(struct perf_pmu *pmu, bool skip_duplicate_pmus,
 		info.desc = event->desc;
 		info.long_desc = event->long_desc;
 		info.encoding_desc = buf + buf_used;
-		parse_events_term__to_strbuf(&event->terms, &sb);
+		parse_events_terms__to_strbuf(&event->terms, &sb);
 		buf_used += snprintf(buf + buf_used, sizeof(buf) - buf_used,
 				"%s/%s/", info.pmu_name, sb.buf) + 1;
 		info.topic = event->topic;
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index 6a4e170c61d6..bd5d804a6736 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -191,15 +191,15 @@ typedef int (*pmu_event_callback)(void *state, struct pmu_event_info *info);
 
 void pmu_add_sys_aliases(struct perf_pmu *pmu);
 int perf_pmu__config(struct perf_pmu *pmu, struct perf_event_attr *attr,
-		     struct list_head *head_terms,
+		     struct parse_events_terms *head_terms,
 		     struct parse_events_error *error);
 int perf_pmu__config_terms(struct perf_pmu *pmu,
 			   struct perf_event_attr *attr,
-			   struct list_head *head_terms,
+			   struct parse_events_terms *terms,
 			   bool zero, struct parse_events_error *error);
 __u64 perf_pmu__format_bits(struct perf_pmu *pmu, const char *name);
 int perf_pmu__format_type(struct perf_pmu *pmu, const char *name);
-int perf_pmu__check_alias(struct perf_pmu *pmu, struct list_head *head_terms,
+int perf_pmu__check_alias(struct perf_pmu *pmu, struct parse_events_terms *head_terms,
 			  struct perf_pmu_info *info, struct parse_events_error *err);
 int perf_pmu__find_event(struct perf_pmu *pmu, const char *event, void *state, pmu_event_callback cb);
 
-- 
2.42.0.283.g2d96d420d3-goog

Re: [PATCH v1 1/6] perf parse-events: Fixes relating to no_value terms

[Arnaldo Carvalho de Melo]

Em Fri, Sep 01, 2023 at 04:39:44PM -0700, Ian Rogers escreveu:
> A term may have no value in which case it is assumed to have a value
> of 1. It doesn't just apply to alias/event terms so change the
> parse_events_term__to_strbuf assert.
> 
> Commit 99e7138eb789 ("perf tools: Fail on using multiple bits long
> terms without value") made it so that no_value terms could only be for
> a single bit. Prior to commit 64199ae4b8a3 ("perf parse-events: Fix
> propagation of term's no_value when cloning") this missed a test case
> where config1 had no_value.
> 
> Fixes: 64199ae4b8a3 ("perf parse-events: Fix propagation of term's no_value when cloning")
> Signed-off-by: Ian Rogers <irogers@google.com>

I'm trying to minimize the number of patches that aren't strict fixes
this late in the process for v6.6, so I think I'll get just this first
one and defer the other cleanups/improvements for v6.7, ok?

Thanks,

- Arnaldo

> ---
>  tools/perf/tests/parse-events.c | 2 +-
>  tools/perf/util/parse-events.c  | 2 +-
>  tools/perf/util/parse-events.h  | 4 ++--
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
> index d86076d575ed..d47f1f871164 100644
> --- a/tools/perf/tests/parse-events.c
> +++ b/tools/perf/tests/parse-events.c
> @@ -2170,7 +2170,7 @@ static const struct evlist_test test__events[] = {
>  
>  static const struct evlist_test test__events_pmu[] = {
>  	{
> -		.name  = "cpu/config=10,config1,config2=3,period=1000/u",
> +		.name  = "cpu/config=10,config1=1,config2=3,period=1000/u",
>  		.valid = test__pmu_cpu_valid,
>  		.check = test__checkevent_pmu,
>  		/* 0 */
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 68fe2c4ff49f..65608a3cba81 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -2607,7 +2607,7 @@ int parse_events_term__to_strbuf(struct list_head *term_list, struct strbuf *sb)
>  
>  		if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM)
>  			if (term->no_value) {
> -				assert(term->type_term == PARSE_EVENTS__TERM_TYPE_USER);
> +				assert(term->val.num == 1);
>  				ret = strbuf_addf(sb, "%s", term->config);
>  			} else
>  				ret = strbuf_addf(sb, "%s=%#"PRIx64, term->config, term->val.num);
> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> index 855b0725c5d4..594e5d2dc67f 100644
> --- a/tools/perf/util/parse-events.h
> +++ b/tools/perf/util/parse-events.h
> @@ -124,8 +124,8 @@ struct parse_events_term {
>  	 */
>  	bool weak;
>  	/**
> -	 * @no_value: Is there no value. TODO: this should really be part of
> -	 * type_val.
> +	 * @no_value: Is there no value. If a numeric term has no value then the
> +	 * value is assumed to be 1. An event name also has no value.
>  	 */
>  	bool no_value;
>  };
> -- 
> 2.42.0.283.g2d96d420d3-goog
> 

-- 

- Arnaldo

Re: [PATCH v1 1/6] perf parse-events: Fixes relating to no_value terms

[Ian Rogers]

On Sat, Sep 2, 2023 at 4:11 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>
> Em Fri, Sep 01, 2023 at 04:39:44PM -0700, Ian Rogers escreveu:
> > A term may have no value in which case it is assumed to have a value
> > of 1. It doesn't just apply to alias/event terms so change the
> > parse_events_term__to_strbuf assert.
> >
> > Commit 99e7138eb789 ("perf tools: Fail on using multiple bits long
> > terms without value") made it so that no_value terms could only be for
> > a single bit. Prior to commit 64199ae4b8a3 ("perf parse-events: Fix
> > propagation of term's no_value when cloning") this missed a test case
> > where config1 had no_value.
> >
> > Fixes: 64199ae4b8a3 ("perf parse-events: Fix propagation of term's no_value when cloning")
> > Signed-off-by: Ian Rogers <irogers@google.com>
>
> I'm trying to minimize the number of patches that aren't strict fixes
> this late in the process for v6.6, so I think I'll get just this first
> one and defer the other cleanups/improvements for v6.7, ok?

Makes sense, this is why I put the fix first in the series.

Thanks,
Ian

> Thanks,
>
> - Arnaldo
>
> > ---
> >  tools/perf/tests/parse-events.c | 2 +-
> >  tools/perf/util/parse-events.c  | 2 +-
> >  tools/perf/util/parse-events.h  | 4 ++--
> >  3 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
> > index d86076d575ed..d47f1f871164 100644
> > --- a/tools/perf/tests/parse-events.c
> > +++ b/tools/perf/tests/parse-events.c
> > @@ -2170,7 +2170,7 @@ static const struct evlist_test test__events[] = {
> >
> >  static const struct evlist_test test__events_pmu[] = {
> >       {
> > -             .name  = "cpu/config=10,config1,config2=3,period=1000/u",
> > +             .name  = "cpu/config=10,config1=1,config2=3,period=1000/u",
> >               .valid = test__pmu_cpu_valid,
> >               .check = test__checkevent_pmu,
> >               /* 0 */
> > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> > index 68fe2c4ff49f..65608a3cba81 100644
> > --- a/tools/perf/util/parse-events.c
> > +++ b/tools/perf/util/parse-events.c
> > @@ -2607,7 +2607,7 @@ int parse_events_term__to_strbuf(struct list_head *term_list, struct strbuf *sb)
> >
> >               if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM)
> >                       if (term->no_value) {
> > -                             assert(term->type_term == PARSE_EVENTS__TERM_TYPE_USER);
> > +                             assert(term->val.num == 1);
> >                               ret = strbuf_addf(sb, "%s", term->config);
> >                       } else
> >                               ret = strbuf_addf(sb, "%s=%#"PRIx64, term->config, term->val.num);
> > diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> > index 855b0725c5d4..594e5d2dc67f 100644
> > --- a/tools/perf/util/parse-events.h
> > +++ b/tools/perf/util/parse-events.h
> > @@ -124,8 +124,8 @@ struct parse_events_term {
> >        */
> >       bool weak;
> >       /**
> > -      * @no_value: Is there no value. TODO: this should really be part of
> > -      * type_val.
> > +      * @no_value: Is there no value. If a numeric term has no value then the
> > +      * value is assumed to be 1. An event name also has no value.
> >        */
> >       bool no_value;
> >  };
> > --
> > 2.42.0.283.g2d96d420d3-goog
> >
>
> --
>
> - Arnaldo

Re: [PATCH v1 1/6] perf parse-events: Fixes relating to no_value terms

[James Clark]

On 02/09/2023 00:39, Ian Rogers wrote:
> A term may have no value in which case it is assumed to have a value
> of 1. It doesn't just apply to alias/event terms so change the
> parse_events_term__to_strbuf assert.
> 
> Commit 99e7138eb789 ("perf tools: Fail on using multiple bits long
> terms without value") made it so that no_value terms could only be for
> a single bit. Prior to commit 64199ae4b8a3 ("perf parse-events: Fix
> propagation of term's no_value when cloning") this missed a test case
> where config1 had no_value.
> 
> Fixes: 64199ae4b8a3 ("perf parse-events: Fix propagation of term's no_value when cloning")
> Signed-off-by: Ian Rogers <irogers@google.com>

For the whole set:

Reviewed-by: James Clark <james.clark@arm.com>

> ---
>  tools/perf/tests/parse-events.c | 2 +-
>  tools/perf/util/parse-events.c  | 2 +-
>  tools/perf/util/parse-events.h  | 4 ++--
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
> index d86076d575ed..d47f1f871164 100644
> --- a/tools/perf/tests/parse-events.c
> +++ b/tools/perf/tests/parse-events.c
> @@ -2170,7 +2170,7 @@ static const struct evlist_test test__events[] = {
>  
>  static const struct evlist_test test__events_pmu[] = {
>  	{
> -		.name  = "cpu/config=10,config1,config2=3,period=1000/u",
> +		.name  = "cpu/config=10,config1=1,config2=3,period=1000/u",
>  		.valid = test__pmu_cpu_valid,
>  		.check = test__checkevent_pmu,
>  		/* 0 */
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 68fe2c4ff49f..65608a3cba81 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -2607,7 +2607,7 @@ int parse_events_term__to_strbuf(struct list_head *term_list, struct strbuf *sb)
>  
>  		if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM)
>  			if (term->no_value) {
> -				assert(term->type_term == PARSE_EVENTS__TERM_TYPE_USER);
> +				assert(term->val.num == 1);
>  				ret = strbuf_addf(sb, "%s", term->config);
>  			} else
>  				ret = strbuf_addf(sb, "%s=%#"PRIx64, term->config, term->val.num);
> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> index 855b0725c5d4..594e5d2dc67f 100644
> --- a/tools/perf/util/parse-events.h
> +++ b/tools/perf/util/parse-events.h
> @@ -124,8 +124,8 @@ struct parse_events_term {
>  	 */
>  	bool weak;
>  	/**
> -	 * @no_value: Is there no value. TODO: this should really be part of
> -	 * type_val.
> +	 * @no_value: Is there no value. If a numeric term has no value then the
> +	 * value is assumed to be 1. An event name also has no value.
>  	 */
>  	bool no_value;
>  };

Re: [PATCH v1 1/6] perf parse-events: Fixes relating to no_value terms

[Arnaldo Carvalho de Melo]

Em Tue, Sep 05, 2023 at 10:39:38AM +0100, James Clark escreveu:
> 
> 
> On 02/09/2023 00:39, Ian Rogers wrote:
> > A term may have no value in which case it is assumed to have a value
> > of 1. It doesn't just apply to alias/event terms so change the
> > parse_events_term__to_strbuf assert.
> > 
> > Commit 99e7138eb789 ("perf tools: Fail on using multiple bits long
> > terms without value") made it so that no_value terms could only be for
> > a single bit. Prior to commit 64199ae4b8a3 ("perf parse-events: Fix
> > propagation of term's no_value when cloning") this missed a test case
> > where config1 had no_value.
> > 
> > Fixes: 64199ae4b8a3 ("perf parse-events: Fix propagation of term's no_value when cloning")
> > Signed-off-by: Ian Rogers <irogers@google.com>
> 
> For the whole set:
> 
> Reviewed-by: James Clark <james.clark@arm.com>

Thanks for providing it, in the future please consider replying to the
cover letter (0/6) to do that, so that b4 can collect it, doing it for
the 1st patch it will collect just for it, not for the whole set as you
intended.

- Arnaldo
 
> > ---
> >  tools/perf/tests/parse-events.c | 2 +-
> >  tools/perf/util/parse-events.c  | 2 +-
> >  tools/perf/util/parse-events.h  | 4 ++--
> >  3 files changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
> > index d86076d575ed..d47f1f871164 100644
> > --- a/tools/perf/tests/parse-events.c
> > +++ b/tools/perf/tests/parse-events.c
> > @@ -2170,7 +2170,7 @@ static const struct evlist_test test__events[] = {
> >  
> >  static const struct evlist_test test__events_pmu[] = {
> >  	{
> > -		.name  = "cpu/config=10,config1,config2=3,period=1000/u",
> > +		.name  = "cpu/config=10,config1=1,config2=3,period=1000/u",
> >  		.valid = test__pmu_cpu_valid,
> >  		.check = test__checkevent_pmu,
> >  		/* 0 */
> > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> > index 68fe2c4ff49f..65608a3cba81 100644
> > --- a/tools/perf/util/parse-events.c
> > +++ b/tools/perf/util/parse-events.c
> > @@ -2607,7 +2607,7 @@ int parse_events_term__to_strbuf(struct list_head *term_list, struct strbuf *sb)
> >  
> >  		if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM)
> >  			if (term->no_value) {
> > -				assert(term->type_term == PARSE_EVENTS__TERM_TYPE_USER);
> > +				assert(term->val.num == 1);
> >  				ret = strbuf_addf(sb, "%s", term->config);
> >  			} else
> >  				ret = strbuf_addf(sb, "%s=%#"PRIx64, term->config, term->val.num);
> > diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> > index 855b0725c5d4..594e5d2dc67f 100644
> > --- a/tools/perf/util/parse-events.h
> > +++ b/tools/perf/util/parse-events.h
> > @@ -124,8 +124,8 @@ struct parse_events_term {
> >  	 */
> >  	bool weak;
> >  	/**
> > -	 * @no_value: Is there no value. TODO: this should really be part of
> > -	 * type_val.
> > +	 * @no_value: Is there no value. If a numeric term has no value then the
> > +	 * value is assumed to be 1. An event name also has no value.
> >  	 */
> >  	bool no_value;
> >  };

-- 

- Arnaldo

Re: [PATCH v1 1/6] perf parse-events: Fixes relating to no_value terms

[Arnaldo Carvalho de Melo]

Em Tue, Sep 05, 2023 at 10:39:38AM +0100, James Clark escreveu:
> 
> 
> On 02/09/2023 00:39, Ian Rogers wrote:
> > A term may have no value in which case it is assumed to have a value
> > of 1. It doesn't just apply to alias/event terms so change the
> > parse_events_term__to_strbuf assert.
> > 
> > Commit 99e7138eb789 ("perf tools: Fail on using multiple bits long
> > terms without value") made it so that no_value terms could only be for
> > a single bit. Prior to commit 64199ae4b8a3 ("perf parse-events: Fix
> > propagation of term's no_value when cloning") this missed a test case
> > where config1 had no_value.
> > 
> > Fixes: 64199ae4b8a3 ("perf parse-events: Fix propagation of term's no_value when cloning")
> > Signed-off-by: Ian Rogers <irogers@google.com>
> 
> For the whole set:
> 
> Reviewed-by: James Clark <james.clark@arm.com>

Thanks, applied.

- Arnaldo

 
> > ---
> >  tools/perf/tests/parse-events.c | 2 +-
> >  tools/perf/util/parse-events.c  | 2 +-
> >  tools/perf/util/parse-events.h  | 4 ++--
> >  3 files changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
> > index d86076d575ed..d47f1f871164 100644
> > --- a/tools/perf/tests/parse-events.c
> > +++ b/tools/perf/tests/parse-events.c
> > @@ -2170,7 +2170,7 @@ static const struct evlist_test test__events[] = {
> >  
> >  static const struct evlist_test test__events_pmu[] = {
> >  	{
> > -		.name  = "cpu/config=10,config1,config2=3,period=1000/u",
> > +		.name  = "cpu/config=10,config1=1,config2=3,period=1000/u",
> >  		.valid = test__pmu_cpu_valid,
> >  		.check = test__checkevent_pmu,
> >  		/* 0 */
> > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> > index 68fe2c4ff49f..65608a3cba81 100644
> > --- a/tools/perf/util/parse-events.c
> > +++ b/tools/perf/util/parse-events.c
> > @@ -2607,7 +2607,7 @@ int parse_events_term__to_strbuf(struct list_head *term_list, struct strbuf *sb)
> >  
> >  		if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM)
> >  			if (term->no_value) {
> > -				assert(term->type_term == PARSE_EVENTS__TERM_TYPE_USER);
> > +				assert(term->val.num == 1);
> >  				ret = strbuf_addf(sb, "%s", term->config);
> >  			} else
> >  				ret = strbuf_addf(sb, "%s=%#"PRIx64, term->config, term->val.num);
> > diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> > index 855b0725c5d4..594e5d2dc67f 100644
> > --- a/tools/perf/util/parse-events.h
> > +++ b/tools/perf/util/parse-events.h
> > @@ -124,8 +124,8 @@ struct parse_events_term {
> >  	 */
> >  	bool weak;
> >  	/**
> > -	 * @no_value: Is there no value. TODO: this should really be part of
> > -	 * type_val.
> > +	 * @no_value: Is there no value. If a numeric term has no value then the
> > +	 * value is assumed to be 1. An event name also has no value.
> >  	 */
> >  	bool no_value;
> >  };

-- 

- Arnaldo