bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next] selftests/bpf: add BPF program dump in veristat
@ 2025-08-11 21:20 Mykyta Yatsenko
  2025-08-13 21:29 ` Eduard Zingerman
  2025-08-13 21:54 ` Andrii Nakryiko
  0 siblings, 2 replies; 5+ messages in thread
From: Mykyta Yatsenko @ 2025-08-11 21:20 UTC (permalink / raw)
  To: bpf, ast, andrii, daniel, kafai, kernel-team, eddyz87; +Cc: Mykyta Yatsenko

From: Mykyta Yatsenko <yatsenko@meta.com>

This patch adds support for dumping BPF program instructions directly
from veristat.
While it is already possible to inspect BPF program dump using bpftool,
it requires multiple commands. During active development, it's common
for developers to use veristat for testing verification. Integrating
instruction dumping into veristat reduces the need to switch tools and
simplifies the workflow.
By making this information more readily accessible, this change aims
to streamline the BPF development cycle and improve usability for
developers.

Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
---
 tools/testing/selftests/bpf/Makefile   |   2 +-
 tools/testing/selftests/bpf/veristat.c | 319 +++++++++++++++++++++++++
 2 files changed, 320 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 4863106034df..c9fe5ed4b7f4 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -847,7 +847,7 @@ $(OUTPUT)/bench: $(OUTPUT)/bench.o \
 #   snprintf(a, "%s/foo", b);      // triggers -Wformat-truncation
 $(OUTPUT)/veristat.o: CFLAGS += -Wno-format-truncation
 $(OUTPUT)/veristat.o: $(BPFOBJ)
-$(OUTPUT)/veristat: $(OUTPUT)/veristat.o
+$(OUTPUT)/veristat: $(OUTPUT)/veristat.o $(OUTPUT)/disasm.o
 	$(call msg,BINARY,,$@)
 	$(Q)$(CC) $(CFLAGS) $(LDFLAGS) $(filter %.a %.o,$^) $(LDLIBS) -o $@
 
diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
index d532dd82a3a8..951b25d71909 100644
--- a/tools/testing/selftests/bpf/veristat.c
+++ b/tools/testing/selftests/bpf/veristat.c
@@ -25,6 +25,8 @@
 #include <limits.h>
 #include <assert.h>
 
+#include "disasm.h"
+
 #ifndef ARRAY_SIZE
 #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
 #endif
@@ -181,6 +183,11 @@ struct var_preset {
 	bool applied;
 };
 
+struct kernel_sym {
+	size_t address;
+	char name[256];
+};
+
 static struct env {
 	char **filenames;
 	int filename_cnt;
@@ -227,6 +234,7 @@ static struct env {
 	char orig_cgroup[PATH_MAX];
 	char stat_cgroup[PATH_MAX];
 	int memory_peak_fd;
+	bool dump;
 } env;
 
 static int libbpf_print_fn(enum libbpf_print_level level, const char *format, va_list args)
@@ -295,6 +303,7 @@ static const struct argp_option opts[] = {
 	  "Force BPF verifier failure on register invariant violation (BPF_F_TEST_REG_INVARIANTS program flag)" },
 	{ "top-src-lines", 'S', "N", 0, "Emit N most frequent source code lines" },
 	{ "set-global-vars", 'G', "GLOBAL", 0, "Set global variables provided in the expression, for example \"var1 = 1\"" },
+	{ "dump", 'p', NULL, 0, "Print BPF program dump" },
 	{},
 };
 
@@ -427,6 +436,9 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
 			return err;
 		}
 		break;
+	case 'p':
+		env.dump = true;
+		break;
 	default:
 		return ARGP_ERR_UNKNOWN;
 	}
@@ -891,6 +903,14 @@ static bool is_desc_sym(char c)
 	return c == 'v' || c == 'V' || c == '.' || c == '!' || c == '_';
 }
 
+static const char *ltrim(const char *s)
+{
+	while (isspace(*s))
+		s++;
+
+	return s;
+}
+
 static char *rtrim(char *str)
 {
 	int i;
@@ -1554,6 +1574,304 @@ static int parse_rvalue(const char *val, struct rvalue *rvalue)
 	return 0;
 }
 
+static int kernel_syms_cmp(const void *sym_a, const void *sym_b)
+{
+	return ((struct kernel_sym *)sym_a)->address -
+	       ((struct kernel_sym *)sym_b)->address;
+}
+
+struct dump_context {
+	struct bpf_prog_info *info;
+	struct kernel_sym *kernel_syms;
+	int kernel_sym_cnt;
+	size_t kfunc_base_addr;
+	char scratch_buf[512];
+};
+
+static void kernel_syms_free(struct dump_context *ctx)
+{
+	free(ctx->kernel_syms);
+	ctx->kernel_syms = NULL;
+	ctx->kernel_sym_cnt = 0;
+}
+
+static void kernel_syms_load(struct dump_context *ctx)
+{
+	struct kernel_sym *sym;
+	char buff[256];
+	void *tmp, *address;
+	FILE *fp;
+
+	fp = fopen("/proc/kallsyms", "r");
+	if (!fp)
+		return;
+	while (fgets(buff, sizeof(buff), fp)) {
+		tmp = reallocarray(ctx->kernel_syms, ctx->kernel_sym_cnt + 1,
+				   sizeof(*ctx->kernel_syms));
+		if (!tmp)
+			goto failure;
+		ctx->kernel_syms = tmp;
+		sym = ctx->kernel_syms + ctx->kernel_sym_cnt;
+
+		if (sscanf(buff, "%p %*c %s", &address, sym->name) < 2)
+			continue;
+		sym->address = (unsigned long)address;
+		if (!strcmp(sym->name, "__bpf_call_base")) {
+			ctx->kfunc_base_addr = sym->address;
+			/* sysctl kernel.kptr_restrict was set */
+			if (!sym->address)
+				goto failure;
+		}
+		if (sym->address)
+			ctx->kernel_sym_cnt++;
+	}
+
+	fclose(fp);
+	qsort(ctx->kernel_syms, ctx->kernel_sym_cnt, sizeof(*ctx->kernel_syms), kernel_syms_cmp);
+	return;
+failure:
+	kernel_syms_free(ctx);
+	fclose(fp);
+}
+
+__attribute__((format(printf, 2, 3)))
+static void print_insn(void *private_data, const char *fmt, ...)
+{
+	va_list args;
+
+	va_start(args, fmt);
+	vprintf(fmt, args);
+	va_end(args);
+}
+
+static struct kernel_sym *kernel_syms_search(unsigned long key, struct dump_context *ctx)
+{
+	struct kernel_sym sym = {
+		.address = key,
+	};
+
+	return ctx->kernel_syms ? bsearch(&sym, ctx->kernel_syms, ctx->kernel_sym_cnt,
+					  sizeof(*ctx->kernel_syms), kernel_syms_cmp) :
+				  NULL;
+}
+
+static const char *print_call(void *private_data, const struct bpf_insn *insn)
+{
+	struct kernel_sym *sym;
+	struct dump_context *ctx = (struct dump_context *)private_data;
+	size_t address = ctx->kfunc_base_addr + insn->imm;
+	struct bpf_prog_info *info = ctx->info;
+
+	if (insn->src_reg == BPF_PSEUDO_CALL) {
+		if ((__u32)insn->imm < info->nr_jited_ksyms && info->jited_ksyms) {
+			__u64 *ptr = (void *)(size_t)info->jited_ksyms;
+
+			address = ptr[insn->imm];
+		}
+
+		sym = kernel_syms_search(address, ctx);
+		if (!info->jited_ksyms)
+			snprintf(ctx->scratch_buf, sizeof(ctx->scratch_buf), "%+d", insn->off);
+		else if (sym)
+			snprintf(ctx->scratch_buf, sizeof(ctx->scratch_buf), "%+d#%s", insn->off,
+				 sym->name);
+		else
+			snprintf(ctx->scratch_buf, sizeof(ctx->scratch_buf), "%+d#0x%lx", insn->off,
+				 address);
+	} else {
+		sym = kernel_syms_search(address, ctx);
+		if (sym)
+			snprintf(ctx->scratch_buf, sizeof(ctx->scratch_buf), "%s", sym->name);
+		else
+			snprintf(ctx->scratch_buf, sizeof(ctx->scratch_buf), "0x%lx", address);
+	}
+	return ctx->scratch_buf;
+}
+
+static const char *print_imm(void *private_data, const struct bpf_insn *insn, __u64 full_imm)
+{
+	struct dump_context *ctx = (struct dump_context *)private_data;
+
+	switch (insn->src_reg) {
+	case BPF_PSEUDO_MAP_FD:
+		snprintf(ctx->scratch_buf, sizeof(ctx->scratch_buf), "map[id:%d]", insn->imm);
+		break;
+	case BPF_PSEUDO_MAP_VALUE:
+		snprintf(ctx->scratch_buf, sizeof(ctx->scratch_buf), "map[id:%d][0]+%d", insn->imm,
+			 insn[1].imm);
+		break;
+	case BPF_PSEUDO_MAP_IDX_VALUE:
+		snprintf(ctx->scratch_buf, sizeof(ctx->scratch_buf), "map[idx:%d]+%d", insn->imm,
+			 insn[1].imm);
+		break;
+	case BPF_PSEUDO_FUNC:
+		snprintf(ctx->scratch_buf, sizeof(ctx->scratch_buf), "subprog[%+d]", insn->imm);
+		break;
+	default:
+		snprintf(ctx->scratch_buf, sizeof(ctx->scratch_buf), "0x%llx",
+			 (unsigned long long)full_imm);
+	}
+	return ctx->scratch_buf;
+}
+
+static void func_printf(void *ctx, const char *fmt, va_list args)
+{
+	vprintf(fmt, args);
+}
+
+static int prep_prog_info(struct bpf_prog_info *info)
+{
+	struct bpf_prog_info holder = {};
+	size_t needed = 0;
+	void *ptr;
+
+	holder.xlated_prog_len = info->xlated_prog_len;
+	needed += info->xlated_prog_len;
+
+	holder.nr_jited_ksyms = info->nr_jited_ksyms;
+	needed += info->nr_jited_ksyms * sizeof(__u64);
+
+	holder.nr_jited_func_lens = info->nr_jited_func_lens;
+	needed += info->nr_jited_func_lens * sizeof(__u32);
+
+	holder.nr_func_info = info->nr_func_info;
+	holder.func_info_rec_size = info->func_info_rec_size;
+	needed += info->nr_func_info * info->func_info_rec_size;
+
+	holder.nr_line_info = info->nr_line_info;
+	holder.line_info_rec_size = info->line_info_rec_size;
+	needed += info->nr_line_info * info->line_info_rec_size;
+
+	holder.nr_jited_line_info = info->nr_jited_line_info;
+	holder.jited_line_info_rec_size = info->jited_line_info_rec_size;
+	needed += info->nr_jited_line_info * info->jited_line_info_rec_size;
+	ptr = malloc(needed);
+	if (!ptr)
+		return -ENOMEM;
+
+	holder.xlated_prog_insns = (unsigned long)(ptr);
+	ptr += holder.xlated_prog_len;
+
+	holder.jited_ksyms = (unsigned long)(ptr);
+	ptr += holder.nr_jited_ksyms * sizeof(__u64);
+
+	holder.jited_func_lens = (unsigned long)(ptr);
+	ptr += holder.nr_jited_func_lens * sizeof(__u32);
+
+	holder.func_info = (unsigned long)(ptr);
+	ptr += holder.nr_func_info * holder.func_info_rec_size;
+
+	holder.line_info = (unsigned long)(ptr);
+	ptr += holder.nr_line_info * holder.line_info_rec_size;
+
+	holder.jited_line_info = (unsigned long)(ptr);
+	ptr += holder.nr_jited_line_info * holder.jited_line_info_rec_size;
+
+	*info = holder;
+	return 0;
+}
+
+static void emit_line_info(const struct btf *btf, const struct bpf_line_info *linfo)
+{
+	const char *line = btf__name_by_offset(btf, linfo->line_off);
+
+	if (!line)
+		return;
+	line = ltrim(line);
+	printf("; %s\n", line);
+}
+
+static void emit_func_info(struct btf_dump *d, const struct btf *btf,
+			   const struct bpf_func_info *finfo)
+{
+	LIBBPF_OPTS(btf_dump_emit_type_decl_opts, emit_opts);
+	const struct btf_type *t;
+	__u32 name_off;
+
+	name_off = btf__type_by_id(btf, finfo->type_id)->name_off;
+	emit_opts.field_name = btf__name_by_offset(btf, name_off);
+	if (!emit_opts.field_name) /* field_name can't be NULL */
+		emit_opts.field_name = "N/A";
+	t = btf__type_by_id(btf, finfo->type_id);
+	btf_dump__emit_type_decl(d, t->type, &emit_opts);
+	printf(":\n");
+}
+
+static void dump_xlated(const struct btf *btf, struct bpf_program *prog, struct bpf_prog_info *info)
+{
+	const struct bpf_insn *insn;
+	const struct bpf_func_info *finfo;
+	const struct bpf_line_info *linfo;
+	const struct bpf_prog_linfo *prog_linfo;
+	struct btf_dump *d;
+	__u32 nr_skip = 0, i, n;
+	bool double_insn = false;
+	LIBBPF_OPTS(btf_dump_opts, dump_opts);
+	LIBBPF_OPTS(btf_dump_emit_type_decl_opts, emit_opts);
+	struct dump_context ctx = {
+		.info = info,
+		.kernel_syms = NULL,
+		.kernel_sym_cnt = 0,
+		.kfunc_base_addr = 0
+	};
+	struct bpf_insn_cbs cbs = {
+		.cb_print = print_insn,
+		.cb_call = print_call,
+		.cb_imm = print_imm,
+		.private_data = &ctx,
+	};
+
+	/* load symbols for each prog, as prog load could have added new items */
+	kernel_syms_load(&ctx);
+
+	prog_linfo = bpf_prog_linfo__new(info);
+	insn = (struct bpf_insn *)info->xlated_prog_insns;
+	finfo = (struct bpf_func_info *)info->func_info;
+	d = btf_dump__new(btf, func_printf, NULL, &dump_opts);
+	n = info->xlated_prog_len / sizeof(*insn);
+
+	for (i = 0; i < n; i += double_insn ? 2 : 1) {
+		if (d && finfo && finfo->insn_off == i) {
+			emit_func_info(d, btf, finfo);
+			finfo++;
+		}
+
+		if (prog_linfo) {
+			linfo = bpf_prog_linfo__lfind(prog_linfo, i, nr_skip);
+			if (linfo) {
+				emit_line_info(btf, linfo);
+				nr_skip++;
+			}
+		}
+		printf("%4u: ", i);
+		print_bpf_insn(&cbs, insn + i, false);
+		double_insn = insn[i].code == (BPF_LD | BPF_IMM | BPF_DW);
+	}
+
+	kernel_syms_free(&ctx);
+	btf_dump__free(d);
+}
+
+static void dump(const struct btf *btf, struct bpf_program *prog, struct bpf_prog_info *info,
+		 int prog_fd)
+{
+	int err;
+	__u32 info_len = sizeof(*info);
+
+	if (!env.dump || prog_fd <= 0)
+		return;
+
+	err = prep_prog_info(info);
+	if (err)
+		return;
+
+	if (bpf_prog_get_info_by_fd(prog_fd, info, &info_len) != 0)
+		goto cleanup;
+	dump_xlated(btf, prog, info);
+cleanup:
+	free((void *)(unsigned long)info->xlated_prog_insns);
+}
+
 static int process_prog(const char *filename, struct bpf_object *obj, struct bpf_program *prog)
 {
 	const char *base_filename = basename(strdupa(filename));
@@ -1634,6 +1952,7 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf
 		stats->stats[JITED_SIZE] = info.jited_prog_len;
 
 	parse_verif_log(buf, buf_sz, stats);
+	dump(bpf_object__btf(obj), prog, &info, fd);
 
 	if (env.verbose) {
 		printf("PROCESSING %s/%s, DURATION US: %ld, VERDICT: %s, VERIFIER LOG:\n%s\n",
-- 
2.50.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH bpf-next] selftests/bpf: add BPF program dump in veristat
  2025-08-11 21:20 [PATCH bpf-next] selftests/bpf: add BPF program dump in veristat Mykyta Yatsenko
@ 2025-08-13 21:29 ` Eduard Zingerman
  2025-08-13 21:51   ` Andrii Nakryiko
  2025-08-13 21:54 ` Andrii Nakryiko
  1 sibling, 1 reply; 5+ messages in thread
From: Eduard Zingerman @ 2025-08-13 21:29 UTC (permalink / raw)
  To: Mykyta Yatsenko, bpf, ast, andrii, daniel, kafai, kernel-team
  Cc: Mykyta Yatsenko

On Mon, 2025-08-11 at 22:20 +0100, Mykyta Yatsenko wrote:
> From: Mykyta Yatsenko <yatsenko@meta.com>
> 
> This patch adds support for dumping BPF program instructions directly
> from veristat.
> While it is already possible to inspect BPF program dump using bpftool,
> it requires multiple commands. During active development, it's common
> for developers to use veristat for testing verification. Integrating
> instruction dumping into veristat reduces the need to switch tools and
> simplifies the workflow.
> By making this information more readily accessible, this change aims
> to streamline the BPF development cycle and improve usability for
> developers.
> 
> Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
> ---

I have a feature request for this:
generate local labels for branch and call targets.
E.g. as in the tools/testing/selftests/bpf/jit_disasm_helpers.c.
Or as in llvm-objdump -d --symbolize-operands.

That aside, it looks like the code is very similar to bpftool's
xlated_dumper.c. Is there a way to share the code?
There would be now three places where xlated program is printed:
- bpftool
- veristat
- selftests (this one does not handle ksyms, but it would be nice if it could)
Should we add something like this to libbpf itself?

[...]

> +static void kernel_syms_load(struct dump_context *ctx)
> +{
> +	struct kernel_sym *sym;
> +	char buff[256];
> +	void *tmp, *address;
> +	FILE *fp;
> +
> +	fp = fopen("/proc/kallsyms", "r");
> +	if (!fp)
> +		return;
> +	while (fgets(buff, sizeof(buff), fp)) {
> +		tmp = reallocarray(ctx->kernel_syms, ctx->kernel_sym_cnt + 1,
> +				   sizeof(*ctx->kernel_syms));
> +		if (!tmp)
> +			goto failure;
> +		ctx->kernel_syms = tmp;
> +		sym = ctx->kernel_syms + ctx->kernel_sym_cnt;
> +
> +		if (sscanf(buff, "%p %*c %s", &address, sym->name) < 2)
> +			continue;
> +		sym->address = (unsigned long)address;
> +		if (!strcmp(sym->name, "__bpf_call_base")) {
> +			ctx->kfunc_base_addr = sym->address;
> +			/* sysctl kernel.kptr_restrict was set */
> +			if (!sym->address)
> +				goto failure;
> +		}
> +		if (sym->address)
> +			ctx->kernel_sym_cnt++;
> +	}
> +
> +	fclose(fp);
> +	qsort(ctx->kernel_syms, ctx->kernel_sym_cnt, sizeof(*ctx->kernel_syms), kernel_syms_cmp);
> +	return;
> +failure:

Say something in debug or verbose mode?

> +	kernel_syms_free(ctx);
> +	fclose(fp);
> +}

[...]

> +static const char *print_call(void *private_data, const struct bpf_insn *insn)
> +{
> +	struct kernel_sym *sym;
> +	struct dump_context *ctx = (struct dump_context *)private_data;
> +	size_t address = ctx->kfunc_base_addr + insn->imm;
> +	struct bpf_prog_info *info = ctx->info;
> +
> +	if (insn->src_reg == BPF_PSEUDO_CALL) {
> +		if ((__u32)insn->imm < info->nr_jited_ksyms && info->jited_ksyms) {

Nit: if info->nr_jited_ksyms is non-zero info->jited_ksyms is
     guaranteed to be not NULL?

> +			__u64 *ptr = (void *)(size_t)info->jited_ksyms;
> +
> +			address = ptr[insn->imm];
> +		}
> +
> +		sym = kernel_syms_search(address, ctx);
> +		if (!info->jited_ksyms)
> +			snprintf(ctx->scratch_buf, sizeof(ctx->scratch_buf), "%+d", insn->off);
> +		else if (sym)
> +			snprintf(ctx->scratch_buf, sizeof(ctx->scratch_buf), "%+d#%s", insn->off,
> +				 sym->name);
> +		else
> +			snprintf(ctx->scratch_buf, sizeof(ctx->scratch_buf), "%+d#0x%lx", insn->off,
> +				 address);
> +	} else {
> +		sym = kernel_syms_search(address, ctx);
> +		if (sym)
> +			snprintf(ctx->scratch_buf, sizeof(ctx->scratch_buf), "%s", sym->name);
> +		else
> +			snprintf(ctx->scratch_buf, sizeof(ctx->scratch_buf), "0x%lx", address);
> +	}
> +	return ctx->scratch_buf;
> +}

[...]

> +static void emit_line_info(const struct btf *btf, const struct bpf_line_info *linfo)
> +{
> +	const char *line = btf__name_by_offset(btf, linfo->line_off);
> +
> +	if (!line)
> +		return;
> +	line = ltrim(line);
> +	printf("; %s\n", line);

Maybe use same format as kernel here (add file name and line number)?

> +}

[...]

> +static void dump_xlated(const struct btf *btf, struct bpf_program *prog, struct bpf_prog_info *info)
> +{
> +	const struct bpf_insn *insn;
> +	const struct bpf_func_info *finfo;
> +	const struct bpf_line_info *linfo;
> +	const struct bpf_prog_linfo *prog_linfo;
> +	struct btf_dump *d;
> +	__u32 nr_skip = 0, i, n;
> +	bool double_insn = false;
> +	LIBBPF_OPTS(btf_dump_opts, dump_opts);
> +	LIBBPF_OPTS(btf_dump_emit_type_decl_opts, emit_opts);
> +	struct dump_context ctx = {
> +		.info = info,
> +		.kernel_syms = NULL,
> +		.kernel_sym_cnt = 0,
> +		.kfunc_base_addr = 0
> +	};
> +	struct bpf_insn_cbs cbs = {
> +		.cb_print = print_insn,
> +		.cb_call = print_call,
> +		.cb_imm = print_imm,
> +		.private_data = &ctx,
> +	};
> +
> +	/* load symbols for each prog, as prog load could have added new items */
> +	kernel_syms_load(&ctx);
> +
> +	prog_linfo = bpf_prog_linfo__new(info);
> +	insn = (struct bpf_insn *)info->xlated_prog_insns;
> +	finfo = (struct bpf_func_info *)info->func_info;
> +	d = btf_dump__new(btf, func_printf, NULL, &dump_opts);

Nit: &dump_opts can be just NULL.
Nit: btf_dump__new() allocates significant amount of memory for
     btf_dump->type_states. Is there a way to share dump instance for
     all programs in the object?

> +	n = info->xlated_prog_len / sizeof(*insn);
> +
> +	for (i = 0; i < n; i += double_insn ? 2 : 1) {
> +		if (d && finfo && finfo->insn_off == i) {
> +			emit_func_info(d, btf, finfo);
> +			finfo++;
> +		}
> +
> +		if (prog_linfo) {
> +			linfo = bpf_prog_linfo__lfind(prog_linfo, i, nr_skip);
> +			if (linfo) {
> +				emit_line_info(btf, linfo);
> +				nr_skip++;
> +			}
> +		}
> +		printf("%4u: ", i);
> +		print_bpf_insn(&cbs, insn + i, false);
> +		double_insn = insn[i].code == (BPF_LD | BPF_IMM | BPF_DW);
> +	}
> +

Nit: Add an empty line after the program?
     E.g. take a look at the output for ./veristat iters.bpf.o (from selftests).

> +	kernel_syms_free(&ctx);
> +	btf_dump__free(d);
> +}
> +
> +static void dump(const struct btf *btf, struct bpf_program *prog, struct bpf_prog_info *info,
> +		 int prog_fd)
> +{
> +	int err;
> +	__u32 info_len = sizeof(*info);
> +
> +	if (!env.dump || prog_fd <= 0)
> +		return;

Nit: move this to caller?

> +
> +	err = prep_prog_info(info);
> +	if (err)
> +		return;
> +
> +	if (bpf_prog_get_info_by_fd(prog_fd, info, &info_len) != 0)
> +		goto cleanup;
> +	dump_xlated(btf, prog, info);
> +cleanup:
> +	free((void *)(unsigned long)info->xlated_prog_insns);

Nit: this is a bit hackish, allocate new info as a part of the malloc
     region in the prep_prog_info()?

> +}
> +
>  static int process_prog(const char *filename, struct bpf_object *obj, struct bpf_program *prog)
>  {
>  	const char *base_filename = basename(strdupa(filename));

[...]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH bpf-next] selftests/bpf: add BPF program dump in veristat
  2025-08-13 21:29 ` Eduard Zingerman
@ 2025-08-13 21:51   ` Andrii Nakryiko
  2025-08-13 21:58     ` Eduard Zingerman
  0 siblings, 1 reply; 5+ messages in thread
From: Andrii Nakryiko @ 2025-08-13 21:51 UTC (permalink / raw)
  To: Eduard Zingerman
  Cc: Mykyta Yatsenko, bpf, ast, andrii, daniel, kafai, kernel-team,
	Mykyta Yatsenko

On Wed, Aug 13, 2025 at 2:29 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Mon, 2025-08-11 at 22:20 +0100, Mykyta Yatsenko wrote:
> > From: Mykyta Yatsenko <yatsenko@meta.com>
> >
> > This patch adds support for dumping BPF program instructions directly
> > from veristat.
> > While it is already possible to inspect BPF program dump using bpftool,
> > it requires multiple commands. During active development, it's common
> > for developers to use veristat for testing verification. Integrating
> > instruction dumping into veristat reduces the need to switch tools and
> > simplifies the workflow.
> > By making this information more readily accessible, this change aims
> > to streamline the BPF development cycle and improve usability for
> > developers.
> >
> > Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
> > ---
>
> I have a feature request for this:
> generate local labels for branch and call targets.
> E.g. as in the tools/testing/selftests/bpf/jit_disasm_helpers.c.
> Or as in llvm-objdump -d --symbolize-operands.

should we teach bpftool that?

>
> That aside, it looks like the code is very similar to bpftool's
> xlated_dumper.c. Is there a way to share the code?
> There would be now three places where xlated program is printed:
> - bpftool
> - veristat
> - selftests (this one does not handle ksyms, but it would be nice if it could)

I was going to ask the question if veristat should just delegate all
this functionality to bpftool using popen() (it's very likely you'll
have bpftool installed if you have veristat), but I guess if we can
share more code between bpftool and veristat, it's fine as well. As it
stands right now, it does seem like a lot of duplication and we'll
just have to maintain two copies of the same logic, which isn't great.

> Should we add something like this to libbpf itself?

I'd rather not, doesn't seem like it's that essential to be part of libbpf...

[...]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH bpf-next] selftests/bpf: add BPF program dump in veristat
  2025-08-11 21:20 [PATCH bpf-next] selftests/bpf: add BPF program dump in veristat Mykyta Yatsenko
  2025-08-13 21:29 ` Eduard Zingerman
@ 2025-08-13 21:54 ` Andrii Nakryiko
  1 sibling, 0 replies; 5+ messages in thread
From: Andrii Nakryiko @ 2025-08-13 21:54 UTC (permalink / raw)
  To: Mykyta Yatsenko
  Cc: bpf, ast, andrii, daniel, kafai, kernel-team, eddyz87,
	Mykyta Yatsenko

On Mon, Aug 11, 2025 at 2:20 PM Mykyta Yatsenko
<mykyta.yatsenko5@gmail.com> wrote:
>
> From: Mykyta Yatsenko <yatsenko@meta.com>
>
> This patch adds support for dumping BPF program instructions directly
> from veristat.
> While it is already possible to inspect BPF program dump using bpftool,
> it requires multiple commands. During active development, it's common
> for developers to use veristat for testing verification. Integrating
> instruction dumping into veristat reduces the need to switch tools and
> simplifies the workflow.
> By making this information more readily accessible, this change aims
> to streamline the BPF development cycle and improve usability for
> developers.
>
> Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
> ---
>  tools/testing/selftests/bpf/Makefile   |   2 +-
>  tools/testing/selftests/bpf/veristat.c | 319 +++++++++++++++++++++++++
>  2 files changed, 320 insertions(+), 1 deletion(-)
>

[...]

> +static int kernel_syms_cmp(const void *sym_a, const void *sym_b)
> +{
> +       return ((struct kernel_sym *)sym_a)->address -
> +              ((struct kernel_sym *)sym_b)->address;

as I was just skimming quickly: this is a bad idea. address is 64-bit,
int is 32-bit, you are all over the place with over(under?)flows.
Compare directly and return -1, 1, or 0, as necessary.

> +}
> +
> +struct dump_context {
> +       struct bpf_prog_info *info;
> +       struct kernel_sym *kernel_syms;
> +       int kernel_sym_cnt;
> +       size_t kfunc_base_addr;
> +       char scratch_buf[512];
> +};
> +

[...]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH bpf-next] selftests/bpf: add BPF program dump in veristat
  2025-08-13 21:51   ` Andrii Nakryiko
@ 2025-08-13 21:58     ` Eduard Zingerman
  0 siblings, 0 replies; 5+ messages in thread
From: Eduard Zingerman @ 2025-08-13 21:58 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Mykyta Yatsenko, bpf, ast, andrii, daniel, kafai, kernel-team,
	Mykyta Yatsenko

On Wed, 2025-08-13 at 14:51 -0700, Andrii Nakryiko wrote:

[...]

> > I have a feature request for this:
> > generate local labels for branch and call targets.
> > E.g. as in the tools/testing/selftests/bpf/jit_disasm_helpers.c.
> > Or as in llvm-objdump -d --symbolize-operands.
> 
> should we teach bpftool that?

Would be nice to, imo.

> > That aside, it looks like the code is very similar to bpftool's
> > xlated_dumper.c. Is there a way to share the code?
> > There would be now three places where xlated program is printed:
> > - bpftool
> > - veristat
> > - selftests (this one does not handle ksyms, but it would be nice if it could)
> 
> I was going to ask the question if veristat should just delegate all
> this functionality to bpftool using popen() (it's very likely you'll
> have bpftool installed if you have veristat), but I guess if we can
> share more code between bpftool and veristat, it's fine as well.

If code reuse proves to be convoluted, popen() sounds like a good idea to me.

[...]

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2025-08-13 21:58 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-11 21:20 [PATCH bpf-next] selftests/bpf: add BPF program dump in veristat Mykyta Yatsenko
2025-08-13 21:29 ` Eduard Zingerman
2025-08-13 21:51   ` Andrii Nakryiko
2025-08-13 21:58     ` Eduard Zingerman
2025-08-13 21:54 ` Andrii Nakryiko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).