Messages in this thread | | | From | Ian Rogers <> | Date | Wed, 25 Aug 2021 09:47:32 -0700 | Subject | Re: [Question] perf tools: lex parsing issue |
| |
On Wed, Aug 25, 2021 at 7:32 AM John Garry <john.garry@huawei.com> 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? This 1 liner fix looks good to me.
Thanks, Ian
> > > > It looks like a similar fix is needed in format_alias. > > And the change in parse_events_term__num(), above, seems to resolve that. > > Previously we would have something like these in format_alias(): > > buf=i915/bcs0-busy, pmu name=i915, alias->name=bcs0-busy ->str=(null)=0x1000 > > And now: > > buf=i915/bcs0-busy, pmu name=i915, alias->name=bcs0-busy ->str=config=0x1000 > > > Thoughts? > > Thanks, > John >
| |