* [PATCH bpf-next] selftests/bpf: do not ignore clang failures @ 2019-06-27 9:14 Ilya Leoshkevich 2019-06-28 20:35 ` Song Liu 0 siblings, 1 reply; 10+ messages in thread From: Ilya Leoshkevich @ 2019-06-27 9:14 UTC (permalink / raw) To: bpf; +Cc: Ilya Leoshkevich When compiling an eBPF prog fails, make still returns 0, because failing clang command's output is piped to llc and therefore its exit status is ignored. This patch uses bash's pipefail option to fail the build when clang fails, and also make's .DELETE_ON_ERROR target to get rid of partial BPF bytecode files. Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> --- tools/testing/selftests/bpf/Makefile | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile index f2dbe2043067..2316fa2d5b3b 100644 --- a/tools/testing/selftests/bpf/Makefile +++ b/tools/testing/selftests/bpf/Makefile @@ -1,5 +1,6 @@ # SPDX-License-Identifier: GPL-2.0 +SHELL := /bin/bash LIBDIR := ../../../lib BPFDIR := $(LIBDIR)/bpf APIDIR := ../../../include/uapi @@ -191,7 +192,7 @@ $(ALU32_BUILD_DIR)/test_progs_32: prog_tests/*.c $(ALU32_BUILD_DIR)/%.o: progs/%.c $(ALU32_BUILD_DIR) \ $(ALU32_BUILD_DIR)/test_progs_32 - $(CLANG) $(CLANG_FLAGS) \ + set -o pipefail; $(CLANG) $(CLANG_FLAGS) \ -O2 -target bpf -emit-llvm -c $< -o - | \ $(LLC) -march=bpf -mattr=+alu32 -mcpu=$(CPU) $(LLC_FLAGS) \ -filetype=obj -o $@ @@ -203,7 +204,7 @@ endif # Have one program compiled without "-target bpf" to test whether libbpf loads # it successfully $(OUTPUT)/test_xdp.o: progs/test_xdp.c - $(CLANG) $(CLANG_FLAGS) \ + set -o pipefail; $(CLANG) $(CLANG_FLAGS) \ -O2 -emit-llvm -c $< -o - | \ $(LLC) -march=bpf -mcpu=$(CPU) $(LLC_FLAGS) -filetype=obj -o $@ ifeq ($(DWARF2BTF),y) @@ -211,7 +212,7 @@ ifeq ($(DWARF2BTF),y) endif $(OUTPUT)/%.o: progs/%.c - $(CLANG) $(CLANG_FLAGS) \ + set -o pipefail; $(CLANG) $(CLANG_FLAGS) \ -O2 -target bpf -emit-llvm -c $< -o - | \ $(LLC) -march=bpf -mcpu=$(CPU) $(LLC_FLAGS) -filetype=obj -o $@ ifeq ($(DWARF2BTF),y) @@ -283,3 +284,5 @@ $(OUTPUT)/verifier/tests.h: $(VERIFIER_TESTS_DIR) $(VERIFIER_TEST_FILES) EXTRA_CLEAN := $(TEST_CUSTOM_PROGS) $(ALU32_BUILD_DIR) \ $(VERIFIER_TESTS_H) $(PROG_TESTS_H) $(MAP_TESTS_H) \ feature + +.DELETE_ON_ERROR: -- 2.21.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: do not ignore clang failures 2019-06-27 9:14 [PATCH bpf-next] selftests/bpf: do not ignore clang failures Ilya Leoshkevich @ 2019-06-28 20:35 ` Song Liu 2019-07-01 8:55 ` Ilya Leoshkevich 0 siblings, 1 reply; 10+ messages in thread From: Song Liu @ 2019-06-28 20:35 UTC (permalink / raw) To: Ilya Leoshkevich; +Cc: bpf On Thu, Jun 27, 2019 at 2:15 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote: > > When compiling an eBPF prog fails, make still returns 0, because > failing clang command's output is piped to llc and therefore its > exit status is ignored. > > This patch uses bash's pipefail option to fail the build when clang > fails, and also make's .DELETE_ON_ERROR target to get rid of partial > BPF bytecode files. > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > --- > tools/testing/selftests/bpf/Makefile | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile > index f2dbe2043067..2316fa2d5b3b 100644 > --- a/tools/testing/selftests/bpf/Makefile > +++ b/tools/testing/selftests/bpf/Makefile > @@ -1,5 +1,6 @@ > # SPDX-License-Identifier: GPL-2.0 > > +SHELL := /bin/bash I am not sure whether it is ok to require bash. I don't see such requirements in other Makefile's under tools/. Can we enable some fall back when bash is not present? Thanks, Song ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: do not ignore clang failures 2019-06-28 20:35 ` Song Liu @ 2019-07-01 8:55 ` Ilya Leoshkevich 2019-07-01 15:31 ` Andrii Nakryiko 0 siblings, 1 reply; 10+ messages in thread From: Ilya Leoshkevich @ 2019-07-01 8:55 UTC (permalink / raw) To: Song Liu; +Cc: bpf > Am 28.06.2019 um 22:35 schrieb Song Liu <liu.song.a23@gmail.com>: > > On Thu, Jun 27, 2019 at 2:15 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote: >> >> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile >> index f2dbe2043067..2316fa2d5b3b 100644 >> --- a/tools/testing/selftests/bpf/Makefile >> +++ b/tools/testing/selftests/bpf/Makefile >> @@ -1,5 +1,6 @@ >> # SPDX-License-Identifier: GPL-2.0 >> >> +SHELL := /bin/bash > > I am not sure whether it is ok to require bash. I don't see such requirements in > other Makefile's under tools/. > > Can we enable some fall back when bash is not present? > > Thanks, > Song I think checking for bash presence would unnecessarily complicate things. What do you think about having separate targets for clang-generated bitcode? Best regards, Ilya ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: do not ignore clang failures 2019-07-01 8:55 ` Ilya Leoshkevich @ 2019-07-01 15:31 ` Andrii Nakryiko 2019-07-01 18:40 ` [PATCH v2 " Ilya Leoshkevich 0 siblings, 1 reply; 10+ messages in thread From: Andrii Nakryiko @ 2019-07-01 15:31 UTC (permalink / raw) To: Ilya Leoshkevich; +Cc: Song Liu, bpf On Mon, Jul 1, 2019 at 1:56 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote: > > > Am 28.06.2019 um 22:35 schrieb Song Liu <liu.song.a23@gmail.com>: > > > > On Thu, Jun 27, 2019 at 2:15 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote: > >> > >> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile > >> index f2dbe2043067..2316fa2d5b3b 100644 > >> --- a/tools/testing/selftests/bpf/Makefile > >> +++ b/tools/testing/selftests/bpf/Makefile > >> @@ -1,5 +1,6 @@ > >> # SPDX-License-Identifier: GPL-2.0 > >> > >> +SHELL := /bin/bash > > > > I am not sure whether it is ok to require bash. I don't see such requirements in > > other Makefile's under tools/. > > > > Can we enable some fall back when bash is not present? > > > > Thanks, > > Song > > I think checking for bash presence would unnecessarily complicate > things. What do you think about having separate targets for > clang-generated bitcode? Do we still need clang | llc pipeline with new clang? Could the same be achieved with single clang invocation? That would solve the problem of not detecting pipeline failures. But either way, I think .DELETE_ON_ERROR is worth adding nevertheless. > > Best regards, > Ilya ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 bpf-next] selftests/bpf: do not ignore clang failures 2019-07-01 15:31 ` Andrii Nakryiko @ 2019-07-01 18:40 ` Ilya Leoshkevich 2019-07-05 14:22 ` Daniel Borkmann 0 siblings, 1 reply; 10+ messages in thread From: Ilya Leoshkevich @ 2019-07-01 18:40 UTC (permalink / raw) To: bpf, liu.song.a23, andrii.nakryiko; +Cc: Ilya Leoshkevich Am 01.07.2019 um 17:31 schrieb Andrii Nakryiko <andrii.nakryiko@gmail.com>: > Do we still need clang | llc pipeline with new clang? Could the same > be achieved with single clang invocation? That would solve the problem > of not detecting pipeline failures. I’ve experimented with this a little, and found that new clang: - Does not understand -march, but -target is sufficient. - Understands -mcpu. - Understands -Xclang -target-feature -Xclang +foo as a replacement for -mattr=foo. However, there are two issues with that: - Don’t older clangs need to be supported? For example, right now alu32 progs are built conditionally. - It does not seem to be possible to build test_xdp.o without -target bpf. For now I'm attaching the new version of this patch, which introduces intermediate targets for LLVM bitcode and does not require bash. --- When compiling an eBPF prog fails, make still returns 0, because failing clang command's output is piped to llc and therefore its exit status is ignored. Create separate targets for clang and llc invocations, so that when clang fails, llc is not invoked at all, and make returns nonzero. Pull Kbuild.include for .SECONDARY target, which prevents make from deleting intermediate LLVM bitcode files. Adding .bc targets triggers the latent problem with depending on $(ALU32_BUILD_DIR): since directories are considered changed whenever a member is added or removed, now everything that depends on $(ALU32_BUILD_DIR) is always considered out-of-date. While removing $(ALU32_BUILD_DIR) target might be tempting, since most targets already depend on files inside it and therefore don't need it, it might create problems in the future, when such dependencies need to be removed. So, instead, add $(ALU32_BUILD_DIR) where needed as an order-only prerequisite. make provides this feature since version 3.80. Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> --- tools/testing/selftests/bpf/.gitignore | 1 + tools/testing/selftests/bpf/Makefile | 34 ++++++++++++++++---------- 2 files changed, 22 insertions(+), 13 deletions(-) diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore index a2f7f79c7908..4604a54e3ff2 100644 --- a/tools/testing/selftests/bpf/.gitignore +++ b/tools/testing/selftests/bpf/.gitignore @@ -42,3 +42,4 @@ xdping test_sockopt test_sockopt_sk test_sockopt_multi +*.bc diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile index de1754a8f5fe..d60fee59fbd1 100644 --- a/tools/testing/selftests/bpf/Makefile +++ b/tools/testing/selftests/bpf/Makefile @@ -1,4 +1,5 @@ # SPDX-License-Identifier: GPL-2.0 +include ../../../../scripts/Kbuild.include LIBDIR := ../../../lib BPFDIR := $(LIBDIR)/bpf @@ -179,12 +180,12 @@ 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)/urandom_read: $(OUTPUT)/urandom_read | $(ALU32_BUILD_DIR) cp $< $@ $(ALU32_BUILD_DIR)/test_progs_32: test_progs.c $(OUTPUT)/libbpf.a\ - $(ALU32_BUILD_DIR) \ - $(ALU32_BUILD_DIR)/urandom_read + $(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 trace_helpers.c prog_tests/*.c \ @@ -193,12 +194,15 @@ $(ALU32_BUILD_DIR)/test_progs_32: test_progs.c $(OUTPUT)/libbpf.a\ $(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) \ - $(ALU32_BUILD_DIR)/test_progs_32 +$(ALU32_BUILD_DIR)/%.bc: progs/%.c $(ALU32_BUILD_DIR)/test_progs_32 \ + | $(ALU32_BUILD_DIR) $(CLANG) $(CLANG_FLAGS) \ - -O2 -target bpf -emit-llvm -c $< -o - | \ + -O2 -target bpf -emit-llvm -c $< -o $@ + +$(ALU32_BUILD_DIR)/%.o: $(ALU32_BUILD_DIR)/%.bc \ + | $(ALU32_BUILD_DIR) $(LLC) -march=bpf -mattr=+alu32 -mcpu=$(CPU) $(LLC_FLAGS) \ - -filetype=obj -o $@ + -filetype=obj -o $@ $< ifeq ($(DWARF2BTF),y) $(BTF_PAHOLE) -J $@ endif @@ -206,18 +210,22 @@ endif # Have one program compiled without "-target bpf" to test whether libbpf loads # it successfully -$(OUTPUT)/test_xdp.o: progs/test_xdp.c +$(OUTPUT)/test_xdp.bc: progs/test_xdp.c $(CLANG) $(CLANG_FLAGS) \ - -O2 -emit-llvm -c $< -o - | \ - $(LLC) -march=bpf -mcpu=$(CPU) $(LLC_FLAGS) -filetype=obj -o $@ + -O2 -emit-llvm -c $< -o $@ + +$(OUTPUT)/test_xdp.o: $(OUTPUT)/test_xdp.bc + $(LLC) -march=bpf -mcpu=$(CPU) $(LLC_FLAGS) -filetype=obj -o $@ $< ifeq ($(DWARF2BTF),y) $(BTF_PAHOLE) -J $@ endif -$(OUTPUT)/%.o: progs/%.c +$(OUTPUT)/%.bc: progs/%.c $(CLANG) $(CLANG_FLAGS) \ - -O2 -target bpf -emit-llvm -c $< -o - | \ - $(LLC) -march=bpf -mcpu=$(CPU) $(LLC_FLAGS) -filetype=obj -o $@ + -O2 -target bpf -emit-llvm -c $< -o $@ + +$(OUTPUT)/%.o: $(OUTPUT)/%.bc + $(LLC) -march=bpf -mcpu=$(CPU) $(LLC_FLAGS) -filetype=obj -o $@ $< ifeq ($(DWARF2BTF),y) $(BTF_PAHOLE) -J $@ endif -- 2.21.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 bpf-next] selftests/bpf: do not ignore clang failures 2019-07-01 18:40 ` [PATCH v2 " Ilya Leoshkevich @ 2019-07-05 14:22 ` Daniel Borkmann 2019-07-08 15:01 ` Ilya Leoshkevich 0 siblings, 1 reply; 10+ messages in thread From: Daniel Borkmann @ 2019-07-05 14:22 UTC (permalink / raw) To: Ilya Leoshkevich, bpf, liu.song.a23, andrii.nakryiko On 07/01/2019 08:40 PM, Ilya Leoshkevich wrote: > Am 01.07.2019 um 17:31 schrieb Andrii Nakryiko <andrii.nakryiko@gmail.com>: >> Do we still need clang | llc pipeline with new clang? Could the same >> be achieved with single clang invocation? That would solve the problem >> of not detecting pipeline failures. > > I’ve experimented with this a little, and found that new clang: > > - Does not understand -march, but -target is sufficient. > - Understands -mcpu. > - Understands -Xclang -target-feature -Xclang +foo as a replacement for > -mattr=foo. > > However, there are two issues with that: > > - Don’t older clangs need to be supported? For example, right now alu32 > progs are built conditionally. We usually require latest clang to be able to test most recent features like BTF such that it helps to catch potential bugs in either of the projects before release. > - It does not seem to be possible to build test_xdp.o without -target > bpf. For everything non-tracing, it does not make sense to invoke clang w/o the -target bpf flag, see also Documentation/bpf/bpf_devel_QA.rst +573 for more explanation, so this needs to be present for building test_xdp.o. Thanks, Daniel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 bpf-next] selftests/bpf: do not ignore clang failures 2019-07-05 14:22 ` Daniel Borkmann @ 2019-07-08 15:01 ` Ilya Leoshkevich 2019-07-09 18:14 ` Andrii Nakryiko 0 siblings, 1 reply; 10+ messages in thread From: Ilya Leoshkevich @ 2019-07-08 15:01 UTC (permalink / raw) To: Daniel Borkmann; +Cc: bpf, Song Liu, andrii.nakryiko, Quentin Monnet > Am 05.07.2019 um 16:22 schrieb Daniel Borkmann <daniel@iogearbox.net>: > > On 07/01/2019 08:40 PM, Ilya Leoshkevich wrote: >> Am 01.07.2019 um 17:31 schrieb Andrii Nakryiko <andrii.nakryiko@gmail.com>: >>> Do we still need clang | llc pipeline with new clang? Could the same >>> be achieved with single clang invocation? That would solve the problem >>> of not detecting pipeline failures. >> >> I’ve experimented with this a little, and found that new clang: >> >> - Does not understand -march, but -target is sufficient. >> - Understands -mcpu. >> - Understands -Xclang -target-feature -Xclang +foo as a replacement for >> -mattr=foo. >> >> However, there are two issues with that: >> >> - Don’t older clangs need to be supported? For example, right now alu32 >> progs are built conditionally. > > We usually require latest clang to be able to test most recent features like > BTF such that it helps to catch potential bugs in either of the projects > before release. > >> - It does not seem to be possible to build test_xdp.o without -target >> bpf. > > For everything non-tracing, it does not make sense to invoke clang w/o > the -target bpf flag, see also Documentation/bpf/bpf_devel_QA.rst +573 > for more explanation, so this needs to be present for building test_xdp.o. I'm referring to the test introduced in [1]. test_xdp.o might not be an ideal target, but even if it's replaced with a more suitable one, the llc invocation would still be required. So I could redo the patch as follows: - Replace test_xdp.o with get_cgroup_id_kern.o, use an intermediate .bc file. - Use clang without llc for all other eBPF programs. - Split out Kbuild include and order-only prerequisites. What do you think? [1] https://lore.kernel.org/netdev/1541593725-27703-1-git-send-email-quentin.monnet@netronome.com/ Best regards, Ilya ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 bpf-next] selftests/bpf: do not ignore clang failures 2019-07-08 15:01 ` Ilya Leoshkevich @ 2019-07-09 18:14 ` Andrii Nakryiko 2019-07-10 13:25 ` Ilya Leoshkevich 0 siblings, 1 reply; 10+ messages in thread From: Andrii Nakryiko @ 2019-07-09 18:14 UTC (permalink / raw) To: Ilya Leoshkevich; +Cc: Daniel Borkmann, bpf, Song Liu, Quentin Monnet On Mon, Jul 8, 2019 at 8:01 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote: > > > Am 05.07.2019 um 16:22 schrieb Daniel Borkmann <daniel@iogearbox.net>: > > > > On 07/01/2019 08:40 PM, Ilya Leoshkevich wrote: > >> Am 01.07.2019 um 17:31 schrieb Andrii Nakryiko <andrii.nakryiko@gmail.com>: > >>> Do we still need clang | llc pipeline with new clang? Could the same > >>> be achieved with single clang invocation? That would solve the problem > >>> of not detecting pipeline failures. > >> > >> I’ve experimented with this a little, and found that new clang: > >> > >> - Does not understand -march, but -target is sufficient. > >> - Understands -mcpu. > >> - Understands -Xclang -target-feature -Xclang +foo as a replacement for > >> -mattr=foo. > >> > >> However, there are two issues with that: > >> > >> - Don’t older clangs need to be supported? For example, right now alu32 > >> progs are built conditionally. > > > > We usually require latest clang to be able to test most recent features like > > BTF such that it helps to catch potential bugs in either of the projects > > before release. > > > >> - It does not seem to be possible to build test_xdp.o without -target > >> bpf. > > > > For everything non-tracing, it does not make sense to invoke clang w/o > > the -target bpf flag, see also Documentation/bpf/bpf_devel_QA.rst +573 > > for more explanation, so this needs to be present for building test_xdp.o. > > I'm referring to the test introduced in [1]. test_xdp.o might not be an > ideal target, but even if it's replaced with a more suitable one, the > llc invocation would still be required. So I could redo the patch as > follows: > > - Replace test_xdp.o with get_cgroup_id_kern.o, use an intermediate .bc > file. > - Use clang without llc for all other eBPF programs. > - Split out Kbuild include and order-only prerequisites. > > What do you think? How about just forcing llc to fail as well like this: (clang <whatever> || echo "clain failed") | llc <whatever> While not pretty, it will get us what we need with very clear messaging as well. E.g.: progs/test_btf_newkv.c:21:37: error: expected identifier PF_ANNOTATE_KV_PAIR(btf_map_legacy, int, struct ipv_counts); ^ progs/test_btf_newkv.c:21:1: warning: type specifier missing, defaults to 'int' [-Wimplicit-int] PF_ANNOTATE_KV_PAIR(btf_map_legacy, int, struct ipv_counts); ^ 1 warning and 1 error generated. llc: error: llc: <stdin>:1:1: error: expected top-level entity clang failed ^ > > [1] https://lore.kernel.org/netdev/1541593725-27703-1-git-send-email-quentin.monnet@netronome.com/ > > Best regards, > Ilya ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 bpf-next] selftests/bpf: do not ignore clang failures 2019-07-09 18:14 ` Andrii Nakryiko @ 2019-07-10 13:25 ` Ilya Leoshkevich 2019-07-10 16:04 ` Andrii Nakryiko 0 siblings, 1 reply; 10+ messages in thread From: Ilya Leoshkevich @ 2019-07-10 13:25 UTC (permalink / raw) To: Andrii Nakryiko; +Cc: Daniel Borkmann, bpf, Song Liu, Quentin Monnet > Am 09.07.2019 um 20:14 schrieb Andrii Nakryiko <andrii.nakryiko@gmail.com>: > > On Mon, Jul 8, 2019 at 8:01 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote: >> >>> Am 05.07.2019 um 16:22 schrieb Daniel Borkmann <daniel@iogearbox.net>: >>> >>> On 07/01/2019 08:40 PM, Ilya Leoshkevich wrote: >>>> Am 01.07.2019 um 17:31 schrieb Andrii Nakryiko <andrii.nakryiko@gmail.com>: >>>>> Do we still need clang | llc pipeline with new clang? Could the same >>>>> be achieved with single clang invocation? That would solve the problem >>>>> of not detecting pipeline failures. >>>> >>>> I’ve experimented with this a little, and found that new clang: >>>> >>>> - Does not understand -march, but -target is sufficient. >>>> - Understands -mcpu. >>>> - Understands -Xclang -target-feature -Xclang +foo as a replacement for >>>> -mattr=foo. >>>> >>>> However, there are two issues with that: >>>> >>>> - Don’t older clangs need to be supported? For example, right now alu32 >>>> progs are built conditionally. >>> >>> We usually require latest clang to be able to test most recent features like >>> BTF such that it helps to catch potential bugs in either of the projects >>> before release. >>> >>>> - It does not seem to be possible to build test_xdp.o without -target >>>> bpf. >>> >>> For everything non-tracing, it does not make sense to invoke clang w/o >>> the -target bpf flag, see also Documentation/bpf/bpf_devel_QA.rst +573 >>> for more explanation, so this needs to be present for building test_xdp.o. >> >> I'm referring to the test introduced in [1]. test_xdp.o might not be an >> ideal target, but even if it's replaced with a more suitable one, the >> llc invocation would still be required. So I could redo the patch as >> follows: >> >> - Replace test_xdp.o with get_cgroup_id_kern.o, use an intermediate .bc >> file. >> - Use clang without llc for all other eBPF programs. >> - Split out Kbuild include and order-only prerequisites. >> >> What do you think? > > How about just forcing llc to fail as well like this: > > (clang <whatever> || echo "clain failed") | llc <whatever> > > While not pretty, it will get us what we need with very clear > messaging as well. E.g.: > > progs/test_btf_newkv.c:21:37: error: expected identifier > PF_ANNOTATE_KV_PAIR(btf_map_legacy, int, struct ipv_counts); > ^ > progs/test_btf_newkv.c:21:1: warning: type specifier missing, defaults > to 'int' [-Wimplicit-int] > PF_ANNOTATE_KV_PAIR(btf_map_legacy, int, struct ipv_counts); > ^ > 1 warning and 1 error generated. > llc: error: llc: <stdin>:1:1: error: expected top-level entity > clang failed > ^ While this would definitely work at least in my scenario, what about the following hypothetical cases? - clang manages to output something before exiting with nonzero rc - future llc version exits with zero rc when given "clang failed" or any other arbitrary string as an input (perhaps, with just a warning?) Come to think of it, what are the downsides of having intermediate bitcode files? While I did not run into this yet, I could imagine it might be even useful from time to time to inspect them. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 bpf-next] selftests/bpf: do not ignore clang failures 2019-07-10 13:25 ` Ilya Leoshkevich @ 2019-07-10 16:04 ` Andrii Nakryiko 0 siblings, 0 replies; 10+ messages in thread From: Andrii Nakryiko @ 2019-07-10 16:04 UTC (permalink / raw) To: Ilya Leoshkevich; +Cc: Daniel Borkmann, bpf, Song Liu, Quentin Monnet On Wed, Jul 10, 2019 at 6:25 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote: > > > Am 09.07.2019 um 20:14 schrieb Andrii Nakryiko <andrii.nakryiko@gmail.com>: > > > > On Mon, Jul 8, 2019 at 8:01 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote: > >> > >>> Am 05.07.2019 um 16:22 schrieb Daniel Borkmann <daniel@iogearbox.net>: > >>> > >>> On 07/01/2019 08:40 PM, Ilya Leoshkevich wrote: > >>>> Am 01.07.2019 um 17:31 schrieb Andrii Nakryiko <andrii.nakryiko@gmail.com>: > >>>>> Do we still need clang | llc pipeline with new clang? Could the same > >>>>> be achieved with single clang invocation? That would solve the problem > >>>>> of not detecting pipeline failures. > >>>> > >>>> I’ve experimented with this a little, and found that new clang: > >>>> > >>>> - Does not understand -march, but -target is sufficient. > >>>> - Understands -mcpu. > >>>> - Understands -Xclang -target-feature -Xclang +foo as a replacement for > >>>> -mattr=foo. > >>>> > >>>> However, there are two issues with that: > >>>> > >>>> - Don’t older clangs need to be supported? For example, right now alu32 > >>>> progs are built conditionally. > >>> > >>> We usually require latest clang to be able to test most recent features like > >>> BTF such that it helps to catch potential bugs in either of the projects > >>> before release. > >>> > >>>> - It does not seem to be possible to build test_xdp.o without -target > >>>> bpf. > >>> > >>> For everything non-tracing, it does not make sense to invoke clang w/o > >>> the -target bpf flag, see also Documentation/bpf/bpf_devel_QA.rst +573 > >>> for more explanation, so this needs to be present for building test_xdp.o. > >> > >> I'm referring to the test introduced in [1]. test_xdp.o might not be an > >> ideal target, but even if it's replaced with a more suitable one, the > >> llc invocation would still be required. So I could redo the patch as > >> follows: > >> > >> - Replace test_xdp.o with get_cgroup_id_kern.o, use an intermediate .bc > >> file. > >> - Use clang without llc for all other eBPF programs. > >> - Split out Kbuild include and order-only prerequisites. > >> > >> What do you think? > > > > How about just forcing llc to fail as well like this: > > > > (clang <whatever> || echo "clain failed") | llc <whatever> > > > > While not pretty, it will get us what we need with very clear > > messaging as well. E.g.: > > > > progs/test_btf_newkv.c:21:37: error: expected identifier > > PF_ANNOTATE_KV_PAIR(btf_map_legacy, int, struct ipv_counts); > > ^ > > progs/test_btf_newkv.c:21:1: warning: type specifier missing, defaults > > to 'int' [-Wimplicit-int] > > PF_ANNOTATE_KV_PAIR(btf_map_legacy, int, struct ipv_counts); > > ^ > > 1 warning and 1 error generated. > > llc: error: llc: <stdin>:1:1: error: expected top-level entity > > clang failed > > ^ > > While this would definitely work at least in my scenario, what about the > following hypothetical cases? > > - clang manages to output something before exiting with nonzero rc > - future llc version exits with zero rc when given "clang failed" or any > other arbitrary string as an input (perhaps, with just a warning?) This seems very far-fetched. While I can imagine partial output from clang, I can't imagine accepting some garbage as an input for llc and yet returning zero exit code. That would be major regression, so I wouldn't worry about it. > > Come to think of it, what are the downsides of having intermediate > bitcode files? While I did not run into this yet, I could imagine it > might be even useful from time to time to inspect them. Main downside is complication of Makefile, which is not the simplest to follow already. So if we can solve problem in simpler way without adding more complexity, that would be my preferred way. If someone wants bitcode, when you do `make`, you see all the commands that are being run, so just copy/paste parts you care about (i.e., clang invocation). That's what I do when I need to repro something for single .o file, for instance. > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-07-10 16:04 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-27 9:14 [PATCH bpf-next] selftests/bpf: do not ignore clang failures Ilya Leoshkevich 2019-06-28 20:35 ` Song Liu 2019-07-01 8:55 ` Ilya Leoshkevich 2019-07-01 15:31 ` Andrii Nakryiko 2019-07-01 18:40 ` [PATCH v2 " Ilya Leoshkevich 2019-07-05 14:22 ` Daniel Borkmann 2019-07-08 15:01 ` Ilya Leoshkevich 2019-07-09 18:14 ` Andrii Nakryiko 2019-07-10 13:25 ` Ilya Leoshkevich 2019-07-10 16:04 ` Andrii Nakryiko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).