| Message ID | 20210122163920.59177-3-jolsa@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | libbpf: Add support to use optional extended section index table | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
Em Fri, Jan 22, 2021 at 05:39:20PM +0100, Jiri Olsa escreveu: > For very large ELF objects (with many sections), we could > get special value SHN_XINDEX (65535) for symbol's st_shndx. > > This patch is adding code to detect the optional extended > section index table and use it to resolve symbol's section > index. > > Adding elf_symtab__for_each_symbol_index macro that returns > symbol's section index and usign it in collect functions. From a quick look it seems you addressed Andrii's review comments, right? I've merged it locally, but would like to have some detailed set of steps on how to test this, so that I can add it to a "Committer testing" section in the cset commit log and probably add it to my local set of regression tests. Who originally reported this? Joe? Also can someone provide a Tested-by: in addition to mine when I get this detailed set of steps to test? Thanks, - Arnaldo > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > btf_encoder.c | 59 +++++++++++++++++++++++++++++++++++++-------------- > elf_symtab.c | 39 +++++++++++++++++++++++++++++++++- > elf_symtab.h | 2 ++ > 3 files changed, 83 insertions(+), 17 deletions(-) > > diff --git a/btf_encoder.c b/btf_encoder.c > index 5557c9efd365..56ee55965093 100644 > --- a/btf_encoder.c > +++ b/btf_encoder.c > @@ -63,13 +63,13 @@ static void delete_functions(void) > #define max(x, y) ((x) < (y) ? (y) : (x)) > #endif > > -static int collect_function(struct btf_elf *btfe, GElf_Sym *sym) > +static int collect_function(struct btf_elf *btfe, GElf_Sym *sym, > + Elf32_Word sym_sec_idx) > { > struct elf_function *new; > static GElf_Shdr sh; > - static int last_idx; > + static Elf32_Word last_idx; > const char *name; > - int idx; > > if (elf_sym__type(sym) != STT_FUNC) > return 0; > @@ -90,12 +90,10 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym) > functions = new; > } > > - idx = elf_sym__section(sym); > - > - if (idx != last_idx) { > - if (!elf_section_by_idx(btfe->elf, &sh, idx)) > + if (sym_sec_idx != last_idx) { > + if (!elf_section_by_idx(btfe->elf, &sh, sym_sec_idx)) > return 0; > - last_idx = idx; > + last_idx = sym_sec_idx; > } > > functions[functions_cnt].name = name; > @@ -542,14 +540,15 @@ static bool percpu_var_exists(uint64_t addr, uint32_t *sz, const char **name) > return true; > } > > -static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym) > +static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym, > + Elf32_Word sym_sec_idx) > { > const char *sym_name; > uint64_t addr; > uint32_t size; > > /* compare a symbol's shndx to determine if it's a percpu variable */ > - if (elf_sym__section(sym) != btfe->percpu_shndx) > + if (sym_sec_idx != btfe->percpu_shndx) > return 0; > if (elf_sym__type(sym) != STT_OBJECT) > return 0; > @@ -585,12 +584,13 @@ static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym) > return 0; > } > > -static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl) > +static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl, > + Elf32_Word sym_sec_idx) > { > if (!fl->mcount_start && > !strcmp("__start_mcount_loc", elf_sym__name(sym, btfe->symtab))) { > fl->mcount_start = sym->st_value; > - fl->mcount_sec_idx = sym->st_shndx; > + fl->mcount_sec_idx = sym_sec_idx; > } > > if (!fl->mcount_stop && > @@ -598,9 +598,36 @@ static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl) > fl->mcount_stop = sym->st_value; > } > > +static bool elf_sym__get(Elf_Data *syms, Elf_Data *syms_sec_idx_table, > + int id, GElf_Sym *sym, Elf32_Word *sym_sec_idx) > +{ > + if (!gelf_getsym(syms, id, sym)) > + return false; > + > + *sym_sec_idx = sym->st_shndx; > + > + if (sym->st_shndx == SHN_XINDEX) { > + if (!syms_sec_idx_table) > + return false; > + if (!gelf_getsymshndx(syms, syms_sec_idx_table, > + id, sym, sym_sec_idx)) > + return false; > + } > + > + return true; > +} > + > +#define elf_symtab__for_each_symbol_index(symtab, id, sym, sym_sec_idx) \ > + for (id = 0; \ > + id < symtab->nr_syms && \ > + elf_sym__get(symtab->syms, symtab->syms_sec_idx_table, \ > + id, &sym, &sym_sec_idx); \ > + id++) > + > static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars) > { > struct funcs_layout fl = { }; > + Elf32_Word sym_sec_idx; > uint32_t core_id; > GElf_Sym sym; > > @@ -608,12 +635,12 @@ static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars) > 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)) > + elf_symtab__for_each_symbol_index(btfe->symtab, core_id, sym, sym_sec_idx) { > + if (collect_percpu_vars && collect_percpu_var(btfe, &sym, sym_sec_idx)) > return -1; > - if (collect_function(btfe, &sym)) > + if (collect_function(btfe, &sym, sym_sec_idx)) > return -1; > - collect_symbol(&sym, &fl); > + collect_symbol(&sym, &fl, sym_sec_idx); > } > > if (collect_percpu_vars) { > diff --git a/elf_symtab.c b/elf_symtab.c > index 741990ea3ed9..fad5e0c0ba3c 100644 > --- a/elf_symtab.c > +++ b/elf_symtab.c > @@ -17,11 +17,13 @@ > > struct elf_symtab *elf_symtab__new(const char *name, Elf *elf, GElf_Ehdr *ehdr) > { > + size_t symtab_index; > + > if (name == NULL) > name = ".symtab"; > > GElf_Shdr shdr; > - Elf_Scn *sec = elf_section_by_name(elf, ehdr, &shdr, name, NULL); > + Elf_Scn *sec = elf_section_by_name(elf, ehdr, &shdr, name, &symtab_index); > > if (sec == NULL) > return NULL; > @@ -41,6 +43,12 @@ struct elf_symtab *elf_symtab__new(const char *name, Elf *elf, GElf_Ehdr *ehdr) > if (symtab->syms == NULL) > goto out_free_name; > > + /* > + * This returns extended section index table's > + * section index, if it exists. > + */ > + int symtab_xindex = elf_scnshndx(sec); > + > sec = elf_getscn(elf, shdr.sh_link); > if (sec == NULL) > goto out_free_name; > @@ -49,6 +57,35 @@ struct elf_symtab *elf_symtab__new(const char *name, Elf *elf, GElf_Ehdr *ehdr) > if (symtab->symstrs == NULL) > goto out_free_name; > > + /* > + * The .symtab section has optional extended section index > + * table, load its data so it can be used to resolve symbol's > + * section index. > + **/ > + if (symtab_xindex > 0) { > + GElf_Shdr shdr_xindex; > + Elf_Scn *sec_xindex; > + > + sec_xindex = elf_getscn(elf, symtab_xindex); > + if (sec_xindex == NULL) > + goto out_free_name; > + > + if (gelf_getshdr(sec_xindex, &shdr_xindex) == NULL) > + goto out_free_name; > + > + /* Extra check to verify it's correct type */ > + if (shdr_xindex.sh_type != SHT_SYMTAB_SHNDX) > + goto out_free_name; > + > + /* Extra check to verify it belongs to the .symtab */ > + if (symtab_index != shdr_xindex.sh_link) > + goto out_free_name; > + > + symtab->syms_sec_idx_table = elf_getdata(elf_getscn(elf, symtab_xindex), NULL); > + if (symtab->syms_sec_idx_table == NULL) > + goto out_free_name; > + } > + > symtab->nr_syms = shdr.sh_size / shdr.sh_entsize; > > return symtab; > diff --git a/elf_symtab.h b/elf_symtab.h > index 359add69c8ab..2e05ca98158b 100644 > --- a/elf_symtab.h > +++ b/elf_symtab.h > @@ -16,6 +16,8 @@ struct elf_symtab { > uint32_t nr_syms; > Elf_Data *syms; > Elf_Data *symstrs; > + /* Data of SHT_SYMTAB_SHNDX section. */ > + Elf_Data *syms_sec_idx_table; > char *name; > }; > > -- > 2.26.2 >
On Fri, Jan 22, 2021 at 8:46 AM Jiri Olsa <jolsa@kernel.org> wrote: > > For very large ELF objects (with many sections), we could > get special value SHN_XINDEX (65535) for symbol's st_shndx. > > This patch is adding code to detect the optional extended > section index table and use it to resolve symbol's section > index. > > Adding elf_symtab__for_each_symbol_index macro that returns > symbol's section index and usign it in collect functions. > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > btf_encoder.c | 59 +++++++++++++++++++++++++++++++++++++-------------- > elf_symtab.c | 39 +++++++++++++++++++++++++++++++++- > elf_symtab.h | 2 ++ > 3 files changed, 83 insertions(+), 17 deletions(-) > > diff --git a/btf_encoder.c b/btf_encoder.c > index 5557c9efd365..56ee55965093 100644 > --- a/btf_encoder.c > +++ b/btf_encoder.c > @@ -63,13 +63,13 @@ static void delete_functions(void) > #define max(x, y) ((x) < (y) ? (y) : (x)) > #endif > > -static int collect_function(struct btf_elf *btfe, GElf_Sym *sym) > +static int collect_function(struct btf_elf *btfe, GElf_Sym *sym, > + Elf32_Word sym_sec_idx) nit: we use size_t or int for this, no need for libelf types here, imo > { > struct elf_function *new; > static GElf_Shdr sh; > - static int last_idx; > + static Elf32_Word last_idx; > const char *name; > - int idx; > > if (elf_sym__type(sym) != STT_FUNC) > return 0; > @@ -90,12 +90,10 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym) > functions = new; > } > > - idx = elf_sym__section(sym); > - > - if (idx != last_idx) { > - if (!elf_section_by_idx(btfe->elf, &sh, idx)) > + if (sym_sec_idx != last_idx) { > + if (!elf_section_by_idx(btfe->elf, &sh, sym_sec_idx)) > return 0; > - last_idx = idx; > + last_idx = sym_sec_idx; > } > > functions[functions_cnt].name = name; > @@ -542,14 +540,15 @@ static bool percpu_var_exists(uint64_t addr, uint32_t *sz, const char **name) > return true; > } > > -static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym) > +static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym, > + Elf32_Word sym_sec_idx) nit: same, size_t or just int would be fine > { > const char *sym_name; > uint64_t addr; > uint32_t size; > > /* compare a symbol's shndx to determine if it's a percpu variable */ > - if (elf_sym__section(sym) != btfe->percpu_shndx) > + if (sym_sec_idx != btfe->percpu_shndx) > return 0; > if (elf_sym__type(sym) != STT_OBJECT) > return 0; > @@ -585,12 +584,13 @@ static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym) > return 0; > } > > -static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl) > +static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl, > + Elf32_Word sym_sec_idx) > { > if (!fl->mcount_start && > !strcmp("__start_mcount_loc", elf_sym__name(sym, btfe->symtab))) { > fl->mcount_start = sym->st_value; > - fl->mcount_sec_idx = sym->st_shndx; > + fl->mcount_sec_idx = sym_sec_idx; > } > > if (!fl->mcount_stop && > @@ -598,9 +598,36 @@ static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl) > fl->mcount_stop = sym->st_value; > } > > +static bool elf_sym__get(Elf_Data *syms, Elf_Data *syms_sec_idx_table, > + int id, GElf_Sym *sym, Elf32_Word *sym_sec_idx) This is a generic function, why don't you want to move it into elf_symtab.h? > +{ > + if (!gelf_getsym(syms, id, sym)) > + return false; > + > + *sym_sec_idx = sym->st_shndx; > + > + if (sym->st_shndx == SHN_XINDEX) { > + if (!syms_sec_idx_table) > + return false; > + if (!gelf_getsymshndx(syms, syms_sec_idx_table, > + id, sym, sym_sec_idx)) > + return false; You also ignored my feedback about not fetching symbol twice. Why? > + } > + > + return true; > +} > + > +#define elf_symtab__for_each_symbol_index(symtab, id, sym, sym_sec_idx) \ > + for (id = 0; \ > + id < symtab->nr_syms && \ > + elf_sym__get(symtab->syms, symtab->syms_sec_idx_table, \ > + id, &sym, &sym_sec_idx); \ > + id++) This should be in elf_symtab.h next to elf_symtab__for_each_symbol. And thinking a bit more, the variant with just ignoring symbols that we failed to get is probably a safer alternative. I.e., currently there is no way to communicate that we terminated iteration with error, so it's probably better to skip failed symbols and still get the rest, no? I was hoping to discuss stuff like this on the previous version of the patch... And please do fix elf_symtab__for_each_symbol(). > + > static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars) > { > struct funcs_layout fl = { }; > + Elf32_Word sym_sec_idx; > uint32_t core_id; > GElf_Sym sym; > [...]
On Fri, Jan 22, 2021 at 04:52:28PM -0300, Arnaldo Carvalho de Melo wrote: > Em Fri, Jan 22, 2021 at 05:39:20PM +0100, Jiri Olsa escreveu: > > For very large ELF objects (with many sections), we could > > get special value SHN_XINDEX (65535) for symbol's st_shndx. > > > > This patch is adding code to detect the optional extended > > section index table and use it to resolve symbol's section > > index. > > > > Adding elf_symtab__for_each_symbol_index macro that returns > > symbol's section index and usign it in collect functions. > > From a quick look it seems you addressed Andrii's review comments, > right? yep, it's described in the cover email > > I've merged it locally, but would like to have some detailed set of > steps on how to test this, so that I can add it to a "Committer testing" > section in the cset commit log and probably add it to my local set of > regression tests. sorry I forgot to mention that: The test was to run pahole on kernel compiled with: make KCFLAGS="-ffunction-sections -fdata-sections" -j$(nproc) vmlinux and ensure FUNC records are generated and match normal build (without above KCFLAGS) Also bpf selftest passed. > > Who originally reported this? Joe? Also can someone provide a Tested-by: > in addition to mine when I get this detailed set of steps to test? oops, it was reported by Yulia Kopkova (just cc-ed) Joe tested the v2 of the patchset, I'll make a dwarves scratch build with v3 and let them test it jirka
Em Fri, Jan 22, 2021 at 09:24:03PM +0100, Jiri Olsa escreveu: > On Fri, Jan 22, 2021 at 04:52:28PM -0300, Arnaldo Carvalho de Melo wrote: > > Em Fri, Jan 22, 2021 at 05:39:20PM +0100, Jiri Olsa escreveu: > > > For very large ELF objects (with many sections), we could > > > get special value SHN_XINDEX (65535) for symbol's st_shndx. > > > > > > This patch is adding code to detect the optional extended > > > section index table and use it to resolve symbol's section > > > index. > > > > > > Adding elf_symtab__for_each_symbol_index macro that returns > > > symbol's section index and usign it in collect functions. > > > > From a quick look it seems you addressed Andrii's review comments, > > right? > > yep, it's described in the cover email > > > > > I've merged it locally, but would like to have some detailed set of > > steps on how to test this, so that I can add it to a "Committer testing" > > section in the cset commit log and probably add it to my local set of > > regression tests. > > sorry I forgot to mention that: > > The test was to run pahole on kernel compiled with: > make KCFLAGS="-ffunction-sections -fdata-sections" -j$(nproc) vmlinux > > and ensure FUNC records are generated and match normal > build (without above KCFLAGS) > > Also bpf selftest passed. Thanks, I'll come up with some shell script to test that. > > > > > Who originally reported this? Joe? Also can someone provide a Tested-by: > > in addition to mine when I get this detailed set of steps to test? > > oops, it was reported by Yulia Kopkova (just cc-ed) > > Joe tested the v2 of the patchset, I'll make a dwarves scratch > build with v3 and let them test it Thanks, and there is a new comment by Andrii that I've found relevant about using size_t instead of Elf_something. - Arnaldo
On Fri, Jan 22, 2021 at 12:16:42PM -0800, Andrii Nakryiko wrote: > On Fri, Jan 22, 2021 at 8:46 AM Jiri Olsa <jolsa@kernel.org> wrote: > > > > For very large ELF objects (with many sections), we could > > get special value SHN_XINDEX (65535) for symbol's st_shndx. > > > > This patch is adding code to detect the optional extended > > section index table and use it to resolve symbol's section > > index. > > > > Adding elf_symtab__for_each_symbol_index macro that returns > > symbol's section index and usign it in collect functions. > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > --- > > btf_encoder.c | 59 +++++++++++++++++++++++++++++++++++++-------------- > > elf_symtab.c | 39 +++++++++++++++++++++++++++++++++- > > elf_symtab.h | 2 ++ > > 3 files changed, 83 insertions(+), 17 deletions(-) > > > > diff --git a/btf_encoder.c b/btf_encoder.c > > index 5557c9efd365..56ee55965093 100644 > > --- a/btf_encoder.c > > +++ b/btf_encoder.c > > @@ -63,13 +63,13 @@ static void delete_functions(void) > > #define max(x, y) ((x) < (y) ? (y) : (x)) > > #endif > > > > -static int collect_function(struct btf_elf *btfe, GElf_Sym *sym) > > +static int collect_function(struct btf_elf *btfe, GElf_Sym *sym, > > + Elf32_Word sym_sec_idx) > > nit: we use size_t or int for this, no need for libelf types here, imo ok > > > > > { > > struct elf_function *new; > > static GElf_Shdr sh; > > - static int last_idx; > > + static Elf32_Word last_idx; > > const char *name; > > - int idx; > > > > if (elf_sym__type(sym) != STT_FUNC) > > return 0; > > @@ -90,12 +90,10 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym) > > functions = new; > > } > > > > - idx = elf_sym__section(sym); > > - > > - if (idx != last_idx) { > > - if (!elf_section_by_idx(btfe->elf, &sh, idx)) > > + if (sym_sec_idx != last_idx) { > > + if (!elf_section_by_idx(btfe->elf, &sh, sym_sec_idx)) > > return 0; > > - last_idx = idx; > > + last_idx = sym_sec_idx; > > } > > > > functions[functions_cnt].name = name; > > @@ -542,14 +540,15 @@ static bool percpu_var_exists(uint64_t addr, uint32_t *sz, const char **name) > > return true; > > } > > > > -static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym) > > +static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym, > > + Elf32_Word sym_sec_idx) > > nit: same, size_t or just int would be fine > > > { > > const char *sym_name; > > uint64_t addr; > > uint32_t size; > > > > /* compare a symbol's shndx to determine if it's a percpu variable */ > > - if (elf_sym__section(sym) != btfe->percpu_shndx) > > + if (sym_sec_idx != btfe->percpu_shndx) > > return 0; > > if (elf_sym__type(sym) != STT_OBJECT) > > return 0; > > @@ -585,12 +584,13 @@ static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym) > > return 0; > > } > > > > -static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl) > > +static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl, > > + Elf32_Word sym_sec_idx) > > { > > if (!fl->mcount_start && > > !strcmp("__start_mcount_loc", elf_sym__name(sym, btfe->symtab))) { > > fl->mcount_start = sym->st_value; > > - fl->mcount_sec_idx = sym->st_shndx; > > + fl->mcount_sec_idx = sym_sec_idx; > > } > > > > if (!fl->mcount_stop && > > @@ -598,9 +598,36 @@ static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl) > > fl->mcount_stop = sym->st_value; > > } > > > > +static bool elf_sym__get(Elf_Data *syms, Elf_Data *syms_sec_idx_table, > > + int id, GElf_Sym *sym, Elf32_Word *sym_sec_idx) > > This is a generic function, why don't you want to move it into elf_symtab.h? > > > +{ > > + if (!gelf_getsym(syms, id, sym)) > > + return false; > > + > > + *sym_sec_idx = sym->st_shndx; > > + > > + if (sym->st_shndx == SHN_XINDEX) { > > + if (!syms_sec_idx_table) > > + return false; > > + if (!gelf_getsymshndx(syms, syms_sec_idx_table, > > + id, sym, sym_sec_idx)) > > + return false; > > You also ignored my feedback about not fetching symbol twice. Why? ugh, I did not read your last 2 comments.. let me check that, sry > > > + } > > + > > + return true; > > +} > > + > > +#define elf_symtab__for_each_symbol_index(symtab, id, sym, sym_sec_idx) \ > > + for (id = 0; \ > > + id < symtab->nr_syms && \ > > + elf_sym__get(symtab->syms, symtab->syms_sec_idx_table, \ > > + id, &sym, &sym_sec_idx); \ > > + id++) > > This should be in elf_symtab.h next to elf_symtab__for_each_symbol. > > And thinking a bit more, the variant with just ignoring symbols that > we failed to get is probably a safer alternative. I.e., currently > there is no way to communicate that we terminated iteration with > error, so it's probably better to skip failed symbols and still get > the rest, no? I was hoping to discuss stuff like this on the previous > version of the patch... I was thinking of that, but then I thought it's better to fail, than have possibly wrong data in BTF, because the ELF data is possibly damaged? no idea.. so I took the safer way jirka
On 1/22/21 2:52 PM, Arnaldo Carvalho de Melo wrote: > > Who originally reported this? Joe? Also can someone provide a Tested-by: > in addition to mine when I get this detailed set of steps to test? > As Jiri noted, we tested v2 I think, so if there is a v4 build we could give a spin, just let us know. In the meantime, for kpatch, we figured that we could just temporarily disable CONFIG_DEBUG_INFO_BTF in the scripts/link-vmlinux.sh file during kpatch builds ... that would leave kernel code intact, but skip the BTF generation step (for which kpatch doesn't need anyway). Thanks, -- Joe
Em Fri, Jan 22, 2021 at 09:37:48PM +0100, Jiri Olsa escreveu: > On Fri, Jan 22, 2021 at 12:16:42PM -0800, Andrii Nakryiko wrote: > > On Fri, Jan 22, 2021 at 8:46 AM Jiri Olsa <jolsa@kernel.org> wrote: > > > > > > For very large ELF objects (with many sections), we could > > > get special value SHN_XINDEX (65535) for symbol's st_shndx. > > > > > > This patch is adding code to detect the optional extended > > > section index table and use it to resolve symbol's section > > > index. > > > > > > Adding elf_symtab__for_each_symbol_index macro that returns > > > symbol's section index and usign it in collect functions. > > > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > > --- > > > btf_encoder.c | 59 +++++++++++++++++++++++++++++++++++++-------------- > > > elf_symtab.c | 39 +++++++++++++++++++++++++++++++++- > > > elf_symtab.h | 2 ++ > > > 3 files changed, 83 insertions(+), 17 deletions(-) > > > > > > diff --git a/btf_encoder.c b/btf_encoder.c > > > index 5557c9efd365..56ee55965093 100644 > > > --- a/btf_encoder.c > > > +++ b/btf_encoder.c > > > @@ -63,13 +63,13 @@ static void delete_functions(void) > > > #define max(x, y) ((x) < (y) ? (y) : (x)) > > > #endif > > > > > > -static int collect_function(struct btf_elf *btfe, GElf_Sym *sym) > > > +static int collect_function(struct btf_elf *btfe, GElf_Sym *sym, > > > + Elf32_Word sym_sec_idx) > > > > nit: we use size_t or int for this, no need for libelf types here, imo > > ok I think it is because this patch ends up calling extern GElf_Sym *gelf_getsymshndx (Elf_Data *__symdata, Elf_Data *__shndxdata, int __ndx, GElf_Sym *__sym, Elf32_Word *__xshndx); Which expects a pointer to a Elf32_Word, right Jiri? Jiri, are you going to submit a new version of this patch? I processed the first one, collecting Andrii's Acked-by, if you're busy I can try to address the other concerns from Andrii, please let me know. - Arnaldo > > > > > > > > > { > > > struct elf_function *new; > > > static GElf_Shdr sh; > > > - static int last_idx; > > > + static Elf32_Word last_idx; > > > const char *name; > > > - int idx; > > > > > > if (elf_sym__type(sym) != STT_FUNC) > > > return 0; > > > @@ -90,12 +90,10 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym) > > > functions = new; > > > } > > > > > > - idx = elf_sym__section(sym); > > > - > > > - if (idx != last_idx) { > > > - if (!elf_section_by_idx(btfe->elf, &sh, idx)) > > > + if (sym_sec_idx != last_idx) { > > > + if (!elf_section_by_idx(btfe->elf, &sh, sym_sec_idx)) > > > return 0; > > > - last_idx = idx; > > > + last_idx = sym_sec_idx; > > > } > > > > > > functions[functions_cnt].name = name; > > > @@ -542,14 +540,15 @@ static bool percpu_var_exists(uint64_t addr, uint32_t *sz, const char **name) > > > return true; > > > } > > > > > > -static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym) > > > +static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym, > > > + Elf32_Word sym_sec_idx) > > > > nit: same, size_t or just int would be fine > > > > > { > > > const char *sym_name; > > > uint64_t addr; > > > uint32_t size; > > > > > > /* compare a symbol's shndx to determine if it's a percpu variable */ > > > - if (elf_sym__section(sym) != btfe->percpu_shndx) > > > + if (sym_sec_idx != btfe->percpu_shndx) > > > return 0; > > > if (elf_sym__type(sym) != STT_OBJECT) > > > return 0; > > > @@ -585,12 +584,13 @@ static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym) > > > return 0; > > > } > > > > > > -static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl) > > > +static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl, > > > + Elf32_Word sym_sec_idx) > > > { > > > if (!fl->mcount_start && > > > !strcmp("__start_mcount_loc", elf_sym__name(sym, btfe->symtab))) { > > > fl->mcount_start = sym->st_value; > > > - fl->mcount_sec_idx = sym->st_shndx; > > > + fl->mcount_sec_idx = sym_sec_idx; > > > } > > > > > > if (!fl->mcount_stop && > > > @@ -598,9 +598,36 @@ static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl) > > > fl->mcount_stop = sym->st_value; > > > } > > > > > > +static bool elf_sym__get(Elf_Data *syms, Elf_Data *syms_sec_idx_table, > > > + int id, GElf_Sym *sym, Elf32_Word *sym_sec_idx) > > > > This is a generic function, why don't you want to move it into elf_symtab.h? > > > > > +{ > > > + if (!gelf_getsym(syms, id, sym)) > > > + return false; > > > + > > > + *sym_sec_idx = sym->st_shndx; > > > + > > > + if (sym->st_shndx == SHN_XINDEX) { > > > + if (!syms_sec_idx_table) > > > + return false; > > > + if (!gelf_getsymshndx(syms, syms_sec_idx_table, > > > + id, sym, sym_sec_idx)) > > > + return false; > > > > You also ignored my feedback about not fetching symbol twice. Why? > > ugh, I did not read your last 2 comments.. let me check that, sry > > > > > > + } > > > + > > > + return true; > > > +} > > > + > > > +#define elf_symtab__for_each_symbol_index(symtab, id, sym, sym_sec_idx) \ > > > + for (id = 0; \ > > > + id < symtab->nr_syms && \ > > > + elf_sym__get(symtab->syms, symtab->syms_sec_idx_table, \ > > > + id, &sym, &sym_sec_idx); \ > > > + id++) > > > > This should be in elf_symtab.h next to elf_symtab__for_each_symbol. > > > > And thinking a bit more, the variant with just ignoring symbols that > > we failed to get is probably a safer alternative. I.e., currently > > there is no way to communicate that we terminated iteration with > > error, so it's probably better to skip failed symbols and still get > > the rest, no? I was hoping to discuss stuff like this on the previous > > version of the patch... > > I was thinking of that, but then I thought it's better to fail, > than have possibly wrong data in BTF, because the ELF data is > possibly damaged? no idea.. so I took the safer way > > jirka >
Em Mon, Jan 25, 2021 at 02:37:11PM -0300, Arnaldo Carvalho de Melo escreveu: > Em Fri, Jan 22, 2021 at 09:37:48PM +0100, Jiri Olsa escreveu: > > On Fri, Jan 22, 2021 at 12:16:42PM -0800, Andrii Nakryiko wrote: > > > On Fri, Jan 22, 2021 at 8:46 AM Jiri Olsa <jolsa@kernel.org> wrote: > > > > > > > > For very large ELF objects (with many sections), we could > > > > get special value SHN_XINDEX (65535) for symbol's st_shndx. > > > > > > > > This patch is adding code to detect the optional extended > > > > section index table and use it to resolve symbol's section > > > > index. > > > > > > > > Adding elf_symtab__for_each_symbol_index macro that returns > > > > symbol's section index and usign it in collect functions. > > > > > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > > > --- > > > > btf_encoder.c | 59 +++++++++++++++++++++++++++++++++++++-------------- > > > > elf_symtab.c | 39 +++++++++++++++++++++++++++++++++- > > > > elf_symtab.h | 2 ++ > > > > 3 files changed, 83 insertions(+), 17 deletions(-) > > > > > > > > diff --git a/btf_encoder.c b/btf_encoder.c > > > > index 5557c9efd365..56ee55965093 100644 > > > > --- a/btf_encoder.c > > > > +++ b/btf_encoder.c > > > > @@ -63,13 +63,13 @@ static void delete_functions(void) > > > > #define max(x, y) ((x) < (y) ? (y) : (x)) > > > > #endif > > > > > > > > -static int collect_function(struct btf_elf *btfe, GElf_Sym *sym) > > > > +static int collect_function(struct btf_elf *btfe, GElf_Sym *sym, > > > > + Elf32_Word sym_sec_idx) > > > > > > nit: we use size_t or int for this, no need for libelf types here, imo > > > > ok > > I think it is because this patch ends up calling > > extern GElf_Sym *gelf_getsymshndx (Elf_Data *__symdata, Elf_Data *__shndxdata, > int __ndx, GElf_Sym *__sym, > Elf32_Word *__xshndx); > > Which expects a pointer to a Elf32_Word, right Jiri? > > Jiri, are you going to submit a new version of this patch? I processed Sorry, saw v4, processing... > the first one, collecting Andrii's Acked-by, if you're busy I can try to > address the other concerns from Andrii, please let me know. > > - Arnaldo > > > > > > > > > > > > > > { > > > > struct elf_function *new; > > > > static GElf_Shdr sh; > > > > - static int last_idx; > > > > + static Elf32_Word last_idx; > > > > const char *name; > > > > - int idx; > > > > > > > > if (elf_sym__type(sym) != STT_FUNC) > > > > return 0; > > > > @@ -90,12 +90,10 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym) > > > > functions = new; > > > > } > > > > > > > > - idx = elf_sym__section(sym); > > > > - > > > > - if (idx != last_idx) { > > > > - if (!elf_section_by_idx(btfe->elf, &sh, idx)) > > > > + if (sym_sec_idx != last_idx) { > > > > + if (!elf_section_by_idx(btfe->elf, &sh, sym_sec_idx)) > > > > return 0; > > > > - last_idx = idx; > > > > + last_idx = sym_sec_idx; > > > > } > > > > > > > > functions[functions_cnt].name = name; > > > > @@ -542,14 +540,15 @@ static bool percpu_var_exists(uint64_t addr, uint32_t *sz, const char **name) > > > > return true; > > > > } > > > > > > > > -static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym) > > > > +static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym, > > > > + Elf32_Word sym_sec_idx) > > > > > > nit: same, size_t or just int would be fine > > > > > > > { > > > > const char *sym_name; > > > > uint64_t addr; > > > > uint32_t size; > > > > > > > > /* compare a symbol's shndx to determine if it's a percpu variable */ > > > > - if (elf_sym__section(sym) != btfe->percpu_shndx) > > > > + if (sym_sec_idx != btfe->percpu_shndx) > > > > return 0; > > > > if (elf_sym__type(sym) != STT_OBJECT) > > > > return 0; > > > > @@ -585,12 +584,13 @@ static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym) > > > > return 0; > > > > } > > > > > > > > -static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl) > > > > +static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl, > > > > + Elf32_Word sym_sec_idx) > > > > { > > > > if (!fl->mcount_start && > > > > !strcmp("__start_mcount_loc", elf_sym__name(sym, btfe->symtab))) { > > > > fl->mcount_start = sym->st_value; > > > > - fl->mcount_sec_idx = sym->st_shndx; > > > > + fl->mcount_sec_idx = sym_sec_idx; > > > > } > > > > > > > > if (!fl->mcount_stop && > > > > @@ -598,9 +598,36 @@ static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl) > > > > fl->mcount_stop = sym->st_value; > > > > } > > > > > > > > +static bool elf_sym__get(Elf_Data *syms, Elf_Data *syms_sec_idx_table, > > > > + int id, GElf_Sym *sym, Elf32_Word *sym_sec_idx) > > > > > > This is a generic function, why don't you want to move it into elf_symtab.h? > > > > > > > +{ > > > > + if (!gelf_getsym(syms, id, sym)) > > > > + return false; > > > > + > > > > + *sym_sec_idx = sym->st_shndx; > > > > + > > > > + if (sym->st_shndx == SHN_XINDEX) { > > > > + if (!syms_sec_idx_table) > > > > + return false; > > > > + if (!gelf_getsymshndx(syms, syms_sec_idx_table, > > > > + id, sym, sym_sec_idx)) > > > > + return false; > > > > > > You also ignored my feedback about not fetching symbol twice. Why? > > > > ugh, I did not read your last 2 comments.. let me check that, sry > > > > > > > > > + } > > > > + > > > > + return true; > > > > +} > > > > + > > > > +#define elf_symtab__for_each_symbol_index(symtab, id, sym, sym_sec_idx) \ > > > > + for (id = 0; \ > > > > + id < symtab->nr_syms && \ > > > > + elf_sym__get(symtab->syms, symtab->syms_sec_idx_table, \ > > > > + id, &sym, &sym_sec_idx); \ > > > > + id++) > > > > > > This should be in elf_symtab.h next to elf_symtab__for_each_symbol. > > > > > > And thinking a bit more, the variant with just ignoring symbols that > > > we failed to get is probably a safer alternative. I.e., currently > > > there is no way to communicate that we terminated iteration with > > > error, so it's probably better to skip failed symbols and still get > > > the rest, no? I was hoping to discuss stuff like this on the previous > > > version of the patch... > > > > I was thinking of that, but then I thought it's better to fail, > > than have possibly wrong data in BTF, because the ELF data is > > possibly damaged? no idea.. so I took the safer way > > > > jirka > > > > -- > > - Arnaldo
diff --git a/btf_encoder.c b/btf_encoder.c index 5557c9efd365..56ee55965093 100644 --- a/btf_encoder.c +++ b/btf_encoder.c @@ -63,13 +63,13 @@ static void delete_functions(void) #define max(x, y) ((x) < (y) ? (y) : (x)) #endif -static int collect_function(struct btf_elf *btfe, GElf_Sym *sym) +static int collect_function(struct btf_elf *btfe, GElf_Sym *sym, + Elf32_Word sym_sec_idx) { struct elf_function *new; static GElf_Shdr sh; - static int last_idx; + static Elf32_Word last_idx; const char *name; - int idx; if (elf_sym__type(sym) != STT_FUNC) return 0; @@ -90,12 +90,10 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym) functions = new; } - idx = elf_sym__section(sym); - - if (idx != last_idx) { - if (!elf_section_by_idx(btfe->elf, &sh, idx)) + if (sym_sec_idx != last_idx) { + if (!elf_section_by_idx(btfe->elf, &sh, sym_sec_idx)) return 0; - last_idx = idx; + last_idx = sym_sec_idx; } functions[functions_cnt].name = name; @@ -542,14 +540,15 @@ static bool percpu_var_exists(uint64_t addr, uint32_t *sz, const char **name) return true; } -static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym) +static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym, + Elf32_Word sym_sec_idx) { const char *sym_name; uint64_t addr; uint32_t size; /* compare a symbol's shndx to determine if it's a percpu variable */ - if (elf_sym__section(sym) != btfe->percpu_shndx) + if (sym_sec_idx != btfe->percpu_shndx) return 0; if (elf_sym__type(sym) != STT_OBJECT) return 0; @@ -585,12 +584,13 @@ static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym) return 0; } -static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl) +static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl, + Elf32_Word sym_sec_idx) { if (!fl->mcount_start && !strcmp("__start_mcount_loc", elf_sym__name(sym, btfe->symtab))) { fl->mcount_start = sym->st_value; - fl->mcount_sec_idx = sym->st_shndx; + fl->mcount_sec_idx = sym_sec_idx; } if (!fl->mcount_stop && @@ -598,9 +598,36 @@ static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl) fl->mcount_stop = sym->st_value; } +static bool elf_sym__get(Elf_Data *syms, Elf_Data *syms_sec_idx_table, + int id, GElf_Sym *sym, Elf32_Word *sym_sec_idx) +{ + if (!gelf_getsym(syms, id, sym)) + return false; + + *sym_sec_idx = sym->st_shndx; + + if (sym->st_shndx == SHN_XINDEX) { + if (!syms_sec_idx_table) + return false; + if (!gelf_getsymshndx(syms, syms_sec_idx_table, + id, sym, sym_sec_idx)) + return false; + } + + return true; +} + +#define elf_symtab__for_each_symbol_index(symtab, id, sym, sym_sec_idx) \ + for (id = 0; \ + id < symtab->nr_syms && \ + elf_sym__get(symtab->syms, symtab->syms_sec_idx_table, \ + id, &sym, &sym_sec_idx); \ + id++) + static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars) { struct funcs_layout fl = { }; + Elf32_Word sym_sec_idx; uint32_t core_id; GElf_Sym sym; @@ -608,12 +635,12 @@ static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars) 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)) + elf_symtab__for_each_symbol_index(btfe->symtab, core_id, sym, sym_sec_idx) { + if (collect_percpu_vars && collect_percpu_var(btfe, &sym, sym_sec_idx)) return -1; - if (collect_function(btfe, &sym)) + if (collect_function(btfe, &sym, sym_sec_idx)) return -1; - collect_symbol(&sym, &fl); + collect_symbol(&sym, &fl, sym_sec_idx); } if (collect_percpu_vars) { diff --git a/elf_symtab.c b/elf_symtab.c index 741990ea3ed9..fad5e0c0ba3c 100644 --- a/elf_symtab.c +++ b/elf_symtab.c @@ -17,11 +17,13 @@ struct elf_symtab *elf_symtab__new(const char *name, Elf *elf, GElf_Ehdr *ehdr) { + size_t symtab_index; + if (name == NULL) name = ".symtab"; GElf_Shdr shdr; - Elf_Scn *sec = elf_section_by_name(elf, ehdr, &shdr, name, NULL); + Elf_Scn *sec = elf_section_by_name(elf, ehdr, &shdr, name, &symtab_index); if (sec == NULL) return NULL; @@ -41,6 +43,12 @@ struct elf_symtab *elf_symtab__new(const char *name, Elf *elf, GElf_Ehdr *ehdr) if (symtab->syms == NULL) goto out_free_name; + /* + * This returns extended section index table's + * section index, if it exists. + */ + int symtab_xindex = elf_scnshndx(sec); + sec = elf_getscn(elf, shdr.sh_link); if (sec == NULL) goto out_free_name; @@ -49,6 +57,35 @@ struct elf_symtab *elf_symtab__new(const char *name, Elf *elf, GElf_Ehdr *ehdr) if (symtab->symstrs == NULL) goto out_free_name; + /* + * The .symtab section has optional extended section index + * table, load its data so it can be used to resolve symbol's + * section index. + **/ + if (symtab_xindex > 0) { + GElf_Shdr shdr_xindex; + Elf_Scn *sec_xindex; + + sec_xindex = elf_getscn(elf, symtab_xindex); + if (sec_xindex == NULL) + goto out_free_name; + + if (gelf_getshdr(sec_xindex, &shdr_xindex) == NULL) + goto out_free_name; + + /* Extra check to verify it's correct type */ + if (shdr_xindex.sh_type != SHT_SYMTAB_SHNDX) + goto out_free_name; + + /* Extra check to verify it belongs to the .symtab */ + if (symtab_index != shdr_xindex.sh_link) + goto out_free_name; + + symtab->syms_sec_idx_table = elf_getdata(elf_getscn(elf, symtab_xindex), NULL); + if (symtab->syms_sec_idx_table == NULL) + goto out_free_name; + } + symtab->nr_syms = shdr.sh_size / shdr.sh_entsize; return symtab; diff --git a/elf_symtab.h b/elf_symtab.h index 359add69c8ab..2e05ca98158b 100644 --- a/elf_symtab.h +++ b/elf_symtab.h @@ -16,6 +16,8 @@ struct elf_symtab { uint32_t nr_syms; Elf_Data *syms; Elf_Data *symstrs; + /* Data of SHT_SYMTAB_SHNDX section. */ + Elf_Data *syms_sec_idx_table; char *name; };
For very large ELF objects (with many sections), we could get special value SHN_XINDEX (65535) for symbol's st_shndx. This patch is adding code to detect the optional extended section index table and use it to resolve symbol's section index. Adding elf_symtab__for_each_symbol_index macro that returns symbol's section index and usign it in collect functions. Signed-off-by: Jiri Olsa <jolsa@kernel.org> --- btf_encoder.c | 59 +++++++++++++++++++++++++++++++++++++-------------- elf_symtab.c | 39 +++++++++++++++++++++++++++++++++- elf_symtab.h | 2 ++ 3 files changed, 83 insertions(+), 17 deletions(-)