On Wednesday, January 1st, 2025 at 8:56 AM, Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
>
> 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
ok
>
> > - 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
I am trying to adhere to the kernel style, although not very strictly.
It's recommended [1] to have a single exit from a function when there
is cleanup work.
I usually check my patches with a script [2] before submitting.
[1] https://www.kernel.org/doc/html/v4.10/process/coding-style.html#centralized-exiting-of-functions
[2] https://github.com/torvalds/linux/blob/master/scripts/checkpatch.pl
>
> > + }
> > +
> > + 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?
How many chars in a line is too many? :)
>
> > - 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
>
> [...]
