On Mon, 2024-10-14 at 19:03 +0000, Ihor Solodrai wrote:
[...]
> 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()?
On a second thought, it is called only from `btf_encoder__pre_cus__load_module`,
hence I don't think it is needed as a separate function at all.
[...]
> > > @@ -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.
The accompanying __delete call would happen here:
int btf_encoder__encode(struct btf_encoder *encoder)
...
elf_functions__delete(elf_functions__get(encoder->functions.elf));
Called from here:
int main(int argc, char *argv[])
...
err = btf_encoder__encode(btf_encoder);
Encoders are per thread and elf table is reassigned upon each call to
btf_encoder__new, so assume the following scenario:
- single encoder;
- multiple ELFs processed.
In such scenario encoder->functions.elf would point to the last ELF processed,
thus function tables corresponding to other ELFs won't be deleted.
(but see my suggestion below).
> 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 patch #3 is good as a separate patch.
> > 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.
My point was that the __delete call for all collected elf_function objects
should be moved to main(). But maybe use `module_post_load()` hook instead,
look for specific `elf` and delete it from the list? This way it would be
both symmetrical to `module_pre_load()` and encapsulated within btf_encoder.c.
