Re: [PATCH bpf-next 2/4] selftests/bpf: utility function to get program disassembly after jit

[Yonghong Song <yonghong.song@linux.dev>]

On 8/8/24 6:05 PM, Eduard Zingerman wrote:
>      int get_jited_program_text(int fd, char *text, size_t text_sz)
>
You need to give some context before the above function signature.

> Loads and disassembles jited instructions for program pointed to by fd.
> Much like 'bpftool prog dump jited ...'.
>
> The code and makefile changes are inspired by jit_disasm.c from bpftool.
> Use llvm libraries to disassemble BPF program instead of libbfd to avoid
> issues with disassembly output stability pointed out in [1].
>
> Selftests makefile uses Makefile.feature to detect if LLVM libraries
> are available. If that is not the case selftests build proceeds but
> the function returns -ENOTSUP at runtime.
>
> [1] commit eb9d1acf634b ("bpftool: Add LLVM as default library for disassembling JIT-ed programs")
>
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>

LGTM except a few nits below.

Acked-by: Yonghong Song <yonghong.song@linux.dev>

> ---
>   tools/testing/selftests/bpf/.gitignore        |   1 +
>   tools/testing/selftests/bpf/Makefile          |  51 +++-
>   .../selftests/bpf/jit_disasm_helpers.c        | 228 ++++++++++++++++++
>   .../selftests/bpf/jit_disasm_helpers.h        |  10 +
>   4 files changed, 288 insertions(+), 2 deletions(-)
>   create mode 100644 tools/testing/selftests/bpf/jit_disasm_helpers.c
>   create mode 100644 tools/testing/selftests/bpf/jit_disasm_helpers.h
>
> diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
> index 8f14d8faeb0b..7de4108771a0 100644
> --- a/tools/testing/selftests/bpf/.gitignore
> +++ b/tools/testing/selftests/bpf/.gitignore
> @@ -8,6 +8,7 @@ test_lru_map
>   test_lpm_map
>   test_tag
>   FEATURE-DUMP.libbpf
> +FEATURE-DUMP.selftests
>   fixdep
>   /test_progs
>   /test_progs-no_alu32
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index f54185e96a95..b1a52739d9e7 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -33,6 +33,13 @@ OPT_FLAGS	?= $(if $(RELEASE),-O2,-O0)
>   LIBELF_CFLAGS	:= $(shell $(PKG_CONFIG) libelf --cflags 2>/dev/null)
>   LIBELF_LIBS	:= $(shell $(PKG_CONFIG) libelf --libs 2>/dev/null || echo -lelf)
>   
> +ifeq ($(srctree),)
> +srctree := $(patsubst %/,%,$(dir $(CURDIR)))
> +srctree := $(patsubst %/,%,$(dir $(srctree)))
> +srctree := $(patsubst %/,%,$(dir $(srctree)))
> +srctree := $(patsubst %/,%,$(dir $(srctree)))
> +endif
> +
>   CFLAGS += -g $(OPT_FLAGS) -rdynamic					\
>   	  -Wall -Werror -fno-omit-frame-pointer				\
>   	  $(GENFLAGS) $(SAN_CFLAGS) $(LIBELF_CFLAGS)			\
> @@ -55,6 +62,11 @@ progs/test_sk_lookup.c-CFLAGS := -fno-strict-aliasing
>   progs/timer_crash.c-CFLAGS := -fno-strict-aliasing
>   progs/test_global_func9.c-CFLAGS := -fno-strict-aliasing
>   
> +# Some utility functions use LLVM libraries
> +jit_disasm_helpers.c-CFLAGS = $(LLVM_CFLAGS)
> +test_progs-LDLIBS = $(LLVM_LDLIBS)
> +test_progs-LDFLAGS = $(LLVM_LDFLAGS)
> +
>   ifneq ($(LLVM),)
>   # Silence some warnings when compiled with clang
>   CFLAGS += -Wno-unused-command-line-argument
> @@ -164,6 +176,31 @@ endef
>   
>   include ../lib.mk
>   
> +NON_CHECK_FEAT_TARGETS := clean docs-clean
> +CHECK_FEAT := $(filter-out $(NON_CHECK_FEAT_TARGETS),$(or $(MAKECMDGOALS), "none"))
> +ifneq ($(CHECK_FEAT),)
> +FEATURE_USER := .selftests
> +FEATURE_TESTS := llvm
> +FEATURE_DISPLAY := $(FEATURE_TESTS)
> +
> +# Makefile.feature expects OUTPUT to end with a slash
> +$(let OUTPUT,$(OUTPUT)/,\
> +	$(eval include ../../../build/Makefile.feature))
> +endif
> +
> +ifeq ($(feature-llvm),1)
> +  LLVM_CFLAGS  += -DHAVE_LLVM_SUPPORT
> +  LLVM_CONFIG_LIB_COMPONENTS := mcdisassembler all-targets
> +  # both llvm-config and lib.mk add -D_GNU_SOURCE, which ends up as conflict
> +  LLVM_CFLAGS  += $(filter-out -D_GNU_SOURCE,$(shell $(LLVM_CONFIG) --cflags))
> +  LLVM_LDLIBS  += $(shell $(LLVM_CONFIG) --libs $(LLVM_CONFIG_LIB_COMPONENTS))
> +  ifeq ($(shell $(LLVM_CONFIG) --shared-mode),static)
> +    LLVM_LDLIBS += $(shell $(LLVM_CONFIG) --system-libs $(LLVM_CONFIG_LIB_COMPONENTS))
> +    LLVM_LDLIBS += -lstdc++
> +  endif
> +  LLVM_LDFLAGS += $(shell $(LLVM_CONFIG) --ldflags)
> +endif
> +
>   SCRATCH_DIR := $(OUTPUT)/tools
>   BUILD_DIR := $(SCRATCH_DIR)/build
>   INCLUDE_DIR := $(SCRATCH_DIR)/include
> @@ -488,6 +525,7 @@ define DEFINE_TEST_RUNNER
>   
>   TRUNNER_OUTPUT := $(OUTPUT)$(if $2,/)$2
>   TRUNNER_BINARY := $1$(if $2,-)$2
> +TRUNNER_BASE_NAME := $1
>   TRUNNER_TEST_OBJS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.test.o,	\
>   				 $$(notdir $$(wildcard $(TRUNNER_TESTS_DIR)/*.c)))
>   TRUNNER_EXTRA_OBJS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.o,		\
> @@ -611,6 +649,10 @@ ifeq ($(filter clean docs-clean,$(MAKECMDGOALS)),)
>   include $(wildcard $(TRUNNER_TEST_OBJS:.o=.d))
>   endif
>   
> +# add per extra obj CFGLAGS definitions
> +$(foreach N,$(patsubst $(TRUNNER_OUTPUT)/%.o,%,$(TRUNNER_EXTRA_OBJS)),	\
> +	$(eval $(TRUNNER_OUTPUT)/$(N).o: CFLAGS += $($(N).c-CFLAGS)))
> +
>   $(TRUNNER_EXTRA_OBJS): $(TRUNNER_OUTPUT)/%.o:				\
>   		       %.c						\
>   		       $(TRUNNER_EXTRA_HDRS)				\
> @@ -627,6 +669,9 @@ ifneq ($2:$(OUTPUT),:$(shell pwd))
>   	$(Q)rsync -aq $$^ $(TRUNNER_OUTPUT)/
>   endif
>   
> +$(OUTPUT)/$(TRUNNER_BINARY): LDLIBS += $$($(TRUNNER_BASE_NAME)-LDLIBS)
> +$(OUTPUT)/$(TRUNNER_BINARY): LDFLAGS += $$($(TRUNNER_BASE_NAME)-LDFLAGS)
> +
>   # some X.test.o files have runtime dependencies on Y.bpf.o files
>   $(OUTPUT)/$(TRUNNER_BINARY): | $(TRUNNER_BPF_OBJS)
>   
> @@ -636,7 +681,7 @@ $(OUTPUT)/$(TRUNNER_BINARY): $(TRUNNER_TEST_OBJS)			\
>   			     $(TRUNNER_BPFTOOL)				\
>   			     | $(TRUNNER_BINARY)-extras
>   	$$(call msg,BINARY,,$$@)
> -	$(Q)$$(CC) $$(CFLAGS) $$(filter %.a %.o,$$^) $$(LDLIBS) -o $$@
> +	$(Q)$$(CC) $$(CFLAGS) $$(filter %.a %.o,$$^) $$(LDLIBS) $$(LDFLAGS) -o $$@
>   	$(Q)$(RESOLVE_BTFIDS) --btf $(TRUNNER_OUTPUT)/btf_data.bpf.o $$@
>   	$(Q)ln -sf $(if $2,..,.)/tools/build/bpftool/$(USE_BOOTSTRAP)bpftool \
>   		   $(OUTPUT)/$(if $2,$2/)bpftool
> @@ -655,6 +700,7 @@ TRUNNER_EXTRA_SOURCES := test_progs.c		\
>   			 cap_helpers.c		\
>   			 unpriv_helpers.c 	\
>   			 netlink_helpers.c	\
> +			 jit_disasm_helpers.c	\
>   			 test_loader.c		\
>   			 xsk.c			\
>   			 disasm.c		\
> @@ -797,7 +843,8 @@ EXTRA_CLEAN := $(SCRATCH_DIR) $(HOST_SCRATCH_DIR)			\
>   	$(addprefix $(OUTPUT)/,*.o *.d *.skel.h *.lskel.h *.subskel.h	\
>   			       no_alu32 cpuv4 bpf_gcc bpf_testmod.ko	\
>   			       bpf_test_no_cfi.ko			\
> -			       liburandom_read.so)
> +			       liburandom_read.so)			\
> +	$(OUTPUT)/FEATURE-DUMP.selftests
>   
>   .PHONY: docs docs-clean
>   
> diff --git a/tools/testing/selftests/bpf/jit_disasm_helpers.c b/tools/testing/selftests/bpf/jit_disasm_helpers.c
> new file mode 100644
> index 000000000000..ae704f7c5ee7
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/jit_disasm_helpers.c
> @@ -0,0 +1,228 @@
> +// SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause)
> +#include <bpf/bpf.h>
> +#include <bpf/libbpf.h>
> +#include <test_progs.h>
> +
> +#ifdef HAVE_LLVM_SUPPORT
> +
> +#include <llvm-c/Core.h>
> +#include <llvm-c/Disassembler.h>
> +#include <llvm-c/Target.h>
> +#include <llvm-c/TargetMachine.h>
> +
> +#define MAX_LOCAL_LABELS 32

Some bpf progs, e.g., pyperf600.bpf.o has thousands of lables.
But since this file is for selftest and we expect the test
case will be simpler, so the above number '32' should be okay.
But it would be good to add a comment for this.

> +
> +static bool llvm_initialized;
> +
> +struct local_labels {
> +	bool print_phase;
> +	__u32 prog_len;
> +	__u32 cnt;
> +	__u32 pcs[MAX_LOCAL_LABELS];
> +	char names[MAX_LOCAL_LABELS][4];
> +};
> +
> +/* Depending on labels->print_phase:
> + * - if false: record save jump labels within the program in labels->pcs;
> + * - if true: if ref_value is in labels->pcs, return corresponding labels->name
> + */
> +static const char *lookup_symbol(void *data, uint64_t ref_value, uint64_t *ref_type,
> +				 uint64_t ref_pc, const char **ref_name)
> +{
> +	struct local_labels *labels = data;
> +	uint64_t type = *ref_type;
> +	int i;
> +
> +	*ref_type = LLVMDisassembler_ReferenceType_InOut_None;
> +	*ref_name = NULL;
> +	if (type != LLVMDisassembler_ReferenceType_In_Branch)
> +		return NULL;
> +	for (i = 0; i < labels->cnt; ++i)
> +		if (labels->pcs[i] == ref_value)
> +			return labels->names[i];
> +	if (!labels->print_phase && labels->cnt < MAX_LOCAL_LABELS &&
> +	    ref_value < labels->prog_len)
> +		labels->pcs[labels->cnt++] = ref_value;

Maybe the following easy to understand?

	if (labels->print_phase) {
		for (i = 0; i < labels->cnt; ++i)
			if (labels->pcs[i] == ref_value)
				return labels->names[i];

	} else {
		if (labels->cnt < MAX_LOCAL_LABELS && ref_value < labels->prog_len)
			labels->pcs[labels->cnt++] = ref_value;
	}

> +	return NULL;
> +}
> +
> +static int disasm_insn(LLVMDisasmContextRef ctx, uint8_t *image, __u32 len, __u32 pc,
> +		       char *buf, __u32 buf_sz)
> +{
> +	int i, cnt;
> +
> +	cnt = LLVMDisasmInstruction(ctx, image + pc, len - pc, pc,
> +				    buf, buf_sz);
> +	if (cnt > 0)
> +		return cnt;
> +	PRINT_FAIL("Can't disasm instruction at offset %d:", pc);
> +	for (i = 0; i < 16 && pc + i < len; ++i)
> +		printf(" %02x", image[pc + i]);
> +	printf("\n");
> +	return -EINVAL;
> +}
> +
> +static int cmp_u32(const void *_a, const void *_b)
> +{
> +	__u32 a = *(__u32 *)_a;
> +	__u32 b = *(__u32 *)_b;
> +
> +	if (a < b)
> +		return -1;
> +	if (a > b)
> +		return 1;
> +	return 0;
> +}
> +
> +static int disasm_one_func(FILE *text_out, uint8_t *image, __u32 len)
> +{
> +	char *label, *colon, *triple = NULL;
> +	LLVMDisasmContextRef ctx = NULL;
> +	struct local_labels labels = {};
> +	__u32 *label_pc, pc;
> +	int i, cnt, err = 0;
> +	char buf[256];

buf[16]?

> +
> +	triple = LLVMGetDefaultTargetTriple();
> +	ctx = LLVMCreateDisasm(triple, &labels, 0, NULL, lookup_symbol);
> +	if (!ASSERT_OK_PTR(ctx, "LLVMCreateDisasm")) {
> +		err = -EINVAL;
> +		goto out;
> +	}
> +
> +	cnt = LLVMSetDisasmOptions(ctx, LLVMDisassembler_Option_PrintImmHex);
> +	if (!ASSERT_EQ(cnt, 1, "LLVMSetDisasmOptions")) {
> +		err = -EINVAL;
> +		goto out;
> +	}
> +
> +	/* discover labels */
> +	labels.prog_len = len;
> +	pc = 0;
> +	while (pc < len) {
> +		cnt = disasm_insn(ctx, image, len, pc, buf, 1);
> +		if (cnt < 0) {
> +			err = cnt;
> +			goto out;
> +		}
> +		pc += cnt;
> +	}
> +	qsort(labels.pcs, labels.cnt, sizeof(*labels.pcs), cmp_u32);

[...]