Re: [PATCH v1 07/14] perf test: Show snippet failure output for verbose=1

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Unbounded output and error context starvation in Pass 0
- [Low] Undefined behavior when passing uncasted signed char to `tolower()`
--

commit 957cea0e8173df2ef71d1dc7480195a00fb7a68a
Author: Ian Rogers <irogers@google.com>

perf test: Show snippet failure output for verbose=1

This commit implements a bounded snippet processor for test failure outputs
when running in verbose mode. It aims to extract important keywords like
'error', 'fail', or 'segv' to isolate the root cause, rather than dumping
the entire raw output buffer.

> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index ad1b941731ca1..45c1afb9ad333 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c
[ ... ]
> @@ -394,6 +396,210 @@ static int print_test_result(struct test_suite *t, int curr_suite, int curr_test
[ ... ]
> +static const char *find_next_keyword(const char *str, size_t max_len, size_t *kw_len)
> +{
> +	const char *best = NULL;
> +	size_t best_len = 0;
> +	int k;
> +
> +	for (k = 0; fail_keywords[k]; k++) {
> +		const char *s = str;
> +		size_t len = strlen(fail_keywords[k]);
> +
> +		while ((size_t)(s - str) + len <= max_len) {
> +			size_t i;
> +
> +			for (i = 0; i < len; i++) {
> +				if (tolower(s[i]) != fail_keywords[k][i])

Could this trigger undefined behavior? The output stream might contain
arbitrary binary data or non-ASCII UTF-8 bytes.

Since the argument to tolower() must be representable as an unsigned char
or EOF, passing a negative value from a signed char can result in
undefined behavior.

Should s[i] be cast to an unsigned char here?

> +					break;
> +			}
[ ... ]
> +static void print_test_failure_snippet(FILE *fp, const char *buf)
> +{
[ ... ]
> +	/* Pass 0: Unconditionally pick marker lines */
> +	for (i = 0; i < num_lines; i++) {
> +		if (line_contains(lines[i], line_lens[i], "----")) {
> +			pick[i] = true;
> +			if (picked_count < failure_snippet_lines)
> +				picked_count++;
> +		}
> +	}

Does this bypass the failure_snippet_lines limit if a test outputs many
marker lines? Since pick[i] is set to true unconditionally, a test with
hundreds of marker lines might cause an unbounded terminal dump.

Additionally, if there are enough marker lines, picked_count will fully
consume the quota before Pass 1 executes. As a result, Pass 1 will be
skipped entirely:

> +
> +	/* Pass 1: Pick lines with failure keywords from start */
> +	for (i = 0; i < num_lines && picked_count < failure_snippet_lines; i++) {

Is it possible this starves the quota and prevents actual failure context
from being picked up?

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