| Message ID | 20191024190202.109403-2-irogers@google.com |
|---|---|
| State | Not Applicable |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | Improvements to memory usage by parse events | expand |
On Thu, Oct 24, 2019 at 12:01:54PM -0700, Ian Rogers wrote: > Parse event error handling may overwrite one error string with another > creating memory leaks and masking errors. Introduce a helper routine > that appends error messages and avoids the memory leak. > > A reproduction of this problem can be seen with: > perf stat -e c/c/ > After this change this produces: > event syntax error: 'c/c/' > \___ unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: Cannot find PMU `c'. Missing kernel support?)(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,pc,in_tx,edge,any,offcore_rsp,in_tx_cp,ldlat,inv,umask,frontend,cmask,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore)) hum... I'd argue that the previous state was better: [jolsa@krava perf]$ ./perf stat -e c/c/ event syntax error: 'c/c/' \___ unknown term jirka > > valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore > Run 'perf list' for a list of valid events > > Usage: perf stat [<options>] [<command>] > > -e, --event <event> event selector. use 'perf list' to list available events > > Signed-off-by: Ian Rogers <irogers@google.com> > --- > tools/perf/util/parse-events.c | 100 +++++++++++++++++++++++---------- > tools/perf/util/parse-events.h | 2 + > tools/perf/util/pmu.c | 30 ++++++---- > 3 files changed, 89 insertions(+), 43 deletions(-) > > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c > index db882f630f7e..edb3ae76777d 100644 > --- a/tools/perf/util/parse-events.c > +++ b/tools/perf/util/parse-events.c > @@ -182,6 +182,38 @@ static int tp_event_has_id(const char *dir_path, struct dirent *evt_dir) > > #define MAX_EVENT_LENGTH 512 > > +void parse_events__append_error(struct parse_events_error *err, int idx, > + char *str, char *help) > +{ > + char *new_str = NULL; > + > + if (WARN(!str, "WARNING: failed to provide error string")) { > + free(help); > + return; > + } > + if (err->str) { > + int ret; > + > + if (err->help) { > + ret = asprintf(&new_str, > + "%s (previous error: %s(help: %s))", > + str, err->str, err->help); > + } else { > + ret = asprintf(&new_str, > + "%s (previous error: %s)", > + str, err->str); > + } > + if (ret < 0) > + new_str = NULL; > + else > + zfree(&str); > + } > + err->idx = idx; > + free(err->str); > + err->str = new_str ?: str; > + free(err->help); > + err->help = help; > +} > > struct tracepoint_path *tracepoint_id_to_path(u64 config) > { > @@ -932,11 +964,11 @@ static int check_type_val(struct parse_events_term *term, > return 0; > > if (err) { > - err->idx = term->err_val; > - if (type == PARSE_EVENTS__TERM_TYPE_NUM) > - err->str = strdup("expected numeric value"); > - else > - err->str = strdup("expected string value"); > + parse_events__append_error(err, term->err_val, > + type == PARSE_EVENTS__TERM_TYPE_NUM > + ? strdup("expected numeric value") > + : strdup("expected string value"), > + NULL); > } > return -EINVAL; > } > @@ -972,8 +1004,11 @@ static bool config_term_shrinked; > static bool > 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) { > - err->str = strdup("Invalid term_type"); > + parse_events__append_error(err, -1, > + strdup("Invalid term_type"), NULL); > return false; > } > if (!config_term_shrinked) > @@ -992,9 +1027,9 @@ config_term_avail(int term_type, struct parse_events_error *err) > return false; > > /* 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) > - err->str = NULL; > + if (asprintf(&err_str, "'%s' is not usable in 'perf stat'", > + config_term_names[term_type]) >= 0) > + parse_events__append_error(err, -1, err_str, NULL); > return false; > } > } > @@ -1036,17 +1071,20 @@ do { \ > case PARSE_EVENTS__TERM_TYPE_BRANCH_SAMPLE_TYPE: > CHECK_TYPE_VAL(STR); > if (strcmp(term->val.str, "no") && > - parse_branch_str(term->val.str, &attr->branch_sample_type)) { > - err->str = strdup("invalid branch sample type"); > - err->idx = term->err_val; > + parse_branch_str(term->val.str, > + &attr->branch_sample_type)) { > + parse_events__append_error(err, term->err_val, > + strdup("invalid branch sample type"), > + NULL); > return -EINVAL; > } > break; > case PARSE_EVENTS__TERM_TYPE_TIME: > CHECK_TYPE_VAL(NUM); > if (term->val.num > 1) { > - err->str = strdup("expected 0 or 1"); > - err->idx = term->err_val; > + parse_events__append_error(err, term->err_val, > + strdup("expected 0 or 1"), > + NULL); > return -EINVAL; > } > break; > @@ -1080,8 +1118,9 @@ do { \ > case PARSE_EVENTS__TERM_TYPE_PERCORE: > CHECK_TYPE_VAL(NUM); > if ((unsigned int)term->val.num > 1) { > - err->str = strdup("expected 0 or 1"); > - err->idx = term->err_val; > + parse_events__append_error(err, term->err_val, > + strdup("expected 0 or 1"), > + NULL); > return -EINVAL; > } > break; > @@ -1089,9 +1128,9 @@ do { \ > CHECK_TYPE_VAL(NUM); > break; > default: > - err->str = strdup("unknown term"); > - err->idx = term->err_term; > - err->help = parse_events_formats_error_string(NULL); > + parse_events__append_error(err, term->err_term, > + strdup("unknown term"), > + parse_events_formats_error_string(NULL)); > return -EINVAL; > } > > @@ -1142,9 +1181,9 @@ static int config_term_tracepoint(struct perf_event_attr *attr, > return config_term_common(attr, term, err); > default: > if (err) { > - err->idx = term->err_term; > - err->str = strdup("unknown term"); > - err->help = strdup("valid terms: call-graph,stack-size\n"); > + parse_events__append_error(err, term->err_term, > + strdup("unknown term"), > + strdup("valid terms: call-graph,stack-size\n")); > } > return -EINVAL; > } > @@ -1323,10 +1362,12 @@ int parse_events_add_pmu(struct parse_events_state *parse_state, > > pmu = perf_pmu__find(name); > if (!pmu) { > - if (asprintf(&err->str, > + char *err_str; > + > + if (asprintf(&err_str, > "Cannot find PMU `%s'. Missing kernel support?", > - name) < 0) > - err->str = NULL; > + name) >= 0) > + parse_events__append_error(err, -1, err_str, NULL); > return -EINVAL; > } > > @@ -2797,13 +2838,10 @@ void parse_events__clear_array(struct parse_events_array *a) > void parse_events_evlist_error(struct parse_events_state *parse_state, > int idx, const char *str) > { > - struct parse_events_error *err = parse_state->error; > - > - if (!err) > + if (!parse_state->error) > return; > - err->idx = idx; > - err->str = strdup(str); > - WARN_ONCE(!err->str, "WARNING: failed to allocate error string"); > + > + parse_events__append_error(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 769e07cddaa2..a7d42faeab0c 100644 > --- a/tools/perf/util/parse-events.h > +++ b/tools/perf/util/parse-events.h > @@ -124,6 +124,8 @@ struct parse_events_state { > struct list_head *terms; > }; > > +void parse_events__append_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, > diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c > index adbe97e941dd..4015ec11944a 100644 > --- a/tools/perf/util/pmu.c > +++ b/tools/perf/util/pmu.c > @@ -1050,9 +1050,9 @@ static int pmu_config_term(struct list_head *formats, > if (err) { > char *pmu_term = pmu_formats_string(formats); > > - err->idx = term->err_term; > - err->str = strdup("unknown term"); > - err->help = parse_events_formats_error_string(pmu_term); > + parse_events__append_error(err, term->err_term, > + strdup("unknown term"), > + parse_events_formats_error_string(pmu_term)); > free(pmu_term); > } > return -EINVAL; > @@ -1080,8 +1080,9 @@ static int pmu_config_term(struct list_head *formats, > if (term->no_value && > bitmap_weight(format->bits, PERF_PMU_FORMAT_BITS) > 1) { > if (err) { > - err->idx = term->err_val; > - err->str = strdup("no value assigned for term"); > + parse_events__append_error(err, term->err_val, > + strdup("no value assigned for term"), > + NULL); > } > return -EINVAL; > } > @@ -1094,8 +1095,9 @@ static int pmu_config_term(struct list_head *formats, > term->config, term->val.str); > } > if (err) { > - err->idx = term->err_val; > - err->str = strdup("expected numeric value"); > + parse_events__append_error(err, term->err_val, > + strdup("expected numeric value"), > + NULL); > } > return -EINVAL; > } > @@ -1108,11 +1110,15 @@ static int pmu_config_term(struct list_head *formats, > max_val = pmu_format_max_value(format->bits); > if (val > max_val) { > if (err) { > - err->idx = term->err_val; > - if (asprintf(&err->str, > - "value too big for format, maximum is %llu", > - (unsigned long long)max_val) < 0) > - err->str = strdup("value too big for format"); > + char *err_str; > + > + parse_events__append_error(err, term->err_val, > + asprintf(&err_str, > + "value too big for format, maximum is %llu", > + (unsigned long long)max_val) < 0 > + ? strdup("value too big for format") > + : err_str, > + NULL); > return -EINVAL; > } > /* > -- > 2.23.0.866.gb869b98d4c-goog >
On Fri, Oct 25, 2019 at 12:58 AM Jiri Olsa <jolsa@redhat.com> wrote: > > On Thu, Oct 24, 2019 at 12:01:54PM -0700, Ian Rogers wrote: > > Parse event error handling may overwrite one error string with another > > creating memory leaks and masking errors. Introduce a helper routine > > that appends error messages and avoids the memory leak. > > > > A reproduction of this problem can be seen with: > > perf stat -e c/c/ > > After this change this produces: > > event syntax error: 'c/c/' > > \___ unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: Cannot find PMU `c'. Missing kernel support?)(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,pc,in_tx,edge,any,offcore_rsp,in_tx_cp,ldlat,inv,umask,frontend,cmask,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore)) > > > hum... I'd argue that the previous state was better: > > [jolsa@krava perf]$ ./perf stat -e c/c/ > event syntax error: 'c/c/' > \___ unknown term > > > jirka I am agnostic. We can either have the previous state or the new state, I'm keen to resolve the memory leak. Another alternative is to warn that multiple errors have occurred before dropping or printing the previous error. As the code is shared in memory places the approach taken here was to try to not conceal anything that could potentially be useful. Given this, is the preference to keep the status quo without any warning? Thanks, Ian > > > > valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore > > Run 'perf list' for a list of valid events > > > > Usage: perf stat [<options>] [<command>] > > > > -e, --event <event> event selector. use 'perf list' to list available events > > > > Signed-off-by: Ian Rogers <irogers@google.com> > > --- > > tools/perf/util/parse-events.c | 100 +++++++++++++++++++++++---------- > > tools/perf/util/parse-events.h | 2 + > > tools/perf/util/pmu.c | 30 ++++++---- > > 3 files changed, 89 insertions(+), 43 deletions(-) > > > > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c > > index db882f630f7e..edb3ae76777d 100644 > > --- a/tools/perf/util/parse-events.c > > +++ b/tools/perf/util/parse-events.c > > @@ -182,6 +182,38 @@ static int tp_event_has_id(const char *dir_path, struct dirent *evt_dir) > > > > #define MAX_EVENT_LENGTH 512 > > > > +void parse_events__append_error(struct parse_events_error *err, int idx, > > + char *str, char *help) > > +{ > > + char *new_str = NULL; > > + > > + if (WARN(!str, "WARNING: failed to provide error string")) { > > + free(help); > > + return; > > + } > > + if (err->str) { > > + int ret; > > + > > + if (err->help) { > > + ret = asprintf(&new_str, > > + "%s (previous error: %s(help: %s))", > > + str, err->str, err->help); > > + } else { > > + ret = asprintf(&new_str, > > + "%s (previous error: %s)", > > + str, err->str); > > + } > > + if (ret < 0) > > + new_str = NULL; > > + else > > + zfree(&str); > > + } > > + err->idx = idx; > > + free(err->str); > > + err->str = new_str ?: str; > > + free(err->help); > > + err->help = help; > > +} > > > > struct tracepoint_path *tracepoint_id_to_path(u64 config) > > { > > @@ -932,11 +964,11 @@ static int check_type_val(struct parse_events_term *term, > > return 0; > > > > if (err) { > > - err->idx = term->err_val; > > - if (type == PARSE_EVENTS__TERM_TYPE_NUM) > > - err->str = strdup("expected numeric value"); > > - else > > - err->str = strdup("expected string value"); > > + parse_events__append_error(err, term->err_val, > > + type == PARSE_EVENTS__TERM_TYPE_NUM > > + ? strdup("expected numeric value") > > + : strdup("expected string value"), > > + NULL); > > } > > return -EINVAL; > > } > > @@ -972,8 +1004,11 @@ static bool config_term_shrinked; > > static bool > > 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) { > > - err->str = strdup("Invalid term_type"); > > + parse_events__append_error(err, -1, > > + strdup("Invalid term_type"), NULL); > > return false; > > } > > if (!config_term_shrinked) > > @@ -992,9 +1027,9 @@ config_term_avail(int term_type, struct parse_events_error *err) > > return false; > > > > /* 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) > > - err->str = NULL; > > + if (asprintf(&err_str, "'%s' is not usable in 'perf stat'", > > + config_term_names[term_type]) >= 0) > > + parse_events__append_error(err, -1, err_str, NULL); > > return false; > > } > > } > > @@ -1036,17 +1071,20 @@ do { \ > > case PARSE_EVENTS__TERM_TYPE_BRANCH_SAMPLE_TYPE: > > CHECK_TYPE_VAL(STR); > > if (strcmp(term->val.str, "no") && > > - parse_branch_str(term->val.str, &attr->branch_sample_type)) { > > - err->str = strdup("invalid branch sample type"); > > - err->idx = term->err_val; > > + parse_branch_str(term->val.str, > > + &attr->branch_sample_type)) { > > + parse_events__append_error(err, term->err_val, > > + strdup("invalid branch sample type"), > > + NULL); > > return -EINVAL; > > } > > break; > > case PARSE_EVENTS__TERM_TYPE_TIME: > > CHECK_TYPE_VAL(NUM); > > if (term->val.num > 1) { > > - err->str = strdup("expected 0 or 1"); > > - err->idx = term->err_val; > > + parse_events__append_error(err, term->err_val, > > + strdup("expected 0 or 1"), > > + NULL); > > return -EINVAL; > > } > > break; > > @@ -1080,8 +1118,9 @@ do { \ > > case PARSE_EVENTS__TERM_TYPE_PERCORE: > > CHECK_TYPE_VAL(NUM); > > if ((unsigned int)term->val.num > 1) { > > - err->str = strdup("expected 0 or 1"); > > - err->idx = term->err_val; > > + parse_events__append_error(err, term->err_val, > > + strdup("expected 0 or 1"), > > + NULL); > > return -EINVAL; > > } > > break; > > @@ -1089,9 +1128,9 @@ do { \ > > CHECK_TYPE_VAL(NUM); > > break; > > default: > > - err->str = strdup("unknown term"); > > - err->idx = term->err_term; > > - err->help = parse_events_formats_error_string(NULL); > > + parse_events__append_error(err, term->err_term, > > + strdup("unknown term"), > > + parse_events_formats_error_string(NULL)); > > return -EINVAL; > > } > > > > @@ -1142,9 +1181,9 @@ static int config_term_tracepoint(struct perf_event_attr *attr, > > return config_term_common(attr, term, err); > > default: > > if (err) { > > - err->idx = term->err_term; > > - err->str = strdup("unknown term"); > > - err->help = strdup("valid terms: call-graph,stack-size\n"); > > + parse_events__append_error(err, term->err_term, > > + strdup("unknown term"), > > + strdup("valid terms: call-graph,stack-size\n")); > > } > > return -EINVAL; > > } > > @@ -1323,10 +1362,12 @@ int parse_events_add_pmu(struct parse_events_state *parse_state, > > > > pmu = perf_pmu__find(name); > > if (!pmu) { > > - if (asprintf(&err->str, > > + char *err_str; > > + > > + if (asprintf(&err_str, > > "Cannot find PMU `%s'. Missing kernel support?", > > - name) < 0) > > - err->str = NULL; > > + name) >= 0) > > + parse_events__append_error(err, -1, err_str, NULL); > > return -EINVAL; > > } > > > > @@ -2797,13 +2838,10 @@ void parse_events__clear_array(struct parse_events_array *a) > > void parse_events_evlist_error(struct parse_events_state *parse_state, > > int idx, const char *str) > > { > > - struct parse_events_error *err = parse_state->error; > > - > > - if (!err) > > + if (!parse_state->error) > > return; > > - err->idx = idx; > > - err->str = strdup(str); > > - WARN_ONCE(!err->str, "WARNING: failed to allocate error string"); > > + > > + parse_events__append_error(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 769e07cddaa2..a7d42faeab0c 100644 > > --- a/tools/perf/util/parse-events.h > > +++ b/tools/perf/util/parse-events.h > > @@ -124,6 +124,8 @@ struct parse_events_state { > > struct list_head *terms; > > }; > > > > +void parse_events__append_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, > > diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c > > index adbe97e941dd..4015ec11944a 100644 > > --- a/tools/perf/util/pmu.c > > +++ b/tools/perf/util/pmu.c > > @@ -1050,9 +1050,9 @@ static int pmu_config_term(struct list_head *formats, > > if (err) { > > char *pmu_term = pmu_formats_string(formats); > > > > - err->idx = term->err_term; > > - err->str = strdup("unknown term"); > > - err->help = parse_events_formats_error_string(pmu_term); > > + parse_events__append_error(err, term->err_term, > > + strdup("unknown term"), > > + parse_events_formats_error_string(pmu_term)); > > free(pmu_term); > > } > > return -EINVAL; > > @@ -1080,8 +1080,9 @@ static int pmu_config_term(struct list_head *formats, > > if (term->no_value && > > bitmap_weight(format->bits, PERF_PMU_FORMAT_BITS) > 1) { > > if (err) { > > - err->idx = term->err_val; > > - err->str = strdup("no value assigned for term"); > > + parse_events__append_error(err, term->err_val, > > + strdup("no value assigned for term"), > > + NULL); > > } > > return -EINVAL; > > } > > @@ -1094,8 +1095,9 @@ static int pmu_config_term(struct list_head *formats, > > term->config, term->val.str); > > } > > if (err) { > > - err->idx = term->err_val; > > - err->str = strdup("expected numeric value"); > > + parse_events__append_error(err, term->err_val, > > + strdup("expected numeric value"), > > + NULL); > > } > > return -EINVAL; > > } > > @@ -1108,11 +1110,15 @@ static int pmu_config_term(struct list_head *formats, > > max_val = pmu_format_max_value(format->bits); > > if (val > max_val) { > > if (err) { > > - err->idx = term->err_val; > > - if (asprintf(&err->str, > > - "value too big for format, maximum is %llu", > > - (unsigned long long)max_val) < 0) > > - err->str = strdup("value too big for format"); > > + char *err_str; > > + > > + parse_events__append_error(err, term->err_val, > > + asprintf(&err_str, > > + "value too big for format, maximum is %llu", > > + (unsigned long long)max_val) < 0 > > + ? strdup("value too big for format") > > + : err_str, > > + NULL); > > return -EINVAL; > > } > > /* > > -- > > 2.23.0.866.gb869b98d4c-goog > > >
On Fri, Oct 25, 2019 at 08:14:36AM -0700, Ian Rogers wrote: > On Fri, Oct 25, 2019 at 12:58 AM Jiri Olsa <jolsa@redhat.com> wrote: > > > > On Thu, Oct 24, 2019 at 12:01:54PM -0700, Ian Rogers wrote: > > > Parse event error handling may overwrite one error string with another > > > creating memory leaks and masking errors. Introduce a helper routine > > > that appends error messages and avoids the memory leak. > > > > > > A reproduction of this problem can be seen with: > > > perf stat -e c/c/ > > > After this change this produces: > > > event syntax error: 'c/c/' > > > \___ unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: Cannot find PMU `c'. Missing kernel support?)(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,pc,in_tx,edge,any,offcore_rsp,in_tx_cp,ldlat,inv,umask,frontend,cmask,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore)) > > > > > > hum... I'd argue that the previous state was better: > > > > [jolsa@krava perf]$ ./perf stat -e c/c/ > > event syntax error: 'c/c/' > > \___ unknown term > > > > > > jirka > > I am agnostic. We can either have the previous state or the new state, > I'm keen to resolve the memory leak. Another alternative is to warn > that multiple errors have occurred before dropping or printing the > previous error. As the code is shared in memory places the approach > taken here was to try to not conceal anything that could potentially > be useful. Given this, is the preference to keep the status quo > without any warning? if the other alternative is string above, yes.. but perhaps keeping just the first error would be the best way? here it seems to be the: "Cannot find PMU `c'. Missing kernel support?)(help: valid..." jirka
On Mon, Oct 28, 2019 at 12:32 PM Jiri Olsa <jolsa@redhat.com> wrote: > > On Fri, Oct 25, 2019 at 08:14:36AM -0700, Ian Rogers wrote: > > On Fri, Oct 25, 2019 at 12:58 AM Jiri Olsa <jolsa@redhat.com> wrote: > > > > > > On Thu, Oct 24, 2019 at 12:01:54PM -0700, Ian Rogers wrote: > > > > Parse event error handling may overwrite one error string with another > > > > creating memory leaks and masking errors. Introduce a helper routine > > > > that appends error messages and avoids the memory leak. > > > > > > > > A reproduction of this problem can be seen with: > > > > perf stat -e c/c/ > > > > After this change this produces: > > > > event syntax error: 'c/c/' > > > > \___ unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: Cannot find PMU `c'. Missing kernel support?)(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,pc,in_tx,edge,any,offcore_rsp,in_tx_cp,ldlat,inv,umask,frontend,cmask,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore)) > > > > > > > > > hum... I'd argue that the previous state was better: > > > > > > [jolsa@krava perf]$ ./perf stat -e c/c/ > > > event syntax error: 'c/c/' > > > \___ unknown term > > > > > > > > > jirka > > > > I am agnostic. We can either have the previous state or the new state, > > I'm keen to resolve the memory leak. Another alternative is to warn > > that multiple errors have occurred before dropping or printing the > > previous error. As the code is shared in memory places the approach > > taken here was to try to not conceal anything that could potentially > > be useful. Given this, is the preference to keep the status quo > > without any warning? > > if the other alternative is string above, yes.. but perhaps > keeping just the first error would be the best way? > > here it seems to be the: > "Cannot find PMU `c'. Missing kernel support?)(help: valid..." I think this is a reasonable idea. I'd propose doing it as an additional patch, the purpose of this patch is to avoid a possible memory leak. I can write the patch and base it on this series. To resolve the issue, I'd add an extra first error to the struct parse_events_error. All callers would need to be responsible for cleaning this up when present, which is why I'd rather not make it part of this patch. Does this sound reasonable? Thanks, Ian > jirka >
On Mon, Oct 28, 2019 at 02:06:24PM -0700, Ian Rogers wrote: > On Mon, Oct 28, 2019 at 12:32 PM Jiri Olsa <jolsa@redhat.com> wrote: > > > > On Fri, Oct 25, 2019 at 08:14:36AM -0700, Ian Rogers wrote: > > > On Fri, Oct 25, 2019 at 12:58 AM Jiri Olsa <jolsa@redhat.com> wrote: > > > > > > > > On Thu, Oct 24, 2019 at 12:01:54PM -0700, Ian Rogers wrote: > > > > > Parse event error handling may overwrite one error string with another > > > > > creating memory leaks and masking errors. Introduce a helper routine > > > > > that appends error messages and avoids the memory leak. > > > > > > > > > > A reproduction of this problem can be seen with: > > > > > perf stat -e c/c/ > > > > > After this change this produces: > > > > > event syntax error: 'c/c/' > > > > > \___ unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: Cannot find PMU `c'. Missing kernel support?)(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,pc,in_tx,edge,any,offcore_rsp,in_tx_cp,ldlat,inv,umask,frontend,cmask,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore)) > > > > > > > > > > > > hum... I'd argue that the previous state was better: > > > > > > > > [jolsa@krava perf]$ ./perf stat -e c/c/ > > > > event syntax error: 'c/c/' > > > > \___ unknown term > > > > > > > > > > > > jirka > > > > > > I am agnostic. We can either have the previous state or the new state, > > > I'm keen to resolve the memory leak. Another alternative is to warn > > > that multiple errors have occurred before dropping or printing the > > > previous error. As the code is shared in memory places the approach > > > taken here was to try to not conceal anything that could potentially > > > be useful. Given this, is the preference to keep the status quo > > > without any warning? > > > > if the other alternative is string above, yes.. but perhaps > > keeping just the first error would be the best way? > > > > here it seems to be the: > > "Cannot find PMU `c'. Missing kernel support?)(help: valid..." > > I think this is a reasonable idea. I'd propose doing it as an > additional patch, the purpose of this patch is to avoid a possible > memory leak. I can write the patch and base it on this series. > To resolve the issue, I'd add an extra first error to the struct > parse_events_error. All callers would need to be responsible for > cleaning this up when present, which is why I'd rather not make it > part of this patch. > Does this sound reasonable? yep, sounds good jirka
Thanks! This is part of the v5 patch set, specifically: https://lkml.org/lkml/2019/10/30/1001 Let me know if there's anything else blocking this. Thanks, Ian On Mon, Oct 28, 2019 at 2:36 PM Jiri Olsa <jolsa@redhat.com> wrote: > > On Mon, Oct 28, 2019 at 02:06:24PM -0700, Ian Rogers wrote: > > On Mon, Oct 28, 2019 at 12:32 PM Jiri Olsa <jolsa@redhat.com> wrote: > > > > > > On Fri, Oct 25, 2019 at 08:14:36AM -0700, Ian Rogers wrote: > > > > On Fri, Oct 25, 2019 at 12:58 AM Jiri Olsa <jolsa@redhat.com> wrote: > > > > > > > > > > On Thu, Oct 24, 2019 at 12:01:54PM -0700, Ian Rogers wrote: > > > > > > Parse event error handling may overwrite one error string with another > > > > > > creating memory leaks and masking errors. Introduce a helper routine > > > > > > that appends error messages and avoids the memory leak. > > > > > > > > > > > > A reproduction of this problem can be seen with: > > > > > > perf stat -e c/c/ > > > > > > After this change this produces: > > > > > > event syntax error: 'c/c/' > > > > > > \___ unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: Cannot find PMU `c'. Missing kernel support?)(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,pc,in_tx,edge,any,offcore_rsp,in_tx_cp,ldlat,inv,umask,frontend,cmask,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore)) > > > > > > > > > > > > > > > hum... I'd argue that the previous state was better: > > > > > > > > > > [jolsa@krava perf]$ ./perf stat -e c/c/ > > > > > event syntax error: 'c/c/' > > > > > \___ unknown term > > > > > > > > > > > > > > > jirka > > > > > > > > I am agnostic. We can either have the previous state or the new state, > > > > I'm keen to resolve the memory leak. Another alternative is to warn > > > > that multiple errors have occurred before dropping or printing the > > > > previous error. As the code is shared in memory places the approach > > > > taken here was to try to not conceal anything that could potentially > > > > be useful. Given this, is the preference to keep the status quo > > > > without any warning? > > > > > > if the other alternative is string above, yes.. but perhaps > > > keeping just the first error would be the best way? > > > > > > here it seems to be the: > > > "Cannot find PMU `c'. Missing kernel support?)(help: valid..." > > > > I think this is a reasonable idea. I'd propose doing it as an > > additional patch, the purpose of this patch is to avoid a possible > > memory leak. I can write the patch and base it on this series. > > To resolve the issue, I'd add an extra first error to the struct > > parse_events_error. All callers would need to be responsible for > > cleaning this up when present, which is why I'd rather not make it > > part of this patch. > > Does this sound reasonable? > > yep, sounds good > > jirka >
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c index db882f630f7e..edb3ae76777d 100644 --- a/tools/perf/util/parse-events.c +++ b/tools/perf/util/parse-events.c @@ -182,6 +182,38 @@ static int tp_event_has_id(const char *dir_path, struct dirent *evt_dir) #define MAX_EVENT_LENGTH 512 +void parse_events__append_error(struct parse_events_error *err, int idx, + char *str, char *help) +{ + char *new_str = NULL; + + if (WARN(!str, "WARNING: failed to provide error string")) { + free(help); + return; + } + if (err->str) { + int ret; + + if (err->help) { + ret = asprintf(&new_str, + "%s (previous error: %s(help: %s))", + str, err->str, err->help); + } else { + ret = asprintf(&new_str, + "%s (previous error: %s)", + str, err->str); + } + if (ret < 0) + new_str = NULL; + else + zfree(&str); + } + err->idx = idx; + free(err->str); + err->str = new_str ?: str; + free(err->help); + err->help = help; +} struct tracepoint_path *tracepoint_id_to_path(u64 config) { @@ -932,11 +964,11 @@ static int check_type_val(struct parse_events_term *term, return 0; if (err) { - err->idx = term->err_val; - if (type == PARSE_EVENTS__TERM_TYPE_NUM) - err->str = strdup("expected numeric value"); - else - err->str = strdup("expected string value"); + parse_events__append_error(err, term->err_val, + type == PARSE_EVENTS__TERM_TYPE_NUM + ? strdup("expected numeric value") + : strdup("expected string value"), + NULL); } return -EINVAL; } @@ -972,8 +1004,11 @@ static bool config_term_shrinked; static bool 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) { - err->str = strdup("Invalid term_type"); + parse_events__append_error(err, -1, + strdup("Invalid term_type"), NULL); return false; } if (!config_term_shrinked) @@ -992,9 +1027,9 @@ config_term_avail(int term_type, struct parse_events_error *err) return false; /* 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) - err->str = NULL; + if (asprintf(&err_str, "'%s' is not usable in 'perf stat'", + config_term_names[term_type]) >= 0) + parse_events__append_error(err, -1, err_str, NULL); return false; } } @@ -1036,17 +1071,20 @@ do { \ case PARSE_EVENTS__TERM_TYPE_BRANCH_SAMPLE_TYPE: CHECK_TYPE_VAL(STR); if (strcmp(term->val.str, "no") && - parse_branch_str(term->val.str, &attr->branch_sample_type)) { - err->str = strdup("invalid branch sample type"); - err->idx = term->err_val; + parse_branch_str(term->val.str, + &attr->branch_sample_type)) { + parse_events__append_error(err, term->err_val, + strdup("invalid branch sample type"), + NULL); return -EINVAL; } break; case PARSE_EVENTS__TERM_TYPE_TIME: CHECK_TYPE_VAL(NUM); if (term->val.num > 1) { - err->str = strdup("expected 0 or 1"); - err->idx = term->err_val; + parse_events__append_error(err, term->err_val, + strdup("expected 0 or 1"), + NULL); return -EINVAL; } break; @@ -1080,8 +1118,9 @@ do { \ case PARSE_EVENTS__TERM_TYPE_PERCORE: CHECK_TYPE_VAL(NUM); if ((unsigned int)term->val.num > 1) { - err->str = strdup("expected 0 or 1"); - err->idx = term->err_val; + parse_events__append_error(err, term->err_val, + strdup("expected 0 or 1"), + NULL); return -EINVAL; } break; @@ -1089,9 +1128,9 @@ do { \ CHECK_TYPE_VAL(NUM); break; default: - err->str = strdup("unknown term"); - err->idx = term->err_term; - err->help = parse_events_formats_error_string(NULL); + parse_events__append_error(err, term->err_term, + strdup("unknown term"), + parse_events_formats_error_string(NULL)); return -EINVAL; } @@ -1142,9 +1181,9 @@ static int config_term_tracepoint(struct perf_event_attr *attr, return config_term_common(attr, term, err); default: if (err) { - err->idx = term->err_term; - err->str = strdup("unknown term"); - err->help = strdup("valid terms: call-graph,stack-size\n"); + parse_events__append_error(err, term->err_term, + strdup("unknown term"), + strdup("valid terms: call-graph,stack-size\n")); } return -EINVAL; } @@ -1323,10 +1362,12 @@ int parse_events_add_pmu(struct parse_events_state *parse_state, pmu = perf_pmu__find(name); if (!pmu) { - if (asprintf(&err->str, + char *err_str; + + if (asprintf(&err_str, "Cannot find PMU `%s'. Missing kernel support?", - name) < 0) - err->str = NULL; + name) >= 0) + parse_events__append_error(err, -1, err_str, NULL); return -EINVAL; } @@ -2797,13 +2838,10 @@ void parse_events__clear_array(struct parse_events_array *a) void parse_events_evlist_error(struct parse_events_state *parse_state, int idx, const char *str) { - struct parse_events_error *err = parse_state->error; - - if (!err) + if (!parse_state->error) return; - err->idx = idx; - err->str = strdup(str); - WARN_ONCE(!err->str, "WARNING: failed to allocate error string"); + + parse_events__append_error(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 769e07cddaa2..a7d42faeab0c 100644 --- a/tools/perf/util/parse-events.h +++ b/tools/perf/util/parse-events.h @@ -124,6 +124,8 @@ struct parse_events_state { struct list_head *terms; }; +void parse_events__append_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, diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c index adbe97e941dd..4015ec11944a 100644 --- a/tools/perf/util/pmu.c +++ b/tools/perf/util/pmu.c @@ -1050,9 +1050,9 @@ static int pmu_config_term(struct list_head *formats, if (err) { char *pmu_term = pmu_formats_string(formats); - err->idx = term->err_term; - err->str = strdup("unknown term"); - err->help = parse_events_formats_error_string(pmu_term); + parse_events__append_error(err, term->err_term, + strdup("unknown term"), + parse_events_formats_error_string(pmu_term)); free(pmu_term); } return -EINVAL; @@ -1080,8 +1080,9 @@ static int pmu_config_term(struct list_head *formats, if (term->no_value && bitmap_weight(format->bits, PERF_PMU_FORMAT_BITS) > 1) { if (err) { - err->idx = term->err_val; - err->str = strdup("no value assigned for term"); + parse_events__append_error(err, term->err_val, + strdup("no value assigned for term"), + NULL); } return -EINVAL; } @@ -1094,8 +1095,9 @@ static int pmu_config_term(struct list_head *formats, term->config, term->val.str); } if (err) { - err->idx = term->err_val; - err->str = strdup("expected numeric value"); + parse_events__append_error(err, term->err_val, + strdup("expected numeric value"), + NULL); } return -EINVAL; } @@ -1108,11 +1110,15 @@ static int pmu_config_term(struct list_head *formats, max_val = pmu_format_max_value(format->bits); if (val > max_val) { if (err) { - err->idx = term->err_val; - if (asprintf(&err->str, - "value too big for format, maximum is %llu", - (unsigned long long)max_val) < 0) - err->str = strdup("value too big for format"); + char *err_str; + + parse_events__append_error(err, term->err_val, + asprintf(&err_str, + "value too big for format, maximum is %llu", + (unsigned long long)max_val) < 0 + ? strdup("value too big for format") + : err_str, + NULL); return -EINVAL; } /*
Parse event error handling may overwrite one error string with another creating memory leaks and masking errors. Introduce a helper routine that appends error messages and avoids the memory leak. A reproduction of this problem can be seen with: perf stat -e c/c/ After this change this produces: event syntax error: 'c/c/' \___ unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: Cannot find PMU `c'. Missing kernel support?)(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,pc,in_tx,edge,any,offcore_rsp,in_tx_cp,ldlat,inv,umask,frontend,cmask,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore)) valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore Run 'perf list' for a list of valid events Usage: perf stat [<options>] [<command>] -e, --event <event> event selector. use 'perf list' to list available events Signed-off-by: Ian Rogers <irogers@google.com> --- tools/perf/util/parse-events.c | 100 +++++++++++++++++++++++---------- tools/perf/util/parse-events.h | 2 + tools/perf/util/pmu.c | 30 ++++++---- 3 files changed, 89 insertions(+), 43 deletions(-)