Compiling perf with make LIBPFM4=1 includes libpfm support and
enables test case 63 'Test libpfm4 support'. This test reports an error
on all platforms for subtest 63.2 'test groups of --pfm-events'.
The reported error message is 'nested event groups not supported'
# ./perf test -F 63
63: Test libpfm4 support :
63.1: test of individual --pfm-events :
Error:
failed to parse event stereolab : event not found
Error:
failed to parse event stereolab,instructions : event not found
Error:
failed to parse event instructions,stereolab : event not found
Ok
63.2: test groups of --pfm-events :
Error:
nested event groups not supported <------ Error message here
Error:
failed to parse event {stereolab} : event not found
Error:
failed to parse event {instructions,cycles},{instructions,stereolab} :\
event not found
Ok
#
This patch addresses the error message 'nested event groups not supported'.
The root cause is function parse_libpfm_events_option() which parses the
event string '{},{instructions}' and can not handle a leading empty
group notation '{},...'.
The code detects the first (empty) group indicator '{' but does not
terminate group processing on the following group closing character '}'.
So when the second group indicator '{' is detected, the code assumes
a nested group and returns an error.
With the error message fixed, also change the expected event number to
one for the test case to succeed.
While at it also fix a memory leak. In good case the function does not
free the duplicated string given as first parameter.
Output after:
# ./perf test -F 63
63: Test libpfm4 support :
63.1: test of individual --pfm-events :
Error:
failed to parse event stereolab : event not found
Error:
failed to parse event stereolab,instructions : event not found
Error:
failed to parse event instructions,stereolab : event not found
Ok
63.2: test groups of --pfm-events :
Error:
failed to parse event {stereolab} : event not found
Error:
failed to parse event {instructions,cycles},{instructions,stereolab} : \
event not found
Ok
#
Error message 'nested event groups not supported' is gone.
Signed-off-by: Thomas Richter <[email protected]>
Acked-By: Sumanth Korikkar <[email protected]>
---
tools/perf/tests/pfm.c | 4 ++--
tools/perf/util/pfm.c | 11 ++++++++++-
2 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/tools/perf/tests/pfm.c b/tools/perf/tests/pfm.c
index 76a53126efdf..d4b0ef74defc 100644
--- a/tools/perf/tests/pfm.c
+++ b/tools/perf/tests/pfm.c
@@ -131,8 +131,8 @@ static int test__pfm_group(void)
},
{
.events = "{},{instructions}",
- .nr_events = 0,
- .nr_groups = 0,
+ .nr_events = 1,
+ .nr_groups = 1,
},
{
.events = "{instructions},{instructions}",
diff --git a/tools/perf/util/pfm.c b/tools/perf/util/pfm.c
index d735acb6c29c..6eef6dfeaa57 100644
--- a/tools/perf/util/pfm.c
+++ b/tools/perf/util/pfm.c
@@ -62,8 +62,16 @@ int parse_libpfm_events_option(const struct option *opt, const char *str,
}
/* no event */
- if (*q == '\0')
+ if (*q == '\0') {
+ if (*sep == '}') {
+ if (grp_evt < 0) {
+ ui__error("cannot close a non-existing event group\n");
+ goto error;
+ }
+ grp_evt--;
+ }
continue;
+ }
memset(&attr, 0, sizeof(attr));
event_attr_init(&attr);
@@ -107,6 +115,7 @@ int parse_libpfm_events_option(const struct option *opt, const char *str,
grp_evt = -1;
}
}
+ free(p_orig);
return 0;
error:
free(p_orig);
--
2.31.1
On Mon, May 17, 2021 at 7:12 AM Thomas Richter <[email protected]> wrote:
>
> Compiling perf with make LIBPFM4=1 includes libpfm support and
> enables test case 63 'Test libpfm4 support'. This test reports an error
> on all platforms for subtest 63.2 'test groups of --pfm-events'.
> The reported error message is 'nested event groups not supported'
The parsing test checks broken and working strings and so errors are
always going to be reported, but agreed this error is wrong.
> # ./perf test -F 63
> 63: Test libpfm4 support :
> 63.1: test of individual --pfm-events :
> Error:
> failed to parse event stereolab : event not found
> Error:
> failed to parse event stereolab,instructions : event not found
> Error:
> failed to parse event instructions,stereolab : event not found
> Ok
> 63.2: test groups of --pfm-events :
> Error:
> nested event groups not supported <------ Error message here
> Error:
> failed to parse event {stereolab} : event not found
> Error:
> failed to parse event {instructions,cycles},{instructions,stereolab} :\
> event not found
> Ok
> #
>
> This patch addresses the error message 'nested event groups not supported'.
> The root cause is function parse_libpfm_events_option() which parses the
> event string '{},{instructions}' and can not handle a leading empty
> group notation '{},...'.
>
> The code detects the first (empty) group indicator '{' but does not
> terminate group processing on the following group closing character '}'.
> So when the second group indicator '{' is detected, the code assumes
> a nested group and returns an error.
>
> With the error message fixed, also change the expected event number to
> one for the test case to succeed.
>
> While at it also fix a memory leak. In good case the function does not
> free the duplicated string given as first parameter.
>
> Output after:
> # ./perf test -F 63
> 63: Test libpfm4 support :
> 63.1: test of individual --pfm-events :
> Error:
> failed to parse event stereolab : event not found
> Error:
> failed to parse event stereolab,instructions : event not found
> Error:
> failed to parse event instructions,stereolab : event not found
> Ok
> 63.2: test groups of --pfm-events :
> Error:
> failed to parse event {stereolab} : event not found
> Error:
> failed to parse event {instructions,cycles},{instructions,stereolab} : \
> event not found
> Ok
> #
> Error message 'nested event groups not supported' is gone.
Acked-By: Ian Rogers <[email protected]>
I wonder if we should add some coverage for the error cases to the pfm
test with something like the following.
Thanks,
Ian
--- a/tools/perf/tests/pfm.c
+++ b/tools/perf/tests/pfm.c
@@ -155,6 +155,16 @@ static int test__pfm_group(void)
.nr_events = 3,
.nr_groups = 1,
},
+ {
+ .events = "instructions}",
+ .nr_events = 1,
+ .nr_groups = 0,
+ },
+ {
+ .events = "{{instructions}}",
+ .nr_events = 0,
+ .nr_groups = 0,
+ },
};
for (i = 0; i < ARRAY_SIZE(table); i++) {
> Signed-off-by: Thomas Richter <[email protected]>
> Acked-By: Sumanth Korikkar <[email protected]>
> ---
> tools/perf/tests/pfm.c | 4 ++--
> tools/perf/util/pfm.c | 11 ++++++++++-
> 2 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/tests/pfm.c b/tools/perf/tests/pfm.c
> index 76a53126efdf..d4b0ef74defc 100644
> --- a/tools/perf/tests/pfm.c
> +++ b/tools/perf/tests/pfm.c
> @@ -131,8 +131,8 @@ static int test__pfm_group(void)
> },
> {
> .events = "{},{instructions}",
> - .nr_events = 0,
> - .nr_groups = 0,
> + .nr_events = 1,
> + .nr_groups = 1,
> },
> {
> .events = "{instructions},{instructions}",
> diff --git a/tools/perf/util/pfm.c b/tools/perf/util/pfm.c
> index d735acb6c29c..6eef6dfeaa57 100644
> --- a/tools/perf/util/pfm.c
> +++ b/tools/perf/util/pfm.c
> @@ -62,8 +62,16 @@ int parse_libpfm_events_option(const struct option *opt, const char *str,
> }
>
> /* no event */
> - if (*q == '\0')
> + if (*q == '\0') {
> + if (*sep == '}') {
> + if (grp_evt < 0) {
> + ui__error("cannot close a non-existing event group\n");
> + goto error;
> + }
> + grp_evt--;
> + }
> continue;
> + }
>
> memset(&attr, 0, sizeof(attr));
> event_attr_init(&attr);
> @@ -107,6 +115,7 @@ int parse_libpfm_events_option(const struct option *opt, const char *str,
> grp_evt = -1;
> }
> }
> + free(p_orig);
> return 0;
> error:
> free(p_orig);
> --
> 2.31.1
>
Em Tue, May 18, 2021 at 04:30:35PM -0700, Ian Rogers escreveu:
> On Mon, May 17, 2021 at 7:12 AM Thomas Richter <[email protected]> wrote:
> >
> > Compiling perf with make LIBPFM4=1 includes libpfm support and
> > enables test case 63 'Test libpfm4 support'. This test reports an error
> > on all platforms for subtest 63.2 'test groups of --pfm-events'.
> > The reported error message is 'nested event groups not supported'
>
> The parsing test checks broken and working strings and so errors are
> always going to be reported, but agreed this error is wrong.
>
> > # ./perf test -F 63
> > 63: Test libpfm4 support :
> > 63.1: test of individual --pfm-events :
> > Error:
> > failed to parse event stereolab : event not found
> > Error:
> > failed to parse event stereolab,instructions : event not found
> > Error:
> > failed to parse event instructions,stereolab : event not found
> > Ok
> > 63.2: test groups of --pfm-events :
> > Error:
> > nested event groups not supported <------ Error message here
> > Error:
> > failed to parse event {stereolab} : event not found
> > Error:
> > failed to parse event {instructions,cycles},{instructions,stereolab} :\
> > event not found
> > Ok
> > #
> >
> > This patch addresses the error message 'nested event groups not supported'.
> > The root cause is function parse_libpfm_events_option() which parses the
> > event string '{},{instructions}' and can not handle a leading empty
> > group notation '{},...'.
> >
> > The code detects the first (empty) group indicator '{' but does not
> > terminate group processing on the following group closing character '}'.
> > So when the second group indicator '{' is detected, the code assumes
> > a nested group and returns an error.
> >
> > With the error message fixed, also change the expected event number to
> > one for the test case to succeed.
> >
> > While at it also fix a memory leak. In good case the function does not
> > free the duplicated string given as first parameter.
> >
> > Output after:
> > # ./perf test -F 63
> > 63: Test libpfm4 support :
> > 63.1: test of individual --pfm-events :
> > Error:
> > failed to parse event stereolab : event not found
> > Error:
> > failed to parse event stereolab,instructions : event not found
> > Error:
> > failed to parse event instructions,stereolab : event not found
> > Ok
> > 63.2: test groups of --pfm-events :
> > Error:
> > failed to parse event {stereolab} : event not found
> > Error:
> > failed to parse event {instructions,cycles},{instructions,stereolab} : \
> > event not found
> > Ok
> > #
> > Error message 'nested event groups not supported' is gone.
>
> Acked-By: Ian Rogers <[email protected]>
>
> I wonder if we should add some coverage for the error cases to the pfm
> test with something like the following.
Yeah, agreed, please consider sending a patch for that.
Thanks, applied.
- Arnaldo
> Thanks,
> Ian
>
> --- a/tools/perf/tests/pfm.c
> +++ b/tools/perf/tests/pfm.c
> @@ -155,6 +155,16 @@ static int test__pfm_group(void)
> .nr_events = 3,
> .nr_groups = 1,
> },
> + {
> + .events = "instructions}",
> + .nr_events = 1,
> + .nr_groups = 0,
> + },
> + {
> + .events = "{{instructions}}",
> + .nr_events = 0,
> + .nr_groups = 0,
> + },
> };
>
> for (i = 0; i < ARRAY_SIZE(table); i++) {
>
> > Signed-off-by: Thomas Richter <[email protected]>
> > Acked-By: Sumanth Korikkar <[email protected]>
> > ---
> > tools/perf/tests/pfm.c | 4 ++--
> > tools/perf/util/pfm.c | 11 ++++++++++-
> > 2 files changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/perf/tests/pfm.c b/tools/perf/tests/pfm.c
> > index 76a53126efdf..d4b0ef74defc 100644
> > --- a/tools/perf/tests/pfm.c
> > +++ b/tools/perf/tests/pfm.c
> > @@ -131,8 +131,8 @@ static int test__pfm_group(void)
> > },
> > {
> > .events = "{},{instructions}",
> > - .nr_events = 0,
> > - .nr_groups = 0,
> > + .nr_events = 1,
> > + .nr_groups = 1,
> > },
> > {
> > .events = "{instructions},{instructions}",
> > diff --git a/tools/perf/util/pfm.c b/tools/perf/util/pfm.c
> > index d735acb6c29c..6eef6dfeaa57 100644
> > --- a/tools/perf/util/pfm.c
> > +++ b/tools/perf/util/pfm.c
> > @@ -62,8 +62,16 @@ int parse_libpfm_events_option(const struct option *opt, const char *str,
> > }
> >
> > /* no event */
> > - if (*q == '\0')
> > + if (*q == '\0') {
> > + if (*sep == '}') {
> > + if (grp_evt < 0) {
> > + ui__error("cannot close a non-existing event group\n");
> > + goto error;
> > + }
> > + grp_evt--;
> > + }
> > continue;
> > + }
> >
> > memset(&attr, 0, sizeof(attr));
> > event_attr_init(&attr);
> > @@ -107,6 +115,7 @@ int parse_libpfm_events_option(const struct option *opt, const char *str,
> > grp_evt = -1;
> > }
> > }
> > + free(p_orig);
> > return 0;
> > error:
> > free(p_orig);
> > --
> > 2.31.1
> >
--
- Arnaldo