Re: [PATCH bpf] selftests/bpf: fix static assert compilation issue for test_cls_*.c

[Yonghong Song <yonghong.song@linux.dev>]

On 8/1/23 3:29 AM, Alan Maguire wrote:
> commit bdeeed3498c7 ("libbpf: fix offsetof() and container_of() to work with CO-RE")
> 
> ...was backported to stable trees such as 5.15. The problem is that with older
> LLVM/clang (14/15) - which is often used for older kernels - we see compilation
> failures in BPF selftests now:
> 
> In file included from progs/test_cls_redirect_subprogs.c:2:
> progs/test_cls_redirect.c:90:2: error: static assertion expression is not an integral constant expression
>          sizeof(flow_ports_t) !=
>          ^~~~~~~~~~~~~~~~~~~~~~~
> progs/test_cls_redirect.c:91:3: note: cast that performs the conversions of a reinterpret_cast is not allowed in a constant expression
>                  offsetofend(struct bpf_sock_tuple, ipv4.dport) -
>                  ^
> progs/test_cls_redirect.c:32:3: note: expanded from macro 'offsetofend'
>          (offsetof(TYPE, MEMBER) + sizeof((((TYPE *)0)->MEMBER)))
>           ^
> tools/testing/selftests/bpf/tools/include/bpf/bpf_helpers.h:86:33: note: expanded from macro 'offsetof'
>                                   ^
> In file included from progs/test_cls_redirect_subprogs.c:2:
> progs/test_cls_redirect.c:95:2: error: static assertion expression is not an integral constant expression
>          sizeof(flow_ports_t) !=
>          ^~~~~~~~~~~~~~~~~~~~~~~
> progs/test_cls_redirect.c:96:3: note: cast that performs the conversions of a reinterpret_cast is not allowed in a constant expression
>                  offsetofend(struct bpf_sock_tuple, ipv6.dport) -
>                  ^
> progs/test_cls_redirect.c:32:3: note: expanded from macro 'offsetofend'
>          (offsetof(TYPE, MEMBER) + sizeof((((TYPE *)0)->MEMBER)))
>           ^
> tools/testing/selftests/bpf/tools/include/bpf/bpf_helpers.h:86:33: note: expanded from macro 'offsetof'
>                                   ^
> 2 errors generated.
> make: *** [Makefile:594: tools/testing/selftests/bpf/test_cls_redirect_subprogs.bpf.o] Error 1
> 
> The problem is the new offsetof() does not play nice with static asserts.
> Given that the context is a static assert (and CO-RE relocation is not
> needed at compile time), offsetof() usage can be replaced by
> __builtin_offsetof(), and all is well.  Define __builtin_offsetofend()
> to be used in static asserts also, since offsetofend() is also defined in
> bpf_util.h and is used in userspace progs, so redefining offsetofend()
> in test_cls_redirect.h won't work.
> 
> Fixes: bdeeed3498c7 ("libbpf: fix offsetof() and container_of() to work with CO-RE")
> Reported-by: Colm Harrington <colm.harrington@oracle.com>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>   tools/testing/selftests/bpf/progs/test_cls_redirect.c | 11 ++++-------
>   tools/testing/selftests/bpf/progs/test_cls_redirect.h |  3 +++
>   .../selftests/bpf/progs/test_cls_redirect_dynptr.c    | 11 ++++-------
>   3 files changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/progs/test_cls_redirect.c b/tools/testing/selftests/bpf/progs/test_cls_redirect.c
> index 66b304982245..e68e0544827c 100644
> --- a/tools/testing/selftests/bpf/progs/test_cls_redirect.c
> +++ b/tools/testing/selftests/bpf/progs/test_cls_redirect.c
> @@ -28,9 +28,6 @@
>   #define INLINING __always_inline
>   #endif
>   
> -#define offsetofend(TYPE, MEMBER) \
> -	(offsetof(TYPE, MEMBER) + sizeof((((TYPE *)0)->MEMBER)))
> -
>   #define IP_OFFSET_MASK (0x1FFF)
>   #define IP_MF (0x2000)
>   
> @@ -88,13 +85,13 @@ typedef struct {
>   
>   _Static_assert(
>   	sizeof(flow_ports_t) !=
> -		offsetofend(struct bpf_sock_tuple, ipv4.dport) -
> -			offsetof(struct bpf_sock_tuple, ipv4.sport) - 1,
> +		__builtin_offsetofend(struct bpf_sock_tuple, ipv4.dport) -
> +		__builtin_offsetof(struct bpf_sock_tuple, ipv4.sport) - 1,
>   	"flow_ports_t must match sport and dport in struct bpf_sock_tuple");
>   _Static_assert(
>   	sizeof(flow_ports_t) !=
> -		offsetofend(struct bpf_sock_tuple, ipv6.dport) -
> -			offsetof(struct bpf_sock_tuple, ipv6.sport) - 1,
> +		__builtin_offsetofend(struct bpf_sock_tuple, ipv6.dport) -
> +		__builtin_offsetof(struct bpf_sock_tuple, ipv6.sport) - 1,
>   	"flow_ports_t must match sport and dport in struct bpf_sock_tuple");
>   
>   typedef int ret_t;
> diff --git a/tools/testing/selftests/bpf/progs/test_cls_redirect.h b/tools/testing/selftests/bpf/progs/test_cls_redirect.h
> index 76eab0aacba0..1de0b727a3f6 100644
> --- a/tools/testing/selftests/bpf/progs/test_cls_redirect.h
> +++ b/tools/testing/selftests/bpf/progs/test_cls_redirect.h
> @@ -12,6 +12,9 @@
>   #include <linux/ipv6.h>
>   #include <linux/udp.h>
>   
> +#define __builtin_offsetofend(TYPE, MEMBER) \
> +	(__builtin_offsetof(TYPE, MEMBER) + sizeof((((TYPE *)0)->MEMBER)))

I think this can be simplified to undef and re-define offsetof like below:

#ifdef offsetof
#undef offsetof
#define offsetof(type, member) __builtin_offsetof(type, member)
#endif

Then other changes in this patch become unnecessary.

You can add comments for the above code to explain
why you want to redefine 'offsetof'.


> +
>   struct gre_base_hdr {
>   	uint16_t flags;
>   	uint16_t protocol;
> 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..463b0513f871 100644
> --- a/tools/testing/selftests/bpf/progs/test_cls_redirect_dynptr.c
> +++ b/tools/testing/selftests/bpf/progs/test_cls_redirect_dynptr.c
> @@ -23,9 +23,6 @@
>   #include "test_cls_redirect.h"
>   #include "bpf_kfuncs.h"
>   
> -#define offsetofend(TYPE, MEMBER) \
> -	(offsetof(TYPE, MEMBER) + sizeof((((TYPE *)0)->MEMBER)))
> -
>   #define IP_OFFSET_MASK (0x1FFF)
>   #define IP_MF (0x2000)
>   
> @@ -83,13 +80,13 @@ typedef struct {
>   
>   _Static_assert(
>   	sizeof(flow_ports_t) !=
> -		offsetofend(struct bpf_sock_tuple, ipv4.dport) -
> -			offsetof(struct bpf_sock_tuple, ipv4.sport) - 1,
> +		__builtin_offsetofend(struct bpf_sock_tuple, ipv4.dport) -
> +		__builtin_offsetof(struct bpf_sock_tuple, ipv4.sport) - 1,
>   	"flow_ports_t must match sport and dport in struct bpf_sock_tuple");
>   _Static_assert(
>   	sizeof(flow_ports_t) !=
> -		offsetofend(struct bpf_sock_tuple, ipv6.dport) -
> -			offsetof(struct bpf_sock_tuple, ipv6.sport) - 1,
> +		__builtin_offsetofend(struct bpf_sock_tuple, ipv6.dport) -
> +		__builtin_offsetof(struct bpf_sock_tuple, ipv6.sport) - 1,
>   	"flow_ports_t must match sport and dport in struct bpf_sock_tuple");
>   
>   struct iphdr_info {