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

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

[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

Re: [PATCH bpf-next] selftests/bpf: do not ignore clang failures

[Song Liu]

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

Re: [PATCH bpf-next] selftests/bpf: do not ignore clang failures

[Ilya Leoshkevich]

> 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

Re: [PATCH bpf-next] selftests/bpf: do not ignore clang failures

[Andrii Nakryiko]

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

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

[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

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

[Daniel Borkmann]

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

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

[Ilya Leoshkevich]

> 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

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

[Andrii Nakryiko]

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

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

[Ilya Leoshkevich]

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

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

[Andrii Nakryiko]

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.

>