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