Re: [PATCH dwarves v3 3/8] btf_encoder: introduce elf_functions struct type

[Jiri Olsa <olsajiri@gmail.com>]

On Sat, Dec 21, 2024 at 01:23:10AM +0000, Ihor Solodrai wrote:

SNIP

> -static int btf_encoder__collect_function(struct btf_encoder *encoder, GElf_Sym *sym)
> +static void 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);
> -	if (!name)
> -		return 0;
> +		return;
>  
> -	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;
> -	}
> +	name = elf_sym__name(sym, functions->symtab);
> +	if (!name)
> +		return;
>  
> -	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, '.');
> -

nit, let's keep that new line after declaration

> -		encoder->functions.suffix_cnt++;
> -		encoder->functions.entries[encoder->functions.cnt].prefixlen = suffix - name;
> +		functions->suffix_cnt++;
> +		func->prefixlen = suffix - name;
>  	} else {
> -		encoder->functions.entries[encoder->functions.cnt].prefixlen = strlen(name);
> +		func->prefixlen = strlen(name);
>  	}
> -	encoder->functions.cnt++;
> -	return 0;
> +
> +	functions->cnt++;
>  }
>  
>  static struct elf_function *btf_encoder__find_function(const struct btf_encoder *encoder,
> @@ -2126,26 +2103,56 @@ 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(*functions->entries));
> +	if (!functions->entries) {
> +		err = -ENOMEM;
> +		goto out_free;

you could just return -ENOMEM here

> +	}
> +
> +	functions->cnt = 0;
> +	elf_symtab__for_each_symbol_index(functions->symtab, core_id, sym, sym_sec_idx) {
> +		elf_functions__collect_function(functions, &sym);
>  	}
>  
> -	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),
>  		      functions_cmp);

nit, why not keep the single line?

> -		if (encoder->verbose)
> -			printf("Found %d functions!\n", encoder->functions.cnt);
> +	} else {
> +		err = 0;
> +		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)
> @@ -2406,6 +2413,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;

I was wondering if we need to keep both symtab pointers, but it's sorted
out in the next patch ;-)
 
thanks,
jirka

>  
>  		/* index the ELF sections for later lookup */
>  
> @@ -2444,7 +2452,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)
> @@ -2476,7 +2484,7 @@ void btf_encoder__delete(struct btf_encoder *encoder)
>  	encoder->btf = NULL;
>  	elf_symtab__delete(encoder->symtab);
>  
> -	encoder->functions.allocated = encoder->functions.cnt = 0;
> +	encoder->functions.cnt = 0;
>  	free(encoder->functions.entries);
>  	encoder->functions.entries = NULL;
>  
> -- 
> 2.47.1
> 
> 
>