[PATCH dwarves v2 0/4] Emit global variables in BTF

[PATCH dwarves v2 0/4] Emit global variables in BTF

[Stephen Brennan]

Hello all,

This is v2 of the series to add global variables to pahole's generated BTF.
Please see v1's cover letter for more justification and background. I've
incorporated Alan's feedback and added a forgotten Cc for the bpf list.

Thanks,
Stephen

v1: https://lore.kernel.org/dwarves/20240912190827.230176-1-stephen.s.brennan@oracle.com/

Stephen Brennan (4):
  dutil: return ELF section name when looked up by index
  dwarf_loader: add "artificial" and "top_level" variable flags
  btf_encoder: cache all ELF section info
  btf_encoder: add global_var feature to encode globals

 btf_encoder.c      | 373 ++++++++++++++++++++++-----------------------
 btf_encoder.h      |   8 +
 dutil.c            |  14 +-
 dutil.h            |   2 +-
 dwarf_loader.c     |  12 +-
 dwarves.h          |   3 +
 man-pages/pahole.1 |   8 +-
 pahole.c           |  11 +-
 8 files changed, 228 insertions(+), 203 deletions(-)

-- 
2.43.5

[PATCH dwarves v2 1/4] dutil: return ELF section name when looked up by index

[Stephen Brennan]

Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
---
 dutil.c | 14 +++++++++++---
 dutil.h |  2 +-
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/dutil.c b/dutil.c
index 97c4474..14f1340 100644
--- a/dutil.c
+++ b/dutil.c
@@ -207,13 +207,21 @@ Elf_Scn *elf_section_by_name(Elf *elf, GElf_Shdr *shp, const char *name, size_t
 	return sec;
 }
 
-Elf_Scn *elf_section_by_idx(Elf *elf, GElf_Shdr *shp, int idx)
+Elf_Scn *elf_section_by_idx(Elf *elf, GElf_Shdr *shp, int idx, const char **name_out)
 {
 	Elf_Scn *sec;
+	size_t str_idx;
 
 	sec = elf_getscn(elf, idx);
-	if (sec)
-		gelf_getshdr(sec, shp);
+	if (!sec)
+		return NULL;
+	if (!gelf_getshdr(sec, shp))
+		return NULL;
+	if (name_out) {
+		if (elf_getshdrstrndx(elf, &str_idx))
+			return NULL;
+		*name_out = elf_strptr(elf, str_idx, shp->sh_name);
+	}
 	return sec;
 }
 
diff --git a/dutil.h b/dutil.h
index 335a17c..ff78aa6 100644
--- a/dutil.h
+++ b/dutil.h
@@ -328,7 +328,7 @@ void *zalloc(const size_t size);
 
 Elf_Scn *elf_section_by_name(Elf *elf, GElf_Shdr *shp, const char *name, size_t *index);
 
-Elf_Scn *elf_section_by_idx(Elf *elf, GElf_Shdr *shp, int idx);
+Elf_Scn *elf_section_by_idx(Elf *elf, GElf_Shdr *shp, int idx, const char **name_out);
 
 #ifndef SHT_GNU_ATTRIBUTES
 /* Just a way to check if we're using an old elfutils version */
-- 
2.43.5

[PATCH dwarves v2 2/4] dwarf_loader: add "artificial" and "top_level" variable flags

[Stephen Brennan]

The "artificial" flag corresponds directly to DW_AT_artificial, which
indicates a compiler-generated variable (e.g. __func__) which shouldn't
be included in the output.

The "top_level" flag is intended to be a better proxy for global scoped
variables. It indicates that a variable was a direct child of a
compilation unit, rather than a child of a subroutine or lexical block.
Currently, the DWARF loader examines the DWARF location expression, and
if the location is found to be at a constant memory address (not stack,
register, etc), then the variable is assumed to be globally scoped.
However, this includes a variety of variables that aren't truly globally
scoped: most commonly, static local variables of functions. Their
locations may be static, but they're not globally accessible in any
useful way.

These flags will be used by the BTF encoder to select global variables.

Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
---
 dwarf_loader.c | 12 +++++++-----
 dwarves.h      |  2 ++
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/dwarf_loader.c b/dwarf_loader.c
index 065ed4d..d162214 100644
--- a/dwarf_loader.c
+++ b/dwarf_loader.c
@@ -730,7 +730,7 @@ const char *variable__scope_str(const struct variable *var)
 	return "unknown";
 }
 
-static struct variable *variable__new(Dwarf_Die *die, struct cu *cu, struct conf_load *conf)
+static struct variable *variable__new(Dwarf_Die *die, struct cu *cu, struct conf_load *conf, int top_level)
 {
 	bool has_specification = dwarf_hasattr(die, DW_AT_specification);
 	struct variable *var = tag__alloc(cu, sizeof(*var));
@@ -743,6 +743,8 @@ static struct variable *variable__new(Dwarf_Die *die, struct cu *cu, struct conf
 		/* non-defining declaration of an object */
 		var->declaration = dwarf_hasattr(die, DW_AT_declaration);
 		var->has_specification = has_specification;
+		var->artificial = dwarf_hasattr(die, DW_AT_artificial);
+		var->top_level = top_level;
 		var->scope = VSCOPE_UNKNOWN;
 		INIT_LIST_HEAD(&var->annots);
 		var->ip.addr = 0;
@@ -1767,9 +1769,9 @@ static struct tag *die__create_new_label(Dwarf_Die *die,
 	return &label->ip.tag;
 }
 
-static struct tag *die__create_new_variable(Dwarf_Die *die, struct cu *cu, struct conf_load *conf)
+static struct tag *die__create_new_variable(Dwarf_Die *die, struct cu *cu, struct conf_load *conf, int top_level)
 {
-	struct variable *var = variable__new(die, cu, conf);
+	struct variable *var = variable__new(die, cu, conf, top_level);
 
 	if (var == NULL || add_child_llvm_annotations(die, -1, conf, &var->annots))
 		return NULL;
@@ -2243,7 +2245,7 @@ static int die__process_function(Dwarf_Die *die, struct ftype *ftype,
 			tag = die__create_new_parameter(die, ftype, lexblock, cu, conf, param_idx++);
 			break;
 		case DW_TAG_variable:
-			tag = die__create_new_variable(die, cu, conf);
+			tag = die__create_new_variable(die, cu, conf, 0);
 			if (tag == NULL)
 				goto out_enomem;
 			lexblock__add_variable(lexblock, tag__variable(tag));
@@ -2367,7 +2369,7 @@ static struct tag *__die__process_tag(Dwarf_Die *die, struct cu *cu,
 	case DW_TAG_union_type:
 		tag = die__create_new_union(die, cu, conf);	break;
 	case DW_TAG_variable:
-		tag = die__create_new_variable(die, cu, conf);	break;
+		tag = die__create_new_variable(die, cu, conf, top_level);	break;
 	case DW_TAG_constant: // First seen in a Go CU
 		tag = die__create_new_constant(die, cu, conf);	break;
 	default:
diff --git a/dwarves.h b/dwarves.h
index f2d3988..0fede91 100644
--- a/dwarves.h
+++ b/dwarves.h
@@ -848,6 +848,8 @@ struct variable {
 	uint8_t		 external:1;
 	uint8_t		 declaration:1;
 	uint8_t		 has_specification:1;
+	uint8_t		 artificial:1;
+	uint8_t		 top_level:1;
 	enum vscope	 scope;
 	struct location	 location;
 	struct hlist_node tool_hnode;
-- 
2.43.5

[PATCH dwarves v2 3/4] btf_encoder: cache all ELF section info

[Stephen Brennan]

To handle outputting all variables generally, we'll need to store more
section data. Create a table of ELF sections so we can refer to all the
cached data, not just the percpu section.

Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
---
 btf_encoder.c | 50 ++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 36 insertions(+), 14 deletions(-)

diff --git a/btf_encoder.c b/btf_encoder.c
index 8a2d92e..97d35e0 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -71,6 +71,12 @@ struct var_info {
 	uint32_t    sz;
 };
 
+struct elf_secinfo {
+	uint64_t    addr;
+	const char *name;
+	uint64_t    sz;
+};
+
 /*
  * cu: cu being processed.
  */
@@ -95,13 +101,13 @@ struct btf_encoder {
 			  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;
-		uint64_t	base_addr;
-		uint64_t	sec_sz;
 	} percpu;
 	struct {
 		struct elf_function *entries;
@@ -1849,7 +1855,7 @@ static int btf_encoder__collect_percpu_var(struct btf_encoder *encoder, GElf_Sym
 	 * ET_EXEC file) we need to subtract the section address.
 	 */
 	if (!encoder->is_rel)
-		addr -= encoder->percpu.base_addr;
+		addr -= encoder->secinfo[encoder->percpu.shndx].addr;
 
 	if (encoder->percpu.var_cnt == encoder->percpu.allocated) {
 		struct var_info *new;
@@ -1923,6 +1929,7 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder)
 	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)
 		return 0;
@@ -1954,9 +1961,9 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder)
 		 * always contains virtual symbol addresses, so subtract
 		 * the section address unconditionally.
 		 */
-		if (addr < encoder->percpu.base_addr || addr >= encoder->percpu.base_addr + encoder->percpu.sec_sz)
+		if (addr < pcpu_scn->addr || addr >= pcpu_scn->addr + pcpu_scn->sz)
 			continue;
-		addr -= encoder->percpu.base_addr;
+		addr -= pcpu_scn->addr;
 
 		if (!btf_encoder__percpu_var_exists(encoder, addr, &size, &name))
 			continue; /* not a per-CPU variable */
@@ -2099,20 +2106,35 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
 			goto out;
 		}
 
-		/* find percpu section's shndx */
+		/* index the ELF sections for later lookup */
 
 		GElf_Shdr shdr;
-		Elf_Scn *sec = elf_section_by_name(cu->elf, &shdr, PERCPU_SECTION, NULL);
+		size_t shndx;
+		if (elf_getshdrnum(cu->elf, &encoder->seccnt))
+			goto out_delete;
+		encoder->secinfo = calloc(encoder->seccnt, sizeof(*encoder->secinfo));
+		if (!encoder->secinfo) {
+			fprintf(stderr, "%s: error allocating memory for %zu ELF sections\n",
+				__func__, encoder->seccnt);
+			goto out_delete;
+		}
 
-		if (!sec) {
-			if (encoder->verbose)
-				printf("%s: '%s' doesn't have '%s' section\n", __func__, cu->filename, PERCPU_SECTION);
-		} else {
-			encoder->percpu.shndx	  = elf_ndxscn(sec);
-			encoder->percpu.base_addr = shdr.sh_addr;
-			encoder->percpu.sec_sz	  = shdr.sh_size;
+		for (shndx = 0; shndx < encoder->seccnt; shndx++) {
+			const char *secname = NULL;
+			Elf_Scn *sec = elf_section_by_idx(cu->elf, &shdr, shndx, &secname);
+			if (!sec)
+				goto out_delete;
+			encoder->secinfo[shndx].addr = shdr.sh_addr;
+			encoder->secinfo[shndx].sz = shdr.sh_size;
+			encoder->secinfo[shndx].name = secname;
+
+			if (strcmp(secname, PERCPU_SECTION) == 0)
+				encoder->percpu.shndx = shndx;
 		}
 
+		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))
 			goto out_delete;
 
-- 
2.43.5

[PATCH dwarves v2 4/4] btf_encoder: add global_var feature to encode globals

[Stephen Brennan]

Currently the "var" feature only encodes percpu variables. Add the
ability to encode all global variables.

This also drops the use of the symbol table to find variable names and
compare them against DWARF names. We simply rely on the DWARF
information to give us all the variables.

Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
Tested-by: Alan Maguire <alan.maguire@oracle.com>
---
 btf_encoder.c      | 347 +++++++++++++++++++++------------------------
 btf_encoder.h      |   8 ++
 dwarves.h          |   1 +
 man-pages/pahole.1 |   8 +-
 pahole.c           |  11 +-
 5 files changed, 183 insertions(+), 192 deletions(-)

diff --git a/btf_encoder.c b/btf_encoder.c
index 97d35e0..d3a66a0 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -65,16 +65,13 @@ struct elf_function {
 	struct btf_encoder_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;
+	bool        include;
+	uint32_t    type;
+	struct gobuffer secinfo;
 };
 
 /*
@@ -84,31 +81,24 @@ 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;
 	uint32_t	  type_id_off;
 	bool		  has_index_type,
 			  need_index_type,
-			  skip_encoding_vars,
 			  raw_output,
 			  verbose,
 			  force,
 			  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;
+	int                encode_vars;
+	uint32_t           percpu_shndx;
 	struct {
 		struct elf_function *entries;
 		int		    allocated;
@@ -735,46 +725,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, int shndx,
+					    uint32_t type, unsigned long addr, uint32_t size)
 {
 	struct btf_var_secinfo si = {
 		.type = type,
-		.offset = offset,
+		.offset = addr,
 		.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;
-
+	int 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 %d: \"%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, int 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);
@@ -1741,13 +1741,14 @@ out:
 int btf_encoder__encode(struct btf_encoder *encoder)
 {
 	bool should_tag_kfuncs;
-	int err;
+	int err, 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)
@@ -1797,111 +1798,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);
@@ -1923,75 +1831,128 @@ 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->symtab)
 		return 0;
 
 	if (encoder->verbose)
-		printf("search cu '%s' for percpu global variables.\n", cu->name);
+		printf("search cu '%s' for global variables.\n", cu->name);
 
 	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;
+		int shndx;
+		struct elf_secinfo *sec = NULL;
 		uint64_t addr;
 		int id;
+		unsigned long size;
 
+		/* 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 >= 0 && shndx < encoder->seccnt)
+			sec = &encoder->secinfo[shndx];
+		if (!sec || !sec->include)
 			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 -= sec->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",
@@ -2003,9 +1964,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 variable '%s'...\n", name ?: "<missing name>");
 			continue;
 		}
 
@@ -2035,13 +1997,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, addr, size);
+		id = btf_encoder__add_var_secinfo(encoder, shndx, id, addr, 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;
 		}
 	}
@@ -2068,7 +2031,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;
@@ -2076,6 +2038,11 @@ 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;
+		if (conf_load->encode_btf_global_vars)
+			encoder->encode_vars |= BTF_VAR_GLOBAL;
 
 		GElf_Ehdr ehdr;
 
@@ -2085,8 +2052,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);
@@ -2127,15 +2092,21 @@ 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;
-
-			if (strcmp(secname, PERCPU_SECTION) == 0)
-				encoder->percpu.shndx = shndx;
+			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) {
+				encoder->percpu_shndx = shndx;
+				if (encoder->encode_vars & BTF_VAR_PERCPU)
+					encoder->secinfo[shndx].include = true;
+			}
 		}
 
-		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->skip_encoding_vars))
+		if (btf_encoder__collect_symbols(encoder))
 			goto out_delete;
 
 		if (encoder->verbose)
@@ -2156,7 +2127,8 @@ void btf_encoder__delete(struct btf_encoder *encoder)
 		return;
 
 	btf_encoders__delete(encoder);
-	__gobuffer__delete(&encoder->percpu_secinfo);
+	for (int i = 0; i < encoder->seccnt; i++)
+		__gobuffer__delete(&encoder->secinfo[i].secinfo);
 	zfree(&encoder->filename);
 	zfree(&encoder->source_filename);
 	btf__free(encoder->btf);
@@ -2166,9 +2138,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);
 }
@@ -2321,7 +2290,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);
 
 	/* It is only safe to delete this CU if we have not stashed any static
diff --git a/btf_encoder.h b/btf_encoder.h
index f54c95a..5e4d53a 100644
--- a/btf_encoder.h
+++ b/btf_encoder.h
@@ -16,7 +16,15 @@ 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,
+	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);
+
 void btf_encoder__delete(struct btf_encoder *encoder);
 
 int btf_encoder__encode(struct btf_encoder *encoder);
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 0a9d8ac..4bc2d03 100644
--- a/man-pages/pahole.1
+++ b/man-pages/pahole.1
@@ -230,7 +230,10 @@ the debugging information.
 
 .TP
 .B \-\-skip_encoding_btf_vars
-Do not encode VARs in BTF.
+.TQ
+.B \-\-encode_btf_global_vars
+By default, VARs are encoded only for percpu variables. These options allow
+to skip encoding them, or alternatively to encode all global variables too.
 
 .TP
 .B \-\-skip_encoding_btf_decl_tag
@@ -296,7 +299,8 @@ 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.
+	global_var         Encode all global variables in the same way.
 	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.
diff --git a/pahole.c b/pahole.c
index a33490d..b123be1 100644
--- a/pahole.c
+++ b/pahole.c
@@ -1239,6 +1239,7 @@ ARGP_PROGRAM_VERSION_HOOK_DEF = dwarves_print_version;
 #define ARGP_contains_enumerator 344
 #define ARGP_reproducible_build 345
 #define ARGP_running_kernel_vmlinux 346
+#define ARGP_encode_btf_global_vars 347
 
 /* --btf_features=feature1[,feature2,..] allows us to specify
  * a list of requested BTF features or "default" to enable all default
@@ -1285,6 +1286,7 @@ struct btf_feature {
 } btf_features[] = {
 	BTF_DEFAULT_FEATURE(encode_force, btf_encode_force, false),
 	BTF_DEFAULT_FEATURE(var, skip_encoding_btf_vars, true),
+	BTF_DEFAULT_FEATURE(global_var, encode_btf_global_vars, false),
 	BTF_DEFAULT_FEATURE(float, btf_gen_floats, false),
 	BTF_DEFAULT_FEATURE(decl_tag, skip_encoding_btf_decl_tag, true),
 	BTF_DEFAULT_FEATURE(type_tag, skip_encoding_btf_type_tag, true),
@@ -1720,7 +1722,12 @@ 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 = "encode_btf_global_vars",
+		.key  = ARGP_skip_encoding_btf_vars,
+		.doc  = "Encode all global VARs in BTF [if this is not specified, only percpu variables are encoded]."
 	},
 	{
 		.name = "btf_encode_force",
@@ -2047,6 +2054,8 @@ static error_t pahole__options_parser(int key, char *arg,
 		show_supported_btf_features(stdout);	exit(0);
 	case ARGP_btf_features_strict:
 		parse_btf_features(arg, true);		break;
+	case ARGP_encode_btf_global_vars:
+		conf_load.encode_btf_global_vars = true;	break;
 	default:
 		return ARGP_ERR_UNKNOWN;
 	}
-- 
2.43.5

Re: [PATCH dwarves v2 4/4] btf_encoder: add global_var feature to encode globals

[Arnaldo Carvalho de Melo]

On Fri, Sep 20, 2024 at 01:19:01AM -0700, Stephen Brennan wrote:
> Currently the "var" feature only encodes percpu variables. Add the
> ability to encode all global variables.
> 
> This also drops the use of the symbol table to find variable names and
> compare them against DWARF names. We simply rely on the DWARF
> information to give us all the variables.

I applied the three first patches to the next branch that soon will move
to master, but the last patch I think does too many things and ends up
being too big.

For instance, you could have done the btf_encoder->skip_encoding_vars
transformation into a bitfield in a separate, prep patch, also you
mentions "this also drops the use of the symbol table", can this be made
a separate, prep patch?

There was a conflict with some new options I added (--padding,
--padding_ge) and I fixed that up and made the series available in the
btf_global_vars branch, can you please go from there and split the last
patch into smaller chunks?

Thanks for your work on this! I noticed that this is not the default,
i.e. one has to explicitely opt in to have the global variables encoded
in BTF, so that would be interesting to have spelled out in the chunked
out patch that introduces the feature, etc.

Also since we have it as a feature and can ask for global variables
using --btf_features=global_var, I don't think we need
--encode_btf_global_vars, right?

That will also make the patch smaller, and even if it was required, that
would be something to have in a separate patch.

- Arnaldo
 
> Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
> Tested-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>  btf_encoder.c      | 347 +++++++++++++++++++++------------------------
>  btf_encoder.h      |   8 ++
>  dwarves.h          |   1 +
>  man-pages/pahole.1 |   8 +-
>  pahole.c           |  11 +-
>  5 files changed, 183 insertions(+), 192 deletions(-)
> 
> diff --git a/btf_encoder.c b/btf_encoder.c
> index 97d35e0..d3a66a0 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -65,16 +65,13 @@ struct elf_function {
>  	struct btf_encoder_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;
> +	bool        include;
> +	uint32_t    type;
> +	struct gobuffer secinfo;
>  };
>  
>  /*
> @@ -84,31 +81,24 @@ 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;
>  	uint32_t	  type_id_off;
>  	bool		  has_index_type,
>  			  need_index_type,
> -			  skip_encoding_vars,
>  			  raw_output,
>  			  verbose,
>  			  force,
>  			  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;
> +	int                encode_vars;
> +	uint32_t           percpu_shndx;
>  	struct {
>  		struct elf_function *entries;
>  		int		    allocated;
> @@ -735,46 +725,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, int shndx,
> +					    uint32_t type, unsigned long addr, uint32_t size)
>  {
>  	struct btf_var_secinfo si = {
>  		.type = type,
> -		.offset = offset,
> +		.offset = addr,
>  		.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;
> -
> +	int 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 %d: \"%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, int 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);
> @@ -1741,13 +1741,14 @@ out:
>  int btf_encoder__encode(struct btf_encoder *encoder)
>  {
>  	bool should_tag_kfuncs;
> -	int err;
> +	int err, 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)
> @@ -1797,111 +1798,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);
> @@ -1923,75 +1831,128 @@ 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->symtab)
>  		return 0;
>  
>  	if (encoder->verbose)
> -		printf("search cu '%s' for percpu global variables.\n", cu->name);
> +		printf("search cu '%s' for global variables.\n", cu->name);
>  
>  	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;
> +		int shndx;
> +		struct elf_secinfo *sec = NULL;
>  		uint64_t addr;
>  		int id;
> +		unsigned long size;
>  
> +		/* 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 >= 0 && shndx < encoder->seccnt)
> +			sec = &encoder->secinfo[shndx];
> +		if (!sec || !sec->include)
>  			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 -= sec->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",
> @@ -2003,9 +1964,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 variable '%s'...\n", name ?: "<missing name>");
>  			continue;
>  		}
>  
> @@ -2035,13 +1997,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, addr, size);
> +		id = btf_encoder__add_var_secinfo(encoder, shndx, id, addr, 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;
>  		}
>  	}
> @@ -2068,7 +2031,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;
> @@ -2076,6 +2038,11 @@ 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;
> +		if (conf_load->encode_btf_global_vars)
> +			encoder->encode_vars |= BTF_VAR_GLOBAL;
>  
>  		GElf_Ehdr ehdr;
>  
> @@ -2085,8 +2052,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);
> @@ -2127,15 +2092,21 @@ 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;
> -
> -			if (strcmp(secname, PERCPU_SECTION) == 0)
> -				encoder->percpu.shndx = shndx;
> +			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) {
> +				encoder->percpu_shndx = shndx;
> +				if (encoder->encode_vars & BTF_VAR_PERCPU)
> +					encoder->secinfo[shndx].include = true;
> +			}
>  		}
>  
> -		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->skip_encoding_vars))
> +		if (btf_encoder__collect_symbols(encoder))
>  			goto out_delete;
>  
>  		if (encoder->verbose)
> @@ -2156,7 +2127,8 @@ void btf_encoder__delete(struct btf_encoder *encoder)
>  		return;
>  
>  	btf_encoders__delete(encoder);
> -	__gobuffer__delete(&encoder->percpu_secinfo);
> +	for (int i = 0; i < encoder->seccnt; i++)
> +		__gobuffer__delete(&encoder->secinfo[i].secinfo);
>  	zfree(&encoder->filename);
>  	zfree(&encoder->source_filename);
>  	btf__free(encoder->btf);
> @@ -2166,9 +2138,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);
>  }
> @@ -2321,7 +2290,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);
>  
>  	/* It is only safe to delete this CU if we have not stashed any static
> diff --git a/btf_encoder.h b/btf_encoder.h
> index f54c95a..5e4d53a 100644
> --- a/btf_encoder.h
> +++ b/btf_encoder.h
> @@ -16,7 +16,15 @@ 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,
> +	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);
> +
>  void btf_encoder__delete(struct btf_encoder *encoder);
>  
>  int btf_encoder__encode(struct btf_encoder *encoder);
> 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 0a9d8ac..4bc2d03 100644
> --- a/man-pages/pahole.1
> +++ b/man-pages/pahole.1
> @@ -230,7 +230,10 @@ the debugging information.
>  
>  .TP
>  .B \-\-skip_encoding_btf_vars
> -Do not encode VARs in BTF.
> +.TQ
> +.B \-\-encode_btf_global_vars
> +By default, VARs are encoded only for percpu variables. These options allow
> +to skip encoding them, or alternatively to encode all global variables too.
>  
>  .TP
>  .B \-\-skip_encoding_btf_decl_tag
> @@ -296,7 +299,8 @@ 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.
> +	global_var         Encode all global variables in the same way.
>  	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.
> diff --git a/pahole.c b/pahole.c
> index a33490d..b123be1 100644
> --- a/pahole.c
> +++ b/pahole.c
> @@ -1239,6 +1239,7 @@ ARGP_PROGRAM_VERSION_HOOK_DEF = dwarves_print_version;
>  #define ARGP_contains_enumerator 344
>  #define ARGP_reproducible_build 345
>  #define ARGP_running_kernel_vmlinux 346
> +#define ARGP_encode_btf_global_vars 347
>  
>  /* --btf_features=feature1[,feature2,..] allows us to specify
>   * a list of requested BTF features or "default" to enable all default
> @@ -1285,6 +1286,7 @@ struct btf_feature {
>  } btf_features[] = {
>  	BTF_DEFAULT_FEATURE(encode_force, btf_encode_force, false),
>  	BTF_DEFAULT_FEATURE(var, skip_encoding_btf_vars, true),
> +	BTF_DEFAULT_FEATURE(global_var, encode_btf_global_vars, false),
>  	BTF_DEFAULT_FEATURE(float, btf_gen_floats, false),
>  	BTF_DEFAULT_FEATURE(decl_tag, skip_encoding_btf_decl_tag, true),
>  	BTF_DEFAULT_FEATURE(type_tag, skip_encoding_btf_type_tag, true),
> @@ -1720,7 +1722,12 @@ 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 = "encode_btf_global_vars",
> +		.key  = ARGP_skip_encoding_btf_vars,
> +		.doc  = "Encode all global VARs in BTF [if this is not specified, only percpu variables are encoded]."
>  	},
>  	{
>  		.name = "btf_encode_force",
> @@ -2047,6 +2054,8 @@ static error_t pahole__options_parser(int key, char *arg,
>  		show_supported_btf_features(stdout);	exit(0);
>  	case ARGP_btf_features_strict:
>  		parse_btf_features(arg, true);		break;
> +	case ARGP_encode_btf_global_vars:
> +		conf_load.encode_btf_global_vars = true;	break;
>  	default:
>  		return ARGP_ERR_UNKNOWN;
>  	}
> -- 
> 2.43.5

Re: [PATCH dwarves v2 4/4] btf_encoder: add global_var feature to encode globals

[Andrii Nakryiko]

On Tue, Oct 1, 2024 at 8:10 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>
> On Fri, Sep 20, 2024 at 01:19:01AM -0700, Stephen Brennan wrote:
> > Currently the "var" feature only encodes percpu variables. Add the
> > ability to encode all global variables.
> >
> > This also drops the use of the symbol table to find variable names and
> > compare them against DWARF names. We simply rely on the DWARF
> > information to give us all the variables.
>
> I applied the three first patches to the next branch that soon will move
> to master, but the last patch I think does too many things and ends up
> being too big.
>
> For instance, you could have done the btf_encoder->skip_encoding_vars
> transformation into a bitfield in a separate, prep patch, also you
> mentions "this also drops the use of the symbol table", can this be made
> a separate, prep patch?
>
> There was a conflict with some new options I added (--padding,
> --padding_ge) and I fixed that up and made the series available in the
> btf_global_vars branch, can you please go from there and split the last
> patch into smaller chunks?
>
> Thanks for your work on this! I noticed that this is not the default,
> i.e. one has to explicitely opt in to have the global variables encoded
> in BTF, so that would be interesting to have spelled out in the chunked
> out patch that introduces the feature, etc.

We probably shouldn't enable this option in kernel build until we work
out details of loading vmlinux BTF(s) through the kernel module.

>
> Also since we have it as a feature and can ask for global variables
> using --btf_features=global_var, I don't think we need
> --encode_btf_global_vars, right?
>
> That will also make the patch smaller, and even if it was required, that
> would be something to have in a separate patch.
>
> - Arnaldo
>
> > Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
> > Tested-by: Alan Maguire <alan.maguire@oracle.com>
> > ---
> >  btf_encoder.c      | 347 +++++++++++++++++++++------------------------
> >  btf_encoder.h      |   8 ++
> >  dwarves.h          |   1 +
> >  man-pages/pahole.1 |   8 +-
> >  pahole.c           |  11 +-
> >  5 files changed, 183 insertions(+), 192 deletions(-)
> >

[...]

Re: [PATCH dwarves v2 4/4] btf_encoder: add global_var feature to encode globals

[Arnaldo Carvalho de Melo]

On Tue, Oct 01, 2024 at 10:13:29AM -0700, Andrii Nakryiko wrote:
> On Tue, Oct 1, 2024 at 8:10 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > On Fri, Sep 20, 2024 at 01:19:01AM -0700, Stephen Brennan wrote:
> > > Currently the "var" feature only encodes percpu variables. Add the
> > > ability to encode all global variables.
> > >
> > > This also drops the use of the symbol table to find variable names and
> > > compare them against DWARF names. We simply rely on the DWARF
> > > information to give us all the variables.
> >
> > I applied the three first patches to the next branch that soon will move
> > to master, but the last patch I think does too many things and ends up
> > being too big.
> >
> > For instance, you could have done the btf_encoder->skip_encoding_vars
> > transformation into a bitfield in a separate, prep patch, also you
> > mentions "this also drops the use of the symbol table", can this be made
> > a separate, prep patch?
> >
> > There was a conflict with some new options I added (--padding,
> > --padding_ge) and I fixed that up and made the series available in the
> > btf_global_vars branch, can you please go from there and split the last
> > patch into smaller chunks?
> >
> > Thanks for your work on this! I noticed that this is not the default,
> > i.e. one has to explicitely opt in to have the global variables encoded
> > in BTF, so that would be interesting to have spelled out in the chunked
> > out patch that introduces the feature, etc.
> 
> We probably shouldn't enable this option in kernel build until we work
> out details of loading vmlinux BTF(s) through the kernel module.

Sure, this should be completely opt-in, and for kernel features, even
for documentational purposes, we need to enable it via --btf_features in
the Kbuild files, etc.

But with the feature in pahole we can go on experimenting with it, etc.

- Arnaldo
 
> > Also since we have it as a feature and can ask for global variables
> > using --btf_features=global_var, I don't think we need
> > --encode_btf_global_vars, right?

> > That will also make the patch smaller, and even if it was required, that
> > would be something to have in a separate patch.

Re: [PATCH dwarves v2 4/4] btf_encoder: add global_var feature to encode globals

[Stephen Brennan]

Arnaldo Carvalho de Melo <acme@kernel.org> writes:
> On Fri, Sep 20, 2024 at 01:19:01AM -0700, Stephen Brennan wrote:
>> Currently the "var" feature only encodes percpu variables. Add the
>> ability to encode all global variables.
>> 
>> This also drops the use of the symbol table to find variable names and
>> compare them against DWARF names. We simply rely on the DWARF
>> information to give us all the variables.
>
> I applied the three first patches to the next branch that soon will move
> to master,

Thanks!

> but the last patch I think does too many things and ends up
> being too big.
>
> For instance, you could have done the btf_encoder->skip_encoding_vars
> transformation into a bitfield in a separate, prep patch, also you
> mentions "this also drops the use of the symbol table", can this be made
> a separate, prep patch?
>
> There was a conflict with some new options I added (--padding,
> --padding_ge) and I fixed that up and made the series available in the
> btf_global_vars branch, can you please go from there and split the last
> patch into smaller chunks?

Yes, and thanks for staging it all up, I'll work from there.

> Thanks for your work on this! I noticed that this is not the default,
> i.e. one has to explicitely opt in to have the global variables encoded
> in BTF, so that would be interesting to have spelled out in the chunked
> out patch that introduces the feature, etc.

Agreed. While I wish it could be the default, best to keep it opt-in so
we can explore how best to integrate it. I'll be sure to document it
better in the commit body.

> Also since we have it as a feature and can ask for global variables
> using --btf_features=global_var, I don't think we need
> --encode_btf_global_vars, right?

I had included the option because most of the other BTF features had CLI
options corresponding to them. I couldn't tell if that was desired, or
if it was just for backward compatibility. Sounds like going forward,
BTF features don't need a CLI argument so I'm happy to remove it.

Thanks,
Stephen

> That will also make the patch smaller, and even if it was required, that
> would be something to have in a separate patch.
>
> - Arnaldo
>  
>> Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
>> Tested-by: Alan Maguire <alan.maguire@oracle.com>
>> ---
>>  btf_encoder.c      | 347 +++++++++++++++++++++------------------------
>>  btf_encoder.h      |   8 ++
>>  dwarves.h          |   1 +
>>  man-pages/pahole.1 |   8 +-
>>  pahole.c           |  11 +-
>>  5 files changed, 183 insertions(+), 192 deletions(-)
>> 
>> diff --git a/btf_encoder.c b/btf_encoder.c
>> index 97d35e0..d3a66a0 100644
>> --- a/btf_encoder.c
>> +++ b/btf_encoder.c
>> @@ -65,16 +65,13 @@ struct elf_function {
>>  	struct btf_encoder_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;
>> +	bool        include;
>> +	uint32_t    type;
>> +	struct gobuffer secinfo;
>>  };
>>  
>>  /*
>> @@ -84,31 +81,24 @@ 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;
>>  	uint32_t	  type_id_off;
>>  	bool		  has_index_type,
>>  			  need_index_type,
>> -			  skip_encoding_vars,
>>  			  raw_output,
>>  			  verbose,
>>  			  force,
>>  			  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;
>> +	int                encode_vars;
>> +	uint32_t           percpu_shndx;
>>  	struct {
>>  		struct elf_function *entries;
>>  		int		    allocated;
>> @@ -735,46 +725,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, int shndx,
>> +					    uint32_t type, unsigned long addr, uint32_t size)
>>  {
>>  	struct btf_var_secinfo si = {
>>  		.type = type,
>> -		.offset = offset,
>> +		.offset = addr,
>>  		.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;
>> -
>> +	int 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 %d: \"%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, int 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);
>> @@ -1741,13 +1741,14 @@ out:
>>  int btf_encoder__encode(struct btf_encoder *encoder)
>>  {
>>  	bool should_tag_kfuncs;
>> -	int err;
>> +	int err, 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)
>> @@ -1797,111 +1798,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);
>> @@ -1923,75 +1831,128 @@ 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->symtab)
>>  		return 0;
>>  
>>  	if (encoder->verbose)
>> -		printf("search cu '%s' for percpu global variables.\n", cu->name);
>> +		printf("search cu '%s' for global variables.\n", cu->name);
>>  
>>  	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;
>> +		int shndx;
>> +		struct elf_secinfo *sec = NULL;
>>  		uint64_t addr;
>>  		int id;
>> +		unsigned long size;
>>  
>> +		/* 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 >= 0 && shndx < encoder->seccnt)
>> +			sec = &encoder->secinfo[shndx];
>> +		if (!sec || !sec->include)
>>  			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 -= sec->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",
>> @@ -2003,9 +1964,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 variable '%s'...\n", name ?: "<missing name>");
>>  			continue;
>>  		}
>>  
>> @@ -2035,13 +1997,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, addr, size);
>> +		id = btf_encoder__add_var_secinfo(encoder, shndx, id, addr, 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;
>>  		}
>>  	}
>> @@ -2068,7 +2031,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;
>> @@ -2076,6 +2038,11 @@ 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;
>> +		if (conf_load->encode_btf_global_vars)
>> +			encoder->encode_vars |= BTF_VAR_GLOBAL;
>>  
>>  		GElf_Ehdr ehdr;
>>  
>> @@ -2085,8 +2052,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);
>> @@ -2127,15 +2092,21 @@ 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;
>> -
>> -			if (strcmp(secname, PERCPU_SECTION) == 0)
>> -				encoder->percpu.shndx = shndx;
>> +			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) {
>> +				encoder->percpu_shndx = shndx;
>> +				if (encoder->encode_vars & BTF_VAR_PERCPU)
>> +					encoder->secinfo[shndx].include = true;
>> +			}
>>  		}
>>  
>> -		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->skip_encoding_vars))
>> +		if (btf_encoder__collect_symbols(encoder))
>>  			goto out_delete;
>>  
>>  		if (encoder->verbose)
>> @@ -2156,7 +2127,8 @@ void btf_encoder__delete(struct btf_encoder *encoder)
>>  		return;
>>  
>>  	btf_encoders__delete(encoder);
>> -	__gobuffer__delete(&encoder->percpu_secinfo);
>> +	for (int i = 0; i < encoder->seccnt; i++)
>> +		__gobuffer__delete(&encoder->secinfo[i].secinfo);
>>  	zfree(&encoder->filename);
>>  	zfree(&encoder->source_filename);
>>  	btf__free(encoder->btf);
>> @@ -2166,9 +2138,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);
>>  }
>> @@ -2321,7 +2290,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);
>>  
>>  	/* It is only safe to delete this CU if we have not stashed any static
>> diff --git a/btf_encoder.h b/btf_encoder.h
>> index f54c95a..5e4d53a 100644
>> --- a/btf_encoder.h
>> +++ b/btf_encoder.h
>> @@ -16,7 +16,15 @@ 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,
>> +	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);
>> +
>>  void btf_encoder__delete(struct btf_encoder *encoder);
>>  
>>  int btf_encoder__encode(struct btf_encoder *encoder);
>> 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 0a9d8ac..4bc2d03 100644
>> --- a/man-pages/pahole.1
>> +++ b/man-pages/pahole.1
>> @@ -230,7 +230,10 @@ the debugging information.
>>  
>>  .TP
>>  .B \-\-skip_encoding_btf_vars
>> -Do not encode VARs in BTF.
>> +.TQ
>> +.B \-\-encode_btf_global_vars
>> +By default, VARs are encoded only for percpu variables. These options allow
>> +to skip encoding them, or alternatively to encode all global variables too.
>>  
>>  .TP
>>  .B \-\-skip_encoding_btf_decl_tag
>> @@ -296,7 +299,8 @@ 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.
>> +	global_var         Encode all global variables in the same way.
>>  	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.
>> diff --git a/pahole.c b/pahole.c
>> index a33490d..b123be1 100644
>> --- a/pahole.c
>> +++ b/pahole.c
>> @@ -1239,6 +1239,7 @@ ARGP_PROGRAM_VERSION_HOOK_DEF = dwarves_print_version;
>>  #define ARGP_contains_enumerator 344
>>  #define ARGP_reproducible_build 345
>>  #define ARGP_running_kernel_vmlinux 346
>> +#define ARGP_encode_btf_global_vars 347
>>  
>>  /* --btf_features=feature1[,feature2,..] allows us to specify
>>   * a list of requested BTF features or "default" to enable all default
>> @@ -1285,6 +1286,7 @@ struct btf_feature {
>>  } btf_features[] = {
>>  	BTF_DEFAULT_FEATURE(encode_force, btf_encode_force, false),
>>  	BTF_DEFAULT_FEATURE(var, skip_encoding_btf_vars, true),
>> +	BTF_DEFAULT_FEATURE(global_var, encode_btf_global_vars, false),
>>  	BTF_DEFAULT_FEATURE(float, btf_gen_floats, false),
>>  	BTF_DEFAULT_FEATURE(decl_tag, skip_encoding_btf_decl_tag, true),
>>  	BTF_DEFAULT_FEATURE(type_tag, skip_encoding_btf_type_tag, true),
>> @@ -1720,7 +1722,12 @@ 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 = "encode_btf_global_vars",
>> +		.key  = ARGP_skip_encoding_btf_vars,
>> +		.doc  = "Encode all global VARs in BTF [if this is not specified, only percpu variables are encoded]."
>>  	},
>>  	{
>>  		.name = "btf_encode_force",
>> @@ -2047,6 +2054,8 @@ static error_t pahole__options_parser(int key, char *arg,
>>  		show_supported_btf_features(stdout);	exit(0);
>>  	case ARGP_btf_features_strict:
>>  		parse_btf_features(arg, true);		break;
>> +	case ARGP_encode_btf_global_vars:
>> +		conf_load.encode_btf_global_vars = true;	break;
>>  	default:
>>  		return ARGP_ERR_UNKNOWN;
>>  	}
>> -- 
>> 2.43.5

Re: [PATCH dwarves v2 4/4] btf_encoder: add global_var feature to encode globals

[Jiri Olsa]

On Fri, Sep 20, 2024 at 01:19:01AM -0700, Stephen Brennan wrote:
> Currently the "var" feature only encodes percpu variables. Add the
> ability to encode all global variables.
> 
> This also drops the use of the symbol table to find variable names and
> compare them against DWARF names. We simply rely on the DWARF
> information to give us all the variables.
> 
> Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
> Tested-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>  btf_encoder.c      | 347 +++++++++++++++++++++------------------------
>  btf_encoder.h      |   8 ++
>  dwarves.h          |   1 +
>  man-pages/pahole.1 |   8 +-
>  pahole.c           |  11 +-
>  5 files changed, 183 insertions(+), 192 deletions(-)
> 
> diff --git a/btf_encoder.c b/btf_encoder.c
> index 97d35e0..d3a66a0 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -65,16 +65,13 @@ struct elf_function {
>  	struct btf_encoder_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;
> +	bool        include;
> +	uint32_t    type;
> +	struct gobuffer secinfo;
>  };
>  
>  /*
> @@ -84,31 +81,24 @@ 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;
>  	uint32_t	  type_id_off;
>  	bool		  has_index_type,
>  			  need_index_type,
> -			  skip_encoding_vars,
>  			  raw_output,
>  			  verbose,
>  			  force,
>  			  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;
> +	int                encode_vars;
> +	uint32_t           percpu_shndx;
>  	struct {
>  		struct elf_function *entries;
>  		int		    allocated;
> @@ -735,46 +725,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, int shndx,
> +					    uint32_t type, unsigned long addr, uint32_t size)
>  {
>  	struct btf_var_secinfo si = {
>  		.type = type,
> -		.offset = offset,
> +		.offset = addr,
>  		.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;
> -
> +	int 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 %d: \"%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, int 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);
> @@ -1741,13 +1741,14 @@ out:
>  int btf_encoder__encode(struct btf_encoder *encoder)
>  {
>  	bool should_tag_kfuncs;
> -	int err;
> +	int err, 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)
> @@ -1797,111 +1798,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);
> @@ -1923,75 +1831,128 @@ 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->symtab)
>  		return 0;
>  
>  	if (encoder->verbose)
> -		printf("search cu '%s' for percpu global variables.\n", cu->name);
> +		printf("search cu '%s' for global variables.\n", cu->name);
>  
>  	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;
> +		int shndx;
> +		struct elf_secinfo *sec = NULL;
>  		uint64_t addr;
>  		int id;
> +		unsigned long size;
>  
> +		/* 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 >= 0 && shndx < encoder->seccnt)
> +			sec = &encoder->secinfo[shndx];
> +		if (!sec || !sec->include)
>  			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 -= sec->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",
> @@ -2003,9 +1964,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 variable '%s'...\n", name ?: "<missing name>");
>  			continue;
>  		}
>  
> @@ -2035,13 +1997,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, addr, size);
> +		id = btf_encoder__add_var_secinfo(encoder, shndx, id, addr, 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;
>  		}
>  	}
> @@ -2068,7 +2031,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;
> @@ -2076,6 +2038,11 @@ 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;
> +		if (conf_load->encode_btf_global_vars)
> +			encoder->encode_vars |= BTF_VAR_GLOBAL;
>  
>  		GElf_Ehdr ehdr;
>  
> @@ -2085,8 +2052,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);
> @@ -2127,15 +2092,21 @@ 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;
> -
> -			if (strcmp(secname, PERCPU_SECTION) == 0)
> -				encoder->percpu.shndx = shndx;
> +			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) {
> +				encoder->percpu_shndx = shndx;
> +				if (encoder->encode_vars & BTF_VAR_PERCPU)
> +					encoder->secinfo[shndx].include = true;
> +			}
>  		}
>  
> -		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->skip_encoding_vars))
> +		if (btf_encoder__collect_symbols(encoder))
>  			goto out_delete;
>  
>  		if (encoder->verbose)
> @@ -2156,7 +2127,8 @@ void btf_encoder__delete(struct btf_encoder *encoder)
>  		return;
>  
>  	btf_encoders__delete(encoder);
> -	__gobuffer__delete(&encoder->percpu_secinfo);
> +	for (int i = 0; i < encoder->seccnt; i++)
> +		__gobuffer__delete(&encoder->secinfo[i].secinfo);
>  	zfree(&encoder->filename);
>  	zfree(&encoder->source_filename);
>  	btf__free(encoder->btf);
> @@ -2166,9 +2138,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);
>  }
> @@ -2321,7 +2290,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);
>  
>  	/* It is only safe to delete this CU if we have not stashed any static
> diff --git a/btf_encoder.h b/btf_encoder.h
> index f54c95a..5e4d53a 100644
> --- a/btf_encoder.h
> +++ b/btf_encoder.h
> @@ -16,7 +16,15 @@ 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,
> +	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);
> +
>  void btf_encoder__delete(struct btf_encoder *encoder);
>  
>  int btf_encoder__encode(struct btf_encoder *encoder);
> 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 0a9d8ac..4bc2d03 100644
> --- a/man-pages/pahole.1
> +++ b/man-pages/pahole.1
> @@ -230,7 +230,10 @@ the debugging information.
>  
>  .TP
>  .B \-\-skip_encoding_btf_vars
> -Do not encode VARs in BTF.
> +.TQ
> +.B \-\-encode_btf_global_vars
> +By default, VARs are encoded only for percpu variables. These options allow
> +to skip encoding them, or alternatively to encode all global variables too.
>  
>  .TP
>  .B \-\-skip_encoding_btf_decl_tag
> @@ -296,7 +299,8 @@ 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.
> +	global_var         Encode all global variables in the same way.

hi,
I tried to test this but I'm not getting DATASEC sections in the BTF,
is the change below enough to enable this in kernel build?

thanks,
jirka


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

Re: [PATCH dwarves v2 4/4] btf_encoder: add global_var feature to encode globals

[Alan Maguire]

On 02/10/2024 15:14, Jiri Olsa wrote:
> On Fri, Sep 20, 2024 at 01:19:01AM -0700, Stephen Brennan wrote:
>> Currently the "var" feature only encodes percpu variables. Add the
>> ability to encode all global variables.
>>
>> This also drops the use of the symbol table to find variable names and
>> compare them against DWARF names. We simply rely on the DWARF
>> information to give us all the variables.
>>
>> Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
>> Tested-by: Alan Maguire <alan.maguire@oracle.com>
>> ---
>>  btf_encoder.c      | 347 +++++++++++++++++++++------------------------
>>  btf_encoder.h      |   8 ++
>>  dwarves.h          |   1 +
>>  man-pages/pahole.1 |   8 +-
>>  pahole.c           |  11 +-
>>  5 files changed, 183 insertions(+), 192 deletions(-)
>>
>> diff --git a/btf_encoder.c b/btf_encoder.c
>> index 97d35e0..d3a66a0 100644
>> --- a/btf_encoder.c
>> +++ b/btf_encoder.c
>> @@ -65,16 +65,13 @@ struct elf_function {
>>  	struct btf_encoder_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;
>> +	bool        include;
>> +	uint32_t    type;
>> +	struct gobuffer secinfo;
>>  };
>>  
>>  /*
>> @@ -84,31 +81,24 @@ 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;
>>  	uint32_t	  type_id_off;
>>  	bool		  has_index_type,
>>  			  need_index_type,
>> -			  skip_encoding_vars,
>>  			  raw_output,
>>  			  verbose,
>>  			  force,
>>  			  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;
>> +	int                encode_vars;
>> +	uint32_t           percpu_shndx;
>>  	struct {
>>  		struct elf_function *entries;
>>  		int		    allocated;
>> @@ -735,46 +725,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, int shndx,
>> +					    uint32_t type, unsigned long addr, uint32_t size)
>>  {
>>  	struct btf_var_secinfo si = {
>>  		.type = type,
>> -		.offset = offset,
>> +		.offset = addr,
>>  		.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;
>> -
>> +	int 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 %d: \"%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, int 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);
>> @@ -1741,13 +1741,14 @@ out:
>>  int btf_encoder__encode(struct btf_encoder *encoder)
>>  {
>>  	bool should_tag_kfuncs;
>> -	int err;
>> +	int err, 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)
>> @@ -1797,111 +1798,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);
>> @@ -1923,75 +1831,128 @@ 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->symtab)
>>  		return 0;
>>  
>>  	if (encoder->verbose)
>> -		printf("search cu '%s' for percpu global variables.\n", cu->name);
>> +		printf("search cu '%s' for global variables.\n", cu->name);
>>  
>>  	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;
>> +		int shndx;
>> +		struct elf_secinfo *sec = NULL;
>>  		uint64_t addr;
>>  		int id;
>> +		unsigned long size;
>>  
>> +		/* 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 >= 0 && shndx < encoder->seccnt)
>> +			sec = &encoder->secinfo[shndx];
>> +		if (!sec || !sec->include)
>>  			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 -= sec->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",
>> @@ -2003,9 +1964,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 variable '%s'...\n", name ?: "<missing name>");
>>  			continue;
>>  		}
>>  
>> @@ -2035,13 +1997,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, addr, size);
>> +		id = btf_encoder__add_var_secinfo(encoder, shndx, id, addr, 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;
>>  		}
>>  	}
>> @@ -2068,7 +2031,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;
>> @@ -2076,6 +2038,11 @@ 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;
>> +		if (conf_load->encode_btf_global_vars)
>> +			encoder->encode_vars |= BTF_VAR_GLOBAL;
>>  
>>  		GElf_Ehdr ehdr;
>>  
>> @@ -2085,8 +2052,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);
>> @@ -2127,15 +2092,21 @@ 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;
>> -
>> -			if (strcmp(secname, PERCPU_SECTION) == 0)
>> -				encoder->percpu.shndx = shndx;
>> +			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) {
>> +				encoder->percpu_shndx = shndx;
>> +				if (encoder->encode_vars & BTF_VAR_PERCPU)
>> +					encoder->secinfo[shndx].include = true;
>> +			}
>>  		}
>>  
>> -		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->skip_encoding_vars))
>> +		if (btf_encoder__collect_symbols(encoder))
>>  			goto out_delete;
>>  
>>  		if (encoder->verbose)
>> @@ -2156,7 +2127,8 @@ void btf_encoder__delete(struct btf_encoder *encoder)
>>  		return;
>>  
>>  	btf_encoders__delete(encoder);
>> -	__gobuffer__delete(&encoder->percpu_secinfo);
>> +	for (int i = 0; i < encoder->seccnt; i++)
>> +		__gobuffer__delete(&encoder->secinfo[i].secinfo);
>>  	zfree(&encoder->filename);
>>  	zfree(&encoder->source_filename);
>>  	btf__free(encoder->btf);
>> @@ -2166,9 +2138,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);
>>  }
>> @@ -2321,7 +2290,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);
>>  
>>  	/* It is only safe to delete this CU if we have not stashed any static
>> diff --git a/btf_encoder.h b/btf_encoder.h
>> index f54c95a..5e4d53a 100644
>> --- a/btf_encoder.h
>> +++ b/btf_encoder.h
>> @@ -16,7 +16,15 @@ 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,
>> +	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);
>> +
>>  void btf_encoder__delete(struct btf_encoder *encoder);
>>  
>>  int btf_encoder__encode(struct btf_encoder *encoder);
>> 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 0a9d8ac..4bc2d03 100644
>> --- a/man-pages/pahole.1
>> +++ b/man-pages/pahole.1
>> @@ -230,7 +230,10 @@ the debugging information.
>>  
>>  .TP
>>  .B \-\-skip_encoding_btf_vars
>> -Do not encode VARs in BTF.
>> +.TQ
>> +.B \-\-encode_btf_global_vars
>> +By default, VARs are encoded only for percpu variables. These options allow
>> +to skip encoding them, or alternatively to encode all global variables too.
>>  
>>  .TP
>>  .B \-\-skip_encoding_btf_decl_tag
>> @@ -296,7 +299,8 @@ 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.
>> +	global_var         Encode all global variables in the same way.
> 
> hi,
> I tried to test this but I'm not getting DATASEC sections in the BTF,
> is the change below enough to enable this in kernel build?
>

Yep, that looks right to me and it's what I did to test with kernel
builds. For me that was enough to get datasecs and all global variables,
but if it doesn't work at your end we can take a look. Thanks!

Stephen, maybe for the respun patches we could add a note to the cover
letter on how to test with kernel builds? Thanks!

Alan

> thanks,
> jirka
> 
> 
> ---
> 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

Re: [PATCH dwarves v2 4/4] btf_encoder: add global_var feature to encode globals

[Jiri Olsa]

On Wed, Oct 02, 2024 at 04:11:23PM +0100, Alan Maguire wrote:

SNIP

> >> diff --git a/man-pages/pahole.1 b/man-pages/pahole.1
> >> index 0a9d8ac..4bc2d03 100644
> >> --- a/man-pages/pahole.1
> >> +++ b/man-pages/pahole.1
> >> @@ -230,7 +230,10 @@ the debugging information.
> >>  
> >>  .TP
> >>  .B \-\-skip_encoding_btf_vars
> >> -Do not encode VARs in BTF.
> >> +.TQ
> >> +.B \-\-encode_btf_global_vars
> >> +By default, VARs are encoded only for percpu variables. These options allow
> >> +to skip encoding them, or alternatively to encode all global variables too.
> >>  
> >>  .TP
> >>  .B \-\-skip_encoding_btf_decl_tag
> >> @@ -296,7 +299,8 @@ 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.
> >> +	global_var         Encode all global variables in the same way.
> > 
> > hi,
> > I tried to test this but I'm not getting DATASEC sections in the BTF,
> > is the change below enough to enable this in kernel build?
> >
> 
> Yep, that looks right to me and it's what I did to test with kernel
> builds. For me that was enough to get datasecs and all global variables,
> but if it doesn't work at your end we can take a look. Thanks!

I managed to get all that by running pahole directly,
will check it closely with the new version of that patchset

thanks,
jirka

> 
> Stephen, maybe for the respun patches we could add a note to the cover
> letter on how to test with kernel builds? Thanks!
> 
> Alan
> 
> > thanks,
> > jirka
> > 
> > 
> > ---
> > 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
>