[PATCH v1 1/2] perf list: Fix json segfault

[PATCH v1 1/2] perf list: Fix json segfault

[Ian Rogers]

Json output didn't set the skip_duplicate_pmus callback yielding a
segfault.

Fixes: cd4e1efbbc40 ("perf pmus: Skip duplicate PMUs and don't print list suffix by default")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/builtin-list.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
index a343823c8ddf..61c2c96cc070 100644
--- a/tools/perf/builtin-list.c
+++ b/tools/perf/builtin-list.c
@@ -434,6 +434,11 @@ static void json_print_metric(void *ps __maybe_unused, const char *group,
 	strbuf_release(&buf);
 }
 
+static bool json_skip_duplicate_pmus(void *ps __maybe_unused)
+{
+	return false;
+}
+
 static bool default_skip_duplicate_pmus(void *ps)
 {
 	struct print_state *print_state = ps;
@@ -503,6 +508,7 @@ int cmd_list(int argc, const char **argv)
 			.print_end = json_print_end,
 			.print_event = json_print_event,
 			.print_metric = json_print_metric,
+			.skip_duplicate_pmus = json_skip_duplicate_pmus,
 		};
 		ps = &json_ps;
 	} else {
-- 
2.43.0.rc1.413.gea7ed67945-goog

[PATCH v1 2/2] perf test: Add basic list test

[Ian Rogers]

Test that json output produces valid json.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/shell/list.sh | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)
 create mode 100755 tools/perf/tests/shell/list.sh

diff --git a/tools/perf/tests/shell/list.sh b/tools/perf/tests/shell/list.sh
new file mode 100755
index 000000000000..286879a9837a
--- /dev/null
+++ b/tools/perf/tests/shell/list.sh
@@ -0,0 +1,29 @@
+#!/bin/sh
+# perf list tests
+# SPDX-License-Identifier: GPL-2.0
+
+set -e
+err=0
+
+if [ "x$PYTHON" == "x" ]
+then
+	if which python3 > /dev/null
+	then
+		PYTHON=python3
+	elif which python > /dev/null
+	then
+		PYTHON=python
+	else
+		echo Skipping test, python not detected please set environment variable PYTHON.
+		exit 2
+	fi
+fi
+
+test_list_json() {
+  echo "Json output test"
+  perf list -j | $PYTHON -m json.tool
+  echo "Json output test [Success]"
+}
+
+test_list_json
+exit $err
-- 
2.43.0.rc1.413.gea7ed67945-goog

Re: [PATCH v1 2/2] perf test: Add basic list test

[Adrian Hunter]

On 29/11/23 10:10, Ian Rogers wrote:
> Test that json output produces valid json.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/tests/shell/list.sh | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>  create mode 100755 tools/perf/tests/shell/list.sh
> 
> diff --git a/tools/perf/tests/shell/list.sh b/tools/perf/tests/shell/list.sh
> new file mode 100755
> index 000000000000..286879a9837a
> --- /dev/null
> +++ b/tools/perf/tests/shell/list.sh
> @@ -0,0 +1,29 @@
> +#!/bin/sh
> +# perf list tests
> +# SPDX-License-Identifier: GPL-2.0
> +
> +set -e
> +err=0
> +
> +if [ "x$PYTHON" == "x" ]
> +then
> +	if which python3 > /dev/null

'which' isn't always present.  Maybe

python3 --version >/dev/null 2>&1 && PYTHON=python3

> +	then
> +		PYTHON=python3
> +	elif which python > /dev/null
> +	then
> +		PYTHON=python
> +	else
> +		echo Skipping test, python not detected please set environment variable PYTHON.
> +		exit 2
> +	fi
> +fi
> +
> +test_list_json() {
> +  echo "Json output test"
> +  perf list -j | $PYTHON -m json.tool
> +  echo "Json output test [Success]"
> +}
> +
> +test_list_json
> +exit $err

Re: [PATCH v1 2/2] perf test: Add basic list test

[James Clark]

On 29/11/2023 09:00, Adrian Hunter wrote:
> On 29/11/23 10:10, Ian Rogers wrote:
>> Test that json output produces valid json.
>>
>> Signed-off-by: Ian Rogers <irogers@google.com>
>> ---
>>  tools/perf/tests/shell/list.sh | 29 +++++++++++++++++++++++++++++
>>  1 file changed, 29 insertions(+)
>>  create mode 100755 tools/perf/tests/shell/list.sh
>>
>> diff --git a/tools/perf/tests/shell/list.sh b/tools/perf/tests/shell/list.sh
>> new file mode 100755
>> index 000000000000..286879a9837a
>> --- /dev/null
>> +++ b/tools/perf/tests/shell/list.sh
>> @@ -0,0 +1,29 @@
>> +#!/bin/sh
>> +# perf list tests
>> +# SPDX-License-Identifier: GPL-2.0
>> +
>> +set -e
>> +err=0
>> +
>> +if [ "x$PYTHON" == "x" ]
>> +then
>> +	if which python3 > /dev/null
> 
> 'which' isn't always present.  Maybe
> 
> python3 --version >/dev/null 2>&1 && PYTHON=python3
> 

Now that we have shellcheck integrated into the build, we could enable
the POSIX mode test which would warn against this usage of which and
suggest the alternative.

At the moment though there are several other usages of which already in
the tests. And probably enabling POSIX mode would come with hundreds of
other warnings to fix.

I'm not saying we shouldn't change this instance though, just adding the
info for the discussion.

>> +	then
>> +		PYTHON=python3
>> +	elif which python > /dev/null
>> +	then
>> +		PYTHON=python
>> +	else
>> +		echo Skipping test, python not detected please set environment variable PYTHON.
>> +		exit 2
>> +	fi
>> +fi
>> +
>> +test_list_json() {
>> +  echo "Json output test"
>> +  perf list -j | $PYTHON -m json.tool
>> +  echo "Json output test [Success]"
>> +}
>> +
>> +test_list_json
>> +exit $err
> 
> 

Re: [PATCH v1 2/2] perf test: Add basic list test

[Ian Rogers]

On Wed, Nov 29, 2023 at 1:27 AM James Clark <james.clark@arm.com> wrote:
>
>
>
> On 29/11/2023 09:00, Adrian Hunter wrote:
> > On 29/11/23 10:10, Ian Rogers wrote:
> >> Test that json output produces valid json.
> >>
> >> Signed-off-by: Ian Rogers <irogers@google.com>
> >> ---
> >>  tools/perf/tests/shell/list.sh | 29 +++++++++++++++++++++++++++++
> >>  1 file changed, 29 insertions(+)
> >>  create mode 100755 tools/perf/tests/shell/list.sh
> >>
> >> diff --git a/tools/perf/tests/shell/list.sh b/tools/perf/tests/shell/list.sh
> >> new file mode 100755
> >> index 000000000000..286879a9837a
> >> --- /dev/null
> >> +++ b/tools/perf/tests/shell/list.sh
> >> @@ -0,0 +1,29 @@
> >> +#!/bin/sh
> >> +# perf list tests
> >> +# SPDX-License-Identifier: GPL-2.0
> >> +
> >> +set -e
> >> +err=0
> >> +
> >> +if [ "x$PYTHON" == "x" ]
> >> +then
> >> +    if which python3 > /dev/null
> >
> > 'which' isn't always present.  Maybe
> >
> > python3 --version >/dev/null 2>&1 && PYTHON=python3
> >
>
> Now that we have shellcheck integrated into the build, we could enable
> the POSIX mode test which would warn against this usage of which and
> suggest the alternative.
>
> At the moment though there are several other usages of which already in
> the tests. And probably enabling POSIX mode would come with hundreds of
> other warnings to fix.
>
> I'm not saying we shouldn't change this instance though, just adding the
> info for the discussion.

Sounds good to me. Fwiw, the instance where I lifted this code was:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/tests/shell/stat+json_output.sh?h=perf-tools-next#n12

With this change:
```
diff --git a/tools/perf/tests/Makefile.tests b/tools/perf/tests/Makefile.tests
index fdaca5f7a946..06de6d3f4842 100644
--- a/tools/perf/tests/Makefile.tests
+++ b/tools/perf/tests/Makefile.tests
@@ -1,7 +1,7 @@
# SPDX-License-Identifier: GPL-2.0
# Athira Rajeev <atrajeev@linux.vnet.ibm.com>, 2023

-PROGS := $(shell find tests/shell -perm -o=x -type f -name '*.sh')
+PROGS := $(shell find tests/shell -executable -type f -name '*.sh')
FILE_NAME := $(notdir $(PROGS))
FILE_NAME := $(FILE_NAME:%=.%)
LOGS := $(join $(dir $(PROGS)),$(FILE_NAME))
```

shellcheck now runs for me. I'll try adding the posix check into the
patch series, as well as fixing other instances I can see.

Thanks,
Ian



> >> +    then
> >> +            PYTHON=python3
> >> +    elif which python > /dev/null
> >> +    then
> >> +            PYTHON=python
> >> +    else
> >> +            echo Skipping test, python not detected please set environment variable PYTHON.
> >> +            exit 2
> >> +    fi
> >> +fi
> >> +
> >> +test_list_json() {
> >> +  echo "Json output test"
> >> +  perf list -j | $PYTHON -m json.tool
> >> +  echo "Json output test [Success]"
> >> +}
> >> +
> >> +test_list_json
> >> +exit $err
> >
> >

Re: [PATCH v1 2/2] perf test: Add basic list test

[Arnaldo Carvalho de Melo]

Em Wed, Nov 29, 2023 at 09:21:12AM -0800, Ian Rogers escreveu:
> On Wed, Nov 29, 2023 at 1:27 AM James Clark <james.clark@arm.com> wrote:
> >
> >
> >
> > On 29/11/2023 09:00, Adrian Hunter wrote:
> > > On 29/11/23 10:10, Ian Rogers wrote:
> > >> Test that json output produces valid json.
> > >>
> > >> Signed-off-by: Ian Rogers <irogers@google.com>
> > >> ---
> > >>  tools/perf/tests/shell/list.sh | 29 +++++++++++++++++++++++++++++
> > >>  1 file changed, 29 insertions(+)
> > >>  create mode 100755 tools/perf/tests/shell/list.sh
> > >>
> > >> diff --git a/tools/perf/tests/shell/list.sh b/tools/perf/tests/shell/list.sh
> > >> new file mode 100755
> > >> index 000000000000..286879a9837a
> > >> --- /dev/null
> > >> +++ b/tools/perf/tests/shell/list.sh
> > >> @@ -0,0 +1,29 @@
> > >> +#!/bin/sh
> > >> +# perf list tests
> > >> +# SPDX-License-Identifier: GPL-2.0
> > >> +
> > >> +set -e
> > >> +err=0
> > >> +
> > >> +if [ "x$PYTHON" == "x" ]
> > >> +then
> > >> +    if which python3 > /dev/null
> > >
> > > 'which' isn't always present.  Maybe
> > >
> > > python3 --version >/dev/null 2>&1 && PYTHON=python3
> > >
> >
> > Now that we have shellcheck integrated into the build, we could enable
> > the POSIX mode test which would warn against this usage of which and
> > suggest the alternative.
> >
> > At the moment though there are several other usages of which already in
> > the tests. And probably enabling POSIX mode would come with hundreds of
> > other warnings to fix.
> >
> > I'm not saying we shouldn't change this instance though, just adding the
> > info for the discussion.
> 
> Sounds good to me. Fwiw, the instance where I lifted this code was:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/tests/shell/stat+json_output.sh?h=perf-tools-next#n12
> 
> With this change:
> ```
> diff --git a/tools/perf/tests/Makefile.tests b/tools/perf/tests/Makefile.tests
> index fdaca5f7a946..06de6d3f4842 100644
> --- a/tools/perf/tests/Makefile.tests
> +++ b/tools/perf/tests/Makefile.tests
> @@ -1,7 +1,7 @@
> # SPDX-License-Identifier: GPL-2.0
> # Athira Rajeev <atrajeev@linux.vnet.ibm.com>, 2023
> 
> -PROGS := $(shell find tests/shell -perm -o=x -type f -name '*.sh')
> +PROGS := $(shell find tests/shell -executable -type f -name '*.sh')
> FILE_NAME := $(notdir $(PROGS))
> FILE_NAME := $(FILE_NAME:%=.%)
> LOGS := $(join $(dir $(PROGS)),$(FILE_NAME))
> ```
> 
> shellcheck now runs for me. I'll try adding the posix check into the
> patch series, as well as fixing other instances I can see.

So I'll wait for a v2 for this one, ok?

- Arnaldo

Re: [PATCH v1 2/2] perf test: Add basic list test

[Ian Rogers]

On Wed, Nov 29, 2023 at 12:30 PM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Wed, Nov 29, 2023 at 09:21:12AM -0800, Ian Rogers escreveu:
> > On Wed, Nov 29, 2023 at 1:27 AM James Clark <james.clark@arm.com> wrote:
> > >
> > >
> > >
> > > On 29/11/2023 09:00, Adrian Hunter wrote:
> > > > On 29/11/23 10:10, Ian Rogers wrote:
> > > >> Test that json output produces valid json.
> > > >>
> > > >> Signed-off-by: Ian Rogers <irogers@google.com>
> > > >> ---
> > > >>  tools/perf/tests/shell/list.sh | 29 +++++++++++++++++++++++++++++
> > > >>  1 file changed, 29 insertions(+)
> > > >>  create mode 100755 tools/perf/tests/shell/list.sh
> > > >>
> > > >> diff --git a/tools/perf/tests/shell/list.sh b/tools/perf/tests/shell/list.sh
> > > >> new file mode 100755
> > > >> index 000000000000..286879a9837a
> > > >> --- /dev/null
> > > >> +++ b/tools/perf/tests/shell/list.sh
> > > >> @@ -0,0 +1,29 @@
> > > >> +#!/bin/sh
> > > >> +# perf list tests
> > > >> +# SPDX-License-Identifier: GPL-2.0
> > > >> +
> > > >> +set -e
> > > >> +err=0
> > > >> +
> > > >> +if [ "x$PYTHON" == "x" ]
> > > >> +then
> > > >> +    if which python3 > /dev/null
> > > >
> > > > 'which' isn't always present.  Maybe
> > > >
> > > > python3 --version >/dev/null 2>&1 && PYTHON=python3
> > > >
> > >
> > > Now that we have shellcheck integrated into the build, we could enable
> > > the POSIX mode test which would warn against this usage of which and
> > > suggest the alternative.
> > >
> > > At the moment though there are several other usages of which already in
> > > the tests. And probably enabling POSIX mode would come with hundreds of
> > > other warnings to fix.
> > >
> > > I'm not saying we shouldn't change this instance though, just adding the
> > > info for the discussion.
> >
> > Sounds good to me. Fwiw, the instance where I lifted this code was:
> > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/tests/shell/stat+json_output.sh?h=perf-tools-next#n12
> >
> > With this change:
> > ```
> > diff --git a/tools/perf/tests/Makefile.tests b/tools/perf/tests/Makefile.tests
> > index fdaca5f7a946..06de6d3f4842 100644
> > --- a/tools/perf/tests/Makefile.tests
> > +++ b/tools/perf/tests/Makefile.tests
> > @@ -1,7 +1,7 @@
> > # SPDX-License-Identifier: GPL-2.0
> > # Athira Rajeev <atrajeev@linux.vnet.ibm.com>, 2023
> >
> > -PROGS := $(shell find tests/shell -perm -o=x -type f -name '*.sh')
> > +PROGS := $(shell find tests/shell -executable -type f -name '*.sh')
> > FILE_NAME := $(notdir $(PROGS))
> > FILE_NAME := $(FILE_NAME:%=.%)
> > LOGS := $(join $(dir $(PROGS)),$(FILE_NAME))
> > ```
> >
> > shellcheck now runs for me. I'll try adding the posix check into the
> > patch series, as well as fixing other instances I can see.
>
> So I'll wait for a v2 for this one, ok?

Yep, sent:
https://lore.kernel.org/lkml/20231129213428.2227448-1-irogers@google.com/

There are 2 fixes, one for perf list and the other for the shellcheck
log file building stuff. The shellcheck stuff took a little longer
PTAL.

Thanks,
Ian

> - Arnaldo