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

Commit Message

Jose E. Marchesi Jan. 30, 2024, 2:32 p.m. UTC

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 <[e-mail]>
Cc: Yonghong Song <[e-mail]>
Cc: Eduard Zingerman <[e-mail]>
Cc: David Faust <[e-mail]>
Cc: Cupertino Miranda <[e-mail]>
---
 tools/testing/selftests/bpf/Makefile | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Andrii Nakryiko Jan. 30, 2024, 6:14 p.m. UTC | #1

On Tue, Jan 30, 2024 at 6:32 AM Jose E. Marchesi
<[e-mail]> 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 <[e-mail]>
> Cc: Yonghong Song <[e-mail]>
> Cc: Eduard Zingerman <[e-mail]>
> Cc: David Faust <[e-mail]>
> Cc: Cupertino Miranda <[e-mail]>
> ---
>  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?

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

Jose E. Marchesi Jan. 30, 2024, 6:24 p.m. UTC | #2

> On Tue, Jan 30, 2024 at 6:32 AM Jose E. Marchesi
> <[e-mail]> 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 <[e-mail]>
>> Cc: Yonghong Song <[e-mail]>
>> Cc: Eduard Zingerman <[e-mail]>
>> Cc: David Faust <[e-mail]>
>> Cc: Cupertino Miranda <[e-mail]>
>> ---
>>  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.

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

Andrii Nakryiko Jan. 30, 2024, 7:26 p.m. UTC | #3

On Tue, Jan 30, 2024 at 10:24 AM Jose E. Marchesi
<[e-mail]> wrote:
>
>
> > On Tue, Jan 30, 2024 at 6:32 AM Jose E. Marchesi
> > <[e-mail]> 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 <[e-mail]>
> >> Cc: Yonghong Song <[e-mail]>
> >> Cc: Eduard Zingerman <[e-mail]>
> >> Cc: David Faust <[e-mail]>
> >> Cc: Cupertino Miranda <[e-mail]>
> >> ---
> >>  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
> >>
> >>

Jose E. Marchesi Jan. 30, 2024, 7:45 p.m. UTC | #4

> On Tue, Jan 30, 2024 at 10:24 AM Jose E. Marchesi
> <[e-mail]> wrote:
>>
>>
>> > On Tue, Jan 30, 2024 at 6:32 AM Jose E. Marchesi
>> > <[e-mail]> 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 <[e-mail]>
>> >> Cc: Yonghong Song <[e-mail]>
>> >> Cc: Eduard Zingerman <[e-mail]>
>> >> Cc: David Faust <[e-mail]>
>> >> Cc: Cupertino Miranda <[e-mail]>
>> >> ---
>> >>  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
>> >>
>> >>

Patch

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