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
>
>
>
