Re: [PATCHv2 bpf-next 06/24] libbpf: Add elf symbol iterator

Andrii Nakryiko
2023-06-23T16:29:29Z

On Fri, Jun 23, 2023 at 1:19 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Thu, Jun 22, 2023 at 05:31:58PM -0700, Andrii Nakryiko wrote:
> > On Tue, Jun 20, 2023 at 1:36 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > Adding elf symbol iterator object (and some functions) that follow
> > > open-coded iterator pattern and some functions to ease up iterating
> > > elf object symbols.
> > >
> > > The idea is to iterate single symbol section with:
> > >
> > > struct elf_symbol_iter iter;
> > > struct elf_symbol *sym;
> > >
> > > if (elf_symbol_iter_new(&iter, elf, binary_path, SHT_DYNSYM))
> > > goto error;
> > >
> > > while ((sym = elf_symbol_iter_next(&iter))) {
> > > ...
> > > }
> > >
> > > I considered opening the elf inside the iterator and iterate all symbol
> > > sections, but then it gets more complicated wrt user checks for when
> > > the next section is processed.
> > >
> > > Plus side is the we don't need 'exit' function, because caller/user is
> > > in charge of that.
> > >
> > > The returned iterated symbol object from elf_symbol_iter_next function
> > > is placed inside the struct elf_symbol_iter, so no extra allocation or
> > > argument is needed.
> > >
> > > Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > > tools/lib/bpf/libbpf.c | 179 ++++++++++++++++++++++++++---------------
> > > 1 file changed, 114 insertions(+), 65 deletions(-)
> > >
> >
> > This is great. Left a few nits below. I'm thinkin maybe we should add
> > a separate elf.c file for all these ELF-related helpers and start
> > offloading code from libbpf.c, which got pretty big already. WDYT?
>
> yes, I thought doing the move after this is merged might be better,
> because it's quite big already

true (and btw, please give me a bit more time to review the rest of
patches before posting a new version), but I'm ok with just going
straight to elf.c move. Either way it's a lot of +++ and ---, whether
it's in the same file or not doesn't matter all that much.

>
> >
> >
> > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > index af52188daa80..cdac368c7ce1 100644
> > > --- a/tools/lib/bpf/libbpf.c
> > > +++ b/tools/lib/bpf/libbpf.c
> > > @@ -10824,6 +10824,109 @@ static Elf_Scn *elf_find_next_scn_by_type(Elf *elf, int sh_type, Elf_Scn *scn)
> > > return NULL;
> > > }
> > >
> > > +struct elf_symbol {
> > > + const char *name;
> > > + unsigned long offset;
> > > + int bind;
> > > +};
> > > +
> > > +struct elf_symbol_iter {
> > …