Re: [PATCHv2 bpf-next 09/24] libbpf: Add elf_find_pattern_func_offset function

[Jiri Olsa <olsajiri@gmail.com>]

On Fri, Jun 23, 2023 at 01:39:58PM -0700, Andrii Nakryiko wrote:
> On Tue, Jun 20, 2023 at 1:37 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding elf_find_pattern_func_offset 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/libbpf.c          | 78 +++++++++++++++++++++++++++++++++
> >  tools/lib/bpf/libbpf_internal.h |  3 ++
> >  2 files changed, 81 insertions(+)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 1c310b718961..3e5c88caf5d5 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -11165,6 +11165,84 @@ int elf_find_multi_func_offset(const char *binary_path, int cnt,
> >         return ret;
> >  }
> >
> > +static int
> > +__elf_find_pattern_func_offset(Elf *elf, const char *binary_path, const char *pattern,
> > +                              const char ***pnames, unsigned long **poffsets, size_t *pcnt)
> > +{
> > +       int sh_types[2] = { SHT_DYNSYM, SHT_SYMTAB };
> > +       struct elf_symbol_offset *func_offs = NULL;
> > +       unsigned long *offsets = NULL;
> > +       const char **names = NULL;
> > +       size_t func_offs_cnt = 0;
> > +       size_t func_offs_cap = 0;
> > +       int err = 0, i;
> > +
> > +       for (i = 0; i < ARRAY_SIZE(sh_types); i++) {
> > +               struct elf_symbol_iter iter;
> > +               struct elf_symbol *sym;
> > +
> > +               if (elf_symbol_iter_new(&iter, elf, binary_path, sh_types[i]))
> > +                       continue;
> 
> same as in the previous patch, error handling?

ok, same solution as in my previous answer

> 
> > +
> > +               while ((sym = elf_symbol_iter_next(&iter))) {
> > +                       if (!glob_match(sym->name, pattern))
> > +                               continue;
> > +
> > +                       err = libbpf_ensure_mem((void **) &func_offs, &func_offs_cap,
> > +                                               sizeof(*func_offs), func_offs_cnt + 1);
> > +                       if (err)
> > +                               goto out;
> > +
> > +                       func_offs[func_offs_cnt].offset = sym->offset;
> > +                       func_offs[func_offs_cnt].name = strdup(sym->name);
> 
> check for NULL?
> 
> and I'm actually unsure why you need to reuse elf_symbol_offset struct
> here? You just need names and offsets in two separate array, so just
> do that?..

I guess it seemed easier to do just single libbpf_ensure_mem call,
then doing two and maintain cap/cnt for both arrays.. but right,
I guess there will be good solution without the extra array

> 
> > +                       func_offs_cnt++;
> > +               }
> > +
> > +               /* If we found anything in the first symbol section,
> > +                * do not search others to avoid duplicates.
> > +                */
> > +               if (func_offs_cnt)
> > +                       break;
> > +       }
> > +
> > +       offsets = calloc(func_offs_cnt, sizeof(*offsets));
> > +       names = calloc(func_offs_cnt, sizeof(*names));
> > +       if (!offsets || !names) {
> > +               free(offsets);
> > +               free(names);
> > +               err = -ENOMEM;
> > +               goto out;
> > +       }
> > +
> > +       for (i = 0; i < func_offs_cnt; i++) {
> > +               offsets[i] = func_offs[i].offset;
> > +               names[i] = func_offs[i].name;
> 
> see above, why not fill these out right away during elf symbols iteration?

ok

> 
> > +       }
> > +
> > +       *pnames = names;
> > +       *poffsets = offsets;
> > +       *pcnt = func_offs_cnt;
> > +out:
> > +       free(func_offs);
> > +       return err;
> > +}
> > +
> > +int elf_find_pattern_func_offset(const char *binary_path, const char *pattern,
> > +                                const char ***pnames, unsigned long **poffsets,
> > +                                size_t *pcnt)
> > +{
> > +       struct elf_fd elf_fd = {};
> > +       long ret = -ENOENT;
> > +
> > +       ret = open_elf(binary_path, &elf_fd);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = __elf_find_pattern_func_offset(elf_fd.elf, binary_path, pattern, pnames, poffsets, pcnt);
> 
> I don't really like these underscored functions,
> elf_find_pattern_func_offset() already has to do goto out for clean
> ups, this close_elf() thing is just another resource to clean up
> there, I don't see much reason to separate open_elf/close_elf in this
> case

I think that happened because I originally did the 'struct elf_fd' cleanup
as last patch and only then moved it up the git log, so I did not realize
elf_find_pattern_func_offset could be single function, will change

thanks,
jirka

> 
> 
> > +       close_elf(&elf_fd);
> > +       return ret;
> > +}
> > +
> >  /* Find offset of function name in ELF object specified by path. "name" matches
> >   * symbol name or name@@LIB for library functions.
> >   */
> > diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> > index 13d5c12fbd0b..22b0834e7fe1 100644
> > --- a/tools/lib/bpf/libbpf_internal.h
> > +++ b/tools/lib/bpf/libbpf_internal.h
> > @@ -579,4 +579,7 @@ int sys_bpf_prog_load(union bpf_attr *attr, unsigned int size, int attempts);
> >
> >  int elf_find_multi_func_offset(const char *binary_path, int cnt,
> >                                const char **syms, unsigned long **poffsets);
> > +int elf_find_pattern_func_offset(const char *binary_path, const char *pattern,
> > +                                const char ***pnames, unsigned long **poffsets,
> > +                                size_t *pcnt);
> >  #endif /* __LIBBPF_LIBBPF_INTERNAL_H */
> > --
> > 2.41.0
> >