Re: [PATCH v1 01/12] perf scripts: Add a coverage harness for perf

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential
issues to consider:
- [Low] perf scripts: llvm_cov_objects() looks for perf.so in wrong
  directory
- [Low] perf scripts: misplaced delta header and bad alignment in
  print_summary()
- [Low] perf scripts: print_uncovered() fails to strip tools/perf/ path
  prefix
- [Low] perf build: inconsistent O= handling in Makefile coverage
  targets
- [Low] perf scripts: locale-sensitive float handling in print_table()
- [Low] perf scripts: print_worst_files() checks total lines instead of
  missed
--

commit e4ee2d1add56afe4f156fafd475dce263c39b01d
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf scripts: Add a coverage harness for perf

This commit adds tools/perf/scripts/coverage.sh and corresponding make
targets to build perf with clang source-based coverage, run the perf test
suite, and generate line and function coverage reports and ASCII tables.

> diff --git a/tools/perf/Makefile b/tools/perf/Makefile
> index 00baf90e5bda7..d00b63ca011c2 100644
> --- a/tools/perf/Makefile
> +++ b/tools/perf/Makefile
> @@ -140,6 +140,15 @@ check-headers:
>  sync-headers:
>  	@./scripts/sync-headers.sh $(SYNC_HEADERS_ARGS)
>  
> +coverage-prep:
> +	@./scripts/coverage.sh --prep $(if $(O),--build-dir=$(O)) $(COVERAGE_ARGS)
> +
> +coverage:
> +	@./scripts/coverage.sh $(COVERAGE_ARGS)
> +
> +coverage-table:
> +	@./scripts/coverage.sh --table $(COVERAGE_ARGS)

[Severity: Low]
Should coverage and coverage-table also pass --build-dir, and should all
three targets use $(FULL_O) instead of $(O)?

Without --build-dir, running make -C tools/perf O=/path/to/build coverage
(or coverage-table) ignores O= and falls back to the default build
directory.  In addition, passing raw $(O) to coverage.sh resolves relative
paths against tools/perf rather than the caller's working directory,
whereas $(FULL_O) canonicalizes $(O) against $(PWD).

[ ... ]

> diff --git a/tools/perf/scripts/coverage.sh b/tools/perf/scripts/coverage.sh
> new file mode 100755
> index 0000000000000..19b0da22000c3
> --- /dev/null
> +++ b/tools/perf/scripts/coverage.sh

[ ... ]

> +llvm_cov_objects()
> +{
> +	echo "$BUILD_DIR/perf"
> +	local so
> +	for so in perf.so libperf.so libperf-jvmti.so; do
> +		[ -f "$BUILD_DIR/$so" ] && echo "--object=$BUILD_DIR/$so"
> +	done
> +}

[Severity: Low]
Does this miss the Python C extension object?  Makefile.perf builds the
extension with PYTHON_EXTENSION_SUFFIX (for example,
perf.cpython-312-x86_64-linux-gnu.so) and copies it into
$(OUTPUT)python/, so $BUILD_DIR/perf.so does not exist and coverage for
util/python.c is omitted from the reports.  Matching
$BUILD_DIR/python/perf*.so would include it.

[ ... ]

> +print_worst_files()
> +{
> +	[ "$TOP" -gt 0 ] || return 0
> +	local rows missed lines cover fn path
> +	rows=$(awk -v min="$MIN_LINES" '
> +		$1 ~ /(^|\/)tools\/perf\// && $8 >= min && $8 > 0 {
> +			path = $1
> +			sub(/^.*tools\/perf\//, "", path)
> +			printf "%s %s %s %s %s\n", $9, $8, $10, $7, path
> +		}' "$REPORT" | sort -k1,1nr -k2,2nr | sed -n "1,${TOP}p")

[Severity: Low]
Should the second awk check be $9 > 0 (missed lines) instead of $8 > 0
(total lines), matching table_rows()?  Checking $8 > 0 is redundant with
$8 >= min and allows files with 0 missed lines to be listed under largest
uncovered files.

[ ... ]

> +	if [ "$USE_BASELINE" = 1 ] && [ -f "$BASELINE" ]; then
> +		printf "  %-8s %8s  %8s  %9s\n" "File" "Previous" "Current" "Delta"
> +		printf "  %-8s %8s  %8s  %9s\n" "--------" "--------" "--------" "---------"
> +	fi
> +
> +	printf "  \033[1m%-44s\033[0m %8s  %8s  %8s  %8s\n" \
> +		"file" "lines" "cover" "missed" "fn-cover"
> +	for file in "${FOCUS[@]}"; do
> +		cov=$(file_cov "$REPORT" "$file")
> +		if [ -z "$cov" ]; then
> +			printf "  %-36s %8s\n" "$file" "not covered"
> +			continue
> +		fi
> +		printf "  %-36s %8s  %6s  %6s  %8s\n" "$file" \
> +			"$(echo "$cov" | awk '{print $1}')" \
> +			"$(echo "$cov" | awk '{print $3}')" \
> +			"$(echo "$cov" | awk '{print $2}')" \
> +			"$(echo "$cov" | awk '{print $4}')"
> +	done
> +
> +	if [ "$USE_BASELINE" = 1 ] && [ -f "$BASELINE" ]; then
> +		printf "\n  per file delta since %s\n" "$BASELINE"
> +		for file in "${FOCUS[@]}"; do
> +			report_delta "$BASELINE" "$REPORT" "$file"
> +		done
> +	fi
> +}

[Severity: Low]
Are the baseline delta header and row widths in print_summary()
misaligned when -f is used?

The 4-column "File Previous Current Delta" header is printed right above
the 5-column focus table instead of in the "per file delta since
$BASELINE" block below where report_delta() is called.  In the 5-column
focus table itself, the header uses "%-44s %8s  %8s  %8s  %8s", while the
rows use "%-36s %8s  %6s  %6s  %8s", shifting the numeric columns out of
alignment.

Also, because update_baseline() copies $REPORT to $BASELINE at the end of
a run (moving the prior baseline to $BASELINE.prev), a subsequent
--report-only run compares $REPORT against $BASELINE (itself) and always
reports a +0.00% delta.

[ ... ]

> +table_rows()
> +{
> +	if [ ${#FOCUS[@]} -gt 0 ]; then
> +		local file
> +		for file in "${FOCUS[@]}"; do
> +			table_row "$REPORT" "$file"
> +		done | sort -t$'\t' -k5,5gr
> +		return
> +	fi
> +
> +	awk -v min="$MIN_LINES" '
> +		$1 ~ /(^|\/)tools\/perf\// && $8 >= min && $9 > 0 {
> +			path = $1
> +			sub(/^.*tools\/perf\//, "", path)
> +			printf "%s\t%s\t%d\t%d\t%.2f\t%s\t%d\t%s\n",
> +			       path, $8, $8 - $9, $9, $10, $5, $5 - $6, $7
> +		}' "$REPORT" |
> +		sort -t$'\t' -k4,4nr |
> +		if [ "$TOP" -gt 0 ]; then sed -n "1,${TOP}p"; else cat; fi |
> +		sort -t$'\t' -k5,5gr
> +}
> +
> +print_table()
> +{
> +	local rows total path lines hit missed pct funcs funcs_hit func_cov
> +	local row_fmt w d_file d29 d13 d10 d8
> +
> +	rows=$(table_rows)

[ ... ]

> +	row_fmt=$(printf '│ %%-%ds │ %%6s │ %%6s │ %%6s │ %%s %%5.1f%%%%' "$w")
> +	row_fmt+=' │ %5s/%-5s │ %8s │\n'
> +
> +	while IFS=$'\t' read -r path lines hit missed pct funcs funcs_hit func_cov; do
> +		printf "$row_fmt" "$path" "$lines" "$hit" "$missed" \
> +			"$(bar "$pct" 20)" "$pct" "$funcs_hit" "$funcs" "$func_cov"
> +	done <<< "$rows"

[Severity: Low]
Does --table fail under locales where LC_NUMERIC uses a comma decimal
separator (for example, de_DE.UTF-8 or pt_BR.UTF-8)?

awk emits dot-separated floats in $pct (such as "33.53"), which sort -g
and bash's built-in printf "%5.1f" then parse according to LC_NUMERIC.
When LC_NUMERIC expects a comma, printf rejects $pct as an invalid number
and exits with status 1, aborting the script under set -e.  Exporting
LC_ALL=C or LC_NUMERIC=C at the top of the script would avoid this.

[ ... ]

> +print_uncovered()
> +{
> +	[ "$UNCOVERED" -gt 0 ] || return 0
> +	[ ${#FOCUS[@]} -gt 0 ] || return 0
> +	local objects file out lines header=0
> +	mapfile -t objects < <(llvm_cov_objects)
> +
> +	for file in "${FOCUS[@]}"; do
> +		# No report row means the file was not built: print_summary()
> +		# already said so, llvm-cov show would dump the whole profile.
> +		[ -n "$(file_cov "$REPORT" "$file")" ] || continue
> +		out=$("$LLVM_COV" show "${objects[@]}" --instr-profile="$PROFDATA" \
> +			"$PERF_DIR/$file" 2>/dev/null |

[Severity: Low]
Should print_uncovered() strip a leading .*tools/perf/ prefix from $file
before prepending $PERF_DIR/?

file_cov() and table_row() both strip ^.*tools/perf/ so that paths like
-f tools/perf/util/evsel.c work, but passing "$PERF_DIR/$file" directly to
llvm-cov show produces "$PERF_DIR/tools/perf/util/evsel.c" and fails to
find the source file.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113150555.1163049-1-acme@kernel.org?part=1