Re: [PATCH v3 dwarves 2/5] btf_encoder: introduce elf_functions struct type

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

On 16/10/2024 01:10, Ihor Solodrai wrote:
> Extract elf_functions struct type from btf_encoder.
> 
> Replace methods operating functions table in btf_encoder by methods
> operating on elf_functions:
> - btf_encoder__collect_function -> elf_functions__collect_function
> - btf_encoder__collect_symbols -> elf_functions__collect
> 
> Now these functions do not depend on btf_encoder being passed to them
> as a parameter.
> 
> Signed-off-by: Ihor Solodrai <ihor.solodrai@pm.me>

a few small things, but

Reviewed-by: Alan Maguire <alan.maguire@oracle.com>

> ---
>  btf_encoder.c | 122 +++++++++++++++++++++++++++-----------------------
>  1 file changed, 66 insertions(+), 56 deletions(-)
> 
> diff --git a/btf_encoder.c b/btf_encoder.c
> index 5954238..9c840fa 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -102,6 +102,13 @@ struct elf_secinfo {
>  	struct gobuffer secinfo;
>  };
>  
> +struct elf_functions {
> +	struct elf_symtab *symtab;
> +	struct elf_function *entries;
> +	int cnt;
> +	int suffix_cnt; /* number of .isra, .part etc */
> +};
> +
>  /*
>   * cu: cu being processed.
>   */
> @@ -126,12 +133,7 @@ struct btf_encoder {
>  	struct elf_secinfo *secinfo;
>  	size_t             seccnt;
>  	int                encode_vars;
> -	struct {
> -		struct elf_function *entries;
> -		int		    allocated;
> -		int		    cnt;
> -		int		    suffix_cnt; /* number of .isra, .part etc */
> -	} functions;
> +	struct elf_functions functions;
>  };
>  
>  struct btf_func {
> @@ -1299,55 +1301,28 @@ static int functions_cmp(const void *_a, const void *_b)
>  	return strcmp(a->name, b->name);
>  }
>  
> -#ifndef max
> -#define max(x, y) ((x) < (y) ? (y) : (x))
> -#endif
> -
> -static void *reallocarray_grow(void *ptr, int *nmemb, size_t size)
> -{
> -	int new_nmemb = max(1000, *nmemb * 3 / 2);
> -	void *new = realloc(ptr, new_nmemb * size);
> -
> -	if (new)
> -		*nmemb = new_nmemb;
> -	return new;
> -}
> -
> -static int btf_encoder__collect_function(struct btf_encoder *encoder, GElf_Sym *sym)
> +static int elf_functions__collect_function(struct elf_functions *functions, GElf_Sym *sym)
>  {
> -	struct elf_function *new;
> +	struct elf_function *func;
>  	const char *name;
>  
>  	if (elf_sym__type(sym) != STT_FUNC)
>  		return 0;
> -	name = elf_sym__name(sym, encoder->symtab);
> +
> +	name = elf_sym__name(sym, functions->symtab);
>  	if (!name)
>  		return 0;
>  
> -	if (encoder->functions.cnt == encoder->functions.allocated) {
> -		new = reallocarray_grow(encoder->functions.entries,
> -					&encoder->functions.allocated,
> -					sizeof(*encoder->functions.entries));
> -		if (!new) {
> -			/*
> -			 * The cleanup - delete_functions is called
> -			 * in btf_encoder__encode_cu error path.
> -			 */
> -			return -1;
> -		}
> -		encoder->functions.entries = new;
> -	}
> -
> -	memset(&encoder->functions.entries[encoder->functions.cnt], 0,
> -	       sizeof(*new));
> -	encoder->functions.entries[encoder->functions.cnt].name = name;
> +	func = &functions->entries[functions->cnt];
> +	func->name = name;
>  	if (strchr(name, '.')) {
>  		const char *suffix = strchr(name, '.');
> -
> -		encoder->functions.suffix_cnt++;
> -		encoder->functions.entries[encoder->functions.cnt].prefixlen = suffix - name;
> +		functions->suffix_cnt++;
> +		func->prefixlen = suffix - name;
>  	}
> -	encoder->functions.cnt++;
> +
> +	functions->cnt++;
> +
>  	return 0;
>  }
>  
> @@ -2099,26 +2074,60 @@ int btf_encoder__encode(struct btf_encoder *encoder)
>  	return err;
>  }
>  
> -
> -static int btf_encoder__collect_symbols(struct btf_encoder *encoder)
> +static int elf_functions__collect(struct elf_functions *functions)
>  {
> -	uint32_t sym_sec_idx;
> +	uint32_t nr_symbols = elf_symtab__nr_symbols(functions->symtab);
> +	struct elf_function *tmp;
> +	Elf32_Word sym_sec_idx;
>  	uint32_t core_id;
>  	GElf_Sym sym;
> +	int err;
>  
> -	elf_symtab__for_each_symbol_index(encoder->symtab, core_id, sym, sym_sec_idx) {
> -		if (btf_encoder__collect_function(encoder, &sym))
> -			return -1;
> +	/* We know that number of functions is less than number of symbols,
> +	 * so we can overallocate temporarily.
> +	 */
> +	functions->entries = calloc(nr_symbols, sizeof(struct elf_function));

So in testing  we hit a case (a module) with no functions, I presume a
case with zero symbols is extremely unlikely, but maybe just in case,

if (nr_symbols == 0)
	goto out_free;

(we should probably just initialize int err = 0; above)

> +	if (!functions->entries) {
> +		fprintf(stderr, "could not allocate memory for elf_functions table\n");
> +		err = -ENOMEM;
> +		goto out_free;
> +	}
> +
> +	functions->cnt = 0;
> +	elf_symtab__for_each_symbol_index(functions->symtab, core_id, sym, sym_sec_idx) {
> +		if (elf_functions__collect_function(functions, &sym)) {
> +			err = -1;
> +			goto out_free;
> +		}
>  	}
>  
> -	if (encoder->functions.cnt) {
> -		qsort(encoder->functions.entries, encoder->functions.cnt, sizeof(encoder->functions.entries[0]),
> +	if (functions->cnt) {
> +		qsort(functions->entries,
> +		      functions->cnt,
> +		      sizeof(functions->entries[0]),
>  		      functions_cmp);
> -		if (encoder->verbose)
> -			printf("Found %d functions!\n", encoder->functions.cnt);
> +	} else {
> +		err = 0;

nit: as noted above start with err = 0 and we can just goto out_free.

> +		goto out_free;
> +	}
> +
> +	/* Reallocate to the exact size */
> +	tmp = realloc(functions->entries, functions->cnt * sizeof(struct elf_function));
> +	if (tmp) {
> +		functions->entries = tmp;
> +	} else {
> +		fprintf(stderr, "could not reallocate memory for elf_functions table\n");
> +		err = -ENOMEM;
> +		goto out_free;
>  	}
>  
>  	return 0;
> +
> +out_free:
> +	free(functions->entries);
> +	functions->entries = NULL;
> +	functions->cnt = 0;
> +	return err;
>  }
>  
>  static bool ftype__has_arg_names(const struct ftype *ftype)
> @@ -2377,6 +2386,7 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
>  				printf("%s: '%s' doesn't have symtab.\n", __func__, cu->filename);
>  			goto out;
>  		}
> +		encoder->functions.symtab = encoder->symtab;
>  

nit: unless there's a reason otherwise, this assignment seems to more
naturally belong in elf_functions__collect().

>  		/* index the ELF sections for later lookup */
>  
> @@ -2415,7 +2425,7 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
>  		if (!found_percpu && encoder->verbose)
>  			printf("%s: '%s' doesn't have '%s' section\n", __func__, cu->filename, PERCPU_SECTION);
>  
> -		if (btf_encoder__collect_symbols(encoder))
> +		if (elf_functions__collect(&encoder->functions))
>  			goto out_delete;
>  
>  		if (encoder->verbose)
> @@ -2456,7 +2466,7 @@ void btf_encoder__delete(struct btf_encoder *encoder)
>  
>  	for (i = 0; i < encoder->functions.cnt; i++)
>  		btf_encoder__delete_func(&encoder->functions.entries[i]);
> -	encoder->functions.allocated = encoder->functions.cnt = 0;
> +	encoder->functions.cnt = 0;
>  	free(encoder->functions.entries);
>  	encoder->functions.entries = NULL;
>