[PATCH v2 1/3] perf parse-events: Rename parse_events_error functions

[PATCH v2 1/3] perf parse-events: Rename parse_events_error functions

[Ian Rogers]

Group error functions and name after the data type they manipulate.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/bench/evlist-open-close.c |   2 +-
 tools/perf/builtin-stat.c            |  10 +--
 tools/perf/builtin-trace.c           |   2 +-
 tools/perf/tests/expand-cgroup.c     |   2 +-
 tools/perf/tests/parse-events.c      |   2 +-
 tools/perf/util/metricgroup.c        |   2 +-
 tools/perf/util/parse-events.c       | 116 +++++++++++++--------------
 tools/perf/util/parse-events.h       |   8 +-
 tools/perf/util/parse-events.y       |   4 +-
 tools/perf/util/pmu.c                |   8 +-
 10 files changed, 78 insertions(+), 78 deletions(-)

diff --git a/tools/perf/bench/evlist-open-close.c b/tools/perf/bench/evlist-open-close.c
index 75a53919126b..3f9518936367 100644
--- a/tools/perf/bench/evlist-open-close.c
+++ b/tools/perf/bench/evlist-open-close.c
@@ -89,7 +89,7 @@ static struct evlist *bench__create_evlist(char *evstr)
 
 	ret = parse_events(evlist, evstr, &err);
 	if (ret) {
-		parse_events_print_error(&err, evstr);
+		parse_events_error__print(&err, evstr);
 		pr_err("Run 'perf list' for a list of valid events\n");
 		ret = 1;
 		goto out_delete_evlist;
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index f0ecfda34ece..af447a179d84 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -1782,7 +1782,7 @@ static int add_default_attributes(void)
 					   &errinfo);
 		if (err) {
 			fprintf(stderr, "Cannot set up transaction events\n");
-			parse_events_print_error(&errinfo, transaction_attrs);
+			parse_events_error__print(&errinfo, transaction_attrs);
 			return -1;
 		}
 		return 0;
@@ -1812,11 +1812,11 @@ static int add_default_attributes(void)
 		} else {
 			fprintf(stderr, "To measure SMI cost, it needs "
 				"msr/aperf/, msr/smi/ and cpu/cycles/ support\n");
-			parse_events_print_error(&errinfo, smi_cost_attrs);
+			parse_events_error__print(&errinfo, smi_cost_attrs);
 			return -1;
 		}
 		if (err) {
-			parse_events_print_error(&errinfo, smi_cost_attrs);
+			parse_events_error__print(&errinfo, smi_cost_attrs);
 			fprintf(stderr, "Cannot set up SMI cost events\n");
 			return -1;
 		}
@@ -1883,7 +1883,7 @@ static int add_default_attributes(void)
 				fprintf(stderr,
 					"Cannot set up top down events %s: %d\n",
 					str, err);
-				parse_events_print_error(&errinfo, str);
+				parse_events_error__print(&errinfo, str);
 				free(str);
 				return -1;
 			}
@@ -1911,7 +1911,7 @@ static int add_default_attributes(void)
 				fprintf(stderr,
 					"Cannot set up hybrid events %s: %d\n",
 					hybrid_str, err);
-				parse_events_print_error(&errinfo, hybrid_str);
+				parse_events_error__print(&errinfo, hybrid_str);
 				return -1;
 			}
 			return err;
diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index 2f1d20553a0a..7f0acc94e9ac 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -4928,7 +4928,7 @@ int cmd_trace(int argc, const char **argv)
 		bzero(&parse_err, sizeof(parse_err));
 		err = parse_events(trace.evlist, trace.perfconfig_events, &parse_err);
 		if (err) {
-			parse_events_print_error(&parse_err, trace.perfconfig_events);
+			parse_events_error__print(&parse_err, trace.perfconfig_events);
 			goto out;
 		}
 	}
diff --git a/tools/perf/tests/expand-cgroup.c b/tools/perf/tests/expand-cgroup.c
index aaad51aba12f..57b4c5f30324 100644
--- a/tools/perf/tests/expand-cgroup.c
+++ b/tools/perf/tests/expand-cgroup.c
@@ -128,7 +128,7 @@ static int expand_group_events(void)
 	if (ret < 0) {
 		pr_debug("failed to parse event '%s', err %d, str '%s'\n",
 			 event_str, ret, err.str);
-		parse_events_print_error(&err, event_str);
+		parse_events_error__print(&err, event_str);
 		goto out;
 	}
 
diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
index 8875e388563e..e200af986613 100644
--- a/tools/perf/tests/parse-events.c
+++ b/tools/perf/tests/parse-events.c
@@ -2059,7 +2059,7 @@ static int test_event(struct evlist_test *e)
 	if (ret) {
 		pr_debug("failed to parse event '%s', err %d, str '%s'\n",
 			 e->name, ret, err.str);
-		parse_events_print_error(&err, e->name);
+		parse_events_error__print(&err, e->name);
 	} else {
 		ret = e->check(evlist);
 	}
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 4917e9704765..edd7180b24e4 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -1342,7 +1342,7 @@ static int parse_ids(struct perf_pmu *fake_pmu, struct expr_parse_ctx *ids,
 	bzero(&parse_error, sizeof(parse_error));
 	ret = __parse_events(parsed_evlist, events.buf, &parse_error, fake_pmu);
 	if (ret) {
-		parse_events_print_error(&parse_error, events.buf);
+		parse_events_error__print(&parse_error, events.buf);
 		goto err_out;
 	}
 	ret = decode_all_metric_ids(parsed_evlist, modifier);
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 278199ed788b..75cafb9a0720 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -191,39 +191,6 @@ static int tp_event_has_id(const char *dir_path, struct dirent *evt_dir)
 
 #define MAX_EVENT_LENGTH 512
 
-void parse_events__handle_error(struct parse_events_error *err, int idx,
-				char *str, char *help)
-{
-	if (WARN(!str, "WARNING: failed to provide error string\n")) {
-		free(help);
-		return;
-	}
-	switch (err->num_errors) {
-	case 0:
-		err->idx = idx;
-		err->str = str;
-		err->help = help;
-		break;
-	case 1:
-		err->first_idx = err->idx;
-		err->idx = idx;
-		err->first_str = err->str;
-		err->str = str;
-		err->first_help = err->help;
-		err->help = help;
-		break;
-	default:
-		pr_debug("Multiple errors dropping message: %s (%s)\n",
-			err->str, err->help);
-		free(err->str);
-		err->str = str;
-		free(err->help);
-		err->help = help;
-		break;
-	}
-	err->num_errors++;
-}
-
 struct tracepoint_path *tracepoint_id_to_path(u64 config)
 {
 	struct tracepoint_path *path = NULL;
@@ -587,7 +554,7 @@ static void tracepoint_error(struct parse_events_error *e, int err,
 	}
 
 	tracing_path__strerror_open_tp(err, help, sizeof(help), sys, name);
-	parse_events__handle_error(e, 0, strdup(str), strdup(help));
+	parse_events_error__handle(e, 0, strdup(str), strdup(help));
 }
 
 static int add_tracepoint(struct list_head *list, int *idx,
@@ -811,7 +778,7 @@ int parse_events_load_bpf_obj(struct parse_events_state *parse_state,
 
 	return 0;
 errout:
-	parse_events__handle_error(parse_state->error, 0,
+	parse_events_error__handle(parse_state->error, 0,
 				strdup(errbuf), strdup("(add -v to see detail)"));
 	return err;
 }
@@ -831,7 +798,7 @@ parse_events_config_bpf(struct parse_events_state *parse_state,
 		int err;
 
 		if (term->type_term != PARSE_EVENTS__TERM_TYPE_USER) {
-			parse_events__handle_error(parse_state->error, term->err_term,
+			parse_events_error__handle(parse_state->error, term->err_term,
 						strdup("Invalid config term for BPF object"),
 						NULL);
 			return -EINVAL;
@@ -851,7 +818,7 @@ parse_events_config_bpf(struct parse_events_state *parse_state,
 			else
 				idx = term->err_term + error_pos;
 
-			parse_events__handle_error(parse_state->error, idx,
+			parse_events_error__handle(parse_state->error, idx,
 						strdup(errbuf),
 						strdup(
 "Hint:\tValid config terms:\n"
@@ -923,7 +890,7 @@ int parse_events_load_bpf(struct parse_events_state *parse_state,
 						   -err, errbuf,
 						   sizeof(errbuf));
 
-		parse_events__handle_error(parse_state->error, 0,
+		parse_events_error__handle(parse_state->error, 0,
 					strdup(errbuf), strdup("(add -v to see detail)"));
 		return err;
 	}
@@ -947,7 +914,7 @@ int parse_events_load_bpf_obj(struct parse_events_state *parse_state,
 			      struct bpf_object *obj __maybe_unused,
 			      struct list_head *head_config __maybe_unused)
 {
-	parse_events__handle_error(parse_state->error, 0,
+	parse_events_error__handle(parse_state->error, 0,
 				   strdup("BPF support is not compiled"),
 				   strdup("Make sure libbpf-devel is available at build time."));
 	return -ENOTSUP;
@@ -959,7 +926,7 @@ int parse_events_load_bpf(struct parse_events_state *parse_state,
 			  bool source __maybe_unused,
 			  struct list_head *head_config __maybe_unused)
 {
-	parse_events__handle_error(parse_state->error, 0,
+	parse_events_error__handle(parse_state->error, 0,
 				   strdup("BPF support is not compiled"),
 				   strdup("Make sure libbpf-devel is available at build time."));
 	return -ENOTSUP;
@@ -1042,7 +1009,7 @@ static int check_type_val(struct parse_events_term *term,
 		return 0;
 
 	if (err) {
-		parse_events__handle_error(err, term->err_val,
+		parse_events_error__handle(err, term->err_val,
 					type == PARSE_EVENTS__TERM_TYPE_NUM
 					? strdup("expected numeric value")
 					: strdup("expected string value"),
@@ -1087,7 +1054,7 @@ config_term_avail(int term_type, struct parse_events_error *err)
 	char *err_str;
 
 	if (term_type < 0 || term_type >= __PARSE_EVENTS__TERM_TYPE_NR) {
-		parse_events__handle_error(err, -1,
+		parse_events_error__handle(err, -1,
 					strdup("Invalid term_type"), NULL);
 		return false;
 	}
@@ -1110,7 +1077,7 @@ config_term_avail(int term_type, struct parse_events_error *err)
 		/* term_type is validated so indexing is safe */
 		if (asprintf(&err_str, "'%s' is not usable in 'perf stat'",
 				config_term_names[term_type]) >= 0)
-			parse_events__handle_error(err, -1, err_str, NULL);
+			parse_events_error__handle(err, -1, err_str, NULL);
 		return false;
 	}
 }
@@ -1154,7 +1121,7 @@ do {									   \
 		if (strcmp(term->val.str, "no") &&
 		    parse_branch_str(term->val.str,
 				    &attr->branch_sample_type)) {
-			parse_events__handle_error(err, term->err_val,
+			parse_events_error__handle(err, term->err_val,
 					strdup("invalid branch sample type"),
 					NULL);
 			return -EINVAL;
@@ -1163,7 +1130,7 @@ do {									   \
 	case PARSE_EVENTS__TERM_TYPE_TIME:
 		CHECK_TYPE_VAL(NUM);
 		if (term->val.num > 1) {
-			parse_events__handle_error(err, term->err_val,
+			parse_events_error__handle(err, term->err_val,
 						strdup("expected 0 or 1"),
 						NULL);
 			return -EINVAL;
@@ -1202,7 +1169,7 @@ do {									   \
 	case PARSE_EVENTS__TERM_TYPE_PERCORE:
 		CHECK_TYPE_VAL(NUM);
 		if ((unsigned int)term->val.num > 1) {
-			parse_events__handle_error(err, term->err_val,
+			parse_events_error__handle(err, term->err_val,
 						strdup("expected 0 or 1"),
 						NULL);
 			return -EINVAL;
@@ -1214,14 +1181,14 @@ do {									   \
 	case PARSE_EVENTS__TERM_TYPE_AUX_SAMPLE_SIZE:
 		CHECK_TYPE_VAL(NUM);
 		if (term->val.num > UINT_MAX) {
-			parse_events__handle_error(err, term->err_val,
+			parse_events_error__handle(err, term->err_val,
 						strdup("too big"),
 						NULL);
 			return -EINVAL;
 		}
 		break;
 	default:
-		parse_events__handle_error(err, term->err_term,
+		parse_events_error__handle(err, term->err_term,
 				strdup("unknown term"),
 				parse_events_formats_error_string(NULL));
 		return -EINVAL;
@@ -1275,7 +1242,7 @@ static int config_term_tracepoint(struct perf_event_attr *attr,
 		return config_term_common(attr, term, err);
 	default:
 		if (err) {
-			parse_events__handle_error(err, term->err_term,
+			parse_events_error__handle(err, term->err_term,
 				strdup("unknown term"),
 				strdup("valid terms: call-graph,stack-size\n"));
 		}
@@ -1574,7 +1541,7 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
 		if (asprintf(&err_str,
 				"Cannot find PMU `%s'. Missing kernel support?",
 				name) >= 0)
-			parse_events__handle_error(err, 0, err_str, NULL);
+			parse_events_error__handle(err, 0, err_str, NULL);
 		return -EINVAL;
 	}
 
@@ -2334,6 +2301,39 @@ int __parse_events(struct evlist *evlist, const char *str,
 	return ret;
 }
 
+void parse_events_error__handle(struct parse_events_error *err, int idx,
+				char *str, char *help)
+{
+	if (WARN(!str, "WARNING: failed to provide error string\n")) {
+		free(help);
+		return;
+	}
+	switch (err->num_errors) {
+	case 0:
+		err->idx = idx;
+		err->str = str;
+		err->help = help;
+		break;
+	case 1:
+		err->first_idx = err->idx;
+		err->idx = idx;
+		err->first_str = err->str;
+		err->str = str;
+		err->first_help = err->help;
+		err->help = help;
+		break;
+	default:
+		pr_debug("Multiple errors dropping message: %s (%s)\n",
+			err->str, err->help);
+		free(err->str);
+		err->str = str;
+		free(err->help);
+		err->help = help;
+		break;
+	}
+	err->num_errors++;
+}
+
 #define MAX_WIDTH 1000
 static int get_term_width(void)
 {
@@ -2343,8 +2343,8 @@ static int get_term_width(void)
 	return ws.ws_col > MAX_WIDTH ? MAX_WIDTH : ws.ws_col;
 }
 
-static void __parse_events_print_error(int err_idx, const char *err_str,
-				const char *err_help, const char *event)
+static void __parse_events_error__print(int err_idx, const char *err_str,
+					const char *err_help, const char *event)
 {
 	const char *str = "invalid or unsupported event: ";
 	char _buf[MAX_WIDTH];
@@ -2398,19 +2398,19 @@ static void __parse_events_print_error(int err_idx, const char *err_str,
 	}
 }
 
-void parse_events_print_error(struct parse_events_error *err,
-			      const char *event)
+void parse_events_error__print(struct parse_events_error *err,
+			       const char *event)
 {
 	if (!err->num_errors)
 		return;
 
-	__parse_events_print_error(err->idx, err->str, err->help, event);
+	__parse_events_error__print(err->idx, err->str, err->help, event);
 	zfree(&err->str);
 	zfree(&err->help);
 
 	if (err->num_errors > 1) {
 		fputs("\nInitial error:\n", stderr);
-		__parse_events_print_error(err->first_idx, err->first_str,
+		__parse_events_error__print(err->first_idx, err->first_str,
 					err->first_help, event);
 		zfree(&err->first_str);
 		zfree(&err->first_help);
@@ -2430,7 +2430,7 @@ int parse_events_option(const struct option *opt, const char *str,
 	ret = parse_events(evlist, str, &err);
 
 	if (ret) {
-		parse_events_print_error(&err, str);
+		parse_events_error__print(&err, str);
 		fprintf(stderr, "Run 'perf list' for a list of valid events\n");
 	}
 
@@ -3324,7 +3324,7 @@ void parse_events_evlist_error(struct parse_events_state *parse_state,
 	if (!parse_state->error)
 		return;
 
-	parse_events__handle_error(parse_state->error, idx, strdup(str), NULL);
+	parse_events_error__handle(parse_state->error, idx, strdup(str), NULL);
 }
 
 static void config_terms_list(char *buf, size_t buf_sz)
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index f60a661a2247..52ac26b3720a 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -142,8 +142,6 @@ struct parse_events_state {
 	char			  *hybrid_pmu_name;
 };
 
-void parse_events__handle_error(struct parse_events_error *err, int idx,
-				char *str, char *help);
 void parse_events__shrink_config_terms(void);
 int parse_events__is_hardcoded_term(struct parse_events_term *term);
 int parse_events_term__num(struct parse_events_term **term,
@@ -244,8 +242,10 @@ int is_valid_tracepoint(const char *event_string);
 int valid_event_mount(const char *eventfs);
 char *parse_events_formats_error_string(char *additional_terms);
 
-void parse_events_print_error(struct parse_events_error *err,
-			      const char *event);
+void parse_events_error__handle(struct parse_events_error *err, int idx,
+				char *str, char *help);
+void parse_events_error__print(struct parse_events_error *err,
+			       const char *event);
 
 #ifdef HAVE_LIBELF_SUPPORT
 /*
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 2d60f3cbe42b..174158982fae 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -186,7 +186,7 @@ group_def ':' PE_MODIFIER_EVENT
 		struct parse_events_state *parse_state = _parse_state;
 		struct parse_events_error *error = parse_state->error;
 
-		parse_events__handle_error(error, @3.first_column,
+		parse_events_error__handle(error, @3.first_column,
 					   strdup("Bad modifier"), NULL);
 		free_list_evsel(list);
 		YYABORT;
@@ -248,7 +248,7 @@ event_name PE_MODIFIER_EVENT
 		struct parse_events_state *parse_state = _parse_state;
 		struct parse_events_error *error = parse_state->error;
 
-		parse_events__handle_error(error, @2.first_column,
+		parse_events_error__handle(error, @2.first_column,
 					   strdup("Bad modifier"), NULL);
 		free_list_evsel(list);
 		YYABORT;
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index f3072c71d132..6ae58406f4fc 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -1283,7 +1283,7 @@ static int pmu_config_term(const char *pmu_name,
 			unknown_term = NULL;
 		help_msg = parse_events_formats_error_string(pmu_term);
 		if (err) {
-			parse_events__handle_error(err, term->err_term,
+			parse_events_error__handle(err, term->err_term,
 						   unknown_term,
 						   help_msg);
 		} else {
@@ -1316,7 +1316,7 @@ static int pmu_config_term(const char *pmu_name,
 		if (term->no_value &&
 		    bitmap_weight(format->bits, PERF_PMU_FORMAT_BITS) > 1) {
 			if (err) {
-				parse_events__handle_error(err, term->err_val,
+				parse_events_error__handle(err, term->err_val,
 					   strdup("no value assigned for term"),
 					   NULL);
 			}
@@ -1331,7 +1331,7 @@ static int pmu_config_term(const char *pmu_name,
 						term->config, term->val.str);
 			}
 			if (err) {
-				parse_events__handle_error(err, term->err_val,
+				parse_events_error__handle(err, term->err_val,
 					strdup("expected numeric value"),
 					NULL);
 			}
@@ -1348,7 +1348,7 @@ static int pmu_config_term(const char *pmu_name,
 		if (err) {
 			char *err_str;
 
-			parse_events__handle_error(err, term->err_val,
+			parse_events_error__handle(err, term->err_val,
 				asprintf(&err_str,
 				    "value too big for format, maximum is %llu",
 				    (unsigned long long)max_val) < 0
-- 
2.34.0.rc0.344.g81b53c2807-goog

[PATCH v2 2/3] perf parse-event: Add init and exit to parse_event_error

[Ian Rogers]

parse_events may succeed but leave string memory allocations reachable
in the error. Add an init/exit that must be called to initialize and
clean up the error. This fixes a leak in metricgroup parse_ids.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/arch/powerpc/util/kvm-stat.c |  3 +-
 tools/perf/bench/evlist-open-close.c    |  6 ++--
 tools/perf/builtin-stat.c               | 38 ++++++++++++++-----------
 tools/perf/builtin-trace.c              | 17 +++++------
 tools/perf/tests/backward-ring-buffer.c |  3 +-
 tools/perf/tests/bpf.c                  |  3 +-
 tools/perf/tests/expand-cgroup.c        |  2 ++
 tools/perf/tests/parse-events.c         |  4 +--
 tools/perf/tests/pmu-events.c           | 22 +++++++-------
 tools/perf/tests/topology.c             |  2 ++
 tools/perf/util/metricgroup.c           |  3 +-
 tools/perf/util/parse-events.c          | 20 +++++++++----
 tools/perf/util/parse-events.h          |  2 ++
 13 files changed, 74 insertions(+), 51 deletions(-)

diff --git a/tools/perf/arch/powerpc/util/kvm-stat.c b/tools/perf/arch/powerpc/util/kvm-stat.c
index 16510686c138..2a74bec15a3e 100644
--- a/tools/perf/arch/powerpc/util/kvm-stat.c
+++ b/tools/perf/arch/powerpc/util/kvm-stat.c
@@ -113,10 +113,11 @@ static int is_tracepoint_available(const char *str, struct evlist *evlist)
 	struct parse_events_error err;
 	int ret;
 
-	bzero(&err, sizeof(err));
+	parse_events_error__init(&err);
 	ret = parse_events(evlist, str, &err);
 	if (err.str)
 		parse_events_print_error(&err, "tracepoint");
+	parse_events_error__exit(&err);
 	return ret;
 }
 
diff --git a/tools/perf/bench/evlist-open-close.c b/tools/perf/bench/evlist-open-close.c
index 3f9518936367..482738e9bdad 100644
--- a/tools/perf/bench/evlist-open-close.c
+++ b/tools/perf/bench/evlist-open-close.c
@@ -78,7 +78,7 @@ static int evlist__count_evsel_fds(struct evlist *evlist)
 
 static struct evlist *bench__create_evlist(char *evstr)
 {
-	struct parse_events_error err = { .idx = 0, };
+	struct parse_events_error err;
 	struct evlist *evlist = evlist__new();
 	int ret;
 
@@ -87,14 +87,16 @@ static struct evlist *bench__create_evlist(char *evstr)
 		return NULL;
 	}
 
+	parse_events_error__init(&err);
 	ret = parse_events(evlist, evstr, &err);
 	if (ret) {
 		parse_events_error__print(&err, evstr);
+		parse_events_error__exit(&err);
 		pr_err("Run 'perf list' for a list of valid events\n");
 		ret = 1;
 		goto out_delete_evlist;
 	}
-
+	parse_events_error__exit(&err);
 	ret = evlist__create_maps(evlist, &opts.target);
 	if (ret < 0) {
 		pr_err("Not enough memory to create thread/cpu maps\n");
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index af447a179d84..7974933dbc77 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -1750,14 +1750,12 @@ static int add_default_attributes(void)
 	(PERF_COUNT_HW_CACHE_OP_PREFETCH	<<  8) |
 	(PERF_COUNT_HW_CACHE_RESULT_MISS	<< 16)				},
 };
-	struct parse_events_error errinfo;
-
 	/* Set attrs if no event is selected and !null_run: */
 	if (stat_config.null_run)
 		return 0;
 
-	bzero(&errinfo, sizeof(errinfo));
 	if (transaction_run) {
+		struct parse_events_error errinfo;
 		/* Handle -T as -M transaction. Once platform specific metrics
 		 * support has been added to the json files, all architectures
 		 * will use this approach. To determine transaction support
@@ -1772,6 +1770,7 @@ static int add_default_attributes(void)
 							 &stat_config.metric_events);
 		}
 
+		parse_events_error__init(&errinfo);
 		if (pmu_have_event("cpu", "cycles-ct") &&
 		    pmu_have_event("cpu", "el-start"))
 			err = parse_events(evsel_list, transaction_attrs,
@@ -1783,12 +1782,13 @@ static int add_default_attributes(void)
 		if (err) {
 			fprintf(stderr, "Cannot set up transaction events\n");
 			parse_events_error__print(&errinfo, transaction_attrs);
-			return -1;
 		}
-		return 0;
+		parse_events_error__exit(&errinfo);
+		return err ? -1 : 0;
 	}
 
 	if (smi_cost) {
+		struct parse_events_error errinfo;
 		int smi;
 
 		if (sysfs__read_int(FREEZE_ON_SMI_PATH, &smi) < 0) {
@@ -1804,23 +1804,23 @@ static int add_default_attributes(void)
 			smi_reset = true;
 		}
 
-		if (pmu_have_event("msr", "aperf") &&
-		    pmu_have_event("msr", "smi")) {
-			if (!force_metric_only)
-				stat_config.metric_only = true;
-			err = parse_events(evsel_list, smi_cost_attrs, &errinfo);
-		} else {
+		if (!pmu_have_event("msr", "aperf") ||
+		    !pmu_have_event("msr", "smi")) {
 			fprintf(stderr, "To measure SMI cost, it needs "
 				"msr/aperf/, msr/smi/ and cpu/cycles/ support\n");
-			parse_events_error__print(&errinfo, smi_cost_attrs);
 			return -1;
 		}
+		if (!force_metric_only)
+			stat_config.metric_only = true;
+
+		parse_events_error__init(&errinfo);
+		err = parse_events(evsel_list, smi_cost_attrs, &errinfo);
 		if (err) {
 			parse_events_error__print(&errinfo, smi_cost_attrs);
 			fprintf(stderr, "Cannot set up SMI cost events\n");
-			return -1;
 		}
-		return 0;
+		parse_events_error__exit(&errinfo);
+		return err ? -1 : 0;
 	}
 
 	if (topdown_run) {
@@ -1875,18 +1875,22 @@ static int add_default_attributes(void)
 			return -1;
 		}
 		if (topdown_attrs[0] && str) {
+			struct parse_events_error errinfo;
 			if (warn)
 				arch_topdown_group_warn();
 setup_metrics:
+			parse_events_error__init(&errinfo);
 			err = parse_events(evsel_list, str, &errinfo);
 			if (err) {
 				fprintf(stderr,
 					"Cannot set up top down events %s: %d\n",
 					str, err);
 				parse_events_error__print(&errinfo, str);
+				parse_events_error__exit(&errinfo);
 				free(str);
 				return -1;
 			}
+			parse_events_error__exit(&errinfo);
 		} else {
 			fprintf(stderr, "System does not support topdown\n");
 			return -1;
@@ -1896,6 +1900,7 @@ static int add_default_attributes(void)
 
 	if (!evsel_list->core.nr_entries) {
 		if (perf_pmu__has_hybrid()) {
+			struct parse_events_error errinfo;
 			const char *hybrid_str = "cycles,instructions,branches,branch-misses";
 
 			if (target__has_cpu(&target))
@@ -1906,15 +1911,16 @@ static int add_default_attributes(void)
 				return -1;
 			}
 
+			parse_events_error__init(&errinfo);
 			err = parse_events(evsel_list, hybrid_str, &errinfo);
 			if (err) {
 				fprintf(stderr,
 					"Cannot set up hybrid events %s: %d\n",
 					hybrid_str, err);
 				parse_events_error__print(&errinfo, hybrid_str);
-				return -1;
 			}
-			return err;
+			parse_events_error__exit(&errinfo);
+			return err ? -1 : 0;
 		}
 
 		if (target__has_cpu(&target))
diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index 7f0acc94e9ac..624ea12ce5ca 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -3063,15 +3063,11 @@ static bool evlist__add_vfs_getname(struct evlist *evlist)
 	struct parse_events_error err;
 	int ret;
 
-	bzero(&err, sizeof(err));
+	parse_events_error__init(&err);
 	ret = parse_events(evlist, "probe:vfs_getname*", &err);
-	if (ret) {
-		free(err.str);
-		free(err.help);
-		free(err.first_str);
-		free(err.first_help);
+	parse_events_error__exit(&err);
+	if (ret)
 		return false;
-	}
 
 	evlist__for_each_entry_safe(evlist, evsel, tmp) {
 		if (!strstarts(evsel__name(evsel), "probe:vfs_getname"))
@@ -4925,12 +4921,13 @@ int cmd_trace(int argc, const char **argv)
 	if (trace.perfconfig_events != NULL) {
 		struct parse_events_error parse_err;
 
-		bzero(&parse_err, sizeof(parse_err));
+		parse_events_error__init(&parse_err);
 		err = parse_events(trace.evlist, trace.perfconfig_events, &parse_err);
-		if (err) {
+		if (err)
 			parse_events_error__print(&parse_err, trace.perfconfig_events);
+		parse_events_error__exit(&parse_err);
+		if (err)
 			goto out;
-		}
 	}
 
 	if ((nr_cgroups || trace.cgroup) && !trace.opts.target.system_wide) {
diff --git a/tools/perf/tests/backward-ring-buffer.c b/tools/perf/tests/backward-ring-buffer.c
index b4b9a9488d51..7447a4478991 100644
--- a/tools/perf/tests/backward-ring-buffer.c
+++ b/tools/perf/tests/backward-ring-buffer.c
@@ -115,12 +115,13 @@ int test__backward_ring_buffer(struct test *test __maybe_unused, int subtest __m
 		goto out_delete_evlist;
 	}
 
-	bzero(&parse_error, sizeof(parse_error));
+	parse_events_error__init(&parse_error);
 	/*
 	 * Set backward bit, ring buffer should be writing from end. Record
 	 * it in aux evlist
 	 */
 	err = parse_events(evlist, "syscalls:sys_enter_prctl/overwrite/", &parse_error);
+	parse_events_error__exit(&parse_error);
 	if (err) {
 		pr_debug("Failed to parse tracepoint event, try use root\n");
 		ret = TEST_SKIP;
diff --git a/tools/perf/tests/bpf.c b/tools/perf/tests/bpf.c
index fa03ff0dc083..2bf146e49ce8 100644
--- a/tools/perf/tests/bpf.c
+++ b/tools/perf/tests/bpf.c
@@ -123,12 +123,13 @@ static int do_test(struct bpf_object *obj, int (*func)(void),
 	struct parse_events_state parse_state;
 	struct parse_events_error parse_error;
 
-	bzero(&parse_error, sizeof(parse_error));
+	parse_events_error__init(&parse_error);
 	bzero(&parse_state, sizeof(parse_state));
 	parse_state.error = &parse_error;
 	INIT_LIST_HEAD(&parse_state.list);
 
 	err = parse_events_load_bpf_obj(&parse_state, &parse_state.list, obj, NULL);
+	parse_events_error__exit(&parse_error);
 	if (err || list_empty(&parse_state.list)) {
 		pr_debug("Failed to add events selected by BPF\n");
 		return TEST_FAIL;
diff --git a/tools/perf/tests/expand-cgroup.c b/tools/perf/tests/expand-cgroup.c
index 57b4c5f30324..80cff8a3558c 100644
--- a/tools/perf/tests/expand-cgroup.c
+++ b/tools/perf/tests/expand-cgroup.c
@@ -124,6 +124,7 @@ static int expand_group_events(void)
 	evlist = evlist__new();
 	TEST_ASSERT_VAL("failed to get evlist", evlist);
 
+	parse_events_error__init(&err);
 	ret = parse_events(evlist, event_str, &err);
 	if (ret < 0) {
 		pr_debug("failed to parse event '%s', err %d, str '%s'\n",
@@ -135,6 +136,7 @@ static int expand_group_events(void)
 	rblist__init(&metric_events);
 	ret = test_expand_events(evlist, &metric_events);
 out:
+	parse_events_error__exit(&err);
 	evlist__delete(evlist);
 	return ret;
 }
diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
index e200af986613..6af94639b14a 100644
--- a/tools/perf/tests/parse-events.c
+++ b/tools/perf/tests/parse-events.c
@@ -2045,7 +2045,6 @@ static int test_event(struct evlist_test *e)
 	struct evlist *evlist;
 	int ret;
 
-	bzero(&err, sizeof(err));
 	if (e->valid && !e->valid()) {
 		pr_debug("... SKIP");
 		return 0;
@@ -2055,6 +2054,7 @@ static int test_event(struct evlist_test *e)
 	if (evlist == NULL)
 		return -ENOMEM;
 
+	parse_events_error__init(&err);
 	ret = parse_events(evlist, e->name, &err);
 	if (ret) {
 		pr_debug("failed to parse event '%s', err %d, str '%s'\n",
@@ -2063,7 +2063,7 @@ static int test_event(struct evlist_test *e)
 	} else {
 		ret = e->check(evlist);
 	}
-
+	parse_events_error__exit(&err);
 	evlist__delete(evlist);
 
 	return ret;
diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
index 50b1299fe643..9ae894c406d8 100644
--- a/tools/perf/tests/pmu-events.c
+++ b/tools/perf/tests/pmu-events.c
@@ -787,9 +787,11 @@ static int check_parse_id(const char *id, struct parse_events_error *error,
 
 static int check_parse_cpu(const char *id, bool same_cpu, const struct pmu_event *pe)
 {
-	struct parse_events_error error = { .idx = 0, };
+	struct parse_events_error error;
+	int ret;
 
-	int ret = check_parse_id(id, &error, NULL);
+	parse_events_error__init(&error);
+	ret = check_parse_id(id, &error, NULL);
 	if (ret && same_cpu) {
 		pr_warning("Parse event failed metric '%s' id '%s' expr '%s'\n",
 			pe->metric_name, id, pe->metric_expr);
@@ -800,22 +802,18 @@ static int check_parse_cpu(const char *id, bool same_cpu, const struct pmu_event
 			  id, pe->metric_name, pe->metric_expr);
 		ret = 0;
 	}
-	free(error.str);
-	free(error.help);
-	free(error.first_str);
-	free(error.first_help);
+	parse_events_error__exit(&error);
 	return ret;
 }
 
 static int check_parse_fake(const char *id)
 {
-	struct parse_events_error error = { .idx = 0, };
-	int ret = check_parse_id(id, &error, &perf_pmu__fake);
+	struct parse_events_error error;
+	int ret;
 
-	free(error.str);
-	free(error.help);
-	free(error.first_str);
-	free(error.first_help);
+	parse_events_error__init(&error);
+	ret = check_parse_id(id, &error, &perf_pmu__fake);
+	parse_events_error__exit(&error);
 	return ret;
 }
 
diff --git a/tools/perf/tests/topology.c b/tools/perf/tests/topology.c
index b9028e304ddd..4574c46260d9 100644
--- a/tools/perf/tests/topology.c
+++ b/tools/perf/tests/topology.c
@@ -49,7 +49,9 @@ static int session_write_header(char *path)
 
 		session->evlist = evlist__new();
 		TEST_ASSERT_VAL("can't get evlist", session->evlist);
+		parse_events_error__init(&err);
 		parse_events(session->evlist, "cpu_core/cycles/", &err);
+		parse_events_error__exit(&err);
 	}
 
 	perf_header__set_feat(&session->header, HEADER_CPU_TOPOLOGY);
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index edd7180b24e4..1b43cbc1961d 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -1339,7 +1339,7 @@ static int parse_ids(struct perf_pmu *fake_pmu, struct expr_parse_ctx *ids,
 		goto err_out;
 	}
 	pr_debug("Parsing metric events '%s'\n", events.buf);
-	bzero(&parse_error, sizeof(parse_error));
+	parse_events_error__init(&parse_error);
 	ret = __parse_events(parsed_evlist, events.buf, &parse_error, fake_pmu);
 	if (ret) {
 		parse_events_error__print(&parse_error, events.buf);
@@ -1352,6 +1352,7 @@ static int parse_ids(struct perf_pmu *fake_pmu, struct expr_parse_ctx *ids,
 	*out_evlist = parsed_evlist;
 	parsed_evlist = NULL;
 err_out:
+	parse_events_error__exit(&parse_error);
 	evlist__delete(parsed_evlist);
 	strbuf_release(&events);
 	return ret;
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 75cafb9a0720..5bfb6f892489 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -2301,6 +2301,19 @@ int __parse_events(struct evlist *evlist, const char *str,
 	return ret;
 }
 
+void parse_events_error__init(struct parse_events_error *err)
+{
+	bzero(err, sizeof(*err));
+}
+
+void parse_events_error__exit(struct parse_events_error *err)
+{
+	zfree(&err->str);
+	zfree(&err->help);
+	zfree(&err->first_str);
+	zfree(&err->first_help);
+}
+
 void parse_events_error__handle(struct parse_events_error *err, int idx,
 				char *str, char *help)
 {
@@ -2405,15 +2418,11 @@ void parse_events_error__print(struct parse_events_error *err,
 		return;
 
 	__parse_events_error__print(err->idx, err->str, err->help, event);
-	zfree(&err->str);
-	zfree(&err->help);
 
 	if (err->num_errors > 1) {
 		fputs("\nInitial error:\n", stderr);
 		__parse_events_error__print(err->first_idx, err->first_str,
 					err->first_help, event);
-		zfree(&err->first_str);
-		zfree(&err->first_help);
 	}
 }
 
@@ -2426,13 +2435,14 @@ int parse_events_option(const struct option *opt, const char *str,
 	struct parse_events_error err;
 	int ret;
 
-	bzero(&err, sizeof(err));
+	parse_events_error__init(&err);
 	ret = parse_events(evlist, str, &err);
 
 	if (ret) {
 		parse_events_error__print(&err, str);
 		fprintf(stderr, "Run 'perf list' for a list of valid events\n");
 	}
+	parse_events_error__exit(&err);
 
 	return ret;
 }
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index 52ac26b3720a..c7fc93f54577 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -242,6 +242,8 @@ int is_valid_tracepoint(const char *event_string);
 int valid_event_mount(const char *eventfs);
 char *parse_events_formats_error_string(char *additional_terms);
 
+void parse_events_error__init(struct parse_events_error *err);
+void parse_events_error__exit(struct parse_events_error *err);
 void parse_events_error__handle(struct parse_events_error *err, int idx,
 				char *str, char *help);
 void parse_events_error__print(struct parse_events_error *err,
-- 
2.34.0.rc0.344.g81b53c2807-goog

Re: [PATCH v2 2/3] perf parse-event: Add init and exit to parse_event_error

[Arnaldo Carvalho de Melo]

Em Sun, Nov 07, 2021 at 01:00:01AM -0800, Ian Rogers escreveu:
> parse_events may succeed but leave string memory allocations reachable
> in the error. Add an init/exit that must be called to initialize and
> clean up the error. This fixes a leak in metricgroup parse_ids.

A bit big, could've been split in more patches, but I couldn't find
problems, so I'm applying.

- Arnaldo
 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/arch/powerpc/util/kvm-stat.c |  3 +-
>  tools/perf/bench/evlist-open-close.c    |  6 ++--
>  tools/perf/builtin-stat.c               | 38 ++++++++++++++-----------
>  tools/perf/builtin-trace.c              | 17 +++++------
>  tools/perf/tests/backward-ring-buffer.c |  3 +-
>  tools/perf/tests/bpf.c                  |  3 +-
>  tools/perf/tests/expand-cgroup.c        |  2 ++
>  tools/perf/tests/parse-events.c         |  4 +--
>  tools/perf/tests/pmu-events.c           | 22 +++++++-------
>  tools/perf/tests/topology.c             |  2 ++
>  tools/perf/util/metricgroup.c           |  3 +-
>  tools/perf/util/parse-events.c          | 20 +++++++++----
>  tools/perf/util/parse-events.h          |  2 ++
>  13 files changed, 74 insertions(+), 51 deletions(-)
> 
> diff --git a/tools/perf/arch/powerpc/util/kvm-stat.c b/tools/perf/arch/powerpc/util/kvm-stat.c
> index 16510686c138..2a74bec15a3e 100644
> --- a/tools/perf/arch/powerpc/util/kvm-stat.c
> +++ b/tools/perf/arch/powerpc/util/kvm-stat.c
> @@ -113,10 +113,11 @@ static int is_tracepoint_available(const char *str, struct evlist *evlist)
>  	struct parse_events_error err;
>  	int ret;
>  
> -	bzero(&err, sizeof(err));
> +	parse_events_error__init(&err);
>  	ret = parse_events(evlist, str, &err);
>  	if (err.str)
>  		parse_events_print_error(&err, "tracepoint");
> +	parse_events_error__exit(&err);
>  	return ret;
>  }
>  
> diff --git a/tools/perf/bench/evlist-open-close.c b/tools/perf/bench/evlist-open-close.c
> index 3f9518936367..482738e9bdad 100644
> --- a/tools/perf/bench/evlist-open-close.c
> +++ b/tools/perf/bench/evlist-open-close.c
> @@ -78,7 +78,7 @@ static int evlist__count_evsel_fds(struct evlist *evlist)
>  
>  static struct evlist *bench__create_evlist(char *evstr)
>  {
> -	struct parse_events_error err = { .idx = 0, };
> +	struct parse_events_error err;
>  	struct evlist *evlist = evlist__new();
>  	int ret;
>  
> @@ -87,14 +87,16 @@ static struct evlist *bench__create_evlist(char *evstr)
>  		return NULL;
>  	}
>  
> +	parse_events_error__init(&err);
>  	ret = parse_events(evlist, evstr, &err);
>  	if (ret) {
>  		parse_events_error__print(&err, evstr);
> +		parse_events_error__exit(&err);
>  		pr_err("Run 'perf list' for a list of valid events\n");
>  		ret = 1;
>  		goto out_delete_evlist;
>  	}
> -
> +	parse_events_error__exit(&err);
>  	ret = evlist__create_maps(evlist, &opts.target);
>  	if (ret < 0) {
>  		pr_err("Not enough memory to create thread/cpu maps\n");
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index af447a179d84..7974933dbc77 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -1750,14 +1750,12 @@ static int add_default_attributes(void)
>  	(PERF_COUNT_HW_CACHE_OP_PREFETCH	<<  8) |
>  	(PERF_COUNT_HW_CACHE_RESULT_MISS	<< 16)				},
>  };
> -	struct parse_events_error errinfo;
> -
>  	/* Set attrs if no event is selected and !null_run: */
>  	if (stat_config.null_run)
>  		return 0;
>  
> -	bzero(&errinfo, sizeof(errinfo));
>  	if (transaction_run) {
> +		struct parse_events_error errinfo;
>  		/* Handle -T as -M transaction. Once platform specific metrics
>  		 * support has been added to the json files, all architectures
>  		 * will use this approach. To determine transaction support
> @@ -1772,6 +1770,7 @@ static int add_default_attributes(void)
>  							 &stat_config.metric_events);
>  		}
>  
> +		parse_events_error__init(&errinfo);
>  		if (pmu_have_event("cpu", "cycles-ct") &&
>  		    pmu_have_event("cpu", "el-start"))
>  			err = parse_events(evsel_list, transaction_attrs,
> @@ -1783,12 +1782,13 @@ static int add_default_attributes(void)
>  		if (err) {
>  			fprintf(stderr, "Cannot set up transaction events\n");
>  			parse_events_error__print(&errinfo, transaction_attrs);
> -			return -1;
>  		}
> -		return 0;
> +		parse_events_error__exit(&errinfo);
> +		return err ? -1 : 0;
>  	}
>  
>  	if (smi_cost) {
> +		struct parse_events_error errinfo;
>  		int smi;
>  
>  		if (sysfs__read_int(FREEZE_ON_SMI_PATH, &smi) < 0) {
> @@ -1804,23 +1804,23 @@ static int add_default_attributes(void)
>  			smi_reset = true;
>  		}
>  
> -		if (pmu_have_event("msr", "aperf") &&
> -		    pmu_have_event("msr", "smi")) {
> -			if (!force_metric_only)
> -				stat_config.metric_only = true;
> -			err = parse_events(evsel_list, smi_cost_attrs, &errinfo);
> -		} else {
> +		if (!pmu_have_event("msr", "aperf") ||
> +		    !pmu_have_event("msr", "smi")) {
>  			fprintf(stderr, "To measure SMI cost, it needs "
>  				"msr/aperf/, msr/smi/ and cpu/cycles/ support\n");
> -			parse_events_error__print(&errinfo, smi_cost_attrs);
>  			return -1;
>  		}
> +		if (!force_metric_only)
> +			stat_config.metric_only = true;
> +
> +		parse_events_error__init(&errinfo);
> +		err = parse_events(evsel_list, smi_cost_attrs, &errinfo);
>  		if (err) {
>  			parse_events_error__print(&errinfo, smi_cost_attrs);
>  			fprintf(stderr, "Cannot set up SMI cost events\n");
> -			return -1;
>  		}
> -		return 0;
> +		parse_events_error__exit(&errinfo);
> +		return err ? -1 : 0;
>  	}
>  
>  	if (topdown_run) {
> @@ -1875,18 +1875,22 @@ static int add_default_attributes(void)
>  			return -1;
>  		}
>  		if (topdown_attrs[0] && str) {
> +			struct parse_events_error errinfo;
>  			if (warn)
>  				arch_topdown_group_warn();
>  setup_metrics:
> +			parse_events_error__init(&errinfo);
>  			err = parse_events(evsel_list, str, &errinfo);
>  			if (err) {
>  				fprintf(stderr,
>  					"Cannot set up top down events %s: %d\n",
>  					str, err);
>  				parse_events_error__print(&errinfo, str);
> +				parse_events_error__exit(&errinfo);
>  				free(str);
>  				return -1;
>  			}
> +			parse_events_error__exit(&errinfo);
>  		} else {
>  			fprintf(stderr, "System does not support topdown\n");
>  			return -1;
> @@ -1896,6 +1900,7 @@ static int add_default_attributes(void)
>  
>  	if (!evsel_list->core.nr_entries) {
>  		if (perf_pmu__has_hybrid()) {
> +			struct parse_events_error errinfo;
>  			const char *hybrid_str = "cycles,instructions,branches,branch-misses";
>  
>  			if (target__has_cpu(&target))
> @@ -1906,15 +1911,16 @@ static int add_default_attributes(void)
>  				return -1;
>  			}
>  
> +			parse_events_error__init(&errinfo);
>  			err = parse_events(evsel_list, hybrid_str, &errinfo);
>  			if (err) {
>  				fprintf(stderr,
>  					"Cannot set up hybrid events %s: %d\n",
>  					hybrid_str, err);
>  				parse_events_error__print(&errinfo, hybrid_str);
> -				return -1;
>  			}
> -			return err;
> +			parse_events_error__exit(&errinfo);
> +			return err ? -1 : 0;
>  		}
>  
>  		if (target__has_cpu(&target))
> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index 7f0acc94e9ac..624ea12ce5ca 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -3063,15 +3063,11 @@ static bool evlist__add_vfs_getname(struct evlist *evlist)
>  	struct parse_events_error err;
>  	int ret;
>  
> -	bzero(&err, sizeof(err));
> +	parse_events_error__init(&err);
>  	ret = parse_events(evlist, "probe:vfs_getname*", &err);
> -	if (ret) {
> -		free(err.str);
> -		free(err.help);
> -		free(err.first_str);
> -		free(err.first_help);
> +	parse_events_error__exit(&err);
> +	if (ret)
>  		return false;
> -	}
>  
>  	evlist__for_each_entry_safe(evlist, evsel, tmp) {
>  		if (!strstarts(evsel__name(evsel), "probe:vfs_getname"))
> @@ -4925,12 +4921,13 @@ int cmd_trace(int argc, const char **argv)
>  	if (trace.perfconfig_events != NULL) {
>  		struct parse_events_error parse_err;
>  
> -		bzero(&parse_err, sizeof(parse_err));
> +		parse_events_error__init(&parse_err);
>  		err = parse_events(trace.evlist, trace.perfconfig_events, &parse_err);
> -		if (err) {
> +		if (err)
>  			parse_events_error__print(&parse_err, trace.perfconfig_events);
> +		parse_events_error__exit(&parse_err);
> +		if (err)
>  			goto out;
> -		}
>  	}
>  
>  	if ((nr_cgroups || trace.cgroup) && !trace.opts.target.system_wide) {
> diff --git a/tools/perf/tests/backward-ring-buffer.c b/tools/perf/tests/backward-ring-buffer.c
> index b4b9a9488d51..7447a4478991 100644
> --- a/tools/perf/tests/backward-ring-buffer.c
> +++ b/tools/perf/tests/backward-ring-buffer.c
> @@ -115,12 +115,13 @@ int test__backward_ring_buffer(struct test *test __maybe_unused, int subtest __m
>  		goto out_delete_evlist;
>  	}
>  
> -	bzero(&parse_error, sizeof(parse_error));
> +	parse_events_error__init(&parse_error);
>  	/*
>  	 * Set backward bit, ring buffer should be writing from end. Record
>  	 * it in aux evlist
>  	 */
>  	err = parse_events(evlist, "syscalls:sys_enter_prctl/overwrite/", &parse_error);
> +	parse_events_error__exit(&parse_error);
>  	if (err) {
>  		pr_debug("Failed to parse tracepoint event, try use root\n");
>  		ret = TEST_SKIP;
> diff --git a/tools/perf/tests/bpf.c b/tools/perf/tests/bpf.c
> index fa03ff0dc083..2bf146e49ce8 100644
> --- a/tools/perf/tests/bpf.c
> +++ b/tools/perf/tests/bpf.c
> @@ -123,12 +123,13 @@ static int do_test(struct bpf_object *obj, int (*func)(void),
>  	struct parse_events_state parse_state;
>  	struct parse_events_error parse_error;
>  
> -	bzero(&parse_error, sizeof(parse_error));
> +	parse_events_error__init(&parse_error);
>  	bzero(&parse_state, sizeof(parse_state));
>  	parse_state.error = &parse_error;
>  	INIT_LIST_HEAD(&parse_state.list);
>  
>  	err = parse_events_load_bpf_obj(&parse_state, &parse_state.list, obj, NULL);
> +	parse_events_error__exit(&parse_error);
>  	if (err || list_empty(&parse_state.list)) {
>  		pr_debug("Failed to add events selected by BPF\n");
>  		return TEST_FAIL;
> diff --git a/tools/perf/tests/expand-cgroup.c b/tools/perf/tests/expand-cgroup.c
> index 57b4c5f30324..80cff8a3558c 100644
> --- a/tools/perf/tests/expand-cgroup.c
> +++ b/tools/perf/tests/expand-cgroup.c
> @@ -124,6 +124,7 @@ static int expand_group_events(void)
>  	evlist = evlist__new();
>  	TEST_ASSERT_VAL("failed to get evlist", evlist);
>  
> +	parse_events_error__init(&err);
>  	ret = parse_events(evlist, event_str, &err);
>  	if (ret < 0) {
>  		pr_debug("failed to parse event '%s', err %d, str '%s'\n",
> @@ -135,6 +136,7 @@ static int expand_group_events(void)
>  	rblist__init(&metric_events);
>  	ret = test_expand_events(evlist, &metric_events);
>  out:
> +	parse_events_error__exit(&err);
>  	evlist__delete(evlist);
>  	return ret;
>  }
> diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
> index e200af986613..6af94639b14a 100644
> --- a/tools/perf/tests/parse-events.c
> +++ b/tools/perf/tests/parse-events.c
> @@ -2045,7 +2045,6 @@ static int test_event(struct evlist_test *e)
>  	struct evlist *evlist;
>  	int ret;
>  
> -	bzero(&err, sizeof(err));
>  	if (e->valid && !e->valid()) {
>  		pr_debug("... SKIP");
>  		return 0;
> @@ -2055,6 +2054,7 @@ static int test_event(struct evlist_test *e)
>  	if (evlist == NULL)
>  		return -ENOMEM;
>  
> +	parse_events_error__init(&err);
>  	ret = parse_events(evlist, e->name, &err);
>  	if (ret) {
>  		pr_debug("failed to parse event '%s', err %d, str '%s'\n",
> @@ -2063,7 +2063,7 @@ static int test_event(struct evlist_test *e)
>  	} else {
>  		ret = e->check(evlist);
>  	}
> -
> +	parse_events_error__exit(&err);
>  	evlist__delete(evlist);
>  
>  	return ret;
> diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
> index 50b1299fe643..9ae894c406d8 100644
> --- a/tools/perf/tests/pmu-events.c
> +++ b/tools/perf/tests/pmu-events.c
> @@ -787,9 +787,11 @@ static int check_parse_id(const char *id, struct parse_events_error *error,
>  
>  static int check_parse_cpu(const char *id, bool same_cpu, const struct pmu_event *pe)
>  {
> -	struct parse_events_error error = { .idx = 0, };
> +	struct parse_events_error error;
> +	int ret;
>  
> -	int ret = check_parse_id(id, &error, NULL);
> +	parse_events_error__init(&error);
> +	ret = check_parse_id(id, &error, NULL);
>  	if (ret && same_cpu) {
>  		pr_warning("Parse event failed metric '%s' id '%s' expr '%s'\n",
>  			pe->metric_name, id, pe->metric_expr);
> @@ -800,22 +802,18 @@ static int check_parse_cpu(const char *id, bool same_cpu, const struct pmu_event
>  			  id, pe->metric_name, pe->metric_expr);
>  		ret = 0;
>  	}
> -	free(error.str);
> -	free(error.help);
> -	free(error.first_str);
> -	free(error.first_help);
> +	parse_events_error__exit(&error);
>  	return ret;
>  }
>  
>  static int check_parse_fake(const char *id)
>  {
> -	struct parse_events_error error = { .idx = 0, };
> -	int ret = check_parse_id(id, &error, &perf_pmu__fake);
> +	struct parse_events_error error;
> +	int ret;
>  
> -	free(error.str);
> -	free(error.help);
> -	free(error.first_str);
> -	free(error.first_help);
> +	parse_events_error__init(&error);
> +	ret = check_parse_id(id, &error, &perf_pmu__fake);
> +	parse_events_error__exit(&error);
>  	return ret;
>  }
>  
> diff --git a/tools/perf/tests/topology.c b/tools/perf/tests/topology.c
> index b9028e304ddd..4574c46260d9 100644
> --- a/tools/perf/tests/topology.c
> +++ b/tools/perf/tests/topology.c
> @@ -49,7 +49,9 @@ static int session_write_header(char *path)
>  
>  		session->evlist = evlist__new();
>  		TEST_ASSERT_VAL("can't get evlist", session->evlist);
> +		parse_events_error__init(&err);
>  		parse_events(session->evlist, "cpu_core/cycles/", &err);
> +		parse_events_error__exit(&err);
>  	}
>  
>  	perf_header__set_feat(&session->header, HEADER_CPU_TOPOLOGY);
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index edd7180b24e4..1b43cbc1961d 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -1339,7 +1339,7 @@ static int parse_ids(struct perf_pmu *fake_pmu, struct expr_parse_ctx *ids,
>  		goto err_out;
>  	}
>  	pr_debug("Parsing metric events '%s'\n", events.buf);
> -	bzero(&parse_error, sizeof(parse_error));
> +	parse_events_error__init(&parse_error);
>  	ret = __parse_events(parsed_evlist, events.buf, &parse_error, fake_pmu);
>  	if (ret) {
>  		parse_events_error__print(&parse_error, events.buf);
> @@ -1352,6 +1352,7 @@ static int parse_ids(struct perf_pmu *fake_pmu, struct expr_parse_ctx *ids,
>  	*out_evlist = parsed_evlist;
>  	parsed_evlist = NULL;
>  err_out:
> +	parse_events_error__exit(&parse_error);
>  	evlist__delete(parsed_evlist);
>  	strbuf_release(&events);
>  	return ret;
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 75cafb9a0720..5bfb6f892489 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -2301,6 +2301,19 @@ int __parse_events(struct evlist *evlist, const char *str,
>  	return ret;
>  }
>  
> +void parse_events_error__init(struct parse_events_error *err)
> +{
> +	bzero(err, sizeof(*err));
> +}
> +
> +void parse_events_error__exit(struct parse_events_error *err)
> +{
> +	zfree(&err->str);
> +	zfree(&err->help);
> +	zfree(&err->first_str);
> +	zfree(&err->first_help);
> +}
> +
>  void parse_events_error__handle(struct parse_events_error *err, int idx,
>  				char *str, char *help)
>  {
> @@ -2405,15 +2418,11 @@ void parse_events_error__print(struct parse_events_error *err,
>  		return;
>  
>  	__parse_events_error__print(err->idx, err->str, err->help, event);
> -	zfree(&err->str);
> -	zfree(&err->help);
>  
>  	if (err->num_errors > 1) {
>  		fputs("\nInitial error:\n", stderr);
>  		__parse_events_error__print(err->first_idx, err->first_str,
>  					err->first_help, event);
> -		zfree(&err->first_str);
> -		zfree(&err->first_help);
>  	}
>  }
>  
> @@ -2426,13 +2435,14 @@ int parse_events_option(const struct option *opt, const char *str,
>  	struct parse_events_error err;
>  	int ret;
>  
> -	bzero(&err, sizeof(err));
> +	parse_events_error__init(&err);
>  	ret = parse_events(evlist, str, &err);
>  
>  	if (ret) {
>  		parse_events_error__print(&err, str);
>  		fprintf(stderr, "Run 'perf list' for a list of valid events\n");
>  	}
> +	parse_events_error__exit(&err);
>  
>  	return ret;
>  }
> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> index 52ac26b3720a..c7fc93f54577 100644
> --- a/tools/perf/util/parse-events.h
> +++ b/tools/perf/util/parse-events.h
> @@ -242,6 +242,8 @@ int is_valid_tracepoint(const char *event_string);
>  int valid_event_mount(const char *eventfs);
>  char *parse_events_formats_error_string(char *additional_terms);
>  
> +void parse_events_error__init(struct parse_events_error *err);
> +void parse_events_error__exit(struct parse_events_error *err);
>  void parse_events_error__handle(struct parse_events_error *err, int idx,
>  				char *str, char *help);
>  void parse_events_error__print(struct parse_events_error *err,
> -- 
> 2.34.0.rc0.344.g81b53c2807-goog

-- 

- Arnaldo

[PATCH v2 3/3] perf metric: Fix memory leaks.

[Ian Rogers]

Certain error paths may leak memory as caught by address sanitizer.
Ensure this is cleaned up to make sure address/leak sanitizer is happy.

Fixes: 5ecd5a0c7d1c ("perf metrics: Modify setup and deduplication")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/metricgroup.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 1b43cbc1961d..fffe02aae3ed 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -228,6 +228,7 @@ static void metric__free(struct metric *m)
 	free(m->metric_refs);
 	expr__ctx_free(m->pctx);
 	free((char *)m->modifier);
+	evlist__delete(m->evlist);
 	free(m);
 }
 
@@ -1482,8 +1483,10 @@ static int parse_groups(struct evlist *perf_evlist, const char *str,
 	}
 
 
-	if (combined_evlist)
+	if (combined_evlist) {
 		evlist__splice_list_tail(perf_evlist, &combined_evlist->core.entries);
+		evlist__delete(combined_evlist);
+	}
 
 	list_for_each_entry(m, &metric_list, nd) {
 		if (m->evlist)
-- 
2.34.0.rc0.344.g81b53c2807-goog

Re: [PATCH v2 3/3] perf metric: Fix memory leaks.

[Arnaldo Carvalho de Melo]

Em Sun, Nov 07, 2021 at 01:00:02AM -0800, Ian Rogers escreveu:
> Certain error paths may leak memory as caught by address sanitizer.
> Ensure this is cleaned up to make sure address/leak sanitizer is happy.

Thanks, applied.

- Arnaldo

 
> Fixes: 5ecd5a0c7d1c ("perf metrics: Modify setup and deduplication")
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/metricgroup.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 1b43cbc1961d..fffe02aae3ed 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -228,6 +228,7 @@ static void metric__free(struct metric *m)
>  	free(m->metric_refs);
>  	expr__ctx_free(m->pctx);
>  	free((char *)m->modifier);
> +	evlist__delete(m->evlist);
>  	free(m);
>  }
>  
> @@ -1482,8 +1483,10 @@ static int parse_groups(struct evlist *perf_evlist, const char *str,
>  	}
>  
>  
> -	if (combined_evlist)
> +	if (combined_evlist) {
>  		evlist__splice_list_tail(perf_evlist, &combined_evlist->core.entries);
> +		evlist__delete(combined_evlist);
> +	}
>  
>  	list_for_each_entry(m, &metric_list, nd) {
>  		if (m->evlist)
> -- 
> 2.34.0.rc0.344.g81b53c2807-goog

-- 

- Arnaldo

Re: [PATCH v2 1/3] perf parse-events: Rename parse_events_error functions

[Arnaldo Carvalho de Melo]

Em Sun, Nov 07, 2021 at 01:00:00AM -0800, Ian Rogers escreveu:
> Group error functions and name after the data type they manipulate.

Sensible, applied.

- Arnaldo
 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/bench/evlist-open-close.c |   2 +-
>  tools/perf/builtin-stat.c            |  10 +--
>  tools/perf/builtin-trace.c           |   2 +-
>  tools/perf/tests/expand-cgroup.c     |   2 +-
>  tools/perf/tests/parse-events.c      |   2 +-
>  tools/perf/util/metricgroup.c        |   2 +-
>  tools/perf/util/parse-events.c       | 116 +++++++++++++--------------
>  tools/perf/util/parse-events.h       |   8 +-
>  tools/perf/util/parse-events.y       |   4 +-
>  tools/perf/util/pmu.c                |   8 +-
>  10 files changed, 78 insertions(+), 78 deletions(-)
> 
> diff --git a/tools/perf/bench/evlist-open-close.c b/tools/perf/bench/evlist-open-close.c
> index 75a53919126b..3f9518936367 100644
> --- a/tools/perf/bench/evlist-open-close.c
> +++ b/tools/perf/bench/evlist-open-close.c
> @@ -89,7 +89,7 @@ static struct evlist *bench__create_evlist(char *evstr)
>  
>  	ret = parse_events(evlist, evstr, &err);
>  	if (ret) {
> -		parse_events_print_error(&err, evstr);
> +		parse_events_error__print(&err, evstr);
>  		pr_err("Run 'perf list' for a list of valid events\n");
>  		ret = 1;
>  		goto out_delete_evlist;
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index f0ecfda34ece..af447a179d84 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -1782,7 +1782,7 @@ static int add_default_attributes(void)
>  					   &errinfo);
>  		if (err) {
>  			fprintf(stderr, "Cannot set up transaction events\n");
> -			parse_events_print_error(&errinfo, transaction_attrs);
> +			parse_events_error__print(&errinfo, transaction_attrs);
>  			return -1;
>  		}
>  		return 0;
> @@ -1812,11 +1812,11 @@ static int add_default_attributes(void)
>  		} else {
>  			fprintf(stderr, "To measure SMI cost, it needs "
>  				"msr/aperf/, msr/smi/ and cpu/cycles/ support\n");
> -			parse_events_print_error(&errinfo, smi_cost_attrs);
> +			parse_events_error__print(&errinfo, smi_cost_attrs);
>  			return -1;
>  		}
>  		if (err) {
> -			parse_events_print_error(&errinfo, smi_cost_attrs);
> +			parse_events_error__print(&errinfo, smi_cost_attrs);
>  			fprintf(stderr, "Cannot set up SMI cost events\n");
>  			return -1;
>  		}
> @@ -1883,7 +1883,7 @@ static int add_default_attributes(void)
>  				fprintf(stderr,
>  					"Cannot set up top down events %s: %d\n",
>  					str, err);
> -				parse_events_print_error(&errinfo, str);
> +				parse_events_error__print(&errinfo, str);
>  				free(str);
>  				return -1;
>  			}
> @@ -1911,7 +1911,7 @@ static int add_default_attributes(void)
>  				fprintf(stderr,
>  					"Cannot set up hybrid events %s: %d\n",
>  					hybrid_str, err);
> -				parse_events_print_error(&errinfo, hybrid_str);
> +				parse_events_error__print(&errinfo, hybrid_str);
>  				return -1;
>  			}
>  			return err;
> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index 2f1d20553a0a..7f0acc94e9ac 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -4928,7 +4928,7 @@ int cmd_trace(int argc, const char **argv)
>  		bzero(&parse_err, sizeof(parse_err));
>  		err = parse_events(trace.evlist, trace.perfconfig_events, &parse_err);
>  		if (err) {
> -			parse_events_print_error(&parse_err, trace.perfconfig_events);
> +			parse_events_error__print(&parse_err, trace.perfconfig_events);
>  			goto out;
>  		}
>  	}
> diff --git a/tools/perf/tests/expand-cgroup.c b/tools/perf/tests/expand-cgroup.c
> index aaad51aba12f..57b4c5f30324 100644
> --- a/tools/perf/tests/expand-cgroup.c
> +++ b/tools/perf/tests/expand-cgroup.c
> @@ -128,7 +128,7 @@ static int expand_group_events(void)
>  	if (ret < 0) {
>  		pr_debug("failed to parse event '%s', err %d, str '%s'\n",
>  			 event_str, ret, err.str);
> -		parse_events_print_error(&err, event_str);
> +		parse_events_error__print(&err, event_str);
>  		goto out;
>  	}
>  
> diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
> index 8875e388563e..e200af986613 100644
> --- a/tools/perf/tests/parse-events.c
> +++ b/tools/perf/tests/parse-events.c
> @@ -2059,7 +2059,7 @@ static int test_event(struct evlist_test *e)
>  	if (ret) {
>  		pr_debug("failed to parse event '%s', err %d, str '%s'\n",
>  			 e->name, ret, err.str);
> -		parse_events_print_error(&err, e->name);
> +		parse_events_error__print(&err, e->name);
>  	} else {
>  		ret = e->check(evlist);
>  	}
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 4917e9704765..edd7180b24e4 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -1342,7 +1342,7 @@ static int parse_ids(struct perf_pmu *fake_pmu, struct expr_parse_ctx *ids,
>  	bzero(&parse_error, sizeof(parse_error));
>  	ret = __parse_events(parsed_evlist, events.buf, &parse_error, fake_pmu);
>  	if (ret) {
> -		parse_events_print_error(&parse_error, events.buf);
> +		parse_events_error__print(&parse_error, events.buf);
>  		goto err_out;
>  	}
>  	ret = decode_all_metric_ids(parsed_evlist, modifier);
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 278199ed788b..75cafb9a0720 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -191,39 +191,6 @@ static int tp_event_has_id(const char *dir_path, struct dirent *evt_dir)
>  
>  #define MAX_EVENT_LENGTH 512
>  
> -void parse_events__handle_error(struct parse_events_error *err, int idx,
> -				char *str, char *help)
> -{
> -	if (WARN(!str, "WARNING: failed to provide error string\n")) {
> -		free(help);
> -		return;
> -	}
> -	switch (err->num_errors) {
> -	case 0:
> -		err->idx = idx;
> -		err->str = str;
> -		err->help = help;
> -		break;
> -	case 1:
> -		err->first_idx = err->idx;
> -		err->idx = idx;
> -		err->first_str = err->str;
> -		err->str = str;
> -		err->first_help = err->help;
> -		err->help = help;
> -		break;
> -	default:
> -		pr_debug("Multiple errors dropping message: %s (%s)\n",
> -			err->str, err->help);
> -		free(err->str);
> -		err->str = str;
> -		free(err->help);
> -		err->help = help;
> -		break;
> -	}
> -	err->num_errors++;
> -}
> -
>  struct tracepoint_path *tracepoint_id_to_path(u64 config)
>  {
>  	struct tracepoint_path *path = NULL;
> @@ -587,7 +554,7 @@ static void tracepoint_error(struct parse_events_error *e, int err,
>  	}
>  
>  	tracing_path__strerror_open_tp(err, help, sizeof(help), sys, name);
> -	parse_events__handle_error(e, 0, strdup(str), strdup(help));
> +	parse_events_error__handle(e, 0, strdup(str), strdup(help));
>  }
>  
>  static int add_tracepoint(struct list_head *list, int *idx,
> @@ -811,7 +778,7 @@ int parse_events_load_bpf_obj(struct parse_events_state *parse_state,
>  
>  	return 0;
>  errout:
> -	parse_events__handle_error(parse_state->error, 0,
> +	parse_events_error__handle(parse_state->error, 0,
>  				strdup(errbuf), strdup("(add -v to see detail)"));
>  	return err;
>  }
> @@ -831,7 +798,7 @@ parse_events_config_bpf(struct parse_events_state *parse_state,
>  		int err;
>  
>  		if (term->type_term != PARSE_EVENTS__TERM_TYPE_USER) {
> -			parse_events__handle_error(parse_state->error, term->err_term,
> +			parse_events_error__handle(parse_state->error, term->err_term,
>  						strdup("Invalid config term for BPF object"),
>  						NULL);
>  			return -EINVAL;
> @@ -851,7 +818,7 @@ parse_events_config_bpf(struct parse_events_state *parse_state,
>  			else
>  				idx = term->err_term + error_pos;
>  
> -			parse_events__handle_error(parse_state->error, idx,
> +			parse_events_error__handle(parse_state->error, idx,
>  						strdup(errbuf),
>  						strdup(
>  "Hint:\tValid config terms:\n"
> @@ -923,7 +890,7 @@ int parse_events_load_bpf(struct parse_events_state *parse_state,
>  						   -err, errbuf,
>  						   sizeof(errbuf));
>  
> -		parse_events__handle_error(parse_state->error, 0,
> +		parse_events_error__handle(parse_state->error, 0,
>  					strdup(errbuf), strdup("(add -v to see detail)"));
>  		return err;
>  	}
> @@ -947,7 +914,7 @@ int parse_events_load_bpf_obj(struct parse_events_state *parse_state,
>  			      struct bpf_object *obj __maybe_unused,
>  			      struct list_head *head_config __maybe_unused)
>  {
> -	parse_events__handle_error(parse_state->error, 0,
> +	parse_events_error__handle(parse_state->error, 0,
>  				   strdup("BPF support is not compiled"),
>  				   strdup("Make sure libbpf-devel is available at build time."));
>  	return -ENOTSUP;
> @@ -959,7 +926,7 @@ int parse_events_load_bpf(struct parse_events_state *parse_state,
>  			  bool source __maybe_unused,
>  			  struct list_head *head_config __maybe_unused)
>  {
> -	parse_events__handle_error(parse_state->error, 0,
> +	parse_events_error__handle(parse_state->error, 0,
>  				   strdup("BPF support is not compiled"),
>  				   strdup("Make sure libbpf-devel is available at build time."));
>  	return -ENOTSUP;
> @@ -1042,7 +1009,7 @@ static int check_type_val(struct parse_events_term *term,
>  		return 0;
>  
>  	if (err) {
> -		parse_events__handle_error(err, term->err_val,
> +		parse_events_error__handle(err, term->err_val,
>  					type == PARSE_EVENTS__TERM_TYPE_NUM
>  					? strdup("expected numeric value")
>  					: strdup("expected string value"),
> @@ -1087,7 +1054,7 @@ config_term_avail(int term_type, struct parse_events_error *err)
>  	char *err_str;
>  
>  	if (term_type < 0 || term_type >= __PARSE_EVENTS__TERM_TYPE_NR) {
> -		parse_events__handle_error(err, -1,
> +		parse_events_error__handle(err, -1,
>  					strdup("Invalid term_type"), NULL);
>  		return false;
>  	}
> @@ -1110,7 +1077,7 @@ config_term_avail(int term_type, struct parse_events_error *err)
>  		/* term_type is validated so indexing is safe */
>  		if (asprintf(&err_str, "'%s' is not usable in 'perf stat'",
>  				config_term_names[term_type]) >= 0)
> -			parse_events__handle_error(err, -1, err_str, NULL);
> +			parse_events_error__handle(err, -1, err_str, NULL);
>  		return false;
>  	}
>  }
> @@ -1154,7 +1121,7 @@ do {									   \
>  		if (strcmp(term->val.str, "no") &&
>  		    parse_branch_str(term->val.str,
>  				    &attr->branch_sample_type)) {
> -			parse_events__handle_error(err, term->err_val,
> +			parse_events_error__handle(err, term->err_val,
>  					strdup("invalid branch sample type"),
>  					NULL);
>  			return -EINVAL;
> @@ -1163,7 +1130,7 @@ do {									   \
>  	case PARSE_EVENTS__TERM_TYPE_TIME:
>  		CHECK_TYPE_VAL(NUM);
>  		if (term->val.num > 1) {
> -			parse_events__handle_error(err, term->err_val,
> +			parse_events_error__handle(err, term->err_val,
>  						strdup("expected 0 or 1"),
>  						NULL);
>  			return -EINVAL;
> @@ -1202,7 +1169,7 @@ do {									   \
>  	case PARSE_EVENTS__TERM_TYPE_PERCORE:
>  		CHECK_TYPE_VAL(NUM);
>  		if ((unsigned int)term->val.num > 1) {
> -			parse_events__handle_error(err, term->err_val,
> +			parse_events_error__handle(err, term->err_val,
>  						strdup("expected 0 or 1"),
>  						NULL);
>  			return -EINVAL;
> @@ -1214,14 +1181,14 @@ do {									   \
>  	case PARSE_EVENTS__TERM_TYPE_AUX_SAMPLE_SIZE:
>  		CHECK_TYPE_VAL(NUM);
>  		if (term->val.num > UINT_MAX) {
> -			parse_events__handle_error(err, term->err_val,
> +			parse_events_error__handle(err, term->err_val,
>  						strdup("too big"),
>  						NULL);
>  			return -EINVAL;
>  		}
>  		break;
>  	default:
> -		parse_events__handle_error(err, term->err_term,
> +		parse_events_error__handle(err, term->err_term,
>  				strdup("unknown term"),
>  				parse_events_formats_error_string(NULL));
>  		return -EINVAL;
> @@ -1275,7 +1242,7 @@ static int config_term_tracepoint(struct perf_event_attr *attr,
>  		return config_term_common(attr, term, err);
>  	default:
>  		if (err) {
> -			parse_events__handle_error(err, term->err_term,
> +			parse_events_error__handle(err, term->err_term,
>  				strdup("unknown term"),
>  				strdup("valid terms: call-graph,stack-size\n"));
>  		}
> @@ -1574,7 +1541,7 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
>  		if (asprintf(&err_str,
>  				"Cannot find PMU `%s'. Missing kernel support?",
>  				name) >= 0)
> -			parse_events__handle_error(err, 0, err_str, NULL);
> +			parse_events_error__handle(err, 0, err_str, NULL);
>  		return -EINVAL;
>  	}
>  
> @@ -2334,6 +2301,39 @@ int __parse_events(struct evlist *evlist, const char *str,
>  	return ret;
>  }
>  
> +void parse_events_error__handle(struct parse_events_error *err, int idx,
> +				char *str, char *help)
> +{
> +	if (WARN(!str, "WARNING: failed to provide error string\n")) {
> +		free(help);
> +		return;
> +	}
> +	switch (err->num_errors) {
> +	case 0:
> +		err->idx = idx;
> +		err->str = str;
> +		err->help = help;
> +		break;
> +	case 1:
> +		err->first_idx = err->idx;
> +		err->idx = idx;
> +		err->first_str = err->str;
> +		err->str = str;
> +		err->first_help = err->help;
> +		err->help = help;
> +		break;
> +	default:
> +		pr_debug("Multiple errors dropping message: %s (%s)\n",
> +			err->str, err->help);
> +		free(err->str);
> +		err->str = str;
> +		free(err->help);
> +		err->help = help;
> +		break;
> +	}
> +	err->num_errors++;
> +}
> +
>  #define MAX_WIDTH 1000
>  static int get_term_width(void)
>  {
> @@ -2343,8 +2343,8 @@ static int get_term_width(void)
>  	return ws.ws_col > MAX_WIDTH ? MAX_WIDTH : ws.ws_col;
>  }
>  
> -static void __parse_events_print_error(int err_idx, const char *err_str,
> -				const char *err_help, const char *event)
> +static void __parse_events_error__print(int err_idx, const char *err_str,
> +					const char *err_help, const char *event)
>  {
>  	const char *str = "invalid or unsupported event: ";
>  	char _buf[MAX_WIDTH];
> @@ -2398,19 +2398,19 @@ static void __parse_events_print_error(int err_idx, const char *err_str,
>  	}
>  }
>  
> -void parse_events_print_error(struct parse_events_error *err,
> -			      const char *event)
> +void parse_events_error__print(struct parse_events_error *err,
> +			       const char *event)
>  {
>  	if (!err->num_errors)
>  		return;
>  
> -	__parse_events_print_error(err->idx, err->str, err->help, event);
> +	__parse_events_error__print(err->idx, err->str, err->help, event);
>  	zfree(&err->str);
>  	zfree(&err->help);
>  
>  	if (err->num_errors > 1) {
>  		fputs("\nInitial error:\n", stderr);
> -		__parse_events_print_error(err->first_idx, err->first_str,
> +		__parse_events_error__print(err->first_idx, err->first_str,
>  					err->first_help, event);
>  		zfree(&err->first_str);
>  		zfree(&err->first_help);
> @@ -2430,7 +2430,7 @@ int parse_events_option(const struct option *opt, const char *str,
>  	ret = parse_events(evlist, str, &err);
>  
>  	if (ret) {
> -		parse_events_print_error(&err, str);
> +		parse_events_error__print(&err, str);
>  		fprintf(stderr, "Run 'perf list' for a list of valid events\n");
>  	}
>  
> @@ -3324,7 +3324,7 @@ void parse_events_evlist_error(struct parse_events_state *parse_state,
>  	if (!parse_state->error)
>  		return;
>  
> -	parse_events__handle_error(parse_state->error, idx, strdup(str), NULL);
> +	parse_events_error__handle(parse_state->error, idx, strdup(str), NULL);
>  }
>  
>  static void config_terms_list(char *buf, size_t buf_sz)
> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> index f60a661a2247..52ac26b3720a 100644
> --- a/tools/perf/util/parse-events.h
> +++ b/tools/perf/util/parse-events.h
> @@ -142,8 +142,6 @@ struct parse_events_state {
>  	char			  *hybrid_pmu_name;
>  };
>  
> -void parse_events__handle_error(struct parse_events_error *err, int idx,
> -				char *str, char *help);
>  void parse_events__shrink_config_terms(void);
>  int parse_events__is_hardcoded_term(struct parse_events_term *term);
>  int parse_events_term__num(struct parse_events_term **term,
> @@ -244,8 +242,10 @@ int is_valid_tracepoint(const char *event_string);
>  int valid_event_mount(const char *eventfs);
>  char *parse_events_formats_error_string(char *additional_terms);
>  
> -void parse_events_print_error(struct parse_events_error *err,
> -			      const char *event);
> +void parse_events_error__handle(struct parse_events_error *err, int idx,
> +				char *str, char *help);
> +void parse_events_error__print(struct parse_events_error *err,
> +			       const char *event);
>  
>  #ifdef HAVE_LIBELF_SUPPORT
>  /*
> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> index 2d60f3cbe42b..174158982fae 100644
> --- a/tools/perf/util/parse-events.y
> +++ b/tools/perf/util/parse-events.y
> @@ -186,7 +186,7 @@ group_def ':' PE_MODIFIER_EVENT
>  		struct parse_events_state *parse_state = _parse_state;
>  		struct parse_events_error *error = parse_state->error;
>  
> -		parse_events__handle_error(error, @3.first_column,
> +		parse_events_error__handle(error, @3.first_column,
>  					   strdup("Bad modifier"), NULL);
>  		free_list_evsel(list);
>  		YYABORT;
> @@ -248,7 +248,7 @@ event_name PE_MODIFIER_EVENT
>  		struct parse_events_state *parse_state = _parse_state;
>  		struct parse_events_error *error = parse_state->error;
>  
> -		parse_events__handle_error(error, @2.first_column,
> +		parse_events_error__handle(error, @2.first_column,
>  					   strdup("Bad modifier"), NULL);
>  		free_list_evsel(list);
>  		YYABORT;
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index f3072c71d132..6ae58406f4fc 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -1283,7 +1283,7 @@ static int pmu_config_term(const char *pmu_name,
>  			unknown_term = NULL;
>  		help_msg = parse_events_formats_error_string(pmu_term);
>  		if (err) {
> -			parse_events__handle_error(err, term->err_term,
> +			parse_events_error__handle(err, term->err_term,
>  						   unknown_term,
>  						   help_msg);
>  		} else {
> @@ -1316,7 +1316,7 @@ static int pmu_config_term(const char *pmu_name,
>  		if (term->no_value &&
>  		    bitmap_weight(format->bits, PERF_PMU_FORMAT_BITS) > 1) {
>  			if (err) {
> -				parse_events__handle_error(err, term->err_val,
> +				parse_events_error__handle(err, term->err_val,
>  					   strdup("no value assigned for term"),
>  					   NULL);
>  			}
> @@ -1331,7 +1331,7 @@ static int pmu_config_term(const char *pmu_name,
>  						term->config, term->val.str);
>  			}
>  			if (err) {
> -				parse_events__handle_error(err, term->err_val,
> +				parse_events_error__handle(err, term->err_val,
>  					strdup("expected numeric value"),
>  					NULL);
>  			}
> @@ -1348,7 +1348,7 @@ static int pmu_config_term(const char *pmu_name,
>  		if (err) {
>  			char *err_str;
>  
> -			parse_events__handle_error(err, term->err_val,
> +			parse_events_error__handle(err, term->err_val,
>  				asprintf(&err_str,
>  				    "value too big for format, maximum is %llu",
>  				    (unsigned long long)max_val) < 0
> -- 
> 2.34.0.rc0.344.g81b53c2807-goog

-- 

- Arnaldo

Re: [PATCH v2 1/3] perf parse-events: Rename parse_events_error functions

[Arnaldo Carvalho de Melo]

Em Sun, Nov 07, 2021 at 12:34:41PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Sun, Nov 07, 2021 at 01:00:00AM -0800, Ian Rogers escreveu:
> > Group error functions and name after the data type they manipulate.
> 
> Sensible, applied.

I'll fix these:

  62     6.89 ubuntu:16.04-x-powerpc        : FAIL gcc version 5.4.0 20160609 (Ubuntu 5.4.0-6ubuntu1~16.04.9)
    arch/powerpc/util/kvm-stat.c: In function 'is_tracepoint_available':
    arch/powerpc/util/kvm-stat.c:119:3: error: implicit declaration of function 'parse_events_print_error' [-Werror=implicit-function-declaration]
       parse_events_print_error(&err, "tracepoint");
       ^
    cc1: all warnings being treated as errors
    /git/perf-5.15.0/tools/build/Makefile.build:139: recipe for target 'util' failed
    make[5]: *** [util] Error 2
    /git/perf-5.15.0/tools/build/Makefile.build:139: recipe for target 'powerpc' failed
    make[4]: *** [powerpc] Error 2
    /git/perf-5.15.0/tools/build/Makefile.build:139: recipe for target 'arch' failed
    make[3]: *** [arch] Error 2
  63     7.19 ubuntu:16.04-x-powerpc64      : FAIL gcc version 5.4.0 20160609 (Ubuntu/IBM 5.4.0-6ubuntu1~16.04.9)
    arch/powerpc/util/kvm-stat.c: In function 'is_tracepoint_available':
    arch/powerpc/util/kvm-stat.c:119:3: error: implicit declaration of function 'parse_events_print_error' [-Werror=implicit-function-declaration]
       parse_events_print_error(&err, "tracepoint");
       ^
    cc1: all warnings being treated as errors
    /git/perf-5.15.0/tools/build/Makefile.build:139: recipe for target 'util' failed
    make[5]: *** [util] Error 2
    /git/perf-5.15.0/tools/build/Makefile.build:139: recipe for target 'powerpc' failed
    make[4]: *** [powerpc] Error 2
    /git/perf-5.15.0/tools/build/Makefile.build:139: recipe for target 'arch' failed
    make[3]: *** [arch] Error 2
  64     7.00 ubuntu:16.04-x-powerpc64el    : FAIL gcc version 5.4.0 20160609 (Ubuntu/IBM 5.4.0-6ubuntu1~16.04.9)
    arch/powerpc/util/kvm-stat.c: In function 'is_tracepoint_available':
    arch/powerpc/util/kvm-stat.c:119:3: error: implicit declaration of function 'parse_events_print_error' [-Werror=implicit-function-declaration]
       parse_events_print_error(&err, "tracepoint");
       ^
    cc1: all warnings being treated as errors
    /git/perf-5.15.0/tools/build/Makefile.build:139: recipe for target 'util' failed
    make[5]: *** [util] Error 2
    /git/perf-5.15.0/tools/build/Makefile.build:139: recipe for target 'powerpc' failed
    make[4]: *** [powerpc] Error 2
    /git/perf-5.15.0/tools/build/Makefile.build:139: recipe for target 'arch' failed
    make[3]: *** [arch] Error 2
  65    18.84 ubuntu:16.04-x-s390           : Ok   s390x-linux-gnu-gcc (Ubuntu 5.4.0-6ubuntu1~16.04.9) 5.4.0 20160609
  66    75.33 ubuntu:18.04                  : Ok   gcc (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0 , clang version 6.0.0-1ubuntu2 (tags/RELEASE_600/final)
  67    20.15 ubuntu:18.04-x-arm            : Ok   arm-linux-gnueabihf-gcc (Ubuntu/Linaro 7.5.0-3ubuntu1~18.04) 7.5.0
  68    20.15 ubuntu:18.04-x-arm64          : Ok   aarch64-linux-gnu-gcc (Ubuntu/Linaro 7.5.0-3ubuntu1~18.04) 7.5.0
  69    16.63 ubuntu:18.04-x-m68k           : Ok   m68k-linux-gnu-gcc (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0
  70     7.39 ubuntu:18.04-x-powerpc        : FAIL gcc version 7.5.0 (Ubuntu 7.5.0-3ubuntu1~18.04)
    arch/powerpc/util/kvm-stat.c: In function 'is_tracepoint_available':
    arch/powerpc/util/kvm-stat.c:119:3: error: implicit declaration of function 'parse_events_print_error'; did you mean 'parse_events_evlist_error'? [-Werror=implicit-function-declaration]
       parse_events_print_error(&err, "tracepoint");
       ^~~~~~~~~~~~~~~~~~~~~~~~
       parse_events_evlist_error
    cc1: all warnings being treated as errors
    /git/perf-5.15.0/tools/build/Makefile.build:139: recipe for target 'util' failed
    make[5]: *** [util] Error 2
    /git/perf-5.15.0/tools/build/Makefile.build:139: recipe for target 'powerpc' failed
    make[4]: *** [powerpc] Error 2
    /git/perf-5.15.0/tools/build/Makefile.build:139: recipe for target 'arch' failed
    make[3]: *** [arch] Error 2
  71     7.90 ubuntu:18.04-x-powerpc64      : FAIL gcc version 7.5.0 (Ubuntu 7.5.0-3ubuntu1~18.04)
    arch/powerpc/util/kvm-stat.c: In function 'is_tracepoint_available':
    arch/powerpc/util/kvm-stat.c:119:3: error: implicit declaration of function 'parse_events_print_error'; did you mean 'parse_events_evlist_error'? [-Werror=implicit-function-declaration]
       parse_events_print_error(&err, "tracepoint");
       ^~~~~~~~~~~~~~~~~~~~~~~~
       parse_events_evlist_error
    cc1: all warnings being treated as errors
    /git/perf-5.15.0/tools/build/Makefile.build:139: recipe for target 'util' failed
    make[5]: *** [util] Error 2
    /git/perf-5.15.0/tools/build/Makefile.build:139: recipe for target 'powerpc' failed
    make[4]: *** [powerpc] Error 2
    /git/perf-5.15.0/tools/build/Makefile.build:139: recipe for target 'arch' failed
    make[3]: *** [arch] Error 2
  72     7.99 ubuntu:18.04-x-powerpc64el    : FAIL gcc version 7.5.0 (Ubuntu 7.5.0-3ubuntu1~18.04)
    arch/powerpc/util/kvm-stat.c: In function 'is_tracepoint_available':
    arch/powerpc/util/kvm-stat.c:119:3: error: implicit declaration of function 'parse_events_print_error'; did you mean 'parse_events_evlist_error'? [-Werror=implicit-function-declaration]
       parse_events_print_error(&err, "tracepoint");
       ^~~~~~~~~~~~~~~~~~~~~~~~
       parse_events_evlist_error
    cc1: all warnings being treated as errors
    /git/perf-5.15.0/tools/build/Makefile.build:139: recipe for target 'util' failed
    make[5]: *** [util] Error 2
    /git/perf-5.15.0/tools/build/Makefile.build:139: recipe for target 'powerpc' failed
    make[4]: *** [powerpc] Error 2
    /git/perf-5.15.0/tools/build/Makefile.build:139: recipe for target 'arch' failed
    make[3]: *** [arch] Error 2
  73    94.41 ubuntu:18.04-x-riscv64        : Ok   riscv64-linux-gnu-gcc (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0
  74    18.24 ubuntu:18.04-x-s390           : Ok   s390x-linux-gnu-gcc (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0
  75    19.55 ubuntu:18.04-x-sh4            : Ok   sh4-linux-gnu-gcc (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0
  76    18.36 ubuntu:18.04-x-sparc64        : Ok   sparc64-linux-gnu-gcc (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0
  77    67.70 ubuntu:20.04                  : FAIL clang version 10.0.0-4ubuntu1

  78     8.29 ubuntu:20.04-x-powerpc64el    : FAIL gcc version 10.3.0 (Ubuntu 10.3.0-1ubuntu1~20.04)
    arch/powerpc/util/kvm-stat.c: In function 'is_tracepoint_available':
    arch/powerpc/util/kvm-stat.c:119:3: error: implicit declaration of function 'parse_events_print_error'; did you mean 'parse_events_evlist_error'? [-Werror=implicit-function-declaration]
      119 |   parse_events_print_error(&err, "tracepoint");
          |   ^~~~~~~~~~~~~~~~~~~~~~~~
          |   parse_events_evlist_error
    cc1: all warnings being treated as errors
    make[5]: *** [/git/perf-5.15.0/tools/build/Makefile.build:139: util] Error 2
    make[4]: *** [/git/perf-5.15.0/tools/build/Makefile.build:139: powerpc] Error 2
    make[3]: *** [/git/perf-5.15.0/tools/build/Makefile.build:139: arch] Error 2