Re: [PATCH] perf test pmu: Set uninitialized PMU alias to null

[Arnaldo Carvalho de Melo <acme@kernel.org>]

On Thu, Aug 08, 2024 at 11:01:28AM -0300, Arnaldo Carvalho de Melo wrote:
> On Thu, Aug 08, 2024 at 12:37:49PM +0200, vmolnaro@redhat.com wrote:
> > From: Veronika Molnarova <vmolnaro@redhat.com>
> > 
> > Commit 3e0bf9 ("perf pmu: Restore full PMU name wildcard support") adds
> > a test case "PMU cmdline match" that covers PMU name wildcard support
> > provided by function perf_pmu__match(). The test works with a wide
> > range of supported combinations of PMU name matching but omits the case
> > that if the perf_pmu__match() cannot match the PMU name to the wildcard,
> > it tries to match its alias. However, this variable is not set up,
> > causing the test case to fail when run with subprocesses or to segfault
> > if run as a single process.
> > 
> > ./perf test -vv 9
> >   9: Sysfs PMU tests                                                 :
> >   9.1: Parsing with PMU format directory                             : Ok
> >   9.2: Parsing with PMU event                                        : Ok
> >   9.3: PMU event names                                               : Ok
> >   9.4: PMU name combining                                            : Ok
> >   9.5: PMU name comparison                                           : Ok
> >   9.6: PMU cmdline match                                             : FAILED!
> > 
> > ./perf test -F 9
> >   9.1: Parsing with PMU format directory                             : Ok
> >   9.2: Parsing with PMU event                                        : Ok
> >   9.3: PMU event names                                               : Ok
> >   9.4: PMU name combining                                            : Ok
> >   9.5: PMU name comparison                                           : Ok
> > Segmentation fault (core dumped)
> > 
> > Initialize the PMU alias to null for all tests of perf_pmu__match()
> > as this functionality is not being tested and the alias matching works
> > exactly the same as the matching of the PMU name.
> > 
> > ./perf test -F 9
> >   9.1: Parsing with PMU format directory                             : Ok
> >   9.2: Parsing with PMU event                                        : Ok
> >   9.3: PMU event names                                               : Ok
> >   9.4: PMU name combining                                            : Ok
> >   9.5: PMU name comparison                                           : Ok
> >   9.6: PMU cmdline match                                             : Ok
> > 
> > Fixes: 3e0bf9 ("perf pmu: Restore full PMU name wildcard support")
> > Signed-off-by: Veronika Molnarova <vmolnaro@redhat.com>
> > ---
> >  tools/perf/tests/pmu.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/perf/tests/pmu.c b/tools/perf/tests/pmu.c
> > index 0b2f04a55d7b..a4730b5dc0d9 100644
> > --- a/tools/perf/tests/pmu.c
> > +++ b/tools/perf/tests/pmu.c
> > @@ -453,11 +453,13 @@ static int test__name_cmp(struct test_suite *test __maybe_unused, int subtest __
> >  /**
> >   * Test perf_pmu__match() that's used to search for a PMU given a name passed
> >   * on the command line. The name that's passed may also be a filename type glob
> > - * match.
> > + * match. If the name does not match, perf_pmu__match() attempts to match the
> > + * alias of the PMU, if provided.
> >   */
> >  static int test__pmu_match(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
> >  {
> >  	struct perf_pmu test_pmu;
> > +	test_pmu.alias_name = NULL;
> 
> We can do a bit more future proofing by instead doing:
> 
> 	struct perf_pmu test_pmu = {
> 		.name = "pmuname",
> 	};
> 
> So that all the other fields are initialized to zero, ok? I'll do this
> change and make a note in the commit, lemme know if you disagree.

Nah, tried with:

diff --git a/tools/perf/tests/pmu.c b/tools/perf/tests/pmu.c
index a4730b5dc0d9259d..c9d4ed6d684551c1 100644
--- a/tools/perf/tests/pmu.c
+++ b/tools/perf/tests/pmu.c
@@ -457,11 +457,11 @@ static int test__name_cmp(struct test_suite *test __maybe_unused, int subtest __
  * alias of the PMU, if provided.
  */
 static int test__pmu_match(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
-{
-	struct perf_pmu test_pmu;
-	test_pmu.alias_name = NULL;
 
-	test_pmu.name = "pmuname";
+	struct perf_pmu test_pmu = {
+		.name = "pmuname",
+	};
+
 	TEST_ASSERT_EQUAL("Exact match", perf_pmu__match(&test_pmu, "pmuname"),	     true);
 	TEST_ASSERT_EQUAL("Longer token", perf_pmu__match(&test_pmu, "longertoken"), false);
 	TEST_ASSERT_EQUAL("Shorter token", perf_pmu__match(&test_pmu, "pmu"),	     false);

But those macros are not liking it:

tests/pmu.c: In function ‘test__pmu_match’:
tests/pmu.c:461:16: error: parameter ‘test_pmu’ is initialized
  461 |         struct perf_pmu test_pmu = {
      |                ^~~~~~~~
In file included from tests/pmu.c:7:
tests/tests.h:22:1: error: expected declaration specifiers before ‘do’
   22 | do {                                                                     \
      | ^~
tests/pmu.c:465:9: note: in expansion of macro ‘TEST_ASSERT_EQUAL’
  465 |         TEST_ASSERT_EQUAL("Exact match", perf_pmu__match(&test_pmu, "pmuname"),      true);
      |         ^~~~~~~~~~~~~~~~~
tests/tests.h:28:3: error: expected declaration specifiers before ‘while’
   28 | } while (0)
      |   ^~~~~

So I'll not spend more time here, I'm keeping your patch as-is.

Thanks and sorry for the noise :-)

- Arnaldo