Re: [PATCH 2/2] btf_encoder: Change functions check due to broken dwarf

[Jiri Olsa <jolsa@redhat.com>]

On Tue, Nov 03, 2020 at 10:55:58AM -0800, Andrii Nakryiko wrote:

SNIP

> > I can still see several differences to ftrace functions in
> > /sys/kernel/debug/tracing/available_filter_functions file:
> >
> >   - available_filter_functions includes modules (7086 functions)
> >   - available_filter_functions includes functions like:
> >       __acpi_match_device.part.0.constprop.0
> >       acpi_ns_check_sorted_list.constprop.0
> >       acpi_os_unmap_generic_address.part.0
> >       acpiphp_check_bridge.part.0
> >
> >     which are not part of dwarf data (1164 functions)
> >   - BTF includes multiple functions like:
> >       __clk_register_clkdev
> >       clk_register_clkdev
> >
> >     which share same code so they appear just as single function
> >     in available_filter_functions, but dwarf keeps track of both
> >     of them (16 functions)
> >
> > With this change I'm getting 38334 BTF functions, which
> > when added above functions to consideration gives same
> > amount of functions in available_filter_functions.
> >
> > The patch still keeps the original function filter condition
> > (that uses current fn->declaration check) in case the object
> > does not contain *_mcount_loc symbol -> object is not vmlinux.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  btf_encoder.c | 222 +++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 220 insertions(+), 2 deletions(-)
> 
> [...]
> 
> > +static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
> > +{
> > +       if (elf_sym__type(sym) != STT_FUNC)
> > +               return 0;
> > +       if (!elf_sym__value(sym))
> > +               return 0;
> > +
> > +       if (functions_cnt == functions_alloc) {
> > +               functions_alloc = max(1000, functions_alloc * 3 / 2);
> > +               functions = realloc(functions, functions_alloc * sizeof(*functions));
> > +               if (!functions)
> > +                       return -1;

ok, I thought that if we go down I don't need to,
but I did not check how pahole is handling this,
probably free everything

> 
> memory leak right here. You need to use a temporary variable and check
> if for NULL, before overwriting functions.
> 
> > +       }
> > +
> > +       functions[functions_cnt].name = elf_sym__name(sym, btfe->symtab);
> > +       functions[functions_cnt].addr = elf_sym__value(sym);
> > +       functions[functions_cnt].generated = false;
> > +       functions[functions_cnt].valid = false;
> > +       functions_cnt++;
> > +       return 0;
> > +}
> > +
> > +static int addrs_cmp(const void *_a, const void *_b)
> > +{
> > +       const unsigned long *a = _a;
> > +       const unsigned long *b = _b;
> > +
> > +       return *a - *b;
> 
> this is cute, but is it always correct? instead of thinking how this
> works with overflows, maybe let's keep it simple with
> 
> if (*a == *b)
>   return 0;
> return *a < *b ? -1 : 1;

sure, will fix

> 
> ?
> 
> > +}
> > +
> > +static int filter_functions(struct btf_elf *btfe, struct mcount_symbols *ms)
> > +{
> > +       bool init_filter = ms->init_begin && ms->init_end;
> > +       unsigned long *addrs, count, offset, i;
> > +       Elf_Data *data;
> > +       GElf_Shdr shdr;
> > +       Elf_Scn *sec;
> > +
> > +       /*
> > +        * Find mcount addressed marked by __start_mcount_loc
> > +        * and __stop_mcount_loc symbols and load them into
> > +        * sorted array.
> > +        */
> > +       sec = elf_getscn(btfe->elf, ms->start_section);
> > +       if (!sec || !gelf_getshdr(sec, &shdr)) {
> > +               fprintf(stderr, "Failed to get section(%lu) header.\n",
> > +                       ms->start_section);
> > +               return -1;
> > +       }
> > +
> > +       offset = ms->start - shdr.sh_addr;
> > +       count  = (ms->stop - ms->start) / 8;
> > +
> > +       data = elf_getdata(sec, 0);
> > +       if (!data) {
> > +               fprintf(stderr, "Failed to section(%lu) data.\n",
> 
> typo: failed to get?

yep

> 
> > +                       ms->start_section);
> > +               return -1;
> > +       }
> > +
> > +       addrs = malloc(count * sizeof(addrs[0]));
> > +       if (!addrs) {
> > +               fprintf(stderr, "Failed to allocate memory for ftrace addresses.\n");
> > +               return -1;
> > +       }
> > +
> 
> [...]
> 
> >
> > +#define SET_SYMBOL(__sym, __var)                                               \
> > +       if (!ms->__var && !strcmp(__sym, elf_sym__name(sym, btfe->symtab)))     \
> > +               ms->__var = sym->st_value;                                      \
> > +
> > +static void collect_mcount_symbol(GElf_Sym *sym, struct mcount_symbols *ms)
> > +{
> > +       if (!ms->start &&
> > +           !strcmp("__start_mcount_loc", elf_sym__name(sym, btfe->symtab))) {
> > +               ms->start = sym->st_value;
> > +               ms->start_section = sym->st_shndx;
> > +       }
> > +       SET_SYMBOL("__stop_mcount_loc", stop)
> > +       SET_SYMBOL("__init_begin", init_begin)
> > +       SET_SYMBOL("__init_end", init_end)
> 
> please don't use macro here, it doesn't save much code but complicates
> reading it quite significantly

ok

> 
> > +}
> > +
> > +#undef SET_SYMBOL
> > +
> >  static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
> >  {
> > +       struct mcount_symbols ms = { };
> >         uint32_t core_id;
> >         GElf_Sym sym;
> >
> > @@ -320,6 +485,9 @@ static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
> >         elf_symtab__for_each_symbol(btfe->symtab, core_id, sym) {
> >                 if (collect_percpu_vars && collect_percpu_var(btfe, &sym))
> >                         return -1;
> > +               if (collect_function(btfe, &sym))
> > +                       return -1;
> > +               collect_mcount_symbol(&sym, &ms);
> >         }
> >
> >         if (collect_percpu_vars) {
> > @@ -329,9 +497,34 @@ static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
> >                 if (btf_elf__verbose)
> >                         printf("Found %d per-CPU variables!\n", percpu_var_cnt);
> >         }
> > +
> > +       if (functions_cnt) {
> > +               qsort(functions, functions_cnt, sizeof(functions[0]), functions_cmp);
> > +               if (ms.start && ms.stop &&
> > +                   filter_functions(btfe, &ms)) {
> 
> nit: single line should fit well, no?

ook

thanks,
jirka