[PATCH 3/3] perf parse: Allow names to start with digits

Dominique Martinet posted 3 patches 1 year, 10 months ago
There is a newer version of this series
[PATCH 3/3] perf parse: Allow names to start with digits
Posted by Dominique Martinet 1 year, 10 months ago
Tracepoints can start with digits, although we don't have many of these:

$ rg -g '*.h' '\bTRACE_EVENT\([0-9]'
net/mac802154/trace.h
53:TRACE_EVENT(802154_drv_return_int,
...

net/ieee802154/trace.h
66:TRACE_EVENT(802154_rdev_add_virtual_intf,
...

include/trace/events/9p.h
124:TRACE_EVENT(9p_client_req,
...

Just allow names to start with digits too so e.g. perf probe -e '9p:*'
works

Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
---
 tools/perf/tests/parse-events.c | 5 +++++
 tools/perf/util/parse-events.l  | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
index ef056e8740fe..6cf055dd5c09 100644
--- a/tools/perf/tests/parse-events.c
+++ b/tools/perf/tests/parse-events.c
@@ -2280,6 +2280,11 @@ static const struct evlist_test test__events[] = {
 		.check = test__checkevent_breakpoint_2_events,
 		/* 3 */
 	},
+	{
+		.name = "9p:9p_client_req",
+		.check = test__checkevent_tracepoint,
+		/* 4 */
+	},
 };
 
 static const struct evlist_test test__events_pmu[] = {
diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index e86c45675e1d..41c30ff29783 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -158,7 +158,7 @@ event		[^,{}/]+
 num_dec		[0-9]+
 num_hex		0x[a-fA-F0-9]{1,16}
 num_raw_hex	[a-fA-F0-9]{1,16}
-name		[a-zA-Z_*?\[\]][a-zA-Z0-9_*?.\[\]!\-]*
+name		[a-zA-Z0-9_*?\[\]][a-zA-Z0-9_*?.\[\]!\-]*
 name_tag	[\'][a-zA-Z_*?\[\]][a-zA-Z0-9_*?\-,\.\[\]:=]*[\']
 name_minus	[a-zA-Z_*?][a-zA-Z0-9\-_*?.:]*
 drv_cfg_term	[a-zA-Z0-9_\.]+(=[a-zA-Z0-9_*?\.:]+)?

-- 
2.43.0
Re: [PATCH 3/3] perf parse: Allow names to start with digits
Posted by Dominique Martinet 1 year, 10 months ago
Dominique Martinet wrote on Sun, Apr 07, 2024 at 09:18:21PM +0900:
> diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
> index e86c45675e1d..41c30ff29783 100644
> --- a/tools/perf/util/parse-events.l
> +++ b/tools/perf/util/parse-events.l
> @@ -158,7 +158,7 @@ event		[^,{}/]+
>  num_dec		[0-9]+
>  num_hex		0x[a-fA-F0-9]{1,16}
>  num_raw_hex	[a-fA-F0-9]{1,16}
> -name		[a-zA-Z_*?\[\]][a-zA-Z0-9_*?.\[\]!\-]*
> +name		[a-zA-Z0-9_*?\[\]][a-zA-Z0-9_*?.\[\]!\-]*

grmbl sorry I guess I didn't actually test this two years ago (?!), or
used it differently (commit message is weird and also needs fixing,
perf probe -e 9p:* does not make sense) or something changed but
that's not enough:

----
$ sudo ./perf trace -e 9p:9p_fid_ref
event syntax error: '9p:9p_fid_ref'
                     \___ parser error
Run 'perf list' for a list of valid events
----

Adding 0-9 to name_tag as well makes perf trace works.

I'm not sure what name_minus is for but I did't need to add that one in
my test.

That also highlights that the test I added isn't sufficient, if someone
familiar with the code could hint at a better place to test please tell!
Otherwise I'll have a look next weekend, I need to get back to my 9p
bugs now I've got a working perf command..

>  name_tag	[\'][a-zA-Z_*?\[\]][a-zA-Z0-9_*?\-,\.\[\]:=]*[\']
>  name_minus	[a-zA-Z_*?][a-zA-Z0-9\-_*?.:]*
>  drv_cfg_term	[a-zA-Z0-9_\.]+(=[a-zA-Z0-9_*?\.:]+)?
> 

-- 
Dominique Martinet | Asmadeus
Re: [PATCH 3/3] perf parse: Allow names to start with digits
Posted by Ian Rogers 1 year, 10 months ago
On Sun, Apr 7, 2024 at 5:38 AM Dominique Martinet
<asmadeus@codewreck.org> wrote:
>
> Dominique Martinet wrote on Sun, Apr 07, 2024 at 09:18:21PM +0900:
> > diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
> > index e86c45675e1d..41c30ff29783 100644
> > --- a/tools/perf/util/parse-events.l
> > +++ b/tools/perf/util/parse-events.l
> > @@ -158,7 +158,7 @@ event             [^,{}/]+
> >  num_dec              [0-9]+
> >  num_hex              0x[a-fA-F0-9]{1,16}
> >  num_raw_hex  [a-fA-F0-9]{1,16}
> > -name         [a-zA-Z_*?\[\]][a-zA-Z0-9_*?.\[\]!\-]*
> > +name         [a-zA-Z0-9_*?\[\]][a-zA-Z0-9_*?.\[\]!\-]*
>
> grmbl sorry I guess I didn't actually test this two years ago (?!), or
> used it differently (commit message is weird and also needs fixing,
> perf probe -e 9p:* does not make sense) or something changed but
> that's not enough:
>
> ----
> $ sudo ./perf trace -e 9p:9p_fid_ref
> event syntax error: '9p:9p_fid_ref'
>                      \___ parser error
> Run 'perf list' for a list of valid events
> ----
>
> Adding 0-9 to name_tag as well makes perf trace works.
>
> I'm not sure what name_minus is for but I did't need to add that one in
> my test.
>
> That also highlights that the test I added isn't sufficient, if someone
> familiar with the code could hint at a better place to test please tell!
> Otherwise I'll have a look next weekend, I need to get back to my 9p
> bugs now I've got a working perf command..
>
> >  name_tag     [\'][a-zA-Z_*?\[\]][a-zA-Z0-9_*?\-,\.\[\]:=]*[\']
> >  name_minus   [a-zA-Z_*?][a-zA-Z0-9\-_*?.:]*
> >  drv_cfg_term [a-zA-Z0-9_\.]+(=[a-zA-Z0-9_*?\.:]+)?
> >

Try adding PARSER_DEBUG=1 to your command line, I need to do the following:
```
$ make EXTRA_CFLAGS="-Wno-error=redundant-decls" PARSER_DEBUG=1
```
For your example it seems to parse with the changes, but I see (which
should be expected):
```
event syntax error: '9p:9p_fid_ref'
                    \___ unknown tracepoint

Error:  File /sys/kernel/tracing//events/9p/9p_fid_ref not found.
Hint:   Perhaps this kernel misses some CONFIG_ setting to enable this feature?.

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
```
I think Jiri's e-mail should be jolsa@kernel.org.

Thanks,
Ian

> --
> Dominique Martinet | Asmadeus
Re: [PATCH 3/3] perf parse: Allow names to start with digits
Posted by Dominique Martinet 1 year, 10 months ago
Ian Rogers wrote on Sun, Apr 07, 2024 at 11:32:31AM -0700:
> Try adding PARSER_DEBUG=1 to your command line, I need to do the following:
> ```
> $ make EXTRA_CFLAGS="-Wno-error=redundant-decls" PARSER_DEBUG=1
> ```

Thanks for this hint!
I tried with that earlier and couldn't reproduce either (e.g. it works
with this patch as you observed), looking at my shell history it turns
out I was just sleep-deprived and I had forgotten the '-e' preceding the
probe and didn't grok the error message...

So this patch is ok.

> I think Jiri's e-mail should be jolsa@kernel.org.

Ah, right -- I used the mail he actually sent the diff with a couple of
years back, but the address in maintainers is jolsa@kernel.org so you're
probably correct that it should be prefered.

I can send a v2 with just this address changed, or whoever picks the
patches up can fix the commit messages for patches 1 & 2, just tell me.
-- 
Dominique Martinet | Asmadeus