Re: [PATCH v5 4/4] perf tools: Add test for weight annotation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The script conflates PEBS support checking with workload execution, masking workload failures as skipped tests.
- [Medium] Silent test failure due to unhandled `grep -q` execution under `set -e`.
--

commit 8db1611c3b92cee56576e6157914227faf90fc5a
Author: Andi Kleen <ak@linux.intel.com>

perf tools: Add test for weight annotation

This commit adds a shell script test case to check if the --stdio weight
annotation functions correctly in perf annotate.

> diff --git a/tools/perf/tests/shell/annotate_weight.sh b/tools/perf/tests/shell/annotate_weight.sh
> new file mode 100755
> index 0000000000000..6b8c105c048b5
> --- /dev/null
> +++ b/tools/perf/tests/shell/annotate_weight.sh
[ ... ]
> +trap 'cleanup; exit 1' TERM INT
> +trap cleanup EXIT
> +
> +# mem-loads:pu requests a precise user PEBS event whose sample weight should
> +# be populated by -W. Unsupported PEBS/weight PMUs are skipped below.
> +if ! perf record -W -e mem-loads:pu -o "${perfdata}" -- perf test -w thloop \
> +	> /dev/null 2> "${record_log}"; then
> +	echo "[SKIP] precise PEBS weight sampling is unavailable"
> +	exit 2
> +fi

[Severity: Medium]
If the inner workload (perf test -w thloop) crashes or otherwise fails to
execute, does this conditional block improperly catch the failure and report 
it as an unsupported hardware feature? 

It looks like any non-zero exit status from the child process will result 
in a skipped test, which might mask underlying environment or test suite bugs.

[ ... ]
> +	echo "[SKIP] PEBS weight sampling produced no nonzero weights"
> +	exit 2
> +fi
> +
> +perf annotate --stdio -i "${perfdata}" --symbol "${testsym}" \
> +	> "${annotate_out}" 2> "${record_log}"
> +
> +grep -q 'Percent Weight' "${annotate_out}"

[Severity: Medium]
Since set -e is enabled at the top of the script, will this isolated grep 
command cause an abrupt and silent exit if the 'Percent Weight' header is 
missing from the annotation output? 

If the string is missing, grep will exit with code 1, which might instantly 
abort the script without printing diagnostic output, making regressions harder 
to debug.

> +
> +# The second numeric column is the rendered average weight. This assertion
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260830193459.2840519-1-ak@linux.intel.com?part=4