Re: [PATCH 2/2] btf_encoder: Detect kernel module ftrace addresses

[Jiri Olsa <jolsa@redhat.com>]

On Thu, Nov 26, 2020 at 08:18:58PM -0800, Andrii Nakryiko wrote:
> On Tue, Nov 24, 2020 at 8:22 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Add support to detect kernel module dtrace addresses and use
> > it as filter for functions.
> 
> typo: dtrace -> ftrace?

heh, honest typo I swear ;-)

SNIP

> >
> >         if (functions_cnt == functions_alloc) {
> >                 functions_alloc = max(1000, functions_alloc * 3 / 2);
> > @@ -84,8 +85,17 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
> >                 functions = new;
> >         }
> >
> > +       if (elf_sym__section(sym) != last_idx) {
> > +               int idx = elf_sym__section(sym);
> 
> nit: elf_sym__section() called twice, maybe have sec_idx local variable instead?

ugh, right, will fix

> 
> > +
> > +               if (!elf_section_by_idx(btfe->elf, &sh, idx))
> > +                       return 0;
> 
> isn't this an error and shouldn't return 0?..

I'm skiping functions without section, I did not want to fail
all this because of some misplaced symbol

mabe we could warn, but still go on..

> 
> > +               last_idx = idx;
> > +       }
> > +
> >         functions[functions_cnt].name = elf_sym__name(sym, btfe->symtab);
> >         functions[functions_cnt].addr = elf_sym__value(sym);
> > +       functions[functions_cnt].sh_addr = sh.sh_addr;
> >         functions[functions_cnt].generated = false;
> >         functions_cnt++;
> >         return 0;
> > @@ -146,10 +156,60 @@ static int get_vmlinux_addrs(struct btf_elf *btfe, struct funcs_layout *fl,
> >         return 0;
> >  }
> >
> > +static int
> > +get_kmod_addrs(struct btf_elf *btfe, unsigned long **paddrs, unsigned long *pcount)
> > +{
> > +       unsigned long *addrs, count;
> > +       GElf_Shdr shdr_mcount;
> > +       Elf_Data *data;
> > +       Elf_Scn *sec;
> > +
> > +       /* get __mcount_loc */
> > +       sec = elf_section_by_name(btfe->elf, &btfe->ehdr, &shdr_mcount,
> > +                                 "__mcount_loc", NULL);
> > +       if (!sec) {
> > +               if (btf_elf__verbose) {
> > +                       printf("%s: '%s' doesn't have __mcount_loc section\n", __func__,
> > +                              btfe->filename);
> > +               }
> 
> 
> nit: unnecessary {} for single-statement if

ah ok, I put it because kernel guys scream with multiline
conditions without {}

> 
> > +               return 0;
> > +       }
> > +
> > +       data = elf_getdata(sec, NULL);
> > +       if (!data) {
> > +               fprintf(stderr, "Failed to data for __mcount_loc section.\n");
> > +               return -1;
> > +       }
> > +
> > +       addrs = malloc(data->d_size);
> > +       if (!addrs) {
> > +               fprintf(stderr, "Failed to allocate memory for ftrace addresses.\n");
> > +               return -1;
> > +       }
> > +
> > +       count = data->d_size / sizeof(unsigned long);
> 
> not sure this is safe to do, e.g., if we are processing ELF of
> different bitness (32 vs 64). Maybev we can get the size of an entry
> from sh_entsize?

it's not filled in, zero in my build

we could get bitness from elf header and use that in here
and also for vmlinux where it's hardcoded as well

SNIP

> > @@ -174,9 +244,18 @@ static int setup_functions(struct btf_elf *btfe, struct funcs_layout *fl)
> >          */
> >         for (i = 0; i < functions_cnt; i++) {
> >                 struct elf_function *func = &functions[i];
> > +               /*
> > +                * For vmlinux image both addrs[x] and functions[x]::addr
> > +                * values are final address and are comparable.
> > +                *
> > +                * For kernel module addrs[x] is final address, but
> > +                * functions[x]::addr is relative address within section
> > +                * and needs to be relocated by adding sh_addr.
> > +                */
> > +               unsigned long addr = kmod ? func->addr + func->sh_addr : func->addr;
> >
> >                 /* Make sure function is within ftrace addresses. */
> > -               if (bsearch(&func->addr, addrs, count, sizeof(addrs[0]), addrs_cmp)) {
> > +               if (bsearch(&addr, addrs, count, sizeof(addrs[0]), addrs_cmp)) {
> >                         /*
> >                          * We iterate over sorted array, so we can easily skip
> >                          * not valid item and move following valid field into
> > diff --git a/dutil.c b/dutil.c
> > index f7b853f0660d..5ebbd2f9c84c 100644
> > --- a/dutil.c
> > +++ b/dutil.c
> > @@ -196,6 +196,22 @@ Elf_Scn *elf_section_by_name(Elf *elf, GElf_Ehdr *ep,
> >         return sec;
> >  }
> >
> > +Elf_Scn *elf_section_by_idx(Elf *elf, GElf_Shdr *shp, int idx)
> 
> 
> there is elf_getscn(), which does the same?

great, did not see it

thanks,
jirka