* [PATCH 1/3] perf test: Check test suite description only
@ 2025-06-30 23:32 Namhyung Kim
2025-06-30 23:32 ` [PATCH 2/3] perf test: Add libsubcmd help tests Namhyung Kim
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Namhyung Kim @ 2025-06-30 23:32 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Ian Rogers, Kan Liang
Cc: Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
linux-perf-users
Currently perf test checks the given string with descriptions for both
test suites and cases (subtests). But sometimes it's confusing since
the subtests don't contain the important keyword.
I think it's better to check the suite level and run the whole suite
together.
Before:
$ perf test hwmon
(no output)
After:
$ perf test hwmon
10: Hwmon PMU :
10.1: Basic parsing test : Ok
10.2: Parsing without PMU name : Ok
10.3: Parsing with PMU name : Ok
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
tools/perf/tests/builtin-test.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index 80375ca39a37a256..dfaff4185eb05a1a 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -567,10 +567,6 @@ static int __cmd_test(struct test_suite **suites, int argc, const char *argv[],
for (unsigned int run = 0; run < runs_per_test; run++) {
test_suite__for_each_test_case(*t, curr_test_case) {
- if (!perf_test__matches(test_description(*t, curr_test_case),
- curr_suite, argc, argv))
- continue;
-
err = start_test(*t, curr_suite, curr_test_case,
&child_tests[child_test_num++],
width, pass);
--
2.50.0.727.gbf7dc18ff4-goog
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 2/3] perf test: Add libsubcmd help tests 2025-06-30 23:32 [PATCH 1/3] perf test: Check test suite description only Namhyung Kim @ 2025-06-30 23:32 ` Namhyung Kim 2025-07-01 15:54 ` Ian Rogers 2025-06-30 23:32 ` [PATCH 3/3] perf tools: Fix use-after-free in help_unknown_cmd() Namhyung Kim 2025-07-01 15:51 ` [PATCH 1/3] perf test: Check test suite description only Ian Rogers 2 siblings, 1 reply; 9+ messages in thread From: Namhyung Kim @ 2025-06-30 23:32 UTC (permalink / raw) To: Arnaldo Carvalho de Melo, Ian Rogers, Kan Liang Cc: Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users Add a set of tests for subcmd routines. Currently it fails the last one since there's a bug. It'll be fixed by the next commit. $ perf test subcmd 69: libsubcmd help tests : 69.1: Load subcmd names : Ok 69.2: Uniquify subcmd names : Ok 69.3: Exclude duplicate subcmd names : FAILED! Signed-off-by: Namhyung Kim <namhyung@kernel.org> --- tools/perf/tests/Build | 1 + tools/perf/tests/builtin-test.c | 1 + tools/perf/tests/subcmd-help.c | 109 ++++++++++++++++++++++++++++++++ tools/perf/tests/tests.h | 1 + 4 files changed, 112 insertions(+) create mode 100644 tools/perf/tests/subcmd-help.c diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build index 2181f5a92148b0b9..13a81154ec1e4cd2 100644 --- a/tools/perf/tests/Build +++ b/tools/perf/tests/Build @@ -69,6 +69,7 @@ perf-test-y += symbols.o perf-test-y += util.o perf-test-y += hwmon_pmu.o perf-test-y += tool_pmu.o +perf-test-y += subcmd-help.o ifeq ($(SRCARCH),$(filter $(SRCARCH),x86 arm arm64 powerpc)) perf-test-$(CONFIG_DWARF_UNWIND) += dwarf-unwind.o diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c index dfaff4185eb05a1a..2da9b69864da53c2 100644 --- a/tools/perf/tests/builtin-test.c +++ b/tools/perf/tests/builtin-test.c @@ -139,6 +139,7 @@ static struct test_suite *generic_tests[] = { &suite__event_groups, &suite__symbols, &suite__util, + &suite__subcmd_help, NULL, }; diff --git a/tools/perf/tests/subcmd-help.c b/tools/perf/tests/subcmd-help.c new file mode 100644 index 0000000000000000..d31259340ae302af --- /dev/null +++ b/tools/perf/tests/subcmd-help.c @@ -0,0 +1,109 @@ +// SPDX-License-Identifier: GPL-2.0 +#include "tests.h" +#include <stdio.h> +#include <subcmd/help.h> +#include "debug.h" + +static int test__load_cmdnames(struct test_suite *test __maybe_unused, + int subtest __maybe_unused) +{ + struct cmdnames cmds = {}; + + add_cmdname(&cmds, "aaa", 3); + add_cmdname(&cmds, "foo", 3); + add_cmdname(&cmds, "xyz", 3); + + TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds, "aaa") == 1); + TEST_ASSERT_VAL("wrong cmd", is_in_cmdlist(&cmds, "bar") == 0); + TEST_ASSERT_VAL("case sensitive", is_in_cmdlist(&cmds, "XYZ") == 0); + + clean_cmdnames(&cmds); + return TEST_OK; +} + +static int test__uniq_cmdnames(struct test_suite *test __maybe_unused, + int subtest __maybe_unused) +{ + struct cmdnames cmds = {}; + + /* uniq() assumes it's sorted */ + add_cmdname(&cmds, "aaa", 3); + add_cmdname(&cmds, "aaa", 3); + add_cmdname(&cmds, "bbb", 3); + + TEST_ASSERT_VAL("invalid original size", cmds.cnt == 3); + /* uniquify command names (to remove second 'aaa') */ + uniq(&cmds); + TEST_ASSERT_VAL("invalid final size", cmds.cnt == 2); + + TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds, "aaa") == 1); + TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds, "bbb") == 1); + TEST_ASSERT_VAL("wrong cmd", is_in_cmdlist(&cmds, "ccc") == 0); + + clean_cmdnames(&cmds); + return TEST_OK; +} + +static int test__exclude_cmdnames(struct test_suite *test __maybe_unused, + int subtest __maybe_unused) +{ + struct cmdnames cmds1 = {}; + struct cmdnames cmds2 = {}; + + add_cmdname(&cmds1, "aaa", 3); + add_cmdname(&cmds1, "bbb", 3); + add_cmdname(&cmds1, "ccc", 3); + add_cmdname(&cmds1, "ddd", 3); + add_cmdname(&cmds1, "eee", 3); + add_cmdname(&cmds1, "fff", 3); + add_cmdname(&cmds1, "ggg", 3); + add_cmdname(&cmds1, "hhh", 3); + add_cmdname(&cmds1, "iii", 3); + add_cmdname(&cmds1, "jjj", 3); + + add_cmdname(&cmds2, "bbb", 3); + add_cmdname(&cmds2, "eee", 3); + add_cmdname(&cmds2, "jjj", 3); + + TEST_ASSERT_VAL("invalid original size", cmds1.cnt == 10); + TEST_ASSERT_VAL("invalid original size", cmds2.cnt == 3); + + /* remove duplicate command names in cmds1 */ + exclude_cmds(&cmds1, &cmds2); + + TEST_ASSERT_VAL("invalid excluded size", cmds1.cnt == 7); + TEST_ASSERT_VAL("invalid excluded size", cmds2.cnt == 3); + + /* excluded commands should not belong to cmds1 */ + TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds1, "aaa") == 1); + TEST_ASSERT_VAL("wrong cmd", is_in_cmdlist(&cmds1, "bbb") == 0); + TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds1, "ccc") == 1); + TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds1, "ddd") == 1); + TEST_ASSERT_VAL("wrong cmd", is_in_cmdlist(&cmds1, "eee") == 0); + TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds1, "fff") == 1); + TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds1, "ggg") == 1); + TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds1, "hhh") == 1); + TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds1, "iii") == 1); + TEST_ASSERT_VAL("wrong cmd", is_in_cmdlist(&cmds1, "jjj") == 0); + + /* they should be only in cmds2 */ + TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds2, "bbb") == 1); + TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds2, "eee") == 1); + TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds2, "jjj") == 1); + + clean_cmdnames(&cmds1); + clean_cmdnames(&cmds2); + return TEST_OK; +} + +static struct test_case tests__subcmd_help[] = { + TEST_CASE("Load subcmd names", load_cmdnames), + TEST_CASE("Uniquify subcmd names", uniq_cmdnames), + TEST_CASE("Exclude duplicate subcmd names", exclude_cmdnames), + { .name = NULL, } +}; + +struct test_suite suite__subcmd_help = { + .desc = "libsubcmd help tests", + .test_cases = tests__subcmd_help, +}; diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h index 4c128a9594413b32..13cabf85185ed2d3 100644 --- a/tools/perf/tests/tests.h +++ b/tools/perf/tests/tests.h @@ -177,6 +177,7 @@ DECLARE_SUITE(sigtrap); DECLARE_SUITE(event_groups); DECLARE_SUITE(symbols); DECLARE_SUITE(util); +DECLARE_SUITE(subcmd_help); /* * PowerPC and S390 do not support creation of instruction breakpoints using the -- 2.50.0.727.gbf7dc18ff4-goog ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] perf test: Add libsubcmd help tests 2025-06-30 23:32 ` [PATCH 2/3] perf test: Add libsubcmd help tests Namhyung Kim @ 2025-07-01 15:54 ` Ian Rogers 2025-07-01 19:56 ` Namhyung Kim 0 siblings, 1 reply; 9+ messages in thread From: Ian Rogers @ 2025-07-01 15:54 UTC (permalink / raw) To: Namhyung Kim Cc: Arnaldo Carvalho de Melo, Kan Liang, Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users On Mon, Jun 30, 2025 at 4:32 PM Namhyung Kim <namhyung@kernel.org> wrote: > > Add a set of tests for subcmd routines. Currently it fails the last one > since there's a bug. It'll be fixed by the next commit. > > $ perf test subcmd > 69: libsubcmd help tests : > 69.1: Load subcmd names : Ok > 69.2: Uniquify subcmd names : Ok > 69.3: Exclude duplicate subcmd names : FAILED! > > Signed-off-by: Namhyung Kim <namhyung@kernel.org> Reviewed-by: Ian Rogers <irogers@google.com> > --- > tools/perf/tests/Build | 1 + > tools/perf/tests/builtin-test.c | 1 + > tools/perf/tests/subcmd-help.c | 109 ++++++++++++++++++++++++++++++++ > tools/perf/tests/tests.h | 1 + > 4 files changed, 112 insertions(+) > create mode 100644 tools/perf/tests/subcmd-help.c > > diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build > index 2181f5a92148b0b9..13a81154ec1e4cd2 100644 > --- a/tools/perf/tests/Build > +++ b/tools/perf/tests/Build > @@ -69,6 +69,7 @@ perf-test-y += symbols.o > perf-test-y += util.o > perf-test-y += hwmon_pmu.o > perf-test-y += tool_pmu.o > +perf-test-y += subcmd-help.o > > ifeq ($(SRCARCH),$(filter $(SRCARCH),x86 arm arm64 powerpc)) > perf-test-$(CONFIG_DWARF_UNWIND) += dwarf-unwind.o > diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c > index dfaff4185eb05a1a..2da9b69864da53c2 100644 > --- a/tools/perf/tests/builtin-test.c > +++ b/tools/perf/tests/builtin-test.c > @@ -139,6 +139,7 @@ static struct test_suite *generic_tests[] = { > &suite__event_groups, > &suite__symbols, > &suite__util, > + &suite__subcmd_help, > NULL, > }; > > diff --git a/tools/perf/tests/subcmd-help.c b/tools/perf/tests/subcmd-help.c > new file mode 100644 > index 0000000000000000..d31259340ae302af > --- /dev/null > +++ b/tools/perf/tests/subcmd-help.c > @@ -0,0 +1,109 @@ > +// SPDX-License-Identifier: GPL-2.0 > +#include "tests.h" > +#include <stdio.h> > +#include <subcmd/help.h> > +#include "debug.h" nit: I don't think stdio.h and debug.h are used here. > + > +static int test__load_cmdnames(struct test_suite *test __maybe_unused, > + int subtest __maybe_unused) > +{ > + struct cmdnames cmds = {}; > + > + add_cmdname(&cmds, "aaa", 3); > + add_cmdname(&cmds, "foo", 3); > + add_cmdname(&cmds, "xyz", 3); > + > + TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds, "aaa") == 1); > + TEST_ASSERT_VAL("wrong cmd", is_in_cmdlist(&cmds, "bar") == 0); > + TEST_ASSERT_VAL("case sensitive", is_in_cmdlist(&cmds, "XYZ") == 0); > + > + clean_cmdnames(&cmds); > + return TEST_OK; > +} > + > +static int test__uniq_cmdnames(struct test_suite *test __maybe_unused, > + int subtest __maybe_unused) > +{ > + struct cmdnames cmds = {}; > + > + /* uniq() assumes it's sorted */ > + add_cmdname(&cmds, "aaa", 3); > + add_cmdname(&cmds, "aaa", 3); > + add_cmdname(&cmds, "bbb", 3); > + > + TEST_ASSERT_VAL("invalid original size", cmds.cnt == 3); > + /* uniquify command names (to remove second 'aaa') */ > + uniq(&cmds); > + TEST_ASSERT_VAL("invalid final size", cmds.cnt == 2); > + > + TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds, "aaa") == 1); > + TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds, "bbb") == 1); > + TEST_ASSERT_VAL("wrong cmd", is_in_cmdlist(&cmds, "ccc") == 0); > + > + clean_cmdnames(&cmds); > + return TEST_OK; > +} > + > +static int test__exclude_cmdnames(struct test_suite *test __maybe_unused, > + int subtest __maybe_unused) > +{ > + struct cmdnames cmds1 = {}; > + struct cmdnames cmds2 = {}; > + > + add_cmdname(&cmds1, "aaa", 3); > + add_cmdname(&cmds1, "bbb", 3); > + add_cmdname(&cmds1, "ccc", 3); > + add_cmdname(&cmds1, "ddd", 3); > + add_cmdname(&cmds1, "eee", 3); > + add_cmdname(&cmds1, "fff", 3); > + add_cmdname(&cmds1, "ggg", 3); > + add_cmdname(&cmds1, "hhh", 3); > + add_cmdname(&cmds1, "iii", 3); > + add_cmdname(&cmds1, "jjj", 3); > + > + add_cmdname(&cmds2, "bbb", 3); > + add_cmdname(&cmds2, "eee", 3); > + add_cmdname(&cmds2, "jjj", 3); > + > + TEST_ASSERT_VAL("invalid original size", cmds1.cnt == 10); > + TEST_ASSERT_VAL("invalid original size", cmds2.cnt == 3); > + > + /* remove duplicate command names in cmds1 */ > + exclude_cmds(&cmds1, &cmds2); > + > + TEST_ASSERT_VAL("invalid excluded size", cmds1.cnt == 7); > + TEST_ASSERT_VAL("invalid excluded size", cmds2.cnt == 3); > + > + /* excluded commands should not belong to cmds1 */ > + TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds1, "aaa") == 1); > + TEST_ASSERT_VAL("wrong cmd", is_in_cmdlist(&cmds1, "bbb") == 0); > + TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds1, "ccc") == 1); > + TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds1, "ddd") == 1); > + TEST_ASSERT_VAL("wrong cmd", is_in_cmdlist(&cmds1, "eee") == 0); > + TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds1, "fff") == 1); > + TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds1, "ggg") == 1); > + TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds1, "hhh") == 1); > + TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds1, "iii") == 1); > + TEST_ASSERT_VAL("wrong cmd", is_in_cmdlist(&cmds1, "jjj") == 0); > + > + /* they should be only in cmds2 */ > + TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds2, "bbb") == 1); > + TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds2, "eee") == 1); > + TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds2, "jjj") == 1); > + > + clean_cmdnames(&cmds1); > + clean_cmdnames(&cmds2); > + return TEST_OK; > +} > + > +static struct test_case tests__subcmd_help[] = { > + TEST_CASE("Load subcmd names", load_cmdnames), > + TEST_CASE("Uniquify subcmd names", uniq_cmdnames), > + TEST_CASE("Exclude duplicate subcmd names", exclude_cmdnames), > + { .name = NULL, } > +}; > + > +struct test_suite suite__subcmd_help = { > + .desc = "libsubcmd help tests", > + .test_cases = tests__subcmd_help, > +}; > diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h > index 4c128a9594413b32..13cabf85185ed2d3 100644 > --- a/tools/perf/tests/tests.h > +++ b/tools/perf/tests/tests.h > @@ -177,6 +177,7 @@ DECLARE_SUITE(sigtrap); > DECLARE_SUITE(event_groups); > DECLARE_SUITE(symbols); > DECLARE_SUITE(util); > +DECLARE_SUITE(subcmd_help); > > /* > * PowerPC and S390 do not support creation of instruction breakpoints using the > -- > 2.50.0.727.gbf7dc18ff4-goog > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] perf test: Add libsubcmd help tests 2025-07-01 15:54 ` Ian Rogers @ 2025-07-01 19:56 ` Namhyung Kim 0 siblings, 0 replies; 9+ messages in thread From: Namhyung Kim @ 2025-07-01 19:56 UTC (permalink / raw) To: Ian Rogers Cc: Arnaldo Carvalho de Melo, Kan Liang, Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users On Tue, Jul 01, 2025 at 08:54:25AM -0700, Ian Rogers wrote: > On Mon, Jun 30, 2025 at 4:32 PM Namhyung Kim <namhyung@kernel.org> wrote: > > > > Add a set of tests for subcmd routines. Currently it fails the last one > > since there's a bug. It'll be fixed by the next commit. > > > > $ perf test subcmd > > 69: libsubcmd help tests : > > 69.1: Load subcmd names : Ok > > 69.2: Uniquify subcmd names : Ok > > 69.3: Exclude duplicate subcmd names : FAILED! > > > > Signed-off-by: Namhyung Kim <namhyung@kernel.org> > > Reviewed-by: Ian Rogers <irogers@google.com> Thanks for your review! > > > --- > > tools/perf/tests/Build | 1 + > > tools/perf/tests/builtin-test.c | 1 + > > tools/perf/tests/subcmd-help.c | 109 ++++++++++++++++++++++++++++++++ > > tools/perf/tests/tests.h | 1 + > > 4 files changed, 112 insertions(+) > > create mode 100644 tools/perf/tests/subcmd-help.c > > > > diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build > > index 2181f5a92148b0b9..13a81154ec1e4cd2 100644 > > --- a/tools/perf/tests/Build > > +++ b/tools/perf/tests/Build > > @@ -69,6 +69,7 @@ perf-test-y += symbols.o > > perf-test-y += util.o > > perf-test-y += hwmon_pmu.o > > perf-test-y += tool_pmu.o > > +perf-test-y += subcmd-help.o > > > > ifeq ($(SRCARCH),$(filter $(SRCARCH),x86 arm arm64 powerpc)) > > perf-test-$(CONFIG_DWARF_UNWIND) += dwarf-unwind.o > > diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c > > index dfaff4185eb05a1a..2da9b69864da53c2 100644 > > --- a/tools/perf/tests/builtin-test.c > > +++ b/tools/perf/tests/builtin-test.c > > @@ -139,6 +139,7 @@ static struct test_suite *generic_tests[] = { > > &suite__event_groups, > > &suite__symbols, > > &suite__util, > > + &suite__subcmd_help, > > NULL, > > }; > > > > diff --git a/tools/perf/tests/subcmd-help.c b/tools/perf/tests/subcmd-help.c > > new file mode 100644 > > index 0000000000000000..d31259340ae302af > > --- /dev/null > > +++ b/tools/perf/tests/subcmd-help.c > > @@ -0,0 +1,109 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +#include "tests.h" > > +#include <stdio.h> > > +#include <subcmd/help.h> > > +#include "debug.h" > > nit: I don't think stdio.h and debug.h are used here. Yeah, will remove. I think I added it for debugging and removed the debug code later. Thanks, Namhyung > > > + > > +static int test__load_cmdnames(struct test_suite *test __maybe_unused, > > + int subtest __maybe_unused) > > +{ > > + struct cmdnames cmds = {}; > > + > > + add_cmdname(&cmds, "aaa", 3); > > + add_cmdname(&cmds, "foo", 3); > > + add_cmdname(&cmds, "xyz", 3); > > + > > + TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds, "aaa") == 1); > > + TEST_ASSERT_VAL("wrong cmd", is_in_cmdlist(&cmds, "bar") == 0); > > + TEST_ASSERT_VAL("case sensitive", is_in_cmdlist(&cmds, "XYZ") == 0); > > + > > + clean_cmdnames(&cmds); > > + return TEST_OK; > > +} > > + > > +static int test__uniq_cmdnames(struct test_suite *test __maybe_unused, > > + int subtest __maybe_unused) > > +{ > > + struct cmdnames cmds = {}; > > + > > + /* uniq() assumes it's sorted */ > > + add_cmdname(&cmds, "aaa", 3); > > + add_cmdname(&cmds, "aaa", 3); > > + add_cmdname(&cmds, "bbb", 3); > > + > > + TEST_ASSERT_VAL("invalid original size", cmds.cnt == 3); > > + /* uniquify command names (to remove second 'aaa') */ > > + uniq(&cmds); > > + TEST_ASSERT_VAL("invalid final size", cmds.cnt == 2); > > + > > + TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds, "aaa") == 1); > > + TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds, "bbb") == 1); > > + TEST_ASSERT_VAL("wrong cmd", is_in_cmdlist(&cmds, "ccc") == 0); > > + > > + clean_cmdnames(&cmds); > > + return TEST_OK; > > +} > > + > > +static int test__exclude_cmdnames(struct test_suite *test __maybe_unused, > > + int subtest __maybe_unused) > > +{ > > + struct cmdnames cmds1 = {}; > > + struct cmdnames cmds2 = {}; > > + > > + add_cmdname(&cmds1, "aaa", 3); > > + add_cmdname(&cmds1, "bbb", 3); > > + add_cmdname(&cmds1, "ccc", 3); > > + add_cmdname(&cmds1, "ddd", 3); > > + add_cmdname(&cmds1, "eee", 3); > > + add_cmdname(&cmds1, "fff", 3); > > + add_cmdname(&cmds1, "ggg", 3); > > + add_cmdname(&cmds1, "hhh", 3); > > + add_cmdname(&cmds1, "iii", 3); > > + add_cmdname(&cmds1, "jjj", 3); > > + > > + add_cmdname(&cmds2, "bbb", 3); > > + add_cmdname(&cmds2, "eee", 3); > > + add_cmdname(&cmds2, "jjj", 3); > > + > > + TEST_ASSERT_VAL("invalid original size", cmds1.cnt == 10); > > + TEST_ASSERT_VAL("invalid original size", cmds2.cnt == 3); > > + > > + /* remove duplicate command names in cmds1 */ > > + exclude_cmds(&cmds1, &cmds2); > > + > > + TEST_ASSERT_VAL("invalid excluded size", cmds1.cnt == 7); > > + TEST_ASSERT_VAL("invalid excluded size", cmds2.cnt == 3); > > + > > + /* excluded commands should not belong to cmds1 */ > > + TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds1, "aaa") == 1); > > + TEST_ASSERT_VAL("wrong cmd", is_in_cmdlist(&cmds1, "bbb") == 0); > > + TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds1, "ccc") == 1); > > + TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds1, "ddd") == 1); > > + TEST_ASSERT_VAL("wrong cmd", is_in_cmdlist(&cmds1, "eee") == 0); > > + TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds1, "fff") == 1); > > + TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds1, "ggg") == 1); > > + TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds1, "hhh") == 1); > > + TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds1, "iii") == 1); > > + TEST_ASSERT_VAL("wrong cmd", is_in_cmdlist(&cmds1, "jjj") == 0); > > + > > + /* they should be only in cmds2 */ > > + TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds2, "bbb") == 1); > > + TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds2, "eee") == 1); > > + TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds2, "jjj") == 1); > > + > > + clean_cmdnames(&cmds1); > > + clean_cmdnames(&cmds2); > > + return TEST_OK; > > +} > > + > > +static struct test_case tests__subcmd_help[] = { > > + TEST_CASE("Load subcmd names", load_cmdnames), > > + TEST_CASE("Uniquify subcmd names", uniq_cmdnames), > > + TEST_CASE("Exclude duplicate subcmd names", exclude_cmdnames), > > + { .name = NULL, } > > +}; > > + > > +struct test_suite suite__subcmd_help = { > > + .desc = "libsubcmd help tests", > > + .test_cases = tests__subcmd_help, > > +}; > > diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h > > index 4c128a9594413b32..13cabf85185ed2d3 100644 > > --- a/tools/perf/tests/tests.h > > +++ b/tools/perf/tests/tests.h > > @@ -177,6 +177,7 @@ DECLARE_SUITE(sigtrap); > > DECLARE_SUITE(event_groups); > > DECLARE_SUITE(symbols); > > DECLARE_SUITE(util); > > +DECLARE_SUITE(subcmd_help); > > > > /* > > * PowerPC and S390 do not support creation of instruction breakpoints using the > > -- > > 2.50.0.727.gbf7dc18ff4-goog > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/3] perf tools: Fix use-after-free in help_unknown_cmd() 2025-06-30 23:32 [PATCH 1/3] perf test: Check test suite description only Namhyung Kim 2025-06-30 23:32 ` [PATCH 2/3] perf test: Add libsubcmd help tests Namhyung Kim @ 2025-06-30 23:32 ` Namhyung Kim 2025-07-01 16:03 ` Ian Rogers 2025-07-01 15:51 ` [PATCH 1/3] perf test: Check test suite description only Ian Rogers 2 siblings, 1 reply; 9+ messages in thread From: Namhyung Kim @ 2025-06-30 23:32 UTC (permalink / raw) To: Arnaldo Carvalho de Melo, Ian Rogers, Kan Liang Cc: Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users Currently perf aborts when it finds an invalid command. I guess it depends on the environment as I have some custom commands in the path. $ perf bad-command perf: 'bad-command' is not a perf-command. See 'perf --help'. Aborted (core dumped) It's because the exclude_cmds() in libsubcmd has a use-after-free when it removes some entries. After copying one to another entry, it keeps the pointer in the both position. And the next copy operation will free the later one but it's the same entry in the previous one. For example, let's say cmds = { A, B, C, D, E } and excludes = { B, E }. ci cj ei cmds-name excludes -----------+-------------------- 0 0 0 | A B : cmp < 0, ci == cj 1 1 0 | B B : cmp == 0 2 1 1 | C E : cmp < 0, ci != cj At this point, it frees cmds->names[1] and cmds->names[1] is assigned to cmds->names[2]. 3 2 1 | D E : cmp < 0, ci != cj Now it frees cmds->names[2] but it's the same as cmds->names[1]. So accessing cmds->names[1] will be invalid. This makes the subcmd tests succeed. $ perf test subcmd 69: libsubcmd help tests : 69.1: Load subcmd names : Ok 69.2: Uniquify subcmd names : Ok 69.3: Exclude duplicate subcmd names : Ok Fixes: 657a3efee43a ("libsubcmd: Avoid SEGV/use-after-free when commands aren't excluded") Signed-off-by: Namhyung Kim <namhyung@kernel.org> --- tools/lib/subcmd/help.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/tools/lib/subcmd/help.c b/tools/lib/subcmd/help.c index 8561b0f01a247690..9ef569492560efd7 100644 --- a/tools/lib/subcmd/help.c +++ b/tools/lib/subcmd/help.c @@ -9,6 +9,7 @@ #include <sys/stat.h> #include <unistd.h> #include <dirent.h> +#include <assert.h> #include "subcmd-util.h" #include "help.h" #include "exec-cmd.h" @@ -82,10 +83,11 @@ void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes) ci++; cj++; } else { - zfree(&cmds->names[cj]); - cmds->names[cj++] = cmds->names[ci++]; + cmds->names[cj++] = cmds->names[ci]; + cmds->names[ci++] = NULL; } } else if (cmp == 0) { + zfree(&cmds->names[ci]); ci++; ei++; } else if (cmp > 0) { @@ -94,12 +96,12 @@ void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes) } if (ci != cj) { while (ci < cmds->cnt) { - zfree(&cmds->names[cj]); - cmds->names[cj++] = cmds->names[ci++]; + cmds->names[cj++] = cmds->names[ci]; + cmds->names[ci++] = NULL; } } for (ci = cj; ci < cmds->cnt; ci++) - zfree(&cmds->names[ci]); + assert(cmds->names[ci] == NULL); cmds->cnt = cj; } -- 2.50.0.727.gbf7dc18ff4-goog ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] perf tools: Fix use-after-free in help_unknown_cmd() 2025-06-30 23:32 ` [PATCH 3/3] perf tools: Fix use-after-free in help_unknown_cmd() Namhyung Kim @ 2025-07-01 16:03 ` Ian Rogers 2025-07-01 19:57 ` Namhyung Kim 0 siblings, 1 reply; 9+ messages in thread From: Ian Rogers @ 2025-07-01 16:03 UTC (permalink / raw) To: Namhyung Kim Cc: Arnaldo Carvalho de Melo, Kan Liang, Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users On Mon, Jun 30, 2025 at 4:32 PM Namhyung Kim <namhyung@kernel.org> wrote: > > Currently perf aborts when it finds an invalid command. I guess it > depends on the environment as I have some custom commands in the path. > > $ perf bad-command > perf: 'bad-command' is not a perf-command. See 'perf --help'. > Aborted (core dumped) > > It's because the exclude_cmds() in libsubcmd has a use-after-free when > it removes some entries. After copying one to another entry, it keeps > the pointer in the both position. And the next copy operation will free > the later one but it's the same entry in the previous one. > > For example, let's say cmds = { A, B, C, D, E } and excludes = { B, E }. > > ci cj ei cmds-name excludes > -----------+-------------------- > 0 0 0 | A B : cmp < 0, ci == cj > 1 1 0 | B B : cmp == 0 > 2 1 1 | C E : cmp < 0, ci != cj > > At this point, it frees cmds->names[1] and cmds->names[1] is assigned to > cmds->names[2]. > > 3 2 1 | D E : cmp < 0, ci != cj > > Now it frees cmds->names[2] but it's the same as cmds->names[1]. So > accessing cmds->names[1] will be invalid. > > This makes the subcmd tests succeed. > > $ perf test subcmd > 69: libsubcmd help tests : > 69.1: Load subcmd names : Ok > 69.2: Uniquify subcmd names : Ok > 69.3: Exclude duplicate subcmd names : Ok > > Fixes: 657a3efee43a ("libsubcmd: Avoid SEGV/use-after-free when commands aren't excluded") > Signed-off-by: Namhyung Kim <namhyung@kernel.org> Reviewed-by: Ian Rogers <irogers@google.com> Fwiw, it seems a shame we're doing this and the code in git is drifting apart: https://github.com/git/git/blob/83014dc05f6fc9275c0a02886cb428805abaf9e5/help.c#L204 Thanks, Ian > --- > tools/lib/subcmd/help.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/tools/lib/subcmd/help.c b/tools/lib/subcmd/help.c > index 8561b0f01a247690..9ef569492560efd7 100644 > --- a/tools/lib/subcmd/help.c > +++ b/tools/lib/subcmd/help.c > @@ -9,6 +9,7 @@ > #include <sys/stat.h> > #include <unistd.h> > #include <dirent.h> > +#include <assert.h> > #include "subcmd-util.h" > #include "help.h" > #include "exec-cmd.h" > @@ -82,10 +83,11 @@ void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes) > ci++; > cj++; > } else { > - zfree(&cmds->names[cj]); > - cmds->names[cj++] = cmds->names[ci++]; > + cmds->names[cj++] = cmds->names[ci]; > + cmds->names[ci++] = NULL; > } > } else if (cmp == 0) { > + zfree(&cmds->names[ci]); > ci++; > ei++; > } else if (cmp > 0) { > @@ -94,12 +96,12 @@ void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes) > } > if (ci != cj) { > while (ci < cmds->cnt) { > - zfree(&cmds->names[cj]); > - cmds->names[cj++] = cmds->names[ci++]; > + cmds->names[cj++] = cmds->names[ci]; > + cmds->names[ci++] = NULL; > } > } > for (ci = cj; ci < cmds->cnt; ci++) > - zfree(&cmds->names[ci]); > + assert(cmds->names[ci] == NULL); > cmds->cnt = cj; > } > > -- > 2.50.0.727.gbf7dc18ff4-goog > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] perf tools: Fix use-after-free in help_unknown_cmd() 2025-07-01 16:03 ` Ian Rogers @ 2025-07-01 19:57 ` Namhyung Kim 0 siblings, 0 replies; 9+ messages in thread From: Namhyung Kim @ 2025-07-01 19:57 UTC (permalink / raw) To: Ian Rogers Cc: Arnaldo Carvalho de Melo, Kan Liang, Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users On Tue, Jul 01, 2025 at 09:03:46AM -0700, Ian Rogers wrote: > On Mon, Jun 30, 2025 at 4:32 PM Namhyung Kim <namhyung@kernel.org> wrote: > > > > Currently perf aborts when it finds an invalid command. I guess it > > depends on the environment as I have some custom commands in the path. > > > > $ perf bad-command > > perf: 'bad-command' is not a perf-command. See 'perf --help'. > > Aborted (core dumped) > > > > It's because the exclude_cmds() in libsubcmd has a use-after-free when > > it removes some entries. After copying one to another entry, it keeps > > the pointer in the both position. And the next copy operation will free > > the later one but it's the same entry in the previous one. > > > > For example, let's say cmds = { A, B, C, D, E } and excludes = { B, E }. > > > > ci cj ei cmds-name excludes > > -----------+-------------------- > > 0 0 0 | A B : cmp < 0, ci == cj > > 1 1 0 | B B : cmp == 0 > > 2 1 1 | C E : cmp < 0, ci != cj > > > > At this point, it frees cmds->names[1] and cmds->names[1] is assigned to > > cmds->names[2]. > > > > 3 2 1 | D E : cmp < 0, ci != cj > > > > Now it frees cmds->names[2] but it's the same as cmds->names[1]. So > > accessing cmds->names[1] will be invalid. > > > > This makes the subcmd tests succeed. > > > > $ perf test subcmd > > 69: libsubcmd help tests : > > 69.1: Load subcmd names : Ok > > 69.2: Uniquify subcmd names : Ok > > 69.3: Exclude duplicate subcmd names : Ok > > > > Fixes: 657a3efee43a ("libsubcmd: Avoid SEGV/use-after-free when commands aren't excluded") > > Signed-off-by: Namhyung Kim <namhyung@kernel.org> > > Reviewed-by: Ian Rogers <irogers@google.com> > > Fwiw, it seems a shame we're doing this and the code in git is drifting apart: > > https://github.com/git/git/blob/83014dc05f6fc9275c0a02886cb428805abaf9e5/help.c#L204 Maybe we can send a fix for them too. Thanks, Namhyung ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] perf test: Check test suite description only 2025-06-30 23:32 [PATCH 1/3] perf test: Check test suite description only Namhyung Kim 2025-06-30 23:32 ` [PATCH 2/3] perf test: Add libsubcmd help tests Namhyung Kim 2025-06-30 23:32 ` [PATCH 3/3] perf tools: Fix use-after-free in help_unknown_cmd() Namhyung Kim @ 2025-07-01 15:51 ` Ian Rogers 2025-07-01 19:49 ` Namhyung Kim 2 siblings, 1 reply; 9+ messages in thread From: Ian Rogers @ 2025-07-01 15:51 UTC (permalink / raw) To: Namhyung Kim Cc: Arnaldo Carvalho de Melo, Kan Liang, Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users On Mon, Jun 30, 2025 at 4:32 PM Namhyung Kim <namhyung@kernel.org> wrote: > > Currently perf test checks the given string with descriptions for both > test suites and cases (subtests). But sometimes it's confusing since > the subtests don't contain the important keyword. > > I think it's better to check the suite level and run the whole suite > together. > > Before: > $ perf test hwmon > (no output) > > After: > $ perf test hwmon > 10: Hwmon PMU : > 10.1: Basic parsing test : Ok > 10.2: Parsing without PMU name : Ok > 10.3: Parsing with PMU name : Ok This is better, thanks! > Signed-off-by: Namhyung Kim <namhyung@kernel.org> > --- > tools/perf/tests/builtin-test.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c > index 80375ca39a37a256..dfaff4185eb05a1a 100644 > --- a/tools/perf/tests/builtin-test.c > +++ b/tools/perf/tests/builtin-test.c > @@ -567,10 +567,6 @@ static int __cmd_test(struct test_suite **suites, int argc, const char *argv[], > > for (unsigned int run = 0; run < runs_per_test; run++) { > test_suite__for_each_test_case(*t, curr_test_case) { > - if (!perf_test__matches(test_description(*t, curr_test_case), > - curr_suite, argc, argv)) > - continue; > - This will change the behavior so that if a sub-test matches but the test suite as a whole doesn't the whole test suite will now be run. For example: ``` $ perf test list ... 39: CPU map 39.1: Synthesize cpu map 39.2: Print cpu map 39.3: Merge cpu map 39.4: Intersect cpu map 39.5: Equal cpu map ... $ perf test -v "Equal cpu map" 39.5: Equal cpu map : Ok ``` whereas with this change the whole of the "CPU map" test suite will be run. I think the condition: ``` if (!perf_test__matches(test_description(*t, curr_test_case), curr_suite, argc, argv)) ``` should be: ``` if (!perf_test__matches(test_description(*t, curr_test_case), curr_suite, argc, argv) && !perf_test__matches(test_description(*t, -1), curr_suite, argc, argv)) ``` But you could avoid computing the extra perf_test__matches with a boolean as that test is done immediately before. Thanks, Ian > err = start_test(*t, curr_suite, curr_test_case, > &child_tests[child_test_num++], > width, pass); > -- > 2.50.0.727.gbf7dc18ff4-goog > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] perf test: Check test suite description only 2025-07-01 15:51 ` [PATCH 1/3] perf test: Check test suite description only Ian Rogers @ 2025-07-01 19:49 ` Namhyung Kim 0 siblings, 0 replies; 9+ messages in thread From: Namhyung Kim @ 2025-07-01 19:49 UTC (permalink / raw) To: Ian Rogers Cc: Arnaldo Carvalho de Melo, Kan Liang, Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users On Tue, Jul 01, 2025 at 08:51:53AM -0700, Ian Rogers wrote: > On Mon, Jun 30, 2025 at 4:32 PM Namhyung Kim <namhyung@kernel.org> wrote: > > > > Currently perf test checks the given string with descriptions for both > > test suites and cases (subtests). But sometimes it's confusing since > > the subtests don't contain the important keyword. > > > > I think it's better to check the suite level and run the whole suite > > together. > > > > Before: > > $ perf test hwmon > > (no output) > > > > After: > > $ perf test hwmon > > 10: Hwmon PMU : > > 10.1: Basic parsing test : Ok > > 10.2: Parsing without PMU name : Ok > > 10.3: Parsing with PMU name : Ok > > This is better, thanks! > > > Signed-off-by: Namhyung Kim <namhyung@kernel.org> > > --- > > tools/perf/tests/builtin-test.c | 4 ---- > > 1 file changed, 4 deletions(-) > > > > diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c > > index 80375ca39a37a256..dfaff4185eb05a1a 100644 > > --- a/tools/perf/tests/builtin-test.c > > +++ b/tools/perf/tests/builtin-test.c > > @@ -567,10 +567,6 @@ static int __cmd_test(struct test_suite **suites, int argc, const char *argv[], > > > > for (unsigned int run = 0; run < runs_per_test; run++) { > > test_suite__for_each_test_case(*t, curr_test_case) { > > - if (!perf_test__matches(test_description(*t, curr_test_case), > > - curr_suite, argc, argv)) > > - continue; > > - > > This will change the behavior so that if a sub-test matches but the > test suite as a whole doesn't the whole test suite will now be run. > For example: > > ``` > $ perf test list > ... > 39: CPU map > 39.1: Synthesize cpu map > 39.2: Print cpu map > 39.3: Merge cpu map > 39.4: Intersect cpu map > 39.5: Equal cpu map > ... > $ perf test -v "Equal cpu map" > 39.5: Equal cpu map : Ok > ``` > > whereas with this change the whole of the "CPU map" test suite will be > run. I think the condition: > > ``` > if (!perf_test__matches(test_description(*t, curr_test_case), > curr_suite, argc, argv)) > ``` > > should be: > > ``` > if (!perf_test__matches(test_description(*t, curr_test_case), > curr_suite, argc, argv) && > !perf_test__matches(test_description(*t, -1), curr_suite, argc, argv)) > ``` > > But you could avoid computing the extra perf_test__matches with a > boolean as that test is done immediately before. Oh, I overlooked the condition in the loop. Will check the both can skip if it only matched to some subtests. Thanks, Namhyung ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2025-07-01 19:57 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-06-30 23:32 [PATCH 1/3] perf test: Check test suite description only Namhyung Kim 2025-06-30 23:32 ` [PATCH 2/3] perf test: Add libsubcmd help tests Namhyung Kim 2025-07-01 15:54 ` Ian Rogers 2025-07-01 19:56 ` Namhyung Kim 2025-06-30 23:32 ` [PATCH 3/3] perf tools: Fix use-after-free in help_unknown_cmd() Namhyung Kim 2025-07-01 16:03 ` Ian Rogers 2025-07-01 19:57 ` Namhyung Kim 2025-07-01 15:51 ` [PATCH 1/3] perf test: Check test suite description only Ian Rogers 2025-07-01 19:49 ` Namhyung Kim
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).