Re: [PATCH v5 02/11] kbuild: implement and use depcheck to check dependency timestamps

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] kbuild: missing fixdep prerequisite for depcheck in scripts/basic/Makefile
- [Medium] kbuild: concurrent Makefile.build runs race on .depcheck.mk.tmp and .depcheck.mk
- [Low] kbuild: install-extmod-build leaves .depcheck.mk files in packaged trees
--

commit f2af2ee8757f13fb26fb18de8c51c7a2f612cd46
Author: Lorenzo Stoakes (ARM) <ljs@kernel.org>

kbuild: implement and use depcheck to check dependency timestamps

This commit introduces scripts/basic/depcheck to check header dependency
timestamps in C and emit a condensed .depcheck.mk fragment containing only
saved command lines for up-to-date targets. scripts/Makefile.build runs
depcheck before including .cmd files, falling back to the original .cmd
files if depcheck fails.

> diff --git a/Makefile b/Makefile
> index f895752d4b05f..d92a5d78b4659 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2252,6 +2252,7 @@ clean: $(clean-dirs)
>  	$(call cmd,rmfiles)
>  	@find . $(RCS_FIND_IGNORE) \
>  		\( -name '*.[aios]' -o -name '*.rsi' -o -name '*.ko' -o -name '.*.cmd' \
> +		-o -name '.depcheck.mk' -o -name '.depcheck.mk.tmp' \
>  		-o -name '*.ko.*' -o -name '*.o.thinlto.bc' \
>  		-o -name '*.dtb' -o -name '*.dtbo' \
>  		-o -name '*.dtb.S' -o -name '*.dtbo.S' \

[Severity: Low]
While the top-level clean target removes .depcheck.mk and .depcheck.mk.tmp
alongside .*.cmd, scripts/package/install-extmod-build only deletes .*.cmd
and *.o after copying and rebuilding scripts/ for external module packaging:

scripts/package/install-extmod-build:
	find "${destdir}" \( -name '.*.cmd' -o -name '*.o' \) -delete

When modules_prepare or the cross-compilation step in install-extmod-build
invokes $(MAKE) $(build)=scripts, depcheck generates .depcheck.mk files
under scripts/. Should scripts/package/install-extmod-build also remove
.depcheck.mk and .depcheck.mk.tmp so those build artifacts are not left in
packaged kernel-devel and linux-headers trees?

> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index bbe9555c47a8b..7b652f169a8fd 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -613,7 +613,20 @@ $(obj)/: $(if $(KBUILD_BUILTIN), $(targets-for-builtin)) \
>  
>  existing-targets := $(wildcard $(sort $(targets)))
>  
> --include $(foreach f,$(existing-targets),$(dir $(f)).$(notdir $(f)).cmd)
> +cmd-files := $(wildcard $(foreach f,$(existing-targets),$(dir $(f)).$(notdir $(f)).cmd))
> +
> +# depcheck checks timestamps first and outputs only saved command lines of
> +# up-to-date targets, falling back to .cmd files if it fails. A directory
> +# whose targets all lie elsewhere may not exist in the output tree.
> +depcheck := $(wildcard $(objtree)/scripts/basic/depcheck)
> +
> +ifneq ($(and $(depcheck),$(cmd-files),$(wildcard $(obj))),)
> +ifeq ($(shell $(depcheck) $(obj)/.depcheck.mk $(cmd-files) && echo ok),ok)
> +cmd-files := $(obj)/.depcheck.mk
> +endif
> +endif
> +
> +-include $(cmd-files)

[ ... ]

> diff --git a/scripts/basic/Makefile b/scripts/basic/Makefile
> index fb8e2c38fbc78..ff98780e474d9 100644
> --- a/scripts/basic/Makefile
> +++ b/scripts/basic/Makefile
> @@ -2,7 +2,7 @@
>  #
>  # fixdep: used to generate dependency information during build process
>  
> -hostprogs-always-y	+= fixdep
> +hostprogs-always-y	+= fixdep depcheck

[Severity: Medium]
Can a parallel build from a clean tree fail when building depcheck before
fixdep is ready?

Both fixdep and depcheck are single-file host programs built by the
host-csingle rule in scripts/Makefile.host:

$(host-csingle): $(obj)/%: $(obj)/%.c FORCE
	$(call if_changed_dep,host-csingle)

which expands cmd_and_fixdep in scripts/Kbuild.include:

cmd_and_fixdep =                                                             \
	$(cmd);                                                              \
	$(objtree)/scripts/basic/fixdep $(depfile) $@ '$(make-cmd)' > $(dot-target).cmd;\
	rm -f $(depfile)

Without an explicit prerequisite ordering $(obj)/depcheck after
$(obj)/fixdep, make -jN can compile fixdep and depcheck concurrently during
scripts_basic. If depcheck finishes compiling before fixdep has finished
linking, cmd_and_fixdep will attempt to execute scripts/basic/fixdep while
it does not yet exist or is still open for writing.

Should scripts/basic/Makefile declare that $(obj)/depcheck depends on
$(obj)/fixdep?

> diff --git a/scripts/basic/depcheck.c b/scripts/basic/depcheck.c
> new file mode 100644
> index 0000000000000..119075353f3bc
> --- /dev/null
> +++ b/scripts/basic/depcheck.c

[ ... ]

> +int main(int argc, char **argv)
> +{
> +	char tmp_path[PATH_MAX];
> +	FILE *out;
> +	int i;
> +
> +	if (argc < 2) {
> +		fprintf(stderr, "usage: %s <output> <.cmd files...>\n", argv[0]);
> +		return 1;
> +	}
> +
> +	if (snprintf(tmp_path, sizeof(tmp_path), "%s.tmp", argv[1]) >=
> +	    (int)sizeof(tmp_path)) {
> +		fprintf(stderr, "%s: path too long\n", argv[1]);
> +		return 1;
> +	}
> +
> +	out = fopen(tmp_path, "w");
> +	if (!out) {
> +		perror(tmp_path);
> +		return 1;
> +	}
> +
> +	for (i = 2; i < argc; i++) {
> +		char target[PATH_MAX];
> +
> +		if (target_of(argv[i], target, sizeof(target)))
> +			add_target(target);
> +	}
> +
> +	for (i = 2; i < argc; i++)
> +		process(out, argv[i]);
> +
> +	/* Write errors are latched in the stream, so one check at the end covers them all. */
> +	if (ferror(out) || fclose(out) || rename(tmp_path, argv[1])) {
> +		perror(argv[1]);
> +		unlink(tmp_path);
> +		return 1;
> +	}
> +
> +	return 0;
> +}

[Severity: Medium]
What happens if two concurrent sub-makes enter the same directory during a
parallel build?

For example, when CONFIG_HEADERS_INSTALL=y (such as in allmodconfig), the
prepare target depends on both prepare0 -> archprepare -> scripts and
headers -> scripts_unifdef without an ordering dependency between them:

Makefile:
scripts: scripts_basic scripts_dtc
	$(Q)$(MAKE) $(build)=$(@)
...
scripts_unifdef: scripts_basic
	$(Q)$(MAKE) $(build)=scripts scripts/unifdef

Similarly, on microblaze with CONFIG_OF_EARLY_FLATTREE=y, the default build
enters arch/microblaze/boot/dts via both vmlinux (with need-builtin=1) and
dtbs (with need-dtbslist=1) in parallel.

When both sub-makes parse scripts/Makefile.build for the same $(obj)
directory concurrently, two depcheck processes open the same
$(obj)/.depcheck.mk.tmp path with fopen(tmp_path, "w"). One process can
truncate and overwrite the shared .depcheck.mk.tmp file while the other is
writing to it, and the first process to finish will rename the truncated or
interleaved file to $(obj)/.depcheck.mk and return 0 (causing make to
include a corrupted makefile fragment), while the second process fails in
rename() with ENOENT and prints an error via perror(argv[1]). In addition,
when the two sub-makes have different $(targets) sets, one sub-make can
overwrite $(obj)/.depcheck.mk before the other sub-make includes it.

Could this race corrupt the generated makefile fragment during parallel
incremental or no-op builds?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-build-speedup-v5-0-0f6018be8dae@kernel.org?part=2