[v3,bpf] selftests/bpf: do not ignore clang failures
diff mbox series

Message ID 20190711091249.59865-1-iii@linux.ibm.com
State Accepted
Delegated to: BPF Maintainers
Headers show
Series
  • [v3,bpf] selftests/bpf: do not ignore clang failures
Related show

Commit Message

Ilya Leoshkevich July 11, 2019, 9:12 a.m. UTC
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.

When clang fails, pipe the string "clang failed" to llc. This will make
llc fail with an informative error message. This solution was chosen
over using pipefail, having separate targets or getting rid of llc
invocation due to its simplicity.

In addition, pull Kbuild.include in order to get .DELETE_ON_ERROR target,
which would cause partial .o files to be removed.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
v1->v2: use intermediate targets instead of pipefail
v2->v3: pipe "clang failed" instead of using intermediate targets

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

Comments

Andrii Nakryiko July 11, 2019, 2:55 p.m. UTC | #1
On Thu, Jul 11, 2019 at 2:14 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.
>
> When clang fails, pipe the string "clang failed" to llc. This will make
> llc fail with an informative error message. This solution was chosen
> over using pipefail, having separate targets or getting rid of llc
> invocation due to its simplicity.
>
> In addition, pull Kbuild.include in order to get .DELETE_ON_ERROR target,

In your original patch you explicitly declared .DELETE_ON_ERROR, but
in this one you just include Kbuild.include.
Is it enough to just include that file to get desired behavior or your
forgot to add .DELETE_ON_ERROR?

> which would cause partial .o files to be removed.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---

Thanks!

Acked-by: Andrii Nakryiko <andriin@fb.com>

> v1->v2: use intermediate targets instead of pipefail
> v2->v3: pipe "clang failed" instead of using intermediate targets
>
> tools/testing/selftests/bpf/Makefile | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index e36356e2377e..e375f399b7a6 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
> @@ -185,8 +186,8 @@ $(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) \
> -                -O2 -target bpf -emit-llvm -c $< -o - |      \
> +       ($(CLANG) $(CLANG_FLAGS) -O2 -target bpf -emit-llvm -c $< -o - || \
> +               echo "clang failed") | \
>         $(LLC) -march=bpf -mattr=+alu32 -mcpu=$(CPU) $(LLC_FLAGS) \
>                 -filetype=obj -o $@
>  ifeq ($(DWARF2BTF),y)
> @@ -197,16 +198,16 @@ 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) \
> -               -O2 -emit-llvm -c $< -o - | \
> +       ($(CLANG) $(CLANG_FLAGS) -O2 -emit-llvm -c $< -o - || \
> +               echo "clang failed") | \
>         $(LLC) -march=bpf -mcpu=$(CPU) $(LLC_FLAGS) -filetype=obj -o $@
>  ifeq ($(DWARF2BTF),y)
>         $(BTF_PAHOLE) -J $@
>  endif
>
>  $(OUTPUT)/%.o: progs/%.c
> -       $(CLANG) $(CLANG_FLAGS) \
> -                -O2 -target bpf -emit-llvm -c $< -o - |      \
> +       ($(CLANG) $(CLANG_FLAGS) -O2 -target bpf -emit-llvm -c $< -o - || \
> +               echo "clang failed") | \
>         $(LLC) -march=bpf -mcpu=$(CPU) $(LLC_FLAGS) -filetype=obj -o $@
>  ifeq ($(DWARF2BTF),y)
>         $(BTF_PAHOLE) -J $@
> --
> 2.21.0
>
Ilya Leoshkevich July 11, 2019, 3 p.m. UTC | #2
> Am 11.07.2019 um 16:55 schrieb Andrii Nakryiko <andrii.nakryiko@gmail.com>:
> 
> On Thu, Jul 11, 2019 at 2:14 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>> 
>> 
>> In addition, pull Kbuild.include in order to get .DELETE_ON_ERROR target,
> 
> In your original patch you explicitly declared .DELETE_ON_ERROR, but
> in this one you just include Kbuild.include.
> Is it enough to just include that file to get desired behavior or your
> forgot to add .DELETE_ON_ERROR?

It’s enough to just include Kbuild.include. I grepped the source tree
and found that no one else declares .DELETE_ON_ERROR explicitly, so I've
decided to avoid doing this as well.
Daniel Borkmann July 12, 2019, 1:42 p.m. UTC | #3
On 07/11/2019 11:12 AM, Ilya Leoshkevich 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.
> 
> When clang fails, pipe the string "clang failed" to llc. This will make
> llc fail with an informative error message. This solution was chosen
> over using pipefail, having separate targets or getting rid of llc
> invocation due to its simplicity.
> 
> In addition, pull Kbuild.include in order to get .DELETE_ON_ERROR target,
> which would cause partial .o files to be removed.
> 
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>

Applied, thanks!

Patch
diff mbox series

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index e36356e2377e..e375f399b7a6 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
@@ -185,8 +186,8 @@  $(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) \
-		 -O2 -target bpf -emit-llvm -c $< -o - |      \
+	($(CLANG) $(CLANG_FLAGS) -O2 -target bpf -emit-llvm -c $< -o - || \
+		echo "clang failed") | \
 	$(LLC) -march=bpf -mattr=+alu32 -mcpu=$(CPU) $(LLC_FLAGS) \
 		-filetype=obj -o $@
 ifeq ($(DWARF2BTF),y)
@@ -197,16 +198,16 @@  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) \
-		-O2 -emit-llvm -c $< -o - | \
+	($(CLANG) $(CLANG_FLAGS) -O2 -emit-llvm -c $< -o - || \
+		echo "clang failed") | \
 	$(LLC) -march=bpf -mcpu=$(CPU) $(LLC_FLAGS) -filetype=obj -o $@
 ifeq ($(DWARF2BTF),y)
 	$(BTF_PAHOLE) -J $@
 endif
 
 $(OUTPUT)/%.o: progs/%.c
-	$(CLANG) $(CLANG_FLAGS) \
-		 -O2 -target bpf -emit-llvm -c $< -o - |      \
+	($(CLANG) $(CLANG_FLAGS) -O2 -target bpf -emit-llvm -c $< -o - || \
+		echo "clang failed") | \
 	$(LLC) -march=bpf -mcpu=$(CPU) $(LLC_FLAGS) -filetype=obj -o $@
 ifeq ($(DWARF2BTF),y)
 	$(BTF_PAHOLE) -J $@