Re: [PATCH v4 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] Memory leak in test_sort() on test assertion failure.
- [Medium] Dead code and completely ignored parameters in test_sort() due to a logical contradiction in test design.
--

commit 412c12ca56b31a24a283d86a447c891e7731f0d0
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 events
are correctly sorted with respect to instructions. It checks that metric
events are placed after slots, which should be the group leader when
required by the PMU. It also verifies that the slots event is appropriately
injected into groups when not specified.

> diff --git a/tools/perf/arch/x86/tests/topdown.c b/tools/perf/arch/x86/tests/topdown.c
> index 3ee4e5e71be3b..ee8d673a0e7b1 100644
> --- a/tools/perf/arch/x86/tests/topdown.c
> +++ b/tools/perf/arch/x86/tests/topdown.c
[ ... ]
> +		if (strstr(evsel__name(evsel), "slots")) {
> +			/*
> +			 * Slots as a leader means the PMU is for a perf metric
> +			 * group as the slots event isn't present when not.
> +			 */
> +			slots_seen = true;
> +			TEST_ASSERT_EQUAL("slots group size", evsel->core.nr_members,
> +					expected_slots_group_size);

Do the TEST_ASSERT_EQUAL and TEST_ASSERT_VAL macros leak evlist and err
if an assertion fails?

These macros execute a direct return TEST_FAIL when a condition is not met.

This would immediately abort the function, bypassing the out_err cleanup
block and leaking both the evlist structure and dynamically allocated
error strings.

[ ... ]
> +		} else if (strstr(evsel__name(evsel), "instructions")) {
> +			TEST_ASSERT_EQUAL("instructions group size", evsel->core.nr_members,
> +					  expected_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);
> +
> +				TEST_ASSERT_VAL("instructions second event is topdown-retiring",
> +						strstr(evsel__name(next), "topdown-retiring")
> +						!= NULL);
> +			}

Is this expected_instructions_group_size check unreachable dead code?

Both parent test functions (test__x86_topdown_sorting and
test__x86_topdown_slots_injection) exit unconditionally with TEST_OK if
topdown_sys_has_perf_metrics() is false.

Because the test only executes on systems that enforce "slots" as a group
leader, "slots" is always injected or reordered to lead the group.

The evlist iteration loop explicitly skips non-leaders via:

    if (!evsel__is_group_leader(evsel))
        continue;

Consequently, "instructions" is never evaluated as the leader of a group
containing "topdown-retiring", making this block unreachable.

Furthermore, tests pass 2 for expected_instructions_group_size in cases
like "{instructions,topdown-retiring,slots}", which contains 3 events.
If "instructions" were actually evaluated as the leader, the assertion
would incorrectly fail. 

The test passes because this parameter is ignored. Does this indicate a
logical contradiction in the test's design?

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