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

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

> 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 <jose.marchesi@oracle.com>
>> Cc: Andrii Nakryiko <andrii.nakryiko@gmail.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/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
> [...]