Re: [PATCH] bpf: use -Wno-error in certain tests when building with GCC

["Jose E. Marchesi" <jose.marchesi@oracle.com>]

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