Re: [PATCH bpf-next v3 2/2] bpftool: Make the LLVM disassembler an optional runtime dependency

[bot+bpf-ci]

> bpftool: Make the LLVM disassembler an optional runtime dependency

TL;DR: Nits only: the new generated llvm-plugin-dir stamp file may need a
.gitignore entry, and the bpftool man page may need updating, since "llvm" in
version output now depends on the plugin loading at runtime.

> diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
> index b0f7168e7943..dc8e4be86296 100644
> --- a/tools/bpf/bpftool/Makefile
> +++ b/tools/bpf/bpftool/Makefile

[ ... ]

> @@ -290,6 +300,21 @@ $(BPFTOOL_BOOTSTRAP): $(BOOTSTRAP_OBJS) $(LIBBPF_BOOTSTRAP)
>  $(OUTPUT)bpftool: $(OBJS) $(LIBBPF)
>  	$(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) $(OBJS) $(LIBS) -o $@
>
> +ifneq ($(LLVM_PLUGIN),)
> +all: $(LLVM_PLUGIN)
> +
> +$(LLVM_PLUGIN): CFLAGS += -fPIC
> +$(LLVM_PLUGIN): $(OUTPUT)llvm_disasm.o
> +	$(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) -shared $< $(LLVM_LIBS) -o $@
> +
> +# The plugin directory, $(libdir)/bpftool, is compiled into jit_disasm.o.
> +# Recompile it if libdir changes, e.g. on "make" then "make install libdir=...".
> +$(OUTPUT)llvm-plugin-dir: FORCE
> +	$(Q)echo '$(libdir)/bpftool' | cmp -s - $@ || echo '$(libdir)/bpftool' > $@
                                                          ^^^^

The new stamp file $(OUTPUT)llvm-plugin-dir is a generated build artifact,
but it does not look like any ignore pattern matches it.

bpftool-llvm.so is covered by '*.so' and llvm_disasm.o by '*.o', but
'git check-ignore' finds no pattern for tools/bpf/bpftool/llvm-plugin-dir.

On an in-tree build with a shared libLLVM (make -C tools/bpf/bpftool with
no OUTPUT), the stamp is written to tools/bpf/bpftool/llvm-plugin-dir and
shows up as an untracked file in 'git status'.

tools/bpf/bpftool/.gitignore already lists the similar generated file
FEATURE-DUMP.bpftool.  Should this patch add '/llvm-plugin-dir' there as
well?

> +
> +$(OUTPUT)jit_disasm.o: $(OUTPUT)llvm-plugin-dir
> +endif
> +

[ ... ]

> diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
> index d15fec71b64e..321e928d651c 100644
> --- a/tools/bpf/bpftool/main.c
> +++ b/tools/bpf/bpftool/main.c
> @@ -124,7 +124,8 @@ static int do_version(int argc, char **argv)
>  	const bool has_libbfd = false;
>  #endif
>  #ifdef HAVE_LLVM_SUPPORT
> -	const bool has_llvm = true;
> +	/* The LLVM disassembler may be a plugin that is not installed */
> +	const bool has_llvm = disasm_llvm_available();
                              ^^^^

With a shared libLLVM, the "llvm" entry printed by "bpftool version" and
"-V" now depends on whether bpftool-llvm.so can be dlopen()ed at runtime
rather than on how bpftool was built.  The man page text was not updated
to match.

tools/bpf/bpftool/Documentation/common_options.rst, which every bpftool man
page includes, still says:

    Print bpftool's version number (similar to **bpftool version**), the
    number of the libbpf version in use, and optional features that were
    included when bpftool was compiled. Optional features include linking
    against LLVM or libbfd to provide the disassembler for JIT-ted programs
    (**bpftool prog dump jited**) ...

In this mode bpftool is no longer linked against LLVM at all, and
do_version() -> disasm_llvm_available() -> llvm_load(false) can fail in
llvm_plugin_find() or dlopen().  has_llvm then comes back false even though
bpftool was compiled with HAVE_LLVM_SUPPORT.

Should common_options.rst say that "llvm" is reported only when the
disassembler plugin is installed and loadable?

>  #else
>  	const bool has_llvm = false;
>  #endif


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/40881490673