2022-04-23 17:12:38

by Yuntao Wang

[permalink] [raw]
Subject: [PATCH bpf-next] selftests/bpf: Fix incorrect TRUNNER_BINARY name output

Currently, when we run 'make test_progs', the output is:

CLNG-BPF [test_maps] atomic_bounds.o
...
GEN-SKEL [test_progs] atomic_bounds.skel.h
...
TEST-OBJ [test_progs] align.test.o
...
TEST-HDR [test_progs] tests.h
EXT-OBJ [test_progs] test_progs.o
...
BINARY test_progs

As you can see, the TRUNNER_BINARY name in the CLNG-BPF part is test_maps,
which is incorrect.

Similarly, when we run 'make test_maps', the output is:

CLNG-BPF [test_maps] atomic_bounds.o
...
GEN-SKEL [test_progs] atomic_bounds.skel.h
...
TEST-OBJ [test_maps] array_map_batch_ops.test.o
...
TEST-HDR [test_maps] tests.h
EXT-OBJ [test_maps] test_maps.o
...
BINARY test_maps

At this time, the TRUNNER_BINARY name in the GEN-SKEL part is wrong.

Again, if we run 'make /full/path/to/selftests/bpf/test_vmlinux.skel.h',
the output is:

CLNG-BPF [test_maps] test_vmlinux.o
GEN-SKEL [test_progs] test_vmlinux.skel.h

Here, the TRUNNER_BINARY names are inappropriate and meaningless, they
should be removed.

This patch fixes these and all other similar issues.

With the patch applied, the output becomes:

$ make test_progs

CLNG-BPF [test_progs] atomic_bounds.o
...
GEN-SKEL [test_progs] atomic_bounds.skel.h
...
TEST-OBJ [test_progs] align.test.o
...
TEST-HDR [test_progs] tests.h
EXT-OBJ [test_progs] test_progs.o
...
BINARY test_progs

$ make test_maps

CLNG-BPF [test_maps] atomic_bounds.o
...
GEN-SKEL [test_maps] atomic_bounds.skel.h
...
TEST-OBJ [test_maps] array_map_batch_ops.test.o
...
TEST-HDR [test_maps] tests.h
EXT-OBJ [test_maps] test_maps.o
...
BINARY test_maps

$ make /full/path/to/selftests/bpf/test_vmlinux.skel.h

CLNG-BPF test_vmlinux.o
GEN-SKEL test_vmlinux.skel.h

Signed-off-by: Yuntao Wang <[email protected]>
---
tools/testing/selftests/bpf/Makefile | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index bafdc5373a13..3cf444cb20af 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -413,7 +413,7 @@ $(TRUNNER_BPF_OBJS): $(TRUNNER_OUTPUT)/%.o: \
$(TRUNNER_BPF_CFLAGS))

$(TRUNNER_BPF_SKELS): %.skel.h: %.o $(BPFTOOL) | $(TRUNNER_OUTPUT)
- $$(call msg,GEN-SKEL,$(TRUNNER_BINARY),$$@)
+ $$(call msg,GEN-SKEL,$$(TRUNNER_BINARY),$$@)
$(Q)$$(BPFTOOL) gen object $$(<:.o=.linked1.o) $$<
$(Q)$$(BPFTOOL) gen object $$(<:.o=.linked2.o) $$(<:.o=.linked1.o)
$(Q)$$(BPFTOOL) gen object $$(<:.o=.linked3.o) $$(<:.o=.linked2.o)
@@ -422,7 +422,7 @@ $(TRUNNER_BPF_SKELS): %.skel.h: %.o $(BPFTOOL) | $(TRUNNER_OUTPUT)
$(Q)$$(BPFTOOL) gen subskeleton $$(<:.o=.linked3.o) name $$(notdir $$(<:.o=)) > $$(@:.skel.h=.subskel.h)

$(TRUNNER_BPF_LSKELS): %.lskel.h: %.o $(BPFTOOL) | $(TRUNNER_OUTPUT)
- $$(call msg,GEN-SKEL,$(TRUNNER_BINARY),$$@)
+ $$(call msg,GEN-SKEL,$$(TRUNNER_BINARY),$$@)
$(Q)$$(BPFTOOL) gen object $$(<:.o=.linked1.o) $$<
$(Q)$$(BPFTOOL) gen object $$(<:.o=.linked2.o) $$(<:.o=.linked1.o)
$(Q)$$(BPFTOOL) gen object $$(<:.o=.linked3.o) $$(<:.o=.linked2.o)
@@ -430,12 +430,12 @@ $(TRUNNER_BPF_LSKELS): %.lskel.h: %.o $(BPFTOOL) | $(TRUNNER_OUTPUT)
$(Q)$$(BPFTOOL) gen skeleton -L $$(<:.o=.linked3.o) name $$(notdir $$(<:.o=_lskel)) > $$@

$(TRUNNER_BPF_SKELS_LINKED): $(TRUNNER_BPF_OBJS) $(BPFTOOL) | $(TRUNNER_OUTPUT)
- $$(call msg,LINK-BPF,$(TRUNNER_BINARY),$$(@:.skel.h=.o))
+ $$(call msg,LINK-BPF,$$(TRUNNER_BINARY),$$(@:.skel.h=.o))
$(Q)$$(BPFTOOL) gen object $$(@:.skel.h=.linked1.o) $$(addprefix $(TRUNNER_OUTPUT)/,$$($$(@F)-deps))
$(Q)$$(BPFTOOL) gen object $$(@:.skel.h=.linked2.o) $$(@:.skel.h=.linked1.o)
$(Q)$$(BPFTOOL) gen object $$(@:.skel.h=.linked3.o) $$(@:.skel.h=.linked2.o)
$(Q)diff $$(@:.skel.h=.linked2.o) $$(@:.skel.h=.linked3.o)
- $$(call msg,GEN-SKEL,$(TRUNNER_BINARY),$$@)
+ $$(call msg,GEN-SKEL,$$(TRUNNER_BINARY),$$@)
$(Q)$$(BPFTOOL) gen skeleton $$(@:.skel.h=.linked3.o) name $$(notdir $$(@:.skel.h=)) > $$@
$(Q)$$(BPFTOOL) gen subskeleton $$(@:.skel.h=.linked3.o) name $$(notdir $$(@:.skel.h=)) > $$(@:.skel.h=.subskel.h)
endif
@@ -444,7 +444,7 @@ endif
ifeq ($($(TRUNNER_TESTS_DIR)-tests-hdr),)
$(TRUNNER_TESTS_DIR)-tests-hdr := y
$(TRUNNER_TESTS_HDR): $(TRUNNER_TESTS_DIR)/*.c
- $$(call msg,TEST-HDR,$(TRUNNER_BINARY),$$@)
+ $$(call msg,TEST-HDR,$$(TRUNNER_BINARY),$$@)
$$(shell (echo '/* Generated header, do not edit */'; \
sed -n -E 's/^void (serial_)?test_([a-zA-Z0-9_]+)\((void)?\).*/DEFINE_TEST(\2)/p' \
$(TRUNNER_TESTS_DIR)/*.c | sort ; \
@@ -461,7 +461,7 @@ $(TRUNNER_TEST_OBJS): $(TRUNNER_OUTPUT)/%.test.o: \
$(TRUNNER_BPF_LSKELS) \
$(TRUNNER_BPF_SKELS_LINKED) \
$$(BPFOBJ) | $(TRUNNER_OUTPUT)
- $$(call msg,TEST-OBJ,$(TRUNNER_BINARY),$$@)
+ $$(call msg,TEST-OBJ,$$(TRUNNER_BINARY),$$@)
$(Q)cd $$(@D) && $$(CC) -I. $$(CFLAGS) -c $(CURDIR)/$$< $$(LDLIBS) -o $$(@F)

$(TRUNNER_EXTRA_OBJS): $(TRUNNER_OUTPUT)/%.o: \
@@ -469,17 +469,19 @@ $(TRUNNER_EXTRA_OBJS): $(TRUNNER_OUTPUT)/%.o: \
$(TRUNNER_EXTRA_HDRS) \
$(TRUNNER_TESTS_HDR) \
$$(BPFOBJ) | $(TRUNNER_OUTPUT)
- $$(call msg,EXT-OBJ,$(TRUNNER_BINARY),$$@)
+ $$(call msg,EXT-OBJ,$$(TRUNNER_BINARY),$$@)
$(Q)$$(CC) $$(CFLAGS) -c $$< $$(LDLIBS) -o $$@

# non-flavored in-srctree builds receive special treatment, in particular, we
# do not need to copy extra resources (see e.g. test_btf_dump_case())
$(TRUNNER_BINARY)-extras: $(TRUNNER_EXTRA_FILES) | $(TRUNNER_OUTPUT)
ifneq ($2:$(OUTPUT),:$(shell pwd))
- $$(call msg,EXT-COPY,$(TRUNNER_BINARY),$(TRUNNER_EXTRA_FILES))
+ $$(call msg,EXT-COPY,$$(TRUNNER_BINARY),$(TRUNNER_EXTRA_FILES))
$(Q)rsync -aq $$^ $(TRUNNER_OUTPUT)/
endif

+$(OUTPUT)/$(TRUNNER_BINARY): TRUNNER_BINARY = $(TRUNNER_BINARY)
+
$(OUTPUT)/$(TRUNNER_BINARY): $(TRUNNER_TEST_OBJS) \
$(TRUNNER_EXTRA_OBJS) $$(BPFOBJ) \
$(RESOLVE_BTFIDS) \
@@ -489,6 +491,8 @@ $(OUTPUT)/$(TRUNNER_BINARY): $(TRUNNER_TEST_OBJS) \
$(Q)$(RESOLVE_BTFIDS) --btf $(TRUNNER_OUTPUT)/btf_data.o $$@
$(Q)ln -sf $(if $2,..,.)/tools/build/bpftool/bootstrap/bpftool $(if $2,$2/)bpftool

+TRUNNER_BINARY =
+
endef

# Define test_progs test runner.
--
2.35.3


2022-04-27 20:08:01

by Andrii Nakryiko

[permalink] [raw]
Subject: Re: [PATCH bpf-next] selftests/bpf: Fix incorrect TRUNNER_BINARY name output

On Sat, Apr 23, 2022 at 7:30 AM Yuntao Wang <[email protected]> wrote:
>
> Currently, when we run 'make test_progs', the output is:
>
> CLNG-BPF [test_maps] atomic_bounds.o
> ...
> GEN-SKEL [test_progs] atomic_bounds.skel.h
> ...
> TEST-OBJ [test_progs] align.test.o
> ...
> TEST-HDR [test_progs] tests.h
> EXT-OBJ [test_progs] test_progs.o
> ...
> BINARY test_progs
>
> As you can see, the TRUNNER_BINARY name in the CLNG-BPF part is test_maps,
> which is incorrect.

It's not incorrect. test_maps and test_progs share the same set of BPF
object files under progs/ so whichever rule is picked first by make
gets to output it's [test_maps] or [test_progs] "badge". Is that a big
deal? Adding this $$(TRUNNER_BINARY) indirection and per-target
private TRUNNER_BINARY envvar is an unnecessary complication of
already complicated Makefile, IMO.

Did you run into any problems with the way Makefile is right now?

>
> Similarly, when we run 'make test_maps', the output is:
>
> CLNG-BPF [test_maps] atomic_bounds.o
> ...
> GEN-SKEL [test_progs] atomic_bounds.skel.h
> ...
> TEST-OBJ [test_maps] array_map_batch_ops.test.o
> ...
> TEST-HDR [test_maps] tests.h
> EXT-OBJ [test_maps] test_maps.o
> ...
> BINARY test_maps
>
> At this time, the TRUNNER_BINARY name in the GEN-SKEL part is wrong.
>
> Again, if we run 'make /full/path/to/selftests/bpf/test_vmlinux.skel.h',
> the output is:
>
> CLNG-BPF [test_maps] test_vmlinux.o
> GEN-SKEL [test_progs] test_vmlinux.skel.h
>
> Here, the TRUNNER_BINARY names are inappropriate and meaningless, they
> should be removed.
>
> This patch fixes these and all other similar issues.
>
> With the patch applied, the output becomes:
>
> $ make test_progs
>
> CLNG-BPF [test_progs] atomic_bounds.o
> ...
> GEN-SKEL [test_progs] atomic_bounds.skel.h
> ...
> TEST-OBJ [test_progs] align.test.o
> ...
> TEST-HDR [test_progs] tests.h
> EXT-OBJ [test_progs] test_progs.o
> ...
> BINARY test_progs
>
> $ make test_maps
>
> CLNG-BPF [test_maps] atomic_bounds.o
> ...
> GEN-SKEL [test_maps] atomic_bounds.skel.h
> ...
> TEST-OBJ [test_maps] array_map_batch_ops.test.o
> ...
> TEST-HDR [test_maps] tests.h
> EXT-OBJ [test_maps] test_maps.o
> ...
> BINARY test_maps
>
> $ make /full/path/to/selftests/bpf/test_vmlinux.skel.h
>
> CLNG-BPF test_vmlinux.o
> GEN-SKEL test_vmlinux.skel.h
>
> Signed-off-by: Yuntao Wang <[email protected]>
> ---
> tools/testing/selftests/bpf/Makefile | 20 ++++++++++++--------
> 1 file changed, 12 insertions(+), 8 deletions(-)
>

[...]

2022-04-29 12:09:59

by Yuntao Wang

[permalink] [raw]
Subject: Re: [PATCH bpf-next] selftests/bpf: Fix incorrect TRUNNER_BINARY name output

Hi Andrii,

I didn't run into any real problems, the purpose of this patch was to make
the output more consistent with expectations.

It confused me when I ran 'make test_progs' but saw [test_maps] output, I
even doubted whether I ran the correct command at first.

But as you said, it's not a big deal, I'm okay with keeping it as it is.

Thanks,
Yuntao