Re: [RFC PATCH bpf-next v4 2/3] selftests/bpf: move shared build definitions into Makefile.buildvars

> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 2ed953b84409..1843679fc0e2 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile

[ ... ]

> @@ -213,21 +149,8 @@ ifeq ($(feature-llvm),1)
>  endif
>  endif
>
> -SCRATCH_DIR := $(OUTPUT)/tools
> -BUILD_DIR := $(SCRATCH_DIR)/build
> -INCLUDE_DIR := $(SCRATCH_DIR)/include
> -BPFOBJ := $(BUILD_DIR)/libbpf/libbpf.a
> -ifneq ($(CROSS_COMPILE),)
> -HOST_BUILD_DIR		:= $(BUILD_DIR)/host
> -HOST_SCRATCH_DIR	:= $(OUTPUT)/host-tools
> -HOST_INCLUDE_DIR	:= $(HOST_SCRATCH_DIR)/include
> -else
> -HOST_BUILD_DIR		:= $(BUILD_DIR)
> -HOST_SCRATCH_DIR	:= $(SCRATCH_DIR)
> -HOST_INCLUDE_DIR	:= $(INCLUDE_DIR)
> -endif
> -HOST_BPFOBJ := $(HOST_BUILD_DIR)/libbpf/libbpf.a
> -RESOLVE_BTFIDS := $(HOST_BUILD_DIR)/resolve_btfids/resolve_btfids
> +include Makefile.buildvars
> +
>  VMLINUX_BTF_PATHS ?= $(if $(O),$(O)/vmlinux)				\
>  		     $(if $(KBUILD_OUTPUT),$(KBUILD_OUTPUT)/vmlinux)	\
>  		     ../../../../vmlinux				\

The srctree fallback block moved into Makefile.buildvars, but that file is
included after ../../../build/Makefile.feature has already been parsed. Does
this break the feature-llvm probe for standalone builds?

In the pre-image, the ordering was:

  include ../../../scripts/Makefile.include
  <srctree fallback at old line 53-58>
  include ../lib.mk
  $(eval include ../../../build/Makefile.feature)  # old line 183/186

After the patch:

  include ../../../scripts/Makefile.include
  include ../lib.mk
  $(eval include ../../../build/Makefile.feature)  # new line 119/122
  include Makefile.buildvars                        # srctree defined here

Makefile.feature line 2 does:

  feature_dir := $(srctree)/tools/build/feature

This is an immediate assignment, so it captures srctree when
Makefile.feature is parsed. In a standalone 'cd tools/testing/selftests/bpf
&& make' build, srctree is not in the environment and nothing in the earlier
include chain defines it - none of tools/build/Build.include,
tools/scripts/Makefile.arch, tools/scripts/Makefile.include or
tools/testing/selftests/lib.mk contain a srctree assignment.

Consequently feature_dir becomes '/tools/build/feature' instead of
'<tree>/tools/build/feature', and the probe at Makefile.feature:11 runs:

  $(MAKE) ... -C /tools/build/feature ...

The '-C /tools/build/feature' fails because the directory does not exist,
stderr is discarded, and the probe silently yields feature-llvm=0.

This flips the conditional at Makefile:127-150 off, so LLVM_CFLAGS never
gains -DHAVE_LLVM_SUPPORT and LLVM_LDLIBS/LLVM_LDFLAGS stay empty.
jit_disasm_helpers.c then compiles its '#else' stub whose get_jited_program_text()
returns -EOPNOTSUPP, and test_loader.c:1442-1449 turns every __jited expectation
into test__skip() with "tests are built w/o LLVM development libs". The six
progs/ files that use __jited stop running, and test_progs loses
-DHAVE_LLVM_SUPPORT and the LLVM libraries it previously linked.

This contradicts the commit message's "Build artifacts are byte-identical
before and after this change" claim for the standard build invocation
documented in Documentation/bpf/bpf_devel_QA.rst:459.

The srctree fallback belongs with BPF_GCC in the "needed before lib.mk"
group that the commit message already identifies, for the same reason -
it is consumed before Makefile.buildvars is included. The same applies to
PKG_CONFIG, which is used in Makefile.feature at lines 178/183 but is now
only defined in Makefile.buildvars.

> diff --git a/tools/testing/selftests/bpf/Makefile.buildvars b/tools/testing/selftests/bpf/Makefile.buildvars
> new file mode 100644
> index 000000000000..ef12018fa254
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/Makefile.buildvars
> @@ -0,0 +1,165 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Shared toolchain, path and flag definitions for the BPF selftests
> +# build, included by both Makefile and Makefile.runner. Definitions
> +# follow the order they had in the Makefile before the split.
> +
> +CXX ?= $(CROSS_COMPILE)g++
> +OBJCOPY ?= $(CROSS_COMPILE)objcopy
> +
> +CURDIR := $(abspath .)
> +
> +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

Same ordering issue: PKG_CONFIG was defined before ../lib.mk in the
pre-image, but now it is only defined here in Makefile.buildvars, which is
reached after Makefile.feature has already been parsed.

Makefile.feature uses PKG_CONFIG at parse time:

  ifneq ($(PKG_CONFIG),)
    $(foreach package,$(FEATURE_PKG_CONFIG),$(call feature_pkg_config,$(package)))

with immediate assignments at lines 178-179:

  FEATURE_CHECK_CFLAGS-$(1)  := $(shell $(PKG_CONFIG) --cflags $(1) 2>/dev/null)
  FEATURE_CHECK_LDFLAGS-$(1) := $(shell $(PKG_CONFIG) --libs $(1) 2>/dev/null)

Nothing parsed before Makefile.buildvars defines PKG_CONFIG, so the ifneq
guard becomes false and the pkg-config-derived FEATURE_CHECK_*FLAGS block is
skipped.

> +ifeq ($(srctree),)
> +srctree := $(patsubst %/,%,$(dir $(CURDIR)))
> +srctree := $(patsubst %/,%,$(dir $(srctree)))
> +srctree := $(patsubst %/,%,$(dir $(srctree)))
> +srctree := $(patsubst %/,%,$(dir $(srctree)))
> +endif

[ ... ]

> +ifneq ($(CROSS_COMPILE),)
> +CLANG_TARGET_ARCH = --target=$(notdir $(CROSS_COMPILE:%-=%))
> +endif
> +
> +# Immediate on purpose: expanding get_sys_includes forks several shells.
> +CLANG_SYS_INCLUDES := $(call get_sys_includes,$(CLANG),$(CLANG_TARGET_ARCH))

The commit message states "Definitions keep the order they had in the
Makefile; the only exception is called out in a comment (CFLAGS is prepended
rather than appended, as this file is included after lib.mk)."

However, CLANG_SYS_INCLUDES also changed from recursive assignment ('=') to
immediate assignment (':='):

  pre  (Makefile:463): CLANG_SYS_INCLUDES = $(call get_sys_includes,$(CLANG),$(CLANG_TARGET_ARCH))
  post (buildvars:139): CLANG_SYS_INCLUDES := $(call get_sys_includes,$(CLANG),$(CLANG_TARGET_ARCH))

The inline comment explains the fork-count motivation, but this is a second
evaluation-time change beyond the CFLAGS exception documented in the
changelog. It happens to be benign at this commit since CLANG and
CLANG_TARGET_ARCH are already final, but any later target-specific or
conditional override of CLANG would silently stop affecting CLANG_SYS_INCLUDES.

Would it be worth mentioning this change in the commit message too?


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