Re: [PATCH v2 1/4] selftests: add shared lib.bpf.mk to build BPF progs and skeletons

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] The `bpf_link` macro drops `LDFLAGS` and `CPPFLAGS` when linking user-space test binaries.
- [Medium] Hardcoded `pkg-config` breaks target feature detection for `libzstd` when cross-compiling.
- [Medium] Missing compiler-generated dependency tracking for BPF objects leads to stale builds.
- [Medium] The documented caller contract omits `$(BPFOBJ)` as a prerequisite for test binaries, breaking incremental relinks.
--

--- Patch [1]: [PATCH v2 1/4] selftests: add shared lib.bpf.mk to build BPF progs and skeletons ---
commit 5037b1678e6c542784cb0b94b1da28ba597740a3
Author: Ziyang Men <[email protected]>

selftests: add shared lib.bpf.mk to build BPF progs and skeletons
    
This patch introduces tools/testing/selftests/lib.bpf.mk, a shared Makefile
fragment to standardize building BPF programs, skeletons, and the libbpf
toolchain. It provides a common configuration and a bpf_link macro for
test binaries to easily link against libbpf.

> diff --git a/tools/testing/selftests/lib.bpf.mk b/tools/testing/selftests/lib.bpf.mk
> new file mode 100644
> --- /dev/null
> +++ b/tools/testing/selftests/lib.bpf.mk
> @@ -0,0 +1,247 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Shared fragment for selftests that compile *.bpf.c into BPF objects +
> +# skeletons and link them into userspace test binaries, without each
> +# subsystem's Makefile re-implementing the libbpf/bpftool/vmlinux.h machinery.
> +#
> +# Caller contract (per-test Makefile):
> +#
> +#     BPF_SRCS         := foo.bpf.c bar.bpf.c
> +#     TEST_GEN_PROGS   := foo_test
> +#     OVERRIDE_TARGETS := 1              # MUST be set before lib.mk
> +#     include ../lib.mk                  # defines OUTPUT, CC, Q, msg,
> +#     include ../lib.bpf.mk              #   selfdir, top_srcdir; honours OVERRIDE
> +#
> +#     $(OUTPUT)/foo_test: foo_test.c $(BPF_SKELS)
> +#         $(call bpf_link,$@,$<)

[Severity: Medium]
Should $(BPFOBJ) be included as a prerequisite in this caller contract
template? If a user follows this example and libbpf.a is rebuilt, make
will not relink the test binaries that depend on it.

[ ... ]

> +# Locate the sources wherever the caller keeps them (e.g. progs/).
> +vpath %$(BPF_PROG_EXT) $(sort $(dir $(BPF_SRCS)))
> +
> +$(BPF_OBJS): $(BPF_OBJ_DIR)/%.bpf.o: %$(BPF_PROG_EXT) $(BPF_EXTRA_HDRS) \
> +	     $(wildcard *.bpf.h) $(INCLUDE_DIR)/vmlinux.h | $(BPF_OBJ_DIR) $(BPFOBJ)
> +	$(call BPF_BUILD_RULE,$<,$@)

[Severity: Medium]
Will relying on this wildcard approach fail to track included headers if they
are located alongside sources in subdirectories like progs/?

Should this use compiler-generated dependency tracking (-MD/-MMD) instead
so developers don't end up testing stale code?

[ ... ]

> +# Static libbpf.a first, then its deps. libbpf may pull in zstd (BTF decompress)
> +# only when built against it; link -lzstd only if libzstd is present.
> +BPF_LDLIBS := $(BPFOBJ) -lelf -lz
> +ifneq ($(shell pkg-config --exists libzstd 2>/dev/null && echo y),)
> +BPF_LDLIBS += -lzstd
> +endif

[Severity: Medium]
Can this hardcoded pkg-config break target feature detection when
cross-compiling? 

This queries the host's pkg-config environment instead of the target's
sysroot, which might cause link errors if libzstd availability differs
between the host and target. Should it use $(PKG_CONFIG) instead?

[ ... ]

> +TEST_GEN_FILES += $(BPF_OBJS)
> +
> +# Link helper: $1 = output binary, $2 = test .c (skels are the target's deps).
> +define bpf_link
> +	$(call msg,BINARY,,$1)
> +	$(Q)$(CC) $(CFLAGS) $2 $(BPF_LDLIBS) $(LDLIBS) -o $1
> +endef

[Severity: Medium]
Does this compilation command drop $(LDFLAGS) and $(CPPFLAGS)? 

These are normally included in lib.mk's default link rule. Skipping them
might ignore user-specified flags like static linking, RPATHs, or sanitizers.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1