Re: [PATCH v6 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 1 potential issue(s) to consider:
- [Low] The logic for handling 'expected_instructions_group_size == 2' in test_sort() is unreachable dead code.
--

commit 593335141c0ab35f13d4085b28aa7eaa6387b85d
Author: Ian Rogers <irogers@google.com>
Subject: perf arch x86 tests: Add test for topdown event sorting

This commit adds tests to ensure that slots, topdown-retiring, and
instructions events are appropriately sorted and grouped when specified
together.

> diff --git a/tools/perf/arch/x86/tests/topdown.c b/tools/perf/arch/x86/tests/topdown.c
> index 3ee4e5e71be3b..dbee32bf973e3 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 expected_instructions_group_size == 2 block reachable?

Looking at test_sort(), the loop iterates over evsels and skips any that are
not group leaders.

The only test cases invoking test_sort() with expected_instructions_group_size
set to 2 are those that also expect a slots group size of 3, such as:

test_sort("{instructions,topdown-retiring,slots}", 3, 2);

When slots groups all 3 events together, it becomes the group leader.
Consequently, instructions acts as a group member (not a leader) and is
skipped by the loop's continue statement.

Additionally, the test enforces that slots must be a leader by requiring
slots_seen = true to pass.

Does this mean there are no scenarios in the tests where instructions can act
as a leader of size 2, rendering this validation block dead code?

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