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

[Eduard Zingerman <eddyz87@gmail.com>]

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()
> 
> btf_encoder__collect_kfuncs() reads the .BTF_ids section of the ELF,
> collecting kfunc information into a list of kfunc_info structs in the
> btf_encoder. It is executed in btf_encoder__new() when tag_kfuncs flag
> is set. This way kfunc information is available during entire lifetime
> of the btf_encoder.
> 
> btf_encoder__tag_kfuncs() is basically the same: collect BTF
> functions, and then for each kfunc find and tag correspoding BTF
> func. Except now kfunc information is not collected in-place, but is
> simply read from the btf_encoder.
> 
> Signed-off-by: Ihor Solodrai <ihor.solodrai@linux.dev>
> ---

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.

[...]

> @@ -1876,11 +1886,10 @@ static int btf_encoder__tag_kfunc(struct btf_encoder *encoder, struct gobuffer *
>  	return 0;
>  }
>  
> -static int btf_encoder__tag_kfuncs(struct btf_encoder *encoder)
> +static int btf_encoder__collect_kfuncs(struct btf_encoder *encoder)
>  {
>  	const char *filename = encoder->source_filename;
>  	struct gobuffer btf_kfunc_ranges = {};
> -	struct gobuffer btf_funcs = {};
>  	Elf_Data *symbols = NULL;
>  	Elf_Data *idlist = NULL;
>  	Elf_Scn *symscn = NULL;
> @@ -1897,6 +1906,8 @@ static int btf_encoder__tag_kfuncs(struct btf_encoder *encoder)
>  	int nr_syms;
>  	int i = 0;
>  
> +	INIT_LIST_HEAD(&encoder->kfuncs);
> +

Nit: do this in the btf_encoder__new?

>  	fd = open(filename, O_RDONLY);
>  	if (fd < 0) {
>  		fprintf(stderr, "Cannot open %s\n", filename);
> @@ -1977,12 +1988,6 @@ static int btf_encoder__tag_kfuncs(struct btf_encoder *encoder)
>  	}
>  	nr_syms = shdr.sh_size / shdr.sh_entsize;
>  
> -	err = btf_encoder__collect_btf_funcs(encoder, &btf_funcs);
> -	if (err) {
> -		fprintf(stderr, "%s: failed to collect BTF funcs\n", __func__);
> -		goto out;
> -	}
> -
>  	/* First collect all kfunc set ranges.
>  	 *
>  	 * Note we choose not to sort these ranges and accept a linear
> @@ -2015,12 +2020,13 @@ static int btf_encoder__tag_kfuncs(struct btf_encoder *encoder)
>  	for (i = 0; i < nr_syms; i++) {
>  		const struct btf_kfunc_set_range *ranges;
>  		const struct btf_id_and_flag *pair;
> +		struct elf_function *elf_fn;
> +		struct kfunc_info *kfunc;
>  		unsigned int ranges_cnt;
>  		char *func, *name;
>  		ptrdiff_t off;
>  		GElf_Sym sym;
>  		bool found;
> -		int err;
>  		int j;
>  
>  		if (!gelf_getsym(symbols, i, &sym)) {
> @@ -2061,18 +2067,26 @@ static int btf_encoder__tag_kfuncs(struct btf_encoder *encoder)
>  			continue;
>  		}
>  
> -		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

> +		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;
>  }
>  

[...]