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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, Oct 31, 2020 at 3:32 PM Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
>
> We need to generate just single BTF instance for the
> function, while DWARF data contains multiple instances
> of DW_TAG_subprogram tag.
>
> Unfortunately we can no longer rely on DW_AT_declaration
> tag (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97060)
>
> Instead we apply following checks:
>   - argument names are defined for the function
>   - there's symbol and address defined for the function
>   - function is generated only once
>
> Also because we want to follow kernel's ftrace traceable
> functions, this patchset is adding extra check that the
> function is one of the ftrace's functions.
>
> All ftrace functions addresses are stored in vmlinux
> binary within symbols:
>   __start_mcount_loc
>   __stop_mcount_loc
>
> During object preparation code we read those addresses,
> sort them and use them as filter for all detected dwarf
> functions.
>
> We also filter out functions within .init section, ftrace
> is doing that in runtime.
>
> 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@xxxxxxxxxx>
> ---
>  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;

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;

?

> +}
> +
> +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?

> +                       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

> +}
> +
> +#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?

> +                       fprintf(stderr, "Failed to filter dwarf functions\n");
> +                       return -1;
> +               }
> +               if (btf_elf__verbose)
> +                       printf("Found %d functions!\n", functions_valid);
> +       }
> +
>         return 0;
>  }
>

[...]



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux