Re: [PATCH bpf-next v2 4/4] resolve_btfids: change in-place update with raw binary output

[Ihor Solodrai <ihor.solodrai@linux.dev>]

On 12/4/25 8:57 AM, Eduard Zingerman wrote:
> 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.

Ok, it seems you're conflating two separate issues.

There is a requirement to *link* .BTF section into vmlinux, because it
must have a SHF_ALLOC flag, which makes objcopying the section data
insufficient: linker has to do some magic under the hood.

The patch doesn't change this behavior, and this was (and is) covered
in the script comments.

A separate issue is what resolve_btfids does: updates ELF in-place
(before the patch) or outputs detached section data (after patch).

The paragraph in the commit message attempted to explain the decision
to output raw section data. And apparently I did a bad job of
that. I'll rewrite this part it in the next revision.

And I feel I should clarify that I didn't claim that libelf is buggy.
I meant that using it is complicated, which makes resolve_btfids buggy.

> 
>>>> [...]
>>>>  # 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.

I think implicit naming convention makes sense for this script.
The script's top comment describes what it does in detail, including
the output naming.

> 
>>> [...]
>>>>  
>>>> +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.
> 
> [...]