[PATCH v2 dwarves 0/3] reduce memory overhead of BTF encoding

[PATCH v2 dwarves 0/3] reduce memory overhead of BTF encoding

[Alan Maguire]

The goal of this series is two-fold

- Reduce memory utilization during pahole BTF generation
- Improve testing for BTF function encoding

Patch 1 switches from using a DWARF representation when saving function
information for (possible) later addition to using BTF; the result
is we can discard DWARF info after use and memory usage will not
get out of hand.  Peak memory utilization is approximately 1Gb versus
3/4Gb prior to this change when specifying "-j --btf_features=all".

Patch 2 adds support to pfunct to show all functions that match a given
name (allowing us to find inconsistent prototypes in the test in patch
3), and also adds stealer support for printing all DWARF function
prototypes to speed up execution (the latter is used in the test also).

Patch 3 is a bash test script which carries out vmlinux BTF generation
and verifies the BTF functions match their DWARF equivalents, and also
verifies the skipped functions should have been skipped.  Patch 3
depends on the pfunct functionality from patch 2 and the pahole changes
in patch 1, but moving forward should be useful for validating pahole
function-related changes quickly.  The test takes approximately 5
minutes to run.

Changes since RFC:

- fixed free issues (Jiri, patch 1)
- embed func state into struct elf_function as each function
  has state, and we can use the stack for state comparison
  avoiding the cost of additional allocations (patch 1)
- fix an intermittent coredump that resulted from using
  btf__str_by_offset() string in adding function prototype;
  needing more space could sometimes result in strset memory
  movement which would in turn invalidate the char * string,
  so copy it to a char array for param name/tag value encoding
  (patch 1)
- improve logging of decision to skip functions so that we can
  gather that info more easily for testing in patch 3
- added new option to pfunct to show all instances of a function
  prototype and - if no function is specified - use the stealer
  to dump out all prototypes more rapidly to aid quicker testing
- added test covering BTF function generation, checking that
  DWARF/BTF are consistent and that for skipped functions the
  skip rationale was valid (Jiri, patch 3)

There's no diff-style tests comparing encoding before/after
changes, but having automated tests probing DWARF/BTF
correspondence is useful I think.

Alan Maguire (3):
  btf_encoder: record BTF-centric function state instead of
    DWARF-centric
  pfunct: show all functions that match filter criteria
  tests: add test validating BTF encoding, reasons we skip functions

 btf_encoder.c          | 543 ++++++++++++++++++++++++++++++-----------
 pfunct.c               |  36 ++-
 tests/btf_functions.sh | 192 +++++++++++++++
 3 files changed, 619 insertions(+), 152 deletions(-)
 create mode 100755 tests/btf_functions.sh

-- 
2.43.5

[PATCH v2 dwarves 1/3] btf_encoder: record BTF-centric function state instead of DWARF-centric

[Alan Maguire]

When recording function information for later comparison (as we
do when skipping inconsistent function descriptions), we utilize
DWARF-based representations because we do not want to jump the
gun and add BTF representations for functions that have inconsistent
representations across CUs (and across encoders in parallel mode).

So to handle this, we save info about functions, and we can then add
them later once we have ensured their various representations are
in fact consistent.  However to ensure that the function info
is still valid, we need to specify LSK__KEEPIT for CUs, which
bloats memory usage (in some cases to ~4Gb).  This is not a good
approach, mea culpa.

Instead, store a BTF-centric representation where we

- store the number of parameters
- store the BTF ids of return type and parameters
- store the name of the parameters where present
- store any LLVM annotation values, component idxs if present

So in summary, store everything we need to add the BTF_KIND_FUNC
and BTF_KIND_FUNC_PROTO and any associated annotations.  This will
allow us to free CUs as we go but make it possible to add functions
later.

For name storage we can take advantage of the fact that
BTF will avoid re-adding a name string so we btf__add_str()
to add the parameter name and store the string offset instead;
this prevents duplicate name storage while ensuring the parameter
name is in BTF.

When we cross-compare functions for consistency, do a shallow
analysis akin to what was done with DWARF prototype comparisons;
compare base types by name, reference types by target type,
match loosely between fwds, structs and unions etc.

When this is done, memory consumption peaks at 1Gb rather than
~4Gb for vmlinux generation.  Time taken appears to be approximately
the same for -j1, but slightly faster for multiple threads;
for example:

Baseline

$ time pahole -J vmlinux -j1 --btf_features=default

real	0m17.268s
user	0m15.808s
sys	0m1.415s

$ time pahole -J vmlinux -j8 --btf_features=default

real	0m10.768s
user	0m30.793s
sys	0m4.199s

With these changes:

$ time pahole -J vmlinux -j1 --btf_features=default

real	0m16.564s
user	0m16.029s
sys	0m0.492s

$ time pahole -J vmlinux -j8 --btf_features=default

real	0m8.332s
user	0m30.627s
sys	0m0.714s

In terms of functions encoded, 360 fewer functions make it into
BTF due to the different approach in consistency checking, but after
examining these cases, they do appear to be legitimately inconsistent
functions where the optimized versions have parameter mismatches
with the non-optimized expectations.

Mileage may vary of course, and any testing folks could do would
be greatly appreciated!

More future work would involve sharing the ELF representation
across encoders; this is the logical next step and judging by
single-threaded performance should get us down to ~500Mb peak memory
utilization; however it is better to approach this by taking
smaller steps (we would also need to think about handling synchronous
read/writes from multiple threads of function state).

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 btf_encoder.c | 543 +++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 401 insertions(+), 142 deletions(-)

diff --git a/btf_encoder.c b/btf_encoder.c
index 8a2d92e..d535b90 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -33,13 +33,18 @@
 #include <search.h> /* for tsearch(), tfind() and tdestroy() */
 #include <pthread.h>
 
-#define BTF_ENCODER_MAX_PROTO	512
 #define BTF_IDS_SECTION		".BTF_ids"
 #define BTF_ID_FUNC_PFX		"__BTF_ID__func__"
 #define BTF_ID_SET8_PFX		"__BTF_ID__set8__"
 #define BTF_SET8_KFUNCS		(1 << 0)
 #define BTF_KFUNC_TYPE_TAG	"bpf_kfunc"
 
+/*
+ * This corresponds to the same macro defined in
+ * include/linux/kallsyms.h
+ */
+#define KSYM_NAME_LEN 128
+
 /* Adapted from include/linux/btf_ids.h */
 struct btf_id_set8 {
         uint32_t cnt;
@@ -50,19 +55,37 @@ struct btf_id_set8 {
         } pairs[];
 };
 
+struct btf_encoder_func_parm {
+	int name_off;
+	uint32_t type_id;
+};
+
+struct btf_encoder_func_annot {
+	int value;
+	int16_t component_idx;
+};
+
 /* state used to do later encoding of saved functions */
-struct btf_encoder_state {
+struct btf_encoder_func_state {
 	uint32_t type_id_off;
-	bool got_proto;
-	char proto[BTF_ENCODER_MAX_PROTO];
+	uint16_t nr_parms;
+	uint16_t nr_annots;
+	uint8_t initialized:1;
+	uint8_t optimized_parms:1;
+	uint8_t unexpected_reg:1;
+	uint8_t inconsistent_proto:1;
+	uint8_t processed:1;
+	int ret_type_id;
+	struct btf_encoder_func_parm *parms;
+	struct btf_encoder_func_annot *annots;
 };
 
 struct elf_function {
 	const char	*name;
+	char		*alias;
 	bool		 generated;
 	size_t		prefixlen;
-	struct function	*function;
-	struct btf_encoder_state state;
+	struct btf_encoder_func_state state;
 };
 
 struct var_info {
@@ -125,7 +148,7 @@ struct btf_kfunc_set_range {
 static LIST_HEAD(encoders);
 static pthread_mutex_t encoders__lock = PTHREAD_MUTEX_INITIALIZER;
 
-static void btf_encoder__add_saved_funcs(struct btf_encoder *encoder);
+static int btf_encoder__add_saved_funcs(struct btf_encoder *encoder);
 
 /* mutex only needed for add/delete, as this can happen in multiple encoding
  * threads.  Traversal of the list is currently confined to thread collection.
@@ -669,18 +692,25 @@ static int32_t btf_encoder__tag_type(struct btf_encoder *encoder, uint32_t tag_t
 	return encoder->type_id_off + tag_type;
 }
 
-static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct ftype *ftype)
+static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct ftype *ftype, struct elf_function *func)
 {
 	struct btf *btf = encoder->btf;
 	const struct btf_type *t;
 	struct parameter *param;
 	uint16_t nr_params, param_idx;
 	int32_t id, type_id;
+	char tmp_name[KSYM_NAME_LEN];
+	const char *name;
+	struct btf_encoder_func_state *state = &func->state;
 
 	/* add btf_type for func_proto */
-	nr_params = ftype->nr_parms + (ftype->unspec_parms ? 1 : 0);
-	type_id = btf_encoder__tag_type(encoder, ftype->tag.type);
-
+	if (ftype) {
+		nr_params = ftype->nr_parms + (ftype->unspec_parms ? 1 : 0);
+		type_id = btf_encoder__tag_type(encoder, ftype->tag.type);
+	} else {
+		nr_params = state->nr_parms;
+		type_id = state->ret_type_id;
+	}
 	id = btf__add_func_proto(btf, type_id);
 	if (id > 0) {
 		t = btf__type_by_id(btf, id);
@@ -694,20 +724,34 @@ static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct f
 
 	/* add parameters */
 	param_idx = 0;
-	ftype__for_each_parameter(ftype, param) {
-		const char *name = parameter__name(param);
+	if (ftype) {
+		ftype__for_each_parameter(ftype, param) {
+			const char *name = parameter__name(param);
+
+			type_id = param->tag.type == 0 ? 0 : encoder->type_id_off + param->tag.type;
+			++param_idx;
+			if (btf_encoder__add_func_param(encoder, name, type_id, param_idx == nr_params))
+				return -1;
+		}
 
-		type_id = param->tag.type == 0 ? 0 : encoder->type_id_off + param->tag.type;
 		++param_idx;
-		if (btf_encoder__add_func_param(encoder, name, type_id, param_idx == nr_params))
-			return -1;
-	}
+		if (ftype->unspec_parms)
+			if (btf_encoder__add_func_param(encoder, NULL, 0, param_idx == nr_params))
+				return -1;
+	} else {
+		for (param_idx = 0; param_idx < nr_params; param_idx++) {
+			struct btf_encoder_func_parm *p = &state->parms[param_idx];
+			name = btf__name_by_offset(btf, p->name_off);
 
-	++param_idx;
-	if (ftype->unspec_parms)
-		if (btf_encoder__add_func_param(encoder, NULL, 0, param_idx == nr_params))
-			return -1;
+			/* adding BTF data may result in a move of the
+			 * name string memory, so make a temporary copy.
+			 */
+			strncpy(tmp_name, name, sizeof(tmp_name));
 
+			if (btf_encoder__add_func_param(encoder, tmp_name, p->type_id, param_idx == nr_params))
+				return -1;
+		}
+	}
 	return id;
 }
 
@@ -831,53 +875,170 @@ static int32_t btf_encoder__add_decl_tag(struct btf_encoder *encoder, const char
 	return id;
 }
 
-static bool proto__get(struct function *func, char *proto, size_t len)
+static void btf_encoder__log_func_skip(struct btf_encoder *encoder, struct elf_function *func,
+				       const char *fmt, ...)
 {
-	const struct conf_fprintf conf = {
-						.name_spacing = 23,
-						.type_spacing = 26,
-						.emit_stats = 0,
-						.no_parm_names = 1,
-						.skip_emitting_errors = 1,
-						.skip_emitting_modifier = 1,
-					};
+	va_list ap;
 
-	return function__prototype_conf(func, func->priv, &conf, proto, len) != NULL;
+	if (!encoder->verbose)
+		return;
+	printf("%s (%s): skipping BTF encoding of function due to ",
+	       func->alias ?: func->name, func->name);
+	va_start(ap, fmt);
+	vprintf(fmt, ap);
+	va_end(ap);
 }
 
-static bool funcs__match(struct btf_encoder *encoder, struct elf_function *func, struct function *f2)
+static bool names__match(struct btf *btf1, const struct btf_type *t1,
+			struct btf *btf2, const struct btf_type *t2)
 {
-	char proto[BTF_ENCODER_MAX_PROTO];
-	struct function *f1 = func->function;
-	const char *name;
+	const char *str1;
+	const char *str2;
 
-	if (!f1)
-		return false;
+	if ((btf1 == btf2) && (t1->name_off == t2->name_off))
+		return true;
 
-	name = function__name(f1);
+	str1 = btf__name_by_offset(btf1, t1->name_off);
+	str2 = btf__name_by_offset(btf2, t2->name_off);
 
-	if (f1->proto.nr_parms != f2->proto.nr_parms) {
-		if (encoder->verbose)
-			printf("function mismatch for '%s'(%s): %d params != %d params\n",
-			       name, f1->alias ?: name,
-			       f1->proto.nr_parms, f2->proto.nr_parms);
-		return false;
-	}
-	if (f1->proto.nr_parms == 0)
-		return true;
+	return strcmp(str1, str2) == 0;
+}
+
+static int fwd__kind(const struct btf_type *t)
+{
+	if (btf_kind(t) == BTF_KIND_FWD)
+		return btf_kflag(t) ? BTF_KIND_UNION : BTF_KIND_STRUCT;
+	return btf_kind(t);
+}
+
+static bool types__match(struct btf_encoder *encoder,
+			 struct btf *btf1, int type_id1,
+			 struct btf *btf2, int type_id2)
+{
+	uint32_t id1 = type_id1;
+	uint32_t id2 = type_id2;
+
+	do {
+		const struct btf_type *t1;
+		const struct btf_type *t2;
+		int k1;
+		int k2;
+
+		if ((btf1 == btf2) && (id1 == id2))
+			return true;
+		if (!id1 || !id2)
+			return id1 == id2;
+
+		t1 = btf__type_by_id(btf1, id1);
+		t2 = btf__type_by_id(btf2, id2);
+
+		k1 = fwd__kind(t1);
+		k2 = fwd__kind(t2);
 
-	if (f1->proto.tag.type == f2->proto.tag.type)
+		if (k1 != k2) {
+			/* loose matching allows us to match const/non-const
+			 * parameters.
+			 */
+			if (k1 == BTF_KIND_CONST) {
+				id1 = t1->type;
+				continue;
+			}
+			if (k2 == BTF_KIND_CONST) {
+				id2 = t2->type;
+				continue;
+			}
+			return false;
+		}
+
+		switch (btf_kind(t1)) {
+		case BTF_KIND_INT:
+		case BTF_KIND_FLOAT:
+		case BTF_KIND_FWD:
+		case BTF_KIND_TYPEDEF:
+		case BTF_KIND_STRUCT:
+		case BTF_KIND_UNION:
+		case BTF_KIND_ENUM:
+		case BTF_KIND_ENUM64:
+			return names__match(btf1, t1, btf2, t2);
+		case BTF_KIND_PTR:
+		case BTF_KIND_VOLATILE:
+		case BTF_KIND_CONST:
+		case BTF_KIND_RESTRICT:
+		case BTF_KIND_TYPE_TAG:
+			id1 = t1->type;
+			id2 = t2->type;
+			break;
+		case BTF_KIND_ARRAY: {
+			const struct btf_array *a1 = btf_array(t1);
+			const struct btf_array *a2 = btf_array(t2);
+
+			if (a1->nelems != a2->nelems)
+				return false;
+			id1 = a1->type;
+			id2 = a2->type;
+			break;
+		}
+		case BTF_KIND_FUNC_PROTO: {
+			const struct btf_param *p1 = btf_params(t1);
+			const struct btf_param *p2 = btf_params(t2);
+			int i, vlen = btf_vlen(t1);
+
+			if (vlen != btf_vlen(t2))
+				return false;
+			if (!types__match(encoder, btf1, t1->type,
+					  btf2, t2->type))
+				return false;
+			for (i = 0; i < vlen; i++, p1++, p2++) {
+				if (!types__match(encoder, btf1, t1->type,
+						  btf2, t2->type))
+					return false;
+			}
+			return true;
+		}
+		default:
+			return false;
+		}
+	} while (1);
+
+	return false;
+}
+
+static bool funcs__match(struct btf_encoder *encoder, struct elf_function *func,
+			 struct btf *btf1, struct btf_encoder_func_state *s1,
+			 struct btf *btf2, struct btf_encoder_func_state *s2)
+{
+	uint8_t i;
+
+	if (!s1->initialized || !s2->initialized)
 		return true;
 
-	if (!func->state.got_proto)
-		func->state.got_proto = proto__get(f1, func->state.proto, sizeof(func->state.proto));
+	if (s1->nr_parms != s2->nr_parms) {
+		btf_encoder__log_func_skip(encoder, func,
+					   "param count mismatch; %d params != %d params\n",
+					   s1->nr_parms, s2->nr_parms);
+		return false;
+	}
+	if (!types__match(encoder, btf1, s1->ret_type_id, btf2, s2->ret_type_id)) {
+		btf_encoder__log_func_skip(encoder, func, "return type mismatch\n");
+		return false;
+	}
+	if (s1->nr_parms == 0)
+		return true;
 
-	if (proto__get(f2, proto, sizeof(proto))) {
-		if (strcmp(func->state.proto, proto) != 0) {
-			if (encoder->verbose)
-				printf("function mismatch for '%s'('%s'): '%s' != '%s'\n",
-				       name, f1->alias ?: name,
-				       func->state.proto, proto);
+	for (i = 0; i < s1->nr_parms; i++) {
+		if (!types__match(encoder, btf1, s1->parms[i].type_id,
+				  btf2, s2->parms[i].type_id)) {
+			if (encoder->verbose) {
+				const char *p1 = btf__name_by_offset(btf1, s1->parms[i].name_off);
+				const char *p2 = btf__name_by_offset(btf2, s2->parms[i].name_off);
+
+				btf_encoder__log_func_skip(encoder, func,
+							   "param type mismatch for param#%d %s %s %s\n",
+							   i + 1,
+							   p1 ?: "",
+							   p1 && p2 ? "!=" : "",
+							   p2 ?: "");
+			}
 			return false;
 		}
 	}
@@ -886,119 +1047,212 @@ static bool funcs__match(struct btf_encoder *encoder, struct elf_function *func,
 
 static int32_t btf_encoder__save_func(struct btf_encoder *encoder, struct function *fn, struct elf_function *func)
 {
-	fn->priv = encoder->cu;
-	if (func->function) {
-		struct function *existing = func->function;
-
-		/* If saving and we find an existing entry, we want to merge
-		 * observations across both functions, checking that the
-		 * "seen optimized parameters", "inconsistent prototype"
-		 * and "unexpected register" status is reflected in the
-		 * the func entry.
-		 * If the entry is new, record encoder state required
-		 * to add the local function later (encoder + type_id_off)
-		 * such that we can add the function later.
-		 */
-		existing->proto.optimized_parms |= fn->proto.optimized_parms;
-		existing->proto.unexpected_reg |= fn->proto.unexpected_reg;
-		if (!existing->proto.unexpected_reg && !existing->proto.inconsistent_proto &&
-		     !funcs__match(encoder, func, fn))
-			existing->proto.inconsistent_proto = 1;
-	} else {
-		func->state.type_id_off = encoder->type_id_off;
-		func->function = fn;
-		encoder->cu->functions_saved++;
+	struct btf_encoder_func_state *existing = &func->state;
+	struct btf_encoder_func_state state = { 0 };
+	struct ftype *ftype = &fn->proto;
+	struct btf *btf = encoder->btf;
+	struct llvm_annotation *annot;
+	struct parameter *param;
+	uint8_t param_idx = 0;
+	int str_off, err = 0;
+
+	/* if already skipping this function, no need to proceed. */
+	if (existing->unexpected_reg || existing->inconsistent_proto)
+		return 0;
+
+	state.nr_parms = ftype->nr_parms + (ftype->unspec_parms ? 1 : 0);
+	state.ret_type_id = ftype->tag.type == 0 ? 0 : encoder->type_id_off + ftype->tag.type;
+	if (state.nr_parms > 0) {
+		state.parms = zalloc(state.nr_parms * sizeof(*state.parms));
+		if (!state.parms) {
+			err = -ENOMEM;
+			goto out;
+		}
 	}
-	return 0;
+	state.inconsistent_proto = ftype->inconsistent_proto;
+	state.unexpected_reg = ftype->unexpected_reg;
+	state.optimized_parms = ftype->optimized_parms;
+	ftype__for_each_parameter(ftype, param) {
+		const char *name = parameter__name(param) ?: "";
+
+		str_off = btf__add_str(btf, name);
+		if (str_off < 0) {
+			err = str_off;
+			goto out;
+		}
+		state.parms[param_idx].name_off = str_off;
+		state.parms[param_idx].type_id = param->tag.type == 0 ? 0 : encoder->type_id_off + param->tag.type;
+		param_idx++;
+	}
+	if (ftype->unspec_parms)
+		state.parms[param_idx].type_id = 0;
+
+	list_for_each_entry(annot, &fn->annots, node)
+		state.nr_annots++;
+	if (state.nr_annots) {
+		uint8_t idx = 0;
+
+		state.annots = zalloc(state.nr_annots * sizeof(*state.annots));
+		if (!state.annots) {
+			err = -ENOMEM;
+			goto out;
+		}
+		list_for_each_entry(annot, &fn->annots, node) {
+			str_off = btf__add_str(encoder->btf, annot->value);
+			if (str_off < 0) {
+				err = str_off;
+				goto out;
+			}
+			state.annots[idx].value = str_off;
+			state.annots[idx].component_idx = annot->component_idx;
+			idx++;
+		}
+	}
+	state.initialized = 1;
+
+	if (state.unexpected_reg)
+		btf_encoder__log_func_skip(encoder, func,
+					   "unexpected register used for parameter\n");
+	if (!existing->initialized) {
+		memcpy(existing, &state, sizeof(*existing));
+		return 0;
+	}
+
+	/* If saving and we find an existing entry, we want to merge
+	 * observations across both functions, checking that the
+	 * "seen optimized parameters", "inconsistent prototype"
+	 * and "unexpected register" status is reflected in the
+	 * func entry.
+	 * If the entry is new, record encoder state required
+	 * to add the local function later (encoder + type_id_off)
+	 * such that we can add the function later.
+	 */
+	existing->optimized_parms |= state.optimized_parms;
+	existing->unexpected_reg |= state.unexpected_reg;
+	if (!existing->unexpected_reg &&
+	    !funcs__match(encoder, func, encoder->btf, &state,
+			   encoder->btf, existing))
+		existing->inconsistent_proto = 1;
+out:
+	zfree(&state.annots);
+	zfree(&state.parms);
+	return err;
 }
 
-static int32_t btf_encoder__add_func(struct btf_encoder *encoder, struct function *fn)
+static int32_t btf_encoder__add_func(struct btf_encoder *encoder, struct function *fn, struct elf_function *func)
 {
-	int btf_fnproto_id, btf_fn_id, tag_type_id;
-	struct llvm_annotation *annot;
+	int btf_fnproto_id, btf_fn_id, tag_type_id = 0;
+	int16_t component_idx = -1;
 	const char *name;
+	const char *value;
+	char tmp_value[KSYM_NAME_LEN];
 
-	btf_fnproto_id = btf_encoder__add_func_proto(encoder, &fn->proto);
-	name = function__name(fn);
-	btf_fn_id = btf_encoder__add_ref_type(encoder, BTF_KIND_FUNC, btf_fnproto_id, name, false);
+	btf_fnproto_id = btf_encoder__add_func_proto(encoder, fn ? &fn->proto : NULL, func);
+	name = func->alias ?: func->name;
+	if (btf_fnproto_id >= 0)
+		btf_fn_id = btf_encoder__add_ref_type(encoder, BTF_KIND_FUNC, btf_fnproto_id, name, false);
 	if (btf_fnproto_id < 0 || btf_fn_id < 0) {
-		printf("error: failed to encode function '%s'\n", function__name(fn));
+		printf("error: failed to encode function '%s': invalid %s\n", name, btf_fnproto_id < 0 ? "proto" : "func");
 		return -1;
 	}
-	list_for_each_entry(annot, &fn->annots, node) {
-		tag_type_id = btf_encoder__add_decl_tag(encoder, annot->value, btf_fn_id,
-							annot->component_idx);
-		if (tag_type_id < 0) {
-			fprintf(stderr, "error: failed to encode tag '%s' to func %s with component_idx %d\n",
-				annot->value, name, annot->component_idx);
-			return -1;
+	if (!fn) {
+		struct btf_encoder_func_state *state = &func->state;
+		uint16_t idx;
+
+		if (!state || state->nr_annots == 0)
+			return 0;
+
+		for (idx = 0; idx < state->nr_annots; idx++) {
+			struct btf_encoder_func_annot *a = &state->annots[idx];
+
+			value = btf__str_by_offset(encoder->btf, a->value);
+			/* adding BTF data may result in a mode of the
+			 * value string memory, so make a temporary copy.
+			 */
+			strncpy(tmp_value, value, sizeof(tmp_value));
+			component_idx = a->component_idx;
+
+			tag_type_id = btf_encoder__add_decl_tag(encoder, tmp_value, btf_fn_id, component_idx);
+			if (tag_type_id < 0)
+				break;
+		}
+	} else {
+		struct llvm_annotation *annot;
+
+		list_for_each_entry(annot, &fn->annots, node) {
+			value = annot->value;
+			component_idx = annot->component_idx;
+
+			tag_type_id = btf_encoder__add_decl_tag(encoder, value, btf_fn_id,
+								component_idx);
+			if (tag_type_id < 0)
+				break;
 		}
 	}
+	if (tag_type_id < 0) {
+		fprintf(stderr, "error: failed to encode tag '%s' to func %s with component_idx %d\n",
+			value, name, component_idx);
+		return -1;
+	}
+
 	return 0;
 }
 
-static void btf_encoder__add_saved_funcs(struct btf_encoder *encoder)
+static int btf_encoder__add_saved_funcs(struct btf_encoder *encoder)
 {
 	int i;
 
 	for (i = 0; i < encoder->functions.cnt; i++) {
 		struct elf_function *func = &encoder->functions.entries[i];
-		struct function *fn = func->function;
-		struct btf_encoder *other_encoder;
+		struct btf_encoder_func_state *state = &func->state;
+		struct btf_encoder *other_encoder = NULL;
 
-		if (!fn || fn->proto.processed)
+		if (!state || !state->initialized || state->processed)
 			continue;
-
 		/* merge optimized-out status across encoders; since each
 		 * encoder has the same elf symbol table we can use the
 		 * same index to access the same elf symbol.
 		 */
 		btf_encoders__for_each_encoder(other_encoder) {
-			struct function *other_fn;
+			struct elf_function *other_func;
+			struct btf_encoder_func_state *other_state;
+			uint8_t optimized, unexpected, inconsistent;
 
 			if (other_encoder == encoder)
 				continue;
 
-			other_fn = other_encoder->functions.entries[i].function;
-			if (!other_fn)
+			other_func = &other_encoder->functions.entries[i];
+			other_state = &other_func->state;
+			if (!other_state)
 				continue;
-			fn->proto.optimized_parms |= other_fn->proto.optimized_parms;
-			fn->proto.unexpected_reg |= other_fn->proto.unexpected_reg;
-			if (other_fn->proto.inconsistent_proto)
-				fn->proto.inconsistent_proto = 1;
-			if (!fn->proto.unexpected_reg && !fn->proto.inconsistent_proto &&
-			    !funcs__match(encoder, func, other_fn))
-				fn->proto.inconsistent_proto = 1;
-			other_fn->proto.processed = 1;
+			optimized = state->optimized_parms | other_state->optimized_parms;
+			unexpected = state->unexpected_reg | other_state->unexpected_reg;
+			inconsistent = state->inconsistent_proto | other_state->inconsistent_proto;
+			if (!unexpected && !inconsistent &&
+			    !funcs__match(encoder, func,
+					  encoder->btf, state,
+					  other_encoder->btf, other_state))
+				inconsistent = 1;
+			state->optimized_parms = other_state->optimized_parms = optimized;
+			state->unexpected_reg = other_state->unexpected_reg = unexpected;
+			state->inconsistent_proto = other_state->inconsistent_proto = inconsistent;
+
+			other_state->processed = 1;
 		}
 		/* do not exclude functions with optimized-out parameters; they
 		 * may still be _called_ with the right parameter values, they
 		 * just do not _use_ them.  Only exclude functions with
 		 * unexpected register use or multiple inconsistent prototypes.
 		 */
-		if (fn->proto.unexpected_reg || fn->proto.inconsistent_proto) {
-			if (encoder->verbose) {
-				const char *name = function__name(fn);
-
-				printf("skipping addition of '%s'(%s) due to %s\n",
-				       name, fn->alias ?: name,
-				       fn->proto.unexpected_reg ? "unexpected register used for parameter" :
-								   "multiple inconsistent function prototypes");
-			}
-		} else {
-			encoder->type_id_off = func->state.type_id_off;
-			btf_encoder__add_func(encoder, fn);
+		if (!state->unexpected_reg && !state->inconsistent_proto) {
+			if (btf_encoder__add_func(encoder, NULL, func))
+				return -1;
 		}
-		fn->proto.processed = 1;
+		state->processed = 1;
 	}
+	return 0;
 }
 
-/*
- * This corresponds to the same macro defined in
- * include/linux/kallsyms.h
- */
-#define KSYM_NAME_LEN 128
-
 static int functions_cmp(const void *_a, const void *_b)
 {
 	const struct elf_function *a = _a;
@@ -1051,6 +1305,8 @@ static int btf_encoder__collect_function(struct btf_encoder *encoder, GElf_Sym *
 		encoder->functions.entries = new;
 	}
 
+	memset(&encoder->functions.entries[encoder->functions.cnt], 0,
+	       sizeof(*new));
 	encoder->functions.entries[encoder->functions.cnt].name = name;
 	if (strchr(name, '.')) {
 		const char *suffix = strchr(name, '.');
@@ -1058,11 +1314,6 @@ static int btf_encoder__collect_function(struct btf_encoder *encoder, GElf_Sym *
 		encoder->functions.suffix_cnt++;
 		encoder->functions.entries[encoder->functions.cnt].prefixlen = suffix - name;
 	}
-	encoder->functions.entries[encoder->functions.cnt].generated = false;
-	encoder->functions.entries[encoder->functions.cnt].function = NULL;
-	encoder->functions.entries[encoder->functions.cnt].state.got_proto = false;
-	encoder->functions.entries[encoder->functions.cnt].state.proto[0] = '\0';
-	encoder->functions.entries[encoder->functions.cnt].state.type_id_off = 0;
 	encoder->functions.cnt++;
 	return 0;
 }
@@ -1236,7 +1487,7 @@ static int btf_encoder__encode_tag(struct btf_encoder *encoder, struct tag *tag,
 	case DW_TAG_enumeration_type:
 		return btf_encoder__add_enum_type(encoder, tag, conf_load);
 	case DW_TAG_subroutine_type:
-		return btf_encoder__add_func_proto(encoder, tag__ftype(tag));
+		return btf_encoder__add_func_proto(encoder, tag__ftype(tag), NULL);
         case DW_TAG_unspecified_type:
 		/* Just don't encode this for now, converting anything with this type to void (0) instead.
 		 *
@@ -2128,8 +2379,17 @@ out_delete:
 	return NULL;
 }
 
+void btf_encoder__delete_func(struct elf_function *func)
+{
+	free(func->alias);
+	zfree(&func->state.annots);
+	zfree(&func->state.parms);
+}
+
 void btf_encoder__delete(struct btf_encoder *encoder)
 {
+	int i;
+
 	if (encoder == NULL)
 		return;
 
@@ -2141,6 +2401,8 @@ void btf_encoder__delete(struct btf_encoder *encoder)
 	encoder->btf = NULL;
 	elf_symtab__delete(encoder->symtab);
 
+	for (i = 0; i < encoder->functions.cnt; i++)
+		btf_encoder__delete_func(&encoder->functions.entries[i]);
 	encoder->functions.allocated = encoder->functions.cnt = 0;
 	free(encoder->functions.entries);
 	encoder->functions.entries = NULL;
@@ -2281,20 +2543,20 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
 						       ", has optimized-out parameters" :
 						       fn->proto.unexpected_reg ? ", has unexpected register use by params" :
 						       "");
-					fn->alias = func->name;
+					func->alias = strdup(name);
 				}
 			}
-			if (!func)
-				continue;
 		} else {
 			if (!fn->external)
 				continue;
 		}
+		if (!func)
+			continue;
 
 		if (save)
 			err = btf_encoder__save_func(encoder, fn, func);
 		else
-			err = btf_encoder__add_func(encoder, fn);
+			err = btf_encoder__add_func(encoder, fn, func);
 		if (err)
 			goto out;
 	}
@@ -2302,11 +2564,8 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
 	if (!encoder->skip_encoding_vars)
 		err = btf_encoder__encode_cu_variables(encoder);
 
-	/* It is only safe to delete this CU if we have not stashed any static
-	 * functions for later addition.
-	 */
 	if (!err)
-		err = encoder->cu->functions_saved > 0 ? LSK__KEEPIT : LSK__DELETE;
+		err = LSK__DELETE;
 out:
 	encoder->cu = NULL;
 	return err;
-- 
2.43.5

Re: [PATCH v2 dwarves 1/3] btf_encoder: record BTF-centric function state instead of DWARF-centric

[Eduard Zingerman]

On Mon, 2024-09-16 at 14:49 +0100, Alan Maguire wrote:

[...]

> When this is done, memory consumption peaks at 1Gb rather than
> ~4Gb for vmlinux generation.  Time taken appears to be approximately
> the same for -j1, but slightly faster for multiple threads;
> for example:

Hi Alan,

Sorry for delayed reply.
I tried this patch with selftests vmlinux and see huge reduction indeed:

$ /usr/bin/time -v \
    pahole -j1 --btf_features=consistent_func --btf_encode_detached=old.btf vmlinux
  ...
  Maximum resident set size (kbytes): 3123660

$ /usr/bin/time -v \
    pahole -j1 --btf_features=consistent_func --btf_encode_detached=new.btf vmlinux
  ...
  Maximum resident set size (kbytes): 464400

[...]

> In terms of functions encoded, 360 fewer functions make it into
> BTF due to the different approach in consistency checking, but after
> examining these cases, they do appear to be legitimately inconsistent
> functions where the optimized versions have parameter mismatches
> with the non-optimized expectations.
> 
> Mileage may vary of course, and any testing folks could do would
> be greatly appreciated!

Interestingly, I don't see any difference in generated "consistent"
funcs (BPF selftests kernel config, compiled using mainline clang):

$ comm -3 <(bpftool btf dump file new.btf | grep ' FUNC ' | sed "s/.*'\(.*\)'.*/\1/" | sort) \
          <(bpftool btf dump file old.btf | grep ' FUNC ' | sed "s/.*'\(.*\)'.*/\1/" | sort) \
          | tee | wc -l
0

A few nitpicks below, but overall looks great.

Reviewed-by: Eduard Zingerman <eddyz87@gmail.com>

[...]

> @@ -669,18 +692,25 @@ static int32_t btf_encoder__tag_type(struct btf_encoder *encoder, uint32_t tag_t
>  	return encoder->type_id_off + tag_type;
>  }
>  
> -static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct ftype *ftype)
> +static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct ftype *ftype, struct elf_function *func)

It is a bit unfortunate that we now have this "doubled" representation
and both ftype and elf_function pointers have to be passed through
several layers.
Ihor (in CC) is now working on having single btf_encoder for each
processed CU (in order to be able to sort btf's by CU names before
merging to a single BTF for dedup, thus making reproducible builds
cheaper). I wonder if this could be further extended to have a
separate BTF for functions and merge only consistent functions from
this BTF to a final BTF.

[...]

> @@ -694,20 +724,34 @@ static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct f
>  
>  	/* add parameters */
>  	param_idx = 0;
> -	ftype__for_each_parameter(ftype, param) {
> -		const char *name = parameter__name(param);
> +	if (ftype) {
> +		ftype__for_each_parameter(ftype, param) {
> +			const char *name = parameter__name(param);
> +
> +			type_id = param->tag.type == 0 ? 0 : encoder->type_id_off + param->tag.type;
> +			++param_idx;
> +			if (btf_encoder__add_func_param(encoder, name, type_id, param_idx == nr_params))
> +				return -1;
> +		}
>  
> -		type_id = param->tag.type == 0 ? 0 : encoder->type_id_off + param->tag.type;
>  		++param_idx;
> -		if (btf_encoder__add_func_param(encoder, name, type_id, param_idx == nr_params))
> -			return -1;
> -	}
> +		if (ftype->unspec_parms)
> +			if (btf_encoder__add_func_param(encoder, NULL, 0, param_idx == nr_params))
> +				return -1;
> +	} else {
> +		for (param_idx = 0; param_idx < nr_params; param_idx++) {
> +			struct btf_encoder_func_parm *p = &state->parms[param_idx];
> +			name = btf__name_by_offset(btf, p->name_off);
>  
> -	++param_idx;
> -	if (ftype->unspec_parms)
> -		if (btf_encoder__add_func_param(encoder, NULL, 0, param_idx == nr_params))
> -			return -1;
> +			/* adding BTF data may result in a move of the
> +			 * name string memory, so make a temporary copy.
> +			 */
> +			strncpy(tmp_name, name, sizeof(tmp_name));

When compiling with fresh gcc (14.2.1) there is a warning reported at this line:

btf_encoder.c:749:25: warning: ‘strncpy’ specified bound 128 equals destination size [-Wstringop-truncation]
  749 |                         strncpy(tmp_name, name, sizeof(tmp_name));
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Which seems to be a legit warning, given section CAVEATS from strncpy man page.
There is also a second warning like this (see below).
(when compiled with BUILD_TYPE=Release).

>  
> +			if (btf_encoder__add_func_param(encoder, tmp_name, p->type_id, param_idx == nr_params))
> +				return -1;
> +		}
> +	}
>  	return id;
>  }
>  
> @@ -831,53 +875,170 @@ static int32_t btf_encoder__add_decl_tag(struct btf_encoder *encoder, const char

[...]

> -static bool funcs__match(struct btf_encoder *encoder, struct elf_function *func, struct function *f2)
> +static bool names__match(struct btf *btf1, const struct btf_type *t1,
> +			struct btf *btf2, const struct btf_type *t2)
>  {

[...]

> +static bool types__match(struct btf_encoder *encoder,
> +			 struct btf *btf1, int type_id1,
> +			 struct btf *btf2, int type_id2)
> +{

[...]

> +		switch (btf_kind(t1)) {
> +		case BTF_KIND_INT:

Nit: ints have a meaningful .size field and are followed by a u32 word,
     both should be compared.

> +		case BTF_KIND_FLOAT:
> +		case BTF_KIND_FWD:

Nit: BTF_KIND_FWD should go to default, fwd__kind steps over it.

> +		case BTF_KIND_TYPEDEF:
> +		case BTF_KIND_STRUCT:
> +		case BTF_KIND_UNION:
> +		case BTF_KIND_ENUM:
> +		case BTF_KIND_ENUM64:
> +			return names__match(btf1, t1, btf2, t2);
> +		case BTF_KIND_PTR:
> +		case BTF_KIND_VOLATILE:
> +		case BTF_KIND_CONST:
> +		case BTF_KIND_RESTRICT:
> +		case BTF_KIND_TYPE_TAG:
> +			id1 = t1->type;
> +			id2 = t2->type;
> +			break;
> +		case BTF_KIND_ARRAY: {
> +			const struct btf_array *a1 = btf_array(t1);
> +			const struct btf_array *a2 = btf_array(t2);
> +
> +			if (a1->nelems != a2->nelems)
> +				return false;
> +			id1 = a1->type;
> +			id2 = a2->type;
> +			break;
> +		}
> +		case BTF_KIND_FUNC_PROTO: {
> +			const struct btf_param *p1 = btf_params(t1);
> +			const struct btf_param *p2 = btf_params(t2);
> +			int i, vlen = btf_vlen(t1);
> +
> +			if (vlen != btf_vlen(t2))
> +				return false;
> +			if (!types__match(encoder, btf1, t1->type,
> +					  btf2, t2->type))
> +				return false;
> +			for (i = 0; i < vlen; i++, p1++, p2++) {
> +				if (!types__match(encoder, btf1, t1->type,
> +						  btf2, t2->type))
> +					return false;
> +			}
> +			return true;
> +		}
> +		default:
> +			return false;
> +		}
> +	} while (1);
> +
> +	return false;
> +}
> +
> +static bool funcs__match(struct btf_encoder *encoder, struct elf_function *func,
> +			 struct btf *btf1, struct btf_encoder_func_state *s1,
> +			 struct btf *btf2, struct btf_encoder_func_state *s2)
> +{
> +	uint8_t i;
> +
> +	if (!s1->initialized || !s2->initialized)
>  		return true;

Nit: is it even possible to get here when !s1->initialized || !s2->initialized?
     Maybe log something about inconsistent state?

>  
> -	if (!func->state.got_proto)
> -		func->state.got_proto = proto__get(f1, func->state.proto, sizeof(func->state.proto));
> +	if (s1->nr_parms != s2->nr_parms) {
> +		btf_encoder__log_func_skip(encoder, func,
> +					   "param count mismatch; %d params != %d params\n",
> +					   s1->nr_parms, s2->nr_parms);
> +		return false;
> +	}
> +	if (!types__match(encoder, btf1, s1->ret_type_id, btf2, s2->ret_type_id)) {
> +		btf_encoder__log_func_skip(encoder, func, "return type mismatch\n");
> +		return false;
> +	}
> +	if (s1->nr_parms == 0)
> +		return true;
>  
> -	if (proto__get(f2, proto, sizeof(proto))) {
> -		if (strcmp(func->state.proto, proto) != 0) {
> -			if (encoder->verbose)
> -				printf("function mismatch for '%s'('%s'): '%s' != '%s'\n",
> -				       name, f1->alias ?: name,
> -				       func->state.proto, proto);
> +	for (i = 0; i < s1->nr_parms; i++) {
> +		if (!types__match(encoder, btf1, s1->parms[i].type_id,
> +				  btf2, s2->parms[i].type_id)) {
> +			if (encoder->verbose) {
> +				const char *p1 = btf__name_by_offset(btf1, s1->parms[i].name_off);
> +				const char *p2 = btf__name_by_offset(btf2, s2->parms[i].name_off);
> +
> +				btf_encoder__log_func_skip(encoder, func,
> +							   "param type mismatch for param#%d %s %s %s\n",
> +							   i + 1,
> +							   p1 ?: "",
> +							   p1 && p2 ? "!=" : "",
> +							   p2 ?: "");
> +			}
>  			return false;
>  		}
>  	}
> @@ -886,119 +1047,212 @@ static bool funcs__match(struct btf_encoder *encoder, struct elf_function *func,

[...]

> -static int32_t btf_encoder__add_func(struct btf_encoder *encoder, struct function *fn)
> +static int32_t btf_encoder__add_func(struct btf_encoder *encoder, struct function *fn, struct elf_function *func)
>  {

[...]

> +	if (!fn) {
> +		struct btf_encoder_func_state *state = &func->state;
> +		uint16_t idx;
> +
> +		if (!state || state->nr_annots == 0)
> +			return 0;
> +
> +		for (idx = 0; idx < state->nr_annots; idx++) {
> +			struct btf_encoder_func_annot *a = &state->annots[idx];
> +
> +			value = btf__str_by_offset(encoder->btf, a->value);
> +			/* adding BTF data may result in a mode of the
> +			 * value string memory, so make a temporary copy.
> +			 */
> +			strncpy(tmp_value, value, sizeof(tmp_value));

(Second warning from gcc 14.2.1 is reported here)

> +			component_idx = a->component_idx;
> +
> +			tag_type_id = btf_encoder__add_decl_tag(encoder, tmp_value, btf_fn_id, component_idx);
> +			if (tag_type_id < 0)
> +				break;
> +		}

[...]

Re: [PATCH v2 dwarves 1/3] btf_encoder: record BTF-centric function state instead of DWARF-centric

[Alan Maguire]

On 20/09/2024 20:18, Eduard Zingerman wrote:
> On Mon, 2024-09-16 at 14:49 +0100, Alan Maguire wrote:
> 
> [...]
> 
>> When this is done, memory consumption peaks at 1Gb rather than
>> ~4Gb for vmlinux generation.  Time taken appears to be approximately
>> the same for -j1, but slightly faster for multiple threads;
>> for example:
> 
> Hi Alan,
> 
> Sorry for delayed reply.
> I tried this patch with selftests vmlinux and see huge reduction indeed:
> 
> $ /usr/bin/time -v \
>     pahole -j1 --btf_features=consistent_func --btf_encode_detached=old.btf vmlinux
>   ...
>   Maximum resident set size (kbytes): 3123660
> 
> $ /usr/bin/time -v \
>     pahole -j1 --btf_features=consistent_func --btf_encode_detached=new.btf vmlinux
>   ...
>   Maximum resident set size (kbytes): 464400
> 
> [...]
> 
>> In terms of functions encoded, 360 fewer functions make it into
>> BTF due to the different approach in consistency checking, but after
>> examining these cases, they do appear to be legitimately inconsistent
>> functions where the optimized versions have parameter mismatches
>> with the non-optimized expectations.
>>
>> Mileage may vary of course, and any testing folks could do would
>> be greatly appreciated!
> 
> Interestingly, I don't see any difference in generated "consistent"
> funcs (BPF selftests kernel config, compiled using mainline clang):
> 
> $ comm -3 <(bpftool btf dump file new.btf | grep ' FUNC ' | sed "s/.*'\(.*\)'.*/\1/" | sort) \
>           <(bpftool btf dump file old.btf | grep ' FUNC ' | sed "s/.*'\(.*\)'.*/\1/" | sort) \
>           | tee | wc -l
> 0
> 
> A few nitpicks below, but overall looks great.
> 
> Reviewed-by: Eduard Zingerman <eddyz87@gmail.com>
>

Thanks for testing/reviewing! Replies below..

> [...]
> 
>> @@ -669,18 +692,25 @@ static int32_t btf_encoder__tag_type(struct btf_encoder *encoder, uint32_t tag_t
>>  	return encoder->type_id_off + tag_type;
>>  }
>>  
>> -static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct ftype *ftype)
>> +static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct ftype *ftype, struct elf_function *func)
> 
> It is a bit unfortunate that we now have this "doubled" representation
> and both ftype and elf_function pointers have to be passed through
> several layers.
> Ihor (in CC) is now working on having single btf_encoder for each
> processed CU (in order to be able to sort btf's by CU names before
> merging to a single BTF for dedup, thus making reproducible builds
> cheaper). I wonder if this could be further extended to have a
> separate BTF for functions and merge only consistent functions from
> this BTF to a final BTF.
> 

If I recall, Arnaldo's work ordered the DWARF CUs to be consistently
processed to allow for reproducibility. The only danger I see with the
approach of having a BTF encoder per CU is that currently we create an
ELF function table per encoder, whereas in reality all CUs are using the
same ELF. So for this approach to work I _think_ we'd need to share ELF
representations across encoders where appropriate.

> [...]
> 
>> @@ -694,20 +724,34 @@ static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct f
>>  
>>  	/* add parameters */
>>  	param_idx = 0;
>> -	ftype__for_each_parameter(ftype, param) {
>> -		const char *name = parameter__name(param);
>> +	if (ftype) {
>> +		ftype__for_each_parameter(ftype, param) {
>> +			const char *name = parameter__name(param);
>> +
>> +			type_id = param->tag.type == 0 ? 0 : encoder->type_id_off + param->tag.type;
>> +			++param_idx;
>> +			if (btf_encoder__add_func_param(encoder, name, type_id, param_idx == nr_params))
>> +				return -1;
>> +		}
>>  
>> -		type_id = param->tag.type == 0 ? 0 : encoder->type_id_off + param->tag.type;
>>  		++param_idx;
>> -		if (btf_encoder__add_func_param(encoder, name, type_id, param_idx == nr_params))
>> -			return -1;
>> -	}
>> +		if (ftype->unspec_parms)
>> +			if (btf_encoder__add_func_param(encoder, NULL, 0, param_idx == nr_params))
>> +				return -1;
>> +	} else {
>> +		for (param_idx = 0; param_idx < nr_params; param_idx++) {
>> +			struct btf_encoder_func_parm *p = &state->parms[param_idx];
>> +			name = btf__name_by_offset(btf, p->name_off);
>>  
>> -	++param_idx;
>> -	if (ftype->unspec_parms)
>> -		if (btf_encoder__add_func_param(encoder, NULL, 0, param_idx == nr_params))
>> -			return -1;
>> +			/* adding BTF data may result in a move of the
>> +			 * name string memory, so make a temporary copy.
>> +			 */
>> +			strncpy(tmp_name, name, sizeof(tmp_name));
> 
> When compiling with fresh gcc (14.2.1) there is a warning reported at this line:
> 
> btf_encoder.c:749:25: warning: ‘strncpy’ specified bound 128 equals destination size [-Wstringop-truncation]
>   749 |                         strncpy(tmp_name, name, sizeof(tmp_name));
>       |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Which seems to be a legit warning, given section CAVEATS from strncpy man page.
> There is also a second warning like this (see below).
> (when compiled with BUILD_TYPE=Release).
> 

thanks, will fix.

>>  
>> +			if (btf_encoder__add_func_param(encoder, tmp_name, p->type_id, param_idx == nr_params))
>> +				return -1;
>> +		}
>> +	}
>>  	return id;
>>  }
>>  
>> @@ -831,53 +875,170 @@ static int32_t btf_encoder__add_decl_tag(struct btf_encoder *encoder, const char
> 
> [...]
> 
>> -static bool funcs__match(struct btf_encoder *encoder, struct elf_function *func, struct function *f2)
>> +static bool names__match(struct btf *btf1, const struct btf_type *t1,
>> +			struct btf *btf2, const struct btf_type *t2)
>>  {
> 
> [...]
> 
>> +static bool types__match(struct btf_encoder *encoder,
>> +			 struct btf *btf1, int type_id1,
>> +			 struct btf *btf2, int type_id2)
>> +{
> 
> [...]
> 
>> +		switch (btf_kind(t1)) {
>> +		case BTF_KIND_INT:
> 
> Nit: ints have a meaningful .size field and are followed by a u32 word,
>      both should be compared.
>

I'll add a comparison of the int info, ditto for float sizes.


>> +		case BTF_KIND_FLOAT:
>> +		case BTF_KIND_FWD:
> 
> Nit: BTF_KIND_FWD should go to default, fwd__kind steps over it.
>

yep, we should switch (k1) (since it resolves fwd kinds) and remove the
fwd-specific case.

>> +		case BTF_KIND_TYPEDEF:
>> +		case BTF_KIND_STRUCT:
>> +		case BTF_KIND_UNION:
>> +		case BTF_KIND_ENUM:
>> +		case BTF_KIND_ENUM64:
>> +			return names__match(btf1, t1, btf2, t2);
>> +		case BTF_KIND_PTR:
>> +		case BTF_KIND_VOLATILE:
>> +		case BTF_KIND_CONST:
>> +		case BTF_KIND_RESTRICT:
>> +		case BTF_KIND_TYPE_TAG:
>> +			id1 = t1->type;
>> +			id2 = t2->type;
>> +			break;
>> +		case BTF_KIND_ARRAY: {
>> +			const struct btf_array *a1 = btf_array(t1);
>> +			const struct btf_array *a2 = btf_array(t2);
>> +
>> +			if (a1->nelems != a2->nelems)
>> +				return false;
>> +			id1 = a1->type;
>> +			id2 = a2->type;
>> +			break;
>> +		}
>> +		case BTF_KIND_FUNC_PROTO: {
>> +			const struct btf_param *p1 = btf_params(t1);
>> +			const struct btf_param *p2 = btf_params(t2);
>> +			int i, vlen = btf_vlen(t1);
>> +
>> +			if (vlen != btf_vlen(t2))
>> +				return false;
>> +			if (!types__match(encoder, btf1, t1->type,
>> +					  btf2, t2->type))
>> +				return false;
>> +			for (i = 0; i < vlen; i++, p1++, p2++) {
>> +				if (!types__match(encoder, btf1, t1->type,
>> +						  btf2, t2->type))
>> +					return false;
>> +			}
>> +			return true;
>> +		}
>> +		default:
>> +			return false;
>> +		}
>> +	} while (1);
>> +
>> +	return false;
>> +}
>> +
>> +static bool funcs__match(struct btf_encoder *encoder, struct elf_function *func,
>> +			 struct btf *btf1, struct btf_encoder_func_state *s1,
>> +			 struct btf *btf2, struct btf_encoder_func_state *s2)
>> +{
>> +	uint8_t i;
>> +
>> +	if (!s1->initialized || !s2->initialized)
>>  		return true;
> 
> Nit: is it even possible to get here when !s1->initialized || !s2->initialized?
>      Maybe log something about inconsistent state?
> 

you're right; it's not possible to get here without initialized being
set for both; I've removed the check.

>>  
>> -	if (!func->state.got_proto)
>> -		func->state.got_proto = proto__get(f1, func->state.proto, sizeof(func->state.proto));
>> +	if (s1->nr_parms != s2->nr_parms) {
>> +		btf_encoder__log_func_skip(encoder, func,
>> +					   "param count mismatch; %d params != %d params\n",
>> +					   s1->nr_parms, s2->nr_parms);
>> +		return false;
>> +	}
>> +	if (!types__match(encoder, btf1, s1->ret_type_id, btf2, s2->ret_type_id)) {
>> +		btf_encoder__log_func_skip(encoder, func, "return type mismatch\n");
>> +		return false;
>> +	}
>> +	if (s1->nr_parms == 0)
>> +		return true;
>>  
>> -	if (proto__get(f2, proto, sizeof(proto))) {
>> -		if (strcmp(func->state.proto, proto) != 0) {
>> -			if (encoder->verbose)
>> -				printf("function mismatch for '%s'('%s'): '%s' != '%s'\n",
>> -				       name, f1->alias ?: name,
>> -				       func->state.proto, proto);
>> +	for (i = 0; i < s1->nr_parms; i++) {
>> +		if (!types__match(encoder, btf1, s1->parms[i].type_id,
>> +				  btf2, s2->parms[i].type_id)) {
>> +			if (encoder->verbose) {
>> +				const char *p1 = btf__name_by_offset(btf1, s1->parms[i].name_off);
>> +				const char *p2 = btf__name_by_offset(btf2, s2->parms[i].name_off);
>> +
>> +				btf_encoder__log_func_skip(encoder, func,
>> +							   "param type mismatch for param#%d %s %s %s\n",
>> +							   i + 1,
>> +							   p1 ?: "",
>> +							   p1 && p2 ? "!=" : "",
>> +							   p2 ?: "");
>> +			}
>>  			return false;
>>  		}
>>  	}
>> @@ -886,119 +1047,212 @@ static bool funcs__match(struct btf_encoder *encoder, struct elf_function *func,
> 
> [...]
> 
>> -static int32_t btf_encoder__add_func(struct btf_encoder *encoder, struct function *fn)
>> +static int32_t btf_encoder__add_func(struct btf_encoder *encoder, struct function *fn, struct elf_function *func)
>>  {
> 
> [...]
> 
>> +	if (!fn) {
>> +		struct btf_encoder_func_state *state = &func->state;
>> +		uint16_t idx;
>> +
>> +		if (!state || state->nr_annots == 0)
>> +			return 0;
>> +
>> +		for (idx = 0; idx < state->nr_annots; idx++) {
>> +			struct btf_encoder_func_annot *a = &state->annots[idx];
>> +
>> +			value = btf__str_by_offset(encoder->btf, a->value);
>> +			/* adding BTF data may result in a mode of the
>> +			 * value string memory, so make a temporary copy.
>> +			 */
>> +			strncpy(tmp_value, value, sizeof(tmp_value));
> 
> (Second warning from gcc 14.2.1 is reported here)
>

fixed too. thanks!


>> +			component_idx = a->component_idx;
>> +
>> +			tag_type_id = btf_encoder__add_decl_tag(encoder, tmp_value, btf_fn_id, component_idx);
>> +			if (tag_type_id < 0)
>> +				break;
>> +		}
> 
> [...]
> 

Re: [PATCH v2 dwarves 1/3] btf_encoder: record BTF-centric function state instead of DWARF-centric

[Ihor Solodrai]

On Tuesday, September 24th, 2024 at 7:31 AM, Alan Maguire <alan.maguire@oracle.com> wrote:

[...]
> 
> On 20/09/2024 20:18, Eduard Zingerman wrote:
> 
> > [...]
> > 
> > > @@ -669,18 +692,25 @@ static int32_t btf_encoder__tag_type(struct btf_encoder *encoder, uint32_t tag_t
> > > return encoder->type_id_off + tag_type;
> > > }
> > > 
> > > -static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct ftype *ftype)
> > > +static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct ftype *ftype, struct elf_function *func)
> > 
> > It is a bit unfortunate that we now have this "doubled" representation
> > and both ftype and elf_function pointers have to be passed through
> > several layers.
> > Ihor (in CC) is now working on having single btf_encoder for each
> > processed CU (in order to be able to sort btf's by CU names before
> > merging to a single BTF for dedup, thus making reproducible builds
> > cheaper). I wonder if this could be further extended to have a
> > separate BTF for functions and merge only consistent functions from
> > this BTF to a final BTF.
> 
> 
> If I recall, Arnaldo's work ordered the DWARF CUs to be consistently
> processed to allow for reproducibility. The only danger I see with the
> approach of having a BTF encoder per CU is that currently we create an
> ELF function table per encoder, whereas in reality all CUs are using the
> same ELF. So for this approach to work I think we'd need to share ELF
> representations across encoders where appropriate.

Hi Alan, Arnaldo, Eduard.

Yes, this is exactly the problem I ran into while experimenting last
week. On vmlinux with selftests config, the tables gathered by
btf_encoder__collect_symbols in btf_encoder__new take around 50Mb of
space (as measured by getrusage [1]). And so having a separate copies
of them for each encoder while creating an encoder for each CU is a
non-starter: there are 2k+ of CUs in vmlinux.

Currently this is not a big problem as the number of encoders equals
to a number of worker threads for non-reproducible builds. Although
it's still a couple of hundred megabytes of likely unnecessary memory
usage.

I talked about this with Eduard off-list, and I was going to try
making these tables shared/global, as they are used mostly for search.
The only write operations on these tables that Eduard and I could find
is in btf_encoder__save_func, and it should be easy to add a lock
there, or even use some kind of atomic write.

I think at this point making ELF function/variable tables shared
between BTF encoders is a good optimization by itself.

However, as I understand, Eduard suggests that these tables might be
unnecessary at all, if the way of checking for and merging
inconsistent functions is changed. Eduard, please feel free to correct
me or expand on the idea.

Alan, Arnaldo, what do you think?

[1] https://man7.org/linux/man-pages/man2/getrusage.2.html

> 
> > [...]

Re: [PATCH v2 dwarves 1/3] btf_encoder: record BTF-centric function state instead of DWARF-centric

[Eduard Zingerman]

On Tue, 2024-09-24 at 19:02 +0000, Ihor Solodrai wrote:
> On Tuesday, September 24th, 2024 at 7:31 AM, Alan Maguire <alan.maguire@oracle.com> wrote:

[...]

> However, as I understand, Eduard suggests that these tables might be
> unnecessary at all, if the way of checking for and merging
> inconsistent functions is changed. Eduard, please feel free to correct
> me or expand on the idea.

I was thinking about keeping the functions in a separate BTF before
decision is made if that function has valid prototype.
But the table would still be necessary because of the flags tracked in:

struct parameter {
	struct tag tag;
	const char *name;
	uint8_t optimized:1;
	uint8_t unexpected_reg:1;
	uint8_t has_loc:1;
};

I'd say let's ignore this for now and do a simple refactoring with a
lock/atomic that you describe.

Re: [PATCH v2 dwarves 1/3] btf_encoder: record BTF-centric function state instead of DWARF-centric

[Jiri Olsa]

On Mon, Sep 16, 2024 at 02:49:44PM +0100, Alan Maguire wrote:

SNIP

> Baseline
> 
> $ time pahole -J vmlinux -j1 --btf_features=default
> 
> real	0m17.268s
> user	0m15.808s
> sys	0m1.415s
> 
> $ time pahole -J vmlinux -j8 --btf_features=default
> 
> real	0m10.768s
> user	0m30.793s
> sys	0m4.199s
> 
> With these changes:
> 
> $ time pahole -J vmlinux -j1 --btf_features=default
> 
> real	0m16.564s
> user	0m16.029s
> sys	0m0.492s
> 
> $ time pahole -J vmlinux -j8 --btf_features=default
> 
> real	0m8.332s
> user	0m30.627s
> sys	0m0.714s
> 
> In terms of functions encoded, 360 fewer functions make it into
> BTF due to the different approach in consistency checking, but after
> examining these cases, they do appear to be legitimately inconsistent
> functions where the optimized versions have parameter mismatches
> with the non-optimized expectations.
> 
> Mileage may vary of course, and any testing folks could do would
> be greatly appreciated!

looks good, some comments below.. I was hoping to find some way
to split the change, but can't think of any ;-)

thanks,
jirka


SNIP

> -static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct ftype *ftype)
> +static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct ftype *ftype, struct elf_function *func)
>  {
>  	struct btf *btf = encoder->btf;
>  	const struct btf_type *t;
>  	struct parameter *param;
>  	uint16_t nr_params, param_idx;
>  	int32_t id, type_id;
> +	char tmp_name[KSYM_NAME_LEN];
> +	const char *name;
> +	struct btf_encoder_func_state *state = &func->state;

func could be NULL right?

SNIP

>  static int32_t btf_encoder__save_func(struct btf_encoder *encoder, struct function *fn, struct elf_function *func)
>  {
> -	fn->priv = encoder->cu;
> -	if (func->function) {
> -		struct function *existing = func->function;
> -
> -		/* If saving and we find an existing entry, we want to merge
> -		 * observations across both functions, checking that the
> -		 * "seen optimized parameters", "inconsistent prototype"
> -		 * and "unexpected register" status is reflected in the
> -		 * the func entry.
> -		 * If the entry is new, record encoder state required
> -		 * to add the local function later (encoder + type_id_off)
> -		 * such that we can add the function later.
> -		 */
> -		existing->proto.optimized_parms |= fn->proto.optimized_parms;
> -		existing->proto.unexpected_reg |= fn->proto.unexpected_reg;
> -		if (!existing->proto.unexpected_reg && !existing->proto.inconsistent_proto &&
> -		     !funcs__match(encoder, func, fn))
> -			existing->proto.inconsistent_proto = 1;
> -	} else {
> -		func->state.type_id_off = encoder->type_id_off;
> -		func->function = fn;
> -		encoder->cu->functions_saved++;

we could remove functions_saved from cu now?

SNIP

> -static int32_t btf_encoder__add_func(struct btf_encoder *encoder, struct function *fn)
> +static int32_t btf_encoder__add_func(struct btf_encoder *encoder, struct function *fn, struct elf_function *func)
>  {
> -	int btf_fnproto_id, btf_fn_id, tag_type_id;
> -	struct llvm_annotation *annot;
> +	int btf_fnproto_id, btf_fn_id, tag_type_id = 0;
> +	int16_t component_idx = -1;
>  	const char *name;
> +	const char *value;
> +	char tmp_value[KSYM_NAME_LEN];
>  
> -	btf_fnproto_id = btf_encoder__add_func_proto(encoder, &fn->proto);
> -	name = function__name(fn);
> -	btf_fn_id = btf_encoder__add_ref_type(encoder, BTF_KIND_FUNC, btf_fnproto_id, name, false);
> +	btf_fnproto_id = btf_encoder__add_func_proto(encoder, fn ? &fn->proto : NULL, func);
> +	name = func->alias ?: func->name;
> +	if (btf_fnproto_id >= 0)
> +		btf_fn_id = btf_encoder__add_ref_type(encoder, BTF_KIND_FUNC, btf_fnproto_id, name, false);
>  	if (btf_fnproto_id < 0 || btf_fn_id < 0) {
> -		printf("error: failed to encode function '%s'\n", function__name(fn));
> +		printf("error: failed to encode function '%s': invalid %s\n", name, btf_fnproto_id < 0 ? "proto" : "func");
>  		return -1;
>  	}
> -	list_for_each_entry(annot, &fn->annots, node) {
> -		tag_type_id = btf_encoder__add_decl_tag(encoder, annot->value, btf_fn_id,
> -							annot->component_idx);
> -		if (tag_type_id < 0) {
> -			fprintf(stderr, "error: failed to encode tag '%s' to func %s with component_idx %d\n",
> -				annot->value, name, annot->component_idx);
> -			return -1;
> +	if (!fn) {
> +		struct btf_encoder_func_state *state = &func->state;
> +		uint16_t idx;
> +
> +		if (!state || state->nr_annots == 0)

it probably can't happen, but state will be allways != NULL in here.. should it be:

		if (!func || state->nr_annots == 0)


> +			return 0;
> +
> +		for (idx = 0; idx < state->nr_annots; idx++) {
> +			struct btf_encoder_func_annot *a = &state->annots[idx];
> +
> +			value = btf__str_by_offset(encoder->btf, a->value);
> +			/* adding BTF data may result in a mode of the
> +			 * value string memory, so make a temporary copy.
> +			 */
> +			strncpy(tmp_value, value, sizeof(tmp_value));
> +			component_idx = a->component_idx;
> +
> +			tag_type_id = btf_encoder__add_decl_tag(encoder, tmp_value, btf_fn_id, component_idx);
> +			if (tag_type_id < 0)
> +				break;
> +		}
> +	} else {
> +		struct llvm_annotation *annot;
> +
> +		list_for_each_entry(annot, &fn->annots, node) {
> +			value = annot->value;
> +			component_idx = annot->component_idx;
> +
> +			tag_type_id = btf_encoder__add_decl_tag(encoder, value, btf_fn_id,
> +								component_idx);
> +			if (tag_type_id < 0)
> +				break;
>  		}
>  	}
> +	if (tag_type_id < 0) {
> +		fprintf(stderr, "error: failed to encode tag '%s' to func %s with component_idx %d\n",
> +			value, name, component_idx);
> +		return -1;
> +	}
> +
>  	return 0;
>  }
>  
> -static void btf_encoder__add_saved_funcs(struct btf_encoder *encoder)
> +static int btf_encoder__add_saved_funcs(struct btf_encoder *encoder)
>  {
>  	int i;
>  
>  	for (i = 0; i < encoder->functions.cnt; i++) {
>  		struct elf_function *func = &encoder->functions.entries[i];
> -		struct function *fn = func->function;
> -		struct btf_encoder *other_encoder;
> +		struct btf_encoder_func_state *state = &func->state;
> +		struct btf_encoder *other_encoder = NULL;
>  
> -		if (!fn || fn->proto.processed)
> +		if (!state || !state->initialized || state->processed)
>  			continue;

state is now placed directly in elf_function so will allways be state != NULL

> -
>  		/* merge optimized-out status across encoders; since each
>  		 * encoder has the same elf symbol table we can use the
>  		 * same index to access the same elf symbol.
>  		 */
>  		btf_encoders__for_each_encoder(other_encoder) {
> -			struct function *other_fn;
> +			struct elf_function *other_func;
> +			struct btf_encoder_func_state *other_state;
> +			uint8_t optimized, unexpected, inconsistent;
>  
>  			if (other_encoder == encoder)
>  				continue;
>  
> -			other_fn = other_encoder->functions.entries[i].function;
> -			if (!other_fn)
> +			other_func = &other_encoder->functions.entries[i];
> +			other_state = &other_func->state;
> +			if (!other_state)
>  				continue;

same as above it will allways be other_state != NULL

> -			fn->proto.optimized_parms |= other_fn->proto.optimized_parms;
> -			fn->proto.unexpected_reg |= other_fn->proto.unexpected_reg;
> -			if (other_fn->proto.inconsistent_proto)
> -				fn->proto.inconsistent_proto = 1;
> -			if (!fn->proto.unexpected_reg && !fn->proto.inconsistent_proto &&
> -			    !funcs__match(encoder, func, other_fn))
> -				fn->proto.inconsistent_proto = 1;
> -			other_fn->proto.processed = 1;
> +			optimized = state->optimized_parms | other_state->optimized_parms;
> +			unexpected = state->unexpected_reg | other_state->unexpected_reg;
> +			inconsistent = state->inconsistent_proto | other_state->inconsistent_proto;
> +			if (!unexpected && !inconsistent &&
> +			    !funcs__match(encoder, func,
> +					  encoder->btf, state,
> +					  other_encoder->btf, other_state))
> +				inconsistent = 1;
> +			state->optimized_parms = other_state->optimized_parms = optimized;
> +			state->unexpected_reg = other_state->unexpected_reg = unexpected;
> +			state->inconsistent_proto = other_state->inconsistent_proto = inconsistent;
> +
> +			other_state->processed = 1;

SNIP

Re: [PATCH v2 dwarves 1/3] btf_encoder: record BTF-centric function state instead of DWARF-centric

[Alan Maguire]

On 23/09/2024 15:19, Jiri Olsa wrote:
> On Mon, Sep 16, 2024 at 02:49:44PM +0100, Alan Maguire wrote:
> 
> SNIP
> 
>> Baseline
>>
>> $ time pahole -J vmlinux -j1 --btf_features=default
>>
>> real	0m17.268s
>> user	0m15.808s
>> sys	0m1.415s
>>
>> $ time pahole -J vmlinux -j8 --btf_features=default
>>
>> real	0m10.768s
>> user	0m30.793s
>> sys	0m4.199s
>>
>> With these changes:
>>
>> $ time pahole -J vmlinux -j1 --btf_features=default
>>
>> real	0m16.564s
>> user	0m16.029s
>> sys	0m0.492s
>>
>> $ time pahole -J vmlinux -j8 --btf_features=default
>>
>> real	0m8.332s
>> user	0m30.627s
>> sys	0m0.714s
>>
>> In terms of functions encoded, 360 fewer functions make it into
>> BTF due to the different approach in consistency checking, but after
>> examining these cases, they do appear to be legitimately inconsistent
>> functions where the optimized versions have parameter mismatches
>> with the non-optimized expectations.
>>
>> Mileage may vary of course, and any testing folks could do would
>> be greatly appreciated!
> 
> looks good, some comments below.. I was hoping to find some way
> to split the change, but can't think of any ;-)
>

thanks for trying it out and looking at the code Jiri! Replies below..


> thanks,
> jirka
> 
> 
> SNIP
> 
>> -static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct ftype *ftype)
>> +static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct ftype *ftype, struct elf_function *func)
>>  {
>>  	struct btf *btf = encoder->btf;
>>  	const struct btf_type *t;
>>  	struct parameter *param;
>>  	uint16_t nr_params, param_idx;
>>  	int32_t id, type_id;
>> +	char tmp_name[KSYM_NAME_LEN];
>> +	const char *name;
>> +	struct btf_encoder_func_state *state = &func->state;
> 
> func could be NULL right?
>

good catch; I've moved the assignment to the code that deals with ELF
function prototype addition.

> SNIP
> 
>>  static int32_t btf_encoder__save_func(struct btf_encoder *encoder, struct function *fn, struct elf_function *func)
>>  {
>> -	fn->priv = encoder->cu;
>> -	if (func->function) {
>> -		struct function *existing = func->function;
>> -
>> -		/* If saving and we find an existing entry, we want to merge
>> -		 * observations across both functions, checking that the
>> -		 * "seen optimized parameters", "inconsistent prototype"
>> -		 * and "unexpected register" status is reflected in the
>> -		 * the func entry.
>> -		 * If the entry is new, record encoder state required
>> -		 * to add the local function later (encoder + type_id_off)
>> -		 * such that we can add the function later.
>> -		 */
>> -		existing->proto.optimized_parms |= fn->proto.optimized_parms;
>> -		existing->proto.unexpected_reg |= fn->proto.unexpected_reg;
>> -		if (!existing->proto.unexpected_reg && !existing->proto.inconsistent_proto &&
>> -		     !funcs__match(encoder, func, fn))
>> -			existing->proto.inconsistent_proto = 1;
>> -	} else {
>> -		func->state.type_id_off = encoder->type_id_off;
>> -		func->function = fn;
>> -		encoder->cu->functions_saved++;
> 
> we could remove functions_saved from cu now?
> 

good idea; removed that and the "processed" flag from ftypes.

> SNIP
> 
>> -static int32_t btf_encoder__add_func(struct btf_encoder *encoder, struct function *fn)
>> +static int32_t btf_encoder__add_func(struct btf_encoder *encoder, struct function *fn, struct elf_function *func)
>>  {
>> -	int btf_fnproto_id, btf_fn_id, tag_type_id;
>> -	struct llvm_annotation *annot;
>> +	int btf_fnproto_id, btf_fn_id, tag_type_id = 0;
>> +	int16_t component_idx = -1;
>>  	const char *name;
>> +	const char *value;
>> +	char tmp_value[KSYM_NAME_LEN];
>>  
>> -	btf_fnproto_id = btf_encoder__add_func_proto(encoder, &fn->proto);
>> -	name = function__name(fn);
>> -	btf_fn_id = btf_encoder__add_ref_type(encoder, BTF_KIND_FUNC, btf_fnproto_id, name, false);
>> +	btf_fnproto_id = btf_encoder__add_func_proto(encoder, fn ? &fn->proto : NULL, func);
>> +	name = func->alias ?: func->name;
>> +	if (btf_fnproto_id >= 0)
>> +		btf_fn_id = btf_encoder__add_ref_type(encoder, BTF_KIND_FUNC, btf_fnproto_id, name, false);
>>  	if (btf_fnproto_id < 0 || btf_fn_id < 0) {
>> -		printf("error: failed to encode function '%s'\n", function__name(fn));
>> +		printf("error: failed to encode function '%s': invalid %s\n", name, btf_fnproto_id < 0 ? "proto" : "func");
>>  		return -1;
>>  	}
>> -	list_for_each_entry(annot, &fn->annots, node) {
>> -		tag_type_id = btf_encoder__add_decl_tag(encoder, annot->value, btf_fn_id,
>> -							annot->component_idx);
>> -		if (tag_type_id < 0) {
>> -			fprintf(stderr, "error: failed to encode tag '%s' to func %s with component_idx %d\n",
>> -				annot->value, name, annot->component_idx);
>> -			return -1;
>> +	if (!fn) {
>> +		struct btf_encoder_func_state *state = &func->state;
>> +		uint16_t idx;
>> +
>> +		if (!state || state->nr_annots == 0)
> 
> it probably can't happen, but state will be allways != NULL in here.. should it be:
> 
> 		if (!func || state->nr_annots == 0)
> 

I've added assert()s to the btf_encoder__add_func[proto]() functions to
ensure that either the fn or func are non-NULL, since winding up with a
NULL fn/func is more of a programming error than a state we can wind up
in.
> 
>> +			return 0;
>> +
>> +		for (idx = 0; idx < state->nr_annots; idx++) {
>> +			struct btf_encoder_func_annot *a = &state->annots[idx];
>> +
>> +			value = btf__str_by_offset(encoder->btf, a->value);
>> +			/* adding BTF data may result in a mode of the
>> +			 * value string memory, so make a temporary copy.
>> +			 */
>> +			strncpy(tmp_value, value, sizeof(tmp_value));
>> +			component_idx = a->component_idx;
>> +
>> +			tag_type_id = btf_encoder__add_decl_tag(encoder, tmp_value, btf_fn_id, component_idx);
>> +			if (tag_type_id < 0)
>> +				break;
>> +		}
>> +	} else {
>> +		struct llvm_annotation *annot;
>> +
>> +		list_for_each_entry(annot, &fn->annots, node) {
>> +			value = annot->value;
>> +			component_idx = annot->component_idx;
>> +
>> +			tag_type_id = btf_encoder__add_decl_tag(encoder, value, btf_fn_id,
>> +								component_idx);
>> +			if (tag_type_id < 0)
>> +				break;
>>  		}
>>  	}
>> +	if (tag_type_id < 0) {
>> +		fprintf(stderr, "error: failed to encode tag '%s' to func %s with component_idx %d\n",
>> +			value, name, component_idx);
>> +		return -1;
>> +	}
>> +
>>  	return 0;
>>  }
>>  
>> -static void btf_encoder__add_saved_funcs(struct btf_encoder *encoder)
>> +static int btf_encoder__add_saved_funcs(struct btf_encoder *encoder)
>>  {
>>  	int i;
>>  
>>  	for (i = 0; i < encoder->functions.cnt; i++) {
>>  		struct elf_function *func = &encoder->functions.entries[i];
>> -		struct function *fn = func->function;
>> -		struct btf_encoder *other_encoder;
>> +		struct btf_encoder_func_state *state = &func->state;
>> +		struct btf_encoder *other_encoder = NULL;
>>  
>> -		if (!fn || fn->proto.processed)
>> +		if (!state || !state->initialized || state->processed)
>>  			continue;
> 
> state is now placed directly in elf_function so will allways be state != NULL
> 
>> -
>>  		/* merge optimized-out status across encoders; since each
>>  		 * encoder has the same elf symbol table we can use the
>>  		 * same index to access the same elf symbol.
>>  		 */
>>  		btf_encoders__for_each_encoder(other_encoder) {
>> -			struct function *other_fn;
>> +			struct elf_function *other_func;
>> +			struct btf_encoder_func_state *other_state;
>> +			uint8_t optimized, unexpected, inconsistent;
>>  
>>  			if (other_encoder == encoder)
>>  				continue;
>>  
>> -			other_fn = other_encoder->functions.entries[i].function;
>> -			if (!other_fn)
>> +			other_func = &other_encoder->functions.entries[i];
>> +			other_state = &other_func->state;
>> +			if (!other_state)
>>  				continue;
> 
> same as above it will allways be other_state != NULL
> 

both fixed now. thanks!


>> -			fn->proto.optimized_parms |= other_fn->proto.optimized_parms;
>> -			fn->proto.unexpected_reg |= other_fn->proto.unexpected_reg;
>> -			if (other_fn->proto.inconsistent_proto)
>> -				fn->proto.inconsistent_proto = 1;
>> -			if (!fn->proto.unexpected_reg && !fn->proto.inconsistent_proto &&
>> -			    !funcs__match(encoder, func, other_fn))
>> -				fn->proto.inconsistent_proto = 1;
>> -			other_fn->proto.processed = 1;
>> +			optimized = state->optimized_parms | other_state->optimized_parms;
>> +			unexpected = state->unexpected_reg | other_state->unexpected_reg;
>> +			inconsistent = state->inconsistent_proto | other_state->inconsistent_proto;
>> +			if (!unexpected && !inconsistent &&
>> +			    !funcs__match(encoder, func,
>> +					  encoder->btf, state,
>> +					  other_encoder->btf, other_state))
>> +				inconsistent = 1;
>> +			state->optimized_parms = other_state->optimized_parms = optimized;
>> +			state->unexpected_reg = other_state->unexpected_reg = unexpected;
>> +			state->inconsistent_proto = other_state->inconsistent_proto = inconsistent;
>> +
>> +			other_state->processed = 1;
> 
> SNIP

[PATCH v2 dwarves 2/3] pfunct: show all functions that match filter criteria

[Alan Maguire]

currently, "pfunct --function <name> vmlinux" will bail once it finds
an instance of a function; add an option

  -A, --all                  show all functions that match filter, or show all
                             function prototypes if no filter is specified

..that allows us to see all instances of a function.  It will be
useful in testing functions skipped by BTF encoding.

When --function is not specified, all prototypes are displayed but
we make use of the pfunct_stealer for this case as it massively
speeds up processing from ~6min to ~12sec.

It will be useful in tests to be able to collect prototypes quickly.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 pfunct.c | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)

diff --git a/pfunct.c b/pfunct.c
index bc3026b..e6cb898 100644
--- a/pfunct.c
+++ b/pfunct.c
@@ -29,6 +29,7 @@ static int show_cc_inlined;
 static int show_cc_uninlined;
 static char *symtab_name;
 static bool show_prototypes;
+static bool show_all_matches;
 static bool expand_types;
 static bool compilable_output;
 static struct type_emissions emissions;
@@ -368,17 +369,19 @@ do_parameters:
 static void function__show(struct function *func, struct cu *cu)
 {
 	struct tag *tag = function__tag(func);
+	struct fn_stats *fstats = NULL;
 
 	if (func->abstract_origin || func->declaration)
 		return;
 
-	struct fn_stats *fstats = fn_stats__find(func->name);
+	if (!show_all_matches) {
+		fstats = fn_stats__find(func->name);
 
-	if (fstats && fstats->printed)
-		return;
-
-	if (expand_types)
-		function__emit_type_definitions(func, cu, stdout);
+		if (fstats && fstats->printed)
+			return;
+		if (expand_types)
+			function__emit_type_definitions(func, cu, stdout);
+	}
 	tag__fprintf(tag, cu, &conf, stdout);
 	if (compilable_output) {
 		struct tag *type = cu__type(cu, func->proto.tag.type);
@@ -399,6 +402,8 @@ static void function__show(struct function *func, struct cu *cu)
 		fprintf(stdout, "\n}\n");
 	}
 	putchar('\n');
+	if (show_all_matches)
+		return;
 	if (show_variables || show_inline_expansions)
 		function__fprintf_stats(tag, cu, &conf, stdout);
 
@@ -417,8 +422,6 @@ static int cu_function_iterator(struct cu *cu, void *cookie)
 		if (cookie && strcmp(function__name(function), cookie) != 0)
 			continue;
 		function__show(function, cu);
-		if (!expand_types)
-			return 1;
 	}
 	return 0;
 }
@@ -514,12 +517,19 @@ static enum load_steal_kind pfunct_stealer(struct cu *cu,
 
 		if (tag) {
 			function__show(tag__function(tag), cu);
-			return LSK__STOP_LOADING;
+			return show_all_matches ? LSK__DELETE : LSK__STOP_LOADING;
 		}
 	} else if (class_name) {
 		cu_class_iterator(cu, class_name);
+	} else if (show_all_matches) {
+		struct function *pos;
+		uint32_t id;
+
+		cu__for_each_function(cu, id, pos)
+			function__show(pos, cu);
 	}
 
+
 	return LSK__DELETE;
 }
 
@@ -537,6 +547,11 @@ static const struct argp_option pfunct__options[] = {
 		.arg  = "ADDR",
 		.doc  = "show just the function that where ADDR is",
 	},
+	{
+		.key  = 'A',
+		.name = "all",
+		.doc  = "show all functions that match filter, or show all function prototypes if no filter is specified",
+	},
 	{
 		.key  = 'b',
 		.name = "expand_types",
@@ -672,6 +687,7 @@ static error_t pfunct__options_parser(int key, char *arg,
 		break;
 	case 'a': addr = strtoull(arg, NULL, 0);
 		  conf_load.get_addr_info = true;	 break;
+	case 'A': show_all_matches = true;		 break;
 	case 'b': expand_types = true;
 		  type_emissions__init(&emissions, &conf);	 break;
 	case 'c': class_name = arg;			 break;
@@ -751,7 +767,7 @@ int main(int argc, char *argv[])
 		goto out_dwarves_exit;
 	}
 
-	if (function_name || class_name)
+	if (function_name || (!function_name && show_all_matches) || class_name)
 		conf_load.steal = pfunct_stealer;
 
 try_sole_arg_as_function_name:
-- 
2.43.5

[PATCH v2 dwarves 3/3] tests: add test validating BTF encoding, reasons we skip functions

[Alan Maguire]

use pahole to encode BTF and pfunct to check that

- DWARF functions that made it into BTF match signatures
- for functions we say we skipped, we did indeed skip them in BTF
  encoding; and
- it was correct to skip these functions

It is not possible to check everything, as pfunct does not
print "."-suffixed optimized representations, and it does not
analyze location calling conventions.  As a starting point we check
for

- functions encoded in BTF match signatures with DWARF
- functions with multiple incompatible return values
- functions with incompatible parameter count/type

This test automates manual validation of encoding/skipping,
and as such will be useful to ensure regressions are not
introduced, for comparing gcc/LLVM-based DWARF representations
etc.

The test takes ~5 minutes to run in all; output looks like this:

$ vmlinux=~/kbuild/bpf-next/vmlinux bash ./btf_functions.sh
Validation of BTF encoding of functions; this may take some time...
Matched 55969 functions exactly.
Matched 239 functions with inlines.
Matched 1 functions with multiple const/non-const instances.
Ok
Validation of skipped function logic...
Validating skipped functions are absent from BTF...
Skipped encoding 748 functions in BTF.
Ok
Validating skipped functions have incompatible return values...
Found 8 functions with multiple incompatible return values.
Ok
Validating skipped functions have incompatible params/counts...
Found 116 instances with multiple instances with incompatible parameters.
Found 2 instances where inline functions were not inlined and had incompatible parameters.
Found 351 instances where the function name suggests optimizations led to inconsistent parameters.
Ok

Suggested-by: Jiri Olsa <olsajiri@gmail.com>
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 tests/btf_functions.sh | 192 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 192 insertions(+)
 create mode 100755 tests/btf_functions.sh

diff --git a/tests/btf_functions.sh b/tests/btf_functions.sh
new file mode 100755
index 0000000..4509407
--- /dev/null
+++ b/tests/btf_functions.sh
@@ -0,0 +1,192 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Copyright (c) 2024, Oracle and/or its affiliates.
+#
+# Examine functions - especially those for which we skipped BTF encoding -
+# to validate that they were indeed skipped for BTF encoding, and that they
+# also should have been.
+#
+
+outdir=
+
+fail()
+{
+	# Do not remove test dir; might be useful for analysis
+	trap - EXIT
+	if [[ -d "$outdir" ]]; then
+		echo "Test data is in $outdir"
+	fi
+	exit 1
+}
+
+cleanup()
+{
+	rm ${outdir}/*
+	rmdir $outdir
+}
+
+vmlinux=${vmlinux:-$1}
+
+if [ -z "$vmlinux" ] ; then
+	vmlinux=$(pahole --running_kernel_vmlinux)
+	if [ -z "$vmlinux" ] ; then
+		echo "Please specify a vmlinux file to operate on"
+		exit 1
+	fi
+fi
+
+if [ ! -f "$vmlinux" ] ; then
+	echo "$vmlinux file not available, please specify another"
+	exit 1
+fi
+
+outdir=$(mktemp -d /tmp/btf_functions.sh.XXXXXX)
+
+trap cleanup EXIT
+
+test -n "$VERBOSE" && printf "Encoding..."
+
+pahole --btf_features=default --btf_encode_detached=$outdir/vmlinux.btf --verbose $vmlinux |grep "skipping BTF encoding of function" > ${outdir}/skipped_fns
+
+test -n "$VERBOSE" && printf "done.\n"
+
+echo "Validation of BTF encoding of functions; this may take some time..."
+
+funcs=$(pfunct --format_path=btf $outdir/vmlinux.btf |sort)
+
+# all functions from DWARF; some inline functions are not inlined so include them too
+pfunct --all --no_parm_names --format_path=dwarf $vmlinux | \
+	awk '{ print $0}'|sort|uniq > $outdir/dwarf.funcs
+# all functions from BTF (removing bpf_kfunc prefix where found)
+pfunct --all --no_parm_names --format_path=btf $outdir/vmlinux.btf |\
+	awk '{ gsub("^bpf_kfunc ",""); print $0}'|sort|uniq > $outdir/btf.funcs
+
+exact=0
+inline=0
+const_insensitive=0
+
+for f in $funcs ; do
+	btf=$(grep " $f(" $outdir/btf.funcs)
+	# look for non-inline match first
+	dwarf=$(grep " $f(" $outdir/dwarf.funcs |egrep -v "^inline ")
+	if [[ "$btf" != "$dwarf" ]]; then
+		# function might be declared inline in DWARF.
+		inline_dwarf=$(grep " $f(" $outdir/dwarf.funcs |grep "^inline ")
+		if [[ "inline $btf" != "$inline_dwarf" ]]; then
+			# some functions have multiple instances in DWARF where one has
+			# const param(s) and another does not (see errpos()).  We do not
+			# mark these functions inconsistent as though they technically
+			# have different prototypes, the data itself is not different.
+			btf_noconst=$(echo $btf | awk '{gsub("const ",""); print $0 }')
+			dwarf_noconst=$(echo $dwarf | awk '{gsub("const ",""); print $0 }')
+			if [[ "$dwarf_noconst" =~ "$btf_noconst" ]]; then
+				const_insensitive=$((const_insensitive+1))
+			else
+				echo "ERROR: mismatch for '$f()' : BTF '$btf' not found; DWARF '$dwarf'"
+				fail
+			fi
+		else
+			inline=$((inline+1))
+		fi
+	else
+		exact=$((exact+1))
+	fi
+done
+
+echo "Matched $exact functions exactly."
+echo "Matched $inline functions with inlines."
+echo "Matched $const_insensitive functions with multiple const/non-const instances."
+echo "Ok"
+
+echo "Validation of skipped function logic..."
+
+skipped_cnt=$(wc -l ${outdir}/skipped_fns | awk '{ print $1}')
+
+if [[ "$skipped_cnt" == "0" ]]; then
+	echo "No skipped functions.  Done."
+	exit 0
+fi
+
+echo "Validating skipped functions are absent from BTF..."
+
+skipped_fns=$(awk '{print $1}' $outdir/skipped_fns)
+for s in $skipped_fns ; do
+	# Ensure the skipped function are not in BTF
+	inbtf=$(grep " $s(" $outdir/btf.funcs)
+	if [[ -n "$inbtf" ]]; then
+		echo "ERROR: '${s}()' was added incorrectly to BTF: '$inbtf'"
+		fail
+	fi
+done
+
+echo "Skipped encoding $skipped_cnt functions in BTF."
+echo "Ok"
+
+echo "Validating skipped functions have incompatible return values..."
+
+return_mismatches=$(awk '/return type mismatch/ { print $1 }' $outdir/skipped_fns)
+return_count=0
+
+for r in $return_mismatches ; do
+	# Ensure there are multiple instances with incompatible return values
+	grep " $r(" $outdir/dwarf.funcs | \
+	awk -v FN=$r '{i = index($0, FN); if (i>0) print substr($0, 0, i-1) }' \
+	| uniq > ${outdir}/retvals.$r
+	test -n "$VERBOSE" && echo "'${r}()' has return values:"
+	test -n "$VERBOSE" && cat ${outdir}/retvals.$r
+	cnt=$(wc -l ${outdir}/retvals.$r | awk '{ print $1 }')
+	if [[ $cnt -lt 2 ]]; then
+		echo "ERROR: '${r}()' has only one return value; it should not be reported as having incompatible return values"
+		fail
+	fi
+	return_count=$((return_count+1))
+done
+
+echo "Found $return_count functions with multiple incompatible return values."
+echo "Ok"
+
+echo "Validating skipped functions have incompatible params/counts..."
+
+param_mismatches=$(awk '/due to param / { print $1 }' $outdir/skipped_fns)
+
+multiple=0
+multiple_inline=0
+optimized=0
+warnings=0
+
+for p in $param_mismatches ; do
+	skipmsg=$(awk -v FN=$p '{ if ($1 == FN) print $0 }' $outdir/skipped_fns)
+	altname=$(echo $skipmsg | awk '{ i=index($2,")"); print substr($2,2,i-2); }')
+	if [[ "$altname" != "$p" ]]; then
+		test -n "$VERBOSE" && echo "skipping optimized function $p ($altname); pfunct may not reflect late optimizations."
+		optimized=$((optimized+1))
+		continue
+	fi
+	# Ensure there are multiple instances with incompatible params
+	grep " $p(" $outdir/dwarf.funcs | uniq > ${outdir}/protos.$p
+	test -n "$VERBOSE" && echo "'${p}()' has prototypes:"
+	test -n "$VERBOSE" && cat ${outdir}/protos.$p
+	cnt=$(wc -l ${outdir}/protos.$p | awk '{ print $1 }')
+	if [[ $cnt -lt 2 ]]; then
+		# function may be inlined in multiple sites with different protos
+		inlined=$(grep inline ${outdir}/protos.$p)
+		if [[ -n "$inlined" ]]; then
+			multiple_inline=$((multiple_inline+1))
+		else
+			echo "WARN: '${p}()' has only one prototype; if it was subject to late optimization, pfunct may not reflect inconsistencies pahole found."
+			echo "Full skip message from pahole: $skipmsg"
+			warnings=$((warnings+1))
+		fi
+	else
+		multiple=$((multiple+1))
+	fi
+done
+
+echo "Found $multiple instances with multiple instances with incompatible parameters."
+echo "Found $multiple_inline instances where inline functions were not inlined and had incompatible parameters."
+echo "Found $optimized instances where the function name suggests optimizations led to inconsistent parameters."
+echo "Found $warnings instances where pfunct did not notice inconsistencies."
+echo "Ok"
+
+exit 0
-- 
2.43.5

Re: [PATCH v2 dwarves 3/3] tests: add test validating BTF encoding, reasons we skip functions

[Arnaldo Carvalho de Melo]

On Mon, Sep 16, 2024 at 02:49:46PM +0100, Alan Maguire wrote:
> use pahole to encode BTF and pfunct to check that
> 
> - DWARF functions that made it into BTF match signatures
> - for functions we say we skipped, we did indeed skip them in BTF
>   encoding; and
> - it was correct to skip these functions
> 
> It is not possible to check everything, as pfunct does not
> print "."-suffixed optimized representations, and it does not
> analyze location calling conventions.  As a starting point we check
> for
> 
> - functions encoded in BTF match signatures with DWARF
> - functions with multiple incompatible return values
> - functions with incompatible parameter count/type
> 
> This test automates manual validation of encoding/skipping,
> and as such will be useful to ensure regressions are not
> introduced, for comparing gcc/LLVM-based DWARF representations
> etc.
> 
> The test takes ~5 minutes to run in all; output looks like this:
> 
> $ vmlinux=~/kbuild/bpf-next/vmlinux bash ./btf_functions.sh

I'm trying without specifying vmlinux, what we have now in the
tests/tests should find the one for the running kernel and use it, but
doing so I got tons of warnings about egrep usage, so I folded this:

diff --git a/tests/btf_functions.sh b/tests/btf_functions.sh
index 45094077809c1e3e..4a60a18845115d3d 100755
--- a/tests/btf_functions.sh
+++ b/tests/btf_functions.sh
@@ -69,7 +69,7 @@ const_insensitive=0
 for f in $funcs ; do
 	btf=$(grep " $f(" $outdir/btf.funcs)
 	# look for non-inline match first
-	dwarf=$(grep " $f(" $outdir/dwarf.funcs |egrep -v "^inline ")
+	dwarf=$(grep " $f(" $outdir/dwarf.funcs | grep -Ev "^inline ")
 	if [[ "$btf" != "$dwarf" ]]; then
 		# function might be declared inline in DWARF.
 		inline_dwarf=$(grep " $f(" $outdir/dwarf.funcs |grep "^inline ")

Running with it now:

root@x1:/home/acme/git/pahole# tests/tests 
  1: Validation of BTF encoding of functions; this may take some time...

Seems stuck, so:

root@x1:/home/acme/git/pahole# pahole --running_kernel_vmlinux
/usr/lib/debug/lib/modules/6.10.8-200.fc40.x86_64/vmlinux
root@x1:/home/acme/git/pahole#

Had to stop (traveling/LPC participation), so sending it now, what is
written above should already be useful :)

- Arnaldo

> Validation of BTF encoding of functions; this may take some time...
> Matched 55969 functions exactly.
> Matched 239 functions with inlines.
> Matched 1 functions with multiple const/non-const instances.
> Ok
> Validation of skipped function logic...
> Validating skipped functions are absent from BTF...
> Skipped encoding 748 functions in BTF.
> Ok
> Validating skipped functions have incompatible return values...
> Found 8 functions with multiple incompatible return values.
> Ok
> Validating skipped functions have incompatible params/counts...
> Found 116 instances with multiple instances with incompatible parameters.
> Found 2 instances where inline functions were not inlined and had incompatible parameters.
> Found 351 instances where the function name suggests optimizations led to inconsistent parameters.
> Ok
> 
> Suggested-by: Jiri Olsa <olsajiri@gmail.com>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>  tests/btf_functions.sh | 192 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 192 insertions(+)
>  create mode 100755 tests/btf_functions.sh
> 
> diff --git a/tests/btf_functions.sh b/tests/btf_functions.sh
> new file mode 100755
> index 0000000..4509407
> --- /dev/null
> +++ b/tests/btf_functions.sh
> @@ -0,0 +1,192 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# Copyright (c) 2024, Oracle and/or its affiliates.
> +#
> +# Examine functions - especially those for which we skipped BTF encoding -
> +# to validate that they were indeed skipped for BTF encoding, and that they
> +# also should have been.
> +#
> +
> +outdir=
> +
> +fail()
> +{
> +	# Do not remove test dir; might be useful for analysis
> +	trap - EXIT
> +	if [[ -d "$outdir" ]]; then
> +		echo "Test data is in $outdir"
> +	fi
> +	exit 1
> +}
> +
> +cleanup()
> +{
> +	rm ${outdir}/*
> +	rmdir $outdir
> +}
> +
> +vmlinux=${vmlinux:-$1}
> +
> +if [ -z "$vmlinux" ] ; then
> +	vmlinux=$(pahole --running_kernel_vmlinux)
> +	if [ -z "$vmlinux" ] ; then
> +		echo "Please specify a vmlinux file to operate on"
> +		exit 1
> +	fi
> +fi
> +
> +if [ ! -f "$vmlinux" ] ; then
> +	echo "$vmlinux file not available, please specify another"
> +	exit 1
> +fi
> +
> +outdir=$(mktemp -d /tmp/btf_functions.sh.XXXXXX)
> +
> +trap cleanup EXIT
> +
> +test -n "$VERBOSE" && printf "Encoding..."
> +
> +pahole --btf_features=default --btf_encode_detached=$outdir/vmlinux.btf --verbose $vmlinux |grep "skipping BTF encoding of function" > ${outdir}/skipped_fns
> +
> +test -n "$VERBOSE" && printf "done.\n"
> +
> +echo "Validation of BTF encoding of functions; this may take some time..."
> +
> +funcs=$(pfunct --format_path=btf $outdir/vmlinux.btf |sort)
> +
> +# all functions from DWARF; some inline functions are not inlined so include them too
> +pfunct --all --no_parm_names --format_path=dwarf $vmlinux | \
> +	awk '{ print $0}'|sort|uniq > $outdir/dwarf.funcs
> +# all functions from BTF (removing bpf_kfunc prefix where found)
> +pfunct --all --no_parm_names --format_path=btf $outdir/vmlinux.btf |\
> +	awk '{ gsub("^bpf_kfunc ",""); print $0}'|sort|uniq > $outdir/btf.funcs
> +
> +exact=0
> +inline=0
> +const_insensitive=0
> +
> +for f in $funcs ; do
> +	btf=$(grep " $f(" $outdir/btf.funcs)
> +	# look for non-inline match first
> +	dwarf=$(grep " $f(" $outdir/dwarf.funcs |egrep -v "^inline ")
> +	if [[ "$btf" != "$dwarf" ]]; then
> +		# function might be declared inline in DWARF.
> +		inline_dwarf=$(grep " $f(" $outdir/dwarf.funcs |grep "^inline ")
> +		if [[ "inline $btf" != "$inline_dwarf" ]]; then
> +			# some functions have multiple instances in DWARF where one has
> +			# const param(s) and another does not (see errpos()).  We do not
> +			# mark these functions inconsistent as though they technically
> +			# have different prototypes, the data itself is not different.
> +			btf_noconst=$(echo $btf | awk '{gsub("const ",""); print $0 }')
> +			dwarf_noconst=$(echo $dwarf | awk '{gsub("const ",""); print $0 }')
> +			if [[ "$dwarf_noconst" =~ "$btf_noconst" ]]; then
> +				const_insensitive=$((const_insensitive+1))
> +			else
> +				echo "ERROR: mismatch for '$f()' : BTF '$btf' not found; DWARF '$dwarf'"
> +				fail
> +			fi
> +		else
> +			inline=$((inline+1))
> +		fi
> +	else
> +		exact=$((exact+1))
> +	fi
> +done
> +
> +echo "Matched $exact functions exactly."
> +echo "Matched $inline functions with inlines."
> +echo "Matched $const_insensitive functions with multiple const/non-const instances."
> +echo "Ok"
> +
> +echo "Validation of skipped function logic..."
> +
> +skipped_cnt=$(wc -l ${outdir}/skipped_fns | awk '{ print $1}')
> +
> +if [[ "$skipped_cnt" == "0" ]]; then
> +	echo "No skipped functions.  Done."
> +	exit 0
> +fi
> +
> +echo "Validating skipped functions are absent from BTF..."
> +
> +skipped_fns=$(awk '{print $1}' $outdir/skipped_fns)
> +for s in $skipped_fns ; do
> +	# Ensure the skipped function are not in BTF
> +	inbtf=$(grep " $s(" $outdir/btf.funcs)
> +	if [[ -n "$inbtf" ]]; then
> +		echo "ERROR: '${s}()' was added incorrectly to BTF: '$inbtf'"
> +		fail
> +	fi
> +done
> +
> +echo "Skipped encoding $skipped_cnt functions in BTF."
> +echo "Ok"
> +
> +echo "Validating skipped functions have incompatible return values..."
> +
> +return_mismatches=$(awk '/return type mismatch/ { print $1 }' $outdir/skipped_fns)
> +return_count=0
> +
> +for r in $return_mismatches ; do
> +	# Ensure there are multiple instances with incompatible return values
> +	grep " $r(" $outdir/dwarf.funcs | \
> +	awk -v FN=$r '{i = index($0, FN); if (i>0) print substr($0, 0, i-1) }' \
> +	| uniq > ${outdir}/retvals.$r
> +	test -n "$VERBOSE" && echo "'${r}()' has return values:"
> +	test -n "$VERBOSE" && cat ${outdir}/retvals.$r
> +	cnt=$(wc -l ${outdir}/retvals.$r | awk '{ print $1 }')
> +	if [[ $cnt -lt 2 ]]; then
> +		echo "ERROR: '${r}()' has only one return value; it should not be reported as having incompatible return values"
> +		fail
> +	fi
> +	return_count=$((return_count+1))
> +done
> +
> +echo "Found $return_count functions with multiple incompatible return values."
> +echo "Ok"
> +
> +echo "Validating skipped functions have incompatible params/counts..."
> +
> +param_mismatches=$(awk '/due to param / { print $1 }' $outdir/skipped_fns)
> +
> +multiple=0
> +multiple_inline=0
> +optimized=0
> +warnings=0
> +
> +for p in $param_mismatches ; do
> +	skipmsg=$(awk -v FN=$p '{ if ($1 == FN) print $0 }' $outdir/skipped_fns)
> +	altname=$(echo $skipmsg | awk '{ i=index($2,")"); print substr($2,2,i-2); }')
> +	if [[ "$altname" != "$p" ]]; then
> +		test -n "$VERBOSE" && echo "skipping optimized function $p ($altname); pfunct may not reflect late optimizations."
> +		optimized=$((optimized+1))
> +		continue
> +	fi
> +	# Ensure there are multiple instances with incompatible params
> +	grep " $p(" $outdir/dwarf.funcs | uniq > ${outdir}/protos.$p
> +	test -n "$VERBOSE" && echo "'${p}()' has prototypes:"
> +	test -n "$VERBOSE" && cat ${outdir}/protos.$p
> +	cnt=$(wc -l ${outdir}/protos.$p | awk '{ print $1 }')
> +	if [[ $cnt -lt 2 ]]; then
> +		# function may be inlined in multiple sites with different protos
> +		inlined=$(grep inline ${outdir}/protos.$p)
> +		if [[ -n "$inlined" ]]; then
> +			multiple_inline=$((multiple_inline+1))
> +		else
> +			echo "WARN: '${p}()' has only one prototype; if it was subject to late optimization, pfunct may not reflect inconsistencies pahole found."
> +			echo "Full skip message from pahole: $skipmsg"
> +			warnings=$((warnings+1))
> +		fi
> +	else
> +		multiple=$((multiple+1))
> +	fi
> +done
> +
> +echo "Found $multiple instances with multiple instances with incompatible parameters."
> +echo "Found $multiple_inline instances where inline functions were not inlined and had incompatible parameters."
> +echo "Found $optimized instances where the function name suggests optimizations led to inconsistent parameters."
> +echo "Found $warnings instances where pfunct did not notice inconsistencies."
> +echo "Ok"
> +
> +exit 0
> -- 
> 2.43.5

Re: [PATCH v2 dwarves 3/3] tests: add test validating BTF encoding, reasons we skip functions

[Alan Maguire]

On 18/09/2024 09:41, Arnaldo Carvalho de Melo wrote:
> On Mon, Sep 16, 2024 at 02:49:46PM +0100, Alan Maguire wrote:
>> use pahole to encode BTF and pfunct to check that
>>
>> - DWARF functions that made it into BTF match signatures
>> - for functions we say we skipped, we did indeed skip them in BTF
>>   encoding; and
>> - it was correct to skip these functions
>>
>> It is not possible to check everything, as pfunct does not
>> print "."-suffixed optimized representations, and it does not
>> analyze location calling conventions.  As a starting point we check
>> for
>>
>> - functions encoded in BTF match signatures with DWARF
>> - functions with multiple incompatible return values
>> - functions with incompatible parameter count/type
>>
>> This test automates manual validation of encoding/skipping,
>> and as such will be useful to ensure regressions are not
>> introduced, for comparing gcc/LLVM-based DWARF representations
>> etc.
>>
>> The test takes ~5 minutes to run in all; output looks like this:
>>
>> $ vmlinux=~/kbuild/bpf-next/vmlinux bash ./btf_functions.sh
> 
> I'm trying without specifying vmlinux, what we have now in the
> tests/tests should find the one for the running kernel and use it, but
> doing so I got tons of warnings about egrep usage, so I folded this:
>

I should have better explained the rationale with the env var;
the idea is that the user can specify vmlinux=/path2/vmlinux and that
would be useful for a "make test" target - the vmlinux to be used could
be specified across multiple tests; we'd just need to add the same
envvar override to the other tests that need vmlinux.


> diff --git a/tests/btf_functions.sh b/tests/btf_functions.sh
> index 45094077809c1e3e..4a60a18845115d3d 100755
> --- a/tests/btf_functions.sh
> +++ b/tests/btf_functions.sh
> @@ -69,7 +69,7 @@ const_insensitive=0
>  for f in $funcs ; do
>  	btf=$(grep " $f(" $outdir/btf.funcs)
>  	# look for non-inline match first
> -	dwarf=$(grep " $f(" $outdir/dwarf.funcs |egrep -v "^inline ")
> +	dwarf=$(grep " $f(" $outdir/dwarf.funcs | grep -Ev "^inline ")
>  	if [[ "$btf" != "$dwarf" ]]; then
>  		# function might be declared inline in DWARF.
>  		inline_dwarf=$(grep " $f(" $outdir/dwarf.funcs |grep "^inline ")
> 

thanks; I incorporated this change (along with some other changes to
minimize the amount of greps/pipes where possible) into the v3 series.

> Running with it now:
> 
> root@x1:/home/acme/git/pahole# tests/tests 
>   1: Validation of BTF encoding of functions; this may take some time...
> 
> Seems stuck, so:
> 

This part takes a while - for me around 3.5 minutes to walk through all
functions in BTF and compare signatures to DWARF.

> root@x1:/home/acme/git/pahole# pahole --running_kernel_vmlinux
> /usr/lib/debug/lib/modules/6.10.8-200.fc40.x86_64/vmlinux
> root@x1:/home/acme/git/pahole#
> 
> Had to stop (traveling/LPC participation), so sending it now, what is
> written above should already be useful :)
> 
> - Arnaldo
> 
>> Validation of BTF encoding of functions; this may take some time...
>> Matched 55969 functions exactly.
>> Matched 239 functions with inlines.
>> Matched 1 functions with multiple const/non-const instances.
>> Ok
>> Validation of skipped function logic...
>> Validating skipped functions are absent from BTF...
>> Skipped encoding 748 functions in BTF.
>> Ok
>> Validating skipped functions have incompatible return values...
>> Found 8 functions with multiple incompatible return values.
>> Ok
>> Validating skipped functions have incompatible params/counts...
>> Found 116 instances with multiple instances with incompatible parameters.
>> Found 2 instances where inline functions were not inlined and had incompatible parameters.
>> Found 351 instances where the function name suggests optimizations led to inconsistent parameters.
>> Ok
>>
>> Suggested-by: Jiri Olsa <olsajiri@gmail.com>
>> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
>> ---
>>  tests/btf_functions.sh | 192 +++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 192 insertions(+)
>>  create mode 100755 tests/btf_functions.sh
>>
>> diff --git a/tests/btf_functions.sh b/tests/btf_functions.sh
>> new file mode 100755
>> index 0000000..4509407
>> --- /dev/null
>> +++ b/tests/btf_functions.sh
>> @@ -0,0 +1,192 @@
>> +#!/bin/bash
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +#
>> +# Copyright (c) 2024, Oracle and/or its affiliates.
>> +#
>> +# Examine functions - especially those for which we skipped BTF encoding -
>> +# to validate that they were indeed skipped for BTF encoding, and that they
>> +# also should have been.
>> +#
>> +
>> +outdir=
>> +
>> +fail()
>> +{
>> +	# Do not remove test dir; might be useful for analysis
>> +	trap - EXIT
>> +	if [[ -d "$outdir" ]]; then
>> +		echo "Test data is in $outdir"
>> +	fi
>> +	exit 1
>> +}
>> +
>> +cleanup()
>> +{
>> +	rm ${outdir}/*
>> +	rmdir $outdir
>> +}
>> +
>> +vmlinux=${vmlinux:-$1}
>> +
>> +if [ -z "$vmlinux" ] ; then
>> +	vmlinux=$(pahole --running_kernel_vmlinux)
>> +	if [ -z "$vmlinux" ] ; then
>> +		echo "Please specify a vmlinux file to operate on"
>> +		exit 1
>> +	fi
>> +fi
>> +
>> +if [ ! -f "$vmlinux" ] ; then
>> +	echo "$vmlinux file not available, please specify another"
>> +	exit 1
>> +fi
>> +
>> +outdir=$(mktemp -d /tmp/btf_functions.sh.XXXXXX)
>> +
>> +trap cleanup EXIT
>> +
>> +test -n "$VERBOSE" && printf "Encoding..."
>> +
>> +pahole --btf_features=default --btf_encode_detached=$outdir/vmlinux.btf --verbose $vmlinux |grep "skipping BTF encoding of function" > ${outdir}/skipped_fns
>> +
>> +test -n "$VERBOSE" && printf "done.\n"
>> +
>> +echo "Validation of BTF encoding of functions; this may take some time..."
>> +
>> +funcs=$(pfunct --format_path=btf $outdir/vmlinux.btf |sort)
>> +
>> +# all functions from DWARF; some inline functions are not inlined so include them too
>> +pfunct --all --no_parm_names --format_path=dwarf $vmlinux | \
>> +	awk '{ print $0}'|sort|uniq > $outdir/dwarf.funcs
>> +# all functions from BTF (removing bpf_kfunc prefix where found)
>> +pfunct --all --no_parm_names --format_path=btf $outdir/vmlinux.btf |\
>> +	awk '{ gsub("^bpf_kfunc ",""); print $0}'|sort|uniq > $outdir/btf.funcs
>> +
>> +exact=0
>> +inline=0
>> +const_insensitive=0
>> +
>> +for f in $funcs ; do
>> +	btf=$(grep " $f(" $outdir/btf.funcs)
>> +	# look for non-inline match first
>> +	dwarf=$(grep " $f(" $outdir/dwarf.funcs |egrep -v "^inline ")
>> +	if [[ "$btf" != "$dwarf" ]]; then
>> +		# function might be declared inline in DWARF.
>> +		inline_dwarf=$(grep " $f(" $outdir/dwarf.funcs |grep "^inline ")
>> +		if [[ "inline $btf" != "$inline_dwarf" ]]; then
>> +			# some functions have multiple instances in DWARF where one has
>> +			# const param(s) and another does not (see errpos()).  We do not
>> +			# mark these functions inconsistent as though they technically
>> +			# have different prototypes, the data itself is not different.
>> +			btf_noconst=$(echo $btf | awk '{gsub("const ",""); print $0 }')
>> +			dwarf_noconst=$(echo $dwarf | awk '{gsub("const ",""); print $0 }')
>> +			if [[ "$dwarf_noconst" =~ "$btf_noconst" ]]; then
>> +				const_insensitive=$((const_insensitive+1))
>> +			else
>> +				echo "ERROR: mismatch for '$f()' : BTF '$btf' not found; DWARF '$dwarf'"
>> +				fail
>> +			fi
>> +		else
>> +			inline=$((inline+1))
>> +		fi
>> +	else
>> +		exact=$((exact+1))
>> +	fi
>> +done
>> +
>> +echo "Matched $exact functions exactly."
>> +echo "Matched $inline functions with inlines."
>> +echo "Matched $const_insensitive functions with multiple const/non-const instances."
>> +echo "Ok"
>> +
>> +echo "Validation of skipped function logic..."
>> +
>> +skipped_cnt=$(wc -l ${outdir}/skipped_fns | awk '{ print $1}')
>> +
>> +if [[ "$skipped_cnt" == "0" ]]; then
>> +	echo "No skipped functions.  Done."
>> +	exit 0
>> +fi
>> +
>> +echo "Validating skipped functions are absent from BTF..."
>> +
>> +skipped_fns=$(awk '{print $1}' $outdir/skipped_fns)
>> +for s in $skipped_fns ; do
>> +	# Ensure the skipped function are not in BTF
>> +	inbtf=$(grep " $s(" $outdir/btf.funcs)
>> +	if [[ -n "$inbtf" ]]; then
>> +		echo "ERROR: '${s}()' was added incorrectly to BTF: '$inbtf'"
>> +		fail
>> +	fi
>> +done
>> +
>> +echo "Skipped encoding $skipped_cnt functions in BTF."
>> +echo "Ok"
>> +
>> +echo "Validating skipped functions have incompatible return values..."
>> +
>> +return_mismatches=$(awk '/return type mismatch/ { print $1 }' $outdir/skipped_fns)
>> +return_count=0
>> +
>> +for r in $return_mismatches ; do
>> +	# Ensure there are multiple instances with incompatible return values
>> +	grep " $r(" $outdir/dwarf.funcs | \
>> +	awk -v FN=$r '{i = index($0, FN); if (i>0) print substr($0, 0, i-1) }' \
>> +	| uniq > ${outdir}/retvals.$r
>> +	test -n "$VERBOSE" && echo "'${r}()' has return values:"
>> +	test -n "$VERBOSE" && cat ${outdir}/retvals.$r
>> +	cnt=$(wc -l ${outdir}/retvals.$r | awk '{ print $1 }')
>> +	if [[ $cnt -lt 2 ]]; then
>> +		echo "ERROR: '${r}()' has only one return value; it should not be reported as having incompatible return values"
>> +		fail
>> +	fi
>> +	return_count=$((return_count+1))
>> +done
>> +
>> +echo "Found $return_count functions with multiple incompatible return values."
>> +echo "Ok"
>> +
>> +echo "Validating skipped functions have incompatible params/counts..."
>> +
>> +param_mismatches=$(awk '/due to param / { print $1 }' $outdir/skipped_fns)
>> +
>> +multiple=0
>> +multiple_inline=0
>> +optimized=0
>> +warnings=0
>> +
>> +for p in $param_mismatches ; do
>> +	skipmsg=$(awk -v FN=$p '{ if ($1 == FN) print $0 }' $outdir/skipped_fns)
>> +	altname=$(echo $skipmsg | awk '{ i=index($2,")"); print substr($2,2,i-2); }')
>> +	if [[ "$altname" != "$p" ]]; then
>> +		test -n "$VERBOSE" && echo "skipping optimized function $p ($altname); pfunct may not reflect late optimizations."
>> +		optimized=$((optimized+1))
>> +		continue
>> +	fi
>> +	# Ensure there are multiple instances with incompatible params
>> +	grep " $p(" $outdir/dwarf.funcs | uniq > ${outdir}/protos.$p
>> +	test -n "$VERBOSE" && echo "'${p}()' has prototypes:"
>> +	test -n "$VERBOSE" && cat ${outdir}/protos.$p
>> +	cnt=$(wc -l ${outdir}/protos.$p | awk '{ print $1 }')
>> +	if [[ $cnt -lt 2 ]]; then
>> +		# function may be inlined in multiple sites with different protos
>> +		inlined=$(grep inline ${outdir}/protos.$p)
>> +		if [[ -n "$inlined" ]]; then
>> +			multiple_inline=$((multiple_inline+1))
>> +		else
>> +			echo "WARN: '${p}()' has only one prototype; if it was subject to late optimization, pfunct may not reflect inconsistencies pahole found."
>> +			echo "Full skip message from pahole: $skipmsg"
>> +			warnings=$((warnings+1))
>> +		fi
>> +	else
>> +		multiple=$((multiple+1))
>> +	fi
>> +done
>> +
>> +echo "Found $multiple instances with multiple instances with incompatible parameters."
>> +echo "Found $multiple_inline instances where inline functions were not inlined and had incompatible parameters."
>> +echo "Found $optimized instances where the function name suggests optimizations led to inconsistent parameters."
>> +echo "Found $warnings instances where pfunct did not notice inconsistencies."
>> +echo "Ok"
>> +
>> +exit 0
>> -- 
>> 2.43.5