Re: [PATCH v2 01/12] perf tools: Add script and make target to sync the header copies with the kernel

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] perf tools: process_header() overwrites intentional tools/ header divergences
- [Medium] perf tools: setup_beauty_args() strips output file argument for syscalltbl.sh
- [Medium] perf tools: find_beauty_scripts() matches unrelated headers by basename
- [Medium] perf tools: build_target() fails when OUTPUT_DIR is a relative path
--

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

perf tools: Add script and make target to sync the header copies with the kernel

This commit adds tools/perf/scripts/sync-headers.sh and a sync-headers make
target to detect stale tools/ header copies, run beauty table generators,
build-test consumers, and create standardized sync commits.

> diff --git a/tools/perf/scripts/sync-headers.sh b/tools/perf/scripts/sync-headers.sh
> new file mode 100755
> index 0000000000000..9a35a3d2095a7
> --- /dev/null
> +++ b/tools/perf/scripts/sync-headers.sh

[ ... ]

> +# --- Map a kernel header path to its beauty script(s) --------------------
> +#
> +# A beauty script is relevant if it reads from the directory containing
> +# the header being updated.
> +
> +find_beauty_scripts() {
> +	local kernel_path="$1"
> +	local header_basename
> +	header_basename=$(basename "$kernel_path")
> +
> +	local scripts=()
> +
> +	# Search all beauty .sh scripts (including subdirs) for references to this header
> +	while IFS= read -r script; do
> +		if grep -F -q -e "/$header_basename" -e "\"$header_basename\"" \
> +			"$script" 2>/dev/null; then
> +			scripts+=("$script")
> +		fi
> +	done < <(find "$PERF_DIR"/trace/beauty -name '*.sh')
> +
> +	printf '%s\n' "${scripts[@]}"
> +}

[Severity: Medium]
Could matching only on header_basename in find_beauty_scripts() associate a
beauty script with the wrong header copy when multiple tracked headers share
the same filename?

For example, include/uapi/linux/stat.h is copied to both
tools/include/uapi/linux/stat.h and
tools/perf/trace/beauty/include/uapi/linux/stat.h, while statx_mask.sh only
reads the beauty copy. When process_header() updates
tools/include/uapi/linux/stat.h, find_beauty_scripts() matches statx_mask.sh,
and run_beauty_pass() swaps tools/include/uapi/linux/stat.h while
statx_mask.sh reads the unupdated beauty copy. Because the output does not
change, process_header() then writes:

  That don't result in changes to the string tables generated from this header.

instead of:

  That just rebuilds perf, silencing this build warning.

Similar basename collisions happen between include/uapi/linux/prctl.h and
arch/x86/include/uapi/asm/prctl.h (matching both prctl_option.sh and
x86_arch_prctl.sh), arch/{x86,powerpc,s390,arm64}/include/uapi/asm/kvm.h
(matching kvm_ioctl.sh, which only reads tools/include/uapi/linux/kvm.h), and
arch/mips/include/asm/errno.h (matching arch_errno_names.sh).

Also, setup_beauty_args() only reads tools/perf/trace/beauty/Build and matches
lowercase variable names in re_tbl:

	local build="$PERF_DIR/trace/beauty/Build"
	...
	re_tbl='^([a-z0-9_]+_tbl[s]?)[[:space:]]*(:=|\?=)[[:space:]]*'

Should setup_beauty_args() also parse
tools/perf/trace/beauty/tracepoints/Build and allow uppercase characters in
variable names so x86_irq_vectors.sh and x86_msr.sh (which uses
x86_arch_MSRs_tbl) receive their directory arguments?

[ ... ]

> +# Keep build output out of the source tree; perf, tracing and objtool take
> +# O=, the libraries and bpftool take OUTPUT=.
> +build_target() {
> +	local name="$1"
> +	local dir="$2"
> +
> +	case "$name" in
> +	perf)
> +		make -C "$dir" O="$OUTPUT_DIR" -j"$(nproc)"
> +		;;
> +	tracing|objtool)
> +		(umask 077 && mkdir -p "$OUTPUT_DIR/$name")
> +		make -C "$dir" O="$OUTPUT_DIR/$name" -j"$(nproc)"
> +		;;
> +	*)
> +		(umask 077 && mkdir -p "$OUTPUT_DIR/$name")
> +		make -C "$dir" OUTPUT="$OUTPUT_DIR/$name/" -j"$(nproc)"
> +		;;
> +	esac
> +}

[Severity: Medium]
What happens in build_target() when a user passes a relative path via -o,
--output, or OUTPUT_DIR?

In main(), the script changes directory to $KERNEL_DIR and creates
$OUTPUT_DIR without converting it to an absolute path:

	cd "$KERNEL_DIR"
	...
	if [[ $BUILD -eq 1 ]]; then
		# umask covers every level created, -m with -p only the last.
		(umask 077 && mkdir -p "$OUTPUT_DIR")
	fi

When build_test_targets() or process_group() then calls build_target(),
make -C "$dir" changes the working directory to the target subdirectory (such
as tools/perf, tools/objtool, or tools/lib/bpf) before evaluating O= or
OUTPUT=. In tools/scripts/Makefile.include, the existence check resolves that
relative path from $dir rather than from $KERNEL_DIR (or the caller's working
directory), causing the build test to fail and sync-headers.sh to revert the
synced header. Should OUTPUT_DIR be canonicalized to an absolute path before
main() changes directory?

[ ... ]

> +		for token in $args; do
> +			case "$token" in
> +			'$@')
> +				continue
> +				;;
> +			'$(srctree)'*)
> +				token="${token//\$(srctree)/$KERNEL_DIR}"
> +				;;
> +			'$('*)
> +				token="$KERNEL_DIR/${BEAUTY_MAKE_DIRS["${token:2:${#token}-3}"]:-}"
> +				[[ "$token" == "$KERNEL_DIR/" ]] && { value=""; break; }
> +				;;
> +			*) ;;
> +			esac
> +			token=$(printf '%s' "$token" | sed "s/\$(SRCARCH)/$srcarch/g")
> +			if [[ "$token" == *'$('* ]]; then
> +				value=""
> +				break
> +			fi
> +			value+="${value:+ }$token"
> +		done
> +		[[ -n "$value" ]] || continue
> +		BEAUTY_SCRIPT_ARGS["$script"]="$value"
> +	done
> +}
> +
> +run_beauty_script() {
> +	local script="$1"
> +	local -a cmd=()
> +
> +	read -ra cmd <<< "${BEAUTY_SCRIPT_ARGS["${script##*/}"]:-}"
> +	bash "$script" "${cmd[@]+"${cmd[@]}"}" 2>/dev/null || true
> +}

[Severity: Medium]
Will run_beauty_script() ever capture string table diffs for syscalltbl.sh?

In tools/perf/trace/beauty/Build, syscalltbl.sh is invoked with $@ as a
positional output file argument rather than via stdout redirection:

	$(Q)$(call echo-cmd,gen)$(SHELL) '$(syscall_tbl)' $(srctree)/tools $@

And tools/perf/trace/beauty/syscalltbl.sh requires two arguments and writes
directly to the output file:

	if [ $# -ne 2 ]; then
		usage
	fi
	tools_dir=$1
	outfile=$2

Because setup_beauty_args() strips the $@ token,
BEAUTY_SCRIPT_ARGS["syscalltbl.sh"] only contains $KERNEL_DIR/tools. When
process_group() -> run_beauty_pass() -> run_beauty_script() runs
syscalltbl.sh, it fails the argument count check, discards the usage message
via 2>/dev/null, and ignores the non-zero status via || true. That leaves the
before and after files empty, so process_group() always reports:

  That don't result in changes to the string tables generated from these headers.

even when syscall tables have new or modified entries.

[ ... ]

> +	# Do the actual copy
> +	if ! cp "$KERNEL_DIR/$kernel_path" "$KERNEL_DIR/$tools_path"; then
> +		err "Failed to copy $kernel_path -> $tools_path"
> +		COPY_FAILED+=("$tools_path")
> +		cleanup_tmpdir "$beauty_tmpdir"
> +		return 1
> +	fi
> +	info "Copied $kernel_path -> $tools_path"

[Severity: Medium]
Does copying $kernel_path directly over $tools_path in process_header()
clobber intentional tools/ adaptations in files that check-headers.sh
compares with ignore rules?

In tools/perf/check-headers.sh, several tracked files intentionally differ
between the kernel and tools/ and are checked with -I patterns or
check_ignore_some_hunks:

	for i in "${SYNC_CHECK_FILES[@]}"
	do
	  check "$i" '-I "^.*\/\*.*__ignore_sync_check__.*\*\/.*$"'
	done

along with arch/x86/include/asm/amd/ibs.h, include/uapi/asm-generic/mman.h,
include/uapi/linux/mman.h, include/linux/build_bug.h, include/linux/ctype.h,
include/linux/unaligned.h, lib/ctype.c, arch/x86/lib/memcpy_64.S,
arch/x86/lib/memset_64.S, and lib/list_sort.c.

When check-headers.sh reports other differences in one of those files,
process_header() overwrites $tools_path with a verbatim copy of $kernel_path,
removing the tools/-specific changes (such as mman-common-tools.h in
tools/include/uapi/asm-generic/mman.h, the static_assert guard in
tools/include/linux/build_bug.h, __isdigit in tools/include/linux/ctype.h,
relative includes marked with __ignore_sync_check__ in x86 insn/inat files,
or omitted kernel hunks in tools/lib/list_sort.c). If the host build still
passes without those adaptations, process_header() will commit the clobbered
file.

In addition, find_sync_point() assumes that files with intentional differences
never had matching blob hashes and will fall through to the last commit
touching $tools_path:

	for ((i = 0; i < ${#candidates[@]}; i++)); do
		if [[ "${blobs[2*i]%% *}" == "${blobs[2*i+1]%% *}" && \
		      "${blobs[2*i]%% *}" != *:* ]]; then
			echo "${candidates[i]}"
			return
		fi
	done

	git log --no-merges --format='%H' -1 -- "$tools_path" 2>/dev/null

For files that were initially copied verbatim and only later adapted for
tools/ (for example, tools/include/uapi/asm-generic/mman.h, which was
byte-identical in commit fb7df12d645c before commit be709d48329a added
mman-common-tools.h), doesn't find_sync_point() match that older
pre-divergence commit and cause find_kernel_commits() to list and Cc commits
that were already synced?

[ ... ]

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