Re: [RFC PATCH v2 5/7] tools: build kernel selftests in tools_build

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] build: Fatal Make syntax error on empty target variables in static pattern rules
- [Medium] selftests: kexec Kconfig unnecessarily restricted to X86_64
- [Medium] kbuild: missing dependencies on scripts/ Makefiles for selftests rebuilds
--

commit f8d06182dd0adbe69406863fbe53894162f78089
Author: Sasha Levin <sashal@kernel.org>
Subject: tools: build kernel selftests in tools_build

This commit adds a "Kernel selftests" sub-menu to the tools build framework,
allowing selftests to be built without writing to the source tree. Each
selftest runs under its own make instance, enabling parallel builds using a
pre-installed C library and UAPI headers. Rebuild dependencies are handled by
tracking timestamps of shared files and headers since selftests do not
natively track header dependencies.

> diff --git a/tools/Kconfig b/tools/Kconfig
> --- a/tools/Kconfig
> +++ b/tools/Kconfig

[ ... ]

> +config TOOLS_BUILD_SELFTESTS_KEXEC
> +	bool "kexec"
> +	depends on X86_64

[Severity: Medium]
Does this unnecessarily restrict the kexec selftest to X86_64?

Looking at tools/testing/selftests/kexec/Makefile, it explicitly supports both
x86 (32-bit and 64-bit) and ppc64le:

    ifeq ($(ARCH_PROCESSED),$(filter $(ARCH_PROCESSED),x86 ppc64le))
    TEST_PROGS := test_kexec_load.sh test_kexec_file_load.sh
    TEST_FILES := kexec_common_lib.sh

Configuring the tools build on ppc64le or 32-bit x86 will not expose this
option, silently excluding the test from being built and installed on these
platforms.

[ ... ]

> diff --git a/tools/Makefile.kbuild b/tools/Makefile.kbuild
> --- a/tools/Makefile.kbuild
> +++ b/tools/Makefile.kbuild

[ ... ]

> +$(selftest-shared): FORCE
> +	$(Q)mkdir -p $(@D); \
> +	newest=$$(find $(CURDIR)/usr/include $(toolsdir)/include $(toolsdir)/arch $(toolsdir)/testing/selftests \
> +		-type f \( -name '*.h' -o -name '*.mk' -o -name Makefile \) -printf '%T@\n' | \
> +		sort -n | tail -n 1); \
> +	touch -d "@$${newest:-0}" $@

[Severity: Medium]
Will this miss rebuild dependencies on shared build scripts?

The find command omits the tools/scripts/ and scripts/ directories, and its
filters exclude files named Makefile.* or *.include.

Modifying core shared files like tools/scripts/Makefile.include,
tools/scripts/Makefile.arch, or scripts/subarch.include will not trigger a
rebuild of selftests that include them, which can lead to stale builds.

> +
> +# As with "make kselftest-all", a selftest that fails to build does not fail
> +# the build unless FORCE_TARGETS is set. kselftest_deps.sh lists the libraries
> +# it needs that the compiler cannot link with. It leaves files behind in
> +# TMPDIR, so it gets one of its own.
> +selftest-failed = (cd $(toolsdir)/testing/selftests && tmp=$$(mktemp -d) && \
> +	TMPDIR=$$tmp ./kselftest_deps.sh '$(call escsq,$(tools-cc))' $(1) | grep '^FAIL' >&2; \
> +	rm -rf "$$tmp"); $(if $(FORCE_TARGETS),false,:)
> +
> +selftest-targets := $(addprefix selftest-,$(selftests-y))
> +
> +__tools_build: $(selftest-targets)
> +
> +PHONY += $(selftest-targets)
> +$(selftest-targets): selftest-%: $(selftest-shared) FORCE
> +	$(call build,$(outdir)/selftests/$*,-C $(toolsdir)/testing/selftests/$* OUTPUT=$(outdir)/selftests/$* $(selftest-flags),$(selftest-shared) $(toolsdir)/testing/selftests/$* $(selftest-extra-$*),TOOLS_BUILD_SELFTESTS_$(shell echo $* | tr a-z/- A-Z__),$(call selftest-failed,$*))

[Severity: High]
Can this trigger a fatal Make syntax error if no selftests are selected?

If a user enables the tools build selftests option but disables all individual
selftests, $(selftests-y) and $(selftest-targets) will be empty.

Since this rule uses two colons, it is a static pattern rule. While GNU Make
ignores empty targets for single-colon rules, an empty target list in a
double-colon static pattern rule evaluates to : selftest-%: ... and
triggers a fatal missing target pattern. Stop. build error.

The same issue would apply to the $(tool-targets): tool-%: FORCE rule if
the tools build is enabled but no individual tools are selected.

Should these static pattern rules be guarded with an ifneq check
(e.g., ifneq ($(selftest-targets),)) to prevent breaking the build on
minimal configurations?

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