diff mbox series

bpf: use -Wno-error in certain tests when building with GCC

Message ID 20240126185059.4376-1-jose.marchesi@oracle.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series bpf: use -Wno-error in certain tests when building with GCC | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-14 fail Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-PR fail PR summary
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc

Commit Message

Jose E. Marchesi Jan. 26, 2024, 6:50 p.m. UTC 
Certain BPF selftests contain code that, albeit being legal C, trigger
warnings in GCC that cannot be disabled.  This is the case for example
for the tests

  progs/btf_dump_test_case_bitfields.c
  progs/btf_dump_test_case_namespacing.c
  progs/btf_dump_test_case_packing.c
  progs/btf_dump_test_case_padding.c
  progs/btf_dump_test_case_syntax.c

which contain struct type declarations inside function parameter
lists.  This is problematic, because:

- The BPF selftests are built with -Werror.

- The Clang and GCC compilers sometimes differ when it comes to handle
  warnings.  in the handling of warnings.  One compiler may emit
  warnings for code that the other compiles compiles silently, and one
  compiler may offer the possibility to disable certain warnings, while
  the other doesn't.

In order to overcome this problem, this patch modifies the
tools/testing/selftests/bpf/Makefile in order to:

1. Enable the possibility of specifing per-source-file extra CFLAGS.
   This is done by defining a make variable like:

   <source-filename>-CFLAGS := <whateverflags>

   And then modifying the proper Make rule in order to use these flags
   when compiling <source-filename>.

2. Use the mechanism above to add -Wno-error to CFLAGS for the
   following selftests:

   progs/btf_dump_test_case_bitfields.c
   progs/btf_dump_test_case_namespacing.c
   progs/btf_dump_test_case_packing.c
   progs/btf_dump_test_case_padding.c
   progs/btf_dump_test_case_syntax.c

   Note the corresponding -CFLAGS variables for these files are
   defined only if the selftests are being built with GCC.

Note that, while compiler pragmas can generally be used to disable
particular warnings per file, this 1) is only possible for warning
that actually can be disabled in the command line, i.e. that have
-Wno-FOO options, and 2) doesn't apply to -Wno-error.

Tested in bpf-next master branch.
No regressions.

Signed-off-by: Jose E. Marchesi <jose.marchesi@oracle.com>
Cc: Yonghong Song <yhs@meta.com>
Cc: Eduard Zingerman <eddyz87@gmail.com>
Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: david.faust@oracle.com
Cc: cupertino.miranda@oracle.com
---
 tools/testing/selftests/bpf/Makefile | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

Comments

Andrii Nakryiko Jan. 26, 2024, 11:07 p.m. UTC | #1 
On Fri, Jan 26, 2024 at 10:51 AM Jose E. Marchesi
<jose.marchesi@oracle.com> wrote:
>
> Certain BPF selftests contain code that, albeit being legal C, trigger
> warnings in GCC that cannot be disabled.  This is the case for example
> for the tests
>
>   progs/btf_dump_test_case_bitfields.c
>   progs/btf_dump_test_case_namespacing.c
>   progs/btf_dump_test_case_packing.c
>   progs/btf_dump_test_case_padding.c
>   progs/btf_dump_test_case_syntax.c
>
> which contain struct type declarations inside function parameter
> lists.  This is problematic, because:
>
> - The BPF selftests are built with -Werror.
>
> - The Clang and GCC compilers sometimes differ when it comes to handle
>   warnings.  in the handling of warnings.  One compiler may emit
>   warnings for code that the other compiles compiles silently, and one
>   compiler may offer the possibility to disable certain warnings, while
>   the other doesn't.
>
> In order to overcome this problem, this patch modifies the
> tools/testing/selftests/bpf/Makefile in order to:
>
> 1. Enable the possibility of specifing per-source-file extra CFLAGS.
>    This is done by defining a make variable like:
>
>    <source-filename>-CFLAGS := <whateverflags>
>
>    And then modifying the proper Make rule in order to use these flags
>    when compiling <source-filename>.
>
> 2. Use the mechanism above to add -Wno-error to CFLAGS for the
>    following selftests:
>
>    progs/btf_dump_test_case_bitfields.c
>    progs/btf_dump_test_case_namespacing.c
>    progs/btf_dump_test_case_packing.c
>    progs/btf_dump_test_case_padding.c
>    progs/btf_dump_test_case_syntax.c
>
>    Note the corresponding -CFLAGS variables for these files are
>    defined only if the selftests are being built with GCC.
>
> Note that, while compiler pragmas can generally be used to disable
> particular warnings per file, this 1) is only possible for warning
> that actually can be disabled in the command line, i.e. that have
> -Wno-FOO options, and 2) doesn't apply to -Wno-error.
>
> Tested in bpf-next master branch.
> No regressions.
>
> Signed-off-by: Jose E. Marchesi <jose.marchesi@oracle.com>
> Cc: Yonghong Song <yhs@meta.com>
> Cc: Eduard Zingerman <eddyz87@gmail.com>
> Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> Cc: david.faust@oracle.com
> Cc: cupertino.miranda@oracle.com
> ---
>  tools/testing/selftests/bpf/Makefile | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index fd15017ed3b1..8c4282766976 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -64,6 +64,15 @@ TEST_INST_SUBDIRS := no_alu32
>  ifneq ($(BPF_GCC),)
>  TEST_GEN_PROGS += test_progs-bpf_gcc
>  TEST_INST_SUBDIRS += bpf_gcc
> +
> +# The following tests contain C code that, although technically legal,
> +# triggers GCC warnings that cannot be disabled: declaration of
> +# anonymous struct types in function parameter lists.
> +progs/btf_dump_test_case_bitfields.c-CFLAGS := -Wno-error
> +progs/btf_dump_test_case_namespacing.c-CFLAGS := -Wno-error
> +progs/btf_dump_test_case_packing.c-CFLAGS := -Wno-error
> +progs/btf_dump_test_case_padding.c-CFLAGS := -Wno-error
> +progs/btf_dump_test_case_syntax.c-CFLAGS := -Wno-error
>  endif
>
>  ifneq ($(CLANG_CPUV4),)
> @@ -504,7 +513,8 @@ $(TRUNNER_BPF_OBJS): $(TRUNNER_OUTPUT)/%.bpf.o:                             \
>                      $(wildcard $(BPFDIR)/*.bpf.h)                      \
>                      | $(TRUNNER_OUTPUT) $$(BPFOBJ)
>         $$(call $(TRUNNER_BPF_BUILD_RULE),$$<,$$@,                      \
> -                                         $(TRUNNER_BPF_CFLAGS))
> +                                         $(TRUNNER_BPF_CFLAGS)         \
> +                                         $$(if $$($$<-CFLAGS),$$($$<-CFLAGS)))

minor nit, but do you even need the $$(if)? why not just
unconditionally use $$($$<-CFLAGS) which should result in an empty
string, right? Or is there some make weirdness that I'm forgetting?

>
>  $(TRUNNER_BPF_SKELS): %.skel.h: %.bpf.o $(BPFTOOL) | $(TRUNNER_OUTPUT)
>         $$(call msg,GEN-SKEL,$(TRUNNER_BINARY),$$@)
> --
> 2.30.2
>
Jose E. Marchesi Jan. 27, 2024, 10:09 a.m. UTC | #2 
> On Fri, Jan 26, 2024 at 10:51 AM Jose E. Marchesi
> <jose.marchesi@oracle.com> wrote:
>>
>> Certain BPF selftests contain code that, albeit being legal C, trigger
>> warnings in GCC that cannot be disabled.  This is the case for example
>> for the tests
>>
>>   progs/btf_dump_test_case_bitfields.c
>>   progs/btf_dump_test_case_namespacing.c
>>   progs/btf_dump_test_case_packing.c
>>   progs/btf_dump_test_case_padding.c
>>   progs/btf_dump_test_case_syntax.c
>>
>> which contain struct type declarations inside function parameter
>> lists.  This is problematic, because:
>>
>> - The BPF selftests are built with -Werror.
>>
>> - The Clang and GCC compilers sometimes differ when it comes to handle
>>   warnings.  in the handling of warnings.  One compiler may emit
>>   warnings for code that the other compiles compiles silently, and one
>>   compiler may offer the possibility to disable certain warnings, while
>>   the other doesn't.
>>
>> In order to overcome this problem, this patch modifies the
>> tools/testing/selftests/bpf/Makefile in order to:
>>
>> 1. Enable the possibility of specifing per-source-file extra CFLAGS.
>>    This is done by defining a make variable like:
>>
>>    <source-filename>-CFLAGS := <whateverflags>
>>
>>    And then modifying the proper Make rule in order to use these flags
>>    when compiling <source-filename>.
>>
>> 2. Use the mechanism above to add -Wno-error to CFLAGS for the
>>    following selftests:
>>
>>    progs/btf_dump_test_case_bitfields.c
>>    progs/btf_dump_test_case_namespacing.c
>>    progs/btf_dump_test_case_packing.c
>>    progs/btf_dump_test_case_padding.c
>>    progs/btf_dump_test_case_syntax.c
>>
>>    Note the corresponding -CFLAGS variables for these files are
>>    defined only if the selftests are being built with GCC.
>>
>> Note that, while compiler pragmas can generally be used to disable
>> particular warnings per file, this 1) is only possible for warning
>> that actually can be disabled in the command line, i.e. that have
>> -Wno-FOO options, and 2) doesn't apply to -Wno-error.
>>
>> Tested in bpf-next master branch.
>> No regressions.
>>
>> Signed-off-by: Jose E. Marchesi <jose.marchesi@oracle.com>
>> Cc: Yonghong Song <yhs@meta.com>
>> Cc: Eduard Zingerman <eddyz87@gmail.com>
>> Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
>> Cc: david.faust@oracle.com
>> Cc: cupertino.miranda@oracle.com
>> ---
>>  tools/testing/selftests/bpf/Makefile | 12 +++++++++++-
>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
>> index fd15017ed3b1..8c4282766976 100644
>> --- a/tools/testing/selftests/bpf/Makefile
>> +++ b/tools/testing/selftests/bpf/Makefile
>> @@ -64,6 +64,15 @@ TEST_INST_SUBDIRS := no_alu32
>>  ifneq ($(BPF_GCC),)
>>  TEST_GEN_PROGS += test_progs-bpf_gcc
>>  TEST_INST_SUBDIRS += bpf_gcc
>> +
>> +# The following tests contain C code that, although technically legal,
>> +# triggers GCC warnings that cannot be disabled: declaration of
>> +# anonymous struct types in function parameter lists.
>> +progs/btf_dump_test_case_bitfields.c-CFLAGS := -Wno-error
>> +progs/btf_dump_test_case_namespacing.c-CFLAGS := -Wno-error
>> +progs/btf_dump_test_case_packing.c-CFLAGS := -Wno-error
>> +progs/btf_dump_test_case_padding.c-CFLAGS := -Wno-error
>> +progs/btf_dump_test_case_syntax.c-CFLAGS := -Wno-error
>>  endif
>>
>>  ifneq ($(CLANG_CPUV4),)
>> @@ -504,7 +513,8 @@ $(TRUNNER_BPF_OBJS): $(TRUNNER_OUTPUT)/%.bpf.o:                             \
>>                      $(wildcard $(BPFDIR)/*.bpf.h)                      \
>>                      | $(TRUNNER_OUTPUT) $$(BPFOBJ)
>>         $$(call $(TRUNNER_BPF_BUILD_RULE),$$<,$$@,                      \
>> -                                         $(TRUNNER_BPF_CFLAGS))
>> +                                         $(TRUNNER_BPF_CFLAGS)         \
>> +                                         $$(if $$($$<-CFLAGS),$$($$<-CFLAGS)))
>
> minor nit, but do you even need the $$(if)? why not just
> unconditionally use $$($$<-CFLAGS) which should result in an empty
> string, right? Or is there some make weirdness that I'm forgetting?

Indeed.  Good catch.  The $(if ...) was a vestigious of some testing.
Just sent V2.

>>
>>  $(TRUNNER_BPF_SKELS): %.skel.h: %.bpf.o $(BPFTOOL) | $(TRUNNER_OUTPUT)
>>         $$(call msg,GEN-SKEL,$(TRUNNER_BINARY),$$@)
>> --
>> 2.30.2
>>
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index fd15017ed3b1..8c4282766976 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -64,6 +64,15 @@  TEST_INST_SUBDIRS := no_alu32
 ifneq ($(BPF_GCC),)
 TEST_GEN_PROGS += test_progs-bpf_gcc
 TEST_INST_SUBDIRS += bpf_gcc
+
+# The following tests contain C code that, although technically legal,
+# triggers GCC warnings that cannot be disabled: declaration of
+# anonymous struct types in function parameter lists.
+progs/btf_dump_test_case_bitfields.c-CFLAGS := -Wno-error
+progs/btf_dump_test_case_namespacing.c-CFLAGS := -Wno-error
+progs/btf_dump_test_case_packing.c-CFLAGS := -Wno-error
+progs/btf_dump_test_case_padding.c-CFLAGS := -Wno-error
+progs/btf_dump_test_case_syntax.c-CFLAGS := -Wno-error
 endif
 
 ifneq ($(CLANG_CPUV4),)
@@ -504,7 +513,8 @@  $(TRUNNER_BPF_OBJS): $(TRUNNER_OUTPUT)/%.bpf.o:				\
 		     $(wildcard $(BPFDIR)/*.bpf.h)			\
 		     | $(TRUNNER_OUTPUT) $$(BPFOBJ)
 	$$(call $(TRUNNER_BPF_BUILD_RULE),$$<,$$@,			\
-					  $(TRUNNER_BPF_CFLAGS))
+					  $(TRUNNER_BPF_CFLAGS)         \
+					  $$(if $$($$<-CFLAGS),$$($$<-CFLAGS)))
 
 $(TRUNNER_BPF_SKELS): %.skel.h: %.bpf.o $(BPFTOOL) | $(TRUNNER_OUTPUT)
 	$$(call msg,GEN-SKEL,$(TRUNNER_BINARY),$$@)