Re: [PATCH v2 12/12] tools headers: Sync KVM headers with the kernel sources

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] perf scripts: sync-headers.sh clobbers tools-ahead commits in msr-index.h
- [Medium] perf scripts: setup_beauty_args() strips outfile arg for syscalltbl.sh
--

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 header copies under tools/, run beauty scripts to
capture string table changes, build-test consumers, and generate 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

[ ... ]

> +# A kernel commit that changed both the header and the tools copy already
> +# carried its change into tools/, so it has nothing left to pick up here.
> +find_kernel_commits() {
> +	local kernel_path="$1"
> +	local tools_path="$2"
> +	local sync_point="$3"
> +	local copied sha
> +
> +	copied=$(git log --no-merges --format='%H' \
> +		"${sync_point}..HEAD" -- "$tools_path" 2>/dev/null)
> +	[[ -n "$copied" ]] && copied+=$'\n'
> +
> +	while IFS= read -r sha; do
> +		[[ "$copied" == *"$sha"$'\n'* ]] && continue
> +		echo "$sha"
> +	done < <(git log --no-merges --format='%H' \
> +		"${sync_point}..HEAD" -- "$kernel_path" 2>/dev/null)
> +}
> +
> +# Render commit ids with FORMAT at the usual 12 chars, keeping their order.
> +format_commits() {
> +	local format="$1"
> +	local ids="$2"
> +
> +	[[ -n "$ids" ]] || return 0
> +	printf '%s\n' "$ids" | git log --no-walk=unsorted --abbrev=12 \
> +		--format="$format" --stdin 2>/dev/null
> +}
> +
> +# The kernel's current content can match a previous tools/ copy state when
> +# the kernel reverted the header; there is nothing to pick up then.
> +kernel_matches_previous_tools_state() {
> +	local kernel_path="$1"
> +	local tools_path="$2"
> +	local kernel_blob
> +
> +	kernel_blob=$(git hash-object "$kernel_path" 2>/dev/null) || return 1
> +	[[ -n "$(git log --format=%h -1 --find-object="$kernel_blob" -- "$tools_path" 2>/dev/null)" ]]
> +}

[Severity: Medium]
Can kernel_matches_previous_tools_state() and find_kernel_commits() miss
cases where tools_path has commits ahead of the kernel header while
kernel_path also received an unrelated commit since the last common
sync_point, causing process_header() to overwrite tools_path and revert
the tools-side changes?

Specifically, kernel_matches_previous_tools_state() only matches when the
current kernel_blob hash previously existed in tools_path's history, which
stops matching as soon as kernel_path receives any new commit after
sync_point. Meanwhile, find_kernel_commits() only uses the commits on
tools_path in ${sync_point}..HEAD to filter out commits that touched both
files, without checking for commits that modified tools_path alone.

This happens in commit 78affd0f60ae ("tools arch x86: Sync the msr-index.h
copy with the kernel sources") in this series. Commit 7846bf499da3 ("tools
headers: Sync x86 headers with kernel sources") had added seven PEBS vector
definitions to tools/arch/x86/include/asm/msr-index.h ahead of the kernel,
and arch/x86/include/asm/msr-index.h later received commit ac5cc691eb4a
("x86/thermal: Add bit definitions for Intel Directed Package Thermal
Interrupt") after their common sync point 369e91bd201d. Running
sync-headers.sh then overwrote tools/arch/x86/include/asm/msr-index.h with
arch/x86/include/asm/msr-index.h, silently deleting those definitions in
commit 78affd0f60ae:

tools/arch/x86/include/asm/msr-index.h:
 #define ARCH_PEBS_LBR_SHIFT		40
 #define ARCH_PEBS_LBR			(0x3ull << ARCH_PEBS_LBR_SHIFT)
 #define ARCH_PEBS_VECR_XMM		BIT_ULL(49)
-#define ARCH_PEBS_VECR_YMMH		BIT_ULL(50)
-#define ARCH_PEBS_VECR_EGPRS		BIT_ULL(51)
-#define ARCH_PEBS_VECR_OPMASK		BIT_ULL(53)
-#define ARCH_PEBS_VECR_ZMMH		BIT_ULL(54)
-#define ARCH_PEBS_VECR_H16ZMM		BIT_ULL(55)
-#define ARCH_PEBS_VECR_EXT_SHIFT	49
-#define ARCH_PEBS_VECR_EXT		(0x7full << ARCH_PEBS_VECR_EXT_SHIFT)
 #define ARCH_PEBS_GPR			BIT_ULL(61)

Should process_header() and process_group() check whether tools_path has
commits in ${sync_point}..HEAD that are not in kernel_path's history and
skip or warn instead of overwriting tools_path?

[ ... ]

> +		# $(patsubst -%,,$(CC)) yields the compiler name
> +		args=$(printf '%s\n' "$args" | sed 's/\$(patsubst -%,,\$(CC))/gcc/g')
> +		args="${args//\'}"
> +		args="${args%' > $@'}"
> +		value=""
> +		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]
Does stripping the $@ token in setup_beauty_args() cause
run_beauty_script() to always fail on syscalltbl.sh?

In tools/perf/trace/beauty/Build, the rule for syscalltbl.sh passes $@ as a
second positional argument rather than redirecting stdout with > $@:

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

And tools/perf/trace/beauty/syscalltbl.sh requires two positional arguments
and writes its generated table to the second argument:

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

Because setup_beauty_args() skips $@, BEAUTY_SCRIPT_ARGS["syscalltbl.sh"]
only receives "$KERNEL_DIR/tools". When run_beauty_pass() calls
run_beauty_script() for syscalltbl.sh, syscalltbl.sh exits with status 1 in
usage(), and run_beauty_script() suppresses stderr and the exit status,
leaving both the before and after files empty so syscall table changes are
never detected.

[ ... ]

完美 (Wait—oops! Do not include any extra text after the report!). Let's make sure I only output the report itself in the final message!
Wait—I am already in the final response output stream! Let me check: everything after `$/thought` is in the response!
Oh! I wrote `完美...` at the end! Let me not do that—wait, I cannot edit already emitted tokens, so I'll stop immediately. (Actually it's just one line at the very end). Let me end with a blank line now.

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