Re: [PATCH v3 6/7] perf test: Improve pmu event metric testing

[Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>]

Em Fri, May 15, 2020 at 03:17:31PM -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,
> skip/warn if metrics for the current architecture fail to parse. To
> support warning for a skip, and an ability for a subtest to describe why
> it skips.
> 
> Tested on power9, skylakex, haswell, broadwell, westmere, sandybridge and
> ivybridge.
> 
> May skip/warn on other architectures if metrics are invalid. In
> particular s390 is untested, but its expressions are trivial. The
> untested architectures with expressions are power8, cascadelakex,
> tremontx, skylake, jaketown, ivytown and variants of haswell and
> broadwell.
> 
> v3. addresses review comments from John Garry <john.garry@huawei.com>,
> Jiri Olsa <jolsa@redhat.com> and Arnaldo Carvalho de Melo
> <acme@kernel.org>.
> v2. changes the commit message as event parsing errors no longer cause
> the test to fail.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Andi Kleen <ak@linux.intel.com>
> Cc: Jin Yao <yao.jin@linux.intel.com>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: John Garry <john.garry@huawei.com>
> Cc: Kajol Jain <kjain@linux.ibm.com>
> Cc: Kan Liang <kan.liang@linux.intel.com>
> Cc: Leo Yan <leo.yan@linaro.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Paul Clarke <pc@us.ibm.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Stephane Eranian <eranian@google.com>
> Link: http://lore.kernel.org/lkml/20200513212933.41273-1-irogers@google.com
> [ split from a larger patch ]
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
>  tools/perf/tests/builtin-test.c |   7 ++
>  tools/perf/tests/pmu-events.c   | 168 ++++++++++++++++++++++++++++++--
>  tools/perf/tests/tests.h        |   3 +
>  3 files changed, 172 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index baee735e6aa5..9553f8061772 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c
> @@ -75,6 +75,13 @@ static struct test generic_tests[] = {
>  	{
>  		.desc = "PMU events",
>  		.func = test__pmu_events,
> +		.subtest = {
> +			.skip_if_fail	= false,
> +			.get_nr		= test__pmu_events_subtest_get_nr,
> +			.get_desc	= test__pmu_events_subtest_get_desc,
> +			.skip_reason	= test__pmu_events_subtest_skip_reason,
> +		},
> +
>  	},
>  	{
>  		.desc = "DSO data read",
> diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
> index d64261da8bf7..e21f0addcfbb 100644
> --- a/tools/perf/tests/pmu-events.c
> +++ b/tools/perf/tests/pmu-events.c
> @@ -8,6 +8,9 @@
>  #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"
>  
>  struct perf_pmu_test_event {
>  	struct pmu_event event;
> @@ -144,7 +147,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 +350,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 +377,159 @@ int test__pmu_events(struct test *test __maybe_unused,
>  
>  	return 0;
>  }
> +
> +static bool is_number(const char *str)
> +{
> +	char *end_ptr;
> +
> +	strtod(str, &end_ptr);
> +	return end_ptr != str;
> +}

So, this breaks in some systems:

cc1: warnings being treated as errors
tests/pmu-events.c: In function 'is_number':
tests/pmu-events.c:385: error: ignoring return value of 'strtod', declared with attribute warn_unused_result
mv: cannot stat `/tmp/build/perf/tests/.pmu-events.o.tmp': No such file or director

So I'm changing it to verify the result of strtod() which is, humm,
interesting, please check:

diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
index 3de59564deb0..6c58c3a89e6b 100644
--- a/tools/perf/tests/pmu-events.c
+++ b/tools/perf/tests/pmu-events.c
@@ -1,4 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0
+#include "math.h"
 #include "parse-events.h"
 #include "pmu.h"
 #include "tests.h"
@@ -381,8 +382,12 @@ static int test_aliases(void)
 static bool is_number(const char *str)
 {
 	char *end_ptr;
+	double v;
 
-	strtod(str, &end_ptr);
+	errno = 0;
+	v = strtod(str, &end_ptr);
+	if ((errno == ERANGE && (v == HUGE_VAL || v == -HUGE_VAL)) || (errno != 0 && v == 0.0))
+		return false;
 	return end_ptr != str;
 }