| Message ID | 20220906133613.54928-5-quentin@isovalent.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | bpftool: Add LLVM as default library for disassembling JIT-ed programs | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf-next-VM_Test-4 | success | Logs for llvm-toolchain |
| bpf/vmtest-bpf-next-VM_Test-5 | success | Logs for set-matrix |
| bpf/vmtest-bpf-next-VM_Test-1 | success | Logs for build for s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-2 | success | Logs for build for x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-3 | success | Logs for build for x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-15 | success | Logs for test_verifier on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-16 | success | Logs for test_verifier on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-17 | success | Logs for test_verifier on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-9 | success | Logs for test_progs on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-10 | success | Logs for test_progs on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-11 | success | Logs for test_progs on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-12 | success | Logs for test_progs_no_alu32 on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-13 | success | Logs for test_progs_no_alu32 on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-14 | success | Logs for test_progs_no_alu32 on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-6 | success | Logs for test_maps on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-7 | success | Logs for test_maps on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-8 | success | Logs for test_maps on x86_64 with llvm-16 |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | warning | 2 maintainers not CCed: song@kernel.org martin.lau@linux.dev |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| 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: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 46 lines checked |
| 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 |
On Tue, Sep 6, 2022 at 6:44 AM Quentin Monnet <quentin@isovalent.com> wrote: > [...] > > +# If one of the above feature combinations is set, we support libbfd > ifneq ($(filter -lbfd,$(LIBS)),) > -CFLAGS += -DHAVE_LIBBFD_SUPPORT > -SRCS += $(BFD_SRCS) > + CFLAGS += -DHAVE_LIBBFD_SUPPORT > + > + # Libbfd interface changed over time, figure out what we need > + ifeq ($(feature-disassembler-four-args), 1) > + CFLAGS += -DDISASM_FOUR_ARGS_SIGNATURE > + endif > + ifeq ($(feature-disassembler-init-styled), 1) > + CFLAGS += -DDISASM_INIT_STYLED > + endif > +endif > +ifeq ($(filter -DHAVE_LIBBFD_SUPPORT,$(CFLAGS)),) > + # No support for JIT disassembly > + SRCS := $(filter-out jit_disasm.c,$(SRCS)) > endif This part could just be an else clause for the ifneq above. Well, I guess the difference is minimal. Acked-by: Song Liu <song@kernel.org> > > HOST_CFLAGS = $(subst -I$(LIBBPF_INCLUDE),-I$(LIBBPF_BOOTSTRAP_INCLUDE),\ > -- > 2.34.1 >
On 07/09/2022 00:31, Song Liu wrote: > On Tue, Sep 6, 2022 at 6:44 AM Quentin Monnet <quentin@isovalent.com> wrote: >> > [...] > >> >> +# If one of the above feature combinations is set, we support libbfd >> ifneq ($(filter -lbfd,$(LIBS)),) >> -CFLAGS += -DHAVE_LIBBFD_SUPPORT >> -SRCS += $(BFD_SRCS) >> + CFLAGS += -DHAVE_LIBBFD_SUPPORT >> + >> + # Libbfd interface changed over time, figure out what we need >> + ifeq ($(feature-disassembler-four-args), 1) >> + CFLAGS += -DDISASM_FOUR_ARGS_SIGNATURE >> + endif >> + ifeq ($(feature-disassembler-init-styled), 1) >> + CFLAGS += -DDISASM_INIT_STYLED >> + endif >> +endif > > >> +ifeq ($(filter -DHAVE_LIBBFD_SUPPORT,$(CFLAGS)),) >> + # No support for JIT disassembly >> + SRCS := $(filter-out jit_disasm.c,$(SRCS)) >> endif > > This part could just be an else clause for the ifneq above. > Well, I guess the difference is minimal. True for this patch, but please see patch 6 with the LLVM support: the ifneq above gets embedded in an outer if/else block (we only run it if LLVM is not found), whereas removing jit_disasm.c from the sources occurs when none of the two libs is available. Ideally we'd have "if LLVM ... else if libbfd ... else remove jit_disasm.c", but the check on libbfd involved checking multiple features so I didn't find a simple way to write that in Makefile syntax and thought it more readable to have a separate block for jit_disasm.c. > > Acked-by: Song Liu <song@kernel.org> Thanks for the review!
diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile index 8b5bfd8256c5..8060c7013d4f 100644 --- a/tools/bpf/bpftool/Makefile +++ b/tools/bpf/bpftool/Makefile @@ -120,13 +120,6 @@ include $(FEATURES_DUMP) endif endif -ifeq ($(feature-disassembler-four-args), 1) -CFLAGS += -DDISASM_FOUR_ARGS_SIGNATURE -endif -ifeq ($(feature-disassembler-init-styled), 1) - CFLAGS += -DDISASM_INIT_STYLED -endif - LIBS = $(LIBBPF) -lelf -lz LIBS_BOOTSTRAP = $(LIBBPF_BOOTSTRAP) -lelf -lz ifeq ($(feature-libcap), 1) @@ -138,9 +131,7 @@ include $(wildcard $(OUTPUT)*.d) all: $(OUTPUT)bpftool -BFD_SRCS = jit_disasm.c - -SRCS = $(filter-out $(BFD_SRCS),$(wildcard *.c)) +SRCS := $(wildcard *.c) ifeq ($(feature-libbfd),1) LIBS += -lbfd -ldl -lopcodes @@ -150,9 +141,21 @@ else ifeq ($(feature-libbfd-liberty-z),1) LIBS += -lbfd -ldl -lopcodes -liberty -lz endif +# If one of the above feature combinations is set, we support libbfd ifneq ($(filter -lbfd,$(LIBS)),) -CFLAGS += -DHAVE_LIBBFD_SUPPORT -SRCS += $(BFD_SRCS) + CFLAGS += -DHAVE_LIBBFD_SUPPORT + + # Libbfd interface changed over time, figure out what we need + ifeq ($(feature-disassembler-four-args), 1) + CFLAGS += -DDISASM_FOUR_ARGS_SIGNATURE + endif + ifeq ($(feature-disassembler-init-styled), 1) + CFLAGS += -DDISASM_INIT_STYLED + endif +endif +ifeq ($(filter -DHAVE_LIBBFD_SUPPORT,$(CFLAGS)),) + # No support for JIT disassembly + SRCS := $(filter-out jit_disasm.c,$(SRCS)) endif HOST_CFLAGS = $(subst -I$(LIBBPF_INCLUDE),-I$(LIBBPF_BOOTSTRAP_INCLUDE),\
Bpftool uses libbfd for disassembling JIT-ed programs. But the feature is optional, and the tool can be compiled without libbfd support. The Makefile sets the relevant variables accordingly. It also sets variables related to libbfd's interface, given that it has changed over time. Group all those libbfd-related definitions so that it's easier to understand what we are testing for, and only use variables related to libbfd's interface if we need libbfd in the first place. In addition to make the Makefile clearer, grouping the definitions related to disassembling JIT-ed programs will help support alternatives to libbfd. Signed-off-by: Quentin Monnet <quentin@isovalent.com> --- tools/bpf/bpftool/Makefile | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-)