[PATCH 1/1] perf tests pmu: Initialize all fields of test_pmu variable

[PATCH 1/1] perf tests pmu: Initialize all fields of test_pmu variable

[Arnaldo Carvalho de Melo]

Instead of explicitely initializing just the .name and .alias_name,
use struct member named initialization of just the non-null -name field,
the compiler will initialize all the other non-explicitely initialized
fields to NULL.

This makes the code more robust, avoiding the error recently fixed when
the .alias_name was used and contained a random value.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Ian Rogers <irogers@google.com>
Cc: James Clark <james.clark@arm.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Michael Petlan <mpetlan@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Radostin Stoyanov <rstoyano@redhat.com>
Cc: Veronika Molnarova <vmolnaro@redhat.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/tests/pmu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/perf/tests/pmu.c b/tools/perf/tests/pmu.c
index a4730b5dc0d9259d..be18506f6a242546 100644
--- a/tools/perf/tests/pmu.c
+++ b/tools/perf/tests/pmu.c
@@ -458,10 +458,10 @@ static int test__name_cmp(struct test_suite *test __maybe_unused, int subtest __
  */
 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;
+	struct perf_pmu test_pmu = {
+		.name = "pmuname",
+	};
 
-	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);
-- 
2.45.2

Re: [PATCH 1/1] perf tests pmu: Initialize all fields of test_pmu variable

[Veronika Molnarova]

On 8/12/24 15:03, Arnaldo Carvalho de Melo wrote:
> Instead of explicitely initializing just the .name and .alias_name,
> use struct member named initialization of just the non-null -name field,
> the compiler will initialize all the other non-explicitely initialized
> fields to NULL.
> 
> This makes the code more robust, avoiding the error recently fixed when
> the .alias_name was used and contained a random value.
> 
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Ian Rogers <irogers@google.com>
> Cc: James Clark <james.clark@arm.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Kan Liang <kan.liang@linux.intel.com>
> Cc: Michael Petlan <mpetlan@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Radostin Stoyanov <rstoyano@redhat.com>
> Cc: Veronika Molnarova <vmolnaro@redhat.com>
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
>  tools/perf/tests/pmu.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/tests/pmu.c b/tools/perf/tests/pmu.c
> index a4730b5dc0d9259d..be18506f6a242546 100644
> --- a/tools/perf/tests/pmu.c
> +++ b/tools/perf/tests/pmu.c
> @@ -458,10 +458,10 @@ static int test__name_cmp(struct test_suite *test __maybe_unused, int subtest __
>   */
>  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;
> +	struct perf_pmu test_pmu = {
> +		.name = "pmuname",
> +	};
>  
> -	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);

Reviewed-by: Veronika Molnarova <vmolnaro@redhat.com>

Thanks for the rework,
Veronika

Re: [PATCH 1/1] perf tests pmu: Initialize all fields of test_pmu variable

[Ian Rogers]

On Mon, Aug 12, 2024 at 7:05 AM Veronika Molnarova <vmolnaro@redhat.com> wrote:
>
>
>
> On 8/12/24 15:03, Arnaldo Carvalho de Melo wrote:
> > Instead of explicitely initializing just the .name and .alias_name,
> > use struct member named initialization of just the non-null -name field,
> > the compiler will initialize all the other non-explicitely initialized
> > fields to NULL.
> >
> > This makes the code more robust, avoiding the error recently fixed when
> > the .alias_name was used and contained a random value.
> >
> > Cc: Adrian Hunter <adrian.hunter@intel.com>
> > Cc: Ian Rogers <irogers@google.com>
> > Cc: James Clark <james.clark@arm.com>
> > Cc: Jiri Olsa <jolsa@kernel.org>
> > Cc: Kan Liang <kan.liang@linux.intel.com>
> > Cc: Michael Petlan <mpetlan@redhat.com>
> > Cc: Namhyung Kim <namhyung@kernel.org>
> > Cc: Radostin Stoyanov <rstoyano@redhat.com>
> > Cc: Veronika Molnarova <vmolnaro@redhat.com>
> > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> > ---
> >  tools/perf/tests/pmu.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/perf/tests/pmu.c b/tools/perf/tests/pmu.c
> > index a4730b5dc0d9259d..be18506f6a242546 100644
> > --- a/tools/perf/tests/pmu.c
> > +++ b/tools/perf/tests/pmu.c
> > @@ -458,10 +458,10 @@ static int test__name_cmp(struct test_suite *test __maybe_unused, int subtest __
> >   */
> >  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;
> > +     struct perf_pmu test_pmu = {
> > +             .name = "pmuname",
> > +     };
> >
> > -     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);
>
> Reviewed-by: Veronika Molnarova <vmolnaro@redhat.com>
>
> Thanks for the rework,
> Veronika

This seems like a simple enough fix for a test that it could be
cherry-picked into perf-tools for v6.11, I'm not seeing it currently:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools.git/log/tools/perf/tests/pmu.c?h=perf-tools

Thanks,
Ian

Re: [PATCH 1/1] perf tests pmu: Initialize all fields of test_pmu variable

[Arnaldo Carvalho de Melo]

On Thu, Aug 29, 2024 at 01:17:13PM -0700, Ian Rogers wrote:
> On Mon, Aug 12, 2024 at 7:05 AM Veronika Molnarova <vmolnaro@redhat.com> wrote:
> > On 8/12/24 15:03, Arnaldo Carvalho de Melo wrote:
> > > This makes the code more robust, avoiding the error recently fixed when
> > > the .alias_name was used and contained a random value.

> > > +++ b/tools/perf/tests/pmu.c
> > > @@ -458,10 +458,10 @@ static int test__name_cmp(struct test_suite *test __maybe_unused, int subtest __
> > >  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;
> > > +     struct perf_pmu test_pmu = {
> > > +             .name = "pmuname",
> > > +     };

> > > -     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);

> > Reviewed-by: Veronika Molnarova <vmolnaro@redhat.com>
 
> This seems like a simple enough fix for a test that it could be
> cherry-picked into perf-tools for v6.11, I'm not seeing it currently:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools.git/log/tools/perf/tests/pmu.c?h=perf-tools

This is not a fix, its just to make the code more future proof by
initializing all non explicitely initialized fields to zeros.

Veronika's fix, that this improves upon, is enough for the problems
detected so far.

Or are you noticing some other bug that gets fixed by my patch?

Ok, now I noticed that Veronika's fix:

	https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools.git/commit/?h=perf-tools&id=37e2a19c98bf99747ca997be876dfc13f9165e0a

is marked with:

  perf test pmu: Set uninitialized PMU alias to null
    Notice: this object is not reachable from any branch.

Being only in perf-tools-next:

  https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/commit/?h=perf-tools-next&id=37e2a19c98bf99747ca997be876dfc13f9165e0a

So yeah, probably Namhyung can cherry-pick that patch (Veronika's) into
perf-tools for v6.11.

There were a few more fixes that I noticed and picked for
perf-tools-next that then people reported that should also be
cherry-picked for v6.11, Namhyung?

- Arnaldo

Re: [PATCH 1/1] perf tests pmu: Initialize all fields of test_pmu variable

[Namhyung Kim]

Hi,

On Thu, Aug 29, 2024 at 2:09 PM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> On Thu, Aug 29, 2024 at 01:17:13PM -0700, Ian Rogers wrote:
> > On Mon, Aug 12, 2024 at 7:05 AM Veronika Molnarova <vmolnaro@redhat.com> wrote:
> > > On 8/12/24 15:03, Arnaldo Carvalho de Melo wrote:
> > > > This makes the code more robust, avoiding the error recently fixed when
> > > > the .alias_name was used and contained a random value.
>
> > > > +++ b/tools/perf/tests/pmu.c
> > > > @@ -458,10 +458,10 @@ static int test__name_cmp(struct test_suite *test __maybe_unused, int subtest __
> > > >  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;
> > > > +     struct perf_pmu test_pmu = {
> > > > +             .name = "pmuname",
> > > > +     };
>
> > > > -     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);
>
> > > Reviewed-by: Veronika Molnarova <vmolnaro@redhat.com>
>
> > This seems like a simple enough fix for a test that it could be
> > cherry-picked into perf-tools for v6.11, I'm not seeing it currently:
> > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools.git/log/tools/perf/tests/pmu.c?h=perf-tools
>
> This is not a fix, its just to make the code more future proof by
> initializing all non explicitely initialized fields to zeros.
>
> Veronika's fix, that this improves upon, is enough for the problems
> detected so far.
>
> Or are you noticing some other bug that gets fixed by my patch?
>
> Ok, now I noticed that Veronika's fix:
>
>         https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools.git/commit/?h=perf-tools&id=37e2a19c98bf99747ca997be876dfc13f9165e0a
>
> is marked with:
>
>   perf test pmu: Set uninitialized PMU alias to null
>     Notice: this object is not reachable from any branch.
>
> Being only in perf-tools-next:
>
>   https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/commit/?h=perf-tools-next&id=37e2a19c98bf99747ca997be876dfc13f9165e0a
>
> So yeah, probably Namhyung can cherry-pick that patch (Veronika's) into
> perf-tools for v6.11.
>
> There were a few more fixes that I noticed and picked for
> perf-tools-next that then people reported that should also be
> cherry-picked for v6.11, Namhyung?

Ok, I can pick this up.  I think my perf lock contention fix also can
go to perf-tools.
What others do you want me to pick up?

Thanks,
Namhyung

Re: [PATCH 1/1] perf tests pmu: Initialize all fields of test_pmu variable

[Arnaldo Carvalho de Melo]

On Thu, Aug 29, 2024 at 08:49:05PM -0700, Namhyung Kim wrote:
> On Thu, Aug 29, 2024 at 2:09 PM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > Being only in perf-tools-next:

> >   https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/commit/?h=perf-tools-next&id=37e2a19c98bf99747ca997be876dfc13f9165e0a

> > So yeah, probably Namhyung can cherry-pick that patch (Veronika's) into
> > perf-tools for v6.11.

> > There were a few more fixes that I noticed and picked for
> > perf-tools-next that then people reported that should also be
> > cherry-picked for v6.11, Namhyung?

> Ok, I can pick this up.  I think my perf lock contention fix also can
> go to perf-tools.
> What others do you want me to pick up?

Here Ben points to one:

https://lore.kernel.org/all/d94d995e476a5c014e1ce4d75d36f8667acd3316.camel@decadent.org.uk/T/#u

Which I think its this one:

commit 2518e13275ab9ea6b2540f828cf78b0280991f85
Author: Xu Yang <xu.yang_2@nxp.com>
Date:   Mon Aug 19 10:34:03 2024 +0800

    perf python: Fix the build on 32-bit arm by including missing "util/sample.h"

There is also this one:

commit 6236ebe07131a7746d870f1d8eb3637a8df13e70
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
Date:   Mon Aug 19 16:46:29 2024 -0300

    perf daemon: Fix the build on more 32-bit architectures
    
    The previous attempt fixed the build on debian:experimental-x-mipsel,
    but when building on a larger set of containers I noticed it broke the
    build on some other 32-bit architectures such as:
    
      42     7.87 ubuntu:18.04-x-arm            : FAIL gcc version 7.5.0 (Ubuntu/Linaro 7.5.0-3ubuntu1~18.04)
        builtin-daemon.c: In function 'cmd_session_list':
        builtin-daemon.c:692:16: error: format '%llu' expects argument of type 'long long unsigned int', but argument 4 has type 'long int' [-Werror=format=]

- Arnaldo