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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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
> >
