* [PATCH dwarves v3 0/5] Emit global variables in BTF
@ 2024-10-02 23:52 Stephen Brennan
2024-10-02 23:52 ` [PATCH dwarves v3 1/5] btf_encoder: use bitfield to control var encoding Stephen Brennan
` (5 more replies)
0 siblings, 6 replies; 20+ messages in thread
From: Stephen Brennan @ 2024-10-02 23:52 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: bpf, dwarves, linux-debuggers, Stephen Brennan, Alan Maguire
Hello all,
This is v3 of the series to add global variables to pahole's generated BTF.
Patches 1-3 of v2 were already merged. This series splits the last patch of v2
and does some small updates. It should apply cleanly to the "next" branch.
https://github.com/acmel/dwarves/commits/btf_global_vars/
Changes since v2:
1. Split things out into several smaller patches as can be seen in the log
below.
2. Previously the global_var feature was defined with BTF_DEFAULT_FEATURE, but I
think we agreed in the discussion of v2 that it would be better as
BTF_NON_DEFAULT_FEATURE, so I changed it to align with our discussion.
3. Removed the "--encode_btf_global_vars" option.
3. I went through and straightened out my use of integer types for ELF section
index (size_t, as returned by libelf) as well as the variable addr and size.
To this end I did add a few checks to explicitly ensure we don't overflow the
uint32_t fields in the DATASEC.
To test this out on a Linux build, you'll want to make the following change:
---------
diff --git a/scripts/Makefile.btf b/scripts/Makefile.btf
index b75f09f3f424..c88d9e526426 100644
--- a/scripts/Makefile.btf
+++ b/scripts/Makefile.btf
@@ -19,7 +19,7 @@ pahole-flags-$(call test-ge, $(pahole-ver), 125) += --skip_encoding_btf_inconsis
else
# Switch to using --btf_features for v1.26 and later.
-pahole-flags-$(call test-ge, $(pahole-ver), 126) = -j --btf_features=encode_force,var,float,enum64,decl_tag,type_tag,optimized_func,consistent_func,decl_tag_kfuncs
+pahole-flags-$(call test-ge, $(pahole-ver), 126) = -j --btf_features=encode_force,var,float,enum64,decl_tag,type_tag,optimized_func,consistent_func,decl_tag_kfuncs,global_var
ifneq ($(KBUILD_EXTMOD),)
module-pahole-flags-$(call test-ge, $(pahole-ver), 126) += --btf_features=distilled_base
---------
With a suitable kernel config that has BTF enabled, you could then build like
so:
PATH=path/to/pahole_build_dir make all
And you'll be able to examine the size of the results with readelf, or dump the
results with bpftool.
v2: https://lore.kernel.org/dwarves/20240920081903.13473-1-stephen.s.brennan@oracle.com/T/
v1: https://lore.kernel.org/dwarves/20240912190827.230176-1-stephen.s.brennan@oracle.com/
Stephen Brennan (5):
btf_encoder: use bitfield to control var encoding
btf_encoder: stop indexing symbols for VARs
btf_encoder: explicitly check addr/size for u32 overflow
btf_encoder: allow encoding VARs from many sections
pahole: add global_var BTF feature
btf_encoder.c | 348 +++++++++++++++++++++------------------------
btf_encoder.h | 7 +
dwarves.h | 1 +
man-pages/pahole.1 | 7 +-
pahole.c | 3 +-
5 files changed, 178 insertions(+), 188 deletions(-)
--
2.43.5
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH dwarves v3 1/5] btf_encoder: use bitfield to control var encoding
2024-10-02 23:52 [PATCH dwarves v3 0/5] Emit global variables in BTF Stephen Brennan
@ 2024-10-02 23:52 ` Stephen Brennan
2024-10-03 13:41 ` Alan Maguire
2024-10-02 23:52 ` [PATCH dwarves v3 2/5] btf_encoder: stop indexing symbols for VARs Stephen Brennan
` (4 subsequent siblings)
5 siblings, 1 reply; 20+ messages in thread
From: Stephen Brennan @ 2024-10-02 23:52 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: bpf, dwarves, linux-debuggers, Stephen Brennan, Alan Maguire
We will need more granularity in the future, in order to add support for
encoding global variables as well. So replace the skip_encoding_vars
boolean with a flag variable named "encode_vars". There is currently
only one bit specified, and it is set when percpu variables should be
emitted.
Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
---
btf_encoder.c | 10 ++++++----
btf_encoder.h | 6 ++++++
2 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/btf_encoder.c b/btf_encoder.c
index 51cd7bf..652a945 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -119,7 +119,6 @@ struct btf_encoder {
uint32_t type_id_off;
bool has_index_type,
need_index_type,
- skip_encoding_vars,
raw_output,
verbose,
force,
@@ -137,6 +136,7 @@ struct btf_encoder {
int allocated;
uint32_t shndx;
} percpu;
+ int encode_vars;
struct {
struct elf_function *entries;
int allocated;
@@ -2369,7 +2369,6 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
encoder->force = conf_load->btf_encode_force;
encoder->gen_floats = conf_load->btf_gen_floats;
- encoder->skip_encoding_vars = conf_load->skip_encoding_btf_vars;
encoder->skip_encoding_decl_tag = conf_load->skip_encoding_btf_decl_tag;
encoder->tag_kfuncs = conf_load->btf_decl_tag_kfuncs;
encoder->gen_distilled_base = conf_load->btf_gen_distilled_base;
@@ -2377,6 +2376,9 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
encoder->has_index_type = false;
encoder->need_index_type = false;
encoder->array_index_id = 0;
+ encoder->encode_vars = 0;
+ if (!conf_load->skip_encoding_btf_vars)
+ encoder->encode_vars |= BTF_VAR_PERCPU;
GElf_Ehdr ehdr;
@@ -2436,7 +2438,7 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
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->skip_encoding_vars))
+ if (btf_encoder__collect_symbols(encoder, encoder->encode_vars & BTF_VAR_PERCPU))
goto out_delete;
if (encoder->verbose)
@@ -2633,7 +2635,7 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
goto out;
}
- if (!encoder->skip_encoding_vars)
+ if (encoder->encode_vars)
err = btf_encoder__encode_cu_variables(encoder);
if (!err)
diff --git a/btf_encoder.h b/btf_encoder.h
index f54c95a..91e7947 100644
--- a/btf_encoder.h
+++ b/btf_encoder.h
@@ -16,6 +16,12 @@ struct btf;
struct cu;
struct list_head;
+/* Bit flags specifying which kinds of variables are emitted */
+enum btf_var_option {
+ BTF_VAR_NONE = 0,
+ BTF_VAR_PERCPU = 1,
+};
+
struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filename, struct btf *base_btf, bool verbose, struct conf_load *conf_load);
void btf_encoder__delete(struct btf_encoder *encoder);
--
2.43.5
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH dwarves v3 2/5] btf_encoder: stop indexing symbols for VARs
2024-10-02 23:52 [PATCH dwarves v3 0/5] Emit global variables in BTF Stephen Brennan
2024-10-02 23:52 ` [PATCH dwarves v3 1/5] btf_encoder: use bitfield to control var encoding Stephen Brennan
@ 2024-10-02 23:52 ` Stephen Brennan
2024-10-03 13:59 ` Alan Maguire
2024-10-02 23:52 ` [PATCH dwarves v3 3/5] btf_encoder: explicitly check addr/size for u32 overflow Stephen Brennan
` (3 subsequent siblings)
5 siblings, 1 reply; 20+ messages in thread
From: Stephen Brennan @ 2024-10-02 23:52 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: bpf, dwarves, linux-debuggers, Stephen Brennan, Alan Maguire
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(-)
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);
}
--
2.43.5
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH dwarves v3 3/5] btf_encoder: explicitly check addr/size for u32 overflow
2024-10-02 23:52 [PATCH dwarves v3 0/5] Emit global variables in BTF Stephen Brennan
2024-10-02 23:52 ` [PATCH dwarves v3 1/5] btf_encoder: use bitfield to control var encoding Stephen Brennan
2024-10-02 23:52 ` [PATCH dwarves v3 2/5] btf_encoder: stop indexing symbols for VARs Stephen Brennan
@ 2024-10-02 23:52 ` Stephen Brennan
2024-10-03 14:19 ` Alan Maguire
2024-10-02 23:52 ` [PATCH dwarves v3 4/5] btf_encoder: allow encoding VARs from many sections Stephen Brennan
` (2 subsequent siblings)
5 siblings, 1 reply; 20+ messages in thread
From: Stephen Brennan @ 2024-10-02 23:52 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: bpf, dwarves, linux-debuggers, Stephen Brennan, Alan Maguire
The addr is a uint64_t, and depending on the size of a data section,
there's no guarantee that it fits into a uint32_t, even after
subtracting out the section start address. Similarly, the variable size
is a size_t which could exceed a uint32_t. Check both for overflow, and
if found, skip the variable with an error message. Use explicit casts
when we cast to uint32_t so it's plain to see that this is happening.
Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
---
btf_encoder.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/btf_encoder.c b/btf_encoder.c
index 31a418a..1872e00 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -2250,9 +2250,16 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder)
tag = cu__type(cu, var->ip.tag.type);
size = tag__size(tag, cu);
- if (size == 0) {
+ if (size == 0 || size > UINT32_MAX) {
if (encoder->verbose)
- fprintf(stderr, "Ignoring zero-sized per-CPU variable '%s'...\n", name);
+ fprintf(stderr, "Ignoring %s-sized per-CPU variable '%s'...\n",
+ size == 0 ? "zero" : "over", name);
+ continue;
+ }
+ if (addr > UINT32_MAX) {
+ if (encoder->verbose)
+ fprintf(stderr, "Ignoring variable '%s' - its offset %zu doesn't fit in a u32\n",
+ name, addr);
continue;
}
@@ -2285,7 +2292,7 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder)
* add a BTF_VAR_SECINFO in encoder->percpu_secinfo, which will be added into
* encoder->types later when we add BTF_VAR_DATASEC.
*/
- id = btf_encoder__add_var_secinfo(encoder, id, addr, size);
+ id = btf_encoder__add_var_secinfo(encoder, id, (uint32_t)addr, (uint32_t)size);
if (id < 0) {
fprintf(stderr, "error: failed to encode section info for variable '%s' at addr 0x%" PRIx64 "\n",
name, addr);
--
2.43.5
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH dwarves v3 4/5] btf_encoder: allow encoding VARs from many sections
2024-10-02 23:52 [PATCH dwarves v3 0/5] Emit global variables in BTF Stephen Brennan
` (2 preceding siblings ...)
2024-10-02 23:52 ` [PATCH dwarves v3 3/5] btf_encoder: explicitly check addr/size for u32 overflow Stephen Brennan
@ 2024-10-02 23:52 ` Stephen Brennan
2024-10-03 14:52 ` Alan Maguire
2024-10-02 23:52 ` [PATCH dwarves v3 5/5] pahole: add global_var BTF feature Stephen Brennan
2024-10-03 20:59 ` [PATCH dwarves v3 0/5] Emit global variables in BTF Jiri Olsa
5 siblings, 1 reply; 20+ messages in thread
From: Stephen Brennan @ 2024-10-02 23:52 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: bpf, dwarves, linux-debuggers, Stephen Brennan, Alan Maguire
Currently we maintain one buffer of DATASEC entries that describe the
offsets for variables in the percpu ELF section. In order to make it
possible to output all global variables, we'll need to output a DATASEC
for each ELF section containing variables, and we'll need to control
whether or not to encode variables on a per-section basis.
With this change, the ability to emit VARs from multiple sections is
technically present, but not enabled, so pahole still only emits percpu
variables. A subsequent change will enable emitting all global
variables.
Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
---
btf_encoder.c | 90 ++++++++++++++++++++++++++++++++-------------------
1 file changed, 56 insertions(+), 34 deletions(-)
diff --git a/btf_encoder.c b/btf_encoder.c
index 1872e00..2fd1648 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -98,6 +98,8 @@ struct elf_secinfo {
const char *name;
uint64_t sz;
uint32_t type;
+ bool include;
+ struct gobuffer secinfo;
};
/*
@@ -107,7 +109,6 @@ struct btf_encoder {
struct list_head node;
struct btf *btf;
struct cu *cu;
- struct gobuffer percpu_secinfo;
const char *source_filename;
const char *filename;
struct elf_symtab *symtab;
@@ -124,7 +125,6 @@ struct btf_encoder {
uint32_t array_index_id;
struct elf_secinfo *secinfo;
size_t seccnt;
- size_t percpu_shndx;
int encode_vars;
struct {
struct elf_function *entries;
@@ -784,46 +784,56 @@ static int32_t btf_encoder__add_var(struct btf_encoder *encoder, uint32_t type,
return id;
}
-static int32_t btf_encoder__add_var_secinfo(struct btf_encoder *encoder, uint32_t type,
- uint32_t offset, uint32_t size)
+static int32_t btf_encoder__add_var_secinfo(struct btf_encoder *encoder, size_t shndx,
+ uint32_t type, uint32_t offset, uint32_t size)
{
struct btf_var_secinfo si = {
.type = type,
.offset = offset,
.size = size,
};
- return gobuffer__add(&encoder->percpu_secinfo, &si, sizeof(si));
+ return gobuffer__add(&encoder->secinfo[shndx].secinfo, &si, sizeof(si));
}
int32_t btf_encoder__add_encoder(struct btf_encoder *encoder, struct btf_encoder *other)
{
- struct gobuffer *var_secinfo_buf = &other->percpu_secinfo;
- size_t sz = gobuffer__size(var_secinfo_buf);
- uint16_t nr_var_secinfo = sz / sizeof(struct btf_var_secinfo);
- uint32_t type_id;
- uint32_t next_type_id = btf__type_cnt(encoder->btf);
- int32_t i, id;
- struct btf_var_secinfo *vsi;
-
+ size_t shndx;
if (encoder == other)
return 0;
btf_encoder__add_saved_funcs(other);
- for (i = 0; i < nr_var_secinfo; i++) {
- vsi = (struct btf_var_secinfo *)var_secinfo_buf->entries + i;
- type_id = next_type_id + vsi->type - 1; /* Type ID starts from 1 */
- id = btf_encoder__add_var_secinfo(encoder, type_id, vsi->offset, vsi->size);
- if (id < 0)
- return id;
+ for (shndx = 0; shndx < other->seccnt; shndx++) {
+ struct gobuffer *var_secinfo_buf = &other->secinfo[shndx].secinfo;
+ size_t sz = gobuffer__size(var_secinfo_buf);
+ uint16_t nr_var_secinfo = sz / sizeof(struct btf_var_secinfo);
+ uint32_t type_id;
+ uint32_t next_type_id = btf__type_cnt(encoder->btf);
+ int32_t i, id;
+ struct btf_var_secinfo *vsi;
+
+ if (strcmp(encoder->secinfo[shndx].name, other->secinfo[shndx].name)) {
+ fprintf(stderr, "mismatched ELF sections at index %zu: \"%s\", \"%s\"\n",
+ shndx, encoder->secinfo[shndx].name, other->secinfo[shndx].name);
+ return -1;
+ }
+
+ for (i = 0; i < nr_var_secinfo; i++) {
+ vsi = (struct btf_var_secinfo *)var_secinfo_buf->entries + i;
+ type_id = next_type_id + vsi->type - 1; /* Type ID starts from 1 */
+ id = btf_encoder__add_var_secinfo(encoder, shndx, type_id, vsi->offset, vsi->size);
+ if (id < 0)
+ return id;
+ }
}
return btf__add_btf(encoder->btf, other->btf);
}
-static int32_t btf_encoder__add_datasec(struct btf_encoder *encoder, const char *section_name)
+static int32_t btf_encoder__add_datasec(struct btf_encoder *encoder, size_t shndx)
{
- struct gobuffer *var_secinfo_buf = &encoder->percpu_secinfo;
+ struct gobuffer *var_secinfo_buf = &encoder->secinfo[shndx].secinfo;
+ const char *section_name = encoder->secinfo[shndx].name;
struct btf *btf = encoder->btf;
size_t sz = gobuffer__size(var_secinfo_buf);
uint16_t nr_var_secinfo = sz / sizeof(struct btf_var_secinfo);
@@ -2032,12 +2042,14 @@ int btf_encoder__encode(struct btf_encoder *encoder)
{
bool should_tag_kfuncs;
int err;
+ size_t shndx;
/* for single-threaded case, saved funcs are added here */
btf_encoder__add_saved_funcs(encoder);
- if (gobuffer__size(&encoder->percpu_secinfo) != 0)
- btf_encoder__add_datasec(encoder, PERCPU_SECTION);
+ for (shndx = 0; shndx < encoder->seccnt; shndx++)
+ if (gobuffer__size(&encoder->secinfo[shndx].secinfo))
+ btf_encoder__add_datasec(encoder, shndx);
/* Empty file, nothing to do, so... done! */
if (btf__type_cnt(encoder->btf) == 1)
@@ -2167,7 +2179,7 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder)
struct tag *pos;
int err = -1;
- if (encoder->percpu_shndx == 0 || !encoder->symtab)
+ if (!encoder->symtab)
return 0;
if (encoder->verbose)
@@ -2180,6 +2192,7 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder)
struct llvm_annotation *annot;
const struct tag *tag;
size_t shndx, size;
+ struct elf_secinfo *sec = NULL;
uint64_t addr;
int id;
@@ -2211,7 +2224,9 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder)
/* Get the ELF section info for the variable */
shndx = get_elf_section(encoder, addr);
- if (shndx != encoder->percpu_shndx)
+ if (shndx >= 0 && shndx < encoder->seccnt)
+ sec = &encoder->secinfo[shndx];
+ if (!sec || !sec->include)
continue;
/* Convert addr to section relative */
@@ -2252,7 +2267,7 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder)
size = tag__size(tag, cu);
if (size == 0 || size > UINT32_MAX) {
if (encoder->verbose)
- fprintf(stderr, "Ignoring %s-sized per-CPU variable '%s'...\n",
+ fprintf(stderr, "Ignoring %s-sized variable '%s'...\n",
size == 0 ? "zero" : "over", name);
continue;
}
@@ -2289,13 +2304,14 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder)
}
/*
- * add a BTF_VAR_SECINFO in encoder->percpu_secinfo, which will be added into
- * encoder->types later when we add BTF_VAR_DATASEC.
+ * Add the variable to the secinfo for the section it appears in.
+ * Later we will generate a BTF_VAR_DATASEC for all any section with
+ * an encoded variable.
*/
- id = btf_encoder__add_var_secinfo(encoder, id, (uint32_t)addr, (uint32_t)size);
+ id = btf_encoder__add_var_secinfo(encoder, shndx, id, (uint32_t)addr, (uint32_t)size);
if (id < 0) {
fprintf(stderr, "error: failed to encode section info for variable '%s' at addr 0x%" PRIx64 "\n",
- name, addr);
+ name, addr);
goto out;
}
}
@@ -2373,6 +2389,7 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
goto out_delete;
}
+ bool found_percpu = false;
for (shndx = 0; shndx < encoder->seccnt; shndx++) {
const char *secname = NULL;
Elf_Scn *sec = elf_section_by_idx(cu->elf, &shdr, shndx, &secname);
@@ -2383,11 +2400,14 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
encoder->secinfo[shndx].name = secname;
encoder->secinfo[shndx].type = shdr.sh_type;
- if (strcmp(secname, PERCPU_SECTION) == 0)
- encoder->percpu_shndx = shndx;
+ if (strcmp(secname, PERCPU_SECTION) == 0) {
+ found_percpu = true;
+ if (encoder->encode_vars & BTF_VAR_PERCPU)
+ encoder->secinfo[shndx].include = true;
+ }
}
- if (!encoder->percpu_shndx && encoder->verbose)
+ if (!found_percpu && encoder->verbose)
printf("%s: '%s' doesn't have '%s' section\n", __func__, cu->filename, PERCPU_SECTION);
if (btf_encoder__collect_symbols(encoder))
@@ -2415,12 +2435,14 @@ void btf_encoder__delete_func(struct elf_function *func)
void btf_encoder__delete(struct btf_encoder *encoder)
{
int i;
+ size_t shndx;
if (encoder == NULL)
return;
btf_encoders__delete(encoder);
- __gobuffer__delete(&encoder->percpu_secinfo);
+ for (shndx = 0; shndx < encoder->seccnt; shndx++)
+ __gobuffer__delete(&encoder->secinfo[shndx].secinfo);
zfree(&encoder->filename);
zfree(&encoder->source_filename);
btf__free(encoder->btf);
--
2.43.5
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH dwarves v3 5/5] pahole: add global_var BTF feature
2024-10-02 23:52 [PATCH dwarves v3 0/5] Emit global variables in BTF Stephen Brennan
` (3 preceding siblings ...)
2024-10-02 23:52 ` [PATCH dwarves v3 4/5] btf_encoder: allow encoding VARs from many sections Stephen Brennan
@ 2024-10-02 23:52 ` Stephen Brennan
2024-10-03 14:40 ` Alan Maguire
2024-10-03 20:59 ` [PATCH dwarves v3 0/5] Emit global variables in BTF Jiri Olsa
5 siblings, 1 reply; 20+ messages in thread
From: Stephen Brennan @ 2024-10-02 23:52 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: bpf, dwarves, linux-debuggers, Stephen Brennan, Alan Maguire
So far, pahole has only encoded type information for percpu variables.
However, there are several reasons why type information for all global
variables would be useful in the kernel:
1. Runtime kernel debuggers like drgn could use the BTF to introspect
kernel data structures without needing to install heavyweight DWARF.
2. BPF programs using the "__ksym" annotation could declare the
variables using the correct type, rather than "void".
It makes sense to introduce a feature for this in pahole so that these
capabilities can be explored in the kernel. The feature is non-default:
when using "--btf-features=default", it is disabled. It must be
explicitly requested, e.g. with "--btf-features=+global_var".
Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
---
btf_encoder.c | 5 +++++
btf_encoder.h | 1 +
dwarves.h | 1 +
man-pages/pahole.1 | 7 +++++--
pahole.c | 3 ++-
5 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/btf_encoder.c b/btf_encoder.c
index 2fd1648..2730ea8 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -2348,6 +2348,8 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
encoder->encode_vars = 0;
if (!conf_load->skip_encoding_btf_vars)
encoder->encode_vars |= BTF_VAR_PERCPU;
+ if (conf_load->encode_btf_global_vars)
+ encoder->encode_vars |= BTF_VAR_GLOBAL;
GElf_Ehdr ehdr;
@@ -2400,6 +2402,9 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
encoder->secinfo[shndx].name = secname;
encoder->secinfo[shndx].type = shdr.sh_type;
+ if (encoder->encode_vars & BTF_VAR_GLOBAL)
+ encoder->secinfo[shndx].include = true;
+
if (strcmp(secname, PERCPU_SECTION) == 0) {
found_percpu = true;
if (encoder->encode_vars & BTF_VAR_PERCPU)
diff --git a/btf_encoder.h b/btf_encoder.h
index 91e7947..824963b 100644
--- a/btf_encoder.h
+++ b/btf_encoder.h
@@ -20,6 +20,7 @@ struct list_head;
enum btf_var_option {
BTF_VAR_NONE = 0,
BTF_VAR_PERCPU = 1,
+ BTF_VAR_GLOBAL = 2,
};
struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filename, struct btf *base_btf, bool verbose, struct conf_load *conf_load);
diff --git a/dwarves.h b/dwarves.h
index 0fede91..fef881f 100644
--- a/dwarves.h
+++ b/dwarves.h
@@ -92,6 +92,7 @@ struct conf_load {
bool btf_gen_optimized;
bool skip_encoding_btf_inconsistent_proto;
bool skip_encoding_btf_vars;
+ bool encode_btf_global_vars;
bool btf_gen_floats;
bool btf_encode_force;
bool reproducible_build;
diff --git a/man-pages/pahole.1 b/man-pages/pahole.1
index b3e6632..7c1a69a 100644
--- a/man-pages/pahole.1
+++ b/man-pages/pahole.1
@@ -238,7 +238,9 @@ the debugging information.
.TP
.B \-\-skip_encoding_btf_vars
-Do not encode VARs in BTF.
+By default, VARs are encoded only for percpu variables. When specified, this
+option prevents encoding any VARs. Note that this option can be overridden
+by the feature "global_var".
.TP
.B \-\-skip_encoding_btf_decl_tag
@@ -304,7 +306,7 @@ Encode BTF using the specified feature list, or specify 'default' for all standa
encode_force Ignore invalid symbols when encoding BTF; for example
if a symbol has an invalid name, it will be ignored
and BTF encoding will continue.
- var Encode variables using BTF_KIND_VAR in BTF.
+ var Encode percpu variables using BTF_KIND_VAR in BTF.
float Encode floating-point types in BTF.
decl_tag Encode declaration tags using BTF_KIND_DECL_TAG.
type_tag Encode type tags using BTF_KIND_TYPE_TAG.
@@ -329,6 +331,7 @@ Supported non-standard features (not enabled for 'default')
the associated base BTF to support later relocation
of split BTF with a possibly changed base, storing
it in a .BTF.base ELF section.
+ global_var Encode all global variables using BTF_KIND_VAR in BTF.
.fi
So for example, specifying \-\-btf_encode=var,enum64 will result in a BTF encoding that (as well as encoding basic BTF information) will contain variables and enum64 values.
diff --git a/pahole.c b/pahole.c
index b21a7f2..9f0dc59 100644
--- a/pahole.c
+++ b/pahole.c
@@ -1301,6 +1301,7 @@ struct btf_feature {
BTF_DEFAULT_FEATURE(decl_tag_kfuncs, btf_decl_tag_kfuncs, false),
BTF_NON_DEFAULT_FEATURE(reproducible_build, reproducible_build, false),
BTF_NON_DEFAULT_FEATURE(distilled_base, btf_gen_distilled_base, false),
+ BTF_NON_DEFAULT_FEATURE(global_var, encode_btf_global_vars, false),
};
#define BTF_MAX_FEATURE_STR 1024
@@ -1733,7 +1734,7 @@ static const struct argp_option pahole__options[] = {
{
.name = "skip_encoding_btf_vars",
.key = ARGP_skip_encoding_btf_vars,
- .doc = "Do not encode VARs in BTF."
+ .doc = "Do not encode any VARs in BTF [if this is not specified, only percpu variables are encoded. To encode global variables too, use --encode_btf_global_vars]."
},
{
.name = "btf_encode_force",
--
2.43.5
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH dwarves v3 1/5] btf_encoder: use bitfield to control var encoding
2024-10-02 23:52 ` [PATCH dwarves v3 1/5] btf_encoder: use bitfield to control var encoding Stephen Brennan
@ 2024-10-03 13:41 ` Alan Maguire
2024-10-03 14:41 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 20+ messages in thread
From: Alan Maguire @ 2024-10-03 13:41 UTC (permalink / raw)
To: Stephen Brennan, Arnaldo Carvalho de Melo; +Cc: bpf, dwarves, linux-debuggers
On 03/10/2024 00:52, Stephen Brennan wrote:
> We will need more granularity in the future, in order to add support for
> encoding global variables as well. So replace the skip_encoding_vars
> boolean with a flag variable named "encode_vars". There is currently
> only one bit specified, and it is set when percpu variables should be
> emitted.
>
> Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
> ---
> btf_encoder.c | 10 ++++++----
> btf_encoder.h | 6 ++++++
> 2 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/btf_encoder.c b/btf_encoder.c
> index 51cd7bf..652a945 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -119,7 +119,6 @@ struct btf_encoder {
> uint32_t type_id_off;
> bool has_index_type,
> need_index_type,
> - skip_encoding_vars,
> raw_output,
> verbose,
> force,
> @@ -137,6 +136,7 @@ struct btf_encoder {
> int allocated;
> uint32_t shndx;
> } percpu;
> + int encode_vars;
> struct {
> struct elf_function *entries;
> int allocated;
> @@ -2369,7 +2369,6 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
>
> encoder->force = conf_load->btf_encode_force;
> encoder->gen_floats = conf_load->btf_gen_floats;
> - encoder->skip_encoding_vars = conf_load->skip_encoding_btf_vars;
> encoder->skip_encoding_decl_tag = conf_load->skip_encoding_btf_decl_tag;
> encoder->tag_kfuncs = conf_load->btf_decl_tag_kfuncs;
> encoder->gen_distilled_base = conf_load->btf_gen_distilled_base;
> @@ -2377,6 +2376,9 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
> encoder->has_index_type = false;
> encoder->need_index_type = false;
> encoder->array_index_id = 0;
> + encoder->encode_vars = 0;
> + if (!conf_load->skip_encoding_btf_vars)
> + encoder->encode_vars |= BTF_VAR_PERCPU;
>
> GElf_Ehdr ehdr;
>
> @@ -2436,7 +2438,7 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
> 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->skip_encoding_vars))
> + if (btf_encoder__collect_symbols(encoder, encoder->encode_vars & BTF_VAR_PERCPU))
> goto out_delete;
>
> if (encoder->verbose)
> @@ -2633,7 +2635,7 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
> goto out;
> }
>
> - if (!encoder->skip_encoding_vars)
> + if (encoder->encode_vars)
> err = btf_encoder__encode_cu_variables(encoder);
>
> if (!err)
> diff --git a/btf_encoder.h b/btf_encoder.h
> index f54c95a..91e7947 100644
> --- a/btf_encoder.h
> +++ b/btf_encoder.h
> @@ -16,6 +16,12 @@ struct btf;
> struct cu;
> struct list_head;
>
> +/* Bit flags specifying which kinds of variables are emitted */
> +enum btf_var_option {
> + BTF_VAR_NONE = 0,
> + BTF_VAR_PERCPU = 1,
> +};
> +
> struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filename, struct btf *base_btf, bool verbose, struct conf_load *conf_load);
> void btf_encoder__delete(struct btf_encoder *encoder);
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH dwarves v3 2/5] btf_encoder: stop indexing symbols for VARs
2024-10-02 23:52 ` [PATCH dwarves v3 2/5] btf_encoder: stop indexing symbols for VARs Stephen Brennan
@ 2024-10-03 13:59 ` Alan Maguire
2024-10-03 17:26 ` Stephen Brennan
0 siblings, 1 reply; 20+ messages in thread
From: Alan Maguire @ 2024-10-03 13:59 UTC (permalink / raw)
To: Stephen Brennan, Arnaldo Carvalho de Melo; +Cc: bpf, dwarves, linux-debuggers
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);
> }
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH dwarves v3 3/5] btf_encoder: explicitly check addr/size for u32 overflow
2024-10-02 23:52 ` [PATCH dwarves v3 3/5] btf_encoder: explicitly check addr/size for u32 overflow Stephen Brennan
@ 2024-10-03 14:19 ` Alan Maguire
0 siblings, 0 replies; 20+ messages in thread
From: Alan Maguire @ 2024-10-03 14:19 UTC (permalink / raw)
To: Stephen Brennan, Arnaldo Carvalho de Melo; +Cc: bpf, dwarves, linux-debuggers
On 03/10/2024 00:52, Stephen Brennan wrote:
> The addr is a uint64_t, and depending on the size of a data section,
> there's no guarantee that it fits into a uint32_t, even after
> subtracting out the section start address. Similarly, the variable size
> is a size_t which could exceed a uint32_t. Check both for overflow, and
> if found, skip the variable with an error message. Use explicit casts
> when we cast to uint32_t so it's plain to see that this is happening.
>
> Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
> ---
> btf_encoder.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/btf_encoder.c b/btf_encoder.c
> index 31a418a..1872e00 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -2250,9 +2250,16 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder)
>
> tag = cu__type(cu, var->ip.tag.type);
> size = tag__size(tag, cu);
> - if (size == 0) {
> + if (size == 0 || size > UINT32_MAX) {
> if (encoder->verbose)
> - fprintf(stderr, "Ignoring zero-sized per-CPU variable '%s'...\n", name);
> + fprintf(stderr, "Ignoring %s-sized per-CPU variable '%s'...\n",
> + size == 0 ? "zero" : "over", name);
> + continue;
> + }
> + if (addr > UINT32_MAX) {
> + if (encoder->verbose)
> + fprintf(stderr, "Ignoring variable '%s' - its offset %zu doesn't fit in a u32\n",
> + name, addr);
> continue;
> }
>
> @@ -2285,7 +2292,7 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder)
> * add a BTF_VAR_SECINFO in encoder->percpu_secinfo, which will be added into
> * encoder->types later when we add BTF_VAR_DATASEC.
> */
> - id = btf_encoder__add_var_secinfo(encoder, id, addr, size);
> + id = btf_encoder__add_var_secinfo(encoder, id, (uint32_t)addr, (uint32_t)size);
> if (id < 0) {
> fprintf(stderr, "error: failed to encode section info for variable '%s' at addr 0x%" PRIx64 "\n",
> name, addr);
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH dwarves v3 5/5] pahole: add global_var BTF feature
2024-10-02 23:52 ` [PATCH dwarves v3 5/5] pahole: add global_var BTF feature Stephen Brennan
@ 2024-10-03 14:40 ` Alan Maguire
2024-10-03 14:53 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 20+ messages in thread
From: Alan Maguire @ 2024-10-03 14:40 UTC (permalink / raw)
To: Stephen Brennan, Arnaldo Carvalho de Melo; +Cc: bpf, dwarves, linux-debuggers
On 03/10/2024 00:52, Stephen Brennan wrote:
> So far, pahole has only encoded type information for percpu variables.
> However, there are several reasons why type information for all global
> variables would be useful in the kernel:
>
> 1. Runtime kernel debuggers like drgn could use the BTF to introspect
> kernel data structures without needing to install heavyweight DWARF.
>
> 2. BPF programs using the "__ksym" annotation could declare the
> variables using the correct type, rather than "void".
>
> It makes sense to introduce a feature for this in pahole so that these
> capabilities can be explored in the kernel. The feature is non-default:
> when using "--btf-features=default", it is disabled. It must be
> explicitly requested, e.g. with "--btf-features=+global_var".
>
I'm not totally sure switching global_var to a non-default feature is
the right answer here.
The --btf_features "default" set of options are meant to closely mirror
the upstream kernel options we enable when doing BTF encoding. However,
in scripts/Makefile.btf we don't use "default"; we explicitly call out
the set of features we want. We can't just use "default" in that context
since the meaning of "default" varies based upon whatever version of
pahole you have.
So "default" is simply a convenient shorthand for pahole testing which
corresponds to "give me the set of features that upstream kernels use".
It could have a better name that reflects that more clearly I suppose.
When we do switch this on in-kernel, we'll add the explicit "global_var"
to the list of features in scripts/Makefile.btf.
So with all this said, do we make global_vars a default or non-default
feature? It would seem to make sense to specify non-default, since it is
not switched on for the kernel yet, but looking ahead, what if the 1.28
pahole release is used to build vmlinux BTF and we add global_var to the
list of features? In such a case, our "default" set of values would be
out of step with the kernel. So it's not a huge deal, but I would
consider keeping this a default feature to facilitate testing; this
won't change what the kernel does, but it makes testing with full
variable generation easier (I can just do "--btf_features=default").
And on that subject, I tested with this series, and all looks good.
vmlinux BTF grew by 1.5Mb to 6.7Mb for me on a bpf-next kernel.
Following datasecs were seen:
[156581] DATASEC '.rodata' size=7379360 vlen=5472
[156582] DATASEC '__init_rodata' size=496 vlen=3
[156583] DATASEC '__param' size=15160 vlen=375
[156584] DATASEC '__modver' size=864 vlen=12
[156585] DATASEC '.data' size=5955041 vlen=15839
[156586] DATASEC '.vvar' size=656 vlen=2
[156587] DATASEC '.data..percpu' size=229632 vlen=389
[156588] DATASEC '.init.data' size=2057888 vlen=5565
[156589] DATASEC '.x86_cpu_dev.init' size=40 vlen=5
[156590] DATASEC '.apicdrivers' size=56 vlen=7
[156591] DATASEC '.data_nosave' size=4 vlen=1
[156592] DATASEC '.bss' size=3788800 vlen=4080
[156593] DATASEC '.brk' size=196608 vlen=2
[156594] DATASEC '.init.scratch' size=4194304 vlen=1
Biggest contributors in terms of BTF size appear to be
- .data (15839 vars)
- .init.data (5565 vars)
- .rodata (5472 vars)
- .bss (4080 vars)
> Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
Tested-by: Alan Maguire <alan.maguire@oracle.com>
> ---
> btf_encoder.c | 5 +++++
> btf_encoder.h | 1 +
> dwarves.h | 1 +
> man-pages/pahole.1 | 7 +++++--
> pahole.c | 3 ++-
> 5 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/btf_encoder.c b/btf_encoder.c
> index 2fd1648..2730ea8 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -2348,6 +2348,8 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
> encoder->encode_vars = 0;
> if (!conf_load->skip_encoding_btf_vars)
> encoder->encode_vars |= BTF_VAR_PERCPU;
> + if (conf_load->encode_btf_global_vars)
> + encoder->encode_vars |= BTF_VAR_GLOBAL;
>
> GElf_Ehdr ehdr;
>
> @@ -2400,6 +2402,9 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
> encoder->secinfo[shndx].name = secname;
> encoder->secinfo[shndx].type = shdr.sh_type;
>
> + if (encoder->encode_vars & BTF_VAR_GLOBAL)
> + encoder->secinfo[shndx].include = true;
> +
> if (strcmp(secname, PERCPU_SECTION) == 0) {
> found_percpu = true;
> if (encoder->encode_vars & BTF_VAR_PERCPU)
> diff --git a/btf_encoder.h b/btf_encoder.h
> index 91e7947..824963b 100644
> --- a/btf_encoder.h
> +++ b/btf_encoder.h
> @@ -20,6 +20,7 @@ struct list_head;
> enum btf_var_option {
> BTF_VAR_NONE = 0,
> BTF_VAR_PERCPU = 1,
> + BTF_VAR_GLOBAL = 2,
> };
>
> struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filename, struct btf *base_btf, bool verbose, struct conf_load *conf_load);
> diff --git a/dwarves.h b/dwarves.h
> index 0fede91..fef881f 100644
> --- a/dwarves.h
> +++ b/dwarves.h
> @@ -92,6 +92,7 @@ struct conf_load {
> bool btf_gen_optimized;
> bool skip_encoding_btf_inconsistent_proto;
> bool skip_encoding_btf_vars;
> + bool encode_btf_global_vars;
> bool btf_gen_floats;
> bool btf_encode_force;
> bool reproducible_build;
> diff --git a/man-pages/pahole.1 b/man-pages/pahole.1
> index b3e6632..7c1a69a 100644
> --- a/man-pages/pahole.1
> +++ b/man-pages/pahole.1
> @@ -238,7 +238,9 @@ the debugging information.
>
> .TP
> .B \-\-skip_encoding_btf_vars
> -Do not encode VARs in BTF.
> +By default, VARs are encoded only for percpu variables. When specified, this
> +option prevents encoding any VARs. Note that this option can be overridden
> +by the feature "global_var".
>
> .TP
> .B \-\-skip_encoding_btf_decl_tag
> @@ -304,7 +306,7 @@ Encode BTF using the specified feature list, or specify 'default' for all standa
> encode_force Ignore invalid symbols when encoding BTF; for example
> if a symbol has an invalid name, it will be ignored
> and BTF encoding will continue.
> - var Encode variables using BTF_KIND_VAR in BTF.
> + var Encode percpu variables using BTF_KIND_VAR in BTF.
> float Encode floating-point types in BTF.
> decl_tag Encode declaration tags using BTF_KIND_DECL_TAG.
> type_tag Encode type tags using BTF_KIND_TYPE_TAG.
> @@ -329,6 +331,7 @@ Supported non-standard features (not enabled for 'default')
> the associated base BTF to support later relocation
> of split BTF with a possibly changed base, storing
> it in a .BTF.base ELF section.
> + global_var Encode all global variables using BTF_KIND_VAR in BTF.
> .fi
>
> So for example, specifying \-\-btf_encode=var,enum64 will result in a BTF encoding that (as well as encoding basic BTF information) will contain variables and enum64 values.
> diff --git a/pahole.c b/pahole.c
> index b21a7f2..9f0dc59 100644
> --- a/pahole.c
> +++ b/pahole.c
> @@ -1301,6 +1301,7 @@ struct btf_feature {
> BTF_DEFAULT_FEATURE(decl_tag_kfuncs, btf_decl_tag_kfuncs, false),
> BTF_NON_DEFAULT_FEATURE(reproducible_build, reproducible_build, false),
> BTF_NON_DEFAULT_FEATURE(distilled_base, btf_gen_distilled_base, false),
> + BTF_NON_DEFAULT_FEATURE(global_var, encode_btf_global_vars, false),
see above, I'd suggest making this a BTF_DEFAULT_FEATURE() to make
testing easier.
> };
>
> #define BTF_MAX_FEATURE_STR 1024
> @@ -1733,7 +1734,7 @@ static const struct argp_option pahole__options[] = {
> {
> .name = "skip_encoding_btf_vars",
> .key = ARGP_skip_encoding_btf_vars,
> - .doc = "Do not encode VARs in BTF."
> + .doc = "Do not encode any VARs in BTF [if this is not specified, only percpu variables are encoded. To encode global variables too, use --encode_btf_global_vars]."
> },
> {
> .name = "btf_encode_force",
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH dwarves v3 1/5] btf_encoder: use bitfield to control var encoding
2024-10-03 13:41 ` Alan Maguire
@ 2024-10-03 14:41 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 20+ messages in thread
From: Arnaldo Carvalho de Melo @ 2024-10-03 14:41 UTC (permalink / raw)
To: Alan Maguire; +Cc: Stephen Brennan, bpf, dwarves, linux-debuggers
On Thu, Oct 03, 2024 at 02:41:17PM +0100, Alan Maguire wrote:
> On 03/10/2024 00:52, Stephen Brennan wrote:
> > We will need more granularity in the future, in order to add support for
> > encoding global variables as well. So replace the skip_encoding_vars
> > boolean with a flag variable named "encode_vars". There is currently
> > only one bit specified, and it is set when percpu variables should be
> > emitted.
> >
> > Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
>
> Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
Thanks, applying this first one.
- Arnaldo
> > ---
> > btf_encoder.c | 10 ++++++----
> > btf_encoder.h | 6 ++++++
> > 2 files changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/btf_encoder.c b/btf_encoder.c
> > index 51cd7bf..652a945 100644
> > --- a/btf_encoder.c
> > +++ b/btf_encoder.c
> > @@ -119,7 +119,6 @@ struct btf_encoder {
> > uint32_t type_id_off;
> > bool has_index_type,
> > need_index_type,
> > - skip_encoding_vars,
> > raw_output,
> > verbose,
> > force,
> > @@ -137,6 +136,7 @@ struct btf_encoder {
> > int allocated;
> > uint32_t shndx;
> > } percpu;
> > + int encode_vars;
> > struct {
> > struct elf_function *entries;
> > int allocated;
> > @@ -2369,7 +2369,6 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
> >
> > encoder->force = conf_load->btf_encode_force;
> > encoder->gen_floats = conf_load->btf_gen_floats;
> > - encoder->skip_encoding_vars = conf_load->skip_encoding_btf_vars;
> > encoder->skip_encoding_decl_tag = conf_load->skip_encoding_btf_decl_tag;
> > encoder->tag_kfuncs = conf_load->btf_decl_tag_kfuncs;
> > encoder->gen_distilled_base = conf_load->btf_gen_distilled_base;
> > @@ -2377,6 +2376,9 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
> > encoder->has_index_type = false;
> > encoder->need_index_type = false;
> > encoder->array_index_id = 0;
> > + encoder->encode_vars = 0;
> > + if (!conf_load->skip_encoding_btf_vars)
> > + encoder->encode_vars |= BTF_VAR_PERCPU;
> >
> > GElf_Ehdr ehdr;
> >
> > @@ -2436,7 +2438,7 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
> > 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->skip_encoding_vars))
> > + if (btf_encoder__collect_symbols(encoder, encoder->encode_vars & BTF_VAR_PERCPU))
> > goto out_delete;
> >
> > if (encoder->verbose)
> > @@ -2633,7 +2635,7 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
> > goto out;
> > }
> >
> > - if (!encoder->skip_encoding_vars)
> > + if (encoder->encode_vars)
> > err = btf_encoder__encode_cu_variables(encoder);
> >
> > if (!err)
> > diff --git a/btf_encoder.h b/btf_encoder.h
> > index f54c95a..91e7947 100644
> > --- a/btf_encoder.h
> > +++ b/btf_encoder.h
> > @@ -16,6 +16,12 @@ struct btf;
> > struct cu;
> > struct list_head;
> >
> > +/* Bit flags specifying which kinds of variables are emitted */
> > +enum btf_var_option {
> > + BTF_VAR_NONE = 0,
> > + BTF_VAR_PERCPU = 1,
> > +};
> > +
> > struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filename, struct btf *base_btf, bool verbose, struct conf_load *conf_load);
> > void btf_encoder__delete(struct btf_encoder *encoder);
> >
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH dwarves v3 4/5] btf_encoder: allow encoding VARs from many sections
2024-10-02 23:52 ` [PATCH dwarves v3 4/5] btf_encoder: allow encoding VARs from many sections Stephen Brennan
@ 2024-10-03 14:52 ` Alan Maguire
2024-10-03 17:29 ` Stephen Brennan
0 siblings, 1 reply; 20+ messages in thread
From: Alan Maguire @ 2024-10-03 14:52 UTC (permalink / raw)
To: Stephen Brennan, Arnaldo Carvalho de Melo; +Cc: bpf, dwarves, linux-debuggers
On 03/10/2024 00:52, Stephen Brennan wrote:
> Currently we maintain one buffer of DATASEC entries that describe the
> offsets for variables in the percpu ELF section. In order to make it
> possible to output all global variables, we'll need to output a DATASEC
> for each ELF section containing variables, and we'll need to control
> whether or not to encode variables on a per-section basis.
>
> With this change, the ability to emit VARs from multiple sections is
> technically present, but not enabled, so pahole still only emits percpu
> variables. A subsequent change will enable emitting all global
> variables.
>
> Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
Some questions about shndx handling, but
Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
> ---
> btf_encoder.c | 90 ++++++++++++++++++++++++++++++++-------------------
> 1 file changed, 56 insertions(+), 34 deletions(-)
>
> diff --git a/btf_encoder.c b/btf_encoder.c
> index 1872e00..2fd1648 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -98,6 +98,8 @@ struct elf_secinfo {
> const char *name;
> uint64_t sz;
> uint32_t type;
> + bool include;
> + struct gobuffer secinfo;
> };
>
> /*
> @@ -107,7 +109,6 @@ struct btf_encoder {
> struct list_head node;
> struct btf *btf;
> struct cu *cu;
> - struct gobuffer percpu_secinfo;
> const char *source_filename;
> const char *filename;
> struct elf_symtab *symtab;
> @@ -124,7 +125,6 @@ struct btf_encoder {
> uint32_t array_index_id;
> struct elf_secinfo *secinfo;
> size_t seccnt;
> - size_t percpu_shndx;
heh, ignore my earlier gripes about the type here since it's being
removed now!
> int encode_vars;
> struct {
> struct elf_function *entries;
> @@ -784,46 +784,56 @@ static int32_t btf_encoder__add_var(struct btf_encoder *encoder, uint32_t type,
> return id;
> }
>
> -static int32_t btf_encoder__add_var_secinfo(struct btf_encoder *encoder, uint32_t type,
> - uint32_t offset, uint32_t size)
> +static int32_t btf_encoder__add_var_secinfo(struct btf_encoder *encoder, size_t shndx,
> + uint32_t type, uint32_t offset, uint32_t size)
> {
> struct btf_var_secinfo si = {
> .type = type,
> .offset = offset,
> .size = size,
> };
> - return gobuffer__add(&encoder->percpu_secinfo, &si, sizeof(si));
> + return gobuffer__add(&encoder->secinfo[shndx].secinfo, &si, sizeof(si));
> }
>
> int32_t btf_encoder__add_encoder(struct btf_encoder *encoder, struct btf_encoder *other)
> {
> - struct gobuffer *var_secinfo_buf = &other->percpu_secinfo;
> - size_t sz = gobuffer__size(var_secinfo_buf);
> - uint16_t nr_var_secinfo = sz / sizeof(struct btf_var_secinfo);
> - uint32_t type_id;
> - uint32_t next_type_id = btf__type_cnt(encoder->btf);
> - int32_t i, id;
> - struct btf_var_secinfo *vsi;
> -
> + size_t shndx;
> if (encoder == other)
> return 0;
>
> btf_encoder__add_saved_funcs(other);
>
> - for (i = 0; i < nr_var_secinfo; i++) {
> - vsi = (struct btf_var_secinfo *)var_secinfo_buf->entries + i;
> - type_id = next_type_id + vsi->type - 1; /* Type ID starts from 1 */
> - id = btf_encoder__add_var_secinfo(encoder, type_id, vsi->offset, vsi->size);
> - if (id < 0)
> - return id;
> + for (shndx = 0; shndx < other->seccnt; shndx++) {
can't we start from 1 here since the first section is SHT_NULL?
> + struct gobuffer *var_secinfo_buf = &other->secinfo[shndx].secinfo;
> + size_t sz = gobuffer__size(var_secinfo_buf);
> + uint16_t nr_var_secinfo = sz / sizeof(struct btf_var_secinfo);
> + uint32_t type_id;
> + uint32_t next_type_id = btf__type_cnt(encoder->btf);
> + int32_t i, id;
> + struct btf_var_secinfo *vsi;
> +
> + if (strcmp(encoder->secinfo[shndx].name, other->secinfo[shndx].name)) {
> + fprintf(stderr, "mismatched ELF sections at index %zu: \"%s\", \"%s\"\n",
> + shndx, encoder->secinfo[shndx].name, other->secinfo[shndx].name);
> + return -1;
> + }
> +
> + for (i = 0; i < nr_var_secinfo; i++) {
> + vsi = (struct btf_var_secinfo *)var_secinfo_buf->entries + i;
> + type_id = next_type_id + vsi->type - 1; /* Type ID starts from 1 */
> + id = btf_encoder__add_var_secinfo(encoder, shndx, type_id, vsi->offset, vsi->size);
> + if (id < 0)
> + return id;
> + }
> }
>
> return btf__add_btf(encoder->btf, other->btf);
> }
>
> -static int32_t btf_encoder__add_datasec(struct btf_encoder *encoder, const char *section_name)
> +static int32_t btf_encoder__add_datasec(struct btf_encoder *encoder, size_t shndx)
> {
> - struct gobuffer *var_secinfo_buf = &encoder->percpu_secinfo;
> + struct gobuffer *var_secinfo_buf = &encoder->secinfo[shndx].secinfo;
> + const char *section_name = encoder->secinfo[shndx].name;
> struct btf *btf = encoder->btf;
> size_t sz = gobuffer__size(var_secinfo_buf);
> uint16_t nr_var_secinfo = sz / sizeof(struct btf_var_secinfo);
> @@ -2032,12 +2042,14 @@ int btf_encoder__encode(struct btf_encoder *encoder)
> {
> bool should_tag_kfuncs;
> int err;
> + size_t shndx;
>
> /* for single-threaded case, saved funcs are added here */
> btf_encoder__add_saved_funcs(encoder);
>
> - if (gobuffer__size(&encoder->percpu_secinfo) != 0)
> - btf_encoder__add_datasec(encoder, PERCPU_SECTION);
> + for (shndx = 0; shndx < encoder->seccnt; shndx++)
same question here for 0 shndx
> + if (gobuffer__size(&encoder->secinfo[shndx].secinfo))
> + btf_encoder__add_datasec(encoder, shndx);
>
> /* Empty file, nothing to do, so... done! */
> if (btf__type_cnt(encoder->btf) == 1)
> @@ -2167,7 +2179,7 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder)
> struct tag *pos;
> int err = -1;
>
> - if (encoder->percpu_shndx == 0 || !encoder->symtab)
> + if (!encoder->symtab)
> return 0;
>
> if (encoder->verbose)
> @@ -2180,6 +2192,7 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder)
> struct llvm_annotation *annot;
> const struct tag *tag;
> size_t shndx, size;
> + struct elf_secinfo *sec = NULL;
> uint64_t addr;
> int id;
>
> @@ -2211,7 +2224,9 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder)
>
> /* Get the ELF section info for the variable */
> shndx = get_elf_section(encoder, addr);
> - if (shndx != encoder->percpu_shndx)
> + if (shndx >= 0 && shndx < encoder->seccnt)
> + sec = &encoder->secinfo[shndx];
> + if (!sec || !sec->include)
> continue;
>
> /* Convert addr to section relative */
> @@ -2252,7 +2267,7 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder)
> size = tag__size(tag, cu);
> if (size == 0 || size > UINT32_MAX) {
> if (encoder->verbose)
> - fprintf(stderr, "Ignoring %s-sized per-CPU variable '%s'...\n",
> + fprintf(stderr, "Ignoring %s-sized variable '%s'...\n",
> size == 0 ? "zero" : "over", name);
> continue;
> }
> @@ -2289,13 +2304,14 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder)
> }
>
> /*
> - * add a BTF_VAR_SECINFO in encoder->percpu_secinfo, which will be added into
> - * encoder->types later when we add BTF_VAR_DATASEC.
> + * Add the variable to the secinfo for the section it appears in.
> + * Later we will generate a BTF_VAR_DATASEC for all any section with
> + * an encoded variable.
> */
> - id = btf_encoder__add_var_secinfo(encoder, id, (uint32_t)addr, (uint32_t)size);
> + id = btf_encoder__add_var_secinfo(encoder, shndx, id, (uint32_t)addr, (uint32_t)size);
> if (id < 0) {
> fprintf(stderr, "error: failed to encode section info for variable '%s' at addr 0x%" PRIx64 "\n",
> - name, addr);
> + name, addr);
> goto out;
> }
> }
> @@ -2373,6 +2389,7 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
> goto out_delete;
> }
>
> + bool found_percpu = false;
> for (shndx = 0; shndx < encoder->seccnt; shndx++) {
> const char *secname = NULL;
> Elf_Scn *sec = elf_section_by_idx(cu->elf, &shdr, shndx, &secname);
> @@ -2383,11 +2400,14 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
> encoder->secinfo[shndx].name = secname;
> encoder->secinfo[shndx].type = shdr.sh_type;
>
> - if (strcmp(secname, PERCPU_SECTION) == 0)
> - encoder->percpu_shndx = shndx;
> + if (strcmp(secname, PERCPU_SECTION) == 0) {
> + found_percpu = true;
> + if (encoder->encode_vars & BTF_VAR_PERCPU)
> + encoder->secinfo[shndx].include = true;
> + }
> }
>
> - if (!encoder->percpu_shndx && encoder->verbose)
> + if (!found_percpu && encoder->verbose)
> printf("%s: '%s' doesn't have '%s' section\n", __func__, cu->filename, PERCPU_SECTION);
>
> if (btf_encoder__collect_symbols(encoder))
> @@ -2415,12 +2435,14 @@ void btf_encoder__delete_func(struct elf_function *func)
> void btf_encoder__delete(struct btf_encoder *encoder)
> {
> int i;
> + size_t shndx;
>
> if (encoder == NULL)
> return;
>
> btf_encoders__delete(encoder);
> - __gobuffer__delete(&encoder->percpu_secinfo);
> + for (shndx = 0; shndx < encoder->seccnt; shndx++)
> + __gobuffer__delete(&encoder->secinfo[shndx].secinfo);
> zfree(&encoder->filename);
> zfree(&encoder->source_filename);
> btf__free(encoder->btf);
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH dwarves v3 5/5] pahole: add global_var BTF feature
2024-10-03 14:40 ` Alan Maguire
@ 2024-10-03 14:53 ` Arnaldo Carvalho de Melo
2024-10-03 15:21 ` Alan Maguire
0 siblings, 1 reply; 20+ messages in thread
From: Arnaldo Carvalho de Melo @ 2024-10-03 14:53 UTC (permalink / raw)
To: Alan Maguire; +Cc: Stephen Brennan, bpf, dwarves, linux-debuggers
On Thu, Oct 03, 2024 at 03:40:35PM +0100, Alan Maguire wrote:
> On 03/10/2024 00:52, Stephen Brennan wrote:
> > So far, pahole has only encoded type information for percpu variables.
> > However, there are several reasons why type information for all global
> > variables would be useful in the kernel:
> > 1. Runtime kernel debuggers like drgn could use the BTF to introspect
> > kernel data structures without needing to install heavyweight DWARF.
> > 2. BPF programs using the "__ksym" annotation could declare the
> > variables using the correct type, rather than "void".
> > It makes sense to introduce a feature for this in pahole so that these
> > capabilities can be explored in the kernel. The feature is non-default:
> > when using "--btf-features=default", it is disabled. It must be
> > explicitly requested, e.g. with "--btf-features=+global_var".
> I'm not totally sure switching global_var to a non-default feature is
> the right answer here.
> The --btf_features "default" set of options are meant to closely mirror
> the upstream kernel options we enable when doing BTF encoding. However,
> in scripts/Makefile.btf we don't use "default"; we explicitly call out
> the set of features we want. We can't just use "default" in that context
> since the meaning of "default" varies based upon whatever version of
> pahole you have.
> So "default" is simply a convenient shorthand for pahole testing which
> corresponds to "give me the set of features that upstream kernels use".
> It could have a better name that reflects that more clearly I suppose.
> When we do switch this on in-kernel, we'll add the explicit "global_var"
> to the list of features in scripts/Makefile.btf.
> So with all this said, do we make global_vars a default or non-default
> feature? It would seem to make sense to specify non-default, since it is
> not switched on for the kernel yet, but looking ahead, what if the 1.28
> pahole release is used to build vmlinux BTF and we add global_var to the
> list of features? In such a case, our "default" set of values would be
> out of step with the kernel. So it's not a huge deal, but I would
> consider keeping this a default feature to facilitate testing; this
> won't change what the kernel does, but it makes testing with full
> variable generation easier (I can just do "--btf_features=default").
This "default" really is confusing, as you spelled out above :-\ When to
add something to it so that it reflects what the kernel has is tricky,
perhaps we should instead have a ~/.config/pahole file where developers
can add BTF features to add to --btf_features=default in the period
where something new was _really_ added to the kernel and before the next
version when it _have been added to the kernel set of BTF features_ thus
should be set into stone in the pahole sources?
So I think we should do as Stephen did, keep it out of
--btf_features=default, as it is not yet in the kernel set of options,
and have the config file, starting with being able to set those
features, i.e. we would have:
$ cat ~/.config/pahole
[btf_encoder]
btf_features=+global_var
wdyt?
- Arnaldo
> And on that subject, I tested with this series, and all looks good.
> vmlinux BTF grew by 1.5Mb to 6.7Mb for me on a bpf-next kernel.
> Following datasecs were seen:
>
> [156581] DATASEC '.rodata' size=7379360 vlen=5472
> [156582] DATASEC '__init_rodata' size=496 vlen=3
> [156583] DATASEC '__param' size=15160 vlen=375
> [156584] DATASEC '__modver' size=864 vlen=12
> [156585] DATASEC '.data' size=5955041 vlen=15839
> [156586] DATASEC '.vvar' size=656 vlen=2
> [156587] DATASEC '.data..percpu' size=229632 vlen=389
> [156588] DATASEC '.init.data' size=2057888 vlen=5565
> [156589] DATASEC '.x86_cpu_dev.init' size=40 vlen=5
> [156590] DATASEC '.apicdrivers' size=56 vlen=7
> [156591] DATASEC '.data_nosave' size=4 vlen=1
> [156592] DATASEC '.bss' size=3788800 vlen=4080
> [156593] DATASEC '.brk' size=196608 vlen=2
> [156594] DATASEC '.init.scratch' size=4194304 vlen=1
>
> Biggest contributors in terms of BTF size appear to be
>
> - .data (15839 vars)
> - .init.data (5565 vars)
> - .rodata (5472 vars)
> - .bss (4080 vars)
>
> > Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
>
> Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
> Tested-by: Alan Maguire <alan.maguire@oracle.com>
>
> > ---
> > btf_encoder.c | 5 +++++
> > btf_encoder.h | 1 +
> > dwarves.h | 1 +
> > man-pages/pahole.1 | 7 +++++--
> > pahole.c | 3 ++-
> > 5 files changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/btf_encoder.c b/btf_encoder.c
> > index 2fd1648..2730ea8 100644
> > --- a/btf_encoder.c
> > +++ b/btf_encoder.c
> > @@ -2348,6 +2348,8 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
> > encoder->encode_vars = 0;
> > if (!conf_load->skip_encoding_btf_vars)
> > encoder->encode_vars |= BTF_VAR_PERCPU;
> > + if (conf_load->encode_btf_global_vars)
> > + encoder->encode_vars |= BTF_VAR_GLOBAL;
> >
> > GElf_Ehdr ehdr;
> >
> > @@ -2400,6 +2402,9 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
> > encoder->secinfo[shndx].name = secname;
> > encoder->secinfo[shndx].type = shdr.sh_type;
> >
> > + if (encoder->encode_vars & BTF_VAR_GLOBAL)
> > + encoder->secinfo[shndx].include = true;
> > +
> > if (strcmp(secname, PERCPU_SECTION) == 0) {
> > found_percpu = true;
> > if (encoder->encode_vars & BTF_VAR_PERCPU)
> > diff --git a/btf_encoder.h b/btf_encoder.h
> > index 91e7947..824963b 100644
> > --- a/btf_encoder.h
> > +++ b/btf_encoder.h
> > @@ -20,6 +20,7 @@ struct list_head;
> > enum btf_var_option {
> > BTF_VAR_NONE = 0,
> > BTF_VAR_PERCPU = 1,
> > + BTF_VAR_GLOBAL = 2,
> > };
> >
> > struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filename, struct btf *base_btf, bool verbose, struct conf_load *conf_load);
> > diff --git a/dwarves.h b/dwarves.h
> > index 0fede91..fef881f 100644
> > --- a/dwarves.h
> > +++ b/dwarves.h
> > @@ -92,6 +92,7 @@ struct conf_load {
> > bool btf_gen_optimized;
> > bool skip_encoding_btf_inconsistent_proto;
> > bool skip_encoding_btf_vars;
> > + bool encode_btf_global_vars;
> > bool btf_gen_floats;
> > bool btf_encode_force;
> > bool reproducible_build;
> > diff --git a/man-pages/pahole.1 b/man-pages/pahole.1
> > index b3e6632..7c1a69a 100644
> > --- a/man-pages/pahole.1
> > +++ b/man-pages/pahole.1
> > @@ -238,7 +238,9 @@ the debugging information.
> >
> > .TP
> > .B \-\-skip_encoding_btf_vars
> > -Do not encode VARs in BTF.
> > +By default, VARs are encoded only for percpu variables. When specified, this
> > +option prevents encoding any VARs. Note that this option can be overridden
> > +by the feature "global_var".
> >
> > .TP
> > .B \-\-skip_encoding_btf_decl_tag
> > @@ -304,7 +306,7 @@ Encode BTF using the specified feature list, or specify 'default' for all standa
> > encode_force Ignore invalid symbols when encoding BTF; for example
> > if a symbol has an invalid name, it will be ignored
> > and BTF encoding will continue.
> > - var Encode variables using BTF_KIND_VAR in BTF.
> > + var Encode percpu variables using BTF_KIND_VAR in BTF.
> > float Encode floating-point types in BTF.
> > decl_tag Encode declaration tags using BTF_KIND_DECL_TAG.
> > type_tag Encode type tags using BTF_KIND_TYPE_TAG.
> > @@ -329,6 +331,7 @@ Supported non-standard features (not enabled for 'default')
> > the associated base BTF to support later relocation
> > of split BTF with a possibly changed base, storing
> > it in a .BTF.base ELF section.
> > + global_var Encode all global variables using BTF_KIND_VAR in BTF.
> > .fi
> >
> > So for example, specifying \-\-btf_encode=var,enum64 will result in a BTF encoding that (as well as encoding basic BTF information) will contain variables and enum64 values.
> > diff --git a/pahole.c b/pahole.c
> > index b21a7f2..9f0dc59 100644
> > --- a/pahole.c
> > +++ b/pahole.c
> > @@ -1301,6 +1301,7 @@ struct btf_feature {
> > BTF_DEFAULT_FEATURE(decl_tag_kfuncs, btf_decl_tag_kfuncs, false),
> > BTF_NON_DEFAULT_FEATURE(reproducible_build, reproducible_build, false),
> > BTF_NON_DEFAULT_FEATURE(distilled_base, btf_gen_distilled_base, false),
> > + BTF_NON_DEFAULT_FEATURE(global_var, encode_btf_global_vars, false),
>
> see above, I'd suggest making this a BTF_DEFAULT_FEATURE() to make
> testing easier.
>
> > };
> >
> > #define BTF_MAX_FEATURE_STR 1024
> > @@ -1733,7 +1734,7 @@ static const struct argp_option pahole__options[] = {
> > {
> > .name = "skip_encoding_btf_vars",
> > .key = ARGP_skip_encoding_btf_vars,
> > - .doc = "Do not encode VARs in BTF."
> > + .doc = "Do not encode any VARs in BTF [if this is not specified, only percpu variables are encoded. To encode global variables too, use --encode_btf_global_vars]."
> > },
> > {
> > .name = "btf_encode_force",
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH dwarves v3 5/5] pahole: add global_var BTF feature
2024-10-03 14:53 ` Arnaldo Carvalho de Melo
@ 2024-10-03 15:21 ` Alan Maguire
2024-10-03 17:18 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 20+ messages in thread
From: Alan Maguire @ 2024-10-03 15:21 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo; +Cc: Stephen Brennan, bpf, dwarves, linux-debuggers
On 03/10/2024 15:53, Arnaldo Carvalho de Melo wrote:
> On Thu, Oct 03, 2024 at 03:40:35PM +0100, Alan Maguire wrote:
>> On 03/10/2024 00:52, Stephen Brennan wrote:
>>> So far, pahole has only encoded type information for percpu variables.
>>> However, there are several reasons why type information for all global
>>> variables would be useful in the kernel:
>
>>> 1. Runtime kernel debuggers like drgn could use the BTF to introspect
>>> kernel data structures without needing to install heavyweight DWARF.
>
>>> 2. BPF programs using the "__ksym" annotation could declare the
>>> variables using the correct type, rather than "void".
>
>>> It makes sense to introduce a feature for this in pahole so that these
>>> capabilities can be explored in the kernel. The feature is non-default:
>>> when using "--btf-features=default", it is disabled. It must be
>>> explicitly requested, e.g. with "--btf-features=+global_var".
>
>> I'm not totally sure switching global_var to a non-default feature is
>> the right answer here.
>
>> The --btf_features "default" set of options are meant to closely mirror
>> the upstream kernel options we enable when doing BTF encoding. However,
>> in scripts/Makefile.btf we don't use "default"; we explicitly call out
>> the set of features we want. We can't just use "default" in that context
>> since the meaning of "default" varies based upon whatever version of
>> pahole you have.
>
>> So "default" is simply a convenient shorthand for pahole testing which
>> corresponds to "give me the set of features that upstream kernels use".
>> It could have a better name that reflects that more clearly I suppose.
>
>> When we do switch this on in-kernel, we'll add the explicit "global_var"
>> to the list of features in scripts/Makefile.btf.
>
>> So with all this said, do we make global_vars a default or non-default
>> feature? It would seem to make sense to specify non-default, since it is
>> not switched on for the kernel yet, but looking ahead, what if the 1.28
>> pahole release is used to build vmlinux BTF and we add global_var to the
>> list of features? In such a case, our "default" set of values would be
>> out of step with the kernel. So it's not a huge deal, but I would
>> consider keeping this a default feature to facilitate testing; this
>> won't change what the kernel does, but it makes testing with full
>> variable generation easier (I can just do "--btf_features=default").
>
> This "default" really is confusing, as you spelled out above :-\ When to
> add something to it so that it reflects what the kernel has is tricky,
> perhaps we should instead have a ~/.config/pahole file where developers
> can add BTF features to add to --btf_features=default in the period
> where something new was _really_ added to the kernel and before the next
> version when it _have been added to the kernel set of BTF features_ thus
> should be set into stone in the pahole sources?
>
it's a nice idea; I suppose once we have more automated tests, this will
be less of an issue too. I'm looking at adding a BTF variable test
shortly, would be good to have coverage there too, especially since
we're growing the amount of info we encode in this area.
> So I think we should do as Stephen did, keep it out of
> --btf_features=default, as it is not yet in the kernel set of options,
> and have the config file, starting with being able to set those
> features, i.e. we would have:
>
> $ cat ~/.config/pahole
> [btf_encoder]
> btf_features=+global_var
>
> wdyt?
>
I think that makes perfect sense, great idea!
Alan
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH dwarves v3 5/5] pahole: add global_var BTF feature
2024-10-03 15:21 ` Alan Maguire
@ 2024-10-03 17:18 ` Arnaldo Carvalho de Melo
2024-10-03 17:48 ` Stephen Brennan
0 siblings, 1 reply; 20+ messages in thread
From: Arnaldo Carvalho de Melo @ 2024-10-03 17:18 UTC (permalink / raw)
To: Alan Maguire; +Cc: Stephen Brennan, bpf, dwarves, linux-debuggers
On Thu, Oct 03, 2024 at 04:21:07PM +0100, Alan Maguire wrote:
> On 03/10/2024 15:53, Arnaldo Carvalho de Melo wrote:
> > On Thu, Oct 03, 2024 at 03:40:35PM +0100, Alan Maguire wrote:
> >> On 03/10/2024 00:52, Stephen Brennan wrote:
> >>> So far, pahole has only encoded type information for percpu variables.
> >>> However, there are several reasons why type information for all global
> >>> variables would be useful in the kernel:
> >
> >>> 1. Runtime kernel debuggers like drgn could use the BTF to introspect
> >>> kernel data structures without needing to install heavyweight DWARF.
> >
> >>> 2. BPF programs using the "__ksym" annotation could declare the
> >>> variables using the correct type, rather than "void".
> >
> >>> It makes sense to introduce a feature for this in pahole so that these
> >>> capabilities can be explored in the kernel. The feature is non-default:
> >>> when using "--btf-features=default", it is disabled. It must be
> >>> explicitly requested, e.g. with "--btf-features=+global_var".
> >
> >> I'm not totally sure switching global_var to a non-default feature is
> >> the right answer here.
> >
> >> The --btf_features "default" set of options are meant to closely mirror
> >> the upstream kernel options we enable when doing BTF encoding. However,
> >> in scripts/Makefile.btf we don't use "default"; we explicitly call out
> >> the set of features we want. We can't just use "default" in that context
> >> since the meaning of "default" varies based upon whatever version of
> >> pahole you have.
> >
> >> So "default" is simply a convenient shorthand for pahole testing which
> >> corresponds to "give me the set of features that upstream kernels use".
> >> It could have a better name that reflects that more clearly I suppose.
> >
> >> When we do switch this on in-kernel, we'll add the explicit "global_var"
> >> to the list of features in scripts/Makefile.btf.
> >
> >> So with all this said, do we make global_vars a default or non-default
> >> feature? It would seem to make sense to specify non-default, since it is
> >> not switched on for the kernel yet, but looking ahead, what if the 1.28
> >> pahole release is used to build vmlinux BTF and we add global_var to the
> >> list of features? In such a case, our "default" set of values would be
> >> out of step with the kernel. So it's not a huge deal, but I would
> >> consider keeping this a default feature to facilitate testing; this
> >> won't change what the kernel does, but it makes testing with full
> >> variable generation easier (I can just do "--btf_features=default").
> >
> > This "default" really is confusing, as you spelled out above :-\ When to
> > add something to it so that it reflects what the kernel has is tricky,
> > perhaps we should instead have a ~/.config/pahole file where developers
> > can add BTF features to add to --btf_features=default in the period
> > where something new was _really_ added to the kernel and before the next
> > version when it _have been added to the kernel set of BTF features_ thus
> > should be set into stone in the pahole sources?
> it's a nice idea; I suppose once we have more automated tests, this will
> be less of an issue too. I'm looking at adding a BTF variable test
> shortly, would be good to have coverage there too, especially since
> we're growing the amount of info we encode in this area.
Sure thing, the more tests, the better!
> > So I think we should do as Stephen did, keep it out of
> > --btf_features=default, as it is not yet in the kernel set of options,
> > and have the config file, starting with being able to set those
> > features, i.e. we would have:
> > $ cat ~/.config/pahole
> > [btf_encoder]
> > btf_features=+global_var
> > wdyt?
> I think that makes perfect sense, great idea!
I was looking for a library to do that to avoid "stealing" the
perf-config code, but perhaps we should use an env var for that?
PAHOLE_BTF_FEATURES='+global_var'
To keep things simple?
- Arnaldo
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH dwarves v3 2/5] btf_encoder: stop indexing symbols for VARs
2024-10-03 13:59 ` Alan Maguire
@ 2024-10-03 17:26 ` Stephen Brennan
0 siblings, 0 replies; 20+ messages in thread
From: Stephen Brennan @ 2024-10-03 17:26 UTC (permalink / raw)
To: Alan Maguire, Arnaldo Carvalho de Melo; +Cc: bpf, dwarves, linux-debuggers
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);
>> }
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH dwarves v3 4/5] btf_encoder: allow encoding VARs from many sections
2024-10-03 14:52 ` Alan Maguire
@ 2024-10-03 17:29 ` Stephen Brennan
0 siblings, 0 replies; 20+ messages in thread
From: Stephen Brennan @ 2024-10-03 17:29 UTC (permalink / raw)
To: Alan Maguire, Arnaldo Carvalho de Melo; +Cc: bpf, dwarves, linux-debuggers
Alan Maguire <alan.maguire@oracle.com> writes:
> On 03/10/2024 00:52, Stephen Brennan wrote:
>> Currently we maintain one buffer of DATASEC entries that describe the
>> offsets for variables in the percpu ELF section. In order to make it
>> possible to output all global variables, we'll need to output a DATASEC
>> for each ELF section containing variables, and we'll need to control
>> whether or not to encode variables on a per-section basis.
>>
>> With this change, the ability to emit VARs from multiple sections is
>> technically present, but not enabled, so pahole still only emits percpu
>> variables. A subsequent change will enable emitting all global
>> variables.
>>
>> Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
>
> Some questions about shndx handling, but
>
> Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
>
>> ---
>> btf_encoder.c | 90 ++++++++++++++++++++++++++++++++-------------------
>> 1 file changed, 56 insertions(+), 34 deletions(-)
>>
>> diff --git a/btf_encoder.c b/btf_encoder.c
>> index 1872e00..2fd1648 100644
>> --- a/btf_encoder.c
>> +++ b/btf_encoder.c
>> @@ -98,6 +98,8 @@ struct elf_secinfo {
>> const char *name;
>> uint64_t sz;
>> uint32_t type;
>> + bool include;
>> + struct gobuffer secinfo;
>> };
>>
>> /*
>> @@ -107,7 +109,6 @@ struct btf_encoder {
>> struct list_head node;
>> struct btf *btf;
>> struct cu *cu;
>> - struct gobuffer percpu_secinfo;
>> const char *source_filename;
>> const char *filename;
>> struct elf_symtab *symtab;
>> @@ -124,7 +125,6 @@ struct btf_encoder {
>> uint32_t array_index_id;
>> struct elf_secinfo *secinfo;
>> size_t seccnt;
>> - size_t percpu_shndx;
>
> heh, ignore my earlier gripes about the type here since it's being
> removed now!
>
>> int encode_vars;
>> struct {
>> struct elf_function *entries;
>> @@ -784,46 +784,56 @@ static int32_t btf_encoder__add_var(struct btf_encoder *encoder, uint32_t type,
>> return id;
>> }
>>
>> -static int32_t btf_encoder__add_var_secinfo(struct btf_encoder *encoder, uint32_t type,
>> - uint32_t offset, uint32_t size)
>> +static int32_t btf_encoder__add_var_secinfo(struct btf_encoder *encoder, size_t shndx,
>> + uint32_t type, uint32_t offset, uint32_t size)
>> {
>> struct btf_var_secinfo si = {
>> .type = type,
>> .offset = offset,
>> .size = size,
>> };
>> - return gobuffer__add(&encoder->percpu_secinfo, &si, sizeof(si));
>> + return gobuffer__add(&encoder->secinfo[shndx].secinfo, &si, sizeof(si));
>> }
>>
>> int32_t btf_encoder__add_encoder(struct btf_encoder *encoder, struct btf_encoder *other)
>> {
>> - struct gobuffer *var_secinfo_buf = &other->percpu_secinfo;
>> - size_t sz = gobuffer__size(var_secinfo_buf);
>> - uint16_t nr_var_secinfo = sz / sizeof(struct btf_var_secinfo);
>> - uint32_t type_id;
>> - uint32_t next_type_id = btf__type_cnt(encoder->btf);
>> - int32_t i, id;
>> - struct btf_var_secinfo *vsi;
>> -
>> + size_t shndx;
>> if (encoder == other)
>> return 0;
>>
>> btf_encoder__add_saved_funcs(other);
>>
>> - for (i = 0; i < nr_var_secinfo; i++) {
>> - vsi = (struct btf_var_secinfo *)var_secinfo_buf->entries + i;
>> - type_id = next_type_id + vsi->type - 1; /* Type ID starts from 1 */
>> - id = btf_encoder__add_var_secinfo(encoder, type_id, vsi->offset, vsi->size);
>> - if (id < 0)
>> - return id;
>> + for (shndx = 0; shndx < other->seccnt; shndx++) {
>
> can't we start from 1 here since the first section is SHT_NULL?
Yeah, we're starting from 1 elsewhere, so I think all the other loops
should do so. Otherwise it's inconsistent. Nice catch, thanks!
Stephen
>> + struct gobuffer *var_secinfo_buf = &other->secinfo[shndx].secinfo;
>> + size_t sz = gobuffer__size(var_secinfo_buf);
>> + uint16_t nr_var_secinfo = sz / sizeof(struct btf_var_secinfo);
>> + uint32_t type_id;
>> + uint32_t next_type_id = btf__type_cnt(encoder->btf);
>> + int32_t i, id;
>> + struct btf_var_secinfo *vsi;
>> +
>> + if (strcmp(encoder->secinfo[shndx].name, other->secinfo[shndx].name)) {
>> + fprintf(stderr, "mismatched ELF sections at index %zu: \"%s\", \"%s\"\n",
>> + shndx, encoder->secinfo[shndx].name, other->secinfo[shndx].name);
>> + return -1;
>> + }
>> +
>> + for (i = 0; i < nr_var_secinfo; i++) {
>> + vsi = (struct btf_var_secinfo *)var_secinfo_buf->entries + i;
>> + type_id = next_type_id + vsi->type - 1; /* Type ID starts from 1 */
>> + id = btf_encoder__add_var_secinfo(encoder, shndx, type_id, vsi->offset, vsi->size);
>> + if (id < 0)
>> + return id;
>> + }
>> }
>>
>> return btf__add_btf(encoder->btf, other->btf);
>> }
>>
>> -static int32_t btf_encoder__add_datasec(struct btf_encoder *encoder, const char *section_name)
>> +static int32_t btf_encoder__add_datasec(struct btf_encoder *encoder, size_t shndx)
>> {
>> - struct gobuffer *var_secinfo_buf = &encoder->percpu_secinfo;
>> + struct gobuffer *var_secinfo_buf = &encoder->secinfo[shndx].secinfo;
>> + const char *section_name = encoder->secinfo[shndx].name;
>> struct btf *btf = encoder->btf;
>> size_t sz = gobuffer__size(var_secinfo_buf);
>> uint16_t nr_var_secinfo = sz / sizeof(struct btf_var_secinfo);
>> @@ -2032,12 +2042,14 @@ int btf_encoder__encode(struct btf_encoder *encoder)
>> {
>> bool should_tag_kfuncs;
>> int err;
>> + size_t shndx;
>>
>> /* for single-threaded case, saved funcs are added here */
>> btf_encoder__add_saved_funcs(encoder);
>>
>> - if (gobuffer__size(&encoder->percpu_secinfo) != 0)
>> - btf_encoder__add_datasec(encoder, PERCPU_SECTION);
>> + for (shndx = 0; shndx < encoder->seccnt; shndx++)
>
> same question here for 0 shndx
>
>
>> + if (gobuffer__size(&encoder->secinfo[shndx].secinfo))
>> + btf_encoder__add_datasec(encoder, shndx);
>>
>> /* Empty file, nothing to do, so... done! */
>> if (btf__type_cnt(encoder->btf) == 1)
>> @@ -2167,7 +2179,7 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder)
>> struct tag *pos;
>> int err = -1;
>>
>> - if (encoder->percpu_shndx == 0 || !encoder->symtab)
>> + if (!encoder->symtab)
>> return 0;
>>
>> if (encoder->verbose)
>> @@ -2180,6 +2192,7 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder)
>> struct llvm_annotation *annot;
>> const struct tag *tag;
>> size_t shndx, size;
>> + struct elf_secinfo *sec = NULL;
>> uint64_t addr;
>> int id;
>>
>> @@ -2211,7 +2224,9 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder)
>>
>> /* Get the ELF section info for the variable */
>> shndx = get_elf_section(encoder, addr);
>> - if (shndx != encoder->percpu_shndx)
>> + if (shndx >= 0 && shndx < encoder->seccnt)
>> + sec = &encoder->secinfo[shndx];
>> + if (!sec || !sec->include)
>> continue;
>>
>> /* Convert addr to section relative */
>> @@ -2252,7 +2267,7 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder)
>> size = tag__size(tag, cu);
>> if (size == 0 || size > UINT32_MAX) {
>> if (encoder->verbose)
>> - fprintf(stderr, "Ignoring %s-sized per-CPU variable '%s'...\n",
>> + fprintf(stderr, "Ignoring %s-sized variable '%s'...\n",
>> size == 0 ? "zero" : "over", name);
>> continue;
>> }
>> @@ -2289,13 +2304,14 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder)
>> }
>>
>> /*
>> - * add a BTF_VAR_SECINFO in encoder->percpu_secinfo, which will be added into
>> - * encoder->types later when we add BTF_VAR_DATASEC.
>> + * Add the variable to the secinfo for the section it appears in.
>> + * Later we will generate a BTF_VAR_DATASEC for all any section with
>> + * an encoded variable.
>> */
>> - id = btf_encoder__add_var_secinfo(encoder, id, (uint32_t)addr, (uint32_t)size);
>> + id = btf_encoder__add_var_secinfo(encoder, shndx, id, (uint32_t)addr, (uint32_t)size);
>> if (id < 0) {
>> fprintf(stderr, "error: failed to encode section info for variable '%s' at addr 0x%" PRIx64 "\n",
>> - name, addr);
>> + name, addr);
>> goto out;
>> }
>> }
>> @@ -2373,6 +2389,7 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
>> goto out_delete;
>> }
>>
>> + bool found_percpu = false;
>> for (shndx = 0; shndx < encoder->seccnt; shndx++) {
>> const char *secname = NULL;
>> Elf_Scn *sec = elf_section_by_idx(cu->elf, &shdr, shndx, &secname);
>> @@ -2383,11 +2400,14 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
>> encoder->secinfo[shndx].name = secname;
>> encoder->secinfo[shndx].type = shdr.sh_type;
>>
>> - if (strcmp(secname, PERCPU_SECTION) == 0)
>> - encoder->percpu_shndx = shndx;
>> + if (strcmp(secname, PERCPU_SECTION) == 0) {
>> + found_percpu = true;
>> + if (encoder->encode_vars & BTF_VAR_PERCPU)
>> + encoder->secinfo[shndx].include = true;
>> + }
>> }
>>
>> - if (!encoder->percpu_shndx && encoder->verbose)
>> + if (!found_percpu && encoder->verbose)
>> printf("%s: '%s' doesn't have '%s' section\n", __func__, cu->filename, PERCPU_SECTION);
>>
>> if (btf_encoder__collect_symbols(encoder))
>> @@ -2415,12 +2435,14 @@ void btf_encoder__delete_func(struct elf_function *func)
>> void btf_encoder__delete(struct btf_encoder *encoder)
>> {
>> int i;
>> + size_t shndx;
>>
>> if (encoder == NULL)
>> return;
>>
>> btf_encoders__delete(encoder);
>> - __gobuffer__delete(&encoder->percpu_secinfo);
>> + for (shndx = 0; shndx < encoder->seccnt; shndx++)
>> + __gobuffer__delete(&encoder->secinfo[shndx].secinfo);
>> zfree(&encoder->filename);
>> zfree(&encoder->source_filename);
>> btf__free(encoder->btf);
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH dwarves v3 5/5] pahole: add global_var BTF feature
2024-10-03 17:18 ` Arnaldo Carvalho de Melo
@ 2024-10-03 17:48 ` Stephen Brennan
2024-10-03 18:33 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 20+ messages in thread
From: Stephen Brennan @ 2024-10-03 17:48 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Alan Maguire; +Cc: bpf, dwarves, linux-debuggers
Arnaldo Carvalho de Melo <acme@kernel.org> writes:
> On Thu, Oct 03, 2024 at 04:21:07PM +0100, Alan Maguire wrote:
>> On 03/10/2024 15:53, Arnaldo Carvalho de Melo wrote:
>> > On Thu, Oct 03, 2024 at 03:40:35PM +0100, Alan Maguire wrote:
>> >> On 03/10/2024 00:52, Stephen Brennan wrote:
>> >>> So far, pahole has only encoded type information for percpu variables.
>> >>> However, there are several reasons why type information for all global
>> >>> variables would be useful in the kernel:
>> >
>> >>> 1. Runtime kernel debuggers like drgn could use the BTF to introspect
>> >>> kernel data structures without needing to install heavyweight DWARF.
>> >
>> >>> 2. BPF programs using the "__ksym" annotation could declare the
>> >>> variables using the correct type, rather than "void".
>> >
>> >>> It makes sense to introduce a feature for this in pahole so that these
>> >>> capabilities can be explored in the kernel. The feature is non-default:
>> >>> when using "--btf-features=default", it is disabled. It must be
>> >>> explicitly requested, e.g. with "--btf-features=+global_var".
>> >
>> >> I'm not totally sure switching global_var to a non-default feature is
>> >> the right answer here.
>> >
>> >> The --btf_features "default" set of options are meant to closely mirror
>> >> the upstream kernel options we enable when doing BTF encoding. However,
>> >> in scripts/Makefile.btf we don't use "default"; we explicitly call out
>> >> the set of features we want. We can't just use "default" in that context
>> >> since the meaning of "default" varies based upon whatever version of
>> >> pahole you have.
>> >
>> >> So "default" is simply a convenient shorthand for pahole testing which
>> >> corresponds to "give me the set of features that upstream kernels use".
>> >> It could have a better name that reflects that more clearly I suppose.
>> >
>> >> When we do switch this on in-kernel, we'll add the explicit "global_var"
>> >> to the list of features in scripts/Makefile.btf.
>> >
>> >> So with all this said, do we make global_vars a default or non-default
>> >> feature? It would seem to make sense to specify non-default, since it is
>> >> not switched on for the kernel yet, but looking ahead, what if the 1.28
>> >> pahole release is used to build vmlinux BTF and we add global_var to the
>> >> list of features? In such a case, our "default" set of values would be
>> >> out of step with the kernel. So it's not a huge deal, but I would
>> >> consider keeping this a default feature to facilitate testing; this
>> >> won't change what the kernel does, but it makes testing with full
>> >> variable generation easier (I can just do "--btf_features=default").
>> >
>> > This "default" really is confusing, as you spelled out above :-\
Yeah, I spent a while staring at the comment and reading the code to
understand the nuance between the initial and default values. I don't
think I fully understood it until this v3 patch, and admittedly I still
didn't have the full context of how "default" was used.
One interesting point of comparison is the "-M" argument to
"qemu-system-$arch". For example:
$ qemu-system-x86_64 -M ?
Supported machines are:
microvm microvm (i386)
pc Standard PC (i440FX + PIIX, 1996) (alias of pc-i440fx-9.0)
pc-i440fx-9.0 Standard PC (i440FX + PIIX, 1996) (default)
pc-i440fx-8.2 Standard PC (i440FX + PIIX, 1996)
pc-i440fx-8.1 Standard PC (i440FX + PIIX, 1996)
pc-i440fx-8.0 Standard PC (i440FX + PIIX, 1996)
[...]
So the default "pc" machine is simply an alias that gets updated to the
most recent machine (with potential new behaviors) every release, but
you can always select a specific machine that you care about.
Maybe it would make sense if there were versioned defaults, so that
"default" always picks whatever is relevant to the most recent upstream
kernel, but you could also select the default as of an older pahole
release.
That does sound like plenty of complexity added to an already somewhat
confusing system, so I'm not sold on it. The flexibility for adjusting
to new kernel defaults is appealing though.
>> >When to
>> > add something to it so that it reflects what the kernel has is tricky,
>> > perhaps we should instead have a ~/.config/pahole file where developers
>> > can add BTF features to add to --btf_features=default in the period
>> > where something new was _really_ added to the kernel and before the next
>> > version when it _have been added to the kernel set of BTF features_ thus
>> > should be set into stone in the pahole sources?
>
>> it's a nice idea; I suppose once we have more automated tests, this will
>> be less of an issue too. I'm looking at adding a BTF variable test
>> shortly, would be good to have coverage there too, especially since
>> we're growing the amount of info we encode in this area.
>
> Sure thing, the more tests, the better!
>
>> > So I think we should do as Stephen did, keep it out of
>> > --btf_features=default, as it is not yet in the kernel set of options,
>> > and have the config file, starting with being able to set those
>> > features, i.e. we would have:
>
>> > $ cat ~/.config/pahole
>> > [btf_encoder]
>> > btf_features=+global_var
>
>> > wdyt?
>
>> I think that makes perfect sense, great idea!
>
> I was looking for a library to do that to avoid "stealing" the
> perf-config code, but perhaps we should use an env var for that?
>
> PAHOLE_BTF_FEATURES='+global_var'
>
> To keep things simple?
One concern with configuration files is that (at least on my system)
they tend to sit around and get forgotten, unless they're super well
known configs like ~/.bashrc. So at some point, I could see myself
setting a pahole config and then 6 months later wondering why pahole
behaves differently on two different systems.
Env vars are easy to set permanently if you want, but are also more
visible and centralized with your other configurations, so they're my
preference.
Thanks,
Stephen
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH dwarves v3 5/5] pahole: add global_var BTF feature
2024-10-03 17:48 ` Stephen Brennan
@ 2024-10-03 18:33 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 20+ messages in thread
From: Arnaldo Carvalho de Melo @ 2024-10-03 18:33 UTC (permalink / raw)
To: Stephen Brennan; +Cc: Alan Maguire, bpf, dwarves, linux-debuggers
On Thu, Oct 03, 2024 at 10:48:23AM -0700, Stephen Brennan wrote:
> Arnaldo Carvalho de Melo <acme@kernel.org> writes:
> > On Thu, Oct 03, 2024 at 04:21:07PM +0100, Alan Maguire wrote:
> >> On 03/10/2024 15:53, Arnaldo Carvalho de Melo wrote:
> >> > On Thu, Oct 03, 2024 at 03:40:35PM +0100, Alan Maguire wrote:
> >> >> On 03/10/2024 00:52, Stephen Brennan wrote:
> >> >>> So far, pahole has only encoded type information for percpu variables.
> >> >>> However, there are several reasons why type information for all global
> >> >>> variables would be useful in the kernel:
> >> >
> >> >>> 1. Runtime kernel debuggers like drgn could use the BTF to introspect
> >> >>> kernel data structures without needing to install heavyweight DWARF.
> >> >
> >> >>> 2. BPF programs using the "__ksym" annotation could declare the
> >> >>> variables using the correct type, rather than "void".
> >> >
> >> >>> It makes sense to introduce a feature for this in pahole so that these
> >> >>> capabilities can be explored in the kernel. The feature is non-default:
> >> >>> when using "--btf-features=default", it is disabled. It must be
> >> >>> explicitly requested, e.g. with "--btf-features=+global_var".
> >> >
> >> >> I'm not totally sure switching global_var to a non-default feature is
> >> >> the right answer here.
> >> >
> >> >> The --btf_features "default" set of options are meant to closely mirror
> >> >> the upstream kernel options we enable when doing BTF encoding. However,
> >> >> in scripts/Makefile.btf we don't use "default"; we explicitly call out
> >> >> the set of features we want. We can't just use "default" in that context
> >> >> since the meaning of "default" varies based upon whatever version of
> >> >> pahole you have.
> >> >
> >> >> So "default" is simply a convenient shorthand for pahole testing which
> >> >> corresponds to "give me the set of features that upstream kernels use".
> >> >> It could have a better name that reflects that more clearly I suppose.
> >> >
> >> >> When we do switch this on in-kernel, we'll add the explicit "global_var"
> >> >> to the list of features in scripts/Makefile.btf.
> >> >
> >> >> So with all this said, do we make global_vars a default or non-default
> >> >> feature? It would seem to make sense to specify non-default, since it is
> >> >> not switched on for the kernel yet, but looking ahead, what if the 1.28
> >> >> pahole release is used to build vmlinux BTF and we add global_var to the
> >> >> list of features? In such a case, our "default" set of values would be
> >> >> out of step with the kernel. So it's not a huge deal, but I would
> >> >> consider keeping this a default feature to facilitate testing; this
> >> >> won't change what the kernel does, but it makes testing with full
> >> >> variable generation easier (I can just do "--btf_features=default").
> >> >
> >> > This "default" really is confusing, as you spelled out above :-\
>
> Yeah, I spent a while staring at the comment and reading the code to
> understand the nuance between the initial and default values. I don't
> think I fully understood it until this v3 patch, and admittedly I still
> didn't have the full context of how "default" was used.
>
> One interesting point of comparison is the "-M" argument to
> "qemu-system-$arch". For example:
>
> $ qemu-system-x86_64 -M ?
> Supported machines are:
> microvm microvm (i386)
> pc Standard PC (i440FX + PIIX, 1996) (alias of pc-i440fx-9.0)
> pc-i440fx-9.0 Standard PC (i440FX + PIIX, 1996) (default)
> pc-i440fx-8.2 Standard PC (i440FX + PIIX, 1996)
> pc-i440fx-8.1 Standard PC (i440FX + PIIX, 1996)
> pc-i440fx-8.0 Standard PC (i440FX + PIIX, 1996)
> [...]
>
> So the default "pc" machine is simply an alias that gets updated to the
> most recent machine (with potential new behaviors) every release, but
> you can always select a specific machine that you care about.
>
> Maybe it would make sense if there were versioned defaults, so that
> "default" always picks whatever is relevant to the most recent upstream
> kernel, but you could also select the default as of an older pahole
> release.
>
> That does sound like plenty of complexity added to an already somewhat
> confusing system, so I'm not sold on it. The flexibility for adjusting
> to new kernel defaults is appealing though.
>
> >> >When to
> >> > add something to it so that it reflects what the kernel has is tricky,
> >> > perhaps we should instead have a ~/.config/pahole file where developers
> >> > can add BTF features to add to --btf_features=default in the period
> >> > where something new was _really_ added to the kernel and before the next
> >> > version when it _have been added to the kernel set of BTF features_ thus
> >> > should be set into stone in the pahole sources?
> >
> >> it's a nice idea; I suppose once we have more automated tests, this will
> >> be less of an issue too. I'm looking at adding a BTF variable test
> >> shortly, would be good to have coverage there too, especially since
> >> we're growing the amount of info we encode in this area.
> >
> > Sure thing, the more tests, the better!
> >
> >> > So I think we should do as Stephen did, keep it out of
> >> > --btf_features=default, as it is not yet in the kernel set of options,
> >> > and have the config file, starting with being able to set those
> >> > features, i.e. we would have:
> >
> >> > $ cat ~/.config/pahole
> >> > [btf_encoder]
> >> > btf_features=+global_var
> >
> >> > wdyt?
> >
> >> I think that makes perfect sense, great idea!
> >
> > I was looking for a library to do that to avoid "stealing" the
> > perf-config code, but perhaps we should use an env var for that?
> >
> > PAHOLE_BTF_FEATURES='+global_var'
> >
> > To keep things simple?
>
> One concern with configuration files is that (at least on my system)
> they tend to sit around and get forgotten, unless they're super well
> known configs like ~/.bashrc. So at some point, I could see myself
> setting a pahole config and then 6 months later wondering why pahole
> behaves differently on two different systems.
>
> Env vars are easy to set permanently if you want, but are also more
> visible and centralized with your other configurations, so they're my
> preference.
Agreed, lets go with an env var.
And this one is even "ephemeral", i.e. as we get new versions of pahole
it should match the most recent set of kernel btf_features set and thus
become unneeded.
At some point we'll stop adding features, right? 8-)
- Arnaldo
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH dwarves v3 0/5] Emit global variables in BTF
2024-10-02 23:52 [PATCH dwarves v3 0/5] Emit global variables in BTF Stephen Brennan
` (4 preceding siblings ...)
2024-10-02 23:52 ` [PATCH dwarves v3 5/5] pahole: add global_var BTF feature Stephen Brennan
@ 2024-10-03 20:59 ` Jiri Olsa
5 siblings, 0 replies; 20+ messages in thread
From: Jiri Olsa @ 2024-10-03 20:59 UTC (permalink / raw)
To: Stephen Brennan
Cc: Arnaldo Carvalho de Melo, bpf, dwarves, linux-debuggers,
Alan Maguire
On Wed, Oct 02, 2024 at 04:52:42PM -0700, Stephen Brennan wrote:
> Hello all,
>
> This is v3 of the series to add global variables to pahole's generated BTF.
> Patches 1-3 of v2 were already merged. This series splits the last patch of v2
> and does some small updates. It should apply cleanly to the "next" branch.
> https://github.com/acmel/dwarves/commits/btf_global_vars/
>
> Changes since v2:
> 1. Split things out into several smaller patches as can be seen in the log
> below.
thanks for the split, so much better for review, lgtm
Acked-by: Jiri Olsa <jolsa@kernel.org>
thanks,
jirka
> 2. Previously the global_var feature was defined with BTF_DEFAULT_FEATURE, but I
> think we agreed in the discussion of v2 that it would be better as
> BTF_NON_DEFAULT_FEATURE, so I changed it to align with our discussion.
> 3. Removed the "--encode_btf_global_vars" option.
> 3. I went through and straightened out my use of integer types for ELF section
> index (size_t, as returned by libelf) as well as the variable addr and size.
> To this end I did add a few checks to explicitly ensure we don't overflow the
> uint32_t fields in the DATASEC.
>
> To test this out on a Linux build, you'll want to make the following change:
>
> ---------
> diff --git a/scripts/Makefile.btf b/scripts/Makefile.btf
> index b75f09f3f424..c88d9e526426 100644
> --- a/scripts/Makefile.btf
> +++ b/scripts/Makefile.btf
> @@ -19,7 +19,7 @@ pahole-flags-$(call test-ge, $(pahole-ver), 125) += --skip_encoding_btf_inconsis
> else
>
> # Switch to using --btf_features for v1.26 and later.
> -pahole-flags-$(call test-ge, $(pahole-ver), 126) = -j --btf_features=encode_force,var,float,enum64,decl_tag,type_tag,optimized_func,consistent_func,decl_tag_kfuncs
> +pahole-flags-$(call test-ge, $(pahole-ver), 126) = -j --btf_features=encode_force,var,float,enum64,decl_tag,type_tag,optimized_func,consistent_func,decl_tag_kfuncs,global_var
>
> ifneq ($(KBUILD_EXTMOD),)
> module-pahole-flags-$(call test-ge, $(pahole-ver), 126) += --btf_features=distilled_base
> ---------
>
> With a suitable kernel config that has BTF enabled, you could then build like
> so:
>
> PATH=path/to/pahole_build_dir make all
>
> And you'll be able to examine the size of the results with readelf, or dump the
> results with bpftool.
>
> v2: https://lore.kernel.org/dwarves/20240920081903.13473-1-stephen.s.brennan@oracle.com/T/
> v1: https://lore.kernel.org/dwarves/20240912190827.230176-1-stephen.s.brennan@oracle.com/
>
> Stephen Brennan (5):
> btf_encoder: use bitfield to control var encoding
> btf_encoder: stop indexing symbols for VARs
> btf_encoder: explicitly check addr/size for u32 overflow
> btf_encoder: allow encoding VARs from many sections
> pahole: add global_var BTF feature
>
> btf_encoder.c | 348 +++++++++++++++++++++------------------------
> btf_encoder.h | 7 +
> dwarves.h | 1 +
> man-pages/pahole.1 | 7 +-
> pahole.c | 3 +-
> 5 files changed, 178 insertions(+), 188 deletions(-)
>
> --
> 2.43.5
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2024-10-03 20:59 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-02 23:52 [PATCH dwarves v3 0/5] Emit global variables in BTF Stephen Brennan
2024-10-02 23:52 ` [PATCH dwarves v3 1/5] btf_encoder: use bitfield to control var encoding Stephen Brennan
2024-10-03 13:41 ` Alan Maguire
2024-10-03 14:41 ` Arnaldo Carvalho de Melo
2024-10-02 23:52 ` [PATCH dwarves v3 2/5] btf_encoder: stop indexing symbols for VARs Stephen Brennan
2024-10-03 13:59 ` Alan Maguire
2024-10-03 17:26 ` Stephen Brennan
2024-10-02 23:52 ` [PATCH dwarves v3 3/5] btf_encoder: explicitly check addr/size for u32 overflow Stephen Brennan
2024-10-03 14:19 ` Alan Maguire
2024-10-02 23:52 ` [PATCH dwarves v3 4/5] btf_encoder: allow encoding VARs from many sections Stephen Brennan
2024-10-03 14:52 ` Alan Maguire
2024-10-03 17:29 ` Stephen Brennan
2024-10-02 23:52 ` [PATCH dwarves v3 5/5] pahole: add global_var BTF feature Stephen Brennan
2024-10-03 14:40 ` Alan Maguire
2024-10-03 14:53 ` Arnaldo Carvalho de Melo
2024-10-03 15:21 ` Alan Maguire
2024-10-03 17:18 ` Arnaldo Carvalho de Melo
2024-10-03 17:48 ` Stephen Brennan
2024-10-03 18:33 ` Arnaldo Carvalho de Melo
2024-10-03 20:59 ` [PATCH dwarves v3 0/5] Emit global variables in BTF 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).