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:
>
> [...]
>
> > diff --git a/btf_encoder.c b/btf_encoder.c
>
>
> [...]
>
> > +static struct elf_functions *elf_functions__new(Elf *elf)
> > +{
>
>
> I think this function should be renamed to elf_functions_list_init()
> and it should return int.
The elf_functions_list itself is static, this line initializes it:
static LIST_HEAD(elf_functions_list);
elf_functions__new() creates a new (single) elf_functions struct,
initializes it by collecting the functions, and adds it to the static
list.
Maybe __new is too cryptic. What do you think about renaming to
elf_functions_list__add(), or maybe even split into
elf_functions__new() and _list__add()?
>
> [...]
>
> > @@ -2071,6 +2139,13 @@ int btf_encoder__encode(struct btf_encoder *encoder)
> > #endif
> > err = btf_encoder__write_elf(encoder, encoder->btf, BTF_ELF_SEC);
> > }
> > +
> > + // TODO: after moving encoders to shared elf_functions,
> > + // replace elf_functions__get(encoder->functions.elf) here
> > + // with encoder->functions
> > + // The pointer to shared elf_functions will be set in btf_encoder__new
>
>
> Nit: the TODO comment is not necessary here, as described change is
> done later in the patch.
>
> > + elf_functions__delete(elf_functions__get(encoder->functions.elf));
> > +
> > return err;
> > }
> >
> > @@ -2369,6 +2444,7 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
> > goto out;
> > }
> > encoder->functions.symtab = encoder->symtab;
> > + encoder->functions.elf = cu->elf;
>
>
> This line is removed in a subsequent patches, is it needed because of
> the __delete call in the hunk above? Note, that if you want to account
> for situation when multiple ELFs are processed the cu->elf might
>
> change, so that __delete call would only delete elf_functions for the
> last processed cu->elf.
>
I wanted to keep intermediate patches small while maintaining that
nothing is broken. These lines are necessary, because on this patch
each encoder is still building it's own table, and each table needs a
reference to the elf.
If I merge patches #3-#5, these changes would disappear. I can try
that, but then patch #3 will be quite big to review. What do you think?
>
> I think that elf_functions should be deleted in main() at the same
> level where btf_encoder__add_saved_funcs() is called.
>
> > /* index the ELF sections for later lookup */
In this patch, the shared elf_functions is built but not used. It is
freed in btf_encoder__encode(), the line with TODO comment.
>
>
> [...]
