On Thursday, October 10th, 2024 at 1:57 AM, Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
>
> On Wed, 2024-10-09 at 23:35 +0000, Ihor Solodrai wrote:
>
> > Extract elf_functions struct type from btf_encoder.
> >
> > Rewrite btf_encoder__collect_function() into
> > elf_functions__collect_function(), and btf_encoder__collect_symbols()
> > into elf_functions__collect().
>
>
> Nit:
>
> the sentence above is somewhat cryptic. I think it's better to say
> something like:
>
> Extract struct elf_functions 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.
ok
>
> > Now these functions do not depend on btf_encoder being passed to them
> > as a parameter.
> >
> > Signed-off-by: Ihor Solodrai ihor.solodrai@xxxxx
> > ---
>
>
> Modulo a few nitpicks this patch looks ok to me.
>
> Reviewed-by: Eduard Zingerman eddyz87@xxxxxxxxx
>
>
> [...]
>
> > @@ -2100,22 +2075,38 @@ int btf_encoder__encode(struct btf_encoder *encoder)
> > }
> >
> > -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);
> > + Elf32_Word sym_sec_idx;
> > uint32_t core_id;
> > GElf_Sym sym;
> >
> > - elf_symtab__for_each_symbol_index(encoder->symtab, core_id, sym, sym_sec_idx) {
> > - if (btf_encoder__collect_function(encoder, &sym))
> > + // 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));
> > + if (!functions->entries) {
> > + fprintf(stderr, "could not allocate memory for elf_functions table\n");
> > + return -ENOMEM;
> > + }
> > +
> > + functions->cnt = 0;
> > + elf_symtab__for_each_symbol_index(functions->symtab, core_id, sym, sym_sec_idx) {;
> > + if (elf_functions__collect_function(functions, &sym))
> > return -1;
> > }
> >
> > - 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);
> > +
> > + // Reallocate to the exact size
> > + functions->entries = realloc(functions->entries, functions->cnt * sizeof(struct elf_function));
>
>
> You need to store realloc result to a temporary, otherwise pointer to
> previously allocated memory in functions->entries is lost.
Good point.
>
> (Although idk if such realloc could fail).
It might fail in principle, although it seems very unlikely [1].
[1] https://stackoverflow.com/questions/12125308/can-realloc-fail-return-null-when-trimming
>
> > + if (!functions->entries) {
> > + fprintf(stderr, "could not reallocate memory for elf_functions table\n");
> > + return -ENOMEM;
> > }
> >
> > return 0;
>
>
> [...]
