Re: [PATCH bpf-next] bpf: use -Wno-address-of-packed-member when building with GCC

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

> On Tue, Jan 30, 2024 at 10:24 AM Jose E. Marchesi
> <jose.marchesi@oracle.com> wrote:
>>
>>
>> > On Tue, Jan 30, 2024 at 6:32 AM Jose E. Marchesi
>> > <jose.marchesi@oracle.com> wrote:
>> >>
>> >> GCC implements the -Wno-address-of-packed-member warning, which is
>> >> enabled by -Wall, that warns about taking the address of a packed
>> >> struct field when it can lead to an "unaligned" address.  Clang
>> >> doesn't support this warning.
>> >>
>> >> This triggers the following errors (-Werror) when building three
>> >> particular BPF selftests with GCC:
>> >>
>> >>   progs/test_cls_redirect.c
>> >>   986 |         if (ipv4_is_fragment((void *)&encap->ip)) {
>> >>   progs/test_cls_redirect_dynptr.c
>> >>   410 |         pkt_ipv4_checksum((void *)&encap_gre->ip);
>> >>   progs/test_cls_redirect.c
>> >>   521 |         pkt_ipv4_checksum((void *)&encap_gre->ip);
>> >>   progs/test_tc_tunnel.c
>> >>    232 |         set_ipv4_csum((void *)&h_outer.ip);
>> >>
>> >> These warnings do not signal any real problem in the tests as far as I
>> >> can see.
>> >>
>> >> This patch modifies selftests/bpf/Makefile to build these particular
>> >> selftests with -Wno-address-of-packed-member when bpf-gcc is used.
>> >> Note that we cannot use diagnostics pragmas (which are generally
>> >> preferred if I understood properly in a recent BPF office hours)
>> >> because Clang doesn't support these warnings.
>> >>
>> >> Tested in bpf-next master.
>> >> 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: David Faust <david.faust@oracle.com>
>> >> Cc: Cupertino Miranda <cupertino.miranda@oracle.com>
>> >> ---
>> >>  tools/testing/selftests/bpf/Makefile | 6 ++++++
>> >>  1 file changed, 6 insertions(+)
>> >>
>> >> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
>> >> index 1a3654bcb5dd..036473060bae 100644
>> >> --- a/tools/testing/selftests/bpf/Makefile
>> >> +++ b/tools/testing/selftests/bpf/Makefile
>> >> @@ -73,6 +73,12 @@ 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
>> >> +
>> >> +# The following selftests take the address of packed struct fields in
>> >> +# a way that can lead to unaligned addresses.  GCC warns about this.
>> >> +progs/test_cls_redirect.c-CFLAGS := -Wno-address-of-packed-member
>> >> +progs/test_cls_redirect_dynpr.c-CFLAGS := -Wno-address-of-packed-member
>> >> +progs/test_tc_tunnel.c-CFLAGS := -Wno-address-of-packed-member
>> >
>> > Why Makefile additions like these are preferable to just using #pragma
>> > in corresponding .c file? I understand there is no #pragma equivalent
>> > of -Wno-error, but these diagnostics do have #pragma equivalent,
>> > right?
>>
>> Not with this particular one, because Clang doesn't support
>> -W[no-]address-of-packed-member so it would lead to compilation error.
>>
>> Hence:
>>
>> >> Note that we cannot use diagnostics pragmas (which are generally
>> >> preferred if I understood properly in a recent BPF office hours)
>> >> because Clang doesn't support these warnings.
>>
>
> But can't we have
>
> #ifdef __gcc__
> #pragma ...
> #endif
>
>
> My main point of contention is that having those pragmas
> (conditionally) added in respective .c files makes it easier to be
> aware of them. While keeping them in Makefile is very opaque and we'll
> definitely forget about them, the only way to even notice them would
> be to run make V=1 and read very-very carefully.

Oh yeah that's certainly possible.  Since clang likes to pretend it is
other compilers, the guard would be:

#if !__clang__
#pragma GCC diagnostic ignored "-Waddress-of-packed-member"
#endif

Will send an updated patch.

FWIW I agree in that per-file pragmas are way better than Makefile
flags.

>
>
>> >
>> >>  endif
>> >>
>> >>  ifneq ($(CLANG_CPUV4),)
>> >> --
>> >> 2.30.2
>> >>
>> >>