Re: [PATCH v1 08/14] perf test: Add summary reporting

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Test summary is bypassed on error paths, leaking memory and contradicting the commit message.
- [Medium] User-overridden skipped tests are double-counted in the summary statistics.
- [Medium] Async-signal-safety deadlock due to `strbuf_addf` allocations in signal-interrupted paths.
--

commit 6ba5c5902b4731354e1e3209d7b4d55a0a3bc650
Author: Ian Rogers <irogers@google.com>

perf test: Add summary reporting

This commit introduces a global execution summary for perf tests, tracking
pass, skip, and fail counts and printing them at the end of the run. It also
records failed test descriptions into a buffer for easy cross-referencing.

> 4. Invoke the summary printer right before freeing the test array at the
>    absolute tail of __cmd_test(), guaranteeing that the summary is
>    successfully printed even if an internal emergency signal cleanup occurs
>    or if the user interrupts the run early.

Is this description accurate? Looking at __cmd_test(), print_tests_summary()
is placed inside an else block that is bypassed if err is set.

> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index 45c1afb9ad333..193a7dec6d5f3 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c
> @@ -389,6 +400,15 @@ static int print_test_result(struct test_suite *t, int curr_suite, int curr_test
>  		break;
>  	case TEST_FAIL:
>  	default:
> +		summary_tests_failed++;
> +		if (test_suite__num_test_cases(t) > 1)
> +			strbuf_addf(&summary_failed_tests_buf, "  %3d.%1d: %s\n",
> +				    curr_suite + 1, curr_test_case + 1,
> +				    test_description(t, curr_test_case));
> +		else
> +			strbuf_addf(&summary_failed_tests_buf, "  %3d: %s\n",
> +				    curr_suite + 1,
> +				    test_description(t, curr_test_case));
>  		color_fprintf(stderr, PERF_COLOR_RED, " FAILED!\n");

If a signal interrupts the main thread while it holds the libc heap lock,
could this lead to a deadlock? The cleanup path triggered by the signal
handler calls:

cmd_test_sig_handler() -> siglongjmp() -> finish_test() ->
print_test_result() -> strbuf_addf()

which can invoke realloc() while the heap lock is already held.

[ ... ]

> @@ -1045,6 +1081,7 @@ static int __cmd_test(struct test_suite **suites, int argc, const char *argv[],
>  				pr_info("%3d: %-*s:", curr_suite + 1, width,
>  					test_description(*t, -1));
>  				color_fprintf(stderr, PERF_COLOR_YELLOW, " Skip (user override)\n");
> +				summary_tests_skipped++;
>  				continue;
>  			}

Since this code runs inside the for (int pass = 1; pass <= 2; pass++) loop,
will this increment summary_tests_skipped twice for each user-skipped
test suite?

[ ... ]

> @@ -1076,6 +1113,8 @@ static int __cmd_test(struct test_suite **suites, int argc, const char *argv[],
>  		pr_err("Internal test harness failure. Completing any started tests:\n:");
>  		for (size_t x = 0; x < num_tests; x++)
>  			finish_test(child_tests, x, num_tests, width);
> +	} else {
> +		print_tests_summary();
>  	}
>  	free(child_tests);
>  	return err;

Since print_tests_summary() is only called in the else branch, does this
leak summary_failed_tests_buf when err is non-zero (for instance, when
handling an internal emergency signal cleanup)?

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