Re: [PATCH v5 2/2] perf arch x86 tests: Add test for topdown event sorting

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The validation block for `expected_instructions_group_size == 2` in `test_sort()` is structurally unreachable dead code.
- [Low] Passing `test_sort()` directly to `TEST_ASSERT_EQUAL` causes the test function to execute twice upon failure.
--

commit 418562cab64daaf4f2d774b19e333bea0ea7f81a
Author: Ian Rogers <irogers@google.com>

perf arch x86 tests: Add test for topdown event sorting

This commit adds a test to verify that slots and topdown-retiring get
appropriately sorted with respect to instructions when specified together. It
also adds a related test to ensure the slots event is injected into the
appropriate group when not explicitly provided.

> diff --git a/tools/perf/arch/x86/tests/topdown.c b/tools/perf/arch/x86/tests/topdown.c
> index 3ee4e5e71be3b..9e9dbea7998dc 100644
> --- a/tools/perf/arch/x86/tests/topdown.c
> +++ b/tools/perf/arch/x86/tests/topdown.c
[ ... ]
> +	evlist__for_each_entry(evlist, evsel) {
> +		if (!evsel__is_group_leader(evsel))
> +			continue;
[ ... ]
> +		} else if (strstr(evsel__name(evsel), "instructions")) {
> +			CHECK_EQUAL(evsel->core.nr_members, expected_instructions_group_size,
> +				    "instructions group size");
> +			if (expected_instructions_group_size == 2) {
> +				/*
> +				 * The instructions event leads a group with a
> +				 * topdown-retiring event, neither of which need
> +				 * reordering for perf metric event support.
> +				 */
> +				struct evsel *next = evsel__next(evsel);
> +
> +				CHECK_COND(strstr(evsel__name(next), "topdown-retiring") != NULL,
> +					   "instructions second event is topdown-retiring");
> +			}

Is this validation block for expected_instructions_group_size == 2 reachable?

The loop explicitly skips events that are not group leaders at the top.
However, in the test cases passing 2 for expected_instructions_group_size,
the slots event is injected as the group leader for all three events.

Consequently, instructions becomes a subordinate group member and is skipped
by the loop entirely, which bypasses these test assertions.

[ ... ]
> +static int test__x86_topdown_sorting(struct test_suite *test __maybe_unused,
> +				     int subtest __maybe_unused)
> +{
> +	if (!topdown_sys_has_perf_metrics())
> +		return TEST_OK;
> +
> +	TEST_ASSERT_EQUAL("all events in a group",
> +			  test_sort("{instructions,topdown-retiring,slots}", 3, 2), TEST_OK);

Does passing test_sort() directly to TEST_ASSERT_EQUAL cause the test to
execute twice upon failure?

The TEST_ASSERT_EQUAL macro in tools/perf/tests/tests.h evaluates the
provided value a second time in its pr_debug statement if the assertion
fails.

Since test_sort() allocates an evlist, parses events, and prints internal
debug logs, evaluating it twice creates duplicate work and duplicate debug
output.

Should the return value of test_sort() be stored in a local variable before
being passed to the assertion macro?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518003704.2434308-1-irogers@google.com?part=2