Re: [Question] perf tools: lex parsing issue

On 25/08/2021 17:47, Ian Rogers wrote:

On Wed, Aug 25, 2021 at 7:32 AM John Garry[off-list ref]  wrote:

quoted

On 25/08/2021 01:13, Ian Rogers wrote:

Thanks for getting back to me

quoted

quoted

+ 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;
   }

quoted

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.

quoted

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