* [PATCH bpf] selftests/bpf: fix static assert compilation issue for test_cls_*.c
@ 2023-08-01 10:29 Alan Maguire
2023-08-01 17:09 ` Yonghong Song
0 siblings, 1 reply; 4+ messages in thread
From: Alan Maguire @ 2023-08-01 10:29 UTC (permalink / raw)
To: ast, andrii, daniel
Cc: martin.lau, song, yhs, john.fastabend, kpsingh, sdf, haoluo,
jolsa, mykolal, bpf, Alan Maguire, Colm Harrington
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)))
+
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 {
--
2.39.3
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH bpf] selftests/bpf: fix static assert compilation issue for test_cls_*.c
2023-08-01 10:29 [PATCH bpf] selftests/bpf: fix static assert compilation issue for test_cls_*.c Alan Maguire
@ 2023-08-01 17:09 ` Yonghong Song
2023-08-01 17:57 ` Alan Maguire
0 siblings, 1 reply; 4+ messages in thread
From: Yonghong Song @ 2023-08-01 17:09 UTC (permalink / raw)
To: Alan Maguire, ast, andrii, daniel
Cc: martin.lau, song, yhs, john.fastabend, kpsingh, sdf, haoluo,
jolsa, mykolal, bpf, Colm Harrington
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 {
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH bpf] selftests/bpf: fix static assert compilation issue for test_cls_*.c
2023-08-01 17:09 ` Yonghong Song
@ 2023-08-01 17:57 ` Alan Maguire
2023-08-01 20:14 ` Yonghong Song
0 siblings, 1 reply; 4+ messages in thread
From: Alan Maguire @ 2023-08-01 17:57 UTC (permalink / raw)
To: yonghong.song, ast, andrii, daniel
Cc: martin.lau, song, yhs, john.fastabend, kpsingh, sdf, haoluo,
jolsa, mykolal, bpf, Colm Harrington
On 01/08/2023 18:09, Yonghong Song wrote:
>
>
> 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'.
>
That's one way to solve it alright, but then other instances of
offsetof() in the BPF code (that are not part of static asserts) aren't
CO-RE-safe. Probably not a big concern for a test case that is usually
run against the same kernel, but it's perhaps worth retaining the
distinction between compile-time and run-time needs?
Alan
>
>> +
>> 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 {
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH bpf] selftests/bpf: fix static assert compilation issue for test_cls_*.c
2023-08-01 17:57 ` Alan Maguire
@ 2023-08-01 20:14 ` Yonghong Song
0 siblings, 0 replies; 4+ messages in thread
From: Yonghong Song @ 2023-08-01 20:14 UTC (permalink / raw)
To: Alan Maguire, ast, andrii, daniel
Cc: martin.lau, song, yhs, john.fastabend, kpsingh, sdf, haoluo,
jolsa, mykolal, bpf, Colm Harrington
On 8/1/23 10:57 AM, Alan Maguire wrote:
> On 01/08/2023 18:09, Yonghong Song wrote:
>>
>>
>> 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'.
>>
>
> That's one way to solve it alright, but then other instances of
> offsetof() in the BPF code (that are not part of static asserts) aren't
> CO-RE-safe. Probably not a big concern for a test case that is usually
There are no CORE relocation here. vmlinux.h is not involved
and no explicit CORE relocation requests in the related C code.
Also I checked all offsetof usage in related C files
(test_cls_redirect.c, test_cls_redirect_dynptr.c) and the
offsetof only operates on uapi headers so CORE relocation
for them are not needed.
> run against the same kernel, but it's perhaps worth retaining the
> distinction between compile-time and run-time needs?
>
> Alan
>
>>
>>> +
>>> 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 {
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-08-01 20:14 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-08-01 10:29 [PATCH bpf] selftests/bpf: fix static assert compilation issue for test_cls_*.c Alan Maguire
2023-08-01 17:09 ` Yonghong Song
2023-08-01 17:57 ` Alan Maguire
2023-08-01 20:14 ` Yonghong Song
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox