[PATCH v1] perf jevents: Fix build issue in '*/' in event descriptions

[PATCH v1] perf jevents: Fix build issue in '*/' in event descriptions

[Ian Rogers]

For big string offsets we output comments for what string the offset
is for. If the string contains a '*/' as seen in Intel Arrowlake event
descriptions, then this causes C parsing issues for the generated
pmu-events.c. Catch such '*/' values and escape to avoid this.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/pmu-events/jevents.py | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
index 6e71b09dbc2a..028cf3c43881 100755
--- a/tools/perf/pmu-events/jevents.py
+++ b/tools/perf/pmu-events/jevents.py
@@ -430,8 +430,11 @@ class JsonEvent:
   def to_c_string(self, metric: bool) -> str:
     """Representation of the event as a C struct initializer."""
 
+    def fix_comment(s: str) -> str:
+        return s.replace('*/', '\*\/')
+
     s = self.build_c_string(metric)
-    return f'{{ { _bcs.offsets[s] } }}, /* {s} */\n'
+    return f'{{ { _bcs.offsets[s] } }}, /* {fix_comment(s)} */\n'
 
 
 @lru_cache(maxsize=None)
-- 
2.47.0.277.g8800431eea-goog

Re: [PATCH v1] perf jevents: Fix build issue in '*/' in event descriptions

[Ian Rogers]

On Wed, Nov 13, 2024 at 8:56 AM Ian Rogers <irogers@google.com> wrote:
>
> For big string offsets we output comments for what string the offset
> is for. If the string contains a '*/' as seen in Intel Arrowlake event
> descriptions, then this causes C parsing issues for the generated
> pmu-events.c. Catch such '*/' values and escape to avoid this.
>
> Signed-off-by: Ian Rogers <irogers@google.com>

Ping.

Thanks,
Ian

> ---
>  tools/perf/pmu-events/jevents.py | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
> index 6e71b09dbc2a..028cf3c43881 100755
> --- a/tools/perf/pmu-events/jevents.py
> +++ b/tools/perf/pmu-events/jevents.py
> @@ -430,8 +430,11 @@ class JsonEvent:
>    def to_c_string(self, metric: bool) -> str:
>      """Representation of the event as a C struct initializer."""
>
> +    def fix_comment(s: str) -> str:
> +        return s.replace('*/', '\*\/')
> +
>      s = self.build_c_string(metric)
> -    return f'{{ { _bcs.offsets[s] } }}, /* {s} */\n'
> +    return f'{{ { _bcs.offsets[s] } }}, /* {fix_comment(s)} */\n'
>
>
>  @lru_cache(maxsize=None)
> --
> 2.47.0.277.g8800431eea-goog
>

Re: [PATCH v1] perf jevents: Fix build issue in '*/' in event descriptions

[Arnaldo Carvalho de Melo]

On Mon, Dec 09, 2024 at 10:35:34AM -0800, Ian Rogers wrote:
> On Wed, Nov 13, 2024 at 8:56 AM Ian Rogers <irogers@google.com> wrote:
> >
> > For big string offsets we output comments for what string the offset
> > is for. If the string contains a '*/' as seen in Intel Arrowlake event
> > descriptions, then this causes C parsing issues for the generated
> > pmu-events.c. Catch such '*/' values and escape to avoid this.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> 
> Ping.

A fixes: is missing, probably this should go via perf-tools, i.e. for
this merge window?

- Arnaldo
 
> Thanks,
> Ian
> 
> > ---
> >  tools/perf/pmu-events/jevents.py | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
> > index 6e71b09dbc2a..028cf3c43881 100755
> > --- a/tools/perf/pmu-events/jevents.py
> > +++ b/tools/perf/pmu-events/jevents.py
> > @@ -430,8 +430,11 @@ class JsonEvent:
> >    def to_c_string(self, metric: bool) -> str:
> >      """Representation of the event as a C struct initializer."""
> >
> > +    def fix_comment(s: str) -> str:
> > +        return s.replace('*/', '\*\/')
> > +
> >      s = self.build_c_string(metric)
> > -    return f'{{ { _bcs.offsets[s] } }}, /* {s} */\n'
> > +    return f'{{ { _bcs.offsets[s] } }}, /* {fix_comment(s)} */\n'
> >
> >
> >  @lru_cache(maxsize=None)
> > --
> > 2.47.0.277.g8800431eea-goog
> >

Re: [PATCH v1] perf jevents: Fix build issue in '*/' in event descriptions

[Ian Rogers]

On Mon, Dec 9, 2024 at 12:28 PM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> On Mon, Dec 09, 2024 at 10:35:34AM -0800, Ian Rogers wrote:
> > On Wed, Nov 13, 2024 at 8:56 AM Ian Rogers <irogers@google.com> wrote:
> > >
> > > For big string offsets we output comments for what string the offset
> > > is for. If the string contains a '*/' as seen in Intel Arrowlake event
> > > descriptions, then this causes C parsing issues for the generated
> > > pmu-events.c. Catch such '*/' values and escape to avoid this.
> > >
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> >
> > Ping.
>
> A fixes: is missing, probably this should go via perf-tools, i.e. for
> this merge window?

We don't yet have arrowlake events/metrics, should there be a fixes?
I'm just preparing the patches for the latest vendor json from Intel,
but they will depend on this. I suspect given the size of the vendor
json it will miss the current merge window.

Thanks,
Ian

Re: [PATCH v1] perf jevents: Fix build issue in '*/' in event descriptions

[Arnaldo Carvalho de Melo]

On Mon, Dec 09, 2024 at 01:29:15PM -0800, Ian Rogers wrote:
> On Mon, Dec 9, 2024 at 12:28 PM Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> > On Mon, Dec 09, 2024 at 10:35:34AM -0800, Ian Rogers wrote:
> > > On Wed, Nov 13, 2024 at 8:56 AM Ian Rogers <irogers@google.com> wrote:
> > > >
> > > > For big string offsets we output comments for what string the offset
> > > > is for. If the string contains a '*/' as seen in Intel Arrowlake event
> > > > descriptions, then this causes C parsing issues for the generated
> > > > pmu-events.c. Catch such '*/' values and escape to avoid this.
> > > >
> > > > Signed-off-by: Ian Rogers <irogers@google.com>
> > >
> > > Ping.
> >
> > A fixes: is missing, probably this should go via perf-tools, i.e. for
> > this merge window?
> 
> We don't yet have arrowlake events/metrics, should there be a fixes?

Ok, thanks for the clarification.

> I'm just preparing the patches for the latest vendor json from Intel,
> but they will depend on this. I suspect given the size of the vendor
> json it will miss the current merge window.

Probably best to have big patches via perf-tools-next at this point in
time.

- Arnaldo

Re: [PATCH v1] perf jevents: Fix build issue in '*/' in event descriptions

[Arnaldo Carvalho de Melo]

On Tue, Dec 10, 2024 at 10:15:30AM -0300, Arnaldo Carvalho de Melo wrote:
> On Mon, Dec 09, 2024 at 01:29:15PM -0800, Ian Rogers wrote:
> > On Mon, Dec 9, 2024 at 12:28 PM Arnaldo Carvalho de Melo
> > <acme@kernel.org> wrote:
> > >
> > > On Mon, Dec 09, 2024 at 10:35:34AM -0800, Ian Rogers wrote:
> > > > On Wed, Nov 13, 2024 at 8:56 AM Ian Rogers <irogers@google.com> wrote:
> > > > >
> > > > > For big string offsets we output comments for what string the offset
> > > > > is for. If the string contains a '*/' as seen in Intel Arrowlake event
> > > > > descriptions, then this causes C parsing issues for the generated
> > > > > pmu-events.c. Catch such '*/' values and escape to avoid this.
> > > > >
> > > > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > >
> > > > Ping.
> > >
> > > A fixes: is missing, probably this should go via perf-tools, i.e. for
> > > this merge window?
> > 
> > We don't yet have arrowlake events/metrics, should there be a fixes?
> 
> Ok, thanks for the clarification.
> 
> > I'm just preparing the patches for the latest vendor json from Intel,
> > but they will depend on this. I suspect given the size of the vendor
> > json it will miss the current merge window.
> 
> Probably best to have big patches via perf-tools-next at this point in
> time.

I'm seeing this after applying:

/home/acme/git/perf-tools-next/tools/perf/pmu-events/jevents.py:434: SyntaxWarning: invalid escape sequence '\*'
  return s.replace('*/', '\*\/')


⬢ [acme@toolbox perf-tools-next]$ head /etc/os-release 
NAME="Fedora Linux"
VERSION="40 (Toolbx Container Image)"
ID=fedora
VERSION_ID=40
VERSION_CODENAME=""
PLATFORM_ID="platform:f40"
PRETTY_NAME="Fedora Linux 40 (Toolbx Container Image)"
ANSI_COLOR="0;38;2;60;110;180"
LOGO=fedora-logo-icon
CPE_NAME="cpe:/o:fedoraproject:fedora:40"
⬢ [acme@toolbox perf-tools-next]$ python --version
Python 3.12.7
⬢ [acme@toolbox perf-tools-next]$

Re: [PATCH v1] perf jevents: Fix build issue in '*/' in event descriptions

[Ian Rogers]

On Tue, Dec 10, 2024 at 11:17 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> On Tue, Dec 10, 2024 at 10:15:30AM -0300, Arnaldo Carvalho de Melo wrote:
> > On Mon, Dec 09, 2024 at 01:29:15PM -0800, Ian Rogers wrote:
> > > On Mon, Dec 9, 2024 at 12:28 PM Arnaldo Carvalho de Melo
> > > <acme@kernel.org> wrote:
> > > >
> > > > On Mon, Dec 09, 2024 at 10:35:34AM -0800, Ian Rogers wrote:
> > > > > On Wed, Nov 13, 2024 at 8:56 AM Ian Rogers <irogers@google.com> wrote:
> > > > > >
> > > > > > For big string offsets we output comments for what string the offset
> > > > > > is for. If the string contains a '*/' as seen in Intel Arrowlake event
> > > > > > descriptions, then this causes C parsing issues for the generated
> > > > > > pmu-events.c. Catch such '*/' values and escape to avoid this.
> > > > > >
> > > > > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > > >
> > > > > Ping.
> > > >
> > > > A fixes: is missing, probably this should go via perf-tools, i.e. for
> > > > this merge window?
> > >
> > > We don't yet have arrowlake events/metrics, should there be a fixes?
> >
> > Ok, thanks for the clarification.
> >
> > > I'm just preparing the patches for the latest vendor json from Intel,
> > > but they will depend on this. I suspect given the size of the vendor
> > > json it will miss the current merge window.
> >
> > Probably best to have big patches via perf-tools-next at this point in
> > time.
>
> I'm seeing this after applying:
>
> /home/acme/git/perf-tools-next/tools/perf/pmu-events/jevents.py:434: SyntaxWarning: invalid escape sequence '\*'
>   return s.replace('*/', '\*\/')

It likely needs to be:
```
return s.replace('*/', r'\*\/')
```
note the r. Could you test for me?

Thanks,
Ian

>
>
> ⬢ [acme@toolbox perf-tools-next]$ head /etc/os-release
> NAME="Fedora Linux"
> VERSION="40 (Toolbx Container Image)"
> ID=fedora
> VERSION_ID=40
> VERSION_CODENAME=""
> PLATFORM_ID="platform:f40"
> PRETTY_NAME="Fedora Linux 40 (Toolbx Container Image)"
> ANSI_COLOR="0;38;2;60;110;180"
> LOGO=fedora-logo-icon
> CPE_NAME="cpe:/o:fedoraproject:fedora:40"
> ⬢ [acme@toolbox perf-tools-next]$ python --version
> Python 3.12.7
> ⬢ [acme@toolbox perf-tools-next]$

Re: [PATCH v1] perf jevents: Fix build issue in '*/' in event descriptions

[Arnaldo Carvalho de Melo]

On Tue, Dec 10, 2024 at 11:24:28AM -0800, Ian Rogers wrote:
> On Tue, Dec 10, 2024 at 11:17 AM Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> > On Tue, Dec 10, 2024 at 10:15:30AM -0300, Arnaldo Carvalho de Melo wrote:
> > > On Mon, Dec 09, 2024 at 01:29:15PM -0800, Ian Rogers wrote:
> > > > On Mon, Dec 9, 2024 at 12:28 PM Arnaldo Carvalho de Melo
> > > > <acme@kernel.org> wrote:
> > > > >
> > > > > On Mon, Dec 09, 2024 at 10:35:34AM -0800, Ian Rogers wrote:
> > > > > > On Wed, Nov 13, 2024 at 8:56 AM Ian Rogers <irogers@google.com> wrote:
> > > > > > >
> > > > > > > For big string offsets we output comments for what string the offset
> > > > > > > is for. If the string contains a '*/' as seen in Intel Arrowlake event
> > > > > > > descriptions, then this causes C parsing issues for the generated
> > > > > > > pmu-events.c. Catch such '*/' values and escape to avoid this.
> > > > > > >
> > > > > > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > > > >
> > > > > > Ping.
> > > > >
> > > > > A fixes: is missing, probably this should go via perf-tools, i.e. for
> > > > > this merge window?
> > > >
> > > > We don't yet have arrowlake events/metrics, should there be a fixes?
> > >
> > > Ok, thanks for the clarification.
> > >
> > > > I'm just preparing the patches for the latest vendor json from Intel,
> > > > but they will depend on this. I suspect given the size of the vendor
> > > > json it will miss the current merge window.
> > >
> > > Probably best to have big patches via perf-tools-next at this point in
> > > time.
> >
> > I'm seeing this after applying:
> >
> > /home/acme/git/perf-tools-next/tools/perf/pmu-events/jevents.py:434: SyntaxWarning: invalid escape sequence '\*'
> >   return s.replace('*/', '\*\/')
> 
> It likely needs to be:
> ```
> return s.replace('*/', r'\*\/')
> ```
> note the r. Could you test for me?

Sure.

- Arnaldo

Re: [PATCH v1] perf jevents: Fix build issue in '*/' in event descriptions

[Arnaldo Carvalho de Melo]

On Tue, Dec 10, 2024 at 04:58:19PM -0300, Arnaldo Carvalho de Melo wrote:
> On Tue, Dec 10, 2024 at 11:24:28AM -0800, Ian Rogers wrote:
> > On Tue, Dec 10, 2024 at 11:17 AM Arnaldo Carvalho de Melo
> > > On Tue, Dec 10, 2024 at 10:15:30AM -0300, Arnaldo Carvalho de Melo wrote:
> > > > Probably best to have big patches via perf-tools-next at this point in
> > > > time.
> > >
> > > I'm seeing this after applying:
> > >
> > > /home/acme/git/perf-tools-next/tools/perf/pmu-events/jevents.py:434: SyntaxWarning: invalid escape sequence '\*'
> > >   return s.replace('*/', '\*\/')
> > 
> > It likely needs to be:
> > ```
> > return s.replace('*/', r'\*\/')
> > ```
> > note the r. Could you test for me?
> 
> Sure.

Yeah, no more warning, thanks, fixed it up.

- Arnaldo

Re: [PATCH v1] perf jevents: Fix build issue in '*/' in event descriptions

[Ian Rogers]

On Tue, Dec 10, 2024 at 12:01 PM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> On Tue, Dec 10, 2024 at 04:58:19PM -0300, Arnaldo Carvalho de Melo wrote:
> > On Tue, Dec 10, 2024 at 11:24:28AM -0800, Ian Rogers wrote:
> > > On Tue, Dec 10, 2024 at 11:17 AM Arnaldo Carvalho de Melo
> > > > On Tue, Dec 10, 2024 at 10:15:30AM -0300, Arnaldo Carvalho de Melo wrote:
> > > > > Probably best to have big patches via perf-tools-next at this point in
> > > > > time.
> > > >
> > > > I'm seeing this after applying:
> > > >
> > > > /home/acme/git/perf-tools-next/tools/perf/pmu-events/jevents.py:434: SyntaxWarning: invalid escape sequence '\*'
> > > >   return s.replace('*/', '\*\/')
> > >
> > > It likely needs to be:
> > > ```
> > > return s.replace('*/', r'\*\/')
> > > ```
> > > note the r. Could you test for me?
> >
> > Sure.
>
> Yeah, no more warning, thanks, fixed it up.

Thanks for your help!
Ian