On Wednesday, January 1st, 2025 at 8:56 AM, Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
>
>
> On Sat, Dec 21, 2024 at 01:23:01AM +0000, Ihor Solodrai wrote:
>
> SNIP
>
> > +static int saved_functions_combine(void *_a, void *_b)
> > +{
> > + uint8_t optimized, unexpected, inconsistent;
> > + struct btf_encoder_func_state *a = _a;
> > + struct btf_encoder_func_state *b = _b;
> > + int ret;
> > +
> > + ret = strncmp(a->elf->name, b->elf->name,
> > + max(a->elf->prefixlen, b->elf->prefixlen));
> > + if (ret != 0)
> > + return ret;
> > + optimized = a->optimized_parms | b->optimized_parms;
> > + unexpected = a->unexpected_reg | b->unexpected_reg;
> > + inconsistent = a->inconsistent_proto | b->inconsistent_proto;
> > + if (!unexpected && !inconsistent && !funcs__match(a, b))
> > + inconsistent = 1;
> > + a->optimized_parms = b->optimized_parms = optimized;
> > + a->unexpected_reg = b->unexpected_reg = unexpected;
> > + a->inconsistent_proto = b->inconsistent_proto = inconsistent;
>
>
> do we still need to update the 'b' state object?
Hi Jiri, thanks for review.
Given how this function is used, we don't need to. But this is a kind
of a "merge", and IMO having both states equal on the way out makes
sense.
while (j < nr_saved_fns && saved_functions_combine(saved_fns[i], saved_fns[j]) == 0)
j++;
>
> > +
> > + return 0;
> > +}
> > +
> > +static void btf_encoder__delete_saved_funcs(struct btf_encoder *encoder)
> > +{
> > + struct btf_encoder_func_state *pos, *s;
> > +
> > + list_for_each_entry_safe(pos, s, &encoder->func_states, node) {
> > + list_del(&pos->node);
> > + free(pos->parms);
> > + free(pos->annots);
> > + free(pos);
> > + }
> > +
> > + for (int i = 0; i < encoder->functions.cnt; i++)
> > + free(encoder->functions.entries[i].alias);
> > +}
> > +
> > +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++;
> > + }
>
>
> the encoder loop goes eventualy away, but still would it make to store
> func_states count instead of the loop?
>
> now when there's just single place that stores 'state' it seems like it
> should be straighforward
Yeah.
>
> SNIP
>
> > void btf_encoder__delete(struct btf_encoder *encoder)
> > {
> > - int i;
> > size_t shndx;
> >
> > if (encoder == NULL)
> > @@ -2447,18 +2469,19 @@ void btf_encoder__delete(struct btf_encoder *encoder)
> > btf_encoders__delete(encoder);
> > for (shndx = 0; shndx < encoder->seccnt; shndx++)
> > __gobuffer__delete(&encoder->secinfo[shndx].secinfo);
> > + free(encoder->secinfo);
>
>
> nit, this seems unrelated to this change, should be in separate fix?
IIRC this was also caught by the sanitizer. I'll sliver this into a
separate patch in the next iteration.
>
> thanks,
> jirka
>
> > zfree(&encoder->filename);
> > zfree(&encoder->source_filename);
> > btf__free(encoder->btf);
> > encoder->btf = NULL;
> > elf_symtab__delete(encoder->symtab);
> >
> > - for (i = 0; i < encoder->functions.cnt; i++)
> > - btf_encoder__delete_func(&encoder->functions.entries[i]);
> > encoder->functions.allocated = encoder->functions.cnt = 0;
> > free(encoder->functions.entries);
> > encoder->functions.entries = NULL;
> >
> > + btf_encoder__delete_saved_funcs(encoder);
> > +
> > free(encoder);
> > }
>
>
> SNIP
