[PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[Sergey Senozhatsky]

Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
thus not all compilers are able to compile the following code:

        (__builtin_constant_p(x) ? \
                ___constant_swab16(x) : __builtin_bswap16(x))

That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
instance:

        error: implicit declaration of function '__builtin_bswap16'

We can use __builtin_bswap16() only if compiler has this built-in,
that is, only if __HAVE_BUILTIN_BSWAP16__ is defined. Standard UAPI
__swab16()/__swab32() take care of that, and, additionally, handle
__builtin_constant_p() cases as well:

 #ifdef __HAVE_BUILTIN_BSWAP16__
 #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
 #else
 #define __swab16(x)                             \
         (__builtin_constant_p((__u16)(x)) ?     \
         ___constant_swab16(x) :                 \
         __fswab16(x))
 #endif

So we can tweak selftests/bpf/bpf_endian.h and use UAPI
__swab16()/__swab32().

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---

v2: fixed build error, reshuffled patches (Stanislav Fomichev)

 tools/testing/selftests/bpf/bpf_endian.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/bpf/bpf_endian.h b/tools/testing/selftests/bpf/bpf_endian.h
index b25595ea4a78..1ed268b2002b 100644
--- a/tools/testing/selftests/bpf/bpf_endian.h
+++ b/tools/testing/selftests/bpf/bpf_endian.h
@@ -20,12 +20,12 @@
  * use different targets.
  */
 #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
-# define __bpf_ntohs(x)			__builtin_bswap16(x)
-# define __bpf_htons(x)			__builtin_bswap16(x)
+# define __bpf_ntohs(x)			__swab16(x)
+# define __bpf_htons(x)			__swab16(x)
 # define __bpf_constant_ntohs(x)	___constant_swab16(x)
 # define __bpf_constant_htons(x)	___constant_swab16(x)
-# define __bpf_ntohl(x)			__builtin_bswap32(x)
-# define __bpf_htonl(x)			__builtin_bswap32(x)
+# define __bpf_ntohl(x)			__swab32(x)
+# define __bpf_htonl(x)			__swab32(x)
 # define __bpf_constant_ntohl(x)	___constant_swab32(x)
 # define __bpf_constant_htonl(x)	___constant_swab32(x)
 #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
-- 
2.21.0

[PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[sergey.senozhatsky]

Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
thus not all compilers are able to compile the following code:

        (__builtin_constant_p(x) ? \
                ___constant_swab16(x) : __builtin_bswap16(x))

That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
instance:

        error: implicit declaration of function '__builtin_bswap16'

We can use __builtin_bswap16() only if compiler has this built-in,
that is, only if __HAVE_BUILTIN_BSWAP16__ is defined. Standard UAPI
__swab16()/__swab32() take care of that, and, additionally, handle
__builtin_constant_p() cases as well:

 #ifdef __HAVE_BUILTIN_BSWAP16__
 #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
 #else
 #define __swab16(x)                             \
         (__builtin_constant_p((__u16)(x)) ?     \
         ___constant_swab16(x) :                 \
         __fswab16(x))
 #endif

So we can tweak selftests/bpf/bpf_endian.h and use UAPI
__swab16()/__swab32().

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky at gmail.com>
---

v2: fixed build error, reshuffled patches (Stanislav Fomichev)

 tools/testing/selftests/bpf/bpf_endian.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/bpf/bpf_endian.h b/tools/testing/selftests/bpf/bpf_endian.h
index b25595ea4a78..1ed268b2002b 100644
--- a/tools/testing/selftests/bpf/bpf_endian.h
+++ b/tools/testing/selftests/bpf/bpf_endian.h
@@ -20,12 +20,12 @@
  * use different targets.
  */
 #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
-# define __bpf_ntohs(x)			__builtin_bswap16(x)
-# define __bpf_htons(x)			__builtin_bswap16(x)
+# define __bpf_ntohs(x)			__swab16(x)
+# define __bpf_htons(x)			__swab16(x)
 # define __bpf_constant_ntohs(x)	___constant_swab16(x)
 # define __bpf_constant_htons(x)	___constant_swab16(x)
-# define __bpf_ntohl(x)			__builtin_bswap32(x)
-# define __bpf_htonl(x)			__builtin_bswap32(x)
+# define __bpf_ntohl(x)			__swab32(x)
+# define __bpf_htonl(x)			__swab32(x)
 # define __bpf_constant_ntohl(x)	___constant_swab32(x)
 # define __bpf_constant_htonl(x)	___constant_swab32(x)
 #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
-- 
2.21.0

[PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[Sergey Senozhatsky]

Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
thus not all compilers are able to compile the following code:

        (__builtin_constant_p(x) ? \
                ___constant_swab16(x) : __builtin_bswap16(x))

That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
instance:

        error: implicit declaration of function '__builtin_bswap16'

We can use __builtin_bswap16() only if compiler has this built-in,
that is, only if __HAVE_BUILTIN_BSWAP16__ is defined. Standard UAPI
__swab16()/__swab32() take care of that, and, additionally, handle
__builtin_constant_p() cases as well:

 #ifdef __HAVE_BUILTIN_BSWAP16__
 #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
 #else
 #define __swab16(x)                             \
         (__builtin_constant_p((__u16)(x)) ?     \
         ___constant_swab16(x) :                 \
         __fswab16(x))
 #endif

So we can tweak selftests/bpf/bpf_endian.h and use UAPI
__swab16()/__swab32().

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky at gmail.com>
---

v2: fixed build error, reshuffled patches (Stanislav Fomichev)

 tools/testing/selftests/bpf/bpf_endian.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/bpf/bpf_endian.h b/tools/testing/selftests/bpf/bpf_endian.h
index b25595ea4a78..1ed268b2002b 100644
--- a/tools/testing/selftests/bpf/bpf_endian.h
+++ b/tools/testing/selftests/bpf/bpf_endian.h
@@ -20,12 +20,12 @@
  * use different targets.
  */
 #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
-# define __bpf_ntohs(x)			__builtin_bswap16(x)
-# define __bpf_htons(x)			__builtin_bswap16(x)
+# define __bpf_ntohs(x)			__swab16(x)
+# define __bpf_htons(x)			__swab16(x)
 # define __bpf_constant_ntohs(x)	___constant_swab16(x)
 # define __bpf_constant_htons(x)	___constant_swab16(x)
-# define __bpf_ntohl(x)			__builtin_bswap32(x)
-# define __bpf_htonl(x)			__builtin_bswap32(x)
+# define __bpf_ntohl(x)			__swab32(x)
+# define __bpf_htonl(x)			__swab32(x)
 # define __bpf_constant_ntohl(x)	___constant_swab32(x)
 # define __bpf_constant_htonl(x)	___constant_swab32(x)
 #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
-- 
2.21.0

[PATCHv2 bpf-next 2/3] bpf, tests: drop unused __bpf_constant_foo defines

[Sergey Senozhatsky]

Additionally, simplify bpf_ntohs/bpf_ntohl/bpf_htons/bpf_htonl
by switching to UAPI swab().

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 tools/testing/selftests/bpf/bpf_endian.h | 37 +++++-------------------
 1 file changed, 8 insertions(+), 29 deletions(-)

diff --git a/tools/testing/selftests/bpf/bpf_endian.h b/tools/testing/selftests/bpf/bpf_endian.h
index 1ed268b2002b..ba06222963d5 100644
--- a/tools/testing/selftests/bpf/bpf_endian.h
+++ b/tools/testing/selftests/bpf/bpf_endian.h
@@ -20,38 +20,17 @@
  * use different targets.
  */
 #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
-# define __bpf_ntohs(x)			__swab16(x)
-# define __bpf_htons(x)			__swab16(x)
-# define __bpf_constant_ntohs(x)	___constant_swab16(x)
-# define __bpf_constant_htons(x)	___constant_swab16(x)
-# define __bpf_ntohl(x)			__swab32(x)
-# define __bpf_htonl(x)			__swab32(x)
-# define __bpf_constant_ntohl(x)	___constant_swab32(x)
-# define __bpf_constant_htonl(x)	___constant_swab32(x)
+# define bpf_ntohs(x)		__swab16(x)
+# define bpf_htons(x)		__swab16(x)
+# define bpf_ntohl(x)		__swab32(x)
+# define bpf_htonl(x)		__swab32(x)
 #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
-# define __bpf_ntohs(x)			(x)
-# define __bpf_htons(x)			(x)
-# define __bpf_constant_ntohs(x)	(x)
-# define __bpf_constant_htons(x)	(x)
-# define __bpf_ntohl(x)			(x)
-# define __bpf_htonl(x)			(x)
-# define __bpf_constant_ntohl(x)	(x)
-# define __bpf_constant_htonl(x)	(x)
+# define bpf_ntohs(x)		(x)
+# define bpf_htons(x)		(x)
+# define bpf_ntohl(x)		(x)
+# define bpf_htonl(x)		(x)
 #else
 # error "Fix your compiler's __BYTE_ORDER__?!"
 #endif
 
-#define bpf_htons(x)				\
-	(__builtin_constant_p(x) ?		\
-	 __bpf_constant_htons(x) : __bpf_htons(x))
-#define bpf_ntohs(x)				\
-	(__builtin_constant_p(x) ?		\
-	 __bpf_constant_ntohs(x) : __bpf_ntohs(x))
-#define bpf_htonl(x)				\
-	(__builtin_constant_p(x) ?		\
-	 __bpf_constant_htonl(x) : __bpf_htonl(x))
-#define bpf_ntohl(x)				\
-	(__builtin_constant_p(x) ?		\
-	 __bpf_constant_ntohl(x) : __bpf_ntohl(x))
-
 #endif /* __BPF_ENDIAN__ */
-- 
2.21.0

[PATCHv2 bpf-next 2/3] bpf, tests: drop unused __bpf_constant_foo defines

[sergey.senozhatsky]

Additionally, simplify bpf_ntohs/bpf_ntohl/bpf_htons/bpf_htonl
by switching to UAPI swab().

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky at gmail.com>
---
 tools/testing/selftests/bpf/bpf_endian.h | 37 +++++-------------------
 1 file changed, 8 insertions(+), 29 deletions(-)

diff --git a/tools/testing/selftests/bpf/bpf_endian.h b/tools/testing/selftests/bpf/bpf_endian.h
index 1ed268b2002b..ba06222963d5 100644
--- a/tools/testing/selftests/bpf/bpf_endian.h
+++ b/tools/testing/selftests/bpf/bpf_endian.h
@@ -20,38 +20,17 @@
  * use different targets.
  */
 #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
-# define __bpf_ntohs(x)			__swab16(x)
-# define __bpf_htons(x)			__swab16(x)
-# define __bpf_constant_ntohs(x)	___constant_swab16(x)
-# define __bpf_constant_htons(x)	___constant_swab16(x)
-# define __bpf_ntohl(x)			__swab32(x)
-# define __bpf_htonl(x)			__swab32(x)
-# define __bpf_constant_ntohl(x)	___constant_swab32(x)
-# define __bpf_constant_htonl(x)	___constant_swab32(x)
+# define bpf_ntohs(x)		__swab16(x)
+# define bpf_htons(x)		__swab16(x)
+# define bpf_ntohl(x)		__swab32(x)
+# define bpf_htonl(x)		__swab32(x)
 #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
-# define __bpf_ntohs(x)			(x)
-# define __bpf_htons(x)			(x)
-# define __bpf_constant_ntohs(x)	(x)
-# define __bpf_constant_htons(x)	(x)
-# define __bpf_ntohl(x)			(x)
-# define __bpf_htonl(x)			(x)
-# define __bpf_constant_ntohl(x)	(x)
-# define __bpf_constant_htonl(x)	(x)
+# define bpf_ntohs(x)		(x)
+# define bpf_htons(x)		(x)
+# define bpf_ntohl(x)		(x)
+# define bpf_htonl(x)		(x)
 #else
 # error "Fix your compiler's __BYTE_ORDER__?!"
 #endif
 
-#define bpf_htons(x)				\
-	(__builtin_constant_p(x) ?		\
-	 __bpf_constant_htons(x) : __bpf_htons(x))
-#define bpf_ntohs(x)				\
-	(__builtin_constant_p(x) ?		\
-	 __bpf_constant_ntohs(x) : __bpf_ntohs(x))
-#define bpf_htonl(x)				\
-	(__builtin_constant_p(x) ?		\
-	 __bpf_constant_htonl(x) : __bpf_htonl(x))
-#define bpf_ntohl(x)				\
-	(__builtin_constant_p(x) ?		\
-	 __bpf_constant_ntohl(x) : __bpf_ntohl(x))
-
 #endif /* __BPF_ENDIAN__ */
-- 
2.21.0

[PATCHv2 bpf-next 2/3] bpf, tests: drop unused __bpf_constant_foo defines

[Sergey Senozhatsky]

Additionally, simplify bpf_ntohs/bpf_ntohl/bpf_htons/bpf_htonl
by switching to UAPI swab().

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky at gmail.com>
---
 tools/testing/selftests/bpf/bpf_endian.h | 37 +++++-------------------
 1 file changed, 8 insertions(+), 29 deletions(-)

diff --git a/tools/testing/selftests/bpf/bpf_endian.h b/tools/testing/selftests/bpf/bpf_endian.h
index 1ed268b2002b..ba06222963d5 100644
--- a/tools/testing/selftests/bpf/bpf_endian.h
+++ b/tools/testing/selftests/bpf/bpf_endian.h
@@ -20,38 +20,17 @@
  * use different targets.
  */
 #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
-# define __bpf_ntohs(x)			__swab16(x)
-# define __bpf_htons(x)			__swab16(x)
-# define __bpf_constant_ntohs(x)	___constant_swab16(x)
-# define __bpf_constant_htons(x)	___constant_swab16(x)
-# define __bpf_ntohl(x)			__swab32(x)
-# define __bpf_htonl(x)			__swab32(x)
-# define __bpf_constant_ntohl(x)	___constant_swab32(x)
-# define __bpf_constant_htonl(x)	___constant_swab32(x)
+# define bpf_ntohs(x)		__swab16(x)
+# define bpf_htons(x)		__swab16(x)
+# define bpf_ntohl(x)		__swab32(x)
+# define bpf_htonl(x)		__swab32(x)
 #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
-# define __bpf_ntohs(x)			(x)
-# define __bpf_htons(x)			(x)
-# define __bpf_constant_ntohs(x)	(x)
-# define __bpf_constant_htons(x)	(x)
-# define __bpf_ntohl(x)			(x)
-# define __bpf_htonl(x)			(x)
-# define __bpf_constant_ntohl(x)	(x)
-# define __bpf_constant_htonl(x)	(x)
+# define bpf_ntohs(x)		(x)
+# define bpf_htons(x)		(x)
+# define bpf_ntohl(x)		(x)
+# define bpf_htonl(x)		(x)
 #else
 # error "Fix your compiler's __BYTE_ORDER__?!"
 #endif
 
-#define bpf_htons(x)				\
-	(__builtin_constant_p(x) ?		\
-	 __bpf_constant_htons(x) : __bpf_htons(x))
-#define bpf_ntohs(x)				\
-	(__builtin_constant_p(x) ?		\
-	 __bpf_constant_ntohs(x) : __bpf_ntohs(x))
-#define bpf_htonl(x)				\
-	(__builtin_constant_p(x) ?		\
-	 __bpf_constant_htonl(x) : __bpf_htonl(x))
-#define bpf_ntohl(x)				\
-	(__builtin_constant_p(x) ?		\
-	 __bpf_constant_ntohl(x) : __bpf_ntohl(x))
-
 #endif /* __BPF_ENDIAN__ */
-- 
2.21.0

[PATCHv2 bpf-next 3/3] bpf, tests: don't use __bpf_constant_htons()

[Sergey Senozhatsky]

Prefer bpf_htons() instead.

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 tools/testing/selftests/bpf/prog_tests/flow_dissector.c | 4 ++--
 tools/testing/selftests/bpf/test_progs.c                | 8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/flow_dissector.c b/tools/testing/selftests/bpf/prog_tests/flow_dissector.c
index bcbd928c96ab..a194305f98d1 100644
--- a/tools/testing/selftests/bpf/prog_tests/flow_dissector.c
+++ b/tools/testing/selftests/bpf/prog_tests/flow_dissector.c
@@ -28,7 +28,7 @@ static struct bpf_flow_keys pkt_v4_flow_keys = {
 	.thoff = sizeof(struct iphdr),
 	.addr_proto = ETH_P_IP,
 	.ip_proto = IPPROTO_TCP,
-	.n_proto = __bpf_constant_htons(ETH_P_IP),
+	.n_proto = bpf_htons(ETH_P_IP),
 };
 
 static struct bpf_flow_keys pkt_v6_flow_keys = {
@@ -36,7 +36,7 @@ static struct bpf_flow_keys pkt_v6_flow_keys = {
 	.thoff = sizeof(struct ipv6hdr),
 	.addr_proto = ETH_P_IPV6,
 	.ip_proto = IPPROTO_TCP,
-	.n_proto = __bpf_constant_htons(ETH_P_IPV6),
+	.n_proto = bpf_htons(ETH_P_IPV6),
 };
 
 void test_flow_dissector(void)
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 5d10aee9e277..909b339f97f4 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -11,18 +11,18 @@ int error_cnt, pass_cnt;
 bool jit_enabled;
 
 struct ipv4_packet pkt_v4 = {
-	.eth.h_proto = __bpf_constant_htons(ETH_P_IP),
+	.eth.h_proto = bpf_htons(ETH_P_IP),
 	.iph.ihl = 5,
 	.iph.protocol = IPPROTO_TCP,
-	.iph.tot_len = __bpf_constant_htons(MAGIC_BYTES),
+	.iph.tot_len = bpf_htons(MAGIC_BYTES),
 	.tcp.urg_ptr = 123,
 	.tcp.doff = 5,
 };
 
 struct ipv6_packet pkt_v6 = {
-	.eth.h_proto = __bpf_constant_htons(ETH_P_IPV6),
+	.eth.h_proto = bpf_htons(ETH_P_IPV6),
 	.iph.nexthdr = IPPROTO_TCP,
-	.iph.payload_len = __bpf_constant_htons(MAGIC_BYTES),
+	.iph.payload_len = bpf_htons(MAGIC_BYTES),
 	.tcp.urg_ptr = 123,
 	.tcp.doff = 5,
 };
-- 
2.21.0

[PATCHv2 bpf-next 3/3] bpf, tests: don't use __bpf_constant_htons()

[sergey.senozhatsky]

Prefer bpf_htons() instead.

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky at gmail.com>
---
 tools/testing/selftests/bpf/prog_tests/flow_dissector.c | 4 ++--
 tools/testing/selftests/bpf/test_progs.c                | 8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/flow_dissector.c b/tools/testing/selftests/bpf/prog_tests/flow_dissector.c
index bcbd928c96ab..a194305f98d1 100644
--- a/tools/testing/selftests/bpf/prog_tests/flow_dissector.c
+++ b/tools/testing/selftests/bpf/prog_tests/flow_dissector.c
@@ -28,7 +28,7 @@ static struct bpf_flow_keys pkt_v4_flow_keys = {
 	.thoff = sizeof(struct iphdr),
 	.addr_proto = ETH_P_IP,
 	.ip_proto = IPPROTO_TCP,
-	.n_proto = __bpf_constant_htons(ETH_P_IP),
+	.n_proto = bpf_htons(ETH_P_IP),
 };
 
 static struct bpf_flow_keys pkt_v6_flow_keys = {
@@ -36,7 +36,7 @@ static struct bpf_flow_keys pkt_v6_flow_keys = {
 	.thoff = sizeof(struct ipv6hdr),
 	.addr_proto = ETH_P_IPV6,
 	.ip_proto = IPPROTO_TCP,
-	.n_proto = __bpf_constant_htons(ETH_P_IPV6),
+	.n_proto = bpf_htons(ETH_P_IPV6),
 };
 
 void test_flow_dissector(void)
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 5d10aee9e277..909b339f97f4 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -11,18 +11,18 @@ int error_cnt, pass_cnt;
 bool jit_enabled;
 
 struct ipv4_packet pkt_v4 = {
-	.eth.h_proto = __bpf_constant_htons(ETH_P_IP),
+	.eth.h_proto = bpf_htons(ETH_P_IP),
 	.iph.ihl = 5,
 	.iph.protocol = IPPROTO_TCP,
-	.iph.tot_len = __bpf_constant_htons(MAGIC_BYTES),
+	.iph.tot_len = bpf_htons(MAGIC_BYTES),
 	.tcp.urg_ptr = 123,
 	.tcp.doff = 5,
 };
 
 struct ipv6_packet pkt_v6 = {
-	.eth.h_proto = __bpf_constant_htons(ETH_P_IPV6),
+	.eth.h_proto = bpf_htons(ETH_P_IPV6),
 	.iph.nexthdr = IPPROTO_TCP,
-	.iph.payload_len = __bpf_constant_htons(MAGIC_BYTES),
+	.iph.payload_len = bpf_htons(MAGIC_BYTES),
 	.tcp.urg_ptr = 123,
 	.tcp.doff = 5,
 };
-- 
2.21.0

[PATCHv2 bpf-next 3/3] bpf, tests: don't use __bpf_constant_htons()

[Sergey Senozhatsky]

Prefer bpf_htons() instead.

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky at gmail.com>
---
 tools/testing/selftests/bpf/prog_tests/flow_dissector.c | 4 ++--
 tools/testing/selftests/bpf/test_progs.c                | 8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/flow_dissector.c b/tools/testing/selftests/bpf/prog_tests/flow_dissector.c
index bcbd928c96ab..a194305f98d1 100644
--- a/tools/testing/selftests/bpf/prog_tests/flow_dissector.c
+++ b/tools/testing/selftests/bpf/prog_tests/flow_dissector.c
@@ -28,7 +28,7 @@ static struct bpf_flow_keys pkt_v4_flow_keys = {
 	.thoff = sizeof(struct iphdr),
 	.addr_proto = ETH_P_IP,
 	.ip_proto = IPPROTO_TCP,
-	.n_proto = __bpf_constant_htons(ETH_P_IP),
+	.n_proto = bpf_htons(ETH_P_IP),
 };
 
 static struct bpf_flow_keys pkt_v6_flow_keys = {
@@ -36,7 +36,7 @@ static struct bpf_flow_keys pkt_v6_flow_keys = {
 	.thoff = sizeof(struct ipv6hdr),
 	.addr_proto = ETH_P_IPV6,
 	.ip_proto = IPPROTO_TCP,
-	.n_proto = __bpf_constant_htons(ETH_P_IPV6),
+	.n_proto = bpf_htons(ETH_P_IPV6),
 };
 
 void test_flow_dissector(void)
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 5d10aee9e277..909b339f97f4 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -11,18 +11,18 @@ int error_cnt, pass_cnt;
 bool jit_enabled;
 
 struct ipv4_packet pkt_v4 = {
-	.eth.h_proto = __bpf_constant_htons(ETH_P_IP),
+	.eth.h_proto = bpf_htons(ETH_P_IP),
 	.iph.ihl = 5,
 	.iph.protocol = IPPROTO_TCP,
-	.iph.tot_len = __bpf_constant_htons(MAGIC_BYTES),
+	.iph.tot_len = bpf_htons(MAGIC_BYTES),
 	.tcp.urg_ptr = 123,
 	.tcp.doff = 5,
 };
 
 struct ipv6_packet pkt_v6 = {
-	.eth.h_proto = __bpf_constant_htons(ETH_P_IPV6),
+	.eth.h_proto = bpf_htons(ETH_P_IPV6),
 	.iph.nexthdr = IPPROTO_TCP,
-	.iph.payload_len = __bpf_constant_htons(MAGIC_BYTES),
+	.iph.payload_len = bpf_htons(MAGIC_BYTES),
 	.tcp.urg_ptr = 123,
 	.tcp.doff = 5,
 };
-- 
2.21.0

Re: [PATCHv2 bpf-next 3/3] bpf, tests: don't use __bpf_constant_htons()

[Sergey Senozhatsky]

Yikes... This is actually - 2/3

	-ss

[PATCHv2 bpf-next 3/3] bpf, tests: don't use __bpf_constant_htons()

[sergey.senozhatsky]

Yikes... This is actually - 2/3

	-ss

[PATCHv2 bpf-next 3/3] bpf, tests: don't use __bpf_constant_htons()

[Sergey Senozhatsky]

Yikes... This is actually - 2/3

	-ss

Re: [PATCHv2 bpf-next 2/3] bpf, tests: drop unused __bpf_constant_foo defines

[Sergey Senozhatsky]

And this one is the last one  -  3/3.

	-ss

[PATCHv2 bpf-next 2/3] bpf, tests: drop unused __bpf_constant_foo defines

[sergey.senozhatsky]

And this one is the last one  -  3/3.

	-ss

[PATCHv2 bpf-next 2/3] bpf, tests: drop unused __bpf_constant_foo defines

[Sergey Senozhatsky]

And this one is the last one  -  3/3.

	-ss

Re: [PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[Stanislav Fomichev]

On 03/20, Sergey Senozhatsky wrote:
> Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
> thus not all compilers are able to compile the following code:
> 
>         (__builtin_constant_p(x) ? \
>                 ___constant_swab16(x) : __builtin_bswap16(x))
> 
> That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
> instance:
> 
>         error: implicit declaration of function '__builtin_bswap16'
> 
> We can use __builtin_bswap16() only if compiler has this built-in,
> that is, only if __HAVE_BUILTIN_BSWAP16__ is defined. Standard UAPI
> __swab16()/__swab32() take care of that, and, additionally, handle
> __builtin_constant_p() cases as well:
> 
>  #ifdef __HAVE_BUILTIN_BSWAP16__
>  #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
>  #else
>  #define __swab16(x)                             \
>          (__builtin_constant_p((__u16)(x)) ?     \
>          ___constant_swab16(x) :                 \
>          __fswab16(x))
>  #endif
> 
> So we can tweak selftests/bpf/bpf_endian.h and use UAPI
> __swab16()/__swab32().
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
> 
> v2: fixed build error, reshuffled patches (Stanislav Fomichev)
Tested them locally with the compiler I saw the initial issues with - all
fine, I don't see any errors with the older gcc.

One last question I have is: what happens in the llvm+bpf case? Have
you tested that? I think LLVM has all the builtins required, but since
we are relying on the swab.h now (and it relies on
__HAVE_BUILTIN_BSWAP16__), I wonder whether this detection works
correctly on the llvm when targeting bpf. (sidenote: bpf_endian.h can be
used from both userspace and bpf programs).

> 
>  tools/testing/selftests/bpf/bpf_endian.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/bpf_endian.h b/tools/testing/selftests/bpf/bpf_endian.h
> index b25595ea4a78..1ed268b2002b 100644
> --- a/tools/testing/selftests/bpf/bpf_endian.h
> +++ b/tools/testing/selftests/bpf/bpf_endian.h
> @@ -20,12 +20,12 @@
>   * use different targets.
>   */
>  #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> -# define __bpf_ntohs(x)			__builtin_bswap16(x)
> -# define __bpf_htons(x)			__builtin_bswap16(x)
> +# define __bpf_ntohs(x)			__swab16(x)
> +# define __bpf_htons(x)			__swab16(x)
>  # define __bpf_constant_ntohs(x)	___constant_swab16(x)
>  # define __bpf_constant_htons(x)	___constant_swab16(x)
> -# define __bpf_ntohl(x)			__builtin_bswap32(x)
> -# define __bpf_htonl(x)			__builtin_bswap32(x)
> +# define __bpf_ntohl(x)			__swab32(x)
> +# define __bpf_htonl(x)			__swab32(x)
>  # define __bpf_constant_ntohl(x)	___constant_swab32(x)
>  # define __bpf_constant_htonl(x)	___constant_swab32(x)
>  #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> -- 
> 2.21.0
> 

[PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[sdf]

On 03/20, Sergey Senozhatsky wrote:
> Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
> thus not all compilers are able to compile the following code:
> 
>         (__builtin_constant_p(x) ? \
>                 ___constant_swab16(x) : __builtin_bswap16(x))
> 
> That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
> instance:
> 
>         error: implicit declaration of function '__builtin_bswap16'
> 
> We can use __builtin_bswap16() only if compiler has this built-in,
> that is, only if __HAVE_BUILTIN_BSWAP16__ is defined. Standard UAPI
> __swab16()/__swab32() take care of that, and, additionally, handle
> __builtin_constant_p() cases as well:
> 
>  #ifdef __HAVE_BUILTIN_BSWAP16__
>  #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
>  #else
>  #define __swab16(x)                             \
>          (__builtin_constant_p((__u16)(x)) ?     \
>          ___constant_swab16(x) :                 \
>          __fswab16(x))
>  #endif
> 
> So we can tweak selftests/bpf/bpf_endian.h and use UAPI
> __swab16()/__swab32().
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky at gmail.com>
> ---
> 
> v2: fixed build error, reshuffled patches (Stanislav Fomichev)
Tested them locally with the compiler I saw the initial issues with - all
fine, I don't see any errors with the older gcc.

One last question I have is: what happens in the llvm+bpf case? Have
you tested that? I think LLVM has all the builtins required, but since
we are relying on the swab.h now (and it relies on
__HAVE_BUILTIN_BSWAP16__), I wonder whether this detection works
correctly on the llvm when targeting bpf. (sidenote: bpf_endian.h can be
used from both userspace and bpf programs).

> 
>  tools/testing/selftests/bpf/bpf_endian.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/bpf_endian.h b/tools/testing/selftests/bpf/bpf_endian.h
> index b25595ea4a78..1ed268b2002b 100644
> --- a/tools/testing/selftests/bpf/bpf_endian.h
> +++ b/tools/testing/selftests/bpf/bpf_endian.h
> @@ -20,12 +20,12 @@
>   * use different targets.
>   */
>  #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> -# define __bpf_ntohs(x)			__builtin_bswap16(x)
> -# define __bpf_htons(x)			__builtin_bswap16(x)
> +# define __bpf_ntohs(x)			__swab16(x)
> +# define __bpf_htons(x)			__swab16(x)
>  # define __bpf_constant_ntohs(x)	___constant_swab16(x)
>  # define __bpf_constant_htons(x)	___constant_swab16(x)
> -# define __bpf_ntohl(x)			__builtin_bswap32(x)
> -# define __bpf_htonl(x)			__builtin_bswap32(x)
> +# define __bpf_ntohl(x)			__swab32(x)
> +# define __bpf_htonl(x)			__swab32(x)
>  # define __bpf_constant_ntohl(x)	___constant_swab32(x)
>  # define __bpf_constant_htonl(x)	___constant_swab32(x)
>  #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> -- 
> 2.21.0
> 

[PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[Stanislav Fomichev]

On 03/20, Sergey Senozhatsky wrote:
> Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
> thus not all compilers are able to compile the following code:
> 
>         (__builtin_constant_p(x) ? \
>                 ___constant_swab16(x) : __builtin_bswap16(x))
> 
> That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
> instance:
> 
>         error: implicit declaration of function '__builtin_bswap16'
> 
> We can use __builtin_bswap16() only if compiler has this built-in,
> that is, only if __HAVE_BUILTIN_BSWAP16__ is defined. Standard UAPI
> __swab16()/__swab32() take care of that, and, additionally, handle
> __builtin_constant_p() cases as well:
> 
>  #ifdef __HAVE_BUILTIN_BSWAP16__
>  #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
>  #else
>  #define __swab16(x)                             \
>          (__builtin_constant_p((__u16)(x)) ?     \
>          ___constant_swab16(x) :                 \
>          __fswab16(x))
>  #endif
> 
> So we can tweak selftests/bpf/bpf_endian.h and use UAPI
> __swab16()/__swab32().
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky at gmail.com>
> ---
> 
> v2: fixed build error, reshuffled patches (Stanislav Fomichev)
Tested them locally with the compiler I saw the initial issues with - all
fine, I don't see any errors with the older gcc.

One last question I have is: what happens in the llvm+bpf case? Have
you tested that? I think LLVM has all the builtins required, but since
we are relying on the swab.h now (and it relies on
__HAVE_BUILTIN_BSWAP16__), I wonder whether this detection works
correctly on the llvm when targeting bpf. (sidenote: bpf_endian.h can be
used from both userspace and bpf programs).

> 
>  tools/testing/selftests/bpf/bpf_endian.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/bpf_endian.h b/tools/testing/selftests/bpf/bpf_endian.h
> index b25595ea4a78..1ed268b2002b 100644
> --- a/tools/testing/selftests/bpf/bpf_endian.h
> +++ b/tools/testing/selftests/bpf/bpf_endian.h
> @@ -20,12 +20,12 @@
>   * use different targets.
>   */
>  #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> -# define __bpf_ntohs(x)			__builtin_bswap16(x)
> -# define __bpf_htons(x)			__builtin_bswap16(x)
> +# define __bpf_ntohs(x)			__swab16(x)
> +# define __bpf_htons(x)			__swab16(x)
>  # define __bpf_constant_ntohs(x)	___constant_swab16(x)
>  # define __bpf_constant_htons(x)	___constant_swab16(x)
> -# define __bpf_ntohl(x)			__builtin_bswap32(x)
> -# define __bpf_htonl(x)			__builtin_bswap32(x)
> +# define __bpf_ntohl(x)			__swab32(x)
> +# define __bpf_htonl(x)			__swab32(x)
>  # define __bpf_constant_ntohl(x)	___constant_swab32(x)
>  # define __bpf_constant_htonl(x)	___constant_swab32(x)
>  #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> -- 
> 2.21.0
> 

Re: [PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[Yonghong Song]

On 3/20/19 10:13 AM, Stanislav Fomichev wrote:
> On 03/20, Sergey Senozhatsky wrote:
>> Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
>> thus not all compilers are able to compile the following code:
>>
>>          (__builtin_constant_p(x) ? \
>>                  ___constant_swab16(x) : __builtin_bswap16(x))
>>
>> That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
>> instance:
>>
>>          error: implicit declaration of function '__builtin_bswap16'
>>
>> We can use __builtin_bswap16() only if compiler has this built-in,
>> that is, only if __HAVE_BUILTIN_BSWAP16__ is defined. Standard UAPI
>> __swab16()/__swab32() take care of that, and, additionally, handle
>> __builtin_constant_p() cases as well:
>>
>>   #ifdef __HAVE_BUILTIN_BSWAP16__
>>   #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
>>   #else
>>   #define __swab16(x)                             \
>>           (__builtin_constant_p((__u16)(x)) ?     \
>>           ___constant_swab16(x) :                 \
>>           __fswab16(x))
>>   #endif
>>
>> So we can tweak selftests/bpf/bpf_endian.h and use UAPI
>> __swab16()/__swab32().
>>
>> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
>> ---
>>
>> v2: fixed build error, reshuffled patches (Stanislav Fomichev)
> Tested them locally with the compiler I saw the initial issues with - all
> fine, I don't see any errors with the older gcc.
> 
> One last question I have is: what happens in the llvm+bpf case? Have
> you tested that? I think LLVM has all the builtins required, but since
> we are relying on the swab.h now (and it relies on
> __HAVE_BUILTIN_BSWAP16__), I wonder whether this detection works
> correctly on the llvm when targeting bpf. (sidenote: bpf_endian.h can be
> used from both userspace and bpf programs).

Inside kernel clang compiler header (linux/compiler-clang.h) does not 
define __HAVE_BUILTIN_BSWAP16__. So it will go to the "else" branch in 
the above. So I think it should work with clang + bpf.

> 
>>
>>   tools/testing/selftests/bpf/bpf_endian.h | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/testing/selftests/bpf/bpf_endian.h b/tools/testing/selftests/bpf/bpf_endian.h
>> index b25595ea4a78..1ed268b2002b 100644
>> --- a/tools/testing/selftests/bpf/bpf_endian.h
>> +++ b/tools/testing/selftests/bpf/bpf_endian.h
>> @@ -20,12 +20,12 @@
>>    * use different targets.
>>    */
>>   #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
>> -# define __bpf_ntohs(x)			__builtin_bswap16(x)
>> -# define __bpf_htons(x)			__builtin_bswap16(x)
>> +# define __bpf_ntohs(x)			__swab16(x)
>> +# define __bpf_htons(x)			__swab16(x)
>>   # define __bpf_constant_ntohs(x)	___constant_swab16(x)
>>   # define __bpf_constant_htons(x)	___constant_swab16(x)
>> -# define __bpf_ntohl(x)			__builtin_bswap32(x)
>> -# define __bpf_htonl(x)			__builtin_bswap32(x)
>> +# define __bpf_ntohl(x)			__swab32(x)
>> +# define __bpf_htonl(x)			__swab32(x)
>>   # define __bpf_constant_ntohl(x)	___constant_swab32(x)
>>   # define __bpf_constant_htonl(x)	___constant_swab32(x)
>>   #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
>> -- 
>> 2.21.0
>>

[PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[yhs]

On 3/20/19 10:13 AM, Stanislav Fomichev wrote:
> On 03/20, Sergey Senozhatsky wrote:
>> Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
>> thus not all compilers are able to compile the following code:
>>
>>          (__builtin_constant_p(x) ? \
>>                  ___constant_swab16(x) : __builtin_bswap16(x))
>>
>> That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
>> instance:
>>
>>          error: implicit declaration of function '__builtin_bswap16'
>>
>> We can use __builtin_bswap16() only if compiler has this built-in,
>> that is, only if __HAVE_BUILTIN_BSWAP16__ is defined. Standard UAPI
>> __swab16()/__swab32() take care of that, and, additionally, handle
>> __builtin_constant_p() cases as well:
>>
>>   #ifdef __HAVE_BUILTIN_BSWAP16__
>>   #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
>>   #else
>>   #define __swab16(x)                             \
>>           (__builtin_constant_p((__u16)(x)) ?     \
>>           ___constant_swab16(x) :                 \
>>           __fswab16(x))
>>   #endif
>>
>> So we can tweak selftests/bpf/bpf_endian.h and use UAPI
>> __swab16()/__swab32().
>>
>> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky at gmail.com>
>> ---
>>
>> v2: fixed build error, reshuffled patches (Stanislav Fomichev)
> Tested them locally with the compiler I saw the initial issues with - all
> fine, I don't see any errors with the older gcc.
> 
> One last question I have is: what happens in the llvm+bpf case? Have
> you tested that? I think LLVM has all the builtins required, but since
> we are relying on the swab.h now (and it relies on
> __HAVE_BUILTIN_BSWAP16__), I wonder whether this detection works
> correctly on the llvm when targeting bpf. (sidenote: bpf_endian.h can be
> used from both userspace and bpf programs).

Inside kernel clang compiler header (linux/compiler-clang.h) does not 
define __HAVE_BUILTIN_BSWAP16__. So it will go to the "else" branch in 
the above. So I think it should work with clang + bpf.

> 
>>
>>   tools/testing/selftests/bpf/bpf_endian.h | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/testing/selftests/bpf/bpf_endian.h b/tools/testing/selftests/bpf/bpf_endian.h
>> index b25595ea4a78..1ed268b2002b 100644
>> --- a/tools/testing/selftests/bpf/bpf_endian.h
>> +++ b/tools/testing/selftests/bpf/bpf_endian.h
>> @@ -20,12 +20,12 @@
>>    * use different targets.
>>    */
>>   #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
>> -# define __bpf_ntohs(x)			__builtin_bswap16(x)
>> -# define __bpf_htons(x)			__builtin_bswap16(x)
>> +# define __bpf_ntohs(x)			__swab16(x)
>> +# define __bpf_htons(x)			__swab16(x)
>>   # define __bpf_constant_ntohs(x)	___constant_swab16(x)
>>   # define __bpf_constant_htons(x)	___constant_swab16(x)
>> -# define __bpf_ntohl(x)			__builtin_bswap32(x)
>> -# define __bpf_htonl(x)			__builtin_bswap32(x)
>> +# define __bpf_ntohl(x)			__swab32(x)
>> +# define __bpf_htonl(x)			__swab32(x)
>>   # define __bpf_constant_ntohl(x)	___constant_swab32(x)
>>   # define __bpf_constant_htonl(x)	___constant_swab32(x)
>>   #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
>> -- 
>> 2.21.0
>>

[PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[Yonghong Song]

On 3/20/19 10:13 AM, Stanislav Fomichev wrote:
> On 03/20, Sergey Senozhatsky wrote:
>> Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
>> thus not all compilers are able to compile the following code:
>>
>>          (__builtin_constant_p(x) ? \
>>                  ___constant_swab16(x) : __builtin_bswap16(x))
>>
>> That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
>> instance:
>>
>>          error: implicit declaration of function '__builtin_bswap16'
>>
>> We can use __builtin_bswap16() only if compiler has this built-in,
>> that is, only if __HAVE_BUILTIN_BSWAP16__ is defined. Standard UAPI
>> __swab16()/__swab32() take care of that, and, additionally, handle
>> __builtin_constant_p() cases as well:
>>
>>   #ifdef __HAVE_BUILTIN_BSWAP16__
>>   #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
>>   #else
>>   #define __swab16(x)                             \
>>           (__builtin_constant_p((__u16)(x)) ?     \
>>           ___constant_swab16(x) :                 \
>>           __fswab16(x))
>>   #endif
>>
>> So we can tweak selftests/bpf/bpf_endian.h and use UAPI
>> __swab16()/__swab32().
>>
>> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky at gmail.com>
>> ---
>>
>> v2: fixed build error, reshuffled patches (Stanislav Fomichev)
> Tested them locally with the compiler I saw the initial issues with - all
> fine, I don't see any errors with the older gcc.
> 
> One last question I have is: what happens in the llvm+bpf case? Have
> you tested that? I think LLVM has all the builtins required, but since
> we are relying on the swab.h now (and it relies on
> __HAVE_BUILTIN_BSWAP16__), I wonder whether this detection works
> correctly on the llvm when targeting bpf. (sidenote: bpf_endian.h can be
> used from both userspace and bpf programs).

Inside kernel clang compiler header (linux/compiler-clang.h) does not 
define __HAVE_BUILTIN_BSWAP16__. So it will go to the "else" branch in 
the above. So I think it should work with clang + bpf.

> 
>>
>>   tools/testing/selftests/bpf/bpf_endian.h | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/testing/selftests/bpf/bpf_endian.h b/tools/testing/selftests/bpf/bpf_endian.h
>> index b25595ea4a78..1ed268b2002b 100644
>> --- a/tools/testing/selftests/bpf/bpf_endian.h
>> +++ b/tools/testing/selftests/bpf/bpf_endian.h
>> @@ -20,12 +20,12 @@
>>    * use different targets.
>>    */
>>   #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
>> -# define __bpf_ntohs(x)			__builtin_bswap16(x)
>> -# define __bpf_htons(x)			__builtin_bswap16(x)
>> +# define __bpf_ntohs(x)			__swab16(x)
>> +# define __bpf_htons(x)			__swab16(x)
>>   # define __bpf_constant_ntohs(x)	___constant_swab16(x)
>>   # define __bpf_constant_htons(x)	___constant_swab16(x)
>> -# define __bpf_ntohl(x)			__builtin_bswap32(x)
>> -# define __bpf_htonl(x)			__builtin_bswap32(x)
>> +# define __bpf_ntohl(x)			__swab32(x)
>> +# define __bpf_htonl(x)			__swab32(x)
>>   # define __bpf_constant_ntohl(x)	___constant_swab32(x)
>>   # define __bpf_constant_htonl(x)	___constant_swab32(x)
>>   #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
>> -- 
>> 2.21.0
>>

Re: [PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[Stanislav Fomichev]

On 03/20, Yonghong Song wrote:
> 
> 
> On 3/20/19 10:13 AM, Stanislav Fomichev wrote:
> > On 03/20, Sergey Senozhatsky wrote:
> >> Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
> >> thus not all compilers are able to compile the following code:
> >>
> >>          (__builtin_constant_p(x) ? \
> >>                  ___constant_swab16(x) : __builtin_bswap16(x))
> >>
> >> That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
> >> instance:
> >>
> >>          error: implicit declaration of function '__builtin_bswap16'
> >>
> >> We can use __builtin_bswap16() only if compiler has this built-in,
> >> that is, only if __HAVE_BUILTIN_BSWAP16__ is defined. Standard UAPI
> >> __swab16()/__swab32() take care of that, and, additionally, handle
> >> __builtin_constant_p() cases as well:
> >>
> >>   #ifdef __HAVE_BUILTIN_BSWAP16__
> >>   #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
> >>   #else
> >>   #define __swab16(x)                             \
> >>           (__builtin_constant_p((__u16)(x)) ?     \
> >>           ___constant_swab16(x) :                 \
> >>           __fswab16(x))
> >>   #endif
> >>
> >> So we can tweak selftests/bpf/bpf_endian.h and use UAPI
> >> __swab16()/__swab32().
> >>
> >> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> >> ---
> >>
> >> v2: fixed build error, reshuffled patches (Stanislav Fomichev)
> > Tested them locally with the compiler I saw the initial issues with - all
> > fine, I don't see any errors with the older gcc.
> > 
> > One last question I have is: what happens in the llvm+bpf case? Have
> > you tested that? I think LLVM has all the builtins required, but since
> > we are relying on the swab.h now (and it relies on
> > __HAVE_BUILTIN_BSWAP16__), I wonder whether this detection works
> > correctly on the llvm when targeting bpf. (sidenote: bpf_endian.h can be
> > used from both userspace and bpf programs).
> 
> Inside kernel clang compiler header (linux/compiler-clang.h) does not 
> define __HAVE_BUILTIN_BSWAP16__. So it will go to the "else" branch in 
> the above. So I think it should work with clang + bpf.
Hm, isn't it the opposite of what we want then? I think for llvm+bpf we always
want to use the builtins to make it properly generate
BPF_TO_BE/BPF_TO_LE instructions.

> > 
> >>
> >>   tools/testing/selftests/bpf/bpf_endian.h | 8 ++++----
> >>   1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/tools/testing/selftests/bpf/bpf_endian.h b/tools/testing/selftests/bpf/bpf_endian.h
> >> index b25595ea4a78..1ed268b2002b 100644
> >> --- a/tools/testing/selftests/bpf/bpf_endian.h
> >> +++ b/tools/testing/selftests/bpf/bpf_endian.h
> >> @@ -20,12 +20,12 @@
> >>    * use different targets.
> >>    */
> >>   #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> >> -# define __bpf_ntohs(x)			__builtin_bswap16(x)
> >> -# define __bpf_htons(x)			__builtin_bswap16(x)
> >> +# define __bpf_ntohs(x)			__swab16(x)
> >> +# define __bpf_htons(x)			__swab16(x)
> >>   # define __bpf_constant_ntohs(x)	___constant_swab16(x)
> >>   # define __bpf_constant_htons(x)	___constant_swab16(x)
> >> -# define __bpf_ntohl(x)			__builtin_bswap32(x)
> >> -# define __bpf_htonl(x)			__builtin_bswap32(x)
> >> +# define __bpf_ntohl(x)			__swab32(x)
> >> +# define __bpf_htonl(x)			__swab32(x)
> >>   # define __bpf_constant_ntohl(x)	___constant_swab32(x)
> >>   # define __bpf_constant_htonl(x)	___constant_swab32(x)
> >>   #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> >> -- 
> >> 2.21.0
> >>

[PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[sdf]

On 03/20, Yonghong Song wrote:
> 
> 
> On 3/20/19 10:13 AM, Stanislav Fomichev wrote:
> > On 03/20, Sergey Senozhatsky wrote:
> >> Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
> >> thus not all compilers are able to compile the following code:
> >>
> >>          (__builtin_constant_p(x) ? \
> >>                  ___constant_swab16(x) : __builtin_bswap16(x))
> >>
> >> That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
> >> instance:
> >>
> >>          error: implicit declaration of function '__builtin_bswap16'
> >>
> >> We can use __builtin_bswap16() only if compiler has this built-in,
> >> that is, only if __HAVE_BUILTIN_BSWAP16__ is defined. Standard UAPI
> >> __swab16()/__swab32() take care of that, and, additionally, handle
> >> __builtin_constant_p() cases as well:
> >>
> >>   #ifdef __HAVE_BUILTIN_BSWAP16__
> >>   #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
> >>   #else
> >>   #define __swab16(x)                             \
> >>           (__builtin_constant_p((__u16)(x)) ?     \
> >>           ___constant_swab16(x) :                 \
> >>           __fswab16(x))
> >>   #endif
> >>
> >> So we can tweak selftests/bpf/bpf_endian.h and use UAPI
> >> __swab16()/__swab32().
> >>
> >> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky at gmail.com>
> >> ---
> >>
> >> v2: fixed build error, reshuffled patches (Stanislav Fomichev)
> > Tested them locally with the compiler I saw the initial issues with - all
> > fine, I don't see any errors with the older gcc.
> > 
> > One last question I have is: what happens in the llvm+bpf case? Have
> > you tested that? I think LLVM has all the builtins required, but since
> > we are relying on the swab.h now (and it relies on
> > __HAVE_BUILTIN_BSWAP16__), I wonder whether this detection works
> > correctly on the llvm when targeting bpf. (sidenote: bpf_endian.h can be
> > used from both userspace and bpf programs).
> 
> Inside kernel clang compiler header (linux/compiler-clang.h) does not 
> define __HAVE_BUILTIN_BSWAP16__. So it will go to the "else" branch in 
> the above. So I think it should work with clang + bpf.
Hm, isn't it the opposite of what we want then? I think for llvm+bpf we always
want to use the builtins to make it properly generate
BPF_TO_BE/BPF_TO_LE instructions.

> > 
> >>
> >>   tools/testing/selftests/bpf/bpf_endian.h | 8 ++++----
> >>   1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/tools/testing/selftests/bpf/bpf_endian.h b/tools/testing/selftests/bpf/bpf_endian.h
> >> index b25595ea4a78..1ed268b2002b 100644
> >> --- a/tools/testing/selftests/bpf/bpf_endian.h
> >> +++ b/tools/testing/selftests/bpf/bpf_endian.h
> >> @@ -20,12 +20,12 @@
> >>    * use different targets.
> >>    */
> >>   #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> >> -# define __bpf_ntohs(x)			__builtin_bswap16(x)
> >> -# define __bpf_htons(x)			__builtin_bswap16(x)
> >> +# define __bpf_ntohs(x)			__swab16(x)
> >> +# define __bpf_htons(x)			__swab16(x)
> >>   # define __bpf_constant_ntohs(x)	___constant_swab16(x)
> >>   # define __bpf_constant_htons(x)	___constant_swab16(x)
> >> -# define __bpf_ntohl(x)			__builtin_bswap32(x)
> >> -# define __bpf_htonl(x)			__builtin_bswap32(x)
> >> +# define __bpf_ntohl(x)			__swab32(x)
> >> +# define __bpf_htonl(x)			__swab32(x)
> >>   # define __bpf_constant_ntohl(x)	___constant_swab32(x)
> >>   # define __bpf_constant_htonl(x)	___constant_swab32(x)
> >>   #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> >> -- 
> >> 2.21.0
> >>

[PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[Stanislav Fomichev]

On 03/20, Yonghong Song wrote:
> 
> 
> On 3/20/19 10:13 AM, Stanislav Fomichev wrote:
> > On 03/20, Sergey Senozhatsky wrote:
> >> Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
> >> thus not all compilers are able to compile the following code:
> >>
> >>          (__builtin_constant_p(x) ? \
> >>                  ___constant_swab16(x) : __builtin_bswap16(x))
> >>
> >> That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
> >> instance:
> >>
> >>          error: implicit declaration of function '__builtin_bswap16'
> >>
> >> We can use __builtin_bswap16() only if compiler has this built-in,
> >> that is, only if __HAVE_BUILTIN_BSWAP16__ is defined. Standard UAPI
> >> __swab16()/__swab32() take care of that, and, additionally, handle
> >> __builtin_constant_p() cases as well:
> >>
> >>   #ifdef __HAVE_BUILTIN_BSWAP16__
> >>   #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
> >>   #else
> >>   #define __swab16(x)                             \
> >>           (__builtin_constant_p((__u16)(x)) ?     \
> >>           ___constant_swab16(x) :                 \
> >>           __fswab16(x))
> >>   #endif
> >>
> >> So we can tweak selftests/bpf/bpf_endian.h and use UAPI
> >> __swab16()/__swab32().
> >>
> >> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky at gmail.com>
> >> ---
> >>
> >> v2: fixed build error, reshuffled patches (Stanislav Fomichev)
> > Tested them locally with the compiler I saw the initial issues with - all
> > fine, I don't see any errors with the older gcc.
> > 
> > One last question I have is: what happens in the llvm+bpf case? Have
> > you tested that? I think LLVM has all the builtins required, but since
> > we are relying on the swab.h now (and it relies on
> > __HAVE_BUILTIN_BSWAP16__), I wonder whether this detection works
> > correctly on the llvm when targeting bpf. (sidenote: bpf_endian.h can be
> > used from both userspace and bpf programs).
> 
> Inside kernel clang compiler header (linux/compiler-clang.h) does not 
> define __HAVE_BUILTIN_BSWAP16__. So it will go to the "else" branch in 
> the above. So I think it should work with clang + bpf.
Hm, isn't it the opposite of what we want then? I think for llvm+bpf we always
want to use the builtins to make it properly generate
BPF_TO_BE/BPF_TO_LE instructions.

> > 
> >>
> >>   tools/testing/selftests/bpf/bpf_endian.h | 8 ++++----
> >>   1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/tools/testing/selftests/bpf/bpf_endian.h b/tools/testing/selftests/bpf/bpf_endian.h
> >> index b25595ea4a78..1ed268b2002b 100644
> >> --- a/tools/testing/selftests/bpf/bpf_endian.h
> >> +++ b/tools/testing/selftests/bpf/bpf_endian.h
> >> @@ -20,12 +20,12 @@
> >>    * use different targets.
> >>    */
> >>   #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> >> -# define __bpf_ntohs(x)			__builtin_bswap16(x)
> >> -# define __bpf_htons(x)			__builtin_bswap16(x)
> >> +# define __bpf_ntohs(x)			__swab16(x)
> >> +# define __bpf_htons(x)			__swab16(x)
> >>   # define __bpf_constant_ntohs(x)	___constant_swab16(x)
> >>   # define __bpf_constant_htons(x)	___constant_swab16(x)
> >> -# define __bpf_ntohl(x)			__builtin_bswap32(x)
> >> -# define __bpf_htonl(x)			__builtin_bswap32(x)
> >> +# define __bpf_ntohl(x)			__swab32(x)
> >> +# define __bpf_htonl(x)			__swab32(x)
> >>   # define __bpf_constant_ntohl(x)	___constant_swab32(x)
> >>   # define __bpf_constant_htonl(x)	___constant_swab32(x)
> >>   #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> >> -- 
> >> 2.21.0
> >>

Re: [PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[Yonghong Song]

On 3/20/19 3:27 PM, Stanislav Fomichev wrote:
> On 03/20, Yonghong Song wrote:
>>
>>
>> On 3/20/19 10:13 AM, Stanislav Fomichev wrote:
>>> On 03/20, Sergey Senozhatsky wrote:
>>>> Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
>>>> thus not all compilers are able to compile the following code:
>>>>
>>>>           (__builtin_constant_p(x) ? \
>>>>                   ___constant_swab16(x) : __builtin_bswap16(x))
>>>>
>>>> That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
>>>> instance:
>>>>
>>>>           error: implicit declaration of function '__builtin_bswap16'
>>>>
>>>> We can use __builtin_bswap16() only if compiler has this built-in,
>>>> that is, only if __HAVE_BUILTIN_BSWAP16__ is defined. Standard UAPI
>>>> __swab16()/__swab32() take care of that, and, additionally, handle
>>>> __builtin_constant_p() cases as well:
>>>>
>>>>    #ifdef __HAVE_BUILTIN_BSWAP16__
>>>>    #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
>>>>    #else
>>>>    #define __swab16(x)                             \
>>>>            (__builtin_constant_p((__u16)(x)) ?     \
>>>>            ___constant_swab16(x) :                 \
>>>>            __fswab16(x))
>>>>    #endif
>>>>
>>>> So we can tweak selftests/bpf/bpf_endian.h and use UAPI
>>>> __swab16()/__swab32().
>>>>
>>>> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
>>>> ---
>>>>
>>>> v2: fixed build error, reshuffled patches (Stanislav Fomichev)
>>> Tested them locally with the compiler I saw the initial issues with - all
>>> fine, I don't see any errors with the older gcc.
>>>
>>> One last question I have is: what happens in the llvm+bpf case? Have
>>> you tested that? I think LLVM has all the builtins required, but since
>>> we are relying on the swab.h now (and it relies on
>>> __HAVE_BUILTIN_BSWAP16__), I wonder whether this detection works
>>> correctly on the llvm when targeting bpf. (sidenote: bpf_endian.h can be
>>> used from both userspace and bpf programs).
>>
>> Inside kernel clang compiler header (linux/compiler-clang.h) does not
>> define __HAVE_BUILTIN_BSWAP16__. So it will go to the "else" branch in
>> the above. So I think it should work with clang + bpf.
> Hm, isn't it the opposite of what we want then? I think for llvm+bpf we always
> want to use the builtins to make it properly generate
> BPF_TO_BE/BPF_TO_LE instructions.

Okay, I see. Then this patch will not achieve that.
The following are two common ways to compile a bpf program:
   - "clang -target bpf ...", maybe add macro __BPF__ somewhere
     to indicate builtin_bswap16 always available?
   - "clang <host target> ..." and then "llc -march=bpf ..."
     in this case, __BPF__ macro is not available and
     we will not be able to use builtin swap for bpf program.

Maybe use __clang__ macro (or gcc macro) to distinguish between clang 
and gcc. If it is gcc we will check builtin availability, otherwise,
we assume builtin always available? This not pretty though.

> 
>>>
>>>>
>>>>    tools/testing/selftests/bpf/bpf_endian.h | 8 ++++----
>>>>    1 file changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/tools/testing/selftests/bpf/bpf_endian.h b/tools/testing/selftests/bpf/bpf_endian.h
>>>> index b25595ea4a78..1ed268b2002b 100644
>>>> --- a/tools/testing/selftests/bpf/bpf_endian.h
>>>> +++ b/tools/testing/selftests/bpf/bpf_endian.h
>>>> @@ -20,12 +20,12 @@
>>>>     * use different targets.
>>>>     */
>>>>    #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
>>>> -# define __bpf_ntohs(x)			__builtin_bswap16(x)
>>>> -# define __bpf_htons(x)			__builtin_bswap16(x)
>>>> +# define __bpf_ntohs(x)			__swab16(x)
>>>> +# define __bpf_htons(x)			__swab16(x)
>>>>    # define __bpf_constant_ntohs(x)	___constant_swab16(x)
>>>>    # define __bpf_constant_htons(x)	___constant_swab16(x)
>>>> -# define __bpf_ntohl(x)			__builtin_bswap32(x)
>>>> -# define __bpf_htonl(x)			__builtin_bswap32(x)
>>>> +# define __bpf_ntohl(x)			__swab32(x)
>>>> +# define __bpf_htonl(x)			__swab32(x)
>>>>    # define __bpf_constant_ntohl(x)	___constant_swab32(x)
>>>>    # define __bpf_constant_htonl(x)	___constant_swab32(x)
>>>>    #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
>>>> -- 
>>>> 2.21.0
>>>>

[PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[yhs]

On 3/20/19 3:27 PM, Stanislav Fomichev wrote:
> On 03/20, Yonghong Song wrote:
>>
>>
>> On 3/20/19 10:13 AM, Stanislav Fomichev wrote:
>>> On 03/20, Sergey Senozhatsky wrote:
>>>> Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
>>>> thus not all compilers are able to compile the following code:
>>>>
>>>>           (__builtin_constant_p(x) ? \
>>>>                   ___constant_swab16(x) : __builtin_bswap16(x))
>>>>
>>>> That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
>>>> instance:
>>>>
>>>>           error: implicit declaration of function '__builtin_bswap16'
>>>>
>>>> We can use __builtin_bswap16() only if compiler has this built-in,
>>>> that is, only if __HAVE_BUILTIN_BSWAP16__ is defined. Standard UAPI
>>>> __swab16()/__swab32() take care of that, and, additionally, handle
>>>> __builtin_constant_p() cases as well:
>>>>
>>>>    #ifdef __HAVE_BUILTIN_BSWAP16__
>>>>    #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
>>>>    #else
>>>>    #define __swab16(x)                             \
>>>>            (__builtin_constant_p((__u16)(x)) ?     \
>>>>            ___constant_swab16(x) :                 \
>>>>            __fswab16(x))
>>>>    #endif
>>>>
>>>> So we can tweak selftests/bpf/bpf_endian.h and use UAPI
>>>> __swab16()/__swab32().
>>>>
>>>> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky at gmail.com>
>>>> ---
>>>>
>>>> v2: fixed build error, reshuffled patches (Stanislav Fomichev)
>>> Tested them locally with the compiler I saw the initial issues with - all
>>> fine, I don't see any errors with the older gcc.
>>>
>>> One last question I have is: what happens in the llvm+bpf case? Have
>>> you tested that? I think LLVM has all the builtins required, but since
>>> we are relying on the swab.h now (and it relies on
>>> __HAVE_BUILTIN_BSWAP16__), I wonder whether this detection works
>>> correctly on the llvm when targeting bpf. (sidenote: bpf_endian.h can be
>>> used from both userspace and bpf programs).
>>
>> Inside kernel clang compiler header (linux/compiler-clang.h) does not
>> define __HAVE_BUILTIN_BSWAP16__. So it will go to the "else" branch in
>> the above. So I think it should work with clang + bpf.
> Hm, isn't it the opposite of what we want then? I think for llvm+bpf we always
> want to use the builtins to make it properly generate
> BPF_TO_BE/BPF_TO_LE instructions.

Okay, I see. Then this patch will not achieve that.
The following are two common ways to compile a bpf program:
   - "clang -target bpf ...", maybe add macro __BPF__ somewhere
     to indicate builtin_bswap16 always available?
   - "clang <host target> ..." and then "llc -march=bpf ..."
     in this case, __BPF__ macro is not available and
     we will not be able to use builtin swap for bpf program.

Maybe use __clang__ macro (or gcc macro) to distinguish between clang 
and gcc. If it is gcc we will check builtin availability, otherwise,
we assume builtin always available? This not pretty though.

> 
>>>
>>>>
>>>>    tools/testing/selftests/bpf/bpf_endian.h | 8 ++++----
>>>>    1 file changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/tools/testing/selftests/bpf/bpf_endian.h b/tools/testing/selftests/bpf/bpf_endian.h
>>>> index b25595ea4a78..1ed268b2002b 100644
>>>> --- a/tools/testing/selftests/bpf/bpf_endian.h
>>>> +++ b/tools/testing/selftests/bpf/bpf_endian.h
>>>> @@ -20,12 +20,12 @@
>>>>     * use different targets.
>>>>     */
>>>>    #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
>>>> -# define __bpf_ntohs(x)			__builtin_bswap16(x)
>>>> -# define __bpf_htons(x)			__builtin_bswap16(x)
>>>> +# define __bpf_ntohs(x)			__swab16(x)
>>>> +# define __bpf_htons(x)			__swab16(x)
>>>>    # define __bpf_constant_ntohs(x)	___constant_swab16(x)
>>>>    # define __bpf_constant_htons(x)	___constant_swab16(x)
>>>> -# define __bpf_ntohl(x)			__builtin_bswap32(x)
>>>> -# define __bpf_htonl(x)			__builtin_bswap32(x)
>>>> +# define __bpf_ntohl(x)			__swab32(x)
>>>> +# define __bpf_htonl(x)			__swab32(x)
>>>>    # define __bpf_constant_ntohl(x)	___constant_swab32(x)
>>>>    # define __bpf_constant_htonl(x)	___constant_swab32(x)
>>>>    #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
>>>> -- 
>>>> 2.21.0
>>>>

[PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[Yonghong Song]

On 3/20/19 3:27 PM, Stanislav Fomichev wrote:
> On 03/20, Yonghong Song wrote:
>>
>>
>> On 3/20/19 10:13 AM, Stanislav Fomichev wrote:
>>> On 03/20, Sergey Senozhatsky wrote:
>>>> Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
>>>> thus not all compilers are able to compile the following code:
>>>>
>>>>           (__builtin_constant_p(x) ? \
>>>>                   ___constant_swab16(x) : __builtin_bswap16(x))
>>>>
>>>> That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
>>>> instance:
>>>>
>>>>           error: implicit declaration of function '__builtin_bswap16'
>>>>
>>>> We can use __builtin_bswap16() only if compiler has this built-in,
>>>> that is, only if __HAVE_BUILTIN_BSWAP16__ is defined. Standard UAPI
>>>> __swab16()/__swab32() take care of that, and, additionally, handle
>>>> __builtin_constant_p() cases as well:
>>>>
>>>>    #ifdef __HAVE_BUILTIN_BSWAP16__
>>>>    #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
>>>>    #else
>>>>    #define __swab16(x)                             \
>>>>            (__builtin_constant_p((__u16)(x)) ?     \
>>>>            ___constant_swab16(x) :                 \
>>>>            __fswab16(x))
>>>>    #endif
>>>>
>>>> So we can tweak selftests/bpf/bpf_endian.h and use UAPI
>>>> __swab16()/__swab32().
>>>>
>>>> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky at gmail.com>
>>>> ---
>>>>
>>>> v2: fixed build error, reshuffled patches (Stanislav Fomichev)
>>> Tested them locally with the compiler I saw the initial issues with - all
>>> fine, I don't see any errors with the older gcc.
>>>
>>> One last question I have is: what happens in the llvm+bpf case? Have
>>> you tested that? I think LLVM has all the builtins required, but since
>>> we are relying on the swab.h now (and it relies on
>>> __HAVE_BUILTIN_BSWAP16__), I wonder whether this detection works
>>> correctly on the llvm when targeting bpf. (sidenote: bpf_endian.h can be
>>> used from both userspace and bpf programs).
>>
>> Inside kernel clang compiler header (linux/compiler-clang.h) does not
>> define __HAVE_BUILTIN_BSWAP16__. So it will go to the "else" branch in
>> the above. So I think it should work with clang + bpf.
> Hm, isn't it the opposite of what we want then? I think for llvm+bpf we always
> want to use the builtins to make it properly generate
> BPF_TO_BE/BPF_TO_LE instructions.

Okay, I see. Then this patch will not achieve that.
The following are two common ways to compile a bpf program:
   - "clang -target bpf ...", maybe add macro __BPF__ somewhere
     to indicate builtin_bswap16 always available?
   - "clang <host target> ..." and then "llc -march=bpf ..."
     in this case, __BPF__ macro is not available and
     we will not be able to use builtin swap for bpf program.

Maybe use __clang__ macro (or gcc macro) to distinguish between clang 
and gcc. If it is gcc we will check builtin availability, otherwise,
we assume builtin always available? This not pretty though.

> 
>>>
>>>>
>>>>    tools/testing/selftests/bpf/bpf_endian.h | 8 ++++----
>>>>    1 file changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/tools/testing/selftests/bpf/bpf_endian.h b/tools/testing/selftests/bpf/bpf_endian.h
>>>> index b25595ea4a78..1ed268b2002b 100644
>>>> --- a/tools/testing/selftests/bpf/bpf_endian.h
>>>> +++ b/tools/testing/selftests/bpf/bpf_endian.h
>>>> @@ -20,12 +20,12 @@
>>>>     * use different targets.
>>>>     */
>>>>    #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
>>>> -# define __bpf_ntohs(x)			__builtin_bswap16(x)
>>>> -# define __bpf_htons(x)			__builtin_bswap16(x)
>>>> +# define __bpf_ntohs(x)			__swab16(x)
>>>> +# define __bpf_htons(x)			__swab16(x)
>>>>    # define __bpf_constant_ntohs(x)	___constant_swab16(x)
>>>>    # define __bpf_constant_htons(x)	___constant_swab16(x)
>>>> -# define __bpf_ntohl(x)			__builtin_bswap32(x)
>>>> -# define __bpf_htonl(x)			__builtin_bswap32(x)
>>>> +# define __bpf_ntohl(x)			__swab32(x)
>>>> +# define __bpf_htonl(x)			__swab32(x)
>>>>    # define __bpf_constant_ntohl(x)	___constant_swab32(x)
>>>>    # define __bpf_constant_htonl(x)	___constant_swab32(x)
>>>>    #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
>>>> -- 
>>>> 2.21.0
>>>>

Re: [PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[Daniel Borkmann]

On 03/20/2019 11:45 PM, Yonghong Song wrote:
> On 3/20/19 3:27 PM, Stanislav Fomichev wrote:
>> On 03/20, Yonghong Song wrote:
>>> On 3/20/19 10:13 AM, Stanislav Fomichev wrote:
>>>> On 03/20, Sergey Senozhatsky wrote:
>>>>> Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
>>>>> thus not all compilers are able to compile the following code:
>>>>>
>>>>>           (__builtin_constant_p(x) ? \
>>>>>                   ___constant_swab16(x) : __builtin_bswap16(x))
>>>>>
>>>>> That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
>>>>> instance:
>>>>>
>>>>>           error: implicit declaration of function '__builtin_bswap16'
>>>>>
>>>>> We can use __builtin_bswap16() only if compiler has this built-in,
>>>>> that is, only if __HAVE_BUILTIN_BSWAP16__ is defined. Standard UAPI
>>>>> __swab16()/__swab32() take care of that, and, additionally, handle
>>>>> __builtin_constant_p() cases as well:
>>>>>
>>>>>    #ifdef __HAVE_BUILTIN_BSWAP16__
>>>>>    #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
>>>>>    #else
>>>>>    #define __swab16(x)                             \
>>>>>            (__builtin_constant_p((__u16)(x)) ?     \
>>>>>            ___constant_swab16(x) :                 \
>>>>>            __fswab16(x))
>>>>>    #endif
>>>>>
>>>>> So we can tweak selftests/bpf/bpf_endian.h and use UAPI
>>>>> __swab16()/__swab32().
>>>>>
>>>>> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
>>>>> ---
>>>>>
>>>>> v2: fixed build error, reshuffled patches (Stanislav Fomichev)
>>>> Tested them locally with the compiler I saw the initial issues with - all
>>>> fine, I don't see any errors with the older gcc.
>>>>
>>>> One last question I have is: what happens in the llvm+bpf case? Have
>>>> you tested that? I think LLVM has all the builtins required, but since
>>>> we are relying on the swab.h now (and it relies on
>>>> __HAVE_BUILTIN_BSWAP16__), I wonder whether this detection works
>>>> correctly on the llvm when targeting bpf. (sidenote: bpf_endian.h can be
>>>> used from both userspace and bpf programs).
>>>
>>> Inside kernel clang compiler header (linux/compiler-clang.h) does not
>>> define __HAVE_BUILTIN_BSWAP16__. So it will go to the "else" branch in
>>> the above. So I think it should work with clang + bpf.
>> Hm, isn't it the opposite of what we want then? I think for llvm+bpf we always
>> want to use the builtins to make it properly generate
>> BPF_TO_BE/BPF_TO_LE instructions.
> 
> Okay, I see. Then this patch will not achieve that.
> The following are two common ways to compile a bpf program:
>    - "clang -target bpf ...", maybe add macro __BPF__ somewhere
>      to indicate builtin_bswap16 always available?
>    - "clang <host target> ..." and then "llc -march=bpf ..."
>      in this case, __BPF__ macro is not available and
>      we will not be able to use builtin swap for bpf program.
> 
> Maybe use __clang__ macro (or gcc macro) to distinguish between clang 
> and gcc. If it is gcc we will check builtin availability, otherwise,
> we assume builtin always available? This not pretty though.

I think the way this should be fixed is the following: In case
of LLVM (aka compiling BPF prog), we want the code to be as-is,
in case if gcc is compiling the hostprog, we either want to keep
using __builtin_bswap16() or fall-back to something else. Thus,
I would suggest, we add a new feature test for tooling infra under
tools/build/feature/ that compiles a dummy prog with __builtin_bswap16().
And in the bpf_endian.h we define __bpf_ntohs(x) to __bpf_swab16(x)
which either resolves to __builtin_bswap16() or some fallback
implementation if not available. I don't think there should be much
of an issue and it would follow the standard way to do it.

>>>>>    tools/testing/selftests/bpf/bpf_endian.h | 8 ++++----
>>>>>    1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/tools/testing/selftests/bpf/bpf_endian.h b/tools/testing/selftests/bpf/bpf_endian.h
>>>>> index b25595ea4a78..1ed268b2002b 100644
>>>>> --- a/tools/testing/selftests/bpf/bpf_endian.h
>>>>> +++ b/tools/testing/selftests/bpf/bpf_endian.h
>>>>> @@ -20,12 +20,12 @@
>>>>>     * use different targets.
>>>>>     */
>>>>>    #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
>>>>> -# define __bpf_ntohs(x)			__builtin_bswap16(x)
>>>>> -# define __bpf_htons(x)			__builtin_bswap16(x)
>>>>> +# define __bpf_ntohs(x)			__swab16(x)
>>>>> +# define __bpf_htons(x)			__swab16(x)
>>>>>    # define __bpf_constant_ntohs(x)	___constant_swab16(x)
>>>>>    # define __bpf_constant_htons(x)	___constant_swab16(x)
>>>>> -# define __bpf_ntohl(x)			__builtin_bswap32(x)
>>>>> -# define __bpf_htonl(x)			__builtin_bswap32(x)
>>>>> +# define __bpf_ntohl(x)			__swab32(x)
>>>>> +# define __bpf_htonl(x)			__swab32(x)
>>>>>    # define __bpf_constant_ntohl(x)	___constant_swab32(x)
>>>>>    # define __bpf_constant_htonl(x)	___constant_swab32(x)
>>>>>    #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
>>>>> -- 
>>>>> 2.21.0
>>>>>

[PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[daniel]

On 03/20/2019 11:45 PM, Yonghong Song wrote:
> On 3/20/19 3:27 PM, Stanislav Fomichev wrote:
>> On 03/20, Yonghong Song wrote:
>>> On 3/20/19 10:13 AM, Stanislav Fomichev wrote:
>>>> On 03/20, Sergey Senozhatsky wrote:
>>>>> Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
>>>>> thus not all compilers are able to compile the following code:
>>>>>
>>>>>           (__builtin_constant_p(x) ? \
>>>>>                   ___constant_swab16(x) : __builtin_bswap16(x))
>>>>>
>>>>> That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
>>>>> instance:
>>>>>
>>>>>           error: implicit declaration of function '__builtin_bswap16'
>>>>>
>>>>> We can use __builtin_bswap16() only if compiler has this built-in,
>>>>> that is, only if __HAVE_BUILTIN_BSWAP16__ is defined. Standard UAPI
>>>>> __swab16()/__swab32() take care of that, and, additionally, handle
>>>>> __builtin_constant_p() cases as well:
>>>>>
>>>>>    #ifdef __HAVE_BUILTIN_BSWAP16__
>>>>>    #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
>>>>>    #else
>>>>>    #define __swab16(x)                             \
>>>>>            (__builtin_constant_p((__u16)(x)) ?     \
>>>>>            ___constant_swab16(x) :                 \
>>>>>            __fswab16(x))
>>>>>    #endif
>>>>>
>>>>> So we can tweak selftests/bpf/bpf_endian.h and use UAPI
>>>>> __swab16()/__swab32().
>>>>>
>>>>> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky at gmail.com>
>>>>> ---
>>>>>
>>>>> v2: fixed build error, reshuffled patches (Stanislav Fomichev)
>>>> Tested them locally with the compiler I saw the initial issues with - all
>>>> fine, I don't see any errors with the older gcc.
>>>>
>>>> One last question I have is: what happens in the llvm+bpf case? Have
>>>> you tested that? I think LLVM has all the builtins required, but since
>>>> we are relying on the swab.h now (and it relies on
>>>> __HAVE_BUILTIN_BSWAP16__), I wonder whether this detection works
>>>> correctly on the llvm when targeting bpf. (sidenote: bpf_endian.h can be
>>>> used from both userspace and bpf programs).
>>>
>>> Inside kernel clang compiler header (linux/compiler-clang.h) does not
>>> define __HAVE_BUILTIN_BSWAP16__. So it will go to the "else" branch in
>>> the above. So I think it should work with clang + bpf.
>> Hm, isn't it the opposite of what we want then? I think for llvm+bpf we always
>> want to use the builtins to make it properly generate
>> BPF_TO_BE/BPF_TO_LE instructions.
> 
> Okay, I see. Then this patch will not achieve that.
> The following are two common ways to compile a bpf program:
>    - "clang -target bpf ...", maybe add macro __BPF__ somewhere
>      to indicate builtin_bswap16 always available?
>    - "clang <host target> ..." and then "llc -march=bpf ..."
>      in this case, __BPF__ macro is not available and
>      we will not be able to use builtin swap for bpf program.
> 
> Maybe use __clang__ macro (or gcc macro) to distinguish between clang 
> and gcc. If it is gcc we will check builtin availability, otherwise,
> we assume builtin always available? This not pretty though.

I think the way this should be fixed is the following: In case
of LLVM (aka compiling BPF prog), we want the code to be as-is,
in case if gcc is compiling the hostprog, we either want to keep
using __builtin_bswap16() or fall-back to something else. Thus,
I would suggest, we add a new feature test for tooling infra under
tools/build/feature/ that compiles a dummy prog with __builtin_bswap16().
And in the bpf_endian.h we define __bpf_ntohs(x) to __bpf_swab16(x)
which either resolves to __builtin_bswap16() or some fallback
implementation if not available. I don't think there should be much
of an issue and it would follow the standard way to do it.

>>>>>    tools/testing/selftests/bpf/bpf_endian.h | 8 ++++----
>>>>>    1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/tools/testing/selftests/bpf/bpf_endian.h b/tools/testing/selftests/bpf/bpf_endian.h
>>>>> index b25595ea4a78..1ed268b2002b 100644
>>>>> --- a/tools/testing/selftests/bpf/bpf_endian.h
>>>>> +++ b/tools/testing/selftests/bpf/bpf_endian.h
>>>>> @@ -20,12 +20,12 @@
>>>>>     * use different targets.
>>>>>     */
>>>>>    #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
>>>>> -# define __bpf_ntohs(x)			__builtin_bswap16(x)
>>>>> -# define __bpf_htons(x)			__builtin_bswap16(x)
>>>>> +# define __bpf_ntohs(x)			__swab16(x)
>>>>> +# define __bpf_htons(x)			__swab16(x)
>>>>>    # define __bpf_constant_ntohs(x)	___constant_swab16(x)
>>>>>    # define __bpf_constant_htons(x)	___constant_swab16(x)
>>>>> -# define __bpf_ntohl(x)			__builtin_bswap32(x)
>>>>> -# define __bpf_htonl(x)			__builtin_bswap32(x)
>>>>> +# define __bpf_ntohl(x)			__swab32(x)
>>>>> +# define __bpf_htonl(x)			__swab32(x)
>>>>>    # define __bpf_constant_ntohl(x)	___constant_swab32(x)
>>>>>    # define __bpf_constant_htonl(x)	___constant_swab32(x)
>>>>>    #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
>>>>> -- 
>>>>> 2.21.0
>>>>>

[PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[Daniel Borkmann]

On 03/20/2019 11:45 PM, Yonghong Song wrote:
> On 3/20/19 3:27 PM, Stanislav Fomichev wrote:
>> On 03/20, Yonghong Song wrote:
>>> On 3/20/19 10:13 AM, Stanislav Fomichev wrote:
>>>> On 03/20, Sergey Senozhatsky wrote:
>>>>> Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
>>>>> thus not all compilers are able to compile the following code:
>>>>>
>>>>>           (__builtin_constant_p(x) ? \
>>>>>                   ___constant_swab16(x) : __builtin_bswap16(x))
>>>>>
>>>>> That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
>>>>> instance:
>>>>>
>>>>>           error: implicit declaration of function '__builtin_bswap16'
>>>>>
>>>>> We can use __builtin_bswap16() only if compiler has this built-in,
>>>>> that is, only if __HAVE_BUILTIN_BSWAP16__ is defined. Standard UAPI
>>>>> __swab16()/__swab32() take care of that, and, additionally, handle
>>>>> __builtin_constant_p() cases as well:
>>>>>
>>>>>    #ifdef __HAVE_BUILTIN_BSWAP16__
>>>>>    #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
>>>>>    #else
>>>>>    #define __swab16(x)                             \
>>>>>            (__builtin_constant_p((__u16)(x)) ?     \
>>>>>            ___constant_swab16(x) :                 \
>>>>>            __fswab16(x))
>>>>>    #endif
>>>>>
>>>>> So we can tweak selftests/bpf/bpf_endian.h and use UAPI
>>>>> __swab16()/__swab32().
>>>>>
>>>>> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky at gmail.com>
>>>>> ---
>>>>>
>>>>> v2: fixed build error, reshuffled patches (Stanislav Fomichev)
>>>> Tested them locally with the compiler I saw the initial issues with - all
>>>> fine, I don't see any errors with the older gcc.
>>>>
>>>> One last question I have is: what happens in the llvm+bpf case? Have
>>>> you tested that? I think LLVM has all the builtins required, but since
>>>> we are relying on the swab.h now (and it relies on
>>>> __HAVE_BUILTIN_BSWAP16__), I wonder whether this detection works
>>>> correctly on the llvm when targeting bpf. (sidenote: bpf_endian.h can be
>>>> used from both userspace and bpf programs).
>>>
>>> Inside kernel clang compiler header (linux/compiler-clang.h) does not
>>> define __HAVE_BUILTIN_BSWAP16__. So it will go to the "else" branch in
>>> the above. So I think it should work with clang + bpf.
>> Hm, isn't it the opposite of what we want then? I think for llvm+bpf we always
>> want to use the builtins to make it properly generate
>> BPF_TO_BE/BPF_TO_LE instructions.
> 
> Okay, I see. Then this patch will not achieve that.
> The following are two common ways to compile a bpf program:
>    - "clang -target bpf ...", maybe add macro __BPF__ somewhere
>      to indicate builtin_bswap16 always available?
>    - "clang <host target> ..." and then "llc -march=bpf ..."
>      in this case, __BPF__ macro is not available and
>      we will not be able to use builtin swap for bpf program.
> 
> Maybe use __clang__ macro (or gcc macro) to distinguish between clang 
> and gcc. If it is gcc we will check builtin availability, otherwise,
> we assume builtin always available? This not pretty though.

I think the way this should be fixed is the following: In case
of LLVM (aka compiling BPF prog), we want the code to be as-is,
in case if gcc is compiling the hostprog, we either want to keep
using __builtin_bswap16() or fall-back to something else. Thus,
I would suggest, we add a new feature test for tooling infra under
tools/build/feature/ that compiles a dummy prog with __builtin_bswap16().
And in the bpf_endian.h we define __bpf_ntohs(x) to __bpf_swab16(x)
which either resolves to __builtin_bswap16() or some fallback
implementation if not available. I don't think there should be much
of an issue and it would follow the standard way to do it.

>>>>>    tools/testing/selftests/bpf/bpf_endian.h | 8 ++++----
>>>>>    1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/tools/testing/selftests/bpf/bpf_endian.h b/tools/testing/selftests/bpf/bpf_endian.h
>>>>> index b25595ea4a78..1ed268b2002b 100644
>>>>> --- a/tools/testing/selftests/bpf/bpf_endian.h
>>>>> +++ b/tools/testing/selftests/bpf/bpf_endian.h
>>>>> @@ -20,12 +20,12 @@
>>>>>     * use different targets.
>>>>>     */
>>>>>    #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
>>>>> -# define __bpf_ntohs(x)			__builtin_bswap16(x)
>>>>> -# define __bpf_htons(x)			__builtin_bswap16(x)
>>>>> +# define __bpf_ntohs(x)			__swab16(x)
>>>>> +# define __bpf_htons(x)			__swab16(x)
>>>>>    # define __bpf_constant_ntohs(x)	___constant_swab16(x)
>>>>>    # define __bpf_constant_htons(x)	___constant_swab16(x)
>>>>> -# define __bpf_ntohl(x)			__builtin_bswap32(x)
>>>>> -# define __bpf_htonl(x)			__builtin_bswap32(x)
>>>>> +# define __bpf_ntohl(x)			__swab32(x)
>>>>> +# define __bpf_htonl(x)			__swab32(x)
>>>>>    # define __bpf_constant_ntohl(x)	___constant_swab32(x)
>>>>>    # define __bpf_constant_htonl(x)	___constant_swab32(x)
>>>>>    #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
>>>>> -- 
>>>>> 2.21.0
>>>>>

Re: [PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[Stanislav Fomichev]

On 03/21, Daniel Borkmann wrote:
> On 03/20/2019 11:45 PM, Yonghong Song wrote:
> > On 3/20/19 3:27 PM, Stanislav Fomichev wrote:
> >> On 03/20, Yonghong Song wrote:
> >>> On 3/20/19 10:13 AM, Stanislav Fomichev wrote:
> >>>> On 03/20, Sergey Senozhatsky wrote:
> >>>>> Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
> >>>>> thus not all compilers are able to compile the following code:
> >>>>>
> >>>>>           (__builtin_constant_p(x) ? \
> >>>>>                   ___constant_swab16(x) : __builtin_bswap16(x))
> >>>>>
> >>>>> That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
> >>>>> instance:
> >>>>>
> >>>>>           error: implicit declaration of function '__builtin_bswap16'
> >>>>>
> >>>>> We can use __builtin_bswap16() only if compiler has this built-in,
> >>>>> that is, only if __HAVE_BUILTIN_BSWAP16__ is defined. Standard UAPI
> >>>>> __swab16()/__swab32() take care of that, and, additionally, handle
> >>>>> __builtin_constant_p() cases as well:
> >>>>>
> >>>>>    #ifdef __HAVE_BUILTIN_BSWAP16__
> >>>>>    #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
> >>>>>    #else
> >>>>>    #define __swab16(x)                             \
> >>>>>            (__builtin_constant_p((__u16)(x)) ?     \
> >>>>>            ___constant_swab16(x) :                 \
> >>>>>            __fswab16(x))
> >>>>>    #endif
> >>>>>
> >>>>> So we can tweak selftests/bpf/bpf_endian.h and use UAPI
> >>>>> __swab16()/__swab32().
> >>>>>
> >>>>> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> >>>>> ---
> >>>>>
> >>>>> v2: fixed build error, reshuffled patches (Stanislav Fomichev)
> >>>> Tested them locally with the compiler I saw the initial issues with - all
> >>>> fine, I don't see any errors with the older gcc.
> >>>>
> >>>> One last question I have is: what happens in the llvm+bpf case? Have
> >>>> you tested that? I think LLVM has all the builtins required, but since
> >>>> we are relying on the swab.h now (and it relies on
> >>>> __HAVE_BUILTIN_BSWAP16__), I wonder whether this detection works
> >>>> correctly on the llvm when targeting bpf. (sidenote: bpf_endian.h can be
> >>>> used from both userspace and bpf programs).
> >>>
> >>> Inside kernel clang compiler header (linux/compiler-clang.h) does not
> >>> define __HAVE_BUILTIN_BSWAP16__. So it will go to the "else" branch in
> >>> the above. So I think it should work with clang + bpf.
> >> Hm, isn't it the opposite of what we want then? I think for llvm+bpf we always
> >> want to use the builtins to make it properly generate
> >> BPF_TO_BE/BPF_TO_LE instructions.
> > 
> > Okay, I see. Then this patch will not achieve that.
> > The following are two common ways to compile a bpf program:
> >    - "clang -target bpf ...", maybe add macro __BPF__ somewhere
> >      to indicate builtin_bswap16 always available?
> >    - "clang <host target> ..." and then "llc -march=bpf ..."
> >      in this case, __BPF__ macro is not available and
> >      we will not be able to use builtin swap for bpf program.
> > 
> > Maybe use __clang__ macro (or gcc macro) to distinguish between clang 
> > and gcc. If it is gcc we will check builtin availability, otherwise,
> > we assume builtin always available? This not pretty though.
> 
> I think the way this should be fixed is the following: In case
> of LLVM (aka compiling BPF prog), we want the code to be as-is,
> in case if gcc is compiling the hostprog, we either want to keep
> using __builtin_bswap16() or fall-back to something else. Thus,
> I would suggest, we add a new feature test for tooling infra under
> tools/build/feature/ that compiles a dummy prog with __builtin_bswap16().
> And in the bpf_endian.h we define __bpf_ntohs(x) to __bpf_swab16(x)
> which either resolves to __builtin_bswap16() or some fallback
> implementation if not available. I don't think there should be much
> of an issue and it would follow the standard way to do it.
It's not as easy as llvm vs gcc. We can compile userland tests with
llvm/clang as well. We really need to distinguish between the target: bfp vs
non-bpf: always use builtins in bpf case and fallback to swab.h for
userland (or use feature detection, but swab.h should be enough in
theory).

Can we rely on __bpf__ define?

$ cat tmp.c
#ifdef __bpf__
#error a
#else
#error b
#endif
$ clang -c -target bpf tmp.c 
tmp.c:2:2: error: a
#error a
 ^
 1 error generated.

> 
> >>>>>    tools/testing/selftests/bpf/bpf_endian.h | 8 ++++----
> >>>>>    1 file changed, 4 insertions(+), 4 deletions(-)
> >>>>>
> >>>>> diff --git a/tools/testing/selftests/bpf/bpf_endian.h b/tools/testing/selftests/bpf/bpf_endian.h
> >>>>> index b25595ea4a78..1ed268b2002b 100644
> >>>>> --- a/tools/testing/selftests/bpf/bpf_endian.h
> >>>>> +++ b/tools/testing/selftests/bpf/bpf_endian.h
> >>>>> @@ -20,12 +20,12 @@
> >>>>>     * use different targets.
> >>>>>     */
> >>>>>    #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> >>>>> -# define __bpf_ntohs(x)			__builtin_bswap16(x)
> >>>>> -# define __bpf_htons(x)			__builtin_bswap16(x)
> >>>>> +# define __bpf_ntohs(x)			__swab16(x)
> >>>>> +# define __bpf_htons(x)			__swab16(x)
> >>>>>    # define __bpf_constant_ntohs(x)	___constant_swab16(x)
> >>>>>    # define __bpf_constant_htons(x)	___constant_swab16(x)
> >>>>> -# define __bpf_ntohl(x)			__builtin_bswap32(x)
> >>>>> -# define __bpf_htonl(x)			__builtin_bswap32(x)
> >>>>> +# define __bpf_ntohl(x)			__swab32(x)
> >>>>> +# define __bpf_htonl(x)			__swab32(x)
> >>>>>    # define __bpf_constant_ntohl(x)	___constant_swab32(x)
> >>>>>    # define __bpf_constant_htonl(x)	___constant_swab32(x)
> >>>>>    #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> >>>>> -- 
> >>>>> 2.21.0
> >>>>>
> 

[PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[sdf]

On 03/21, Daniel Borkmann wrote:
> On 03/20/2019 11:45 PM, Yonghong Song wrote:
> > On 3/20/19 3:27 PM, Stanislav Fomichev wrote:
> >> On 03/20, Yonghong Song wrote:
> >>> On 3/20/19 10:13 AM, Stanislav Fomichev wrote:
> >>>> On 03/20, Sergey Senozhatsky wrote:
> >>>>> Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
> >>>>> thus not all compilers are able to compile the following code:
> >>>>>
> >>>>>           (__builtin_constant_p(x) ? \
> >>>>>                   ___constant_swab16(x) : __builtin_bswap16(x))
> >>>>>
> >>>>> That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
> >>>>> instance:
> >>>>>
> >>>>>           error: implicit declaration of function '__builtin_bswap16'
> >>>>>
> >>>>> We can use __builtin_bswap16() only if compiler has this built-in,
> >>>>> that is, only if __HAVE_BUILTIN_BSWAP16__ is defined. Standard UAPI
> >>>>> __swab16()/__swab32() take care of that, and, additionally, handle
> >>>>> __builtin_constant_p() cases as well:
> >>>>>
> >>>>>    #ifdef __HAVE_BUILTIN_BSWAP16__
> >>>>>    #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
> >>>>>    #else
> >>>>>    #define __swab16(x)                             \
> >>>>>            (__builtin_constant_p((__u16)(x)) ?     \
> >>>>>            ___constant_swab16(x) :                 \
> >>>>>            __fswab16(x))
> >>>>>    #endif
> >>>>>
> >>>>> So we can tweak selftests/bpf/bpf_endian.h and use UAPI
> >>>>> __swab16()/__swab32().
> >>>>>
> >>>>> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky at gmail.com>
> >>>>> ---
> >>>>>
> >>>>> v2: fixed build error, reshuffled patches (Stanislav Fomichev)
> >>>> Tested them locally with the compiler I saw the initial issues with - all
> >>>> fine, I don't see any errors with the older gcc.
> >>>>
> >>>> One last question I have is: what happens in the llvm+bpf case? Have
> >>>> you tested that? I think LLVM has all the builtins required, but since
> >>>> we are relying on the swab.h now (and it relies on
> >>>> __HAVE_BUILTIN_BSWAP16__), I wonder whether this detection works
> >>>> correctly on the llvm when targeting bpf. (sidenote: bpf_endian.h can be
> >>>> used from both userspace and bpf programs).
> >>>
> >>> Inside kernel clang compiler header (linux/compiler-clang.h) does not
> >>> define __HAVE_BUILTIN_BSWAP16__. So it will go to the "else" branch in
> >>> the above. So I think it should work with clang + bpf.
> >> Hm, isn't it the opposite of what we want then? I think for llvm+bpf we always
> >> want to use the builtins to make it properly generate
> >> BPF_TO_BE/BPF_TO_LE instructions.
> > 
> > Okay, I see. Then this patch will not achieve that.
> > The following are two common ways to compile a bpf program:
> >    - "clang -target bpf ...", maybe add macro __BPF__ somewhere
> >      to indicate builtin_bswap16 always available?
> >    - "clang <host target> ..." and then "llc -march=bpf ..."
> >      in this case, __BPF__ macro is not available and
> >      we will not be able to use builtin swap for bpf program.
> > 
> > Maybe use __clang__ macro (or gcc macro) to distinguish between clang 
> > and gcc. If it is gcc we will check builtin availability, otherwise,
> > we assume builtin always available? This not pretty though.
> 
> I think the way this should be fixed is the following: In case
> of LLVM (aka compiling BPF prog), we want the code to be as-is,
> in case if gcc is compiling the hostprog, we either want to keep
> using __builtin_bswap16() or fall-back to something else. Thus,
> I would suggest, we add a new feature test for tooling infra under
> tools/build/feature/ that compiles a dummy prog with __builtin_bswap16().
> And in the bpf_endian.h we define __bpf_ntohs(x) to __bpf_swab16(x)
> which either resolves to __builtin_bswap16() or some fallback
> implementation if not available. I don't think there should be much
> of an issue and it would follow the standard way to do it.
It's not as easy as llvm vs gcc. We can compile userland tests with
llvm/clang as well. We really need to distinguish between the target: bfp vs
non-bpf: always use builtins in bpf case and fallback to swab.h for
userland (or use feature detection, but swab.h should be enough in
theory).

Can we rely on __bpf__ define?

$ cat tmp.c
#ifdef __bpf__
#error a
#else
#error b
#endif
$ clang -c -target bpf tmp.c 
tmp.c:2:2: error: a
#error a
 ^
 1 error generated.

> 
> >>>>>    tools/testing/selftests/bpf/bpf_endian.h | 8 ++++----
> >>>>>    1 file changed, 4 insertions(+), 4 deletions(-)
> >>>>>
> >>>>> diff --git a/tools/testing/selftests/bpf/bpf_endian.h b/tools/testing/selftests/bpf/bpf_endian.h
> >>>>> index b25595ea4a78..1ed268b2002b 100644
> >>>>> --- a/tools/testing/selftests/bpf/bpf_endian.h
> >>>>> +++ b/tools/testing/selftests/bpf/bpf_endian.h
> >>>>> @@ -20,12 +20,12 @@
> >>>>>     * use different targets.
> >>>>>     */
> >>>>>    #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> >>>>> -# define __bpf_ntohs(x)			__builtin_bswap16(x)
> >>>>> -# define __bpf_htons(x)			__builtin_bswap16(x)
> >>>>> +# define __bpf_ntohs(x)			__swab16(x)
> >>>>> +# define __bpf_htons(x)			__swab16(x)
> >>>>>    # define __bpf_constant_ntohs(x)	___constant_swab16(x)
> >>>>>    # define __bpf_constant_htons(x)	___constant_swab16(x)
> >>>>> -# define __bpf_ntohl(x)			__builtin_bswap32(x)
> >>>>> -# define __bpf_htonl(x)			__builtin_bswap32(x)
> >>>>> +# define __bpf_ntohl(x)			__swab32(x)
> >>>>> +# define __bpf_htonl(x)			__swab32(x)
> >>>>>    # define __bpf_constant_ntohl(x)	___constant_swab32(x)
> >>>>>    # define __bpf_constant_htonl(x)	___constant_swab32(x)
> >>>>>    #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> >>>>> -- 
> >>>>> 2.21.0
> >>>>>
> 

[PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[Stanislav Fomichev]

On 03/21, Daniel Borkmann wrote:
> On 03/20/2019 11:45 PM, Yonghong Song wrote:
> > On 3/20/19 3:27 PM, Stanislav Fomichev wrote:
> >> On 03/20, Yonghong Song wrote:
> >>> On 3/20/19 10:13 AM, Stanislav Fomichev wrote:
> >>>> On 03/20, Sergey Senozhatsky wrote:
> >>>>> Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
> >>>>> thus not all compilers are able to compile the following code:
> >>>>>
> >>>>>           (__builtin_constant_p(x) ? \
> >>>>>                   ___constant_swab16(x) : __builtin_bswap16(x))
> >>>>>
> >>>>> That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
> >>>>> instance:
> >>>>>
> >>>>>           error: implicit declaration of function '__builtin_bswap16'
> >>>>>
> >>>>> We can use __builtin_bswap16() only if compiler has this built-in,
> >>>>> that is, only if __HAVE_BUILTIN_BSWAP16__ is defined. Standard UAPI
> >>>>> __swab16()/__swab32() take care of that, and, additionally, handle
> >>>>> __builtin_constant_p() cases as well:
> >>>>>
> >>>>>    #ifdef __HAVE_BUILTIN_BSWAP16__
> >>>>>    #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
> >>>>>    #else
> >>>>>    #define __swab16(x)                             \
> >>>>>            (__builtin_constant_p((__u16)(x)) ?     \
> >>>>>            ___constant_swab16(x) :                 \
> >>>>>            __fswab16(x))
> >>>>>    #endif
> >>>>>
> >>>>> So we can tweak selftests/bpf/bpf_endian.h and use UAPI
> >>>>> __swab16()/__swab32().
> >>>>>
> >>>>> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky at gmail.com>
> >>>>> ---
> >>>>>
> >>>>> v2: fixed build error, reshuffled patches (Stanislav Fomichev)
> >>>> Tested them locally with the compiler I saw the initial issues with - all
> >>>> fine, I don't see any errors with the older gcc.
> >>>>
> >>>> One last question I have is: what happens in the llvm+bpf case? Have
> >>>> you tested that? I think LLVM has all the builtins required, but since
> >>>> we are relying on the swab.h now (and it relies on
> >>>> __HAVE_BUILTIN_BSWAP16__), I wonder whether this detection works
> >>>> correctly on the llvm when targeting bpf. (sidenote: bpf_endian.h can be
> >>>> used from both userspace and bpf programs).
> >>>
> >>> Inside kernel clang compiler header (linux/compiler-clang.h) does not
> >>> define __HAVE_BUILTIN_BSWAP16__. So it will go to the "else" branch in
> >>> the above. So I think it should work with clang + bpf.
> >> Hm, isn't it the opposite of what we want then? I think for llvm+bpf we always
> >> want to use the builtins to make it properly generate
> >> BPF_TO_BE/BPF_TO_LE instructions.
> > 
> > Okay, I see. Then this patch will not achieve that.
> > The following are two common ways to compile a bpf program:
> >    - "clang -target bpf ...", maybe add macro __BPF__ somewhere
> >      to indicate builtin_bswap16 always available?
> >    - "clang <host target> ..." and then "llc -march=bpf ..."
> >      in this case, __BPF__ macro is not available and
> >      we will not be able to use builtin swap for bpf program.
> > 
> > Maybe use __clang__ macro (or gcc macro) to distinguish between clang 
> > and gcc. If it is gcc we will check builtin availability, otherwise,
> > we assume builtin always available? This not pretty though.
> 
> I think the way this should be fixed is the following: In case
> of LLVM (aka compiling BPF prog), we want the code to be as-is,
> in case if gcc is compiling the hostprog, we either want to keep
> using __builtin_bswap16() or fall-back to something else. Thus,
> I would suggest, we add a new feature test for tooling infra under
> tools/build/feature/ that compiles a dummy prog with __builtin_bswap16().
> And in the bpf_endian.h we define __bpf_ntohs(x) to __bpf_swab16(x)
> which either resolves to __builtin_bswap16() or some fallback
> implementation if not available. I don't think there should be much
> of an issue and it would follow the standard way to do it.
It's not as easy as llvm vs gcc. We can compile userland tests with
llvm/clang as well. We really need to distinguish between the target: bfp vs
non-bpf: always use builtins in bpf case and fallback to swab.h for
userland (or use feature detection, but swab.h should be enough in
theory).

Can we rely on __bpf__ define?

$ cat tmp.c
#ifdef __bpf__
#error a
#else
#error b
#endif
$ clang -c -target bpf tmp.c 
tmp.c:2:2: error: a
#error a
 ^
 1 error generated.

> 
> >>>>>    tools/testing/selftests/bpf/bpf_endian.h | 8 ++++----
> >>>>>    1 file changed, 4 insertions(+), 4 deletions(-)
> >>>>>
> >>>>> diff --git a/tools/testing/selftests/bpf/bpf_endian.h b/tools/testing/selftests/bpf/bpf_endian.h
> >>>>> index b25595ea4a78..1ed268b2002b 100644
> >>>>> --- a/tools/testing/selftests/bpf/bpf_endian.h
> >>>>> +++ b/tools/testing/selftests/bpf/bpf_endian.h
> >>>>> @@ -20,12 +20,12 @@
> >>>>>     * use different targets.
> >>>>>     */
> >>>>>    #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> >>>>> -# define __bpf_ntohs(x)			__builtin_bswap16(x)
> >>>>> -# define __bpf_htons(x)			__builtin_bswap16(x)
> >>>>> +# define __bpf_ntohs(x)			__swab16(x)
> >>>>> +# define __bpf_htons(x)			__swab16(x)
> >>>>>    # define __bpf_constant_ntohs(x)	___constant_swab16(x)
> >>>>>    # define __bpf_constant_htons(x)	___constant_swab16(x)
> >>>>> -# define __bpf_ntohl(x)			__builtin_bswap32(x)
> >>>>> -# define __bpf_htonl(x)			__builtin_bswap32(x)
> >>>>> +# define __bpf_ntohl(x)			__swab32(x)
> >>>>> +# define __bpf_htonl(x)			__swab32(x)
> >>>>>    # define __bpf_constant_ntohl(x)	___constant_swab32(x)
> >>>>>    # define __bpf_constant_htonl(x)	___constant_swab32(x)
> >>>>>    #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> >>>>> -- 
> >>>>> 2.21.0
> >>>>>
> 

Re: [PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[Sergey Senozhatsky]

On (03/20/19 10:13), Stanislav Fomichev wrote:
> Tested them locally with the compiler I saw the initial issues with - all
> fine, I don't see any errors with the older gcc.

Thanks!

> One last question I have is: what happens in the llvm+bpf case? Have
> you tested that? I think LLVM has all the builtins required, but since
> we are relying on the swab.h now (and it relies on
> __HAVE_BUILTIN_BSWAP16__), I wonder whether this detection works
> correctly on the llvm when targeting bpf. (sidenote: bpf_endian.h can be
> used from both userspace and bpf programs).

Honestly, I haven't, but I think we should be fine.

For !__HAVE_BUILTIN_BSWAP16__ compilers we still do constant folding.
swab16/swab32 turn into

	__builtin_constant_p((__u16)(x)) ? ___constant_swab16(x) : __fswab16(x))
and
	__builtin_constant_p((__u32)(x)) ? ___constant_swab32(x) : __fswab32(x))

clang/llvm support __builtin_constant_p GCC extension [1]:

 : Clang supports a number of builtin library functions with the same
 : syntax as GCC, including things like __builtin_nan, __builtin_constant_p,
 : __builtin_choose_expr, __builtin_types_compatible_p,
 : __builtin_assume_aligned, __sync_fetch_and_add, etc.

So clang should be able to detect swab on a compile time constant and
optimize it.

[1] https://clang.llvm.org/docs/LanguageExtensions.html

	-ss

[PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[sergey.senozhatsky.work]

On (03/20/19 10:13), Stanislav Fomichev wrote:
> Tested them locally with the compiler I saw the initial issues with - all
> fine, I don't see any errors with the older gcc.

Thanks!

> One last question I have is: what happens in the llvm+bpf case? Have
> you tested that? I think LLVM has all the builtins required, but since
> we are relying on the swab.h now (and it relies on
> __HAVE_BUILTIN_BSWAP16__), I wonder whether this detection works
> correctly on the llvm when targeting bpf. (sidenote: bpf_endian.h can be
> used from both userspace and bpf programs).

Honestly, I haven't, but I think we should be fine.

For !__HAVE_BUILTIN_BSWAP16__ compilers we still do constant folding.
swab16/swab32 turn into

	__builtin_constant_p((__u16)(x)) ? ___constant_swab16(x) : __fswab16(x))
and
	__builtin_constant_p((__u32)(x)) ? ___constant_swab32(x) : __fswab32(x))

clang/llvm support __builtin_constant_p GCC extension [1]:

 : Clang supports a number of builtin library functions with the same
 : syntax as GCC, including things like __builtin_nan, __builtin_constant_p,
 : __builtin_choose_expr, __builtin_types_compatible_p,
 : __builtin_assume_aligned, __sync_fetch_and_add, etc.

So clang should be able to detect swab on a compile time constant and
optimize it.

[1] https://clang.llvm.org/docs/LanguageExtensions.html

	-ss

[PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[Sergey Senozhatsky]

On (03/20/19 10:13), Stanislav Fomichev wrote:
> Tested them locally with the compiler I saw the initial issues with - all
> fine, I don't see any errors with the older gcc.

Thanks!

> One last question I have is: what happens in the llvm+bpf case? Have
> you tested that? I think LLVM has all the builtins required, but since
> we are relying on the swab.h now (and it relies on
> __HAVE_BUILTIN_BSWAP16__), I wonder whether this detection works
> correctly on the llvm when targeting bpf. (sidenote: bpf_endian.h can be
> used from both userspace and bpf programs).

Honestly, I haven't, but I think we should be fine.

For !__HAVE_BUILTIN_BSWAP16__ compilers we still do constant folding.
swab16/swab32 turn into

	__builtin_constant_p((__u16)(x)) ? ___constant_swab16(x) : __fswab16(x))
and
	__builtin_constant_p((__u32)(x)) ? ___constant_swab32(x) : __fswab32(x))

clang/llvm support __builtin_constant_p GCC extension [1]:

 : Clang supports a number of builtin library functions with the same
 : syntax as GCC, including things like __builtin_nan, __builtin_constant_p,
 : __builtin_choose_expr, __builtin_types_compatible_p,
 : __builtin_assume_aligned, __sync_fetch_and_add, etc.

So clang should be able to detect swab on a compile time constant and
optimize it.

[1] https://clang.llvm.org/docs/LanguageExtensions.html

	-ss

Re: [PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[Yonghong Song]

On 3/20/19 5:03 PM, Stanislav Fomichev wrote:
> On 03/21, Daniel Borkmann wrote:
>> On 03/20/2019 11:45 PM, Yonghong Song wrote:
>>> On 3/20/19 3:27 PM, Stanislav Fomichev wrote:
>>>> On 03/20, Yonghong Song wrote:
>>>>> On 3/20/19 10:13 AM, Stanislav Fomichev wrote:
>>>>>> On 03/20, Sergey Senozhatsky wrote:
>>>>>>> Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
>>>>>>> thus not all compilers are able to compile the following code:
>>>>>>>
>>>>>>>            (__builtin_constant_p(x) ? \
>>>>>>>                    ___constant_swab16(x) : __builtin_bswap16(x))
>>>>>>>
>>>>>>> That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
>>>>>>> instance:
>>>>>>>
>>>>>>>            error: implicit declaration of function '__builtin_bswap16'
>>>>>>>
>>>>>>> We can use __builtin_bswap16() only if compiler has this built-in,
>>>>>>> that is, only if __HAVE_BUILTIN_BSWAP16__ is defined. Standard UAPI
>>>>>>> __swab16()/__swab32() take care of that, and, additionally, handle
>>>>>>> __builtin_constant_p() cases as well:
>>>>>>>
>>>>>>>     #ifdef __HAVE_BUILTIN_BSWAP16__
>>>>>>>     #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
>>>>>>>     #else
>>>>>>>     #define __swab16(x)                             \
>>>>>>>             (__builtin_constant_p((__u16)(x)) ?     \
>>>>>>>             ___constant_swab16(x) :                 \
>>>>>>>             __fswab16(x))
>>>>>>>     #endif
>>>>>>>
>>>>>>> So we can tweak selftests/bpf/bpf_endian.h and use UAPI
>>>>>>> __swab16()/__swab32().
>>>>>>>
>>>>>>> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
>>>>>>> ---
>>>>>>>
>>>>>>> v2: fixed build error, reshuffled patches (Stanislav Fomichev)
>>>>>> Tested them locally with the compiler I saw the initial issues with - all
>>>>>> fine, I don't see any errors with the older gcc.
>>>>>>
>>>>>> One last question I have is: what happens in the llvm+bpf case? Have
>>>>>> you tested that? I think LLVM has all the builtins required, but since
>>>>>> we are relying on the swab.h now (and it relies on
>>>>>> __HAVE_BUILTIN_BSWAP16__), I wonder whether this detection works
>>>>>> correctly on the llvm when targeting bpf. (sidenote: bpf_endian.h can be
>>>>>> used from both userspace and bpf programs).
>>>>>
>>>>> Inside kernel clang compiler header (linux/compiler-clang.h) does not
>>>>> define __HAVE_BUILTIN_BSWAP16__. So it will go to the "else" branch in
>>>>> the above. So I think it should work with clang + bpf.
>>>> Hm, isn't it the opposite of what we want then? I think for llvm+bpf we always
>>>> want to use the builtins to make it properly generate
>>>> BPF_TO_BE/BPF_TO_LE instructions.
>>>
>>> Okay, I see. Then this patch will not achieve that.
>>> The following are two common ways to compile a bpf program:
>>>     - "clang -target bpf ...", maybe add macro __BPF__ somewhere
>>>       to indicate builtin_bswap16 always available?
>>>     - "clang <host target> ..." and then "llc -march=bpf ..."
>>>       in this case, __BPF__ macro is not available and
>>>       we will not be able to use builtin swap for bpf program.
>>>
>>> Maybe use __clang__ macro (or gcc macro) to distinguish between clang
>>> and gcc. If it is gcc we will check builtin availability, otherwise,
>>> we assume builtin always available? This not pretty though.
>>
>> I think the way this should be fixed is the following: In case
>> of LLVM (aka compiling BPF prog), we want the code to be as-is,
>> in case if gcc is compiling the hostprog, we either want to keep
>> using __builtin_bswap16() or fall-back to something else. Thus,
>> I would suggest, we add a new feature test for tooling infra under
>> tools/build/feature/ that compiles a dummy prog with __builtin_bswap16().
>> And in the bpf_endian.h we define __bpf_ntohs(x) to __bpf_swab16(x)
>> which either resolves to __builtin_bswap16() or some fallback
>> implementation if not available. I don't think there should be much
>> of an issue and it would follow the standard way to do it.
> It's not as easy as llvm vs gcc. We can compile userland tests with
> llvm/clang as well. We really need to distinguish between the target: bfp vs
> non-bpf: always use builtins in bpf case and fallback to swab.h for
> userland (or use feature detection, but swab.h should be enough in
> theory).
> 
> Can we rely on __bpf__ define?
> 
> $ cat tmp.c
> #ifdef __bpf__
> #error a
> #else
> #error b
> #endif
> $ clang -c -target bpf tmp.c
> tmp.c:2:2: error: a
> #error a
>   ^
>   1 error generated.

Yes, you can rely this, __bpf__, __bpf or __BPF__. These three
are clang predefined macros for target bpf.

> 
>>
>>>>>>>     tools/testing/selftests/bpf/bpf_endian.h | 8 ++++----
>>>>>>>     1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/tools/testing/selftests/bpf/bpf_endian.h b/tools/testing/selftests/bpf/bpf_endian.h
>>>>>>> index b25595ea4a78..1ed268b2002b 100644
>>>>>>> --- a/tools/testing/selftests/bpf/bpf_endian.h
>>>>>>> +++ b/tools/testing/selftests/bpf/bpf_endian.h
>>>>>>> @@ -20,12 +20,12 @@
>>>>>>>      * use different targets.
>>>>>>>      */
>>>>>>>     #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
>>>>>>> -# define __bpf_ntohs(x)			__builtin_bswap16(x)
>>>>>>> -# define __bpf_htons(x)			__builtin_bswap16(x)
>>>>>>> +# define __bpf_ntohs(x)			__swab16(x)
>>>>>>> +# define __bpf_htons(x)			__swab16(x)
>>>>>>>     # define __bpf_constant_ntohs(x)	___constant_swab16(x)
>>>>>>>     # define __bpf_constant_htons(x)	___constant_swab16(x)
>>>>>>> -# define __bpf_ntohl(x)			__builtin_bswap32(x)
>>>>>>> -# define __bpf_htonl(x)			__builtin_bswap32(x)
>>>>>>> +# define __bpf_ntohl(x)			__swab32(x)
>>>>>>> +# define __bpf_htonl(x)			__swab32(x)
>>>>>>>     # define __bpf_constant_ntohl(x)	___constant_swab32(x)
>>>>>>>     # define __bpf_constant_htonl(x)	___constant_swab32(x)
>>>>>>>     #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
>>>>>>> -- 
>>>>>>> 2.21.0
>>>>>>>
>>

[PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[yhs]

On 3/20/19 5:03 PM, Stanislav Fomichev wrote:
> On 03/21, Daniel Borkmann wrote:
>> On 03/20/2019 11:45 PM, Yonghong Song wrote:
>>> On 3/20/19 3:27 PM, Stanislav Fomichev wrote:
>>>> On 03/20, Yonghong Song wrote:
>>>>> On 3/20/19 10:13 AM, Stanislav Fomichev wrote:
>>>>>> On 03/20, Sergey Senozhatsky wrote:
>>>>>>> Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
>>>>>>> thus not all compilers are able to compile the following code:
>>>>>>>
>>>>>>>            (__builtin_constant_p(x) ? \
>>>>>>>                    ___constant_swab16(x) : __builtin_bswap16(x))
>>>>>>>
>>>>>>> That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
>>>>>>> instance:
>>>>>>>
>>>>>>>            error: implicit declaration of function '__builtin_bswap16'
>>>>>>>
>>>>>>> We can use __builtin_bswap16() only if compiler has this built-in,
>>>>>>> that is, only if __HAVE_BUILTIN_BSWAP16__ is defined. Standard UAPI
>>>>>>> __swab16()/__swab32() take care of that, and, additionally, handle
>>>>>>> __builtin_constant_p() cases as well:
>>>>>>>
>>>>>>>     #ifdef __HAVE_BUILTIN_BSWAP16__
>>>>>>>     #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
>>>>>>>     #else
>>>>>>>     #define __swab16(x)                             \
>>>>>>>             (__builtin_constant_p((__u16)(x)) ?     \
>>>>>>>             ___constant_swab16(x) :                 \
>>>>>>>             __fswab16(x))
>>>>>>>     #endif
>>>>>>>
>>>>>>> So we can tweak selftests/bpf/bpf_endian.h and use UAPI
>>>>>>> __swab16()/__swab32().
>>>>>>>
>>>>>>> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky at gmail.com>
>>>>>>> ---
>>>>>>>
>>>>>>> v2: fixed build error, reshuffled patches (Stanislav Fomichev)
>>>>>> Tested them locally with the compiler I saw the initial issues with - all
>>>>>> fine, I don't see any errors with the older gcc.
>>>>>>
>>>>>> One last question I have is: what happens in the llvm+bpf case? Have
>>>>>> you tested that? I think LLVM has all the builtins required, but since
>>>>>> we are relying on the swab.h now (and it relies on
>>>>>> __HAVE_BUILTIN_BSWAP16__), I wonder whether this detection works
>>>>>> correctly on the llvm when targeting bpf. (sidenote: bpf_endian.h can be
>>>>>> used from both userspace and bpf programs).
>>>>>
>>>>> Inside kernel clang compiler header (linux/compiler-clang.h) does not
>>>>> define __HAVE_BUILTIN_BSWAP16__. So it will go to the "else" branch in
>>>>> the above. So I think it should work with clang + bpf.
>>>> Hm, isn't it the opposite of what we want then? I think for llvm+bpf we always
>>>> want to use the builtins to make it properly generate
>>>> BPF_TO_BE/BPF_TO_LE instructions.
>>>
>>> Okay, I see. Then this patch will not achieve that.
>>> The following are two common ways to compile a bpf program:
>>>     - "clang -target bpf ...", maybe add macro __BPF__ somewhere
>>>       to indicate builtin_bswap16 always available?
>>>     - "clang <host target> ..." and then "llc -march=bpf ..."
>>>       in this case, __BPF__ macro is not available and
>>>       we will not be able to use builtin swap for bpf program.
>>>
>>> Maybe use __clang__ macro (or gcc macro) to distinguish between clang
>>> and gcc. If it is gcc we will check builtin availability, otherwise,
>>> we assume builtin always available? This not pretty though.
>>
>> I think the way this should be fixed is the following: In case
>> of LLVM (aka compiling BPF prog), we want the code to be as-is,
>> in case if gcc is compiling the hostprog, we either want to keep
>> using __builtin_bswap16() or fall-back to something else. Thus,
>> I would suggest, we add a new feature test for tooling infra under
>> tools/build/feature/ that compiles a dummy prog with __builtin_bswap16().
>> And in the bpf_endian.h we define __bpf_ntohs(x) to __bpf_swab16(x)
>> which either resolves to __builtin_bswap16() or some fallback
>> implementation if not available. I don't think there should be much
>> of an issue and it would follow the standard way to do it.
> It's not as easy as llvm vs gcc. We can compile userland tests with
> llvm/clang as well. We really need to distinguish between the target: bfp vs
> non-bpf: always use builtins in bpf case and fallback to swab.h for
> userland (or use feature detection, but swab.h should be enough in
> theory).
> 
> Can we rely on __bpf__ define?
> 
> $ cat tmp.c
> #ifdef __bpf__
> #error a
> #else
> #error b
> #endif
> $ clang -c -target bpf tmp.c
> tmp.c:2:2: error: a
> #error a
>   ^
>   1 error generated.

Yes, you can rely this, __bpf__, __bpf or __BPF__. These three
are clang predefined macros for target bpf.

> 
>>
>>>>>>>     tools/testing/selftests/bpf/bpf_endian.h | 8 ++++----
>>>>>>>     1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/tools/testing/selftests/bpf/bpf_endian.h b/tools/testing/selftests/bpf/bpf_endian.h
>>>>>>> index b25595ea4a78..1ed268b2002b 100644
>>>>>>> --- a/tools/testing/selftests/bpf/bpf_endian.h
>>>>>>> +++ b/tools/testing/selftests/bpf/bpf_endian.h
>>>>>>> @@ -20,12 +20,12 @@
>>>>>>>      * use different targets.
>>>>>>>      */
>>>>>>>     #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
>>>>>>> -# define __bpf_ntohs(x)			__builtin_bswap16(x)
>>>>>>> -# define __bpf_htons(x)			__builtin_bswap16(x)
>>>>>>> +# define __bpf_ntohs(x)			__swab16(x)
>>>>>>> +# define __bpf_htons(x)			__swab16(x)
>>>>>>>     # define __bpf_constant_ntohs(x)	___constant_swab16(x)
>>>>>>>     # define __bpf_constant_htons(x)	___constant_swab16(x)
>>>>>>> -# define __bpf_ntohl(x)			__builtin_bswap32(x)
>>>>>>> -# define __bpf_htonl(x)			__builtin_bswap32(x)
>>>>>>> +# define __bpf_ntohl(x)			__swab32(x)
>>>>>>> +# define __bpf_htonl(x)			__swab32(x)
>>>>>>>     # define __bpf_constant_ntohl(x)	___constant_swab32(x)
>>>>>>>     # define __bpf_constant_htonl(x)	___constant_swab32(x)
>>>>>>>     #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
>>>>>>> -- 
>>>>>>> 2.21.0
>>>>>>>
>>

[PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[Yonghong Song]

On 3/20/19 5:03 PM, Stanislav Fomichev wrote:
> On 03/21, Daniel Borkmann wrote:
>> On 03/20/2019 11:45 PM, Yonghong Song wrote:
>>> On 3/20/19 3:27 PM, Stanislav Fomichev wrote:
>>>> On 03/20, Yonghong Song wrote:
>>>>> On 3/20/19 10:13 AM, Stanislav Fomichev wrote:
>>>>>> On 03/20, Sergey Senozhatsky wrote:
>>>>>>> Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
>>>>>>> thus not all compilers are able to compile the following code:
>>>>>>>
>>>>>>>            (__builtin_constant_p(x) ? \
>>>>>>>                    ___constant_swab16(x) : __builtin_bswap16(x))
>>>>>>>
>>>>>>> That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
>>>>>>> instance:
>>>>>>>
>>>>>>>            error: implicit declaration of function '__builtin_bswap16'
>>>>>>>
>>>>>>> We can use __builtin_bswap16() only if compiler has this built-in,
>>>>>>> that is, only if __HAVE_BUILTIN_BSWAP16__ is defined. Standard UAPI
>>>>>>> __swab16()/__swab32() take care of that, and, additionally, handle
>>>>>>> __builtin_constant_p() cases as well:
>>>>>>>
>>>>>>>     #ifdef __HAVE_BUILTIN_BSWAP16__
>>>>>>>     #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
>>>>>>>     #else
>>>>>>>     #define __swab16(x)                             \
>>>>>>>             (__builtin_constant_p((__u16)(x)) ?     \
>>>>>>>             ___constant_swab16(x) :                 \
>>>>>>>             __fswab16(x))
>>>>>>>     #endif
>>>>>>>
>>>>>>> So we can tweak selftests/bpf/bpf_endian.h and use UAPI
>>>>>>> __swab16()/__swab32().
>>>>>>>
>>>>>>> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky at gmail.com>
>>>>>>> ---
>>>>>>>
>>>>>>> v2: fixed build error, reshuffled patches (Stanislav Fomichev)
>>>>>> Tested them locally with the compiler I saw the initial issues with - all
>>>>>> fine, I don't see any errors with the older gcc.
>>>>>>
>>>>>> One last question I have is: what happens in the llvm+bpf case? Have
>>>>>> you tested that? I think LLVM has all the builtins required, but since
>>>>>> we are relying on the swab.h now (and it relies on
>>>>>> __HAVE_BUILTIN_BSWAP16__), I wonder whether this detection works
>>>>>> correctly on the llvm when targeting bpf. (sidenote: bpf_endian.h can be
>>>>>> used from both userspace and bpf programs).
>>>>>
>>>>> Inside kernel clang compiler header (linux/compiler-clang.h) does not
>>>>> define __HAVE_BUILTIN_BSWAP16__. So it will go to the "else" branch in
>>>>> the above. So I think it should work with clang + bpf.
>>>> Hm, isn't it the opposite of what we want then? I think for llvm+bpf we always
>>>> want to use the builtins to make it properly generate
>>>> BPF_TO_BE/BPF_TO_LE instructions.
>>>
>>> Okay, I see. Then this patch will not achieve that.
>>> The following are two common ways to compile a bpf program:
>>>     - "clang -target bpf ...", maybe add macro __BPF__ somewhere
>>>       to indicate builtin_bswap16 always available?
>>>     - "clang <host target> ..." and then "llc -march=bpf ..."
>>>       in this case, __BPF__ macro is not available and
>>>       we will not be able to use builtin swap for bpf program.
>>>
>>> Maybe use __clang__ macro (or gcc macro) to distinguish between clang
>>> and gcc. If it is gcc we will check builtin availability, otherwise,
>>> we assume builtin always available? This not pretty though.
>>
>> I think the way this should be fixed is the following: In case
>> of LLVM (aka compiling BPF prog), we want the code to be as-is,
>> in case if gcc is compiling the hostprog, we either want to keep
>> using __builtin_bswap16() or fall-back to something else. Thus,
>> I would suggest, we add a new feature test for tooling infra under
>> tools/build/feature/ that compiles a dummy prog with __builtin_bswap16().
>> And in the bpf_endian.h we define __bpf_ntohs(x) to __bpf_swab16(x)
>> which either resolves to __builtin_bswap16() or some fallback
>> implementation if not available. I don't think there should be much
>> of an issue and it would follow the standard way to do it.
> It's not as easy as llvm vs gcc. We can compile userland tests with
> llvm/clang as well. We really need to distinguish between the target: bfp vs
> non-bpf: always use builtins in bpf case and fallback to swab.h for
> userland (or use feature detection, but swab.h should be enough in
> theory).
> 
> Can we rely on __bpf__ define?
> 
> $ cat tmp.c
> #ifdef __bpf__
> #error a
> #else
> #error b
> #endif
> $ clang -c -target bpf tmp.c
> tmp.c:2:2: error: a
> #error a
>   ^
>   1 error generated.

Yes, you can rely this, __bpf__, __bpf or __BPF__. These three
are clang predefined macros for target bpf.

> 
>>
>>>>>>>     tools/testing/selftests/bpf/bpf_endian.h | 8 ++++----
>>>>>>>     1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/tools/testing/selftests/bpf/bpf_endian.h b/tools/testing/selftests/bpf/bpf_endian.h
>>>>>>> index b25595ea4a78..1ed268b2002b 100644
>>>>>>> --- a/tools/testing/selftests/bpf/bpf_endian.h
>>>>>>> +++ b/tools/testing/selftests/bpf/bpf_endian.h
>>>>>>> @@ -20,12 +20,12 @@
>>>>>>>      * use different targets.
>>>>>>>      */
>>>>>>>     #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
>>>>>>> -# define __bpf_ntohs(x)			__builtin_bswap16(x)
>>>>>>> -# define __bpf_htons(x)			__builtin_bswap16(x)
>>>>>>> +# define __bpf_ntohs(x)			__swab16(x)
>>>>>>> +# define __bpf_htons(x)			__swab16(x)
>>>>>>>     # define __bpf_constant_ntohs(x)	___constant_swab16(x)
>>>>>>>     # define __bpf_constant_htons(x)	___constant_swab16(x)
>>>>>>> -# define __bpf_ntohl(x)			__builtin_bswap32(x)
>>>>>>> -# define __bpf_htonl(x)			__builtin_bswap32(x)
>>>>>>> +# define __bpf_ntohl(x)			__swab32(x)
>>>>>>> +# define __bpf_htonl(x)			__swab32(x)
>>>>>>>     # define __bpf_constant_ntohl(x)	___constant_swab32(x)
>>>>>>>     # define __bpf_constant_htonl(x)	___constant_swab32(x)
>>>>>>>     #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
>>>>>>> -- 
>>>>>>> 2.21.0
>>>>>>>
>>

Re: [PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[Sergey Senozhatsky]

On (03/20/19 15:27), Stanislav Fomichev wrote:
[..]
> > Inside kernel clang compiler header (linux/compiler-clang.h) does not 
> > define __HAVE_BUILTIN_BSWAP16__. So it will go to the "else" branch in 
> > the above. So I think it should work with clang + bpf.
> Hm, isn't it the opposite of what we want then? I think for llvm+bpf we always
> want to use the builtins to make it properly generate
> BPF_TO_BE/BPF_TO_LE instructions.

Oh, hmm, OK. I see your point now. bpf insn set for variables.

	-ss

[PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[sergey.senozhatsky.work]

On (03/20/19 15:27), Stanislav Fomichev wrote:
[..]
> > Inside kernel clang compiler header (linux/compiler-clang.h) does not 
> > define __HAVE_BUILTIN_BSWAP16__. So it will go to the "else" branch in 
> > the above. So I think it should work with clang + bpf.
> Hm, isn't it the opposite of what we want then? I think for llvm+bpf we always
> want to use the builtins to make it properly generate
> BPF_TO_BE/BPF_TO_LE instructions.

Oh, hmm, OK. I see your point now. bpf insn set for variables.

	-ss

[PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[Sergey Senozhatsky]

On (03/20/19 15:27), Stanislav Fomichev wrote:
[..]
> > Inside kernel clang compiler header (linux/compiler-clang.h) does not 
> > define __HAVE_BUILTIN_BSWAP16__. So it will go to the "else" branch in 
> > the above. So I think it should work with clang + bpf.
> Hm, isn't it the opposite of what we want then? I think for llvm+bpf we always
> want to use the builtins to make it properly generate
> BPF_TO_BE/BPF_TO_LE instructions.

Oh, hmm, OK. I see your point now. bpf insn set for variables.

	-ss

Re: [PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[Alexei Starovoitov]

On Wed, Mar 20, 2019 at 09:53:33PM +0900, Sergey Senozhatsky wrote:
> Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
> thus not all compilers are able to compile the following code:
> 
>         (__builtin_constant_p(x) ? \
>                 ___constant_swab16(x) : __builtin_bswap16(x))
> 
> That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
> instance:

nack to fixes to support such old compilers.

[PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[alexei.starovoitov]

On Wed, Mar 20, 2019 at 09:53:33PM +0900, Sergey Senozhatsky wrote:
> Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
> thus not all compilers are able to compile the following code:
> 
>         (__builtin_constant_p(x) ? \
>                 ___constant_swab16(x) : __builtin_bswap16(x))
> 
> That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
> instance:

nack to fixes to support such old compilers.

[PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[Alexei Starovoitov]

On Wed, Mar 20, 2019@09:53:33PM +0900, Sergey Senozhatsky wrote:
> Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
> thus not all compilers are able to compile the following code:
> 
>         (__builtin_constant_p(x) ? \
>                 ___constant_swab16(x) : __builtin_bswap16(x))
> 
> That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
> instance:

nack to fixes to support such old compilers.

Re: [PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[Sergey Senozhatsky]

On (03/20/19 20:24), Alexei Starovoitov wrote:
> On Wed, Mar 20, 2019 at 09:53:33PM +0900, Sergey Senozhatsky wrote:
> > Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
> > thus not all compilers are able to compile the following code:
> > 
> >         (__builtin_constant_p(x) ? \
> >                 ___constant_swab16(x) : __builtin_bswap16(x))
> > 
> > That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
> > instance:
> 
> nack to fixes to support such old compilers.

Fair enough.

	-ss

[PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[sergey.senozhatsky.work]

On (03/20/19 20:24), Alexei Starovoitov wrote:
> On Wed, Mar 20, 2019 at 09:53:33PM +0900, Sergey Senozhatsky wrote:
> > Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
> > thus not all compilers are able to compile the following code:
> > 
> >         (__builtin_constant_p(x) ? \
> >                 ___constant_swab16(x) : __builtin_bswap16(x))
> > 
> > That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
> > instance:
> 
> nack to fixes to support such old compilers.

Fair enough.

	-ss

[PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[Sergey Senozhatsky]

On (03/20/19 20:24), Alexei Starovoitov wrote:
> On Wed, Mar 20, 2019@09:53:33PM +0900, Sergey Senozhatsky wrote:
> > Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
> > thus not all compilers are able to compile the following code:
> > 
> >         (__builtin_constant_p(x) ? \
> >                 ___constant_swab16(x) : __builtin_bswap16(x))
> > 
> > That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
> > instance:
> 
> nack to fixes to support such old compilers.

Fair enough.

	-ss

Re: [PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[Stanislav Fomichev]

On 03/21, Sergey Senozhatsky wrote:
> On (03/20/19 20:24), Alexei Starovoitov wrote:
> > On Wed, Mar 20, 2019 at 09:53:33PM +0900, Sergey Senozhatsky wrote:
> > > Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
> > > thus not all compilers are able to compile the following code:
> > > 
> > >         (__builtin_constant_p(x) ? \
> > >                 ___constant_swab16(x) : __builtin_bswap16(x))
> > > 
> > > That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
> > > instance:
> > 
> > nack to fixes to support such old compilers.
> 
> Fair enough.
What is too old? Documentation/process/changes.rst says that minimum
supported gcc is 4.6, do we lift that requirement for the tests?

> 	-ss

[PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[sdf]

On 03/21, Sergey Senozhatsky wrote:
> On (03/20/19 20:24), Alexei Starovoitov wrote:
> > On Wed, Mar 20, 2019 at 09:53:33PM +0900, Sergey Senozhatsky wrote:
> > > Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
> > > thus not all compilers are able to compile the following code:
> > > 
> > >         (__builtin_constant_p(x) ? \
> > >                 ___constant_swab16(x) : __builtin_bswap16(x))
> > > 
> > > That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
> > > instance:
> > 
> > nack to fixes to support such old compilers.
> 
> Fair enough.
What is too old? Documentation/process/changes.rst says that minimum
supported gcc is 4.6, do we lift that requirement for the tests?

> 	-ss

[PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[Stanislav Fomichev]

On 03/21, Sergey Senozhatsky wrote:
> On (03/20/19 20:24), Alexei Starovoitov wrote:
> > On Wed, Mar 20, 2019@09:53:33PM +0900, Sergey Senozhatsky wrote:
> > > Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
> > > thus not all compilers are able to compile the following code:
> > > 
> > >         (__builtin_constant_p(x) ? \
> > >                 ___constant_swab16(x) : __builtin_bswap16(x))
> > > 
> > > That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
> > > instance:
> > 
> > nack to fixes to support such old compilers.
> 
> Fair enough.
What is too old? Documentation/process/changes.rst says that minimum
supported gcc is 4.6, do we lift that requirement for the tests?

> 	-ss

Re: [PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[Sergey Senozhatsky]

On (03/21/19 08:49), Stanislav Fomichev wrote:
> On 03/21, Sergey Senozhatsky wrote:
> > On (03/20/19 20:24), Alexei Starovoitov wrote:
> > > On Wed, Mar 20, 2019 at 09:53:33PM +0900, Sergey Senozhatsky wrote:
> > > > Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
> > > > thus not all compilers are able to compile the following code:
> > > > 
> > > >         (__builtin_constant_p(x) ? \
> > > >                 ___constant_swab16(x) : __builtin_bswap16(x))
> > > > 
> > > > That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
> > > > instance:
> > > 
> > > nack to fixes to support such old compilers.
> > 
> > Fair enough.
> What is too old? Documentation/process/changes.rst says that minimum
> supported gcc is 4.6, do we lift that requirement for the tests?

Hmm, good point, Stanislav. I thought it was gcc 4.9 which introduced
asm goto and hence 4.9 is the minimum supported version. But it seems
that it was 4.5/4.6, so the min supported gcc version is 4.6. Which
means that those bpf defines won't work on some compilers.

Alexei, does your NACK still stand?

	-ss

[PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[sergey.senozhatsky.work]

On (03/21/19 08:49), Stanislav Fomichev wrote:
> On 03/21, Sergey Senozhatsky wrote:
> > On (03/20/19 20:24), Alexei Starovoitov wrote:
> > > On Wed, Mar 20, 2019 at 09:53:33PM +0900, Sergey Senozhatsky wrote:
> > > > Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
> > > > thus not all compilers are able to compile the following code:
> > > > 
> > > >         (__builtin_constant_p(x) ? \
> > > >                 ___constant_swab16(x) : __builtin_bswap16(x))
> > > > 
> > > > That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
> > > > instance:
> > > 
> > > nack to fixes to support such old compilers.
> > 
> > Fair enough.
> What is too old? Documentation/process/changes.rst says that minimum
> supported gcc is 4.6, do we lift that requirement for the tests?

Hmm, good point, Stanislav. I thought it was gcc 4.9 which introduced
asm goto and hence 4.9 is the minimum supported version. But it seems
that it was 4.5/4.6, so the min supported gcc version is 4.6. Which
means that those bpf defines won't work on some compilers.

Alexei, does your NACK still stand?

	-ss

[PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[Sergey Senozhatsky]

On (03/21/19 08:49), Stanislav Fomichev wrote:
> On 03/21, Sergey Senozhatsky wrote:
> > On (03/20/19 20:24), Alexei Starovoitov wrote:
> > > On Wed, Mar 20, 2019@09:53:33PM +0900, Sergey Senozhatsky wrote:
> > > > Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
> > > > thus not all compilers are able to compile the following code:
> > > > 
> > > >         (__builtin_constant_p(x) ? \
> > > >                 ___constant_swab16(x) : __builtin_bswap16(x))
> > > > 
> > > > That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
> > > > instance:
> > > 
> > > nack to fixes to support such old compilers.
> > 
> > Fair enough.
> What is too old? Documentation/process/changes.rst says that minimum
> supported gcc is 4.6, do we lift that requirement for the tests?

Hmm, good point, Stanislav. I thought it was gcc 4.9 which introduced
asm goto and hence 4.9 is the minimum supported version. But it seems
that it was 4.5/4.6, so the min supported gcc version is 4.6. Which
means that those bpf defines won't work on some compilers.

Alexei, does your NACK still stand?

	-ss

Re: [PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[Alexei Starovoitov]

On Fri, Mar 22, 2019 at 11:46:52AM +0900, Sergey Senozhatsky wrote:
> On (03/21/19 08:49), Stanislav Fomichev wrote:
> > On 03/21, Sergey Senozhatsky wrote:
> > > On (03/20/19 20:24), Alexei Starovoitov wrote:
> > > > On Wed, Mar 20, 2019 at 09:53:33PM +0900, Sergey Senozhatsky wrote:
> > > > > Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
> > > > > thus not all compilers are able to compile the following code:
> > > > > 
> > > > >         (__builtin_constant_p(x) ? \
> > > > >                 ___constant_swab16(x) : __builtin_bswap16(x))
> > > > > 
> > > > > That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
> > > > > instance:
> > > > 
> > > > nack to fixes to support such old compilers.
> > > 
> > > Fair enough.
> > What is too old? Documentation/process/changes.rst says that minimum
> > supported gcc is 4.6, do we lift that requirement for the tests?
> 
> Hmm, good point, Stanislav. I thought it was gcc 4.9 which introduced
> asm goto and hence 4.9 is the minimum supported version. But it seems
> that it was 4.5/4.6, so the min supported gcc version is 4.6. Which
> means that those bpf defines won't work on some compilers.
> 
> Alexei, does your NACK still stand?

yes.
bpf samples and selftests require llvm and new features like BTF
require the latest llvm which requires gcc 5.1.
Things are more or less working still with gcc 4.8, but soon will
likely start breaking.

[PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[alexei.starovoitov]

On Fri, Mar 22, 2019 at 11:46:52AM +0900, Sergey Senozhatsky wrote:
> On (03/21/19 08:49), Stanislav Fomichev wrote:
> > On 03/21, Sergey Senozhatsky wrote:
> > > On (03/20/19 20:24), Alexei Starovoitov wrote:
> > > > On Wed, Mar 20, 2019 at 09:53:33PM +0900, Sergey Senozhatsky wrote:
> > > > > Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
> > > > > thus not all compilers are able to compile the following code:
> > > > > 
> > > > >         (__builtin_constant_p(x) ? \
> > > > >                 ___constant_swab16(x) : __builtin_bswap16(x))
> > > > > 
> > > > > That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
> > > > > instance:
> > > > 
> > > > nack to fixes to support such old compilers.
> > > 
> > > Fair enough.
> > What is too old? Documentation/process/changes.rst says that minimum
> > supported gcc is 4.6, do we lift that requirement for the tests?
> 
> Hmm, good point, Stanislav. I thought it was gcc 4.9 which introduced
> asm goto and hence 4.9 is the minimum supported version. But it seems
> that it was 4.5/4.6, so the min supported gcc version is 4.6. Which
> means that those bpf defines won't work on some compilers.
> 
> Alexei, does your NACK still stand?

yes.
bpf samples and selftests require llvm and new features like BTF
require the latest llvm which requires gcc 5.1.
Things are more or less working still with gcc 4.8, but soon will
likely start breaking.

[PATCHv2 bpf-next 1/3] bpf, tests: tweak endianness selection

[Alexei Starovoitov]

On Fri, Mar 22, 2019@11:46:52AM +0900, Sergey Senozhatsky wrote:
> On (03/21/19 08:49), Stanislav Fomichev wrote:
> > On 03/21, Sergey Senozhatsky wrote:
> > > On (03/20/19 20:24), Alexei Starovoitov wrote:
> > > > On Wed, Mar 20, 2019@09:53:33PM +0900, Sergey Senozhatsky wrote:
> > > > > Not all compilers have __builtin_bswap16() and __builtin_bswap32(),
> > > > > thus not all compilers are able to compile the following code:
> > > > > 
> > > > >         (__builtin_constant_p(x) ? \
> > > > >                 ___constant_swab16(x) : __builtin_bswap16(x))
> > > > > 
> > > > > That's the reason why bpf_ntohl() doesn't work on GCC < 4.8, for
> > > > > instance:
> > > > 
> > > > nack to fixes to support such old compilers.
> > > 
> > > Fair enough.
> > What is too old? Documentation/process/changes.rst says that minimum
> > supported gcc is 4.6, do we lift that requirement for the tests?
> 
> Hmm, good point, Stanislav. I thought it was gcc 4.9 which introduced
> asm goto and hence 4.9 is the minimum supported version. But it seems
> that it was 4.5/4.6, so the min supported gcc version is 4.6. Which
> means that those bpf defines won't work on some compilers.
> 
> Alexei, does your NACK still stand?

yes.
bpf samples and selftests require llvm and new features like BTF
require the latest llvm which requires gcc 5.1.
Things are more or less working still with gcc 4.8, but soon will
likely start breaking.