selftests/bpf: do not ignore clang failures

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 <[email protected]>

---
 tools/testing/selftests/bpf/Makefile | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

tools/testing/selftests/bpf/Makefile +0 -0

index f2dbe2043067..2316fa2d5b3b 100644

Am 28.06.2019 um 22:35 schrieb Song Liu <[email protected]>:

tools/testing/selftests/bpf/Makefile line 3

Quoted 9 lines · Ilya Leoshkevich

tools/testing/selftests/bpf/Makefile +1 -0

index f2dbe2043067..2316fa2d5b3b 100644
@@ -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

Quoted 17 lines

Am 28.06.2019 um 22:35 schrieb Song Liu <[email protected]>:

tools/testing/selftests/bpf/Makefile line 3

Quoted 9 lines · Ilya Leoshkevich

tools/testing/selftests/bpf/Makefile +1 -0

index f2dbe2043067..2316fa2d5b3b 100644
@@ -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/.

Quoted 4 lines

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

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.

Am 01.07.2019 um 17:31 schrieb Andrii Nakryiko <[email protected]>:

Quoted 3 lines

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.

Am 05.07.2019 um 16:22 schrieb Daniel Borkmann <[email protected]>:

Quoted 12 lines · Ilya Leoshkevich

Am 01.07.2019 um 17:31 schrieb Andrii Nakryiko <[email protected]>:

Quoted 3 lines

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.

Quoted 4 lines

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.

Quoted 2 lines

- 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.

Quoted 25 lines

Am 05.07.2019 um 16:22 schrieb Daniel Borkmann <[email protected]>:

Quoted 12 lines · Ilya Leoshkevich

Am 01.07.2019 um 17:31 schrieb Andrii Nakryiko <[email protected]>:

Quoted 3 lines

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.

Quoted 4 lines

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.

Quoted 6 lines

Quoted 2 lines

- 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/[email protected]/

Best regards,
Ilya

Am 09.07.2019 um 20:14 schrieb Andrii Nakryiko <[email protected]>:

Quoted 37 lines · Ilya Leoshkevich

Quoted 25 lines

Am 05.07.2019 um 16:22 schrieb Daniel Borkmann <[email protected]>:

Quoted 12 lines · Ilya Leoshkevich

Am 01.07.2019 um 17:31 schrieb Andrii Nakryiko <[email protected]>:

Quoted 3 lines

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.

Quoted 4 lines

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.

Quoted 6 lines

Quoted 2 lines

- 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:

Quoted 6 lines

- 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
^

Quoted 58 lines

Am 09.07.2019 um 20:14 schrieb Andrii Nakryiko <[email protected]>:

Quoted 37 lines · Ilya Leoshkevich

Quoted 25 lines

Am 05.07.2019 um 16:22 schrieb Daniel Borkmann <[email protected]>:

Quoted 12 lines · Ilya Leoshkevich

Am 01.07.2019 um 17:31 schrieb Andrii Nakryiko <[email protected]>:

Quoted 3 lines

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.

Quoted 4 lines

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.

Quoted 6 lines

Quoted 2 lines

- 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:

Quoted 6 lines

- 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);
                                   ^

Quoted 8 lines

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.