Re: [Question] perf tools: lex parsing issue

[John Garry]

On 25/08/2021 17:47, Ian Rogers wrote:
> On Wed, Aug 25, 2021 at 7:32 AM John Garry<john.garry@xxxxxxxxxx>  wrote:
> > On 25/08/2021 01:13, Ian Rogers wrote:
> >
> > Thanks for getting back to me
> >
> >
> > > > + if (term->type_term == PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD) {
> > > > +       term->config = strdup("period");
> > > > + }
> > > > +
> > > > if (!head) {
> > > >           parse_events_term__delete(term);
> > > >           YYABORT;
> > > > --
> > > >
> > > > ----8-----
> > > Agreed this is hacky,
> > This might be a*bit*  better:
> >
> > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> > index e5eae23cfceb..e597beaaa179 100644
> > --- a/tools/perf/util/parse-events.c
> > +++ b/tools/perf/util/parse-events.c
> > @@ -1058,6 +1058,13 @@ static const char
> > *config_term_names[__PARSE_EVENTS__TERM_TYPE_NR] = {
> >          [PARSE_EVENTS__TERM_TYPE_AUX_SAMPLE_SIZE]       = "aux-sample-size",
> >    };
> >
> > +const char * get_config_term(int term_type)
> > +{
> > +       if (term_type >= __PARSE_EVENTS__TERM_TYPE_NR)
> > +               return "";
> > +       return config_term_names[term_type];
> > +}
> > +
> >    static bool config_term_shrinked;
> >
> >    static bool
> > diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> > index bf6e41aa9b6a..0f658732535f 100644
> > --- a/tools/perf/util/parse-events.h
> > +++ b/tools/perf/util/parse-events.h
> > @@ -90,6 +90,8 @@ enum {
> >          __PARSE_EVENTS__TERM_TYPE_NR,
> >    };
> >
> > +const char *get_config_term(int term_type);
> > +
> >    struct parse_events_array {
> >          size_t nr_ranges;
> >          struct {
> > diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> > index 9321bd0e2f76..8d6d3fae226d 100644
> > --- a/tools/perf/util/parse-events.y
> > +++ b/tools/perf/util/parse-events.y
> > @@ -832,8 +832,9 @@ PE_TERM '=' PE_NAME
> >    PE_TERM '=' PE_VALUE
> >    {
> >          struct parse_events_term *term;
> > +       char *config = strdup(get_config_term($1));
> >
> > -       ABORT_ON(parse_events_term__num(&term, (int)$1, NULL, $3, false, &@1,
> > &@3));
> > +       ABORT_ON(parse_events_term__num(&term, (int)$1, config, $3, false,
> > &@1, &@3));
> >          $$ = term;
> >    }
> >
> >
> > > I think it'd be better to fix this up in the
> > > output. For example:
> > >
> > > diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> > > index 6cdbee8a12e7..c77c42275efa 100644
> > > --- a/tools/perf/util/pmu.c
> > > +++ b/tools/perf/util/pmu.c
> > > @@ -365,15 +365,21 @@ static int __perf_pmu__new_alias(struct
> > > list_head *list, char *dir, char *name,
> > >           memset(newval, 0, sizeof(newval));
> > >           ret = 0;
> > >           list_for_each_entry(term, &alias->terms, list) {
> > > +               const char * config = term->config;
> > > +
> > >                   if (ret)
> > >                           ret += scnprintf(newval + ret, sizeof(newval) - ret,
> > >                                            ",");
> > > +               if (!config) {
> > > +                       /* Note: config_term_names in parse_events.c
> > > isn't accessible */
> > > +                       config = config_term_names[term->type_term];
> > We could just expose some parse-events.c API, like above.
> >
> > > +               }
> > >                   if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM)
> > >                           ret += scnprintf(newval + ret, sizeof(newval) - ret,
> > > -                                        "%s=%#x", term->config, term->val.num);
> > > +                                        "%s=%#x", config, term->val.num);
> > >                   else if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR)
> > >                           ret += scnprintf(newval + ret, sizeof(newval) - ret,
> > > -                                        "%s=%s", term->config, term->val.str);
> > > +                                        "%s=%s", config, term->val.str);
> > >           }
> > >
> > >           alias->name = strdup(name);
> > But how about this alternative simple one:
> >
> > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> > index e5eae23cfceb..9e5df934d22d 100644
> > --- a/tools/perf/util/parse-events.c
> > +++ b/tools/perf/util/parse-events.c
> > @@ -3084,7 +3084,7 @@ int parse_events_term__num(struct
> > parse_events_term **term,
> >          struct parse_events_term temp = {
> >                  .type_val  = PARSE_EVENTS__TERM_TYPE_NUM,
> >                  .type_term = type_term,
> > -               .config    = config,
> > +               .config    = config ? : strdup(config_term_names[type_term]),
> >                  .no_value  = no_value,
> >                  .err_term  = loc_term ? loc_term->first_column : 0,
> >                  .err_val   = loc_val  ? loc_val->first_column  : 0,
> > --
> If you have this change isn't the change to parse_events_term__num()
> unnecessary?

eh, this is parse_events_term__num().

Anyway, yes, this 1-line change in parse_events_term__num() only does 
look to fix it. More testing required, though.

This 1 liner fix looks good to me.

Cheers,
John