On 10/16, Andrii Nakryiko wrote: > On Wed, Oct 16, 2019 at 9:32 AM Stanislav Fomichev <sdf@xxxxxxxxxxx> wrote: > > > > On 10/15, Andrii Nakryiko wrote: > > > Define test runner generation meta-rule that codifies dependencies > > > between test runner, its tests, and its dependent BPF programs. Use that > > > for defining test_progs and test_maps test-runners. Also additionally define > > > 2 flavors of test_progs: > > > - alu32, which builds BPF programs with 32-bit registers codegen; > > > - bpf_gcc, which build BPF programs using GCC, if it supports BPF target. > > Question: > > > > Why not merge test_maps tests into test_progs framework and have a > > single binary instead of doing all this makefile-related work? > > We can independently address the story with alu32/gcc progs (presumably > > in the same manner, with make defines). > > test_maps wasn't a reason for doing this, alue2/bpf_gcc was. test_maps > is a simple sub-case that was just easy to convert to. I dare you to > try solve alu32/bpf_gcc with make defines (whatever you mean by that) > and in a simpler manner ;) I think my concern comes from the fact that I don't really understand why we need all that complexity (and the problem you're solving for alu/gcc; part of that is that you're replacing everything, so it's hard to understand what's the real diff). In particular, why do we need to compile test_progs 3 times for normal/alu32/gcc? Isn't it the same test_progs? Can we just teach test_progs to run the tests for 3 output dirs with different versions of BPF programs? (kind of like you do in your first patch with -<flavor>, but just in a loop). > > I can hardly follow the existing makefile and now with the evals it's > > 10x more complicated for no good reason. > > I agree that existing Makefile logic is hard to follow, especially > given it's broken. But I think 10x more complexity is gross > exaggeration and just means you haven't tried to follow rules' logic. Not 10x, but it does raise a complexity bar. I tried to follow the rules, but I admit that I didn't try too hard :-) > The rules inside DEFINE_TEST_RUNNER_RULES are exactly (minus one or > two ifs to prevent re-definition of target) the rules that should have > been written for test_progs, test_progs-alu32, test_progs-bpf_gcc. > They define a chain of BPF .c -> BPF .o -> tests .c -> tests .o -> > final binary + test.h generation. Previously we were getting away with > this for, e.g., test_progs-alu32, because we always also built > test_progs in parallel, which generated necessary stuff. Now with > recent changes to test_attach_probe.c which now embeds BPF .o file, > this doesn't work anymore. And it's going to be more and more > prevalent form, so we need to fix it. > > Surely $(eval) and $(call) are not common for simple Makefiles, but > just ignore it, we need that to only dynamically generate > per-test-runner rules. DEFINE_TEST_RUNNER_RULES can be almost read > like a normal Makefile definitions, module $$(VAR) which is turned > into a normal $(VAR) upon $(call) evaluation. > > But really, I'd like to be wrong and if there is simpler way to > achieve the same - go for it, I'll gladly review and ack. Again, it probably comes from the fact that I don't see the problem you're solving. Can we start by removing 3 test_progs variations (somthing like patch below)? If we can do it, then the leftover parts that generate alu32/gcc bpf program don't look too bad and can probably be tweaked without makefile codegen. --- a/tools/testing/selftests/bpf/Makefile +++ b/tools/testing/selftests/bpf/Makefile @@ -157,26 +157,10 @@ TEST_VERIFIER_CFLAGS := -I. -I$(OUTPUT) -Iverifier ifneq ($(SUBREG_CODEGEN),) ALU32_BUILD_DIR = $(OUTPUT)/alu32 -TEST_CUSTOM_PROGS += $(ALU32_BUILD_DIR)/test_progs_32 $(ALU32_BUILD_DIR): mkdir -p $@ -$(ALU32_BUILD_DIR)/urandom_read: $(OUTPUT)/urandom_read | $(ALU32_BUILD_DIR) - cp $< $@ - -$(ALU32_BUILD_DIR)/test_progs_32: test_progs.c $(OUTPUT)/libbpf.a\ - $(ALU32_BUILD_DIR)/urandom_read \ - | $(ALU32_BUILD_DIR) - $(CC) $(TEST_PROGS_CFLAGS) $(CFLAGS) \ - -o $(ALU32_BUILD_DIR)/test_progs_32 \ - test_progs.c test_stub.c cgroup_helpers.c trace_helpers.c prog_tests/*.c \ - $(OUTPUT)/libbpf.a $(LDLIBS) - -$(ALU32_BUILD_DIR)/test_progs_32: $(PROG_TESTS_H) -$(ALU32_BUILD_DIR)/test_progs_32: prog_tests/*.c - -$(ALU32_BUILD_DIR)/%.o: progs/%.c $(ALU32_BUILD_DIR)/test_progs_32 \ - | $(ALU32_BUILD_DIR) +$(ALU32_BUILD_DIR)/%.o: progs/%.c | $(ALU32_BUILD_DIR) ($(CLANG) $(BPF_CFLAGS) $(CLANG_CFLAGS) -O2 -target bpf -emit-llvm \ -c $< -o - || echo "clang failed") | \ $(LLC) -march=bpf -mcpu=probe -mattr=+alu32 $(LLC_FLAGS) \ @@ -194,19 +178,10 @@ MENDIAN=-mlittle-endian endif BPF_GCC_CFLAGS = $(GCC_SYS_INCLUDES) $(MENDIAN) BPF_GCC_BUILD_DIR = $(OUTPUT)/bpf_gcc -TEST_CUSTOM_PROGS += $(BPF_GCC_BUILD_DIR)/test_progs_bpf_gcc $(BPF_GCC_BUILD_DIR): mkdir -p $@ -$(BPF_GCC_BUILD_DIR)/urandom_read: $(OUTPUT)/urandom_read | $(BPF_GCC_BUILD_DIR) - cp $< $@ - -$(BPF_GCC_BUILD_DIR)/test_progs_bpf_gcc: $(OUTPUT)/test_progs \ - | $(BPF_GCC_BUILD_DIR) - cp $< $@ - -$(BPF_GCC_BUILD_DIR)/%.o: progs/%.c $(BPF_GCC_BUILD_DIR)/test_progs_bpf_gcc \ - | $(BPF_GCC_BUILD_DIR) +$(BPF_GCC_BUILD_DIR)/%.o: progs/%.c | $(BPF_GCC_BUILD_DIR) $(BPF_GCC) $(BPF_CFLAGS) $(BPF_GCC_CFLAGS) -O2 -c $< -o $@ endif > Please truncate irrelevant parts, easier to review. Sure, will do, but I always forget because I don't have this problem. In mutt I can press shift+s to jump to the next unquoted section.
