Re: [PATCHv3 bpf-next 11/26] libbpf: Add elf_resolve_pattern_offsets function

[Jiri Olsa <olsajiri@gmail.com>]

On Thu, Jul 06, 2023 at 08:52:13PM -0700, Andrii Nakryiko wrote:
> On Fri, Jun 30, 2023 at 1:36 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding elf_resolve_pattern_offsets function that looks up
> > offsets for symbols specified by pattern argument.
> >
> > The 'pattern' argument allows wildcards (*?' supported).
> >
> > Offsets are returned in allocated array together with its
> > size and needs to be released by the caller.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/lib/bpf/elf.c             | 57 +++++++++++++++++++++++++++++++++
> >  tools/lib/bpf/libbpf.c          |  2 +-
> >  tools/lib/bpf/libbpf_elf.h      |  3 ++
> >  tools/lib/bpf/libbpf_internal.h |  1 +
> >  4 files changed, 62 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/lib/bpf/elf.c b/tools/lib/bpf/elf.c
> > index 7e2f3b2e1fb6..f2d1a8cc2f9d 100644
> > --- a/tools/lib/bpf/elf.c
> > +++ b/tools/lib/bpf/elf.c
> > @@ -376,3 +376,60 @@ int elf_resolve_syms_offsets(const char *binary_path, int cnt,
> >         elf_close(&elf_fd);
> >         return err;
> >  }
> > +
> 
> same, leave comment that caller should free offsets on success?

yes, will add

> 
> > +int elf_resolve_pattern_offsets(const char *binary_path, const char *pattern,
> > +                               unsigned long **poffsets, size_t *pcnt)
> > +{
> > +       int sh_types[2] = { SHT_DYNSYM, SHT_SYMTAB };
> > +       unsigned long *offsets = NULL;
> > +       size_t cap = 0, cnt = 0;
> > +       struct elf_fd elf_fd;
> > +       int err = 0, i;
> > +
> > +       err = elf_open(binary_path, &elf_fd);
> > +       if (err)
> > +               return err;
> > +
> > +       for (i = 0; i < ARRAY_SIZE(sh_types); i++) {
> > +               struct elf_sym_iter iter;
> > +               struct elf_sym *sym;
> > +
> > +               err = elf_sym_iter_new(&iter, elf_fd.elf, binary_path, sh_types[i], STT_FUNC);
> > +               if (err) {
> > +                       if (err == -ENOENT)
> > +                               continue;
> > +                       goto out;
> > +               }
> 
> ditto, minimize nesting, please

ok

> 
> > +
> > +               while ((sym = elf_sym_iter_next(&iter))) {
> > +                       if (!glob_match(sym->name, pattern))
> > +                               continue;
> > +
> > +                       err = libbpf_ensure_mem((void **) &offsets, &cap, sizeof(*offsets),
> > +                                               cnt + 1);
> > +                       if (err)
> > +                               goto out;
> > +
> > +                       offsets[cnt++] = elf_sym_offset(sym);
> > +               }
> > +
> > +               /* If we found anything in the first symbol section,
> > +                * do not search others to avoid duplicates.
> 
> DYNSYM is going to have only exposed symbols, so for this pattern
> matching, maybe it's best to start with SYMTAB and only fallback to
> DYNSYM if we didn't find anything in SYMTAB (more realistically it
> would be that SYMTAB section is missing, so we fallback to DYNSYM;
> otherwise neither DYNSYM nor SYMTAB will have matching symbols, most
> probably, but that's minor)

makes sense, will switch

> 
> other than that, LGTM

thanks,
jirka