Re: [PATCH v3 1/9] perf tools: add parse events append error

[Ian Rogers <irogers@google.com>]

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
> >
>