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.
> >
> >> endif
> >>
> >> ifneq ($(CLANG_CPUV4),)
> >> --
> >> 2.30.2
> >>
> >>
