> On Tue, Jan 30, 2024 at 10:24 AM Jose E. Marchesi
> <jose.marchesi@xxxxxxxxxx> wrote:
>>
>>
>> > On Tue, Jan 30, 2024 at 6:32 AM Jose E. Marchesi
>> > <jose.marchesi@xxxxxxxxxx> 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@xxxxxxxxxx>
>> >> Cc: Yonghong Song <yhs@xxxxxxxx>
>> >> Cc: Eduard Zingerman <eddyz87@xxxxxxxxx>
>> >> Cc: David Faust <david.faust@xxxxxxxxxx>
>> >> Cc: Cupertino Miranda <cupertino.miranda@xxxxxxxxxx>
>> >> ---
>> >> 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
>> >>
>> >>
