Re: [RFC PATCH 08/11] kbuild: make link-vmlinux.sh respect $dry_run

[Nicolas Schier <nicolas@fjasle.eu>]

On Mon, Aug 19, 2024 at 06:03:05PM +0200, Vegard Nossum wrote:
> Make link-vmlinux.sh print the commands it wants to run instead of
> actually running them when $dry_run is defined.

Do we need link-vmlinux.sh to show it's commands?  Couldn't we just show
the call of link-vmlinux.sh instead (and replace/hide the '+' prefix if
'dry_run' is set)?  Why not?  (I guess, you found good reasons.  Can you
please add them to the commit message?)

> 
> This is needed in order for make -n/--dry-run to output a complete
> build script.
> 
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> ---
>  scripts/Makefile.vmlinux | 15 ++++++++++++--
>  scripts/link-vmlinux.sh  | 44 ++++++++++++++++++++++++++--------------
>  2 files changed, 42 insertions(+), 17 deletions(-)
> 
> diff --git a/scripts/Makefile.vmlinux b/scripts/Makefile.vmlinux
> index 10d80e07f945c..1fe0faab52889 100644
> --- a/scripts/Makefile.vmlinux
> +++ b/scripts/Makefile.vmlinux
> @@ -1,5 +1,12 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  
> +# there doesn't seem to be a straightforward way for 'make -n' to output
> +# one command while actually executing another, so we resort to this hack
> +# where we set an environment variable differently depending on whether
> +# we are executing from 'make' or executing from the 'make -n' log.
> +LINK_VMLINUX=scripts/link-vmlinux.sh
> +export LINK_VMLINUX
> +
>  PHONY := __default
>  __default: vmlinux
>  
> @@ -29,7 +36,8 @@ ARCH_POSTLINK := $(wildcard $(srctree)/arch/$(SRCARCH)/Makefile.postlink)
>  
>  # Final link of vmlinux with optional arch pass after final link
>  cmd_link_vmlinux =							\
> -	$< "$(LD)" "$(KBUILD_LDFLAGS)" "$(LDFLAGS_vmlinux)";		\
> +	"$${LINK_VMLINUX}"						\
> +	"$(LD)" "$(KBUILD_LDFLAGS)" "$(LDFLAGS_vmlinux)";		\
>  	$(if $(ARCH_POSTLINK), $(MAKE) -f $(ARCH_POSTLINK) $@, true)
>  
>  # We don't know how to build these
> @@ -37,7 +45,10 @@ PHONY += vmlinux.o
>  PHONY += $(KBUILD_LDS)
>  
>  targets += vmlinux
> -vmlinux: scripts/link-vmlinux.sh vmlinux.o $(KBUILD_LDS) FORCE
> +vmlinux: $(LINK_VMLINUX) vmlinux.o $(KBUILD_LDS) FORCE
> +ifdef dry_run
> +	@LINK_VMLINUX=/bin/true
> +endif
>  	+$(call if_changed_dep,link_vmlinux)
>  
>  # Add FORCE to the prequisites of a target to force it to be always rebuilt.
> diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
> index f7b2503cdba95..fd32e48a8a455 100755
> --- a/scripts/link-vmlinux.sh
> +++ b/scripts/link-vmlinux.sh
> @@ -32,6 +32,17 @@ LD="$1"
>  KBUILD_LDFLAGS="$2"
>  LDFLAGS_vmlinux="$3"
>  
> +# If $dry_run is set, just echo the command instead of executing it.
> +run()
> +{
> +	if [ ! -v dry_run ]
> +	then
> +		eval "$1"
> +	else
> +		echo "$1"
> +	fi
> +}
> +
>  is_enabled() {
>  	grep -q "^$1=y" include/config/auto.conf
>  }
> @@ -40,7 +51,10 @@ is_enabled() {
>  # Will be supressed by "make -s"
>  info()
>  {
> -	printf "  %-7s %s\n" "${1}" "${2}"
> +	if [ ! -v dry_run ]
> +	then
> +		printf "  %-7s %s\n" "${1}" "${2}"
> +	fi
>  }
>  
>  # Link of vmlinux

Do you have some thoughts about the

     if is_enabled ...

lines?  Do we need to defer the evaluation of include/config/auto.conf,
too?


> @@ -97,10 +111,10 @@ vmlinux_link()
>  		ldflags="${ldflags} ${wl}-Map=${output}.map"
>  	fi
>  
> -	${ld} ${ldflags} -o ${output}					\
> +	run "${ld} ${ldflags} -o ${output}				\
>  		${wl}--whole-archive ${objs} ${wl}--no-whole-archive	\
>  		${wl}--start-group ${libs} ${wl}--end-group		\
> -		${kallsymso} ${btf_vmlinux_bin_o} ${ldlibs}
> +		${kallsymso} ${btf_vmlinux_bin_o} ${ldlibs}"
>  }
>  
>  # generate .BTF typeinfo from DWARF debuginfo
> @@ -129,8 +143,8 @@ gen_btf()
>  	# deletes all symbols including __start_BTF and __stop_BTF, which will
>  	# be redefined in the linker script. Add 2>/dev/null to suppress GNU
>  	# objcopy warnings: "empty loadable segment detected at ..."
> -	${OBJCOPY} --only-section=.BTF --set-section-flags .BTF=alloc,readonly \
> -		--strip-all ${1} "${btf_data}" 2>/dev/null
> +	run "${OBJCOPY} --only-section=.BTF --set-section-flags .BTF=alloc,readonly \
> +		--strip-all ${1} "${btf_data}" 2>/dev/null"

                                 ^^^^^^^^^^^^^
Backslashes to keep ${btf_data} quoted are missing.

>  	# Change e_type to ET_REL so that it can be used to link final vmlinux.
>  	# GNU ld 2.35+ and lld do not allow an ET_EXEC input.
>  	if is_enabled CONFIG_CPU_BIG_ENDIAN; then

printf "${et_rel}" | dd of="${btf_data}" conv=notrunc bs=1 seek=16 status=none

'dd' should probably be called via 'run'.


> @@ -161,11 +175,11 @@ kallsyms()
>  	fi
>  
>  	info KSYMS "${2}.S"
> -	scripts/kallsyms ${kallsymopt} "${1}" > "${2}.S"
> +	run "scripts/kallsyms ${kallsymopt} \"${1}\" > \"${2}.S\""
>  
>  	info AS "${2}.o"
> -	${CC} ${NOSTDINC_FLAGS} ${LINUXINCLUDE} ${KBUILD_CPPFLAGS} \
> -	      ${KBUILD_AFLAGS} ${KBUILD_AFLAGS_KERNEL} -c -o "${2}.o" "${2}.S"
> +	run "${CC} ${NOSTDINC_FLAGS} ${LINUXINCLUDE} ${KBUILD_CPPFLAGS} \
> +	      ${KBUILD_AFLAGS} ${KBUILD_AFLAGS_KERNEL} -c -o \"${2}.o\" \"${2}.S\""
>  
>  	kallsymso=${2}.o
>  }
> @@ -184,12 +198,12 @@ sysmap_and_kallsyms()
>  mksysmap()
>  {
>  	info NM ${2}
> -	${NM} -n "${1}" | sed -f "${srctree}/scripts/mksysmap" > "${2}"
> +	run "${NM} -n \"${1}\" | sed -f \"${srctree}/scripts/mksysmap\" > \"${2}\""
>  }
>  
>  sorttable()
>  {
> -	${objtree}/scripts/sorttable ${1}
> +	run "${objtree}/scripts/sorttable ${1}"
>  }
>  
>  cleanup()

I know, 'link-vmlinux.sh clean' will not by called by 'make --dry-run
clean' (as there is no '+' prefix in the call in Makefile), but for
consistency, I'd like to see also cleanup() be prepared for handling
dry_run correctly.


Can you please move patch 11 (resetting of KBUILD_VERBOSE if '--dry-run'
or '--silent' is given) before this one?


Does '${MAKE} ...' also need to be called via 'run'?

> @@ -270,13 +284,13 @@ if is_enabled CONFIG_KALLSYMS; then
>  	strip_debug=1
>  
>  	sysmap_and_kallsyms .tmp_vmlinux1
> -	size1=$(${CONFIG_SHELL} "${srctree}/scripts/file-size.sh" ${kallsymso})
> +	[ -v dry_run ] || size1=$(${CONFIG_SHELL} "${srctree}/scripts/file-size.sh" ${kallsymso})
>  
>  	vmlinux_link .tmp_vmlinux2
>  	sysmap_and_kallsyms .tmp_vmlinux2
> -	size2=$(${CONFIG_SHELL} "${srctree}/scripts/file-size.sh" ${kallsymso})
> +	[ -v dry_run ] || size2=$(${CONFIG_SHELL} "${srctree}/scripts/file-size.sh" ${kallsymso})
>  
> -	if [ $size1 -ne $size2 ] || [ -n "${KALLSYMS_EXTRA_PASS}" ]; then
> +	if [ ! -v dry_run ] || [ $size1 -ne $size2 ] || [ -n "${KALLSYMS_EXTRA_PASS}" ]; then
>  		vmlinux_link .tmp_vmlinux3
>  		sysmap_and_kallsyms .tmp_vmlinux3
>  	fi
> @@ -289,7 +303,7 @@ vmlinux_link vmlinux
>  # fill in BTF IDs
>  if is_enabled CONFIG_DEBUG_INFO_BTF && is_enabled CONFIG_BPF; then
>  	info BTFIDS vmlinux
> -	${RESOLVE_BTFIDS} vmlinux
> +	run ${RESOLVE_BTFIDS} vmlinux

Quotes are missing, as run only evaluated its first argument.

>  fi
>  
>  mksysmap vmlinux System.map
> @@ -303,7 +317,7 @@ if is_enabled CONFIG_BUILDTIME_TABLE_SORT; then

I am sceptical about

	if ! sorttable vmlinux

and similar.  With dry_run being set, sorttable shows the call of
'sorttable' and always returns success.  In effect, the resulting output
shell build script will miss such error detection.

I think it would be really good, if we could skip patching
link-vmlinux.sh but show its call instead.

Kind regards,
Nicolas

>  fi
>  
>  # step a (see comment above)
> -if is_enabled CONFIG_KALLSYMS; then
> +if [ ! -v dry_run ] && is_enabled CONFIG_KALLSYMS; then
>  	if ! cmp -s System.map "${kallsyms_sysmap}"; then
>  		echo >&2 Inconsistent kallsyms data
>  		echo >&2 'Try "make KALLSYMS_EXTRA_PASS=1" as a workaround'
> -- 
> 2.34.1
> 

-- 
Nicolas