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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] BPF objects will not automatically rebuild when libbpf headers are modified, breaking incremental compilation.
- [Low] Missing Clang system include definitions for LoongArch and MIPS architectures.
--

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

selftests: add shared lib.bpf.mk to build BPF progs and skeletons

This commit adds a shared tools/testing/selftests/lib.bpf.mk file to provide
a common build toolchain for BPF programs and skeletons across different
selftest directories. It aims to reduce duplication of the libbpf, bpftool,
vmlinux.h, and skeleton generation Makefiles currently present in bpf,
sched_ext, and hid selftests.

> diff --git a/tools/testing/selftests/lib.bpf.mk b/tools/testing/selftests/lib.bpf.mk
> new file mode 100644
> index 0000000000000..433a26a36960b
> --- /dev/null
> +++ b/tools/testing/selftests/lib.bpf.mk

[ ... ]

> +# ---- compiler flags -------------------------------------------------------
> +# Find the normal system headers that Clang omits with --target=bpf. Put them
> +# last with -idirafter so they cannot replace project headers.
> +define get_sys_includes
> +$(shell $(1) $(2) -v -E - </dev/null 2>&1 \
> +	| sed -n '/<...> search starts here:/,/End of search list./{ s| \(/.*\)|-idirafter \1|p }') \
> +$(shell $(1) $(2) -dM -E - </dev/null | grep '__riscv_xlen ' | awk '{printf("-D__riscv_xlen=%d -D__BITS_PER_LONG=%d", $$3, $$3)}')
> +endef

[Severity: Low]
Will BPF programs fail to compile on LoongArch and MIPS architectures?

The original selftests/bpf/Makefile extracts necessary host definitions
like __loongarch_grlen and MIPS macros.

This new macro appears to drop those extractions, which might cause Clang's
--target=bpf to fail to parse system headers on those architectures.

[ ... ]

> +# Make one rule for each source. A global vpath would also match the test's
> +# normal C rules when BPF_PROG_EXT is .c.
> +define bpf_obj_rule
> +$(BPF_OBJ_DIR)/$(patsubst %$(BPF_PROG_EXT),%,$(notdir $(1))).bpf.o: $(1)	\
> +		$(BPF_EXTRA_HDRS) $(INCLUDE_DIR)/vmlinux.h			\
> +		| $(BPF_OBJ_DIR) $(BPFOBJ)

[Severity: Medium]
Will this break incremental compilation when libbpf headers are modified?

Since $(BPFOBJ) is specified only as an order-only prerequisite and there is
no explicit Make rule for the installed headers, Make evaluates their
timestamps before $(BPFOBJ) updates them.

Does this cause BPF objects to be incorrectly skipped during the first make
invocation after modifying a libbpf header?

> +	$$(call BPF_BUILD_RULE,$$<,$$@)
> +endef

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