Re: [PATCH dwarves 1/3] btf_encoder: collect kfuncs info in btf_encoder__new

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

On 2/10/25 12:57 PM, Eduard Zingerman wrote:
> On Thu, 2025-02-06 at 18:14 -0800, Ihor Solodrai wrote:
>> From: Ihor Solodrai <ihor.solodrai@pm.me>
>>
>> btf_encoder__tag_kfuncs() is a post-processing step of BTF encoding,
>> executed right before BTF is deduped and dumped to the output.
>>
>> Split btf_encoder__tag_kfuncs() routine in two parts:
>>   * btf_encoder__collect_kfuncs()
>>   * btf_encoder__tag_kfuncs()
>>
>> [...]
>
> Tbh, I don't think this split is necessary, modifying btf_type
> in-place should be fine (and libbpf does it at-least in one place).
> E.g. like here:
> https://github.com/acmel/dwarves/compare/master...eddyz87:dwarves:arena-attrs-no-split
> I like it because it keeps the change a bit more contained,
> but I do not insist.

There are a couple of reasons this split makes sense to me.

First, I wanted to avoid modifying BTF. Having btf_encoder only
appending things to BTF is easy to reason about. But you're saying
modification does happen somewhere already?

The second reason is that the input for kfunc tagging is ELF, and so
it can be read at around the same time other ELF data is read (such as
for fucntions table). This has an additional benefit of running in
parallel to dwarf encoders (because one of the dwarf workers is
creating btf_encoder struct), as opposed to a sequential
post-processing step.

Finally I think it is generally useful to have kfunc info available at
any point of btf encoding, which becomes possible if the BTF_ids
section is read at the beginning of the encoding process.

>
> [...]
>>  
>> -		err = btf_encoder__tag_kfunc(encoder, &btf_funcs, func, pair->flags);
>> -		if (err) {
>> -			fprintf(stderr, "%s: failed to tag kfunc '%s'\n", __func__, func);
>> -			free(func);
>> +		elf_fn = btf_encoder__find_function(encoder, func, 0);
>> +		free(func);
>> +		if (!elf_fn)
>> +			continue;
>> +		elf_fn->kfunc = true;
>> +
>> +		kfunc = calloc(1, sizeof(*kfunc));
>> +		if (!kfunc) {
>> +			fprintf(stderr, "%s: failed to allocate memory for kfunc info\n", __func__);
>> +			err = -ENOMEM;
>>  			goto out;
>>  		}
>> -		free(func);
>> +		kfunc->id = pair->id;
>> +		kfunc->flags = pair->flags;
>> +		kfunc->name = elf_fn->name;
>
> If we do go with split, maybe make refactoring a bit more drastic and
> merge kfunc_info with elf_function?
> This would make maintaining a separate encoder->kfuncs list unnecessary.
> Also, can get rid of separate 'struct gobuffer *funcs'.
> E.g. see my commit on top of yours:
> https://github.com/acmel/dwarves/compare/master...eddyz87:dwarves:arena-attrs-merge-kfunc-info

Yeah, I like it. I'll play with this for v2, thanks.

>
>> +		list_add(&kfunc->node, &encoder->kfuncs);
>>  	}
>>  
>>  	err = 0;
>>  out:
>> -	__gobuffer__delete(&btf_funcs);
>>  	__gobuffer__delete(&btf_kfunc_ranges);
>>  	if (elf)
>>  		elf_end(elf);
>> @@ -2081,6 +2095,34 @@ out:
>>  	return err;
>>  }
>>  
>
> [...]
>