[PATCH v4 dwarves 0/2] fixes for memory optimized BTF

[PATCH v4 dwarves 0/2] fixes for memory optimized BTF

[Alan Maguire]

This series is identical in effect to [1], it is simply applied
on top of the "next" branch which now includes v2 of the series.

Patch 1 fixes issues identified in code review around use of
func->state, strncpy issues.

Patch 2 speeds up the btf_functions.sh test by reducing the
number of greps/pipes, shaving ~1 minute off execution time.

[1] https://lore.kernel.org/dwarves/20240925091734.1943742-1-alan.maguire@oracle.com/

Changes since v2:

- fixed checks of func->state since it is always non-NULL and removed
  now-unneeded fields from dwarves.h; added assertions that either
  ELF or DWARF data is present for func encoding (Jiri, patch 1)
- fixed strncpy() issues, improved equality comparions between types
  (Eduard, patch 1)
- reworked btf_functions test to minimize overheads (patch 2)

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)

Alan Maguire (2):
  btf_encoder: fix strncpy issues, other issues identified in code
    review
  tests: improve btf_functions.sh by reducing greps/pipes

 btf_encoder.c          | 68 ++++++++++++++++++++++++++++--------------
 tests/btf_functions.sh | 22 +++++---------
 2 files changed, 54 insertions(+), 36 deletions(-)

-- 
2.43.5

[PATCH v4 dwarves 1/2] btf_encoder: fix strncpy issues, other issues identified in code review

[Alan Maguire]

- fixed checks of func->state since it is always non-NULL and removed
  now-unneeded fields from dwarves.h; added assertions that either
  ELF or DWARF data is present for func encoding (Jiri)
- fixed strncpy() issues, improved equality comparions between types
  (Eduard)

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

diff --git a/btf_encoder.c b/btf_encoder.c
index d535b90..92dc5c4 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -28,6 +28,7 @@
 
 #include <unistd.h>
 
+#include <assert.h>
 #include <errno.h>
 #include <stdint.h>
 #include <search.h> /* for tsearch(), tfind() and tdestroy() */
@@ -692,7 +693,8 @@ 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, struct elf_function *func)
+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;
@@ -701,16 +703,22 @@ static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct f
 	int32_t id, type_id;
 	char tmp_name[KSYM_NAME_LEN];
 	const char *name;
-	struct btf_encoder_func_state *state = &func->state;
+	struct btf_encoder_func_state *state;
+
+	assert(ftype != NULL || func != NULL);
 
 	/* add btf_type for func_proto */
 	if (ftype) {
 		nr_params = ftype->nr_parms + (ftype->unspec_parms ? 1 : 0);
 		type_id = btf_encoder__tag_type(encoder, ftype->tag.type);
-	} else {
+	} else if (func) {
+		state = &func->state;
 		nr_params = state->nr_parms;
 		type_id = state->ret_type_id;
+	} else {
+		return 0;
 	}
+
 	id = btf__add_func_proto(btf, type_id);
 	if (id > 0) {
 		t = btf__type_by_id(btf, id);
@@ -730,25 +738,29 @@ static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct f
 
 			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))
+			if (btf_encoder__add_func_param(encoder, name, type_id,
+							param_idx == nr_params))
 				return -1;
 		}
 
 		++param_idx;
 		if (ftype->unspec_parms)
-			if (btf_encoder__add_func_param(encoder, NULL, 0, param_idx == nr_params))
+			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);
 
 			/* 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));
+			strncpy(tmp_name, name, sizeof(tmp_name) - 1);
 
-			if (btf_encoder__add_func_param(encoder, tmp_name, p->type_id, param_idx == nr_params))
+			if (btf_encoder__add_func_param(encoder, tmp_name, p->type_id,
+							param_idx == nr_params))
 				return -1;
 		}
 	}
@@ -950,10 +962,17 @@ static bool types__match(struct btf_encoder *encoder,
 			return false;
 		}
 
-		switch (btf_kind(t1)) {
+		switch (k1) {
 		case BTF_KIND_INT:
+			if (t1->size != t2->size)
+				return false;
+			if (*(__u32 *)(t1 + 1) != *(__u32 *)(t2 + 1))
+				return false;
+			return names__match(btf1, t1, btf2, t2);
 		case BTF_KIND_FLOAT:
-		case BTF_KIND_FWD:
+			if (t1->size != t2->size)
+				return false;
+			return names__match(btf1, t1, btf2, t2);
 		case BTF_KIND_TYPEDEF:
 		case BTF_KIND_STRUCT:
 		case BTF_KIND_UNION:
@@ -1009,9 +1028,6 @@ static bool funcs__match(struct btf_encoder *encoder, struct elf_function *func,
 {
 	uint8_t i;
 
-	if (!s1->initialized || !s2->initialized)
-		return true;
-
 	if (s1->nr_parms != s2->nr_parms) {
 		btf_encoder__log_func_skip(encoder, func,
 					   "param count mismatch; %d params != %d params\n",
@@ -1081,7 +1097,8 @@ static int32_t btf_encoder__save_func(struct btf_encoder *encoder, struct functi
 			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;
+		state.parms[param_idx].type_id = param->tag.type == 0 ? 0 :
+						encoder->type_id_off + param->tag.type;
 		param_idx++;
 	}
 	if (ftype->unspec_parms)
@@ -1139,7 +1156,8 @@ out:
 	return err;
 }
 
-static int32_t btf_encoder__add_func(struct btf_encoder *encoder, struct function *fn, struct elf_function *func)
+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 = 0;
 	int16_t component_idx = -1;
@@ -1147,19 +1165,23 @@ static int32_t btf_encoder__add_func(struct btf_encoder *encoder, struct functio
 	const char *value;
 	char tmp_value[KSYM_NAME_LEN];
 
+	assert(fn != NULL || func != NULL);
+
 	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);
+		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': invalid %s\n", name, btf_fnproto_id < 0 ? "proto" : "func");
+		printf("error: failed to encode function '%s': invalid %s\n",
+		       name, btf_fnproto_id < 0 ? "proto" : "func");
 		return -1;
 	}
 	if (!fn) {
 		struct btf_encoder_func_state *state = &func->state;
 		uint16_t idx;
 
-		if (!state || state->nr_annots == 0)
+		if (state->nr_annots == 0)
 			return 0;
 
 		for (idx = 0; idx < state->nr_annots; idx++) {
@@ -1169,10 +1191,11 @@ static int32_t btf_encoder__add_func(struct btf_encoder *encoder, struct functio
 			/* 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));
+			strncpy(tmp_value, value, sizeof(tmp_value) - 1);
 			component_idx = a->component_idx;
 
-			tag_type_id = btf_encoder__add_decl_tag(encoder, tmp_value, btf_fn_id, component_idx);
+			tag_type_id = btf_encoder__add_decl_tag(encoder, tmp_value,
+								btf_fn_id, component_idx);
 			if (tag_type_id < 0)
 				break;
 		}
@@ -1190,7 +1213,8 @@ static int32_t btf_encoder__add_func(struct btf_encoder *encoder, struct functio
 		}
 	}
 	if (tag_type_id < 0) {
-		fprintf(stderr, "error: failed to encode tag '%s' to func %s with component_idx %d\n",
+		fprintf(stderr,
+			"error: failed to encode tag '%s' to func %s with component_idx %d\n",
 			value, name, component_idx);
 		return -1;
 	}
@@ -1207,7 +1231,7 @@ static int btf_encoder__add_saved_funcs(struct btf_encoder *encoder)
 		struct btf_encoder_func_state *state = &func->state;
 		struct btf_encoder *other_encoder = NULL;
 
-		if (!state || !state->initialized || state->processed)
+		if (!state->initialized || state->processed)
 			continue;
 		/* merge optimized-out status across encoders; since each
 		 * encoder has the same elf symbol table we can use the
@@ -1223,7 +1247,7 @@ static int btf_encoder__add_saved_funcs(struct btf_encoder *encoder)
 
 			other_func = &other_encoder->functions.entries[i];
 			other_state = &other_func->state;
-			if (!other_state)
+			if (!other_state->initialized)
 				continue;
 			optimized = state->optimized_parms | other_state->optimized_parms;
 			unexpected = state->unexpected_reg | other_state->unexpected_reg;
-- 
2.43.5

[PATCH v4 dwarves 2/2] tests: improve btf_functions.sh by reducing greps/pipes

[Alan Maguire]

Reduce number of greps/pipes to minimize overhead of test; in my
measurements this shaves ~1min off test run time to be approximately
4 minutes.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 tests/btf_functions.sh | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/tests/btf_functions.sh b/tests/btf_functions.sh
index 4a60a18..b8645cc 100755
--- a/tests/btf_functions.sh
+++ b/tests/btf_functions.sh
@@ -47,7 +47,8 @@ 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
+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"
 
@@ -57,7 +58,7 @@ 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
+	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
@@ -66,14 +67,12 @@ exact=0
 inline=0
 const_insensitive=0
 
-for f in $funcs ; do
-	btf=$(grep " $f(" $outdir/btf.funcs)
+while IFS= read -r btf ; do
 	# look for non-inline match first
-	dwarf=$(grep " $f(" $outdir/dwarf.funcs | grep -Ev "^inline ")
+	dwarf=$(grep -F "$btf" $outdir/dwarf.funcs)
 	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
+		if [[ "inline $btf" != "$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
@@ -83,7 +82,7 @@ for f in $funcs ; do
 			if [[ "$dwarf_noconst" =~ "$btf_noconst" ]]; then
 				const_insensitive=$((const_insensitive+1))
 			else
-				echo "ERROR: mismatch for '$f()' : BTF '$btf' not found; DWARF '$dwarf'"
+				echo "ERROR: mismatch : BTF '$btf' not found; DWARF '$dwarf'"
 				fail
 			fi
 		else
@@ -92,7 +91,7 @@ for f in $funcs ; do
 	else
 		exact=$((exact+1))
 	fi
-done
+done < $outdir/btf.funcs
 
 echo "Matched $exact functions exactly."
 echo "Matched $inline functions with inlines."
@@ -133,8 +132,6 @@ for r in $return_mismatches ; do
 	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"
@@ -159,14 +156,11 @@ 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
-- 
2.43.5

Re: [PATCH v4 dwarves 0/2] fixes for memory optimized BTF

[Jiri Olsa]

On Mon, Sep 30, 2024 at 09:17:23AM +0100, Alan Maguire wrote:
> This series is identical in effect to [1], it is simply applied
> on top of the "next" branch which now includes v2 of the series.
> 
> Patch 1 fixes issues identified in code review around use of
> func->state, strncpy issues.
> 
> Patch 2 speeds up the btf_functions.sh test by reducing the
> number of greps/pipes, shaving ~1 minute off execution time.
> 
> [1] https://lore.kernel.org/dwarves/20240925091734.1943742-1-alan.maguire@oracle.com/
> 
> Changes since v2:
> 
> - fixed checks of func->state since it is always non-NULL and removed
>   now-unneeded fields from dwarves.h; added assertions that either
>   ELF or DWARF data is present for func encoding (Jiri, patch 1)

lgtm

Acked-by: Jiri Olsa <jolsa@kernel.org>

jirka

> - fixed strncpy() issues, improved equality comparions between types
>   (Eduard, patch 1)
> - reworked btf_functions test to minimize overheads (patch 2)
> 
> 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)
> 
> Alan Maguire (2):
>   btf_encoder: fix strncpy issues, other issues identified in code
>     review
>   tests: improve btf_functions.sh by reducing greps/pipes
> 
>  btf_encoder.c          | 68 ++++++++++++++++++++++++++++--------------
>  tests/btf_functions.sh | 22 +++++---------
>  2 files changed, 54 insertions(+), 36 deletions(-)
> 
> -- 
> 2.43.5
> 

Re: [PATCH v4 dwarves 0/2] fixes for memory optimized BTF

[Arnaldo Carvalho de Melo]

On Mon, Sep 30, 2024 at 01:20:11PM +0200, Jiri Olsa wrote:
> On Mon, Sep 30, 2024 at 09:17:23AM +0100, Alan Maguire wrote:
> > This series is identical in effect to [1], it is simply applied
> > on top of the "next" branch which now includes v2 of the series.
> > 
> > Patch 1 fixes issues identified in code review around use of
> > func->state, strncpy issues.
> > 
> > Patch 2 speeds up the btf_functions.sh test by reducing the
> > number of greps/pipes, shaving ~1 minute off execution time.
> > 
> > [1] https://lore.kernel.org/dwarves/20240925091734.1943742-1-alan.maguire@oracle.com/
> > 
> > Changes since v2:
> > 
> > - fixed checks of func->state since it is always non-NULL and removed
> >   now-unneeded fields from dwarves.h; added assertions that either
> >   ELF or DWARF data is present for func encoding (Jiri, patch 1)
> 
> lgtm
> 
> Acked-by: Jiri Olsa <jolsa@kernel.org>

Thanks, applied to next,

- Arnaldo