diff mbox series

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

Message ID 20240809010518.1137758-3-eddyz87@gmail.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series __jited_x86 test tag to check x86 assembly after jit | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 42 this patch: 42
netdev/build_tools success Errors and warnings before: 10 this patch: 10
netdev/cc_maintainers warning 14 maintainers not CCed: kpsingh@kernel.org shuah@kernel.org haoluo@google.com nathan@kernel.org llvm@lists.linux.dev justinstitt@google.com john.fastabend@gmail.com jolsa@kernel.org linux-kselftest@vger.kernel.org mykolal@fb.com song@kernel.org morbo@google.com ndesaulniers@google.com sdf@fomichev.me
netdev/build_clang success Errors and warnings before: 44 this patch: 44
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 49 this patch: 49
netdev/checkpatch warning WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? WARNING: line length of 83 exceeds 80 columns WARNING: line length of 84 exceeds 80 columns WARNING: line length of 85 exceeds 80 columns WARNING: line length of 94 exceeds 80 columns WARNING: line length of 98 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-18 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18

Commit Message

Eduard Zingerman Aug. 9, 2024, 1:05 a.m. UTC 
int get_jited_program_text(int fd, char *text, size_t text_sz)

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

Comments

Yonghong Song Aug. 13, 2024, 4:05 p.m. UTC | #1 
On 8/8/24 6:05 PM, Eduard Zingerman wrote:
>      int get_jited_program_text(int fd, char *text, size_t text_sz)
>
> 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>
> ---
>   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
>
[...]
> +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];
> +
> +	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);
> +	/* GCC can't figure max bound for i and thus reports possible truncation */
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Wformat-truncation"
> +	for (i = 0; i < labels.cnt; ++i)
> +		snprintf(labels.names[i], sizeof(labels.names[i]), "L%d", i);
> +#pragma GCC diagnostic pop

"-Wformat-truncation" is only available for llvm >= 18. One of my build with llvm15
has the following warning/error:

jit_disasm_helpers.c:113:32: error: unknown warning group '-Wformat-truncation', ignored [-Werror,-Wunknown-warning-option]
#pragma GCC diagnostic ignored "-Wformat-truncation"

Maybe you want to guard with proper clang version?
Not sure on gcc side when "-Wformat-truncation" is supported.

> +
> +	/* now print with labels */
> +	labels.print_phase = true;
> +	pc = 0;
> +	while (pc < len) {
> +		cnt = disasm_insn(ctx, image, len, pc, buf, sizeof(buf));
> +		if (cnt < 0) {
> +			err = cnt;
> +			goto out;
> +		}
> +		label_pc = bsearch(&pc, labels.pcs, labels.cnt, sizeof(*labels.pcs), cmp_u32);
> +		label = "";
> +		colon = "";
> +		if (label_pc) {
> +			label = labels.names[label_pc - labels.pcs];
> +			colon = ":";
> +		}
> +		fprintf(text_out, "%x:\t", pc);
> +		for (i = 0; i < cnt; ++i)
> +			fprintf(text_out, "%02x ", image[pc + i]);
> +		for (i = cnt * 3; i < 12 * 3; ++i)
> +			fputc(' ', text_out);
> +		fprintf(text_out, "%s%s%s\n", label, colon, buf);
> +		pc += cnt;
> +	}
> +
> +out:
> +	if (triple)
> +		LLVMDisposeMessage(triple);
> +	if (ctx)
> +		LLVMDisasmDispose(ctx);
> +	return err;
> +}
> +
[...]
Eduard Zingerman Aug. 13, 2024, 10:01 p.m. UTC | #2 
On Tue, 2024-08-13 at 09:05 -0700, Yonghong Song wrote:

[...]

> > +	/* GCC can't figure max bound for i and thus reports possible truncation */
> > +#pragma GCC diagnostic push
> > +#pragma GCC diagnostic ignored "-Wformat-truncation"
> > +	for (i = 0; i < labels.cnt; ++i)
> > +		snprintf(labels.names[i], sizeof(labels.names[i]), "L%d", i);
> > +#pragma GCC diagnostic pop
> 
> "-Wformat-truncation" is only available for llvm >= 18. One of my build with llvm15
> has the following warning/error:
> 
> jit_disasm_helpers.c:113:32: error: unknown warning group '-Wformat-truncation', ignored [-Werror,-Wunknown-warning-option]
> #pragma GCC diagnostic ignored "-Wformat-truncation"
> 
> Maybe you want to guard with proper clang version?
> Not sure on gcc side when "-Wformat-truncation" is supported.

Thank you for catching this, I think I'll fix it as below in the v2.
Will wait for additional comments before submitting v2.

---

--- a/tools/testing/selftests/bpf/jit_disasm_helpers.c
+++ b/tools/testing/selftests/bpf/jit_disasm_helpers.c
@@ -108,12 +108,9 @@ static int disasm_one_func(FILE *text_out, uint8_t *image, __u32 len)
                pc += cnt;
        }
        qsort(labels.pcs, labels.cnt, sizeof(*labels.pcs), cmp_u32);
-       /* GCC can't figure max bound for i and thus reports possible truncation */
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wformat-truncation"
        for (i = 0; i < labels.cnt; ++i)
-               snprintf(labels.names[i], sizeof(labels.names[i]), "L%d", i);
-#pragma GCC diagnostic pop
+               /* use (i % 100) to avoid format truncation warning */
+               snprintf(labels.names[i], sizeof(labels.names[i]), "L%d", i % 100);
 
        /* now print with labels */
        labels.print_phase = true;
Yonghong Song Aug. 15, 2024, 7:27 p.m. UTC | #3 
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);

[...]
Eduard Zingerman Aug. 15, 2024, 7:34 p.m. UTC | #4 
On Thu, 2024-08-15 at 12:27 -0700, Yonghong Song wrote:
> 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.

Will do.

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

Thank you for the review.
I agree with the nits, will fix as suggested.
Will send v2 shortly.

[...]
Andrii Nakryiko Aug. 15, 2024, 9:06 p.m. UTC | #5 
On Thu, Aug 8, 2024 at 6:05 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
>     int get_jited_program_text(int fd, char *text, size_t text_sz)
>
> 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>
> ---
>  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)

is there any reason why you need to have this blah-LDFLAGS convention
and then applying that with extra pass, instead of just writing

$(OUTPUT)/$(TRUNNER_BINARY): LDFLAGS += $(LLVM_LDFLAGS)

I'm not sure I understand the need for extra logical hops to do this

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

[...]
Eduard Zingerman Aug. 15, 2024, 9:50 p.m. UTC | #6 
On Thu, 2024-08-15 at 14:06 -0700, Andrii Nakryiko wrote:

[...]

> > @@ -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)
> 
> is there any reason why you need to have this blah-LDFLAGS convention
> and then applying that with extra pass, instead of just writing
> 
> $(OUTPUT)/$(TRUNNER_BINARY): LDFLAGS += $(LLVM_LDFLAGS)
> 
> I'm not sure I understand the need for extra logical hops to do this

No real reason, that's how it is organized in bpftool makefile,
monkey see, monkey do. Will combine to have single LDFLAGS change.

[...]
Andrii Nakryiko Aug. 15, 2024, 10:04 p.m. UTC | #7 
On Thu, Aug 15, 2024 at 2:50 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Thu, 2024-08-15 at 14:06 -0700, Andrii Nakryiko wrote:
>
> [...]
>
> > > @@ -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)
> >
> > is there any reason why you need to have this blah-LDFLAGS convention
> > and then applying that with extra pass, instead of just writing
> >
> > $(OUTPUT)/$(TRUNNER_BINARY): LDFLAGS += $(LLVM_LDFLAGS)
> >
> > I'm not sure I understand the need for extra logical hops to do this
>
> No real reason, that's how it is organized in bpftool makefile,
> monkey see, monkey do. Will combine to have single LDFLAGS change.

I think such an approach makes sense for Linux kernel where there is a
Kbuild system of conventions and you mostly don't write real Makefile
statements (just declaratively specifying a few bits here and there).
But that's not the case for BPF selftests (and not for bpftool, but
that's a separate discussion), so extra hops just make everything
harder to follow, IMO.

>
> [...]
>
Eduard Zingerman Aug. 19, 2024, 7:45 p.m. UTC | #8 
On Thu, 2024-08-15 at 14:06 -0700, Andrii Nakryiko wrote:

[...]

> > @@ -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)
> 
> is there any reason why you need to have this blah-LDFLAGS convention
> and then applying that with extra pass, instead of just writing
> 
> $(OUTPUT)/$(TRUNNER_BINARY): LDFLAGS += $(LLVM_LDFLAGS)
> 
> I'm not sure I understand the need for extra logical hops to do this

Sorry, I missed this detail on Thursday.
The $$($(TRUNNER_BASE_NAME)-LDLIBS) thing is needed to avoid linking
LLVM dependencies for test_maps. Still, I can remove it if you think
this does not matter.

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

[...]
Andrii Nakryiko Aug. 19, 2024, 9:05 p.m. UTC | #9 
On Mon, Aug 19, 2024 at 12:45 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Thu, 2024-08-15 at 14:06 -0700, Andrii Nakryiko wrote:
>
> [...]
>
> > > @@ -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)
> >
> > is there any reason why you need to have this blah-LDFLAGS convention
> > and then applying that with extra pass, instead of just writing
> >
> > $(OUTPUT)/$(TRUNNER_BINARY): LDFLAGS += $(LLVM_LDFLAGS)
> >
> > I'm not sure I understand the need for extra logical hops to do this
>
> Sorry, I missed this detail on Thursday.
> The $$($(TRUNNER_BASE_NAME)-LDLIBS) thing is needed to avoid linking
> LLVM dependencies for test_maps. Still, I can remove it if you think
> this does not matter.
>

I don't think it does, let's keep it simple.

> > > +
> > >  # 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
>
> [...]
>
diff mbox series

Patch

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
+
+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;
+	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];
+
+	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);
+	/* GCC can't figure max bound for i and thus reports possible truncation */
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wformat-truncation"
+	for (i = 0; i < labels.cnt; ++i)
+		snprintf(labels.names[i], sizeof(labels.names[i]), "L%d", i);
+#pragma GCC diagnostic pop
+
+	/* now print with labels */
+	labels.print_phase = true;
+	pc = 0;
+	while (pc < len) {
+		cnt = disasm_insn(ctx, image, len, pc, buf, sizeof(buf));
+		if (cnt < 0) {
+			err = cnt;
+			goto out;
+		}
+		label_pc = bsearch(&pc, labels.pcs, labels.cnt, sizeof(*labels.pcs), cmp_u32);
+		label = "";
+		colon = "";
+		if (label_pc) {
+			label = labels.names[label_pc - labels.pcs];
+			colon = ":";
+		}
+		fprintf(text_out, "%x:\t", pc);
+		for (i = 0; i < cnt; ++i)
+			fprintf(text_out, "%02x ", image[pc + i]);
+		for (i = cnt * 3; i < 12 * 3; ++i)
+			fputc(' ', text_out);
+		fprintf(text_out, "%s%s%s\n", label, colon, buf);
+		pc += cnt;
+	}
+
+out:
+	if (triple)
+		LLVMDisposeMessage(triple);
+	if (ctx)
+		LLVMDisasmDispose(ctx);
+	return err;
+}
+
+int get_jited_program_text(int fd, char *text, size_t text_sz)
+{
+	struct bpf_prog_info info = {};
+	__u32 info_len = sizeof(info);
+	__u32 jited_funcs, len, pc;
+	__u32 *func_lens = NULL;
+	FILE *text_out = NULL;
+	uint8_t *image = NULL;
+	int i, err = 0;
+
+	if (!llvm_initialized) {
+		LLVMInitializeAllTargetInfos();
+		LLVMInitializeAllTargetMCs();
+		LLVMInitializeAllDisassemblers();
+		llvm_initialized = 1;
+	}
+
+	text_out = fmemopen(text, text_sz, "w");
+	if (!ASSERT_OK_PTR(text_out, "open_memstream")) {
+		err = -errno;
+		goto out;
+	}
+
+	/* first call is to find out jited program len */
+	err = bpf_prog_get_info_by_fd(fd, &info, &info_len);
+	if (!ASSERT_OK(err, "bpf_prog_get_info_by_fd #1"))
+		goto out;
+
+	len = info.jited_prog_len;
+	image = malloc(len);
+	if (!ASSERT_OK_PTR(image, "malloc(info.jited_prog_len)")) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	jited_funcs = info.nr_jited_func_lens;
+	func_lens = malloc(jited_funcs * sizeof(__u32));
+	if (!ASSERT_OK_PTR(func_lens, "malloc(info.nr_jited_func_lens)")) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	memset(&info, 0, sizeof(info));
+	info.jited_prog_insns = (__u64)image;
+	info.jited_prog_len = len;
+	info.jited_func_lens = (__u64)func_lens;
+	info.nr_jited_func_lens = jited_funcs;
+	err = bpf_prog_get_info_by_fd(fd, &info, &info_len);
+	if (!ASSERT_OK(err, "bpf_prog_get_info_by_fd #2"))
+		goto out;
+
+	for (pc = 0, i = 0; i < jited_funcs; ++i) {
+		fprintf(text_out, "func #%d:\n", i);
+		disasm_one_func(text_out, image + pc, func_lens[i]);
+		fprintf(text_out, "\n");
+		pc += func_lens[i];
+	}
+
+out:
+	if (text_out)
+		fclose(text_out);
+	if (image)
+		free(image);
+	if (func_lens)
+		free(func_lens);
+	return err;
+}
+
+#else /* HAVE_LLVM_SUPPORT */
+
+int get_jited_program_text(int fd, char *text, size_t text_sz)
+{
+	if (env.verbosity >= VERBOSE_VERY)
+		printf("compiled w/o llvm development libraries, can't dis-assembly binary code");
+	return -ENOTSUP;
+}
+
+#endif /* HAVE_LLVM_SUPPORT */
diff --git a/tools/testing/selftests/bpf/jit_disasm_helpers.h b/tools/testing/selftests/bpf/jit_disasm_helpers.h
new file mode 100644
index 000000000000..e6924fd65ecf
--- /dev/null
+++ b/tools/testing/selftests/bpf/jit_disasm_helpers.h
@@ -0,0 +1,10 @@ 
+/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
+
+#ifndef __JIT_DISASM_HELPERS_H
+#define __JIT_DISASM_HELPERS_H
+
+#include <stddef.h>
+
+int get_jited_program_text(int fd, char *text, size_t text_sz);
+
+#endif /* __JIT_DISASM_HELPERS_H */