Re: [PATCH v3 dwarves 3/5] btf_encoder: collect elf_functions in btf_encoder__pre_load_module

[Alan Maguire <alan.maguire@oracle.com>]

On 16/10/2024 01:10, Ihor Solodrai wrote:
> Introduce a global elf_functions_list variable in btf_encoder.c that
> contains an elf_functions per ELF.
>

Arnaldo can help provide context here, but at least notionally I think
the idea of maintaining libdwarves as a library has value. In that
context, avoiding global lists where possible is a good thing I think,
since if it was used as a library, multiple invokations could confuse
the elf_functions list. To that end, can we make the elf_functions_list
a field in the conf_load perhaps? It already contains base_btf so there
is a precedent for storing data relevant to all encoders there, and
btf_encoder__new() has conf_load as a parameter, so the elf functions
list can still always be retrieved on encoder creation. In addition the
parameter to cus__process_dwflmod() has the parms structure which
contains the conf_load; you'd just need to pass that through to your
pre_load_module() callback I think.

It shouldn't be a massive change but I think it would be worthwhile.

Thanks!

> An elf_functions structure is allocated and filled out by
> btf_encoder__pre_load_module() hook, and the list is cleared after
> btf_encoder__encode() is done.
> 
> At this point btf_encoders don't use shared elf_functions yet (each
> maintains their own copy as before), but it is built before encoders
> are initialized.
> 
> Signed-off-by: Ihor Solodrai <ihor.solodrai@pm.me>
> ---
>  btf_encoder.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  btf_encoder.h |  2 ++
>  pahole.c      |  3 +++
>  3 files changed, 71 insertions(+)
> 
> diff --git a/btf_encoder.c b/btf_encoder.c
> index 9c840fa..8e8fd05 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -103,6 +103,8 @@ struct elf_secinfo {
>  };
>  
>  struct elf_functions {
> +	struct list_head node; /* for elf_functions_list */
> +	Elf *elf; /* source ELF */
>  	struct elf_symtab *symtab;
>  	struct elf_function *entries;
>  	int cnt;
> @@ -147,6 +149,67 @@ struct btf_kfunc_set_range {
>  	uint64_t end;
>  };
>  
> +
> +/* In principle, multiple ELFs can be processed in one pahole run,
> + * so we have to store elf_functions table per ELF.
> + * An element is added to the list on btf_encoder__pre_load_module,
> + * and removed after btf_encoder__encode is done.
> + */
> +static LIST_HEAD(elf_functions_list);
> +
> +static inline void elf_functions__delete(struct elf_functions *funcs)
> +{
> +	free(funcs->entries);
> +	elf_symtab__delete(funcs->symtab);
> +	list_del(&funcs->node);
> +	free(funcs);
> +}
> +
> +static inline void elf_functions__delete_all(void)
> +{
> +	struct list_head *pos, *tmp;
> +
> +	list_for_each_safe(pos, tmp, &elf_functions_list) {
> +		struct elf_functions *funcs = list_entry(pos, struct elf_functions, node);
> +
> +		elf_functions__delete(funcs);
> +	}
> +}
> +
> +static int elf_functions__collect(struct elf_functions *functions);
> +
> +int btf_encoder__pre_load_module(Dwfl_Module *mod, Elf *elf)
> +{
> +	struct elf_functions *funcs;
> +	int err;
> +
> +	funcs = calloc(1, sizeof(*funcs));
> +	if (!funcs) {
> +		err = -ENOMEM;
> +		goto out_delete;
> +	}
> +
> +	funcs->symtab = elf_symtab__new(NULL, elf);
> +	if (!funcs->symtab) {
> +		err = -1;
> +		goto out_delete;
> +	}
> +
> +	funcs->elf = elf;
> +	err = elf_functions__collect(funcs);
> +	if (err)
> +		goto out_delete;
> +
> +	list_add_tail(&funcs->node, &elf_functions_list);
> +
> +	return 0;
> +
> +out_delete:
> +	elf_functions__delete(funcs);
> +	return err;
> +}
> +
> +
>  static LIST_HEAD(encoders);
>  static pthread_mutex_t encoders__lock = PTHREAD_MUTEX_INITIALIZER;
>  
> @@ -2071,6 +2134,8 @@ int btf_encoder__encode(struct btf_encoder *encoder)
>  #endif
>  		err = btf_encoder__write_elf(encoder, encoder->btf, BTF_ELF_SEC);
>  	}
> +
> +	elf_functions__delete_all();
>  	return err;
>  }
>  
> @@ -2387,6 +2452,7 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
>  			goto out;
>  		}
>  		encoder->functions.symtab = encoder->symtab;
> +		encoder->functions.elf = cu->elf;
>  
>  		/* index the ELF sections for later lookup */
>  
> diff --git a/btf_encoder.h b/btf_encoder.h
> index 824963b..7debd67 100644
> --- a/btf_encoder.h
> +++ b/btf_encoder.h
> @@ -34,4 +34,6 @@ struct btf *btf_encoder__btf(struct btf_encoder *encoder);
>  
>  int btf_encoder__add_encoder(struct btf_encoder *encoder, struct btf_encoder *other);
>  
> +int btf_encoder__pre_load_module(Dwfl_Module *mod, Elf *elf);
> +
>  #endif /* _BTF_ENCODER_H_ */
> diff --git a/pahole.c b/pahole.c
> index b9e97ef..891af3a 100644
> --- a/pahole.c
> +++ b/pahole.c
> @@ -3814,6 +3814,9 @@ int main(int argc, char *argv[])
>  		conf_load.threads_collect = pahole_threads_collect;
>  	}
>  
> +	if (btf_encode)
> +		conf_load.pre_load_module = btf_encoder__pre_load_module;
> +
>  	// Make 'pahole --header type < file' a shorter form of 'pahole -C type --count 1 < file'
>  	if (conf.header_type && !class_name && prettify_input) {
>  		conf.count = 1;