diff mbox series

[v2,bpf-next] selftests/bpf: do not ignore clang failures

Message ID 20190701184025.25731-1-iii@linux.ibm.com
State Changes Requested
Delegated to: BPF Maintainers
Headers show
Series [v2,bpf-next] selftests/bpf: do not ignore clang failures | expand

Commit Message

Ilya Leoshkevich July 1, 2019, 6:40 p.m. UTC 
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(-)

Comments

Daniel Borkmann July 5, 2019, 2:22 p.m. UTC | #1 
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
Ilya Leoshkevich July 8, 2019, 3:01 p.m. UTC | #2 
> 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
Andrii Nakryiko July 9, 2019, 6:14 p.m. UTC | #3 
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
Ilya Leoshkevich July 10, 2019, 1:25 p.m. UTC | #4 
> 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.
Andrii Nakryiko July 10, 2019, 4:04 p.m. UTC | #5 
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.

>
diff mbox series

Patch

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