* [PATCH v2 1/3] perf parse-events: Rename parse_events_error functions
@ 2021-11-07 9:00 Ian Rogers
2021-11-07 9:00 ` [PATCH v2 2/3] perf parse-event: Add init and exit to parse_event_error Ian Rogers
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Ian Rogers @ 2021-11-07 9:00 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
John Garry, linux-perf-users, linux-kernel
Cc: eranian, 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 2/3] perf parse-event: Add init and exit to parse_event_error
2021-11-07 9:00 [PATCH v2 1/3] perf parse-events: Rename parse_events_error functions Ian Rogers
@ 2021-11-07 9:00 ` Ian Rogers
2021-11-07 15:39 ` Arnaldo Carvalho de Melo
2021-11-07 9:00 ` [PATCH v2 3/3] perf metric: Fix memory leaks Ian Rogers
2021-11-07 15:34 ` [PATCH v2 1/3] perf parse-events: Rename parse_events_error functions Arnaldo Carvalho de Melo
2 siblings, 1 reply; 7+ messages in thread
From: Ian Rogers @ 2021-11-07 9:00 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
John Garry, linux-perf-users, linux-kernel
Cc: eranian, 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 3/3] perf metric: Fix memory leaks.
2021-11-07 9:00 [PATCH v2 1/3] perf parse-events: Rename parse_events_error functions Ian Rogers
2021-11-07 9:00 ` [PATCH v2 2/3] perf parse-event: Add init and exit to parse_event_error Ian Rogers
@ 2021-11-07 9:00 ` Ian Rogers
2021-11-07 15:41 ` Arnaldo Carvalho de Melo
2021-11-07 15:34 ` [PATCH v2 1/3] perf parse-events: Rename parse_events_error functions Arnaldo Carvalho de Melo
2 siblings, 1 reply; 7+ messages in thread
From: Ian Rogers @ 2021-11-07 9:00 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
John Garry, linux-perf-users, linux-kernel
Cc: eranian, 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/3] perf parse-events: Rename parse_events_error functions
2021-11-07 9:00 [PATCH v2 1/3] perf parse-events: Rename parse_events_error functions Ian Rogers
2021-11-07 9:00 ` [PATCH v2 2/3] perf parse-event: Add init and exit to parse_event_error Ian Rogers
2021-11-07 9:00 ` [PATCH v2 3/3] perf metric: Fix memory leaks Ian Rogers
@ 2021-11-07 15:34 ` Arnaldo Carvalho de Melo
2021-11-07 18:21 ` Arnaldo Carvalho de Melo
2 siblings, 1 reply; 7+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-11-07 15:34 UTC (permalink / raw)
To: Ian Rogers
Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
Jiri Olsa, Namhyung Kim, John Garry, linux-perf-users,
linux-kernel, eranian
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/3] perf parse-event: Add init and exit to parse_event_error
2021-11-07 9:00 ` [PATCH v2 2/3] perf parse-event: Add init and exit to parse_event_error Ian Rogers
@ 2021-11-07 15:39 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 7+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-11-07 15:39 UTC (permalink / raw)
To: Ian Rogers
Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
Jiri Olsa, Namhyung Kim, John Garry, linux-perf-users,
linux-kernel, eranian
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 3/3] perf metric: Fix memory leaks.
2021-11-07 9:00 ` [PATCH v2 3/3] perf metric: Fix memory leaks Ian Rogers
@ 2021-11-07 15:41 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 7+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-11-07 15:41 UTC (permalink / raw)
To: Ian Rogers
Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
Jiri Olsa, Namhyung Kim, John Garry, linux-perf-users,
linux-kernel, eranian
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/3] perf parse-events: Rename parse_events_error functions
2021-11-07 15:34 ` [PATCH v2 1/3] perf parse-events: Rename parse_events_error functions Arnaldo Carvalho de Melo
@ 2021-11-07 18:21 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 7+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-11-07 18:21 UTC (permalink / raw)
To: Ian Rogers
Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
Jiri Olsa, Namhyung Kim, John Garry, linux-perf-users,
linux-kernel, eranian
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-11-07 18:21 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-07 9:00 [PATCH v2 1/3] perf parse-events: Rename parse_events_error functions Ian Rogers
2021-11-07 9:00 ` [PATCH v2 2/3] perf parse-event: Add init and exit to parse_event_error Ian Rogers
2021-11-07 15:39 ` Arnaldo Carvalho de Melo
2021-11-07 9:00 ` [PATCH v2 3/3] perf metric: Fix memory leaks Ian Rogers
2021-11-07 15:41 ` Arnaldo Carvalho de Melo
2021-11-07 15:34 ` [PATCH v2 1/3] perf parse-events: Rename parse_events_error functions Arnaldo Carvalho de Melo
2021-11-07 18:21 ` Arnaldo Carvalho de Melo
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.