Message-ID: <763200e4f55197da44789b97fd5379ae8bf32c08.camel@gmail.com>
Date: Thu, 04 Dec 2025 08:57:10 -0800
From: Eduard Zingerman <eddyz87@...il.com>
To: Ihor Solodrai <ihor.solodrai@...ux.dev>, Alexei Starovoitov
 <ast@...nel.org>,  Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko
 <andrii@...nel.org>, Martin KaFai Lau <martin.lau@...ux.dev>,  Song Liu
 <song@...nel.org>, Yonghong Song <yonghong.song@...ux.dev>, John Fastabend	
 <john.fastabend@...il.com>, KP Singh <kpsingh@...nel.org>, Stanislav
 Fomichev	 <sdf@...ichev.me>, Hao Luo <haoluo@...gle.com>, Jiri Olsa
 <jolsa@...nel.org>,  Nathan Chancellor	 <nathan@...nel.org>, Nicolas Schier
 <nicolas.schier@...ux.dev>, Nick Desaulniers
 <nick.desaulniers+lkml@...il.com>, Bill Wendling <morbo@...gle.com>, Justin
 Stitt	 <justinstitt@...gle.com>, Alan Maguire <alan.maguire@...cle.com>,
 Donglin Peng	 <dolinux.peng@...il.com>
Cc: bpf@...r.kernel.org, dwarves@...r.kernel.org,
 linux-kernel@...r.kernel.org, 	linux-kbuild@...r.kernel.org
Subject: Re: [PATCH bpf-next v2 4/4] resolve_btfids: change in-place update
 with raw binary output

On Wed, 2025-12-03 at 21:13 -0800, Ihor Solodrai wrote:
> On 12/1/25 11:55 AM, Eduard Zingerman wrote:
> > On Thu, 2025-11-27 at 10:52 -0800, Ihor Solodrai wrote:
> > > Currently resolve_btfids updates .BTF_ids section of an ELF file
> > > in-place, based on the contents of provided BTF, usually within the
> > > same input file, and optionally a BTF base.
> > > 
> 
> Hi Eduard, thank you for the review.
> 
> > > This patch changes resolve_btfids behavior to enable BTF
> > > transformations as part of its main operation. To achieve this
> > > in-place ELF write in resolve_btfids is replaced with generation of
> > > the following binaries:
> > >   * ${1}.btf with .BTF section data
> > >   * ${1}.distilled_base.btf with .BTF.base section data (for
> > >     out-of-tree modules)
> > >   * ${1}.btf_ids with .BTF_ids section data, if it exists in ${1}
> > 
> > Nit: use ${1}.BTF / ${1}.BTF.base / ${1}.BTF_ids, so that each file is
> >      named by it's corresponding section?
> 
> Sure, makes sense.
> 
> > 
> > > 
> > > The execution of resolve_btfids and consumption of its output is
> > > orchestrated by scripts/gen-btf.sh introduced in this patch.
> > > 
> > > The rationale for this approach is that updating ELF in-place with
> > > libelf API is complicated and bug-prone, especially in the context of
> > > the kernel build. On the other hand applying objcopy to manipulate ELF
> > > sections is simpler and more reliable.
> > 
> > Nit: more context needed, as is the statement raises questions but not
> >      answers them.
> 
> Would you like to see more details about why using libelf is complicated?
> I don't follow what's unclear here, sorry...

The claim here is: "libelf API is complicated and bug-prone ... in
context of the kernel build". This is a very vague wording.
The decision to rely on objcopy/linker comes from a specific needs
outlined by Andrii in an off-list discussion. It will be good to have
this context captured in the commit message, instead of bluntly
stating that libelf is bug-prone.

> > > There are two distinct paths for BTF generation and resolve_btfids
> > > application in the kernel build: for vmlinux and for kernel modules.
> > > 
> > > For the vmlinux binary a .BTF section is added in a roundabout way to
> > > ensure correct linking (details below). The patch doesn't change this
> > > approach, only the implementation is a little different.
> > > 
> > > Before this patch it worked like follows:
> > > 
> > >   * pahole consumed .tmp_vmlinux1 [1] and added .BTF section with
> > >     llvm-objcopy [2] to it
> > >   * then everything except the .BTF section was stripped from .tmp_vmlinux1
> > >     into a .tmp_vmlinux1.bpf.o object [1], later linked into vmlinux
> > >   * resolve_btfids was executed later on vmlinux.unstripped [3],
> > >     updating it in-place
> > > 
> > > After this patch gen-btf.sh implements the following:
> > > 
> > >   * pahole consumes .tmp_vmlinux1 and produces a *detached* file with
> > >     raw BTF data
> > >   * resolve_btfids consumes .tmp_vmlinux1 and detached BTF to produce
> > >     (potentially modified) .BTF, and .BTF_ids sections data
> > >   * a .tmp_vmlinux1.bpf.o object is then produced with objcopy copying
> > >     BTF output of resolve_btfids
> > >   * .BTF_ids data gets embedded into vmlinux.unstripped in
> > >     link-vmlinux.sh by objcopy --update-section
> > > 
> > > For the kernel modules creating special .bpf.o file is not necessary,
> > > and so embedding of sections data produced by resolve_btfids is
> > > straightforward with the objcopy.
> > > 
> > > With this patch an ELF file becomes effectively read-only within
> > > resolve_btfids, which allows to delete elf_update() call and satelite
> > > code (like compressed_section_fix [4]).
> > > 
> > > Endianness handling of .BTF_ids data is also changed. Previously the
> > > "flags" part of the section was bswapped in sets_patch() [5], and then
> > > Elf_Type was modified before elf_update() to signal to libelf that
> > > bswap may be necessary. With this patch we explicitly bswap entire
> > > data buffer on load and on dump.
> > > 
> > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git/tree/scripts/link-vmlinux.sh#n115
> > > [2] https://git.kernel.org/pub/scm/devel/pahole/pahole.git/tree/btf_encoder.c#n1835
> > > [3] https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git/tree/scripts/link-vmlinux.sh#n285
> > 
> > Nit: these links are moving target, should refer to a commit or a tag.
> 
> Acked
> 
> > 
> > > [4] https://lore.kernel.org/bpf/20200819092342.259004-1-jolsa@kernel.org/
> > > [5] https://lore.kernel.org/bpf/cover.1707223196.git.vmalik@redhat.com/
> > > 
> > > Signed-off-by: Ihor Solodrai <ihor.solodrai@...ux.dev>
> > > ---
> > >  MAINTAINERS                          |   1 +
> > >  scripts/Makefile.modfinal            |   5 +-
> > >  scripts/gen-btf.sh                   | 167 ++++++++++++++++++++
> > >  scripts/link-vmlinux.sh              |  42 +-----
> > >  tools/bpf/resolve_btfids/main.c      | 218 +++++++++++++++++----------
> > >  tools/testing/selftests/bpf/Makefile |   5 +
> > >  6 files changed, 317 insertions(+), 121 deletions(-)
> > >  create mode 100755 scripts/gen-btf.sh
> > 
> > Since resolve_btfids is now responsible for distilled base generation,
> > does Makefile.btf need modification to remove "--btf_features=distilled_base"?
> 
> Yes, good catch.
> 
> > 
> > > 
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 48aabeeed029..5cd34419d952 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -4672,6 +4672,7 @@ F:	net/sched/act_bpf.c
> > >  F:	net/sched/cls_bpf.c
> > >  F:	samples/bpf/
> > >  F:	scripts/bpf_doc.py
> > > +F:	scripts/gen-btf.sh
> > >  F:	scripts/Makefile.btf
> > >  F:	scripts/pahole-version.sh
> > >  F:	tools/bpf/
> > > diff --git a/scripts/Makefile.modfinal b/scripts/Makefile.modfinal
> > > index 542ba462ed3e..3862fdfa1267 100644
> > > --- a/scripts/Makefile.modfinal
> > > +++ b/scripts/Makefile.modfinal
> > > @@ -38,9 +38,8 @@ quiet_cmd_btf_ko = BTF [M] $@
> > >        cmd_btf_ko = 							\
> > >  	if [ ! -f $(objtree)/vmlinux ]; then				\
> > >  		printf "Skipping BTF generation for %s due to unavailability of vmlinux\n" $@ 1>&2; \
> > > -	else								\
> > > -		LLVM_OBJCOPY="$(OBJCOPY)" $(PAHOLE) -J $(PAHOLE_FLAGS) $(MODULE_PAHOLE_FLAGS) --btf_base $(objtree)/vmlinux $@; \
> > > -		$(RESOLVE_BTFIDS) -b $(objtree)/vmlinux $@;		\
> > > +	else	\
> > > +		$(srctree)/scripts/gen-btf.sh --btf_base $(objtree)/vmlinux $@; \
> > >  	fi;
> > >  
> > >  # Same as newer-prereqs, but allows to exclude specified extra dependencies
> > > diff --git a/scripts/gen-btf.sh b/scripts/gen-btf.sh
> > > new file mode 100755
> > > index 000000000000..2dfb7ab289ca
> > > --- /dev/null
> > > +++ b/scripts/gen-btf.sh
> > > @@ -0,0 +1,167 @@
> > > +#!/bin/bash
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +# Copyright (c) 2025 Meta Platforms, Inc. and affiliates.
> > > +#
> > > +# This script generates BTF data for the provided ELF file.
> > > +#
> > > +# Kernel BTF generation involves these conceptual steps:
> > > +#   1. pahole generates BTF from DWARF data
> > > +#   2. resolve_btfids applies kernel-specific btf2btf
> > > +#      transformations and computes data for .BTF_ids section
> > > +#   3. the result gets linked/objcopied into the target binary
> > > +#
> > > +# How step (3) should be done differs between vmlinux, and
> > > +# kernel modules, which is the primary reason for the existence
> > > +# of this script.
> > > +#
> > > +# For modules the script expects vmlinux passed in as --btf_base.
> > > +# Generated .BTF, .BTF.base and .BTF_ids sections become embedded
> > > +# into the input ELF file with objcopy.
> > > +#
> > > +# For vmlinux the input file remains unchanged and two files are produced:
> > > +#   - ${1}.btf.o ready for linking into vmlinux
> > > +#   - ${1}.btf_ids with .BTF_ids data blob
> > > +# This output is consumed by scripts/link-vmlinux.sh
> > > +
> > > +set -e
> > > +
> > > +usage()
> > > +{
> > > +	echo "Usage: $0 [--btf_base <file>] <target ELF file>"
> > > +	exit 1
> > > +}
> > > +
> > > +BTF_BASE=""
> > > +
> > > +while [ $# -gt 0 ]; do
> > > +	case "$1" in
> > > +	--btf_base)
> > > +		BTF_BASE="$2"
> > > +		shift 2
> > > +		;;
> > > +	-*)
> > > +		echo "Unknown option: $1" >&2
> > > +		usage
> > > +		;;
> > > +	*)
> > > +		break
> > > +		;;
> > > +	esac
> > > +done
> > > +
> > > +if [ $# -ne 1 ]; then
> > > +	usage
> > > +fi
> > > +
> > > +ELF_FILE="$1"
> > > +shift
> > > +
> > > +is_enabled() {
> > > +	grep -q "^$1=y" ${objtree}/include/config/auto.conf
> > > +}
> > > +
> > > +info()
> > > +{
> > > +	printf "  %-7s %s\n" "${1}" "${2}"
> > > +}
> > > +
> > > +case "${KBUILD_VERBOSE}" in
> > > +*1*)
> > > +	set -x
> > > +	;;
> > > +esac
> > > +
> > > +if ! is_enabled CONFIG_DEBUG_INFO_BTF; then
> > > +	exit 0
> > > +fi
> > > +
> > > +gen_btf_data()
> > > +{
> > > +	info BTF "${ELF_FILE}"
> > > +	btf1="${ELF_FILE}.btf.1"
> > > +	${PAHOLE} -J ${PAHOLE_FLAGS}			\
> > > +		${BTF_BASE:+--btf_base ${BTF_BASE}}	\
> > > +		--btf_encode_detached=${btf1}		\
> > > +		"${ELF_FILE}"
> > > +
> > > +	info BTFIDS "${ELF_FILE}"
> > > +	RESOLVE_BTFIDS_OPTS=""
> > > +	if is_enabled CONFIG_WERROR; then
> > > +		RESOLVE_BTFIDS_OPTS+=" --fatal_warnings "
> > > +	fi
> > > +	if [ -n "${KBUILD_VERBOSE}" ]; then
> > > +		RESOLVE_BTFIDS_OPTS+=" -v "
> > > +	fi
> > > +	${RESOLVE_BTFIDS} ${RESOLVE_BTFIDS_OPTS}	\
> > > +		${BTF_BASE:+--btf_base ${BTF_BASE}}	\
> > > +		--btf ${btf1} "${ELF_FILE}"
> > > +}
> > > +
> > > +gen_btf_o()
> > > +{
> > > +	local btf_data=${ELF_FILE}.btf.o
> > > +
> > > +	# 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.
> > > +	info OBJCOPY "${btf_data}"
> > > +	echo "" | ${CC} -c -x c -o ${btf_data} -
> > > +	${OBJCOPY} --add-section .BTF=${ELF_FILE}.btf \
> > > +		--set-section-flags .BTF=alloc,readonly ${btf_data}
> > > +	${OBJCOPY} --only-section=.BTF --strip-all ${btf_data}
> > > +
> > > +	# 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
> > > +		et_rel='\0\1'
> > > +	else
> > > +		et_rel='\1\0'
> > > +	fi
> > > +	printf "${et_rel}" | dd of="${btf_data}" conv=notrunc bs=1 seek=16 status=none
> > > +}
> > > +
> > > +embed_btf_data()
> > > +{
> > > +	info OBJCOPY "${ELF_FILE}"
> > > +	${OBJCOPY} --add-section .BTF=${ELF_FILE}.btf ${ELF_FILE}
> > > +
> > > +	# a module might not have a .BTF_ids or .BTF.base section
> > > +	local btf_base="${ELF_FILE}.distilled_base.btf"
> > > +	if [ -f "${btf_base}" ]; then
> > > +		${OBJCOPY} --add-section .BTF.base=${btf_base} ${ELF_FILE}
> > > +	fi
> > > +	local btf_ids="${ELF_FILE}.btf_ids"
> > > +	if [ -f "${btf_ids}" ]; then
> > > +		${OBJCOPY} --update-section .BTF_ids=${btf_ids} ${ELF_FILE}
> > > +	fi
> > > +}
> > > +
> > > +cleanup()
> > > +{
> > > +	rm -f "${ELF_FILE}.btf.1"
> > > +	rm -f "${ELF_FILE}.btf"
> > > +	if [ "${BTFGEN_MODE}" = "module" ]; then
> > > +		rm -f "${ELF_FILE}.distilled_base.btf"
> > > +		rm -f "${ELF_FILE}.btf_ids"
> > > +	fi
> > > +}
> > > +trap cleanup EXIT
> > > +
> > > +BTFGEN_MODE="vmlinux"
> > > +if [ -n "${BTF_BASE}" ]; then
> > > +	BTFGEN_MODE="module"
> > > +fi
> > > +
> > > +gen_btf_data
> > > +
> > > +case "${BTFGEN_MODE}" in
> > > +vmlinux)
> > > +	gen_btf_o
> > > +	;;
> > > +module)
> > > +	embed_btf_data
> > > +	;;
> > > +esac
> > > +
> > > +exit 0
> > > diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
> > > index 433849ff7529..5bea8795f96d 100755
> > > --- a/scripts/link-vmlinux.sh
> > > +++ b/scripts/link-vmlinux.sh
> > > @@ -105,34 +105,6 @@ vmlinux_link()
> > >  		${kallsymso} ${btf_vmlinux_bin_o} ${arch_vmlinux_o} ${ldlibs}
> > >  }
> > >  
> > > -# generate .BTF typeinfo from DWARF debuginfo
> > > -# ${1} - vmlinux image
> > > -gen_btf()
> > > -{
> > > -	local btf_data=${1}.btf.o
> > > -
> > > -	info BTF "${btf_data}"
> > > -	LLVM_OBJCOPY="${OBJCOPY}" ${PAHOLE} -J ${PAHOLE_FLAGS} ${1}
> > > -
> > > -	# 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} "${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
> > > -		et_rel='\0\1'
> > > -	else
> > > -		et_rel='\1\0'
> > > -	fi
> > > -	printf "${et_rel}" | dd of="${btf_data}" conv=notrunc bs=1 seek=16 status=none
> > > -
> > > -	btf_vmlinux_bin_o=${btf_data}
> > > -}
> > > -
> > >  # Create ${2}.o file with all symbols from the ${1} object file
> > >  kallsyms()
> > >  {
> > > @@ -204,6 +176,7 @@ if is_enabled CONFIG_ARCH_WANTS_PRE_LINK_VMLINUX; then
> > >  fi
> > >  
> > >  btf_vmlinux_bin_o=
> > > +btfids_vmlinux=
> > >  kallsymso=
> > >  strip_debug=
> > >  generate_map=
> > > @@ -224,11 +197,13 @@ if is_enabled CONFIG_KALLSYMS || is_enabled CONFIG_DEBUG_INFO_BTF; then
> > >  fi
> > >  
> > >  if is_enabled CONFIG_DEBUG_INFO_BTF; then
> > > -	if ! gen_btf .tmp_vmlinux1; then
> > > +	if ! ${srctree}/scripts/gen-btf.sh .tmp_vmlinux1; then
> > 
> > Nit: maybe pass output file names as parameters for get-btf.sh?
> 
> I don't see a point in that. The script and callsite will become
> more complicated, but what is the benefit?

In order to avoid implicit naming conventions.  Hence, the reader of
the script code has clear understanding about in and out parameters.

> > 
> > >  		echo >&2 "Failed to generate BTF for vmlinux"
> > >  		echo >&2 "Try to disable CONFIG_DEBUG_INFO_BTF"
> > >  		exit 1
> > >  	fi
> > > +	btf_vmlinux_bin_o=.tmp_vmlinux1.btf.o
> > > +	btfids_vmlinux=.tmp_vmlinux1.btf_ids
> > >  fi
> > >  
> > >  if is_enabled CONFIG_KALLSYMS; then
> > > @@ -281,14 +256,9 @@ fi
> > >  
> > >  vmlinux_link "${VMLINUX}"
> > >  
> > > -# fill in BTF IDs
> > >  if is_enabled CONFIG_DEBUG_INFO_BTF; then
> > > -	info BTFIDS "${VMLINUX}"
> > > -	RESOLVE_BTFIDS_ARGS=""
> > > -	if is_enabled CONFIG_WERROR; then
> > > -		RESOLVE_BTFIDS_ARGS=" --fatal_warnings "
> > > -	fi
> > > -	${RESOLVE_BTFIDS} ${RESOLVE_BTFIDS_ARGS} "${VMLINUX}"
> > > +	info OBJCOPY ${btfids_vmlinux}
> > > +	${OBJCOPY} --update-section .BTF_ids=${btfids_vmlinux} ${VMLINUX}
> > >  fi
> > 
> > Nit: Maybe do this in get-btf.sh as for modules?
> >      To avoid checking for CONFIG_DEBUG_INFO_BTF in two places.
> 
> IIRC we can't do that, because we have to update .BTF_ids after all
> the linking steps. I'll double check.
> 
> > 
> > >  
> > >  mksysmap "${VMLINUX}" System.map
> > > diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
> > > index c60d303ca6ed..b8df6256e29e 100644
> > > --- a/tools/bpf/resolve_btfids/main.c
> > > +++ b/tools/bpf/resolve_btfids/main.c
> > > @@ -71,9 +71,11 @@
> > >  #include <fcntl.h>
> > >  #include <errno.h>
> > >  #include <linux/btf_ids.h>
> > > +#include <linux/kallsyms.h>
> > >  #include <linux/rbtree.h>
> > >  #include <linux/zalloc.h>
> > >  #include <linux/err.h>
> > > +#include <linux/limits.h>
> > >  #include <bpf/btf.h>
> > >  #include <bpf/libbpf.h>
> > >  #include <subcmd/parse-options.h>
> > > @@ -124,6 +126,7 @@ struct object {
> > >  
> > >  	struct btf *btf;
> > >  	struct btf *base_btf;
> > > +	bool distilled_base;
> > >  
> > >  	struct {
> > >  		int		 fd;
> > > @@ -308,42 +311,16 @@ static struct btf_id *add_symbol(struct rb_root *root, char *name, size_t size)
> > >  	return btf_id;
> > >  }
> > >  
> > > -/* Older libelf.h and glibc elf.h might not yet define the ELF compression types. */
> > > -#ifndef SHF_COMPRESSED
> > > -#define SHF_COMPRESSED (1 << 11) /* Section with compressed data. */
> > > -#endif
> > > -
> > > -/*
> > > - * The data of compressed section should be aligned to 4
> > > - * (for 32bit) or 8 (for 64 bit) bytes. The binutils ld
> > > - * sets sh_addralign to 1, which makes libelf fail with
> > > - * misaligned section error during the update:
> > > - *    FAILED elf_update(WRITE): invalid section alignment
> > > - *
> > > - * While waiting for ld fix, we fix the compressed sections
> > > - * sh_addralign value manualy.
> > > - */
> > > -static int compressed_section_fix(Elf *elf, Elf_Scn *scn, GElf_Shdr *sh)
> > > +static void bswap_32_data(void *data, u32 nr_bytes)
> > >  {
> > > -	int expected = gelf_getclass(elf) == ELFCLASS32 ? 4 : 8;
> > > +	u32 cnt, i;
> > > +	u32 *ptr;
> > >  
> > > -	if (!(sh->sh_flags & SHF_COMPRESSED))
> > > -		return 0;
> > > +	cnt = nr_bytes / sizeof(u32);
> > > +	ptr = data;
> > >  
> > > -	if (sh->sh_addralign == expected)
> > > -		return 0;
> > > -
> > > -	pr_debug2(" - fixing wrong alignment sh_addralign %u, expected %u\n",
> > > -		  sh->sh_addralign, expected);
> > > -
> > > -	sh->sh_addralign = expected;
> > > -
> > > -	if (gelf_update_shdr(scn, sh) == 0) {
> > > -		pr_err("FAILED cannot update section header: %s\n",
> > > -			elf_errmsg(-1));
> > > -		return -1;
> > > -	}
> > > -	return 0;
> > > +	for (i = 0; i < cnt; i++)
> > > +		ptr[i] = bswap_32(ptr[i]);
> > >  }
> > >  
> > >  static int elf_collect(struct object *obj)
> > > @@ -364,7 +341,7 @@ static int elf_collect(struct object *obj)
> > >  
> > >  	elf_version(EV_CURRENT);
> > >  
> > > -	elf = elf_begin(fd, ELF_C_RDWR_MMAP, NULL);
> > > +	elf = elf_begin(fd, ELF_C_READ_MMAP_PRIVATE, NULL);
> > >  	if (!elf) {
> > >  		close(fd);
> > >  		pr_err("FAILED cannot create ELF descriptor: %s\n",
> > > @@ -427,21 +404,20 @@ static int elf_collect(struct object *obj)
> > >  			obj->efile.symbols_shndx = idx;
> > >  			obj->efile.strtabidx     = sh.sh_link;
> > >  		} else if (!strcmp(name, BTF_IDS_SECTION)) {
> > > +			/*
> > > +			 * If target endianness differs from host, we need to bswap32
> > > +			 * the .BTF_ids section data on load, because .BTF_ids has
> > > +			 * Elf_Type = ELF_T_BYTE, and so libelf returns data buffer in
> > > +			 * the target endiannes. We repeat this on dump.
> > > +			 */
> > > +			if (obj->efile.encoding != ELFDATANATIVE) {
> > > +				pr_debug("bswap_32 .BTF_ids data from target to host endianness\n");
> > > +				bswap_32_data(data->d_buf, data->d_size);
> > > +			}
> > >  			obj->efile.idlist       = data;
> > >  			obj->efile.idlist_shndx = idx;
> > >  			obj->efile.idlist_addr  = sh.sh_addr;
> > > -		} else if (!strcmp(name, BTF_BASE_ELF_SEC)) {
> > > -			/* If a .BTF.base section is found, do not resolve
> > > -			 * BTF ids relative to vmlinux; resolve relative
> > > -			 * to the .BTF.base section instead.  btf__parse_split()
> > > -			 * will take care of this once the base BTF it is
> > > -			 * passed is NULL.
> > > -			 */
> > > -			obj->base_btf_path = NULL;
> > >  		}
> > > -
> > > -		if (compressed_section_fix(elf, scn, &sh))
> > > -			return -1;
> > >  	}
> > >  
> > >  	return 0;
> > > @@ -545,6 +521,13 @@ static int symbols_collect(struct object *obj)
> > >  	return 0;
> > >  }
> > >  
> > > +static inline bool is_envvar_set(const char *var_name)
> > > +{
> > > +	const char *value = getenv(var_name);
> > > +
> > > +	return value && value[0] != '\0';
> > > +}
> > > +
> > >  static int load_btf(struct object *obj)
> > >  {
> > >  	struct btf *base_btf = NULL, *btf = NULL;
> > > @@ -571,6 +554,20 @@ static int load_btf(struct object *obj)
> > >  	obj->base_btf = base_btf;
> > >  	obj->btf = btf;
> > >  
> > > +	if (obj->base_btf && is_envvar_set("KBUILD_EXTMOD")) {
> > 
> > This is a bit ugly, maybe use a dedicated parameter instead of
> > checking environment variable?
> 
> Disagree. I intentionally tried to avoid adding options to
> resolve_btfids, because it's not intendend for general CLI usage (as
> opposed to pahole, for example). IMO the interface should be as simple
> as possible.
> 
> If we add an option, we still have to check for the env variable
> somewhere, and then pass the argument through. Why? Just checking an
> env var when it matters is simpler.
> 
> I don't think we want or expect resolve_btfids to run outside of the
> kernel or selftests build.

This comes to personal opinion, of-course.
So, in my personal opinion, obfuscating a command line tool interface
with it being parameterized by both environment variables and command
line parameters is rarely justified.  In this particular case it will
only make life a tad harder for someone debugging resolve_btfids by
copy-pasting command from make output.

Hence, I find this piece of code ugly.

[...]

Powered by blists - more mailing lists