Re: [PATCH v3 6/7] kbuild: build the tools selected in tools/Kconfig as part of "make all"

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] kbuild: selftest-vDSO spawns recursive make headers during parallel make all
- [Medium] kbuild: unfiltered find in tools_build wipes selftest-vDSO on in-tree make all
- [Medium] kbuild: make -n all fails in tools_build when CONFIG_TOOLS_BUILD_PERF is set
--

commit 77668b75cf4c2802e979bfdc034af230364fe4c5
Author: Sasha Levin <sashal@kernel.org>

kbuild: build the tools selected in tools/Kconfig as part of "make all"

This commit makes the top-level all target depend on tools_build when
CONFIG_TOOLS_BUILD is enabled so that selected tools in tools/Kconfig are
built during standard kernel builds.

> diff --git a/Makefile b/Makefile
> index 7919263484f0a..2a69f4c73a641 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1650,6 +1650,10 @@ PHONY += tools_build
>  tools_build: headers
>  	$(Q)$(MAKE) -f $(srctree)/tools/Makefile.kbuild
>  
> +ifdef CONFIG_TOOLS_BUILD
> +all: tools_build
> +endif

[Severity: Medium]
When running a parallel make all on x86_64 with CONFIG_TOOLS_BUILD=y,
CONFIG_TOOLS_BUILD_SELFTESTS_VDSO=y, and CONFIG_WERROR=y (as in allmodconfig
and allyesconfig), can this trigger an uncoordinated recursive make headers
pass in the kernel object tree that races with the main build and repeatedly
recompiles scripts/unifdef?

Once headers completes, tools_build runs tools/Makefile.kbuild in parallel
with the rest of the kernel build after unexporting sub_make_done, and invokes
each enabled selftest via:

tools/Makefile.kbuild:
$(selftest-targets): selftest-%: $(selftest-shared) FORCE
	$(call build,$(outdir)/selftests/$*,-C $(toolsdir)/testing/selftests/$* OUTPUT=$(outdir)/selftests/$* $(selftest-flags),...)

On x86, tools/testing/selftests/vDSO/Makefile declares an order-only
prerequisite on headers:

$(OUTPUT)/vdso_standalone_test_x86: vdso_standalone_test_x86.c parse_vdso.c | headers

Because tools/testing/selftests/lib.mk marks headers as .PHONY and no file
named headers exists on disk, GNU Make unconditionally runs the recipe in
lib.mk:

headers:
	$(Q)$(MAKE) -f $(top_srcdir)/Makefile -C $(khdr_output) headers

That spawns a second top-level make headers pass inside the kernel object tree
while the outer make all and other selftests are running. Because headers is
in no-dot-config-targets in the top-level Makefile, include/config/auto.conf
is not loaded in that sub-make, leaving CONFIG_WERROR unset. When
CONFIG_WERROR=y, the outer make all builds scripts/unifdef with -Werror and
-Wl,--fatal-warnings, whereas the inner make headers sees a command-line
mismatch in scripts/.unifdef.cmd and rebuilds scripts/unifdef concurrently
with the main build (and recompiles it twice on every subsequent incremental
make all or make tools_build).

[Severity: Medium]
On an in-tree make all build (without O=) with CONFIG_TOOLS_BUILD=y and
CONFIG_TOOLS_BUILD_SELFTESTS_VDSO=y or CONFIG_TOOLS_BUILD_SELFTESTS_X86=y,
will the unfiltered find in tools/Makefile.kbuild cause selftest-vDSO and
selftest-x86 to be wiped and rebuilt from scratch on the next incremental make
invocation?

In tools/Makefile.kbuild, the extra dependency directories for those targets
point into the kernel source tree:

selftest-extra-vDSO := $(abs_srctree)/arch/$(SRCARCH) $(abs_srctree)/include
selftest-extra-x86 := $(abs_srctree)/arch/x86/include

And the build macro checks for newer files across those directories without
filtering out build artifacts:

define build
	+$(Q)$(if $(dry-run),,if [ "$$(cat $(1)/.tools_build 2>/dev/null)" != '$(build-key)' ] $(if $(3),|| \
	   [ -n "$$(find $(3) -newer $(1)/.tools_build -print -quit 2>/dev/null)" ]); then \
		rm -rf $(1); \
	fi;) \

Because all: tools_build only orders tools_build after headers, the main
in-tree kernel build continues writing object files, .cmd files, generated
headers (such as include/generated/compile.h), and boot images (such as
arch/x86/boot/bzImage) under arch/$(SRCARCH) and include after
tools-build/selftests/vDSO/.tools_build is written. On the following make
invocation, find matches those newer kernel build artifacts and runs rm -rf on
the selftest output directory even when no sources or headers changed.

[Severity: Medium]
Does adding tools_build to all break make -n all on a clean tree when
CONFIG_TOOLS_BUILD=y and CONFIG_TOOLS_BUILD_PERF=y?

In tools/Makefile.kbuild, the build macro uses a leading + prefix and runs
mkdir -p before invoking the sub-make:

define build
	...
	mkdir -p $(1) && $(MAKE) $(2) && \
	$(if $(dry-run),:,printf '%s' '$(build-key)' > $(1)/.tools_build) || \

Under make -n, this creates tools-build/<target> directories on disk and
invokes tools/perf/Makefile.perf with -n in MAKEFLAGS. In
tools/perf/Makefile.perf, the sub-make pass for FIXDEP_BUILT=1 runs under
make -n (writing detection files into tools-build/perf) and evaluates the
prepare prerequisite $(LIBAPI):

$(LIBAPI): FORCE | $(LIBAPI_OUTPUT)
	$(Q)$(MAKE) -C $(LIBAPI_DIR) O=$(LIBAPI_OUTPUT) \
		DESTDIR=$(LIBAPI_DESTDIR) prefix= subdir= \
		$@ install_headers

However, the rule that creates $(LIBAPI_OUTPUT) in tools/perf/Makefile.perf:

$(LIBAPI_OUTPUT):
	$(Q)$(MKDIR) -p $@

has neither a + prefix nor $(MAKE) in its recipe, so GNU Make skips it during
make -n and never creates tools-build/perf/libapi. When the $(LIBAPI) recipe
then runs its sub-make, tools/scripts/Makefile.include checks whether $(O)
exists at parse time:

$(if $(shell cd $(PWD); test -d $(O) || echo $(O)),$(error O=$(O) does not exist),)

which aborts make -n all with an error that tools-build/perf/libapi does not
exist.

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