* [PATCHv2 0/2] pahole: Workaround dwarf bug for function encoding @ 2020-10-31 22:31 Jiri Olsa 2020-10-31 22:31 ` [PATCH 1/2] btf_encoder: Move find_all_percpu_vars in generic collect_symbols Jiri Olsa 2020-10-31 22:31 ` [PATCH 2/2] btf_encoder: Change functions check due to broken dwarf Jiri Olsa 0 siblings, 2 replies; 15+ messages in thread From: Jiri Olsa @ 2020-10-31 22:31 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: dwarves, bpf, Alexei Starovoitov, Andrii Nakryiko, Yonghong Song, Hao Luo, Frank Ch. Eigler, Mark Wielaard hi, because of gcc bug [1] we can no longer rely on DW_AT_declaration attribute to filter out declarations and end up with just one copy of the function in the BTF data. It seems this bug is not easy to fix, but regardless if the it's coming soon, it's probably good idea not to depend so much only on dwarf data and make some extra checks. Thus for function encoding we are now doing following checks: - argument names are defined for the function - there's symbol and address defined for the function - function address belongs to ftrace locations (new in v2) - function is generated only once v2 changes: - add check ensuring functions belong to ftrace's mcount locations, this way we ensure to have in BTF only functions available for ftrace - patch 2 changelog describes all details - use collect* function names [Andrii] - use conventional size increase in realloc [Andrii] - drop elf_sym__is_function check - drop patch 3, it's not needed, because we follow ftrace locations With v2 patchset I'm getting 38334 BTF functions, which is reasonable subset of ftrace's available_filter_functions. thanks, jirka [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97060 --- Jiri Olsa (2): btf_encoder: Move find_all_percpu_vars in generic collect_symbols btf_encoder: Change functions check due to broken dwarf btf_encoder.c | 338 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------- 1 file changed, 283 insertions(+), 55 deletions(-) ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/2] btf_encoder: Move find_all_percpu_vars in generic collect_symbols 2020-10-31 22:31 [PATCHv2 0/2] pahole: Workaround dwarf bug for function encoding Jiri Olsa @ 2020-10-31 22:31 ` Jiri Olsa 2020-11-02 18:29 ` Hao Luo 2020-10-31 22:31 ` [PATCH 2/2] btf_encoder: Change functions check due to broken dwarf Jiri Olsa 1 sibling, 1 reply; 15+ messages in thread From: Jiri Olsa @ 2020-10-31 22:31 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: dwarves, bpf, Alexei Starovoitov, Andrii Nakryiko, Yonghong Song, Hao Luo, Frank Ch. Eigler, Mark Wielaard Moving find_all_percpu_vars under generic collect_symbols function that walks over symbols and calls collect_percpu_var. We will add another collect function that needs to go through all the symbols, so it's better we go through them just once. There's no functional change intended. Signed-off-by: Jiri Olsa <jolsa@kernel.org> --- btf_encoder.c | 124 +++++++++++++++++++++++++++----------------------- 1 file changed, 67 insertions(+), 57 deletions(-) diff --git a/btf_encoder.c b/btf_encoder.c index 4c92908beab2..1866bb16a8ba 100644 --- a/btf_encoder.c +++ b/btf_encoder.c @@ -250,75 +250,85 @@ static bool percpu_var_exists(uint64_t addr, uint32_t *sz, const char **name) return true; } -static int find_all_percpu_vars(struct btf_elf *btfe) +static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym) { - uint32_t core_id; - GElf_Sym sym; + const char *sym_name; + uint64_t addr; + uint32_t size; - /* cache variables' addresses, preparing for searching in symtab. */ - percpu_var_cnt = 0; + /* compare a symbol's shndx to determine if it's a percpu variable */ + if (elf_sym__section(sym) != btfe->percpu_shndx) + return 0; + if (elf_sym__type(sym) != STT_OBJECT) + return 0; - /* search within symtab for percpu variables */ - elf_symtab__for_each_symbol(btfe->symtab, core_id, sym) { - const char *sym_name; - uint64_t addr; - uint32_t size; + addr = elf_sym__value(sym); + /* + * Store only those symbols that have allocated space in the percpu section. + * This excludes the following three types of symbols: + * + * 1. __ADDRESSABLE(sym), which are forcely emitted as symbols. + * 2. __UNIQUE_ID(prefix), which are introduced to generate unique ids. + * 3. __exitcall(fn), functions which are labeled as exit calls. + * + * In addition, the variables defined using DEFINE_PERCPU_FIRST are + * also not included, which currently includes: + * + * 1. fixed_percpu_data + */ + if (!addr) + return 0; - /* compare a symbol's shndx to determine if it's a percpu variable */ - if (elf_sym__section(&sym) != btfe->percpu_shndx) - continue; - if (elf_sym__type(&sym) != STT_OBJECT) - continue; + size = elf_sym__size(sym); + if (!size) + return 0; /* ignore zero-sized symbols */ - addr = elf_sym__value(&sym); - /* - * Store only those symbols that have allocated space in the percpu section. - * This excludes the following three types of symbols: - * - * 1. __ADDRESSABLE(sym), which are forcely emitted as symbols. - * 2. __UNIQUE_ID(prefix), which are introduced to generate unique ids. - * 3. __exitcall(fn), functions which are labeled as exit calls. - * - * In addition, the variables defined using DEFINE_PERCPU_FIRST are - * also not included, which currently includes: - * - * 1. fixed_percpu_data - */ - if (!addr) - continue; + sym_name = elf_sym__name(sym, btfe->symtab); + if (!btf_name_valid(sym_name)) { + dump_invalid_symbol("Found symbol of invalid name when encoding btf", + sym_name, btf_elf__verbose, btf_elf__force); + if (btf_elf__force) + return 0; + return -1; + } - size = elf_sym__size(&sym); - if (!size) - continue; /* ignore zero-sized symbols */ + if (btf_elf__verbose) + printf("Found per-CPU symbol '%s' at address 0x%lx\n", sym_name, addr); - sym_name = elf_sym__name(&sym, btfe->symtab); - if (!btf_name_valid(sym_name)) { - dump_invalid_symbol("Found symbol of invalid name when encoding btf", - sym_name, btf_elf__verbose, btf_elf__force); - if (btf_elf__force) - continue; - return -1; - } + if (percpu_var_cnt == MAX_PERCPU_VAR_CNT) { + fprintf(stderr, "Reached the limit of per-CPU variables: %d\n", + MAX_PERCPU_VAR_CNT); + return -1; + } + percpu_vars[percpu_var_cnt].addr = addr; + percpu_vars[percpu_var_cnt].sz = size; + percpu_vars[percpu_var_cnt].name = sym_name; + percpu_var_cnt++; - if (btf_elf__verbose) - printf("Found per-CPU symbol '%s' at address 0x%lx\n", sym_name, addr); + return 0; +} + +static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars) +{ + uint32_t core_id; + GElf_Sym sym; - if (percpu_var_cnt == MAX_PERCPU_VAR_CNT) { - fprintf(stderr, "Reached the limit of per-CPU variables: %d\n", - MAX_PERCPU_VAR_CNT); + /* cache variables' addresses, preparing for searching in symtab. */ + percpu_var_cnt = 0; + + /* search within symtab for percpu variables */ + elf_symtab__for_each_symbol(btfe->symtab, core_id, sym) { + if (collect_percpu_vars && collect_percpu_var(btfe, &sym)) return -1; - } - percpu_vars[percpu_var_cnt].addr = addr; - percpu_vars[percpu_var_cnt].sz = size; - percpu_vars[percpu_var_cnt].name = sym_name; - percpu_var_cnt++; } - if (percpu_var_cnt) - qsort(percpu_vars, percpu_var_cnt, sizeof(percpu_vars[0]), percpu_var_cmp); + if (collect_percpu_vars) { + if (percpu_var_cnt) + qsort(percpu_vars, percpu_var_cnt, sizeof(percpu_vars[0]), percpu_var_cmp); - if (btf_elf__verbose) - printf("Found %d per-CPU variables!\n", percpu_var_cnt); + if (btf_elf__verbose) + printf("Found %d per-CPU variables!\n", percpu_var_cnt); + } return 0; } @@ -347,7 +357,7 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force, if (!btfe) return -1; - if (!skip_encoding_vars && find_all_percpu_vars(btfe)) + if (collect_symbols(btfe, !skip_encoding_vars)) goto out; has_index_type = false; -- 2.26.2 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] btf_encoder: Move find_all_percpu_vars in generic collect_symbols 2020-10-31 22:31 ` [PATCH 1/2] btf_encoder: Move find_all_percpu_vars in generic collect_symbols Jiri Olsa @ 2020-11-02 18:29 ` Hao Luo 2020-11-03 17:31 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 15+ messages in thread From: Hao Luo @ 2020-11-02 18:29 UTC (permalink / raw) To: Jiri Olsa Cc: Arnaldo Carvalho de Melo, dwarves, bpf, Alexei Starovoitov, Andrii Nakryiko, Yonghong Song, Frank Ch. Eigler, Mark Wielaard This looks good to me. Thanks, Jiri. Acked-by: Hao Luo <haoluo@google.com> On Sat, Oct 31, 2020 at 3:31 PM Jiri Olsa <jolsa@kernel.org> wrote: > > Moving find_all_percpu_vars under generic collect_symbols > function that walks over symbols and calls collect_percpu_var. > > We will add another collect function that needs to go through > all the symbols, so it's better we go through them just once. > > There's no functional change intended. > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > btf_encoder.c | 124 +++++++++++++++++++++++++++----------------------- > 1 file changed, 67 insertions(+), 57 deletions(-) > > diff --git a/btf_encoder.c b/btf_encoder.c > index 4c92908beab2..1866bb16a8ba 100644 > --- a/btf_encoder.c > +++ b/btf_encoder.c > @@ -250,75 +250,85 @@ static bool percpu_var_exists(uint64_t addr, uint32_t *sz, const char **name) > return true; > } > > -static int find_all_percpu_vars(struct btf_elf *btfe) > +static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym) > { > - uint32_t core_id; > - GElf_Sym sym; > + const char *sym_name; > + uint64_t addr; > + uint32_t size; > > - /* cache variables' addresses, preparing for searching in symtab. */ > - percpu_var_cnt = 0; > + /* compare a symbol's shndx to determine if it's a percpu variable */ > + if (elf_sym__section(sym) != btfe->percpu_shndx) > + return 0; > + if (elf_sym__type(sym) != STT_OBJECT) > + return 0; > > - /* search within symtab for percpu variables */ > - elf_symtab__for_each_symbol(btfe->symtab, core_id, sym) { > - const char *sym_name; > - uint64_t addr; > - uint32_t size; > + addr = elf_sym__value(sym); > + /* > + * Store only those symbols that have allocated space in the percpu section. > + * This excludes the following three types of symbols: > + * > + * 1. __ADDRESSABLE(sym), which are forcely emitted as symbols. > + * 2. __UNIQUE_ID(prefix), which are introduced to generate unique ids. > + * 3. __exitcall(fn), functions which are labeled as exit calls. > + * > + * In addition, the variables defined using DEFINE_PERCPU_FIRST are > + * also not included, which currently includes: > + * > + * 1. fixed_percpu_data > + */ > + if (!addr) > + return 0; > > - /* compare a symbol's shndx to determine if it's a percpu variable */ > - if (elf_sym__section(&sym) != btfe->percpu_shndx) > - continue; > - if (elf_sym__type(&sym) != STT_OBJECT) > - continue; > + size = elf_sym__size(sym); > + if (!size) > + return 0; /* ignore zero-sized symbols */ > > - addr = elf_sym__value(&sym); > - /* > - * Store only those symbols that have allocated space in the percpu section. > - * This excludes the following three types of symbols: > - * > - * 1. __ADDRESSABLE(sym), which are forcely emitted as symbols. > - * 2. __UNIQUE_ID(prefix), which are introduced to generate unique ids. > - * 3. __exitcall(fn), functions which are labeled as exit calls. > - * > - * In addition, the variables defined using DEFINE_PERCPU_FIRST are > - * also not included, which currently includes: > - * > - * 1. fixed_percpu_data > - */ > - if (!addr) > - continue; > + sym_name = elf_sym__name(sym, btfe->symtab); > + if (!btf_name_valid(sym_name)) { > + dump_invalid_symbol("Found symbol of invalid name when encoding btf", > + sym_name, btf_elf__verbose, btf_elf__force); > + if (btf_elf__force) > + return 0; > + return -1; > + } > > - size = elf_sym__size(&sym); > - if (!size) > - continue; /* ignore zero-sized symbols */ > + if (btf_elf__verbose) > + printf("Found per-CPU symbol '%s' at address 0x%lx\n", sym_name, addr); > > - sym_name = elf_sym__name(&sym, btfe->symtab); > - if (!btf_name_valid(sym_name)) { > - dump_invalid_symbol("Found symbol of invalid name when encoding btf", > - sym_name, btf_elf__verbose, btf_elf__force); > - if (btf_elf__force) > - continue; > - return -1; > - } > + if (percpu_var_cnt == MAX_PERCPU_VAR_CNT) { > + fprintf(stderr, "Reached the limit of per-CPU variables: %d\n", > + MAX_PERCPU_VAR_CNT); > + return -1; > + } > + percpu_vars[percpu_var_cnt].addr = addr; > + percpu_vars[percpu_var_cnt].sz = size; > + percpu_vars[percpu_var_cnt].name = sym_name; > + percpu_var_cnt++; > > - if (btf_elf__verbose) > - printf("Found per-CPU symbol '%s' at address 0x%lx\n", sym_name, addr); > + return 0; > +} > + > +static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars) > +{ > + uint32_t core_id; > + GElf_Sym sym; > > - if (percpu_var_cnt == MAX_PERCPU_VAR_CNT) { > - fprintf(stderr, "Reached the limit of per-CPU variables: %d\n", > - MAX_PERCPU_VAR_CNT); > + /* cache variables' addresses, preparing for searching in symtab. */ > + percpu_var_cnt = 0; > + > + /* search within symtab for percpu variables */ > + elf_symtab__for_each_symbol(btfe->symtab, core_id, sym) { > + if (collect_percpu_vars && collect_percpu_var(btfe, &sym)) > return -1; > - } > - percpu_vars[percpu_var_cnt].addr = addr; > - percpu_vars[percpu_var_cnt].sz = size; > - percpu_vars[percpu_var_cnt].name = sym_name; > - percpu_var_cnt++; > } > > - if (percpu_var_cnt) > - qsort(percpu_vars, percpu_var_cnt, sizeof(percpu_vars[0]), percpu_var_cmp); > + if (collect_percpu_vars) { > + if (percpu_var_cnt) > + qsort(percpu_vars, percpu_var_cnt, sizeof(percpu_vars[0]), percpu_var_cmp); > > - if (btf_elf__verbose) > - printf("Found %d per-CPU variables!\n", percpu_var_cnt); > + if (btf_elf__verbose) > + printf("Found %d per-CPU variables!\n", percpu_var_cnt); > + } > return 0; > } > > @@ -347,7 +357,7 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force, > if (!btfe) > return -1; > > - if (!skip_encoding_vars && find_all_percpu_vars(btfe)) > + if (collect_symbols(btfe, !skip_encoding_vars)) > goto out; > > has_index_type = false; > -- > 2.26.2 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] btf_encoder: Move find_all_percpu_vars in generic collect_symbols 2020-11-02 18:29 ` Hao Luo @ 2020-11-03 17:31 ` Arnaldo Carvalho de Melo 0 siblings, 0 replies; 15+ messages in thread From: Arnaldo Carvalho de Melo @ 2020-11-03 17:31 UTC (permalink / raw) To: Hao Luo Cc: Jiri Olsa, dwarves, bpf, Alexei Starovoitov, Andrii Nakryiko, Yonghong Song, Frank Ch. Eigler, Mark Wielaard Em Mon, Nov 02, 2020 at 10:29:22AM -0800, Hao Luo escreveu: > This looks good to me. Thanks, Jiri. > > Acked-by: Hao Luo <haoluo@google.com> Thanks, applied, will wait for a v2 for the other. > On Sat, Oct 31, 2020 at 3:31 PM Jiri Olsa <jolsa@kernel.org> wrote: > > > > Moving find_all_percpu_vars under generic collect_symbols > > function that walks over symbols and calls collect_percpu_var. > > > > We will add another collect function that needs to go through > > all the symbols, so it's better we go through them just once. > > > > There's no functional change intended. > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > --- > > btf_encoder.c | 124 +++++++++++++++++++++++++++----------------------- > > 1 file changed, 67 insertions(+), 57 deletions(-) > > > > diff --git a/btf_encoder.c b/btf_encoder.c > > index 4c92908beab2..1866bb16a8ba 100644 > > --- a/btf_encoder.c > > +++ b/btf_encoder.c > > @@ -250,75 +250,85 @@ static bool percpu_var_exists(uint64_t addr, uint32_t *sz, const char **name) > > return true; > > } > > > > -static int find_all_percpu_vars(struct btf_elf *btfe) > > +static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym) > > { > > - uint32_t core_id; > > - GElf_Sym sym; > > + const char *sym_name; > > + uint64_t addr; > > + uint32_t size; > > > > - /* cache variables' addresses, preparing for searching in symtab. */ > > - percpu_var_cnt = 0; > > + /* compare a symbol's shndx to determine if it's a percpu variable */ > > + if (elf_sym__section(sym) != btfe->percpu_shndx) > > + return 0; > > + if (elf_sym__type(sym) != STT_OBJECT) > > + return 0; > > > > - /* search within symtab for percpu variables */ > > - elf_symtab__for_each_symbol(btfe->symtab, core_id, sym) { > > - const char *sym_name; > > - uint64_t addr; > > - uint32_t size; > > + addr = elf_sym__value(sym); > > + /* > > + * Store only those symbols that have allocated space in the percpu section. > > + * This excludes the following three types of symbols: > > + * > > + * 1. __ADDRESSABLE(sym), which are forcely emitted as symbols. > > + * 2. __UNIQUE_ID(prefix), which are introduced to generate unique ids. > > + * 3. __exitcall(fn), functions which are labeled as exit calls. > > + * > > + * In addition, the variables defined using DEFINE_PERCPU_FIRST are > > + * also not included, which currently includes: > > + * > > + * 1. fixed_percpu_data > > + */ > > + if (!addr) > > + return 0; > > > > - /* compare a symbol's shndx to determine if it's a percpu variable */ > > - if (elf_sym__section(&sym) != btfe->percpu_shndx) > > - continue; > > - if (elf_sym__type(&sym) != STT_OBJECT) > > - continue; > > + size = elf_sym__size(sym); > > + if (!size) > > + return 0; /* ignore zero-sized symbols */ > > > > - addr = elf_sym__value(&sym); > > - /* > > - * Store only those symbols that have allocated space in the percpu section. > > - * This excludes the following three types of symbols: > > - * > > - * 1. __ADDRESSABLE(sym), which are forcely emitted as symbols. > > - * 2. __UNIQUE_ID(prefix), which are introduced to generate unique ids. > > - * 3. __exitcall(fn), functions which are labeled as exit calls. > > - * > > - * In addition, the variables defined using DEFINE_PERCPU_FIRST are > > - * also not included, which currently includes: > > - * > > - * 1. fixed_percpu_data > > - */ > > - if (!addr) > > - continue; > > + sym_name = elf_sym__name(sym, btfe->symtab); > > + if (!btf_name_valid(sym_name)) { > > + dump_invalid_symbol("Found symbol of invalid name when encoding btf", > > + sym_name, btf_elf__verbose, btf_elf__force); > > + if (btf_elf__force) > > + return 0; > > + return -1; > > + } > > > > - size = elf_sym__size(&sym); > > - if (!size) > > - continue; /* ignore zero-sized symbols */ > > + if (btf_elf__verbose) > > + printf("Found per-CPU symbol '%s' at address 0x%lx\n", sym_name, addr); > > > > - sym_name = elf_sym__name(&sym, btfe->symtab); > > - if (!btf_name_valid(sym_name)) { > > - dump_invalid_symbol("Found symbol of invalid name when encoding btf", > > - sym_name, btf_elf__verbose, btf_elf__force); > > - if (btf_elf__force) > > - continue; > > - return -1; > > - } > > + if (percpu_var_cnt == MAX_PERCPU_VAR_CNT) { > > + fprintf(stderr, "Reached the limit of per-CPU variables: %d\n", > > + MAX_PERCPU_VAR_CNT); > > + return -1; > > + } > > + percpu_vars[percpu_var_cnt].addr = addr; > > + percpu_vars[percpu_var_cnt].sz = size; > > + percpu_vars[percpu_var_cnt].name = sym_name; > > + percpu_var_cnt++; > > > > - if (btf_elf__verbose) > > - printf("Found per-CPU symbol '%s' at address 0x%lx\n", sym_name, addr); > > + return 0; > > +} > > + > > +static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars) > > +{ > > + uint32_t core_id; > > + GElf_Sym sym; > > > > - if (percpu_var_cnt == MAX_PERCPU_VAR_CNT) { > > - fprintf(stderr, "Reached the limit of per-CPU variables: %d\n", > > - MAX_PERCPU_VAR_CNT); > > + /* cache variables' addresses, preparing for searching in symtab. */ > > + percpu_var_cnt = 0; > > + > > + /* search within symtab for percpu variables */ > > + elf_symtab__for_each_symbol(btfe->symtab, core_id, sym) { > > + if (collect_percpu_vars && collect_percpu_var(btfe, &sym)) > > return -1; > > - } > > - percpu_vars[percpu_var_cnt].addr = addr; > > - percpu_vars[percpu_var_cnt].sz = size; > > - percpu_vars[percpu_var_cnt].name = sym_name; > > - percpu_var_cnt++; > > } > > > > - if (percpu_var_cnt) > > - qsort(percpu_vars, percpu_var_cnt, sizeof(percpu_vars[0]), percpu_var_cmp); > > + if (collect_percpu_vars) { > > + if (percpu_var_cnt) > > + qsort(percpu_vars, percpu_var_cnt, sizeof(percpu_vars[0]), percpu_var_cmp); > > > > - if (btf_elf__verbose) > > - printf("Found %d per-CPU variables!\n", percpu_var_cnt); > > + if (btf_elf__verbose) > > + printf("Found %d per-CPU variables!\n", percpu_var_cnt); > > + } > > return 0; > > } > > > > @@ -347,7 +357,7 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force, > > if (!btfe) > > return -1; > > > > - if (!skip_encoding_vars && find_all_percpu_vars(btfe)) > > + if (collect_symbols(btfe, !skip_encoding_vars)) > > goto out; > > > > has_index_type = false; > > -- > > 2.26.2 > > -- - Arnaldo ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/2] btf_encoder: Change functions check due to broken dwarf 2020-10-31 22:31 [PATCHv2 0/2] pahole: Workaround dwarf bug for function encoding Jiri Olsa 2020-10-31 22:31 ` [PATCH 1/2] btf_encoder: Move find_all_percpu_vars in generic collect_symbols Jiri Olsa @ 2020-10-31 22:31 ` Jiri Olsa 2020-11-02 21:59 ` Jiri Olsa 2020-11-03 18:55 ` Andrii Nakryiko 1 sibling, 2 replies; 15+ messages in thread From: Jiri Olsa @ 2020-10-31 22:31 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: dwarves, bpf, Alexei Starovoitov, Andrii Nakryiko, Yonghong Song, Hao Luo, Frank Ch. Eigler, Mark Wielaard 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@kernel.org> --- btf_encoder.c | 222 +++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 220 insertions(+), 2 deletions(-) diff --git a/btf_encoder.c b/btf_encoder.c index 1866bb16a8ba..0a378aa92142 100644 --- a/btf_encoder.c +++ b/btf_encoder.c @@ -26,6 +26,151 @@ */ #define KSYM_NAME_LEN 128 +struct mcount_symbols { + unsigned long start; + unsigned long stop; + unsigned long init_begin; + unsigned long init_end; + unsigned long start_section; +}; + +struct elf_function { + const char *name; + unsigned long addr; + bool generated; + bool valid; +}; + +static struct elf_function *functions; +static int functions_alloc; +static int functions_cnt; +static int functions_valid; + +static int functions_cmp(const void *_a, const void *_b) +{ + const struct elf_function *a = _a; + const struct elf_function *b = _b; + + return strcmp(a->name, b->name); +} + +static void delete_functions(void) +{ + free(functions); + functions_alloc = functions_cnt = functions_valid = 0; +} + +#ifndef max +# define max(x, y) ((x) < (y) ? (y) : (x)) +#endif + +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; + } + + 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; +} + +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", + 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; + } + + memcpy(addrs, data->d_buf + offset, count * sizeof(addrs[0])); + qsort(addrs, count, sizeof(addrs[0]), addrs_cmp); + + /* + * Let's got through all collected functions and filter + * out those that are not in ftrace and init code. + */ + for (i = 0; i < functions_cnt; i++) { + struct elf_function *func = &functions[i]; + + /* Do not enable .init section functions. */ + if (init_filter && + func->addr >= ms->init_begin && + func->addr < ms->init_end) + continue; + + /* Make sure function is within mcount addresses. */ + if (bsearch(&func->addr, addrs, count, sizeof(addrs[0]), addrs_cmp)) { + functions_valid++; + func->valid = true; + } + } + + free(addrs); + return 0; +} + +static bool should_generate_function(const struct btf_elf *btfe, const char *name) +{ + struct elf_function *p; + struct elf_function key = { .name = name }; + + p = bsearch(&key, functions, functions_cnt, + sizeof(functions[0]), functions_cmp); + if (!p || !p->valid || p->generated) + return false; + + p->generated = true; + return true; +} + static bool btf_name_char_ok(char c, bool first) { if (c == '_' || c == '.') @@ -207,6 +352,7 @@ int btf_encoder__encode() btf_elf__add_datasec_type(btfe, PERCPU_SECTION, &btfe->percpu_secinfo); err = btf_elf__encode(btfe, 0); + delete_functions(); btf_elf__delete(btfe); btfe = NULL; @@ -308,8 +454,27 @@ static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym) return 0; } +#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) +} + +#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)) { + fprintf(stderr, "Failed to filter dwarf functions\n"); + return -1; + } + if (btf_elf__verbose) + printf("Found %d functions!\n", functions_valid); + } + return 0; } +static bool has_arg_names(struct cu *cu, struct ftype *ftype) +{ + struct parameter *param; + const char *name; + + ftype__for_each_parameter(ftype, param) { + name = dwarves__active_loader->strings__ptr(cu, param->name); + if (name == NULL) + return false; + } + return true; +} + int cu__encode_btf(struct cu *cu, int verbose, bool force, bool skip_encoding_vars) { @@ -407,8 +600,32 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force, int btf_fnproto_id, btf_fn_id; const char *name; - if (fn->declaration || !fn->external) - continue; + /* + * The functions_valid != 0 means we parsed all necessary + * kernel symbols and we are using mcount location filter + * for functions. If it's not available keep the current + * dwarf declaration check. + */ + if (functions_valid) { + /* + * We need to generate just single BTF instance for the + * function, while DWARF data contains multiple instances + * of DW_TAG_subprogram tag. + * + * We check following conditions: + * - argument names are defined + * - there's symbol and address defined for the function + * - function address belongs to ftrace locations + * - function is generated only once + */ + if (!has_arg_names(cu, &fn->proto)) + continue; + if (!should_generate_function(btfe, function__name(fn, cu))) + continue; + } else { + if (fn->declaration || !fn->external) + continue; + } btf_fnproto_id = btf_elf__add_func_proto(btfe, cu, &fn->proto, type_id_off); name = dwarves__active_loader->strings__ptr(cu, fn->name); @@ -492,6 +709,7 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force, out: if (err) { + delete_functions(); btf_elf__delete(btfe); btfe = NULL; } -- 2.26.2 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] btf_encoder: Change functions check due to broken dwarf 2020-10-31 22:31 ` [PATCH 2/2] btf_encoder: Change functions check due to broken dwarf Jiri Olsa @ 2020-11-02 21:59 ` Jiri Olsa 2020-11-02 22:56 ` Jiri Olsa 2020-11-03 18:55 ` Andrii Nakryiko 1 sibling, 1 reply; 15+ messages in thread From: Jiri Olsa @ 2020-11-02 21:59 UTC (permalink / raw) To: Jiri Olsa Cc: Arnaldo Carvalho de Melo, dwarves, bpf, Alexei Starovoitov, Andrii Nakryiko, Yonghong Song, Hao Luo, Frank Ch. Eigler, Mark Wielaard On Sat, Oct 31, 2020 at 11:31:31PM +0100, Jiri Olsa 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 hum, for some reason this does not pass through bpf internal functions like bpf_iter_bpf_map.. I learned it hard way ;-) will check jirka ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] btf_encoder: Change functions check due to broken dwarf 2020-11-02 21:59 ` Jiri Olsa @ 2020-11-02 22:56 ` Jiri Olsa 2020-11-03 18:58 ` Andrii Nakryiko 0 siblings, 1 reply; 15+ messages in thread From: Jiri Olsa @ 2020-11-02 22:56 UTC (permalink / raw) To: Jiri Olsa Cc: Arnaldo Carvalho de Melo, dwarves, bpf, Alexei Starovoitov, Andrii Nakryiko, Yonghong Song, Hao Luo, Frank Ch. Eigler, Mark Wielaard On Mon, Nov 02, 2020 at 10:59:08PM +0100, Jiri Olsa wrote: > On Sat, Oct 31, 2020 at 11:31:31PM +0100, Jiri Olsa 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 > > hum, for some reason this does not pass through bpf internal > functions like bpf_iter_bpf_map.. I learned it hard way ;-) > will check so it gets filtered out because it's __init function I'll check if the fix below catches all internal functions, but I guess we should do something more robust jirka --- diff --git a/btf_encoder.c b/btf_encoder.c index 0a378aa92142..3cd94280c35b 100644 --- a/btf_encoder.c +++ b/btf_encoder.c @@ -143,7 +143,8 @@ static int filter_functions(struct btf_elf *btfe, struct mcount_symbols *ms) /* Do not enable .init section functions. */ if (init_filter && func->addr >= ms->init_begin && - func->addr < ms->init_end) + func->addr < ms->init_end && + strncmp("bpf_", func->name, 4)) continue; /* Make sure function is within mcount addresses. */ ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] btf_encoder: Change functions check due to broken dwarf 2020-11-02 22:56 ` Jiri Olsa @ 2020-11-03 18:58 ` Andrii Nakryiko 2020-11-03 19:05 ` Jiri Olsa 0 siblings, 1 reply; 15+ messages in thread From: Andrii Nakryiko @ 2020-11-03 18:58 UTC (permalink / raw) To: Jiri Olsa Cc: Jiri Olsa, Arnaldo Carvalho de Melo, dwarves, bpf, Alexei Starovoitov, Andrii Nakryiko, Yonghong Song, Hao Luo, Frank Ch. Eigler, Mark Wielaard On Mon, Nov 2, 2020 at 2:57 PM Jiri Olsa <jolsa@redhat.com> wrote: > > On Mon, Nov 02, 2020 at 10:59:08PM +0100, Jiri Olsa wrote: > > On Sat, Oct 31, 2020 at 11:31:31PM +0100, Jiri Olsa 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 > > > > hum, for some reason this does not pass through bpf internal > > functions like bpf_iter_bpf_map.. I learned it hard way ;-) what's the exact name of the function that was missing? bpf_iter_bpf_map doesn't exist. And if it's __init function, why does it matter, it's not going to be even available at runtime, right? > > will check > > so it gets filtered out because it's __init function > I'll check if the fix below catches all internal functions, > but I guess we should do something more robust > > jirka > > > --- > diff --git a/btf_encoder.c b/btf_encoder.c > index 0a378aa92142..3cd94280c35b 100644 > --- a/btf_encoder.c > +++ b/btf_encoder.c > @@ -143,7 +143,8 @@ static int filter_functions(struct btf_elf *btfe, struct mcount_symbols *ms) > /* Do not enable .init section functions. */ > if (init_filter && > func->addr >= ms->init_begin && > - func->addr < ms->init_end) > + func->addr < ms->init_end && > + strncmp("bpf_", func->name, 4)) this looks like a very wrong way to do this? Can you please elaborate on what's missing and why it shouldn't be missing? > continue; > > /* Make sure function is within mcount addresses. */ > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] btf_encoder: Change functions check due to broken dwarf 2020-11-03 18:58 ` Andrii Nakryiko @ 2020-11-03 19:05 ` Jiri Olsa 2020-11-03 19:23 ` Andrii Nakryiko 0 siblings, 1 reply; 15+ messages in thread From: Jiri Olsa @ 2020-11-03 19:05 UTC (permalink / raw) To: Andrii Nakryiko Cc: Jiri Olsa, Arnaldo Carvalho de Melo, dwarves, bpf, Alexei Starovoitov, Andrii Nakryiko, Yonghong Song, Hao Luo, Frank Ch. Eigler, Mark Wielaard On Tue, Nov 03, 2020 at 10:58:58AM -0800, Andrii Nakryiko wrote: > On Mon, Nov 2, 2020 at 2:57 PM Jiri Olsa <jolsa@redhat.com> wrote: > > > > On Mon, Nov 02, 2020 at 10:59:08PM +0100, Jiri Olsa wrote: > > > On Sat, Oct 31, 2020 at 11:31:31PM +0100, Jiri Olsa 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 > > > > > > hum, for some reason this does not pass through bpf internal > > > functions like bpf_iter_bpf_map.. I learned it hard way ;-) > > what's the exact name of the function that was missing? > bpf_iter_bpf_map doesn't exist. And if it's __init function, why does > it matter, it's not going to be even available at runtime, right? > bpf_map iter definition: DEFINE_BPF_ITER_FUNC(bpf_map, struct bpf_iter_meta *meta, struct bpf_map *map) goes to: #define DEFINE_BPF_ITER_FUNC(target, args...) \ extern int bpf_iter_ ## target(args); \ int __init bpf_iter_ ## target(args) { return 0; } that creates __init bpf_iter_bpf_map function that will make it into BTF where it's expected when opening iterator, but the code will be freed because it's __init function there are few iteratos functions like that, and I was going to check if there's more > > > > will check > > > > so it gets filtered out because it's __init function > > I'll check if the fix below catches all internal functions, > > but I guess we should do something more robust > > > > jirka > > > > > > --- > > diff --git a/btf_encoder.c b/btf_encoder.c > > index 0a378aa92142..3cd94280c35b 100644 > > --- a/btf_encoder.c > > +++ b/btf_encoder.c > > @@ -143,7 +143,8 @@ static int filter_functions(struct btf_elf *btfe, struct mcount_symbols *ms) > > /* Do not enable .init section functions. */ > > if (init_filter && > > func->addr >= ms->init_begin && > > - func->addr < ms->init_end) > > + func->addr < ms->init_end && > > + strncmp("bpf_", func->name, 4)) > > this looks like a very wrong way to do this? Can you please elaborate > on what's missing and why it shouldn't be missing? yes, it's just a hack, we should do something more robust as I mentioned above it just allowed me to use iterators finaly ;-) jirka ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] btf_encoder: Change functions check due to broken dwarf 2020-11-03 19:05 ` Jiri Olsa @ 2020-11-03 19:23 ` Andrii Nakryiko 2020-11-03 19:34 ` Jiri Olsa 2020-11-03 20:27 ` Yonghong Song 0 siblings, 2 replies; 15+ messages in thread From: Andrii Nakryiko @ 2020-11-03 19:23 UTC (permalink / raw) To: Jiri Olsa Cc: Jiri Olsa, Arnaldo Carvalho de Melo, dwarves, bpf, Alexei Starovoitov, Andrii Nakryiko, Yonghong Song, Hao Luo, Frank Ch. Eigler, Mark Wielaard On Tue, Nov 3, 2020 at 11:06 AM Jiri Olsa <jolsa@redhat.com> wrote: > > On Tue, Nov 03, 2020 at 10:58:58AM -0800, Andrii Nakryiko wrote: > > On Mon, Nov 2, 2020 at 2:57 PM Jiri Olsa <jolsa@redhat.com> wrote: > > > > > > On Mon, Nov 02, 2020 at 10:59:08PM +0100, Jiri Olsa wrote: > > > > On Sat, Oct 31, 2020 at 11:31:31PM +0100, Jiri Olsa 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 > > > > > > > > hum, for some reason this does not pass through bpf internal > > > > functions like bpf_iter_bpf_map.. I learned it hard way ;-) > > > > what's the exact name of the function that was missing? > > bpf_iter_bpf_map doesn't exist. And if it's __init function, why does > > it matter, it's not going to be even available at runtime, right? > > > > bpf_map iter definition: > > DEFINE_BPF_ITER_FUNC(bpf_map, struct bpf_iter_meta *meta, struct bpf_map *map) > > goes to: > > #define DEFINE_BPF_ITER_FUNC(target, args...) \ > extern int bpf_iter_ ## target(args); \ > int __init bpf_iter_ ## target(args) { return 0; } > > that creates __init bpf_iter_bpf_map function that will make > it into BTF where it's expected when opening iterator, but the > code will be freed because it's __init function hm... should we just drop __init there? Yonghong, is __init strictly necessary, or was just an optimization to save a tiny bit of space? > > there are few iteratos functions like that, and I was going to > check if there's more > > > > > > > will check > > > > > > so it gets filtered out because it's __init function > > > I'll check if the fix below catches all internal functions, > > > but I guess we should do something more robust > > > > > > jirka > > > > > > > > > --- > > > diff --git a/btf_encoder.c b/btf_encoder.c > > > index 0a378aa92142..3cd94280c35b 100644 > > > --- a/btf_encoder.c > > > +++ b/btf_encoder.c > > > @@ -143,7 +143,8 @@ static int filter_functions(struct btf_elf *btfe, struct mcount_symbols *ms) > > > /* Do not enable .init section functions. */ > > > if (init_filter && > > > func->addr >= ms->init_begin && > > > - func->addr < ms->init_end) > > > + func->addr < ms->init_end && > > > + strncmp("bpf_", func->name, 4)) > > > > this looks like a very wrong way to do this? Can you please elaborate > > on what's missing and why it shouldn't be missing? > > yes, it's just a hack, we should do something more > robust as I mentioned above > > it just allowed me to use iterators finaly ;-) sure, I get it, I was just trying to understand why there is such a problem in the first place. Turns out we need FUNCs not just for fentry/fexit and similar, but also for bpf_iter, which is an entirely different use case (similar to raw_tp, but raw_tp is using typedef -> func_proto approach). So I don't know, we might as well just not do mcount checks?.. As an alternative, but it's not great as well. > > jirka > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] btf_encoder: Change functions check due to broken dwarf 2020-11-03 19:23 ` Andrii Nakryiko @ 2020-11-03 19:34 ` Jiri Olsa 2020-11-03 20:27 ` Yonghong Song 1 sibling, 0 replies; 15+ messages in thread From: Jiri Olsa @ 2020-11-03 19:34 UTC (permalink / raw) To: Andrii Nakryiko Cc: Jiri Olsa, Arnaldo Carvalho de Melo, dwarves, bpf, Alexei Starovoitov, Andrii Nakryiko, Yonghong Song, Hao Luo, Frank Ch. Eigler, Mark Wielaard On Tue, Nov 03, 2020 at 11:23:33AM -0800, Andrii Nakryiko wrote: SNIP > > > on what's missing and why it shouldn't be missing? > > > > yes, it's just a hack, we should do something more > > robust as I mentioned above > > > > it just allowed me to use iterators finaly ;-) > > sure, I get it, I was just trying to understand why there is such a > problem in the first place. Turns out we need FUNCs not just for > fentry/fexit and similar, but also for bpf_iter, which is an entirely > different use case (similar to raw_tp, but raw_tp is using typedef -> > func_proto approach). > > So I don't know, we might as well just not do mcount checks?.. As an > alternative, but it's not great as well. how about moving all such functions to separate new .init.XXX section, and pahole would make one extr acheck for that.. and it still gets freed jirka ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] btf_encoder: Change functions check due to broken dwarf 2020-11-03 19:23 ` Andrii Nakryiko 2020-11-03 19:34 ` Jiri Olsa @ 2020-11-03 20:27 ` Yonghong Song 2020-11-03 23:18 ` Jiri Olsa 1 sibling, 1 reply; 15+ messages in thread From: Yonghong Song @ 2020-11-03 20:27 UTC (permalink / raw) To: Andrii Nakryiko, Jiri Olsa Cc: Jiri Olsa, Arnaldo Carvalho de Melo, dwarves, bpf, Alexei Starovoitov, Andrii Nakryiko, Hao Luo, Frank Ch. Eigler, Mark Wielaard On 11/3/20 11:23 AM, Andrii Nakryiko wrote: > On Tue, Nov 3, 2020 at 11:06 AM Jiri Olsa <jolsa@redhat.com> wrote: >> >> On Tue, Nov 03, 2020 at 10:58:58AM -0800, Andrii Nakryiko wrote: >>> On Mon, Nov 2, 2020 at 2:57 PM Jiri Olsa <jolsa@redhat.com> wrote: >>>> >>>> On Mon, Nov 02, 2020 at 10:59:08PM +0100, Jiri Olsa wrote: >>>>> On Sat, Oct 31, 2020 at 11:31:31PM +0100, Jiri Olsa 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 >>>>> >>>>> hum, for some reason this does not pass through bpf internal >>>>> functions like bpf_iter_bpf_map.. I learned it hard way ;-) >>> >>> what's the exact name of the function that was missing? >>> bpf_iter_bpf_map doesn't exist. And if it's __init function, why does >>> it matter, it's not going to be even available at runtime, right? >>> >> >> bpf_map iter definition: >> >> DEFINE_BPF_ITER_FUNC(bpf_map, struct bpf_iter_meta *meta, struct bpf_map *map) >> >> goes to: >> >> #define DEFINE_BPF_ITER_FUNC(target, args...) \ >> extern int bpf_iter_ ## target(args); \ >> int __init bpf_iter_ ## target(args) { return 0; } >> >> that creates __init bpf_iter_bpf_map function that will make >> it into BTF where it's expected when opening iterator, but the >> code will be freed because it's __init function > > hm... should we just drop __init there? > > Yonghong, is __init strictly necessary, or was just an optimization to > save a tiny bit of space? It is an optimization to save some space. We only need function signature, not function body, for bpf_iter. The macro definition is in include/linux/bpf.h. #define DEFINE_BPF_ITER_FUNC(target, args...) \ extern int bpf_iter_ ## target(args); \ int __init bpf_iter_ ## target(args) { return 0; } Maybe you could have a section, e.g., called .init.bpf.preserve_type which you can scan through to preserve the types. Alternatively you can drop the above __init, the saving is indeed tiny. But this adds overhead to ksymbol lookup and may not be desirable. > >> >> there are few iteratos functions like that, and I was going to >> check if there's more >> >>> >>>>> will check >>>> >>>> so it gets filtered out because it's __init function >>>> I'll check if the fix below catches all internal functions, >>>> but I guess we should do something more robust >>>> >>>> jirka >>>> >>>> >>>> --- >>>> diff --git a/btf_encoder.c b/btf_encoder.c >>>> index 0a378aa92142..3cd94280c35b 100644 >>>> --- a/btf_encoder.c >>>> +++ b/btf_encoder.c >>>> @@ -143,7 +143,8 @@ static int filter_functions(struct btf_elf *btfe, struct mcount_symbols *ms) >>>> /* Do not enable .init section functions. */ >>>> if (init_filter && >>>> func->addr >= ms->init_begin && >>>> - func->addr < ms->init_end) >>>> + func->addr < ms->init_end && >>>> + strncmp("bpf_", func->name, 4)) >>> >>> this looks like a very wrong way to do this? Can you please elaborate >>> on what's missing and why it shouldn't be missing? >> >> yes, it's just a hack, we should do something more >> robust as I mentioned above >> >> it just allowed me to use iterators finaly ;-) > > sure, I get it, I was just trying to understand why there is such a > problem in the first place. Turns out we need FUNCs not just for > fentry/fexit and similar, but also for bpf_iter, which is an entirely > different use case (similar to raw_tp, but raw_tp is using typedef -> > func_proto approach). > > So I don't know, we might as well just not do mcount checks?.. As an > alternative, but it's not great as well. > >> >> jirka >> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] btf_encoder: Change functions check due to broken dwarf 2020-11-03 20:27 ` Yonghong Song @ 2020-11-03 23:18 ` Jiri Olsa 0 siblings, 0 replies; 15+ messages in thread From: Jiri Olsa @ 2020-11-03 23:18 UTC (permalink / raw) To: Yonghong Song Cc: Andrii Nakryiko, Jiri Olsa, Arnaldo Carvalho de Melo, dwarves, bpf, Alexei Starovoitov, Andrii Nakryiko, Hao Luo, Frank Ch. Eigler, Mark Wielaard On Tue, Nov 03, 2020 at 12:27:56PM -0800, Yonghong Song wrote: SNIP > > > > > > > > > > bpf_map iter definition: > > > > > > DEFINE_BPF_ITER_FUNC(bpf_map, struct bpf_iter_meta *meta, struct bpf_map *map) > > > > > > goes to: > > > > > > #define DEFINE_BPF_ITER_FUNC(target, args...) \ > > > extern int bpf_iter_ ## target(args); \ > > > int __init bpf_iter_ ## target(args) { return 0; } > > > > > > that creates __init bpf_iter_bpf_map function that will make > > > it into BTF where it's expected when opening iterator, but the > > > code will be freed because it's __init function > > > > hm... should we just drop __init there? > > > > Yonghong, is __init strictly necessary, or was just an optimization to > > save a tiny bit of space? > > It is an optimization to save some space. We only need function > signature, not function body, for bpf_iter. > > The macro definition is in include/linux/bpf.h. > > #define DEFINE_BPF_ITER_FUNC(target, args...) \ > extern int bpf_iter_ ## target(args); \ > int __init bpf_iter_ ## target(args) { return 0; } > > Maybe you could have a section, e.g., called > .init.bpf.preserve_type > which you can scan through to preserve the types. right, sounds good, will send v3 with that thanks, jirka ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] btf_encoder: Change functions check due to broken dwarf 2020-10-31 22:31 ` [PATCH 2/2] btf_encoder: Change functions check due to broken dwarf Jiri Olsa 2020-11-02 21:59 ` Jiri Olsa @ 2020-11-03 18:55 ` Andrii Nakryiko 2020-11-03 23:22 ` Jiri Olsa 1 sibling, 1 reply; 15+ messages in thread From: Andrii Nakryiko @ 2020-11-03 18:55 UTC (permalink / raw) To: Jiri Olsa Cc: Arnaldo Carvalho de Melo, dwarves, bpf, Alexei Starovoitov, Andrii Nakryiko, Yonghong Song, Hao Luo, Frank Ch. Eigler, Mark Wielaard On Sat, Oct 31, 2020 at 3:32 PM Jiri Olsa <jolsa@kernel.org> 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@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; 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; > } > [...] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] btf_encoder: Change functions check due to broken dwarf 2020-11-03 18:55 ` Andrii Nakryiko @ 2020-11-03 23:22 ` Jiri Olsa 0 siblings, 0 replies; 15+ messages in thread From: Jiri Olsa @ 2020-11-03 23:22 UTC (permalink / raw) To: Andrii Nakryiko Cc: Jiri Olsa, Arnaldo Carvalho de Melo, dwarves, bpf, Alexei Starovoitov, Andrii Nakryiko, Yonghong Song, Hao Luo, Frank Ch. Eigler, Mark Wielaard 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2020-11-03 23:22 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-31 22:31 [PATCHv2 0/2] pahole: Workaround dwarf bug for function encoding Jiri Olsa 2020-10-31 22:31 ` [PATCH 1/2] btf_encoder: Move find_all_percpu_vars in generic collect_symbols Jiri Olsa 2020-11-02 18:29 ` Hao Luo 2020-11-03 17:31 ` Arnaldo Carvalho de Melo 2020-10-31 22:31 ` [PATCH 2/2] btf_encoder: Change functions check due to broken dwarf Jiri Olsa 2020-11-02 21:59 ` Jiri Olsa 2020-11-02 22:56 ` Jiri Olsa 2020-11-03 18:58 ` Andrii Nakryiko 2020-11-03 19:05 ` Jiri Olsa 2020-11-03 19:23 ` Andrii Nakryiko 2020-11-03 19:34 ` Jiri Olsa 2020-11-03 20:27 ` Yonghong Song 2020-11-03 23:18 ` Jiri Olsa 2020-11-03 18:55 ` Andrii Nakryiko 2020-11-03 23:22 ` Jiri Olsa
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).