Re: [PATCH bpf-next v6 8/9] selftests/bpf: move shared build definitions into Makefile.buildvars

> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 57907859c210..fe838bb23bcd 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile

[ ... ]

> @@ -5,109 +5,21 @@ include ../../../scripts/Makefile.include
>
>  CXX ?= $(CROSS_COMPILE)g++
>
> -TOOLSDIR := $(abspath ../../..)
> -LIBDIR := $(TOOLSDIR)/lib
> -BPFDIR := $(LIBDIR)/bpf
> -TOOLSINCDIR := $(TOOLSDIR)/include
> -TOOLSARCHINCDIR := $(TOOLSDIR)/arch/$(SRCARCH)/include
> -BPFTOOLDIR := $(TOOLSDIR)/bpf/bpftool
> -APIDIR := $(TOOLSINCDIR)/uapi
> -ifneq ($(O),)
> -GENDIR := $(O)/include/generated
> -else
> -GENDIR := $(abspath ../../../../include/generated)
> -endif
> -GENHDR := $(GENDIR)/autoconf.h
> -PKG_CONFIG ?= $(CROSS_COMPILE)pkg-config

[ ... ]

> -# Check whether clang supports BPF address sanitizer (requires LLVM 22+)
> -CLANG_HAS_ARENA_ASAN := $(shell echo 'int x;' | \
> -	$(CLANG) --target=bpf -fsanitize=kernel-address \
> -	-mllvm -asan-shadow-addr-space=1 \
> -	-x c -c - -o /dev/null 2>/dev/null && echo 1)
> -
>  # Order correspond to 'make run_tests' order
>  TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_progs \
>  	test_sockmap \

[ ... ]

>  include ../lib.mk
> +include Makefile.buildvars

[ ... ]

> diff --git a/tools/testing/selftests/bpf/Makefile.buildvars b/tools/testing/selftests/bpf/Makefile.buildvars
> new file mode 100644
> index 000000000000..75e4cfddeb62
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/Makefile.buildvars

[ ... ]

> +PCAP_CFLAGS	:= $(shell $(PKG_CONFIG) --cflags libpcap 2>/dev/null && echo "-DTRAFFIC_MONITOR=1")
> +PCAP_LIBS	:= $(shell $(PKG_CONFIG) --libs libpcap 2>/dev/null)
> +LDLIBS += $(PCAP_LIBS)
> +# Unlike the pre-split Makefile, this file is included after ../lib.mk
> +# (top level) or without it (runner), so COMMON_CFLAGS is prepended to
> +# preserve the original include-search order.
> +LIB_MK_CFLAGS := $(CFLAGS)
> +CFLAGS = $(COMMON_CFLAGS) $(PCAP_CFLAGS) $(LIB_MK_CFLAGS)

The comment says this preserves the original include-search order, but
does this handle inherited CFLAGS correctly?

In the pre-split Makefile, the assembly was `CFLAGS += $(COMMON_CFLAGS)
$(PCAP_CFLAGS)` executed before `include ../lib.mk`, producing:

  [env CFLAGS] [COMMON_CFLAGS] [PCAP_CFLAGS] [lib.mk additions]

After this patch, `LIB_MK_CFLAGS := $(CFLAGS)` captures the environment
value plus lib.mk's additions, and the new `CFLAGS =` puts that snapshot
last:

  [COMMON_CFLAGS] [PCAP_CFLAGS] [env CFLAGS] [lib.mk additions]

So an exported CFLAGS moves from first to after COMMON_CFLAGS/PCAP_CFLAGS.
Two observable consequences when CFLAGS is set in the environment (e.g. a
distro/packaging build where rpmbuild and dpkg-buildpackage both export
CFLAGS):

* `-O` level flips. COMMON_CFLAGS contains `$(OPT_FLAGS)`, which defaults
  to `-O0`. Previously an environment `-O2` was overridden by the
  selftests' `-O0`; now the environment `-O2` comes last and wins. Anything
  that depends on the selftests being built at -O0 by default (and on
  RELEASE=1 being the way to opt into -O2) changes behaviour.

* `-I` search order flips for environment-supplied include paths, which is
  exactly what the comment and the changelog claim is preserved. The claim
  is accurate only when CFLAGS is empty at the include point.

Separately, USERCFLAGS is no longer the final entry:
`-Wno-unused-command-line-argument` (LLVM builds) and
`-DHAS_BPF_ARENA_ASAN` are appended after it, and the matching
`LDFLAGS += $(SAN_LDFLAGS)` now lands after lib.mk's
`LDFLAGS += $(USERLDFLAGS)` instead of before it. lib.mk documents
USERCFLAGS/USERLDFLAGS as the user's extension point.

Could LIB_MK_CFLAGS capture only lib.mk's additions, or should the
comment/changelog be narrowed to say the order is preserved for lib.mk's
own additions rather than claiming it is identical?

> +
> +ifneq ($(LLVM),)
> +# Silence some warnings when compiled with clang
> +CFLAGS += -Wno-unused-command-line-argument
> +endif
> +
> +# Check whether clang supports BPF address sanitizer (requires LLVM 22+)
> +CLANG_HAS_ARENA_ASAN := $(shell echo 'int x;' | \
> +	$(CLANG) --target=bpf -fsanitize=kernel-address \
> +	-mllvm -asan-shadow-addr-space=1 \
> +	-x c -c - -o /dev/null 2>/dev/null && echo 1)

This probe moved from before the ../lib.mk include to after it, changing
which $(CLANG) it uses. Does this need to be documented?

Before the patch the probe sat at Makefile line 106, before
`include ../lib.mk` (Makefile:188), so $(CLANG) was
`tools/scripts/Makefile.include`'s `CLANG ?= clang`. After the patch it
lives in Makefile.buildvars, which is included at Makefile:101, after
../lib.mk. lib.mk:3-10 does:

  ifneq ($(LLVM),)
  ifneq ($(filter %/,$(LLVM)),)
  LLVM_PREFIX := $(LLVM)
  else ifneq ($(filter -%,$(LLVM)),)
  LLVM_SUFFIX := $(LLVM)
  endif
  CLANG := $(LLVM_PREFIX)clang$(LLVM_SUFFIX)

That `CLANG :=` overrides the `?=` default, so for `make LLVM=-22` or
`make LLVM=/opt/llvm/bin/` the probe now runs a different compiler than
it used to. The result is not cosmetic: CLANG_HAS_ARENA_ASAN gates
`LIBARENA_ASAN_SKEL := libarena/libarena_asan.skel.h` and
`CFLAGS += -DHAS_BPF_ARENA_ASAN` (Makefile.buildvars lines 170-173), and
LIBARENA_ASAN_SKEL is a member of TRUNNER_EXTRA_SOURCES. So on a host
whose default `clang` is older than the LLVM-selected one, the set of
built artifacts and the test_progs preprocessor state change with this
commit.

The new value is arguably the correct one (the BPF objects are compiled
with the post-lib.mk $(CLANG)), but two things argue for making it
explicit:

1. The changelog states "No rules or recipes are changed; definitions keep
   their order" and then calls out exactly two evaluation-time changes (the
   CFLAGS prepend and CLANG_SYS_INCLUDES becoming immediate). This is a
   third one, and unlike those two it changes what gets built.

2. The sibling probe `CLANG_CPUV4` (Makefile lines 19-21) deliberately
   stayed before ../lib.mk. After this patch the two clang capability
   probes in the same build disagree about which clang they are testing,
   which is a new inconsistency that will confuse the next person to touch
   either.

Either move CLANG_CPUV4 into Makefile.buildvars as well so both probes use
the resolved $(CLANG), or note the change in the changelog?

Regarding the commit message itself:

>     selftests/bpf: move shared build definitions into Makefile.buildvars
>
>     Move the definitions that are not rules into Makefile.buildvars:
>     output-tree layout, libbpf/bpftool binary locations,
>     BPF_CFLAGS/COMMON_CFLAGS assembly, libelf/libpcap probes, endianness
>     and clang feature detection, signing key paths, the permissive-mode
>     knob with its skip_on_fail and skip_if_missing helpers, the srctree
>     fallback, PKG_CONFIG and the libarena skeleton names.

[ ... ]

>     Definitions nothing outside the Makefile reads also stay: CXX in its
>     original place; BPFTOOLDIR, HOST_BPFOBJ, BPF_TARGET_ENDIAN, CLANG_CFLAGS
>     and VERIFY_SIG_HDR in a block right after the include, since they derive
>     from Makefile.buildvars values.

There are two small accuracy items in the final paragraph. First,
"BPFTOOLDIR, HOST_BPFOBJ, BPF_TARGET_ENDIAN, CLANG_CFLAGS and
VERIFY_SIG_HDR ... since they derive from Makefile.buildvars values" - four
of the five do (TOOLSDIR, HOST_BUILD_DIR, IS_LITTLE_ENDIAN,
CLANG_SYS_INCLUDES respectively), but `VERIFY_SIG_HDR := verification_cert.h`
(Makefile:109) is a bare literal with no dependency on anything. It was at
old Makefile line 765 and has been relocated ~650 lines earlier for no
reason the changelog gives; it could have stayed where it was, which would
have kept it next to the `$(VERIFY_SIG_HDR): $(VERIFICATION_CERT)` rule
that consumes it.

Second, "and the libarena skeleton names" in the list of what moved reads
as if all of them moved. LIBARENA_SKEL and LIBARENA_ASAN_SKEL moved;
LIBARENA_BENCH_SKEL (Makefile line ~657) stayed in the Makefile.

Could these be clarified?


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