Re: [PATCH v3 1/3] kbuild: refactor variables in scripts/link-vmlinux.sh

Nicolas Schier
2024-06-12T20:01:45Z

On Mon, Jun 10, 2024 at 08:25:16PM +0900, Masahiro Yamada wrote:
> Clean up the variables in scripts/link-vmlinux.sh
>
> - Specify the extra objects directly in vmlinux_link()
> - Move the AS rule to kallsyms()
> - Set kallsymso and btf_vmlinux_bin_o where they are generated
> - Remove unneeded variable, kallsymso_prev
> - Introduce the btf_data variable
> - Introduce the strip_debug flag instead of checking the output name
>
> No functional change intended.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>
> Changes in v3:
> - Use ${strip_debug} for cleanup
>
> scripts/link-vmlinux.sh | 67 +++++++++++++++++++++--------------------
> 1 file changed, 35 insertions(+), 32 deletions(-)
>
> diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
> index 518c70b8db50..3d9d7257143a 100755
> --- a/scripts/link-vmlinux.sh
> +++ b/scripts/link-vmlinux.sh
> @@ -45,7 +45,6 @@ info()
>
> # Link of vmlinux
> # ${1} - output file
> -# ${2}, ${3}, ... - optional extra .o files
> vmlinux_link()
> {
> local output=${1}
> @@ -90,7 +89,7 @@ vmlinux_link()
> ldflags="${ldflags} ${wl}--script=${objtree}/${KBUILD_LDS}"
>
> # The kallsyms linking does not need debug symbols included.
> - if [ "$output" != "${output#.tmp_vmlinux.kallsyms}" ] ; then
> + if [ -n "${strip_debug}" ] ; then
> ldflags="${ldflags} ${wl}--strip-debug"
> fi
>
> @@ -101,7 +100,7 @@ vmlinux_link()
> ${ld} ${ldflags} -o ${output} \
> ${wl}--whole-archive ${objs} ${wl}--no-whole-archive \
> ${wl}--start-group ${libs} ${wl}--end-group \
> - $@ ${ldlibs}
> + ${kallsymso} ${btf_vmlinux_bin_o} ${ldlibs}
> }
>
> # generate .BTF typeinfo from DWARF debuginfo
> @@ -110,6 +109,7 @@ vmlinux_link()
> gen_btf()
> {
> local pahole_ver
> + local btf_data=${2}
>
> if ! [ -x "$(command -v ${PAHOLE})" ]; then
> echo >&2 "BTF: ${1}: pahole (${PAHOLE}) is not available"
> @@ -124,16 +124,16 @@ gen_btf()
>
> vmlinux_link ${1}
>
> - info "BTF" ${2}
> + info BTF "${btf_data}"
> LLVM_OBJCOPY="${OBJCOPY}" ${PAHOLE} -J ${PAHOLE_FLAGS} ${1}
>
> - # Create ${2} which contains just .BTF section but no symbols. Add
> + # Create ${btf_data} which contains just .BTF section but no symbols. Add
> # SHF_ALLOC because .BTF will be part of the vmlinux image. --strip-all
> # 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} ${2} 2>/dev/null
> + --strip-all ${1} "${btf_data}" 2>/dev/null
> # 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
> @@ -141,10 +141,12 @@ gen_btf()
> else
> et_rel='\1\0'
> fi
> - printf "${et_rel}" | dd of=${2} conv=notrunc bs=1 seek=16 status=none
> + printf "${et_rel}" | dd of="${btf_data}" conv=notrunc bs=1 seek=16 status=none
> +
> + btf_vmlinux_bin_o=${btf_data}
> }
>
> -# Create ${2} .S file with all symbols from the ${1} object file
> +# Create ${2}.o file with all symbols from the ${1} object file
> kallsyms()
> {
> local kallsymopt;
> @@ -165,27 +167,25 @@ kallsyms()
> kallsymopt="${kallsymopt} --lto-clang"
> fi
>
> - info KSYMS ${2}
> - scripts/kallsyms ${kallsymopt} ${1} > ${2}
> + info KSYMS "${2}.S"
> + 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"
> +
> + kallsymso=${2}.o
> }
>
> # Perform one step in kallsyms generation, including temporary linking of
> # vmlinux.
> kallsyms_step()
> {
> - kallsymso_prev=${kallsymso}
> kallsyms_vmlinux=.tmp_vmlinux.kallsyms${1}
> - kallsymso=${kallsyms_vmlinux}.o
> - kallsyms_S=${kallsyms_vmlinux}.S
>
> - vmlinux_link ${kallsyms_vmlinux} "${kallsymso_prev}" ${btf_vmlinux_bin_o}
> - mksysmap ${kallsyms_vmlinux} ${kallsyms_vmlinux}.syms
> - kallsyms ${kallsyms_vmlinux}.syms ${kallsyms_S}
> -
> - info AS ${kallsymso}
> - ${CC} ${NOSTDINC_FLAGS} ${LINUXINCLUDE} ${KBUILD_CPPFLAGS} \
> - ${KBUILD_AFLAGS} ${KBUILD_AFLAGS_KERNEL} \
> - -c -o ${kallsymso} ${kallsyms_S}
> + vmlinux_link "${kallsyms_vmlinux}"
> + mksysmap "${kallsyms_vmlinux}" "${kallsyms_vmlinux}.syms"
> + kallsyms "${kallsyms_vmlinux}.syms" "${kallsyms_vmlinux}"
> }
>
> # Create map file with all symbols from ${1}
> @@ -223,19 +223,18 @@ fi
>
> ${MAKE} -f "${srctree}/scripts/Makefile.build" obj=init init/version-timestamp.o
>
> -btf_vmlinux_bin_o=""
> +btf_vmlinux_bin_o=
> +kallsymso=
> +strip_debug=
> +
> if is_enabled CONFIG_DEBUG_INFO_BTF; then
> - btf_vmlinux_bin_o=.btf.vmlinux.bin.o
> - if ! gen_btf .tmp_vmlinux.btf $btf_vmlinux_bin_o ; then
> + if ! gen_btf .tmp_vmlinux.btf .btf.vmlinux.bin.o ; then
> echo >&2 "Failed to generate BTF for vmlinux"
> echo >&2 "Try to disable CONFIG_DEBUG_INFO_BTF"
> exit 1
> fi
> fi
>
> -kallsymso=""
> -kallsymso_prev=""
> -kallsyms_vmlinux=""
> if is_enabled CONFIG_KALLSYMS; then
>
> # kallsyms support
> @@ -261,11 +260,13 @@ if is_enabled CONFIG_KALLSYMS; then
> # a) Verify that the System.map from vmlinux matches the map from
> # ${kallsymso}.
>
> - kallsyms_step 1
> - kallsyms_step 2
> + # The kallsyms linking does not need debug symbols included.
> + strip_debug=1
>
> - # step 3
> - size1=$(${CONFIG_SHELL} "${srctree}/scripts/file-size.sh" ${kallsymso_prev})
> + kallsyms_step 1
> + size1=$(${CONFIG_SHELL} "${srctree}/scripts/file-size.sh" ${kallsymso})
> +
> + kallsyms_step 2
> size2=$(${CONFIG_SHELL} "${srctree}/scripts/file-size.sh" ${kallsymso})
>
> if [ $size1 -ne $size2 ] || [ -n "${KALLSYMS_EXTRA_PASS}" ]; then
> @@ -273,7 +274,9 @@ if is_enabled CONFIG_KALLSYMS; then
> fi
> fi
>
> -vmlinux_link vmlinux "${kallsymso}" ${btf_vmlinux_bin_o}
> +strip_debug=
> +
> +vmlinux_link vmlinux
>
> # fill in BTF IDs
> if is_enabled CONFIG_DEBUG_INFO_BTF && is_enabled CONFIG_BPF; then
> --
> 2.43.0
>
>

Looks good to me.

Reviewed-by: Nicolas Schier <nicolas@fjasle.eu>