LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [Question] perf tools: lex parsing issue
@ 2021-08-23 11:27 John Garry
  2021-08-25  0:13 ` Ian Rogers
  0 siblings, 1 reply; 5+ messages in thread
From: John Garry @ 2021-08-23 11:27 UTC (permalink / raw)
  To: Jiri Olsa, Arnaldo Carvalho de Melo
  Cc: linux-kernel, linux-perf-users, Peter Zijlstra, irogers,
	Ingo Molnar, Mark Rutland, Alexander Shishkin, Namhyung Kim

Hi jirka,

If you remember from some time ago we discussed how the lex parsing 
creates strange aliases:

https://lore.kernel.org/lkml/20200320093006.GA1343171@krava/

I am no expert on l+y, but it seems that we simply don't set the term 
config field for known term types. Well, not for 
PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD type anyway.

This super hack resolves that issue:

--->8----

--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -765,7 +765,12 @@ event_config ',' event_term
struct list_head *head = $1;
struct parse_events_term *term = $3

+ if (term->type_term == PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD) {
+ 	term->config = strdup("period");
+ }
+
if (!head) {
	parse_events_term__delete(term);
	YYABORT;
-- 

----8-----

So we get "umask=0x80,period=0x30d40,event=0x6" now, rather than 
"umask=0x80,(null)=0x30d40,event=0x6", for the perf_pmu_alias.str, as an 
example.

Did you ever get a chance to look into this issue? Do you know how could 
or should this field be set properly?

Some more background:
The reason I was looking at this is because I think it causes a problem 
for pmu-events (JSONs) aliasing for some PMUs. Specifically it's PMU 
which use "config=xxx" in sysfs files in 
/sys/bus/event_source/devices/PMUx/events/, rather than "event=xxx". The 
actual problem is that I trigger this warn in pmu.c:

static void perf_pmu_assign_str(char *name, const char *field, char 
**old_str,
char **new_str)
{

if (*new_str) { /* Have new string, check with old */
	if (strcasecmp(*old_str, *new_str))
		pr_debug("alias %s differs i ... <---

As I get "config=event=0xXXX" vs "config=(null)=0xXXX"

As I am not sure how to solve that yet, but, since we have 
config=(null), I thought it best to solve the first issue first.

Thanks,
John

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Question] perf tools: lex parsing issue
  2021-08-23 11:27 [Question] perf tools: lex parsing issue John Garry
@ 2021-08-25  0:13 ` Ian Rogers
  2021-08-25 14:36   ` John Garry
  0 siblings, 1 reply; 5+ messages in thread
From: Ian Rogers @ 2021-08-25  0:13 UTC (permalink / raw)
  To: John Garry
  Cc: Jiri Olsa, Arnaldo Carvalho de Melo, linux-kernel,
	linux-perf-users, Peter Zijlstra, Ingo Molnar, Mark Rutland,
	Alexander Shishkin, Namhyung Kim

On Mon, Aug 23, 2021 at 4:23 AM John Garry <john.garry@huawei.com> wrote:
>
> Hi jirka,
>
> If you remember from some time ago we discussed how the lex parsing
> creates strange aliases:
>
> https://lore.kernel.org/lkml/20200320093006.GA1343171@krava/
>
> I am no expert on l+y, but it seems that we simply don't set the term
> config field for known term types. Well, not for
> PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD type anyway.
>
> This super hack resolves that issue:
>
> --->8----
>
> --- a/tools/perf/util/parse-events.y
> +++ b/tools/perf/util/parse-events.y
> @@ -765,7 +765,12 @@ event_config ',' event_term
> struct list_head *head = $1;
> struct parse_events_term *term = $3
>
> + 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, 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];
+               }
                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);

It looks like a similar fix is needed in format_alias.

Thanks,
Ian

> So we get "umask=0x80,period=0x30d40,event=0x6" now, rather than
> "umask=0x80,(null)=0x30d40,event=0x6", for the perf_pmu_alias.str, as an
> example.
>
> Did you ever get a chance to look into this issue? Do you know how could
> or should this field be set properly?
>
> Some more background:
> The reason I was looking at this is because I think it causes a problem
> for pmu-events (JSONs) aliasing for some PMUs. Specifically it's PMU
> which use "config=xxx" in sysfs files in
> /sys/bus/event_source/devices/PMUx/events/, rather than "event=xxx". The
> actual problem is that I trigger this warn in pmu.c:
>
> static void perf_pmu_assign_str(char *name, const char *field, char
> **old_str,
> char **new_str)
> {
>
> if (*new_str) { /* Have new string, check with old */
>         if (strcasecmp(*old_str, *new_str))
>                 pr_debug("alias %s differs i ... <---
>
> As I get "config=event=0xXXX" vs "config=(null)=0xXXX"
>
> As I am not sure how to solve that yet, but, since we have
> config=(null), I thought it best to solve the first issue first.
>
> Thanks,
> John

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Question] perf tools: lex parsing issue
  2021-08-25  0:13 ` Ian Rogers
@ 2021-08-25 14:36   ` John Garry
  2021-08-25 16:47     ` Ian Rogers
  0 siblings, 1 reply; 5+ messages in thread
From: John Garry @ 2021-08-25 14:36 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Jiri Olsa, Arnaldo Carvalho de Melo, linux-kernel,
	linux-perf-users, Peter Zijlstra, Ingo Molnar, Mark Rutland,
	Alexander Shishkin, Namhyung Kim

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

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Question] perf tools: lex parsing issue
  2021-08-25 14:36   ` John Garry
@ 2021-08-25 16:47     ` Ian Rogers
  2021-08-25 16:58       ` John Garry
  0 siblings, 1 reply; 5+ messages in thread
From: Ian Rogers @ 2021-08-25 16:47 UTC (permalink / raw)
  To: John Garry
  Cc: Jiri Olsa, Arnaldo Carvalho de Melo, linux-kernel,
	linux-perf-users, Peter Zijlstra, Ingo Molnar, Mark Rutland,
	Alexander Shishkin, Namhyung Kim

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
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Question] perf tools: lex parsing issue
  2021-08-25 16:47     ` Ian Rogers
@ 2021-08-25 16:58       ` John Garry
  0 siblings, 0 replies; 5+ messages in thread
From: John Garry @ 2021-08-25 16:58 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Jiri Olsa, Arnaldo Carvalho de Melo, linux-kernel,
	linux-perf-users, Peter Zijlstra, Ingo Molnar, Mark Rutland,
	Alexander Shishkin, Namhyung Kim

On 25/08/2021 17:47, Ian Rogers wrote:
> 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?

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-08-25 16:54 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-23 11:27 [Question] perf tools: lex parsing issue John Garry
2021-08-25  0:13 ` Ian Rogers
2021-08-25 14:36   ` John Garry
2021-08-25 16:47     ` Ian Rogers
2021-08-25 16:58       ` John Garry

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).