On Friday, November 29th, 2024 at 12:37 PM, Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
> On Thu, 2024-11-28 at 01:23 +0000, Ihor Solodrai wrote:
>
> [...]
>
> Acked-by: Eduard Zingerman eddyz87@xxxxxxxxx
>
>
> I like what this patch does, a few nits below.
>
> Note:
> this patch leads to 58 less functions being generated,
> compared to a previous patch, for my test configuration.
> For example, functions like:
> - hid_map_usage_clear
> - jhash
> - nlmsg_parse_deprecated_strict
> Are not in the BTF anymore. It would be good if patch message could
> explain why this happens.
Hey Eduard.
I decided to remove patch 2/9 [1] for the v2 of this series, as it is
largely unrelated.
Applying 1/9 and 3/9 (this patch) to next, I couldn't reproduce the
difference you described. With reproducible_build I get identical text
dump of BTFs between next [2] and the changes [3].
It looks to me the difference you noted here is caused by introduction
of section-relative addresses in the patch 2/9.
>
> [...]
>
> > +static int btf_encoder__add_saved_funcs(struct btf_encoder *encoder)
> > +{
> > + struct btf_encoder_func_state **saved_fns, *s;
> > + struct btf_encoder e = NULL;
> > + int i = 0, j, nr_saved_fns = 0;
> > +
> > + / Retrieve function states from each encoder, combine them
> > + * and sort by name, addr.
> > + /
> > + btf_encoders__for_each_encoder(e) {
> > + list_for_each_entry(s, &e->func_states, node)
> > + nr_saved_fns++;
> > + }
> > + / Another thread already did this work */
> > + if (nr_saved_fns == 0) {
> > + printf("nothing to do for encoder...\n");
> > + return 0;
> > + }
>
>
> Nit: this function is called from pahole_threads_collect():
>
> static int pahole_threads_collect(...)
> for (i = 0; i < nr_threads; i++)
> ...
> err = btf_encoder__add_encoder(btf_encoder, threads[i]->encoder);
>
> ...
>
> int32_t btf_encoder__add_encoder(struct btf_encoder *encoder, struct btf_encoder *other)
> ...
> btf_encoder__add_saved_funcs(other);
> ...
>
> maybe move call to btf_encoder__add_saved_funcs() to pahole_threads_collect()
> outside of the loop? So that comment about another thread won't be necessary.
Done in [3].
>
> [...]
>
> > @@ -2437,16 +2470,8 @@ out_delete:
> > return NULL;
> > }
> >
> > -void btf_encoder__delete_func(struct elf_function *func)
> > -{
> > - free(func->alias);
>
>
> Nit: it looks like func->alias is never freed after this change.
Yeap. Fixed in [3].
Sanitizer also caught leaked secinfo here, the one you noted in a
different thread [4]. Fixed in [3] as well.
[1]: https://lore.kernel.org/dwarves/20241128012341.4081072-3-ihor.solodrai@xxxxx/
[2]: https://git.kernel.org/pub/scm/devel/pahole/pahole.git/commit/?h=next&id=3ddadc131586d6f3aa68775636adff5f7e7ff0f0
[3]: https://github.com/acmel/dwarves/commit/20ef1cd5131d340caaa4719e980b4da77c345579
[4]: https://lore.kernel.org/dwarves/e1df45360963d265ea5e0b3634f0a3dae0c9c343.camel@xxxxxxxxx/
>
> > - zfree(&func->state.annots);
> > - zfree(&func->state.parms);
> > -}
>
>
> [...]
