| Message ID | 20241002235253.487251-3-stephen.s.brennan@oracle.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | BPF |
| Headers | show |
| Series | [dwarves,v3,1/5] btf_encoder: use bitfield to control var encoding | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On 03/10/2024 00:52, Stephen Brennan wrote: > Currently we index symbols from the percpu ELF section, and when > processing DWARF variables for inclusion, we check whether the variable > matches an existing symbol. The matched symbol is used for three > purposes: > > 1. When no symbol of the same address is found, the variable is skipped. > This can occur because the symbol name was an invalid BTF > identifier, and so it did not get indexed. Or more commonly, it can > be because the variable is not stored in the per-cpu section, and > thus was not indexed. > 2. If the symbol offset is 0, then we compare the DWARF variable's name > against the symbol name to filter out "special" DWARF variables. > 3. We use the symbol size in the DATASEC entry for the variable. > > For 1, we don't need the symbol table: we can simply check the DWARF > variable name directly, and we can use the variable address to determine > the ELF section it is contained in. For 3, we also don't need the symbol > table: we can use the variable's size information from DWARF. Issue 2 is > more complicated, but thanks to the addition of the "artificial" and > "top_level" flags, many of the "special" DWARF variables can be directly > filtered out, and the few remaining problematic variables can be > filtered by name from a kernel-specific list of patterns. > > This allows the symbol table index to be removed. The benefit of > removing this index is twofold. First, handling variable addresses is > simplified, since we don't need to know whether the file is ET_REL. > Second, this will make it easier to output variables that aren't just > percpu, since we won't need to index variables from all ELF sections. > > Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com> a few small things below, but Reviewed-by: Alan Maguire <alan.maguire@oracle.com> > --- > btf_encoder.c | 250 +++++++++++++++++++------------------------------- > 1 file changed, 96 insertions(+), 154 deletions(-) > > diff --git a/btf_encoder.c b/btf_encoder.c > index 652a945..31a418a 100644 > --- a/btf_encoder.c > +++ b/btf_encoder.c > @@ -93,16 +93,11 @@ struct elf_function { > struct btf_encoder_func_state state; > }; > > -struct var_info { > - uint64_t addr; > - const char *name; > - uint32_t sz; > -}; > - > struct elf_secinfo { > uint64_t addr; > const char *name; > uint64_t sz; > + uint32_t type; > }; > > /* > @@ -125,17 +120,11 @@ struct btf_encoder { > gen_floats, > skip_encoding_decl_tag, > tag_kfuncs, > - is_rel, > gen_distilled_base; > uint32_t array_index_id; > struct elf_secinfo *secinfo; > size_t seccnt; > - struct { > - struct var_info *vars; > - int var_cnt; > - int allocated; > - uint32_t shndx; > - } percpu; > + size_t percpu_shndx; nit: feels odd to specify the shndx as a size_t ; libelf uses an int as return value for elf_scnshndx(). Not a big deal tho. > int encode_vars; > struct { > struct elf_function *entries; > @@ -2098,111 +2087,18 @@ int btf_encoder__encode(struct btf_encoder *encoder) > return err; > } > > -static int percpu_var_cmp(const void *_a, const void *_b) > -{ > - const struct var_info *a = _a; > - const struct var_info *b = _b; > - > - if (a->addr == b->addr) > - return 0; > - return a->addr < b->addr ? -1 : 1; > -} > - > -static bool btf_encoder__percpu_var_exists(struct btf_encoder *encoder, uint64_t addr, uint32_t *sz, const char **name) > -{ > - struct var_info key = { .addr = addr }; > - const struct var_info *p = bsearch(&key, encoder->percpu.vars, encoder->percpu.var_cnt, > - sizeof(encoder->percpu.vars[0]), percpu_var_cmp); > - if (!p) > - return false; > - > - *sz = p->sz; > - *name = p->name; > - return true; > -} > - > -static int btf_encoder__collect_percpu_var(struct btf_encoder *encoder, GElf_Sym *sym, size_t 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 (sym_sec_idx != encoder->percpu.shndx) > - return 0; > - if (elf_sym__type(sym) != STT_OBJECT) > - return 0; > - > - addr = elf_sym__value(sym); > - > - size = elf_sym__size(sym); > - if (!size) > - return 0; /* ignore zero-sized symbols */ > - > - sym_name = elf_sym__name(sym, encoder->symtab); > - if (!btf_name_valid(sym_name)) { > - dump_invalid_symbol("Found symbol of invalid name when encoding btf", > - sym_name, encoder->verbose, encoder->force); > - if (encoder->force) > - return 0; > - return -1; > - } > - > - if (encoder->verbose) > - printf("Found per-CPU symbol '%s' at address 0x%" PRIx64 "\n", sym_name, addr); > - > - /* Make sure addr is section-relative. For kernel modules (which are > - * ET_REL files) this is already the case. For vmlinux (which is an > - * ET_EXEC file) we need to subtract the section address. > - */ > - if (!encoder->is_rel) > - addr -= encoder->secinfo[encoder->percpu.shndx].addr; > - > - if (encoder->percpu.var_cnt == encoder->percpu.allocated) { > - struct var_info *new; > - > - new = reallocarray_grow(encoder->percpu.vars, > - &encoder->percpu.allocated, > - sizeof(*encoder->percpu.vars)); > - if (!new) { > - fprintf(stderr, "Failed to allocate memory for variables\n"); > - return -1; > - } > - encoder->percpu.vars = new; > - } > - encoder->percpu.vars[encoder->percpu.var_cnt].addr = addr; > - encoder->percpu.vars[encoder->percpu.var_cnt].sz = size; > - encoder->percpu.vars[encoder->percpu.var_cnt].name = sym_name; > - encoder->percpu.var_cnt++; > - > - return 0; > -} > > -static int btf_encoder__collect_symbols(struct btf_encoder *encoder, bool collect_percpu_vars) > +static int btf_encoder__collect_symbols(struct btf_encoder *encoder) > { > - Elf32_Word sym_sec_idx; > + uint32_t sym_sec_idx; > uint32_t core_id; > GElf_Sym sym; > > - /* cache variables' addresses, preparing for searching in symtab. */ > - encoder->percpu.var_cnt = 0; > - > - /* search within symtab for percpu variables */ > elf_symtab__for_each_symbol_index(encoder->symtab, core_id, sym, sym_sec_idx) { > - if (collect_percpu_vars && btf_encoder__collect_percpu_var(encoder, &sym, sym_sec_idx)) > - return -1; > if (btf_encoder__collect_function(encoder, &sym)) > return -1; > } > > - if (collect_percpu_vars) { > - if (encoder->percpu.var_cnt) > - qsort(encoder->percpu.vars, encoder->percpu.var_cnt, sizeof(encoder->percpu.vars[0]), percpu_var_cmp); > - > - if (encoder->verbose) > - printf("Found %d per-CPU variables!\n", encoder->percpu.var_cnt); > - } > - > if (encoder->functions.cnt) { > qsort(encoder->functions.entries, encoder->functions.cnt, sizeof(encoder->functions.entries[0]), > functions_cmp); > @@ -2224,15 +2120,54 @@ static bool ftype__has_arg_names(const struct ftype *ftype) > return true; > } > > +static int get_elf_section(struct btf_encoder *encoder, unsigned long addr) > +{ > + /* Start at index 1 to ignore initial SHT_NULL section */ > + for (int i = 1; i < encoder->seccnt; i++) > + /* Variables are only present in PROGBITS or NOBITS (.bss) */ > + if ((encoder->secinfo[i].type == SHT_PROGBITS || > + encoder->secinfo[i].type == SHT_NOBITS) && > + encoder->secinfo[i].addr <= addr && > + (addr - encoder->secinfo[i].addr) < encoder->secinfo[i].sz) > + return i; nit again: for readability this would benefit from brackets after the for () loop. because of the number of conditions might also be no harm to rewrite as for (int i = 1; i < encoder->seccnt; i++) { /* Variables are only present in PROGBITS or NOBITS (.bss) */ if (encoder->secinfo[i].type != SHT_PROGBITS && encoder->secinfo[i].type != SHT_NOBITS) continue; if (encoder->secinfo[i].addr <= addr && (addr - encoder->secinfo[i].addr) < encoder->secinfo[i].sz) return i; } > + return -ENOENT; > +} > + > +/* > + * Filter out variables / symbol names with common prefixes and no useful > + * values. Prefixes should be added sparingly, and it should be objectively > + * obvious that they are not useful. > + */ > +static bool filter_variable_name(const char *name) > +{ > + static const struct { char *s; size_t len; } skip[] = { > + #define X(str) {str, sizeof(str) - 1} > + X("__UNIQUE_ID"), > + X("__tpstrtab_"), > + X("__exitcall_"), > + X("__func_stack_frame_non_standard_") > + #undef X > + }; > + int i; > + > + if (*name != '_') > + return false; > + > + for (i = 0; i < ARRAY_SIZE(skip); i++) { > + if (strncmp(name, skip[i].s, skip[i].len) == 0) > + return true; > + } > + return false; > +} > + > static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder) > { > struct cu *cu = encoder->cu; > uint32_t core_id; > struct tag *pos; > int err = -1; > - struct elf_secinfo *pcpu_scn = &encoder->secinfo[encoder->percpu.shndx]; > > - if (encoder->percpu.shndx == 0 || !encoder->symtab) > + if (encoder->percpu_shndx == 0 || !encoder->symtab) > return 0; > > if (encoder->verbose) > @@ -2240,59 +2175,69 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder) > > cu__for_each_variable(cu, core_id, pos) { > struct variable *var = tag__variable(pos); > - uint32_t size, type, linkage; > - const char *name, *dwarf_name; > + uint32_t type, linkage; > + const char *name; > struct llvm_annotation *annot; > const struct tag *tag; > + size_t shndx, size; > uint64_t addr; > int id; > > + /* Skip incomplete (non-defining) declarations */ > if (var->declaration && !var->spec) > continue; > > - /* percpu variables are allocated in global space */ > - if (variable__scope(var) != VSCOPE_GLOBAL && !var->spec) > + /* > + * top_level: indicates that the variable is declared at the top > + * level of the CU, and thus it is globally scoped. > + * artificial: indicates that the variable is a compiler-generated > + * "fake" variable that doesn't appear in the source. > + * scope: set by pahole to indicate the type of storage the > + * variable has. GLOBAL indicates it is stored in static > + * memory (as opposed to a stack variable or register) > + * > + * Some variables are "top_level" but not GLOBAL: > + * e.g. current_stack_pointer, which is a register variable, > + * despite having global CU-declarations. We don't want that, > + * since no code could actually find this variable. > + * Some variables are GLOBAL but not top_level: > + * e.g. function static variables > + */ > + if (!var->top_level || var->artificial || var->scope != VSCOPE_GLOBAL) > continue; > > /* addr has to be recorded before we follow spec */ > addr = var->ip.addr; > - dwarf_name = variable__name(var); > > - /* Make sure addr is section-relative. DWARF, unlike ELF, > - * always contains virtual symbol addresses, so subtract > - * the section address unconditionally. > - */ > - if (addr < pcpu_scn->addr || addr >= pcpu_scn->addr + pcpu_scn->sz) > + /* Get the ELF section info for the variable */ > + shndx = get_elf_section(encoder, addr); > + if (shndx != encoder->percpu_shndx) > continue; > - addr -= pcpu_scn->addr; > > - if (!btf_encoder__percpu_var_exists(encoder, addr, &size, &name)) > - continue; /* not a per-CPU variable */ > + /* Convert addr to section relative */ > + addr -= encoder->secinfo[shndx].addr; > > - /* A lot of "special" DWARF variables (e.g, __UNIQUE_ID___xxx) > - * have addr == 0, which is the same as, say, valid > - * fixed_percpu_data per-CPU variable. To distinguish between > - * them, additionally compare DWARF and ELF symbol names. If > - * DWARF doesn't provide proper name, pessimistically assume > - * bad variable. > - * > - * Examples of such special variables are: > - * > - * 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. > - * > - * This is relevant only for vmlinux image, as for kernel > - * modules per-CPU data section has non-zero offset so all > - * per-CPU symbols have non-zero values. > - */ > - if (var->ip.addr == 0) { > - if (!dwarf_name || strcmp(dwarf_name, name)) > + /* DWARF specification reference should be followed, because > + * information like the name & type may not be present on var */ > + if (var->spec) > + var = var->spec; > + > + name = variable__name(var); > + if (!name) > + continue; > + > + /* Check for invalid BTF names */ > + if (!btf_name_valid(name)) { > + dump_invalid_symbol("Found invalid variable name when encoding btf", > + name, encoder->verbose, encoder->force); > + if (encoder->force) > continue; > + else > + return -1; > } > > - if (var->spec) > - var = var->spec; > + if (filter_variable_name(name)) > + continue; > > if (var->ip.tag.type == 0) { > fprintf(stderr, "error: found variable '%s' in CU '%s' that has void type\n", > @@ -2304,9 +2249,10 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder) > } > > tag = cu__type(cu, var->ip.tag.type); > - if (tag__size(tag, cu) == 0) { > + size = tag__size(tag, cu); > + if (size == 0) { > if (encoder->verbose) > - fprintf(stderr, "Ignoring zero-sized per-CPU variable '%s'...\n", dwarf_name ?: "<missing name>"); > + fprintf(stderr, "Ignoring zero-sized per-CPU variable '%s'...\n", name); > continue; > } > > @@ -2388,8 +2334,6 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam > goto out_delete; > } > > - encoder->is_rel = ehdr.e_type == ET_REL; > - > switch (ehdr.e_ident[EI_DATA]) { > case ELFDATA2LSB: > btf__set_endianness(encoder->btf, BTF_LITTLE_ENDIAN); > @@ -2430,15 +2374,16 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam > encoder->secinfo[shndx].addr = shdr.sh_addr; > encoder->secinfo[shndx].sz = shdr.sh_size; > encoder->secinfo[shndx].name = secname; > + encoder->secinfo[shndx].type = shdr.sh_type; > > if (strcmp(secname, PERCPU_SECTION) == 0) > - encoder->percpu.shndx = shndx; > + encoder->percpu_shndx = shndx; > } > > - if (!encoder->percpu.shndx && encoder->verbose) > + if (!encoder->percpu_shndx && encoder->verbose) > printf("%s: '%s' doesn't have '%s' section\n", __func__, cu->filename, PERCPU_SECTION); > > - if (btf_encoder__collect_symbols(encoder, encoder->encode_vars & BTF_VAR_PERCPU)) > + if (btf_encoder__collect_symbols(encoder)) > goto out_delete; > > if (encoder->verbose) > @@ -2480,9 +2425,6 @@ void btf_encoder__delete(struct btf_encoder *encoder) > encoder->functions.allocated = encoder->functions.cnt = 0; > free(encoder->functions.entries); > encoder->functions.entries = NULL; > - encoder->percpu.allocated = encoder->percpu.var_cnt = 0; > - free(encoder->percpu.vars); > - encoder->percpu.vars = NULL; > > free(encoder); > }
Alan Maguire <alan.maguire@oracle.com> writes: > On 03/10/2024 00:52, Stephen Brennan wrote: >> Currently we index symbols from the percpu ELF section, and when >> processing DWARF variables for inclusion, we check whether the variable >> matches an existing symbol. The matched symbol is used for three >> purposes: >> >> 1. When no symbol of the same address is found, the variable is skipped. >> This can occur because the symbol name was an invalid BTF >> identifier, and so it did not get indexed. Or more commonly, it can >> be because the variable is not stored in the per-cpu section, and >> thus was not indexed. >> 2. If the symbol offset is 0, then we compare the DWARF variable's name >> against the symbol name to filter out "special" DWARF variables. >> 3. We use the symbol size in the DATASEC entry for the variable. >> >> For 1, we don't need the symbol table: we can simply check the DWARF >> variable name directly, and we can use the variable address to determine >> the ELF section it is contained in. For 3, we also don't need the symbol >> table: we can use the variable's size information from DWARF. Issue 2 is >> more complicated, but thanks to the addition of the "artificial" and >> "top_level" flags, many of the "special" DWARF variables can be directly >> filtered out, and the few remaining problematic variables can be >> filtered by name from a kernel-specific list of patterns. >> >> This allows the symbol table index to be removed. The benefit of >> removing this index is twofold. First, handling variable addresses is >> simplified, since we don't need to know whether the file is ET_REL. >> Second, this will make it easier to output variables that aren't just >> percpu, since we won't need to index variables from all ELF sections. >> >> Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com> > > a few small things below, but > > Reviewed-by: Alan Maguire <alan.maguire@oracle.com> > >> --- >> btf_encoder.c | 250 +++++++++++++++++++------------------------------- >> 1 file changed, 96 insertions(+), 154 deletions(-) >> >> diff --git a/btf_encoder.c b/btf_encoder.c >> index 652a945..31a418a 100644 >> --- a/btf_encoder.c >> +++ b/btf_encoder.c >> @@ -93,16 +93,11 @@ struct elf_function { >> struct btf_encoder_func_state state; >> }; >> >> -struct var_info { >> - uint64_t addr; >> - const char *name; >> - uint32_t sz; >> -}; >> - >> struct elf_secinfo { >> uint64_t addr; >> const char *name; >> uint64_t sz; >> + uint32_t type; >> }; >> >> /* >> @@ -125,17 +120,11 @@ struct btf_encoder { >> gen_floats, >> skip_encoding_decl_tag, >> tag_kfuncs, >> - is_rel, >> gen_distilled_base; >> uint32_t array_index_id; >> struct elf_secinfo *secinfo; >> size_t seccnt; >> - struct { >> - struct var_info *vars; >> - int var_cnt; >> - int allocated; >> - uint32_t shndx; >> - } percpu; >> + size_t percpu_shndx; > > nit: feels odd to specify the shndx as a size_t ; libelf uses an int as > return value for elf_scnshndx(). Not a big deal tho. I picked size_t because elf_getshdrnum() places its result in a size_t variable, and technically the extended value of e_shnum (which lives in the sh_size field of the 0th entry in the section header table) could have a 64-bit value. I suppose that means that uint64_t would have been most correct (what if pahole is built on a 32-bit platform and analyzing a 64-bit ELF file?), but I decided to match the size_t from the libelf API, and it also matches the "seccnt" variable above. Anyway as you pointed out, it's not necessarily a huge deal since this will get deleted shortly. >> int encode_vars; >> struct { >> struct elf_function *entries; >> @@ -2098,111 +2087,18 @@ int btf_encoder__encode(struct btf_encoder *encoder) >> return err; >> } >> >> -static int percpu_var_cmp(const void *_a, const void *_b) >> -{ >> - const struct var_info *a = _a; >> - const struct var_info *b = _b; >> - >> - if (a->addr == b->addr) >> - return 0; >> - return a->addr < b->addr ? -1 : 1; >> -} >> - >> -static bool btf_encoder__percpu_var_exists(struct btf_encoder *encoder, uint64_t addr, uint32_t *sz, const char **name) >> -{ >> - struct var_info key = { .addr = addr }; >> - const struct var_info *p = bsearch(&key, encoder->percpu.vars, encoder->percpu.var_cnt, >> - sizeof(encoder->percpu.vars[0]), percpu_var_cmp); >> - if (!p) >> - return false; >> - >> - *sz = p->sz; >> - *name = p->name; >> - return true; >> -} >> - >> -static int btf_encoder__collect_percpu_var(struct btf_encoder *encoder, GElf_Sym *sym, size_t 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 (sym_sec_idx != encoder->percpu.shndx) >> - return 0; >> - if (elf_sym__type(sym) != STT_OBJECT) >> - return 0; >> - >> - addr = elf_sym__value(sym); >> - >> - size = elf_sym__size(sym); >> - if (!size) >> - return 0; /* ignore zero-sized symbols */ >> - >> - sym_name = elf_sym__name(sym, encoder->symtab); >> - if (!btf_name_valid(sym_name)) { >> - dump_invalid_symbol("Found symbol of invalid name when encoding btf", >> - sym_name, encoder->verbose, encoder->force); >> - if (encoder->force) >> - return 0; >> - return -1; >> - } >> - >> - if (encoder->verbose) >> - printf("Found per-CPU symbol '%s' at address 0x%" PRIx64 "\n", sym_name, addr); >> - >> - /* Make sure addr is section-relative. For kernel modules (which are >> - * ET_REL files) this is already the case. For vmlinux (which is an >> - * ET_EXEC file) we need to subtract the section address. >> - */ >> - if (!encoder->is_rel) >> - addr -= encoder->secinfo[encoder->percpu.shndx].addr; >> - >> - if (encoder->percpu.var_cnt == encoder->percpu.allocated) { >> - struct var_info *new; >> - >> - new = reallocarray_grow(encoder->percpu.vars, >> - &encoder->percpu.allocated, >> - sizeof(*encoder->percpu.vars)); >> - if (!new) { >> - fprintf(stderr, "Failed to allocate memory for variables\n"); >> - return -1; >> - } >> - encoder->percpu.vars = new; >> - } >> - encoder->percpu.vars[encoder->percpu.var_cnt].addr = addr; >> - encoder->percpu.vars[encoder->percpu.var_cnt].sz = size; >> - encoder->percpu.vars[encoder->percpu.var_cnt].name = sym_name; >> - encoder->percpu.var_cnt++; >> - >> - return 0; >> -} >> >> -static int btf_encoder__collect_symbols(struct btf_encoder *encoder, bool collect_percpu_vars) >> +static int btf_encoder__collect_symbols(struct btf_encoder *encoder) >> { >> - Elf32_Word sym_sec_idx; >> + uint32_t sym_sec_idx; >> uint32_t core_id; >> GElf_Sym sym; >> >> - /* cache variables' addresses, preparing for searching in symtab. */ >> - encoder->percpu.var_cnt = 0; >> - >> - /* search within symtab for percpu variables */ >> elf_symtab__for_each_symbol_index(encoder->symtab, core_id, sym, sym_sec_idx) { >> - if (collect_percpu_vars && btf_encoder__collect_percpu_var(encoder, &sym, sym_sec_idx)) >> - return -1; >> if (btf_encoder__collect_function(encoder, &sym)) >> return -1; >> } >> >> - if (collect_percpu_vars) { >> - if (encoder->percpu.var_cnt) >> - qsort(encoder->percpu.vars, encoder->percpu.var_cnt, sizeof(encoder->percpu.vars[0]), percpu_var_cmp); >> - >> - if (encoder->verbose) >> - printf("Found %d per-CPU variables!\n", encoder->percpu.var_cnt); >> - } >> - >> if (encoder->functions.cnt) { >> qsort(encoder->functions.entries, encoder->functions.cnt, sizeof(encoder->functions.entries[0]), >> functions_cmp); >> @@ -2224,15 +2120,54 @@ static bool ftype__has_arg_names(const struct ftype *ftype) >> return true; >> } >> >> +static int get_elf_section(struct btf_encoder *encoder, unsigned long addr) >> +{ >> + /* Start at index 1 to ignore initial SHT_NULL section */ >> + for (int i = 1; i < encoder->seccnt; i++) >> + /* Variables are only present in PROGBITS or NOBITS (.bss) */ >> + if ((encoder->secinfo[i].type == SHT_PROGBITS || >> + encoder->secinfo[i].type == SHT_NOBITS) && >> + encoder->secinfo[i].addr <= addr && >> + (addr - encoder->secinfo[i].addr) < encoder->secinfo[i].sz) >> + return i; > > > nit again: for readability this would benefit from brackets after the > for () loop. because of the number of conditions might also be no harm > to rewrite as > > for (int i = 1; i < encoder->seccnt; i++) { > /* Variables are only present in PROGBITS or NOBITS (.bss) */ > if (encoder->secinfo[i].type != SHT_PROGBITS && > encoder->secinfo[i].type != SHT_NOBITS) > continue; > > if (encoder->secinfo[i].addr <= addr && > (addr - encoder->secinfo[i].addr) < encoder->secinfo[i].sz) > return i; > } That's much clearer than mine! Thanks, I'll add this to my commit. >> + return -ENOENT; >> +} >> + >> +/* >> + * Filter out variables / symbol names with common prefixes and no useful >> + * values. Prefixes should be added sparingly, and it should be objectively >> + * obvious that they are not useful. >> + */ >> +static bool filter_variable_name(const char *name) >> +{ >> + static const struct { char *s; size_t len; } skip[] = { >> + #define X(str) {str, sizeof(str) - 1} >> + X("__UNIQUE_ID"), >> + X("__tpstrtab_"), >> + X("__exitcall_"), >> + X("__func_stack_frame_non_standard_") >> + #undef X >> + }; >> + int i; >> + >> + if (*name != '_') >> + return false; >> + >> + for (i = 0; i < ARRAY_SIZE(skip); i++) { >> + if (strncmp(name, skip[i].s, skip[i].len) == 0) >> + return true; >> + } >> + return false; >> +} >> + >> static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder) >> { >> struct cu *cu = encoder->cu; >> uint32_t core_id; >> struct tag *pos; >> int err = -1; >> - struct elf_secinfo *pcpu_scn = &encoder->secinfo[encoder->percpu.shndx]; >> >> - if (encoder->percpu.shndx == 0 || !encoder->symtab) >> + if (encoder->percpu_shndx == 0 || !encoder->symtab) >> return 0; >> >> if (encoder->verbose) >> @@ -2240,59 +2175,69 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder) >> >> cu__for_each_variable(cu, core_id, pos) { >> struct variable *var = tag__variable(pos); >> - uint32_t size, type, linkage; >> - const char *name, *dwarf_name; >> + uint32_t type, linkage; >> + const char *name; >> struct llvm_annotation *annot; >> const struct tag *tag; >> + size_t shndx, size; >> uint64_t addr; >> int id; >> >> + /* Skip incomplete (non-defining) declarations */ >> if (var->declaration && !var->spec) >> continue; >> >> - /* percpu variables are allocated in global space */ >> - if (variable__scope(var) != VSCOPE_GLOBAL && !var->spec) >> + /* >> + * top_level: indicates that the variable is declared at the top >> + * level of the CU, and thus it is globally scoped. >> + * artificial: indicates that the variable is a compiler-generated >> + * "fake" variable that doesn't appear in the source. >> + * scope: set by pahole to indicate the type of storage the >> + * variable has. GLOBAL indicates it is stored in static >> + * memory (as opposed to a stack variable or register) >> + * >> + * Some variables are "top_level" but not GLOBAL: >> + * e.g. current_stack_pointer, which is a register variable, >> + * despite having global CU-declarations. We don't want that, >> + * since no code could actually find this variable. >> + * Some variables are GLOBAL but not top_level: >> + * e.g. function static variables >> + */ >> + if (!var->top_level || var->artificial || var->scope != VSCOPE_GLOBAL) >> continue; >> >> /* addr has to be recorded before we follow spec */ >> addr = var->ip.addr; >> - dwarf_name = variable__name(var); >> >> - /* Make sure addr is section-relative. DWARF, unlike ELF, >> - * always contains virtual symbol addresses, so subtract >> - * the section address unconditionally. >> - */ >> - if (addr < pcpu_scn->addr || addr >= pcpu_scn->addr + pcpu_scn->sz) >> + /* Get the ELF section info for the variable */ >> + shndx = get_elf_section(encoder, addr); >> + if (shndx != encoder->percpu_shndx) >> continue; >> - addr -= pcpu_scn->addr; >> >> - if (!btf_encoder__percpu_var_exists(encoder, addr, &size, &name)) >> - continue; /* not a per-CPU variable */ >> + /* Convert addr to section relative */ >> + addr -= encoder->secinfo[shndx].addr; >> >> - /* A lot of "special" DWARF variables (e.g, __UNIQUE_ID___xxx) >> - * have addr == 0, which is the same as, say, valid >> - * fixed_percpu_data per-CPU variable. To distinguish between >> - * them, additionally compare DWARF and ELF symbol names. If >> - * DWARF doesn't provide proper name, pessimistically assume >> - * bad variable. >> - * >> - * Examples of such special variables are: >> - * >> - * 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. >> - * >> - * This is relevant only for vmlinux image, as for kernel >> - * modules per-CPU data section has non-zero offset so all >> - * per-CPU symbols have non-zero values. >> - */ >> - if (var->ip.addr == 0) { >> - if (!dwarf_name || strcmp(dwarf_name, name)) >> + /* DWARF specification reference should be followed, because >> + * information like the name & type may not be present on var */ >> + if (var->spec) >> + var = var->spec; >> + >> + name = variable__name(var); >> + if (!name) >> + continue; >> + >> + /* Check for invalid BTF names */ >> + if (!btf_name_valid(name)) { >> + dump_invalid_symbol("Found invalid variable name when encoding btf", >> + name, encoder->verbose, encoder->force); >> + if (encoder->force) >> continue; >> + else >> + return -1; >> } >> >> - if (var->spec) >> - var = var->spec; >> + if (filter_variable_name(name)) >> + continue; >> >> if (var->ip.tag.type == 0) { >> fprintf(stderr, "error: found variable '%s' in CU '%s' that has void type\n", >> @@ -2304,9 +2249,10 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder) >> } >> >> tag = cu__type(cu, var->ip.tag.type); >> - if (tag__size(tag, cu) == 0) { >> + size = tag__size(tag, cu); >> + if (size == 0) { >> if (encoder->verbose) >> - fprintf(stderr, "Ignoring zero-sized per-CPU variable '%s'...\n", dwarf_name ?: "<missing name>"); >> + fprintf(stderr, "Ignoring zero-sized per-CPU variable '%s'...\n", name); >> continue; >> } >> >> @@ -2388,8 +2334,6 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam >> goto out_delete; >> } >> >> - encoder->is_rel = ehdr.e_type == ET_REL; >> - >> switch (ehdr.e_ident[EI_DATA]) { >> case ELFDATA2LSB: >> btf__set_endianness(encoder->btf, BTF_LITTLE_ENDIAN); >> @@ -2430,15 +2374,16 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam >> encoder->secinfo[shndx].addr = shdr.sh_addr; >> encoder->secinfo[shndx].sz = shdr.sh_size; >> encoder->secinfo[shndx].name = secname; >> + encoder->secinfo[shndx].type = shdr.sh_type; >> >> if (strcmp(secname, PERCPU_SECTION) == 0) >> - encoder->percpu.shndx = shndx; >> + encoder->percpu_shndx = shndx; >> } >> >> - if (!encoder->percpu.shndx && encoder->verbose) >> + if (!encoder->percpu_shndx && encoder->verbose) >> printf("%s: '%s' doesn't have '%s' section\n", __func__, cu->filename, PERCPU_SECTION); >> >> - if (btf_encoder__collect_symbols(encoder, encoder->encode_vars & BTF_VAR_PERCPU)) >> + if (btf_encoder__collect_symbols(encoder)) >> goto out_delete; >> >> if (encoder->verbose) >> @@ -2480,9 +2425,6 @@ void btf_encoder__delete(struct btf_encoder *encoder) >> encoder->functions.allocated = encoder->functions.cnt = 0; >> free(encoder->functions.entries); >> encoder->functions.entries = NULL; >> - encoder->percpu.allocated = encoder->percpu.var_cnt = 0; >> - free(encoder->percpu.vars); >> - encoder->percpu.vars = NULL; >> >> free(encoder); >> }
diff --git a/btf_encoder.c b/btf_encoder.c index 652a945..31a418a 100644 --- a/btf_encoder.c +++ b/btf_encoder.c @@ -93,16 +93,11 @@ struct elf_function { struct btf_encoder_func_state state; }; -struct var_info { - uint64_t addr; - const char *name; - uint32_t sz; -}; - struct elf_secinfo { uint64_t addr; const char *name; uint64_t sz; + uint32_t type; }; /* @@ -125,17 +120,11 @@ struct btf_encoder { gen_floats, skip_encoding_decl_tag, tag_kfuncs, - is_rel, gen_distilled_base; uint32_t array_index_id; struct elf_secinfo *secinfo; size_t seccnt; - struct { - struct var_info *vars; - int var_cnt; - int allocated; - uint32_t shndx; - } percpu; + size_t percpu_shndx; int encode_vars; struct { struct elf_function *entries; @@ -2098,111 +2087,18 @@ int btf_encoder__encode(struct btf_encoder *encoder) return err; } -static int percpu_var_cmp(const void *_a, const void *_b) -{ - const struct var_info *a = _a; - const struct var_info *b = _b; - - if (a->addr == b->addr) - return 0; - return a->addr < b->addr ? -1 : 1; -} - -static bool btf_encoder__percpu_var_exists(struct btf_encoder *encoder, uint64_t addr, uint32_t *sz, const char **name) -{ - struct var_info key = { .addr = addr }; - const struct var_info *p = bsearch(&key, encoder->percpu.vars, encoder->percpu.var_cnt, - sizeof(encoder->percpu.vars[0]), percpu_var_cmp); - if (!p) - return false; - - *sz = p->sz; - *name = p->name; - return true; -} - -static int btf_encoder__collect_percpu_var(struct btf_encoder *encoder, GElf_Sym *sym, size_t 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 (sym_sec_idx != encoder->percpu.shndx) - return 0; - if (elf_sym__type(sym) != STT_OBJECT) - return 0; - - addr = elf_sym__value(sym); - - size = elf_sym__size(sym); - if (!size) - return 0; /* ignore zero-sized symbols */ - - sym_name = elf_sym__name(sym, encoder->symtab); - if (!btf_name_valid(sym_name)) { - dump_invalid_symbol("Found symbol of invalid name when encoding btf", - sym_name, encoder->verbose, encoder->force); - if (encoder->force) - return 0; - return -1; - } - - if (encoder->verbose) - printf("Found per-CPU symbol '%s' at address 0x%" PRIx64 "\n", sym_name, addr); - - /* Make sure addr is section-relative. For kernel modules (which are - * ET_REL files) this is already the case. For vmlinux (which is an - * ET_EXEC file) we need to subtract the section address. - */ - if (!encoder->is_rel) - addr -= encoder->secinfo[encoder->percpu.shndx].addr; - - if (encoder->percpu.var_cnt == encoder->percpu.allocated) { - struct var_info *new; - - new = reallocarray_grow(encoder->percpu.vars, - &encoder->percpu.allocated, - sizeof(*encoder->percpu.vars)); - if (!new) { - fprintf(stderr, "Failed to allocate memory for variables\n"); - return -1; - } - encoder->percpu.vars = new; - } - encoder->percpu.vars[encoder->percpu.var_cnt].addr = addr; - encoder->percpu.vars[encoder->percpu.var_cnt].sz = size; - encoder->percpu.vars[encoder->percpu.var_cnt].name = sym_name; - encoder->percpu.var_cnt++; - - return 0; -} -static int btf_encoder__collect_symbols(struct btf_encoder *encoder, bool collect_percpu_vars) +static int btf_encoder__collect_symbols(struct btf_encoder *encoder) { - Elf32_Word sym_sec_idx; + uint32_t sym_sec_idx; uint32_t core_id; GElf_Sym sym; - /* cache variables' addresses, preparing for searching in symtab. */ - encoder->percpu.var_cnt = 0; - - /* search within symtab for percpu variables */ elf_symtab__for_each_symbol_index(encoder->symtab, core_id, sym, sym_sec_idx) { - if (collect_percpu_vars && btf_encoder__collect_percpu_var(encoder, &sym, sym_sec_idx)) - return -1; if (btf_encoder__collect_function(encoder, &sym)) return -1; } - if (collect_percpu_vars) { - if (encoder->percpu.var_cnt) - qsort(encoder->percpu.vars, encoder->percpu.var_cnt, sizeof(encoder->percpu.vars[0]), percpu_var_cmp); - - if (encoder->verbose) - printf("Found %d per-CPU variables!\n", encoder->percpu.var_cnt); - } - if (encoder->functions.cnt) { qsort(encoder->functions.entries, encoder->functions.cnt, sizeof(encoder->functions.entries[0]), functions_cmp); @@ -2224,15 +2120,54 @@ static bool ftype__has_arg_names(const struct ftype *ftype) return true; } +static int get_elf_section(struct btf_encoder *encoder, unsigned long addr) +{ + /* Start at index 1 to ignore initial SHT_NULL section */ + for (int i = 1; i < encoder->seccnt; i++) + /* Variables are only present in PROGBITS or NOBITS (.bss) */ + if ((encoder->secinfo[i].type == SHT_PROGBITS || + encoder->secinfo[i].type == SHT_NOBITS) && + encoder->secinfo[i].addr <= addr && + (addr - encoder->secinfo[i].addr) < encoder->secinfo[i].sz) + return i; + return -ENOENT; +} + +/* + * Filter out variables / symbol names with common prefixes and no useful + * values. Prefixes should be added sparingly, and it should be objectively + * obvious that they are not useful. + */ +static bool filter_variable_name(const char *name) +{ + static const struct { char *s; size_t len; } skip[] = { + #define X(str) {str, sizeof(str) - 1} + X("__UNIQUE_ID"), + X("__tpstrtab_"), + X("__exitcall_"), + X("__func_stack_frame_non_standard_") + #undef X + }; + int i; + + if (*name != '_') + return false; + + for (i = 0; i < ARRAY_SIZE(skip); i++) { + if (strncmp(name, skip[i].s, skip[i].len) == 0) + return true; + } + return false; +} + static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder) { struct cu *cu = encoder->cu; uint32_t core_id; struct tag *pos; int err = -1; - struct elf_secinfo *pcpu_scn = &encoder->secinfo[encoder->percpu.shndx]; - if (encoder->percpu.shndx == 0 || !encoder->symtab) + if (encoder->percpu_shndx == 0 || !encoder->symtab) return 0; if (encoder->verbose) @@ -2240,59 +2175,69 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder) cu__for_each_variable(cu, core_id, pos) { struct variable *var = tag__variable(pos); - uint32_t size, type, linkage; - const char *name, *dwarf_name; + uint32_t type, linkage; + const char *name; struct llvm_annotation *annot; const struct tag *tag; + size_t shndx, size; uint64_t addr; int id; + /* Skip incomplete (non-defining) declarations */ if (var->declaration && !var->spec) continue; - /* percpu variables are allocated in global space */ - if (variable__scope(var) != VSCOPE_GLOBAL && !var->spec) + /* + * top_level: indicates that the variable is declared at the top + * level of the CU, and thus it is globally scoped. + * artificial: indicates that the variable is a compiler-generated + * "fake" variable that doesn't appear in the source. + * scope: set by pahole to indicate the type of storage the + * variable has. GLOBAL indicates it is stored in static + * memory (as opposed to a stack variable or register) + * + * Some variables are "top_level" but not GLOBAL: + * e.g. current_stack_pointer, which is a register variable, + * despite having global CU-declarations. We don't want that, + * since no code could actually find this variable. + * Some variables are GLOBAL but not top_level: + * e.g. function static variables + */ + if (!var->top_level || var->artificial || var->scope != VSCOPE_GLOBAL) continue; /* addr has to be recorded before we follow spec */ addr = var->ip.addr; - dwarf_name = variable__name(var); - /* Make sure addr is section-relative. DWARF, unlike ELF, - * always contains virtual symbol addresses, so subtract - * the section address unconditionally. - */ - if (addr < pcpu_scn->addr || addr >= pcpu_scn->addr + pcpu_scn->sz) + /* Get the ELF section info for the variable */ + shndx = get_elf_section(encoder, addr); + if (shndx != encoder->percpu_shndx) continue; - addr -= pcpu_scn->addr; - if (!btf_encoder__percpu_var_exists(encoder, addr, &size, &name)) - continue; /* not a per-CPU variable */ + /* Convert addr to section relative */ + addr -= encoder->secinfo[shndx].addr; - /* A lot of "special" DWARF variables (e.g, __UNIQUE_ID___xxx) - * have addr == 0, which is the same as, say, valid - * fixed_percpu_data per-CPU variable. To distinguish between - * them, additionally compare DWARF and ELF symbol names. If - * DWARF doesn't provide proper name, pessimistically assume - * bad variable. - * - * Examples of such special variables are: - * - * 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. - * - * This is relevant only for vmlinux image, as for kernel - * modules per-CPU data section has non-zero offset so all - * per-CPU symbols have non-zero values. - */ - if (var->ip.addr == 0) { - if (!dwarf_name || strcmp(dwarf_name, name)) + /* DWARF specification reference should be followed, because + * information like the name & type may not be present on var */ + if (var->spec) + var = var->spec; + + name = variable__name(var); + if (!name) + continue; + + /* Check for invalid BTF names */ + if (!btf_name_valid(name)) { + dump_invalid_symbol("Found invalid variable name when encoding btf", + name, encoder->verbose, encoder->force); + if (encoder->force) continue; + else + return -1; } - if (var->spec) - var = var->spec; + if (filter_variable_name(name)) + continue; if (var->ip.tag.type == 0) { fprintf(stderr, "error: found variable '%s' in CU '%s' that has void type\n", @@ -2304,9 +2249,10 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder) } tag = cu__type(cu, var->ip.tag.type); - if (tag__size(tag, cu) == 0) { + size = tag__size(tag, cu); + if (size == 0) { if (encoder->verbose) - fprintf(stderr, "Ignoring zero-sized per-CPU variable '%s'...\n", dwarf_name ?: "<missing name>"); + fprintf(stderr, "Ignoring zero-sized per-CPU variable '%s'...\n", name); continue; } @@ -2388,8 +2334,6 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam goto out_delete; } - encoder->is_rel = ehdr.e_type == ET_REL; - switch (ehdr.e_ident[EI_DATA]) { case ELFDATA2LSB: btf__set_endianness(encoder->btf, BTF_LITTLE_ENDIAN); @@ -2430,15 +2374,16 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam encoder->secinfo[shndx].addr = shdr.sh_addr; encoder->secinfo[shndx].sz = shdr.sh_size; encoder->secinfo[shndx].name = secname; + encoder->secinfo[shndx].type = shdr.sh_type; if (strcmp(secname, PERCPU_SECTION) == 0) - encoder->percpu.shndx = shndx; + encoder->percpu_shndx = shndx; } - if (!encoder->percpu.shndx && encoder->verbose) + if (!encoder->percpu_shndx && encoder->verbose) printf("%s: '%s' doesn't have '%s' section\n", __func__, cu->filename, PERCPU_SECTION); - if (btf_encoder__collect_symbols(encoder, encoder->encode_vars & BTF_VAR_PERCPU)) + if (btf_encoder__collect_symbols(encoder)) goto out_delete; if (encoder->verbose) @@ -2480,9 +2425,6 @@ void btf_encoder__delete(struct btf_encoder *encoder) encoder->functions.allocated = encoder->functions.cnt = 0; free(encoder->functions.entries); encoder->functions.entries = NULL; - encoder->percpu.allocated = encoder->percpu.var_cnt = 0; - free(encoder->percpu.vars); - encoder->percpu.vars = NULL; free(encoder); }
Currently we index symbols from the percpu ELF section, and when processing DWARF variables for inclusion, we check whether the variable matches an existing symbol. The matched symbol is used for three purposes: 1. When no symbol of the same address is found, the variable is skipped. This can occur because the symbol name was an invalid BTF identifier, and so it did not get indexed. Or more commonly, it can be because the variable is not stored in the per-cpu section, and thus was not indexed. 2. If the symbol offset is 0, then we compare the DWARF variable's name against the symbol name to filter out "special" DWARF variables. 3. We use the symbol size in the DATASEC entry for the variable. For 1, we don't need the symbol table: we can simply check the DWARF variable name directly, and we can use the variable address to determine the ELF section it is contained in. For 3, we also don't need the symbol table: we can use the variable's size information from DWARF. Issue 2 is more complicated, but thanks to the addition of the "artificial" and "top_level" flags, many of the "special" DWARF variables can be directly filtered out, and the few remaining problematic variables can be filtered by name from a kernel-specific list of patterns. This allows the symbol table index to be removed. The benefit of removing this index is twofold. First, handling variable addresses is simplified, since we don't need to know whether the file is ET_REL. Second, this will make it easier to output variables that aren't just percpu, since we won't need to index variables from all ELF sections. Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com> --- btf_encoder.c | 250 +++++++++++++++++++------------------------------- 1 file changed, 96 insertions(+), 154 deletions(-)