[PATCH v2 1/2] perf expr: Test parsing of floating point numbers

[PATCH v2 1/2] perf expr: Test parsing of floating point numbers

[Ian Rogers]

Add test for fix in:
commit 5741da3dee4c ("perf expr: Parse numbers as doubles")

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/expr.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
index f9e8e5628836..3f742612776a 100644
--- a/tools/perf/tests/expr.c
+++ b/tools/perf/tests/expr.c
@@ -39,6 +39,7 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused)
 	ret |= test(&ctx, "min(1,2) + 1", 2);
 	ret |= test(&ctx, "max(1,2) + 1", 3);
 	ret |= test(&ctx, "1+1 if 3*4 else 0", 2);
+	ret |= test(&ctx, "1.1 + 2.1", 3.2);
 
 	if (ret)
 		return ret;
-- 
2.26.2.645.ge9eca65c58-goog

[PATCH v2 2/2] perf test: Improve pmu event metric testing

[Ian Rogers]

Break pmu-events test into 2 and add a test to verify that all pmu metric
expressions simply parse. Try to parse all metric ids/events, warning if
metrics for the current architecture fail to parse.

Tested on power9, skylakex, haswell, broadwell, westmere, sandybridge and
ivybridge with the patch set in place.
May fail on other architectures if metrics are invalid. In particular s390
is untested, but its expressions are trivial. The event encodings could
be wrong but are only warned about. The untested architectures with
expressions are power8, cascadelakex, tremontx, skylake, jaketown, ivytown
and variants of haswell and broadwell.

v2. changes the commit message as event parsing errors no longer cause
the test to fail.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/builtin-test.c |   5 +
 tools/perf/tests/pmu-events.c   | 158 ++++++++++++++++++++++++++++++--
 tools/perf/tests/tests.h        |   2 +
 3 files changed, 159 insertions(+), 6 deletions(-)

diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index 3471ec52ea11..8147c17c71ab 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -75,6 +75,11 @@ static struct test generic_tests[] = {
 	{
 		.desc = "PMU events",
 		.func = test__pmu_events,
+		.subtest = {
+			.get_nr		= test__pmu_events_subtest_get_nr,
+			.get_desc	= test__pmu_events_subtest_get_desc,
+		},
+
 	},
 	{
 		.desc = "DSO data read",
diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
index d64261da8bf7..c18b9ce8cace 100644
--- a/tools/perf/tests/pmu-events.c
+++ b/tools/perf/tests/pmu-events.c
@@ -8,6 +8,10 @@
 #include <linux/zalloc.h>
 #include "debug.h"
 #include "../pmu-events/pmu-events.h"
+#include "util/evlist.h"
+#include "util/expr.h"
+#include "util/parse-events.h"
+#include <ctype.h>
 
 struct perf_pmu_test_event {
 	struct pmu_event event;
@@ -144,7 +148,7 @@ static struct pmu_events_map *__test_pmu_get_events_map(void)
 }
 
 /* Verify generated events from pmu-events.c is as expected */
-static int __test_pmu_event_table(void)
+static int test_pmu_event_table(void)
 {
 	struct pmu_events_map *map = __test_pmu_get_events_map();
 	struct pmu_event *table;
@@ -347,14 +351,11 @@ static int __test__pmu_event_aliases(char *pmu_name, int *count)
 	return res;
 }
 
-int test__pmu_events(struct test *test __maybe_unused,
-		     int subtest __maybe_unused)
+
+static int test_aliases(void)
 {
 	struct perf_pmu *pmu = NULL;
 
-	if (__test_pmu_event_table())
-		return -1;
-
 	while ((pmu = perf_pmu__scan(pmu)) != NULL) {
 		int count = 0;
 
@@ -377,3 +378,148 @@ int test__pmu_events(struct test *test __maybe_unused,
 
 	return 0;
 }
+
+static bool is_number(const char *str)
+{
+	size_t i;
+
+	for (i = 0; i < strlen(str); i++) {
+		if (!isdigit(str[i]) && str[i] != '.')
+			return false;
+	}
+	return true;
+}
+
+static int check_parse_id(const char *id, bool same_cpu, struct pmu_event *pe)
+{
+	struct parse_events_error error;
+	struct evlist *evlist;
+	int ret;
+
+	/* Numbers are always valid. */
+	if (is_number(id))
+		return 0;
+
+	evlist = evlist__new();
+	memset(&error, 0, sizeof(error));
+	ret = parse_events(evlist, id, &error);
+	if (ret && same_cpu) {
+		fprintf(stderr,
+			"\nWARNING: Parse event failed metric '%s' id '%s' expr '%s'\n",
+			pe->metric_name, id, pe->metric_expr);
+		fprintf(stderr, "Error string '%s' help '%s'\n",
+			error.str, error.help);
+	} else if (ret) {
+		pr_debug3("Parse event failed, but for an event that may not be supported by this CPU.\nid '%s' metric '%s' expr '%s'\n",
+			id, pe->metric_name, pe->metric_expr);
+	}
+	evlist__delete(evlist);
+	free(error.str);
+	free(error.help);
+	free(error.first_str);
+	free(error.first_help);
+	/* TODO: too many metrics are broken to fail on this test currently. */
+	return 0;
+}
+
+static int test_parsing(void)
+{
+	struct pmu_events_map *cpus_map = perf_pmu__find_map(NULL);
+	struct pmu_events_map *map;
+	struct pmu_event *pe;
+	int i, j, k;
+	const char **ids;
+	int idnum;
+	int ret = 0;
+	struct expr_parse_ctx ctx;
+	double result;
+
+	i = 0;
+	for (;;) {
+		map = &pmu_events_map[i++];
+		if (!map->table) {
+			map = NULL;
+			break;
+		}
+		j = 0;
+		for (;;) {
+			pe = &map->table[j++];
+			if (!pe->name && !pe->metric_group && !pe->metric_name)
+				break;
+			if (!pe->metric_expr)
+				continue;
+			if (expr__find_other(pe->metric_expr, NULL,
+						&ids, &idnum, 0) < 0) {
+				pr_debug("Parse other failed for map %s %s %s\n",
+					map->cpuid, map->version, map->type);
+				pr_debug("On metric %s\n", pe->metric_name);
+				pr_debug("On expression %s\n", pe->metric_expr);
+				ret++;
+				continue;
+			}
+			expr__ctx_init(&ctx);
+
+			/*
+			 * Add all ids with a made up value. The value may
+			 * trigger divide by zero when subtracted and so try to
+			 * make them unique.
+			 */
+			for (k = 0; k < idnum; k++)
+				expr__add_id(&ctx, ids[k], k + 1);
+
+			for (k = 0; k < idnum; k++) {
+				if (check_parse_id(ids[k], map == cpus_map, pe))
+					ret++;
+			}
+
+			if (expr__parse(&result, &ctx, pe->metric_expr, 0)) {
+				pr_debug("Parse failed for map %s %s %s\n",
+					map->cpuid, map->version, map->type);
+				pr_debug("On metric %s\n", pe->metric_name);
+				pr_debug("On expression %s\n", pe->metric_expr);
+				ret++;
+			}
+			for (k = 0; k < idnum; k++)
+				zfree(&ids[k]);
+			free(ids);
+		}
+	}
+	return ret;
+}
+
+static const struct {
+	int (*func)(void);
+	const char *desc;
+} pmu_events_testcase_table[] = {
+	{
+		.func = test_pmu_event_table,
+		.desc = "PMU event table sanity",
+	},
+	{
+		.func = test_aliases,
+		.desc = "PMU event map aliases",
+	},
+	{
+		.func = test_parsing,
+		.desc = "Parsing of PMU event table metrics",
+	},
+};
+
+const char *test__pmu_events_subtest_get_desc(int i)
+{
+	if (i < 0 || i >= (int)ARRAY_SIZE(pmu_events_testcase_table))
+		return NULL;
+	return pmu_events_testcase_table[i].desc;
+}
+
+int test__pmu_events_subtest_get_nr(void)
+{
+	return (int)ARRAY_SIZE(pmu_events_testcase_table);
+}
+
+int test__pmu_events(struct test *test __maybe_unused, int i)
+{
+	if (i < 0 || i >= (int)ARRAY_SIZE(pmu_events_testcase_table))
+		return TEST_FAIL;
+	return pmu_events_testcase_table[i].func();
+}
diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
index d6d4ac34eeb7..8e316c30ed3c 100644
--- a/tools/perf/tests/tests.h
+++ b/tools/perf/tests/tests.h
@@ -50,6 +50,8 @@ int test__perf_evsel__tp_sched_test(struct test *test, int subtest);
 int test__syscall_openat_tp_fields(struct test *test, int subtest);
 int test__pmu(struct test *test, int subtest);
 int test__pmu_events(struct test *test, int subtest);
+const char *test__pmu_events_subtest_get_desc(int subtest);
+int test__pmu_events_subtest_get_nr(void);
 int test__attr(struct test *test, int subtest);
 int test__dso_data(struct test *test, int subtest);
 int test__dso_data_cache(struct test *test, int subtest);
-- 
2.26.2.645.ge9eca65c58-goog

Re: [PATCH v2 2/2] perf test: Improve pmu event metric testing

[Arnaldo Carvalho de Melo]

Em Tue, May 12, 2020 at 11:27:52PM -0700, Ian Rogers escreveu:
> Break pmu-events test into 2 and add a test to verify that all pmu metric
> expressions simply parse. Try to parse all metric ids/events, warning if
> metrics for the current architecture fail to parse.
> 
> Tested on power9, skylakex, haswell, broadwell, westmere, sandybridge and
> ivybridge with the patch set in place.
> May fail on other architectures if metrics are invalid. In particular s390
> is untested, but its expressions are trivial. The event encodings could
> be wrong but are only warned about. The untested architectures with
> expressions are power8, cascadelakex, tremontx, skylake, jaketown, ivytown
> and variants of haswell and broadwell.
> 
> v2. changes the commit message as event parsing errors no longer cause
> the test to fail.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/tests/builtin-test.c |   5 +
>  tools/perf/tests/pmu-events.c   | 158 ++++++++++++++++++++++++++++++--
>  tools/perf/tests/tests.h        |   2 +
>  3 files changed, 159 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index 3471ec52ea11..8147c17c71ab 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c
> @@ -75,6 +75,11 @@ static struct test generic_tests[] = {
>  	{
>  		.desc = "PMU events",
>  		.func = test__pmu_events,
> +		.subtest = {
> +			.get_nr		= test__pmu_events_subtest_get_nr,
> +			.get_desc	= test__pmu_events_subtest_get_desc,
> +		},
> +
>  	},
>  	{
>  		.desc = "DSO data read",
> diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
> index d64261da8bf7..c18b9ce8cace 100644
> --- a/tools/perf/tests/pmu-events.c
> +++ b/tools/perf/tests/pmu-events.c
> @@ -8,6 +8,10 @@
>  #include <linux/zalloc.h>
>  #include "debug.h"
>  #include "../pmu-events/pmu-events.h"
> +#include "util/evlist.h"
> +#include "util/expr.h"
> +#include "util/parse-events.h"
> +#include <ctype.h>

historically we have been using a ctype.h we got from the git tool
repository, its in:

  tools/include/linux/ctype.h

[acme@five perf]$ find tools/perf/ -name "*.[ch]" | xargs grep '<linux/ctype.h>' | wc -l
39
[acme@five perf]$ find tools/perf/ -name "*.[ch]" | xargs grep '<ctype.h>' | wc -l
2
[acme@five perf]$

[acme@five perf]$ find tools/perf/ -name "*.[ch]" | xargs grep '<ctype.h>'
tools/perf/pmu-events/jevents.c:#include <ctype.h>
tools/perf/util/scripting-engines/trace-event-perl.c:#include <ctype.h>
[acme@five perf]$
  
>  struct perf_pmu_test_event {
>  	struct pmu_event event;
> @@ -144,7 +148,7 @@ static struct pmu_events_map *__test_pmu_get_events_map(void)
>  }
>  
>  /* Verify generated events from pmu-events.c is as expected */
> -static int __test_pmu_event_table(void)
> +static int test_pmu_event_table(void)
>  {
>  	struct pmu_events_map *map = __test_pmu_get_events_map();
>  	struct pmu_event *table;
> @@ -347,14 +351,11 @@ static int __test__pmu_event_aliases(char *pmu_name, int *count)
>  	return res;
>  }
>  
> -int test__pmu_events(struct test *test __maybe_unused,
> -		     int subtest __maybe_unused)
> +
> +static int test_aliases(void)
>  {
>  	struct perf_pmu *pmu = NULL;
>  
> -	if (__test_pmu_event_table())
> -		return -1;
> -
>  	while ((pmu = perf_pmu__scan(pmu)) != NULL) {
>  		int count = 0;
>  
> @@ -377,3 +378,148 @@ int test__pmu_events(struct test *test __maybe_unused,
>  
>  	return 0;
>  }
> +
> +static bool is_number(const char *str)
> +{
> +	size_t i;
> +
> +	for (i = 0; i < strlen(str); i++) {
> +		if (!isdigit(str[i]) && str[i] != '.')
> +			return false;
> +	}
> +	return true;
> +}

The above can still get some wrong numbers, can't we instead use
strtold() and check its return value?

> +static int check_parse_id(const char *id, bool same_cpu, struct pmu_event *pe)
> +{
> +	struct parse_events_error error;
> +	struct evlist *evlist;
> +	int ret;
> +
> +	/* Numbers are always valid. */
> +	if (is_number(id))
> +		return 0;
> +
> +	evlist = evlist__new();
> +	memset(&error, 0, sizeof(error));
> +	ret = parse_events(evlist, id, &error);
> +	if (ret && same_cpu) {
> +		fprintf(stderr,
> +			"\nWARNING: Parse event failed metric '%s' id '%s' expr '%s'\n",
> +			pe->metric_name, id, pe->metric_expr);
> +		fprintf(stderr, "Error string '%s' help '%s'\n",
> +			error.str, error.help);

Can we pr_warning() above to be consistent with using pr_debug3(), right
in the else branch?

> +	} else if (ret) {
> +		pr_debug3("Parse event failed, but for an event that may not be supported by this CPU.\nid '%s' metric '%s' expr '%s'\n",
> +			id, pe->metric_name, pe->metric_expr);
> +	}
> +	evlist__delete(evlist);
> +	free(error.str);
> +	free(error.help);
> +	free(error.first_str);
> +	free(error.first_help);
> +	/* TODO: too many metrics are broken to fail on this test currently. */
> +	return 0;

I was thinking if we could handle the failure of this specific
check_parse_id() in its caller differently and then, at the end, use
some marking like with:

58: builtin clang support                                 : Skip (not compiled in)

Perhaps: 

NN: Parsing of PMU event table metrics                    : Skip (Some metrics failed)

Or some other wording, perhaps a new return value in addition to skip,
fail, ok. That allows the test to continue but flags it as having issues
that should be checked with 'perf test -v'

> +}
> +
> +static int test_parsing(void)
> +{
> +	struct pmu_events_map *cpus_map = perf_pmu__find_map(NULL);
> +	struct pmu_events_map *map;
> +	struct pmu_event *pe;
> +	int i, j, k;
> +	const char **ids;
> +	int idnum;
> +	int ret = 0;
> +	struct expr_parse_ctx ctx;
> +	double result;
> +
> +	i = 0;
> +	for (;;) {
> +		map = &pmu_events_map[i++];
> +		if (!map->table) {
> +			map = NULL;
> +			break;
> +		}
> +		j = 0;
> +		for (;;) {
> +			pe = &map->table[j++];
> +			if (!pe->name && !pe->metric_group && !pe->metric_name)
> +				break;
> +			if (!pe->metric_expr)
> +				continue;
> +			if (expr__find_other(pe->metric_expr, NULL,
> +						&ids, &idnum, 0) < 0) {
> +				pr_debug("Parse other failed for map %s %s %s\n",
> +					map->cpuid, map->version, map->type);
> +				pr_debug("On metric %s\n", pe->metric_name);
> +				pr_debug("On expression %s\n", pe->metric_expr);
> +				ret++;
> +				continue;
> +			}
> +			expr__ctx_init(&ctx);
> +
> +			/*
> +			 * Add all ids with a made up value. The value may
> +			 * trigger divide by zero when subtracted and so try to
> +			 * make them unique.
> +			 */
> +			for (k = 0; k < idnum; k++)
> +				expr__add_id(&ctx, ids[k], k + 1);
> +
> +			for (k = 0; k < idnum; k++) {
> +				if (check_parse_id(ids[k], map == cpus_map, pe))
> +					ret++;
> +			}
> +
> +			if (expr__parse(&result, &ctx, pe->metric_expr, 0)) {
> +				pr_debug("Parse failed for map %s %s %s\n",
> +					map->cpuid, map->version, map->type);
> +				pr_debug("On metric %s\n", pe->metric_name);
> +				pr_debug("On expression %s\n", pe->metric_expr);
> +				ret++;
> +			}
> +			for (k = 0; k < idnum; k++)
> +				zfree(&ids[k]);
> +			free(ids);
> +		}
> +	}
> +	return ret;
> +}
> +
> +static const struct {
> +	int (*func)(void);
> +	const char *desc;
> +} pmu_events_testcase_table[] = {
> +	{
> +		.func = test_pmu_event_table,
> +		.desc = "PMU event table sanity",
> +	},
> +	{
> +		.func = test_aliases,
> +		.desc = "PMU event map aliases",
> +	},
> +	{
> +		.func = test_parsing,
> +		.desc = "Parsing of PMU event table metrics",
> +	},
> +};
> +
> +const char *test__pmu_events_subtest_get_desc(int i)
> +{
> +	if (i < 0 || i >= (int)ARRAY_SIZE(pmu_events_testcase_table))
> +		return NULL;
> +	return pmu_events_testcase_table[i].desc;
> +}
> +
> +int test__pmu_events_subtest_get_nr(void)
> +{
> +	return (int)ARRAY_SIZE(pmu_events_testcase_table);
> +}
> +
> +int test__pmu_events(struct test *test __maybe_unused, int i)
> +{
> +	if (i < 0 || i >= (int)ARRAY_SIZE(pmu_events_testcase_table))
> +		return TEST_FAIL;
> +	return pmu_events_testcase_table[i].func();
> +}
> diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
> index d6d4ac34eeb7..8e316c30ed3c 100644
> --- a/tools/perf/tests/tests.h
> +++ b/tools/perf/tests/tests.h
> @@ -50,6 +50,8 @@ int test__perf_evsel__tp_sched_test(struct test *test, int subtest);
>  int test__syscall_openat_tp_fields(struct test *test, int subtest);
>  int test__pmu(struct test *test, int subtest);
>  int test__pmu_events(struct test *test, int subtest);
> +const char *test__pmu_events_subtest_get_desc(int subtest);
> +int test__pmu_events_subtest_get_nr(void);
>  int test__attr(struct test *test, int subtest);
>  int test__dso_data(struct test *test, int subtest);
>  int test__dso_data_cache(struct test *test, int subtest);
> -- 
> 2.26.2.645.ge9eca65c58-goog
> 

-- 

- Arnaldo

Re: [PATCH v2 1/2] perf expr: Test parsing of floating point numbers

[Arnaldo Carvalho de Melo]

Em Tue, May 12, 2020 at 11:27:51PM -0700, Ian Rogers escreveu:
> Add test for fix in:
> commit 5741da3dee4c ("perf expr: Parse numbers as doubles")
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/tests/expr.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
> index f9e8e5628836..3f742612776a 100644
> --- a/tools/perf/tests/expr.c
> +++ b/tools/perf/tests/expr.c
> @@ -39,6 +39,7 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused)
>  	ret |= test(&ctx, "min(1,2) + 1", 2);
>  	ret |= test(&ctx, "max(1,2) + 1", 3);
>  	ret |= test(&ctx, "1+1 if 3*4 else 0", 2);
> +	ret |= test(&ctx, "1.1 + 2.1", 3.2);
>  
>  	if (ret)
>  		return ret;
> -- 
> 2.26.2.645.ge9eca65c58-goog

Applied.

- Arnaldo

Re: [PATCH v2 2/2] perf test: Improve pmu event metric testing

[Ian Rogers]

On Wed, May 13, 2020 at 8:39 AM Arnaldo Carvalho de Melo
<arnaldo.melo@gmail.com> wrote:
>
> Em Tue, May 12, 2020 at 11:27:52PM -0700, Ian Rogers escreveu:
> > Break pmu-events test into 2 and add a test to verify that all pmu metric
> > expressions simply parse. Try to parse all metric ids/events, warning if
> > metrics for the current architecture fail to parse.
> >
> > Tested on power9, skylakex, haswell, broadwell, westmere, sandybridge and
> > ivybridge with the patch set in place.
> > May fail on other architectures if metrics are invalid. In particular s390
> > is untested, but its expressions are trivial. The event encodings could
> > be wrong but are only warned about. The untested architectures with
> > expressions are power8, cascadelakex, tremontx, skylake, jaketown, ivytown
> > and variants of haswell and broadwell.
> >
> > v2. changes the commit message as event parsing errors no longer cause
> > the test to fail.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/tests/builtin-test.c |   5 +
> >  tools/perf/tests/pmu-events.c   | 158 ++++++++++++++++++++++++++++++--
> >  tools/perf/tests/tests.h        |   2 +
> >  3 files changed, 159 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> > index 3471ec52ea11..8147c17c71ab 100644
> > --- a/tools/perf/tests/builtin-test.c
> > +++ b/tools/perf/tests/builtin-test.c
> > @@ -75,6 +75,11 @@ static struct test generic_tests[] = {
> >       {
> >               .desc = "PMU events",
> >               .func = test__pmu_events,
> > +             .subtest = {
> > +                     .get_nr         = test__pmu_events_subtest_get_nr,
> > +                     .get_desc       = test__pmu_events_subtest_get_desc,
> > +             },
> > +
> >       },
> >       {
> >               .desc = "DSO data read",
> > diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
> > index d64261da8bf7..c18b9ce8cace 100644
> > --- a/tools/perf/tests/pmu-events.c
> > +++ b/tools/perf/tests/pmu-events.c
> > @@ -8,6 +8,10 @@
> >  #include <linux/zalloc.h>
> >  #include "debug.h"
> >  #include "../pmu-events/pmu-events.h"
> > +#include "util/evlist.h"
> > +#include "util/expr.h"
> > +#include "util/parse-events.h"
> > +#include <ctype.h>
>
> historically we have been using a ctype.h we got from the git tool
> repository, its in:
>
>   tools/include/linux/ctype.h
>
> [acme@five perf]$ find tools/perf/ -name "*.[ch]" | xargs grep '<linux/ctype.h>' | wc -l
> 39
> [acme@five perf]$ find tools/perf/ -name "*.[ch]" | xargs grep '<ctype.h>' | wc -l
> 2
> [acme@five perf]$
>
> [acme@five perf]$ find tools/perf/ -name "*.[ch]" | xargs grep '<ctype.h>'
> tools/perf/pmu-events/jevents.c:#include <ctype.h>
> tools/perf/util/scripting-engines/trace-event-perl.c:#include <ctype.h>
> [acme@five perf]$

Thanks, done.

> >  struct perf_pmu_test_event {
> >       struct pmu_event event;
> > @@ -144,7 +148,7 @@ static struct pmu_events_map *__test_pmu_get_events_map(void)
> >  }
> >
> >  /* Verify generated events from pmu-events.c is as expected */
> > -static int __test_pmu_event_table(void)
> > +static int test_pmu_event_table(void)
> >  {
> >       struct pmu_events_map *map = __test_pmu_get_events_map();
> >       struct pmu_event *table;
> > @@ -347,14 +351,11 @@ static int __test__pmu_event_aliases(char *pmu_name, int *count)
> >       return res;
> >  }
> >
> > -int test__pmu_events(struct test *test __maybe_unused,
> > -                  int subtest __maybe_unused)
> > +
> > +static int test_aliases(void)
> >  {
> >       struct perf_pmu *pmu = NULL;
> >
> > -     if (__test_pmu_event_table())
> > -             return -1;
> > -
> >       while ((pmu = perf_pmu__scan(pmu)) != NULL) {
> >               int count = 0;
> >
> > @@ -377,3 +378,148 @@ int test__pmu_events(struct test *test __maybe_unused,
> >
> >       return 0;
> >  }
> > +
> > +static bool is_number(const char *str)
> > +{
> > +     size_t i;
> > +
> > +     for (i = 0; i < strlen(str); i++) {
> > +             if (!isdigit(str[i]) && str[i] != '.')
> > +                     return false;
> > +     }
> > +     return true;
> > +}
>
> The above can still get some wrong numbers, can't we instead use
> strtold() and check its return value?

Done with strtod to avoid debates on 80-bit vs 128-bit long doubles.
Fwiw, I'd done it this way as conversion from TMA_Metrics.xlsx
sometimes yields a number as .5 rather than 0.5 which causes strtod to
choke. Checking on:
https://download.01.org/perfmon/
The csv isn't doing this (Intel feature request, can we get a tsv as
the comma in a csv is naively matched with commas in min, max and
other expressions) and presumably we can fix any xls conversion
script.

> > +static int check_parse_id(const char *id, bool same_cpu, struct pmu_event *pe)
> > +{
> > +     struct parse_events_error error;
> > +     struct evlist *evlist;
> > +     int ret;
> > +
> > +     /* Numbers are always valid. */
> > +     if (is_number(id))
> > +             return 0;
> > +
> > +     evlist = evlist__new();
> > +     memset(&error, 0, sizeof(error));
> > +     ret = parse_events(evlist, id, &error);
> > +     if (ret && same_cpu) {
> > +             fprintf(stderr,
> > +                     "\nWARNING: Parse event failed metric '%s' id '%s' expr '%s'\n",
> > +                     pe->metric_name, id, pe->metric_expr);
> > +             fprintf(stderr, "Error string '%s' help '%s'\n",
> > +                     error.str, error.help);
>
> Can we pr_warning() above to be consistent with using pr_debug3(), right
> in the else branch?
>
> > +     } else if (ret) {
> > +             pr_debug3("Parse event failed, but for an event that may not be supported by this CPU.\nid '%s' metric '%s' expr '%s'\n",
> > +                     id, pe->metric_name, pe->metric_expr);
> > +     }
> > +     evlist__delete(evlist);
> > +     free(error.str);
> > +     free(error.help);
> > +     free(error.first_str);
> > +     free(error.first_help);
> > +     /* TODO: too many metrics are broken to fail on this test currently. */
> > +     return 0;
>
> I was thinking if we could handle the failure of this specific
> check_parse_id() in its caller differently and then, at the end, use
> some marking like with:
>
> 58: builtin clang support                                 : Skip (not compiled in)
>
> Perhaps:
>
> NN: Parsing of PMU event table metrics                    : Skip (Some metrics failed)
>
> Or some other wording, perhaps a new return value in addition to skip,
> fail, ok. That allows the test to continue but flags it as having issues
> that should be checked with 'perf test -v'

I've added a subtest helper that gives a reason for a skip, similar to
get_desc, this avoids changing all the test function parameters and is
ignored if not initialized.

Thanks,
Ian

> > +}
> > +
> > +static int test_parsing(void)
> > +{
> > +     struct pmu_events_map *cpus_map = perf_pmu__find_map(NULL);
> > +     struct pmu_events_map *map;
> > +     struct pmu_event *pe;
> > +     int i, j, k;
> > +     const char **ids;
> > +     int idnum;
> > +     int ret = 0;
> > +     struct expr_parse_ctx ctx;
> > +     double result;
> > +
> > +     i = 0;
> > +     for (;;) {
> > +             map = &pmu_events_map[i++];
> > +             if (!map->table) {
> > +                     map = NULL;
> > +                     break;
> > +             }
> > +             j = 0;
> > +             for (;;) {
> > +                     pe = &map->table[j++];
> > +                     if (!pe->name && !pe->metric_group && !pe->metric_name)
> > +                             break;
> > +                     if (!pe->metric_expr)
> > +                             continue;
> > +                     if (expr__find_other(pe->metric_expr, NULL,
> > +                                             &ids, &idnum, 0) < 0) {
> > +                             pr_debug("Parse other failed for map %s %s %s\n",
> > +                                     map->cpuid, map->version, map->type);
> > +                             pr_debug("On metric %s\n", pe->metric_name);
> > +                             pr_debug("On expression %s\n", pe->metric_expr);
> > +                             ret++;
> > +                             continue;
> > +                     }
> > +                     expr__ctx_init(&ctx);
> > +
> > +                     /*
> > +                      * Add all ids with a made up value. The value may
> > +                      * trigger divide by zero when subtracted and so try to
> > +                      * make them unique.
> > +                      */
> > +                     for (k = 0; k < idnum; k++)
> > +                             expr__add_id(&ctx, ids[k], k + 1);
> > +
> > +                     for (k = 0; k < idnum; k++) {
> > +                             if (check_parse_id(ids[k], map == cpus_map, pe))
> > +                                     ret++;
> > +                     }
> > +
> > +                     if (expr__parse(&result, &ctx, pe->metric_expr, 0)) {
> > +                             pr_debug("Parse failed for map %s %s %s\n",
> > +                                     map->cpuid, map->version, map->type);
> > +                             pr_debug("On metric %s\n", pe->metric_name);
> > +                             pr_debug("On expression %s\n", pe->metric_expr);
> > +                             ret++;
> > +                     }
> > +                     for (k = 0; k < idnum; k++)
> > +                             zfree(&ids[k]);
> > +                     free(ids);
> > +             }
> > +     }
> > +     return ret;
> > +}
> > +
> > +static const struct {
> > +     int (*func)(void);
> > +     const char *desc;
> > +} pmu_events_testcase_table[] = {
> > +     {
> > +             .func = test_pmu_event_table,
> > +             .desc = "PMU event table sanity",
> > +     },
> > +     {
> > +             .func = test_aliases,
> > +             .desc = "PMU event map aliases",
> > +     },
> > +     {
> > +             .func = test_parsing,
> > +             .desc = "Parsing of PMU event table metrics",
> > +     },
> > +};
> > +
> > +const char *test__pmu_events_subtest_get_desc(int i)
> > +{
> > +     if (i < 0 || i >= (int)ARRAY_SIZE(pmu_events_testcase_table))
> > +             return NULL;
> > +     return pmu_events_testcase_table[i].desc;
> > +}
> > +
> > +int test__pmu_events_subtest_get_nr(void)
> > +{
> > +     return (int)ARRAY_SIZE(pmu_events_testcase_table);
> > +}
> > +
> > +int test__pmu_events(struct test *test __maybe_unused, int i)
> > +{
> > +     if (i < 0 || i >= (int)ARRAY_SIZE(pmu_events_testcase_table))
> > +             return TEST_FAIL;
> > +     return pmu_events_testcase_table[i].func();
> > +}
> > diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
> > index d6d4ac34eeb7..8e316c30ed3c 100644
> > --- a/tools/perf/tests/tests.h
> > +++ b/tools/perf/tests/tests.h
> > @@ -50,6 +50,8 @@ int test__perf_evsel__tp_sched_test(struct test *test, int subtest);
> >  int test__syscall_openat_tp_fields(struct test *test, int subtest);
> >  int test__pmu(struct test *test, int subtest);
> >  int test__pmu_events(struct test *test, int subtest);
> > +const char *test__pmu_events_subtest_get_desc(int subtest);
> > +int test__pmu_events_subtest_get_nr(void);
> >  int test__attr(struct test *test, int subtest);
> >  int test__dso_data(struct test *test, int subtest);
> >  int test__dso_data_cache(struct test *test, int subtest);
> > --
> > 2.26.2.645.ge9eca65c58-goog
> >
>
> --
>
> - Arnaldo