On 2/10/25 12:57 PM, Eduard Zingerman wrote: > On Thu, 2025-02-06 at 18:14 -0800, Ihor Solodrai wrote: >> From: Ihor Solodrai <ihor.solodrai@xxxxx> >> >> btf_encoder__tag_kfuncs() is a post-processing step of BTF encoding, >> executed right before BTF is deduped and dumped to the output. >> >> Split btf_encoder__tag_kfuncs() routine in two parts: >> * btf_encoder__collect_kfuncs() >> * btf_encoder__tag_kfuncs() >> >> [...] > > Tbh, I don't think this split is necessary, modifying btf_type > in-place should be fine (and libbpf does it at-least in one place). > E.g. like here: > https://github.com/acmel/dwarves/compare/master...eddyz87:dwarves:arena-attrs-no-split > I like it because it keeps the change a bit more contained, > but I do not insist. There are a couple of reasons this split makes sense to me. First, I wanted to avoid modifying BTF. Having btf_encoder only appending things to BTF is easy to reason about. But you're saying modification does happen somewhere already? The second reason is that the input for kfunc tagging is ELF, and so it can be read at around the same time other ELF data is read (such as for fucntions table). This has an additional benefit of running in parallel to dwarf encoders (because one of the dwarf workers is creating btf_encoder struct), as opposed to a sequential post-processing step. Finally I think it is generally useful to have kfunc info available at any point of btf encoding, which becomes possible if the BTF_ids section is read at the beginning of the encoding process. > > [...] >> >> - err = btf_encoder__tag_kfunc(encoder, &btf_funcs, func, pair->flags); >> - if (err) { >> - fprintf(stderr, "%s: failed to tag kfunc '%s'\n", __func__, func); >> - free(func); >> + elf_fn = btf_encoder__find_function(encoder, func, 0); >> + free(func); >> + if (!elf_fn) >> + continue; >> + elf_fn->kfunc = true; >> + >> + kfunc = calloc(1, sizeof(*kfunc)); >> + if (!kfunc) { >> + fprintf(stderr, "%s: failed to allocate memory for kfunc info\n", __func__); >> + err = -ENOMEM; >> goto out; >> } >> - free(func); >> + kfunc->id = pair->id; >> + kfunc->flags = pair->flags; >> + kfunc->name = elf_fn->name; > > If we do go with split, maybe make refactoring a bit more drastic and > merge kfunc_info with elf_function? > This would make maintaining a separate encoder->kfuncs list unnecessary. > Also, can get rid of separate 'struct gobuffer *funcs'. > E.g. see my commit on top of yours: > https://github.com/acmel/dwarves/compare/master...eddyz87:dwarves:arena-attrs-merge-kfunc-info Yeah, I like it. I'll play with this for v2, thanks. > >> + list_add(&kfunc->node, &encoder->kfuncs); >> } >> >> err = 0; >> out: >> - __gobuffer__delete(&btf_funcs); >> __gobuffer__delete(&btf_kfunc_ranges); >> if (elf) >> elf_end(elf); >> @@ -2081,6 +2095,34 @@ out: >> return err; >> } >> > > [...] >
