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

Commit Message

Jose E. Marchesi Jan. 31, 2024, 9:44 a.m. UTC

[Differences from V1:
- Now pragmas are used in testfiles instead of flags
  in Makefile.]

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 adds pragmas to these test files that inhibit the
-Waddress-of-packed-member if the compiler is not Clang.

Tested in bpf-next master.
No regressions.

Signed-off-by: Jose E. Marchesi <[e-mail]>
Cc: Andrii Nakryiko <[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/progs/test_cls_redirect.c        | 4 ++++
 tools/testing/selftests/bpf/progs/test_cls_redirect_dynptr.c | 4 ++++
 tools/testing/selftests/bpf/progs/test_tc_tunnel.c           | 4 ++++
 3 files changed, 12 insertions(+)

Comments

Eduard Zingerman Jan. 31, 2024, 3:57 p.m. UTC | #1

On Wed, 2024-01-31 at 10:44 +0100, Jose E. Marchesi wrote:
> [Differences from V1:
> - Now pragmas are used in testfiles instead of flags
>   in Makefile.]
> 
> 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 adds pragmas to these test files that inhibit the
> -Waddress-of-packed-member if the compiler is not Clang.
> 
> Tested in bpf-next master.
> No regressions.
> 
> Signed-off-by: Jose E. Marchesi <[e-mail]>
> Cc: Andrii Nakryiko <[e-mail]>
> Cc: Yonghong Song <[e-mail]>
> Cc: Eduard Zingerman <[e-mail]>
> Cc: David Faust <[e-mail]>
> Cc: Cupertino Miranda <[e-mail]>
> ---

Acked-by: Eduard Zingerman <[e-mail]>

[...]

Yonghong Song Jan. 31, 2024, 6:33 p.m. UTC | #2

On 1/31/24 1:44 AM, Jose E. Marchesi wrote:
> [Differences from V1:
> - Now pragmas are used in testfiles instead of flags
>    in Makefile.]
>
> 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.

Look like this is not true.

$ cat t.c
struct __attribute__ ((packed)) Packed {
   char a;
   int b;
   int c;
   char d;
};

void test(const int *i, int *ptr);
int foo() {
   struct Packed p;
   p.c = 1;
   test(&p.c, &p.c);
   return 0;
}
$ /home/yhs/work/llvm-project/llvm/build.16/install/bin/clang --version
clang version 16.0.3 (https://github.com/llvm/llvm-project.git da3cd333bea572fb10470f610a27f22bcb84b08c)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /home/yhs/work/llvm-project/llvm/build.16/install/bin
$ /home/yhs/work/llvm-project/llvm/build.16/install/bin/clang --target=bpf -O2 -c t.c
t.c:12:9: warning: taking address of packed member 'c' of class or structure 'Packed' may result in an unaligned pointer value [-Waddress-of-packed-member]
   test(&p.c, &p.c);
         ^~~
t.c:12:15: warning: taking address of packed member 'c' of class or structure 'Packed' may result in an unaligned pointer value [-Waddress-of-packed-member]
   test(&p.c, &p.c);
               ^~~
2 warnings generated.
$ /home/yhs/work/llvm-project/llvm/build.16/install/bin/clang --target=bpf -O2 -c t.c -Wno-address-of-packed-member
$

But each compiler internal diag detection logic could be different, so
it is totally possible that gcc might emit warning while clang does not
like in some selftests mentioned.

>
> 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 adds pragmas to these test files that inhibit the
> -Waddress-of-packed-member if the compiler is not Clang.
>
> Tested in bpf-next master.
> No regressions.
>
> Signed-off-by: Jose E. Marchesi <[e-mail]>
> Cc: Andrii Nakryiko <[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/progs/test_cls_redirect.c        | 4 ++++
>   tools/testing/selftests/bpf/progs/test_cls_redirect_dynptr.c | 4 ++++
>   tools/testing/selftests/bpf/progs/test_tc_tunnel.c           | 4 ++++
>   3 files changed, 12 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/progs/test_cls_redirect.c b/tools/testing/selftests/bpf/progs/test_cls_redirect.c
> index 66b304982245..23e950ad84d2 100644
> --- a/tools/testing/selftests/bpf/progs/test_cls_redirect.c
> +++ b/tools/testing/selftests/bpf/progs/test_cls_redirect.c
> @@ -22,6 +22,10 @@
>   
>   #include "test_cls_redirect.h"
>   
> +#if !__clang__
> +#pragma GCC diagnostic ignored "-Waddress-of-packed-member"
> +#endif

So I suggest to remove the above '#if !__clang__' guard.

> +
>   #ifdef SUBPROGS
>   #define INLINING __noinline
>   #else
[...]

Jose E. Marchesi Feb. 1, 2024, 11:10 a.m. UTC | #3

> On 1/31/24 1:44 AM, Jose E. Marchesi wrote:
>> [Differences from V1:
>> - Now pragmas are used in testfiles instead of flags
>>    in Makefile.]
>>
>> 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.
>
> Look like this is not true.
>
> $ cat t.c
> struct __attribute__ ((packed)) Packed {
>   char a;
>   int b;
>   int c;
>   char d;
> };
>
> void test(const int *i, int *ptr);
> int foo() {
>   struct Packed p;
>   p.c = 1;
>   test(&p.c, &p.c);
>   return 0;
> }
> $ /home/yhs/work/llvm-project/llvm/build.16/install/bin/clang --version
> clang version 16.0.3 (https://github.com/llvm/llvm-project.git da3cd333bea572fb10470f610a27f22bcb84b08c)
> Target: x86_64-unknown-linux-gnu
> Thread model: posix
> InstalledDir: /home/yhs/work/llvm-project/llvm/build.16/install/bin
> $ /home/yhs/work/llvm-project/llvm/build.16/install/bin/clang --target=bpf -O2 -c t.c
> t.c:12:9: warning: taking address of packed member 'c' of class or structure 'Packed' may result in an unaligned pointer value [-Waddress-of-packed-member]
>   test(&p.c, &p.c);
>         ^~~
> t.c:12:15: warning: taking address of packed member 'c' of class or structure 'Packed' may result in an unaligned pointer value [-Waddress-of-packed-member]
>   test(&p.c, &p.c);
>               ^~~
> 2 warnings generated.
> $ /home/yhs/work/llvm-project/llvm/build.16/install/bin/clang --target=bpf -O2 -c t.c -Wno-address-of-packed-member
> $

Hm I can actually confirm my local clang (18.0.0
586986a063ee4b9a7490aac102e103bab121c764) does support the warning.

I should have been confused by some other warning.  I'm dealing with
many of them.  Sorry about that.

I will submit a patch to remove the !__clang__ conditionals.

> But each compiler internal diag detection logic could be different, so
> it is totally possible that gcc might emit warning while clang does not
> like in some selftests mentioned.
>
>>
>> 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 adds pragmas to these test files that inhibit the
>> -Waddress-of-packed-member if the compiler is not Clang.
>>
>> Tested in bpf-next master.
>> No regressions.
>>
>> Signed-off-by: Jose E. Marchesi <[e-mail]>
>> Cc: Andrii Nakryiko <[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/progs/test_cls_redirect.c        | 4 ++++
>>   tools/testing/selftests/bpf/progs/test_cls_redirect_dynptr.c | 4 ++++
>>   tools/testing/selftests/bpf/progs/test_tc_tunnel.c           | 4 ++++
>>   3 files changed, 12 insertions(+)
>>
>> diff --git a/tools/testing/selftests/bpf/progs/test_cls_redirect.c b/tools/testing/selftests/bpf/progs/test_cls_redirect.c
>> index 66b304982245..23e950ad84d2 100644
>> --- a/tools/testing/selftests/bpf/progs/test_cls_redirect.c
>> +++ b/tools/testing/selftests/bpf/progs/test_cls_redirect.c
>> @@ -22,6 +22,10 @@
>>     #include "test_cls_redirect.h"
>>   +#if !__clang__
>> +#pragma GCC diagnostic ignored "-Waddress-of-packed-member"
>> +#endif
>
> So I suggest to remove the above '#if !__clang__' guard.
>
>> +
>>   #ifdef SUBPROGS
>>   #define INLINING __noinline
>>   #else
> [...]

Patch

diff --git a/tools/testing/selftests/bpf/progs/test_cls_redirect.c b/tools/testing/selftests/bpf/progs/test_cls_redirect.c
index 66b304982245..23e950ad84d2 100644
--- a/tools/testing/selftests/bpf/progs/test_cls_redirect.c
+++ b/tools/testing/selftests/bpf/progs/test_cls_redirect.c
@@ -22,6 +22,10 @@ 
 
 #include "test_cls_redirect.h"
 
+#if !__clang__
+#pragma GCC diagnostic ignored "-Waddress-of-packed-member"
+#endif
+
 #ifdef SUBPROGS
 #define INLINING __noinline
 #else
diff --git a/tools/testing/selftests/bpf/progs/test_cls_redirect_dynptr.c b/tools/testing/selftests/bpf/progs/test_cls_redirect_dynptr.c
index f41c81212ee9..af280e197c55 100644
--- a/tools/testing/selftests/bpf/progs/test_cls_redirect_dynptr.c
+++ b/tools/testing/selftests/bpf/progs/test_cls_redirect_dynptr.c
@@ -23,6 +23,10 @@ 
 #include "test_cls_redirect.h"
 #include "bpf_kfuncs.h"
 
+#if !__clang__
+#pragma GCC diagnostic ignored "-Waddress-of-packed-member"
+#endif
+
 #define offsetofend(TYPE, MEMBER) \
 	(offsetof(TYPE, MEMBER) + sizeof((((TYPE *)0)->MEMBER)))
 
diff --git a/tools/testing/selftests/bpf/progs/test_tc_tunnel.c b/tools/testing/selftests/bpf/progs/test_tc_tunnel.c
index e6e678aa9874..d3e439ff323b 100644
--- a/tools/testing/selftests/bpf/progs/test_tc_tunnel.c
+++ b/tools/testing/selftests/bpf/progs/test_tc_tunnel.c
@@ -20,6 +20,10 @@ 
 #include <bpf/bpf_endian.h>
 #include <bpf/bpf_helpers.h>
 
+#if !__clang__
+#pragma GCC diagnostic ignored "-Waddress-of-packed-member"
+#endif
+
 static const int cfg_port = 8000;
 
 static const int cfg_udp_src = 20000;