Re: [PATCH v3 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] The patch introduces an unused helper function `line_contains` which causes a `-Werror` build failure, and Pass 0 fails to search for execution boundary markers as claimed in the commit message.
- [Medium] Passing a signed `char` directly to `tolower()` invokes undefined behavior and can cause out-of-bounds memory reads for non-ASCII characters.
--

commit 341f346c5efffc91ead705378b8412e2de4cf957
Author: Ian Rogers <irogers@google.com>

perf test: Show snippet failure output for verbose=1

This patch introduces a bounded snippet processor to limit the amount of
output dumped to stderr when a test case fails in verbose mode, extracting
lines that contain relevant failure keywords.

> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index 9f71f11928c60..a29925656865b 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c

[ ... ]

> @@ -398,6 +400,204 @@ static int print_test_result(struct test_suite *t, int curr_suite, int curr_test
>  	return 0;
>  }
>  
> +static const char * const fail_keywords[] = {
> +	"error", "fail", "segv", "abort",
> +	"signal", "fatal", "panic", "corrupt", NULL
> +};
> +
> +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])
> +					break;
> +			}

[Severity: Medium]
Is it safe to pass s[i] directly to tolower() without casting to unsigned
char first? 

If s[i] is a signed char containing a negative value, such as non-ASCII UTF-8
sequences or binary garbage from a crashing test case, could this invoke
undefined behavior and lead to out-of-bounds array lookups in libc?

[ ... ]

> +static bool line_contains(const char *line, size_t len, const char *tok)
> +{
> +	size_t tok_len = strlen(tok);
> +	const char *s = line;
> +
> +	while ((size_t)(s - line) + tok_len <= len) {
> +		if (memcmp(s, tok, tok_len) == 0)
> +			return true;
> +		s++;
> +	}
> +	return false;
> +}

[Severity: Medium]
This function appears to be completely unused. Does this cause a build failure
when the tree is compiled with -Werror=unused-function, which is typically
enabled by default for perf tools?

[ ... ]

> +	/* Pass 0: Always pick the very first line */
> +	if (num_lines > 0 && picked_count < failure_snippet_lines) {
> +		pick[0] = true;
> +		picked_count++;
> +	}

[Severity: Medium]
The commit message mentions that Pass 0 prioritizes "retaining outline marker
lines containing dashes (====)". However, this implementation just
unconditionally selects the first line instead. 

Was the unused line_contains() function above originally intended to be used
here to search for the execution boundary markers?

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