[RFC] eal: use _Static_assert() for RTE_BUILD_BUG_ON

[RFC] eal: use _Static_assert() for RTE_BUILD_BUG_ON

[Stephen Hemminger]

The method of doing sizeof a bad array element was an interesting
hack but it has a couple of problems. First, it won't work if
VLA checking is enabled. It doesn't enforce that the expression
is constant.

Replace that with the _Static_assert builtin available in
Gcc, Clang, and MSVC.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/eal/include/rte_common.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index c1ba32d00e47..2c63206223b8 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -495,7 +495,7 @@ rte_is_aligned(const void * const __rte_restrict ptr, const unsigned int align)
 /**
  * Triggers an error at compilation time if the condition is true.
  */
-#define RTE_BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
+#define RTE_BUILD_BUG_ON(e) _Static_assert(!(e),  #e)
 
 /*********** Cache line related macros ********/
 
-- 
2.39.2

RE: [RFC] eal: use _Static_assert() for RTE_BUILD_BUG_ON

[Morten Brørup]

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Saturday, 11 November 2023 18.22
> 
> The method of doing sizeof a bad array element was an interesting
> hack but it has a couple of problems. First, it won't work if
> VLA checking is enabled. It doesn't enforce that the expression
> is constant.
> 
> Replace that with the _Static_assert builtin available in
> Gcc, Clang, and MSVC.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---

Two souls, one thought...
I have been considering exactly the same, and thus strongly support this.

> -#define RTE_BUILD_BUG_ON(condition) ((void)sizeof(char[1 -
> 2*!!(condition)]))
> +#define RTE_BUILD_BUG_ON(e) _Static_assert(!(e),  #e)

Please use static_assert instead of _Static_assert, as discussed with Bruce:

http://inbox.dpdk.org/dev/ZR%2FlDC88s+HYXw27@bricha3-MOBL.ger.corp.intel.com/

Acked-by: Morten Brørup <mb@smartsharesystems.com>

Re: [RFC] eal: use _Static_assert() for RTE_BUILD_BUG_ON

[Tyler Retzlaff]

On Sat, Nov 11, 2023 at 09:21:53AM -0800, Stephen Hemminger wrote:
> The method of doing sizeof a bad array element was an interesting
> hack but it has a couple of problems. First, it won't work if
> VLA checking is enabled. It doesn't enforce that the expression
> is constant.
> 
> Replace that with the _Static_assert builtin available in
> Gcc, Clang, and MSVC.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---

Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>

Re: [RFC] eal: use _Static_assert() for RTE_BUILD_BUG_ON

[Tyler Retzlaff]

On Sat, Nov 11, 2023 at 06:52:26PM +0100, Morten Brørup wrote:
> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Saturday, 11 November 2023 18.22
> > 
> > The method of doing sizeof a bad array element was an interesting
> > hack but it has a couple of problems. First, it won't work if
> > VLA checking is enabled. It doesn't enforce that the expression
> > is constant.
> > 
> > Replace that with the _Static_assert builtin available in
> > Gcc, Clang, and MSVC.
> > 
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> 
> Two souls, one thought...
> I have been considering exactly the same, and thus strongly support this.
> 
> > -#define RTE_BUILD_BUG_ON(condition) ((void)sizeof(char[1 -
> > 2*!!(condition)]))
> > +#define RTE_BUILD_BUG_ON(e) _Static_assert(!(e),  #e)
> 
> Please use static_assert instead of _Static_assert, as discussed with Bruce:
> 
> http://inbox.dpdk.org/dev/ZR%2FlDC88s+HYXw27@bricha3-MOBL.ger.corp.intel.com/

+1
At least in public headers use the macro static_assert for portability
with C++.

> 
> Acked-by: Morten Brørup <mb@smartsharesystems.com>

one more time for good measure, this is an important change.

Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>

[PATCH v2 0/3] use static_assertion for build errors

[Stephen Hemminger]

This series fixes a couple places where expressions that could not
be evaluated as constant early in compiler passes were used. And then
converts RTE_BUILD_BUG_ON() with static_assert.

Stephen Hemminger (3):
  event/opdl: fix non-constant compile time assertion
  net/sfc: fix non-constant expression inr RTE_BUILD_BUG_ON()
  eal: replace out of bounds VLA with static_assert

 drivers/event/opdl/opdl_ring.c | 5 ++++-
 drivers/net/sfc/sfc_ef100_tx.c | 7 +++++--
 lib/eal/include/rte_common.h   | 3 ++-
 3 files changed, 11 insertions(+), 4 deletions(-)

-- 
2.39.2

[PATCH v2 1/3] event/opdl: fix non-constant compile time assertion

[Stephen Hemminger]

RTE_BUILD_BUG_ON() was being used with a non-constant value.
The inline function rte_is_power_of_2() is not constant since
inline expansion happens later in the compile process.
Replace it with macro which will be constant.

Fixes: 4236ce9bf5bf ("event/opdl: add OPDL ring infrastructure library")
Cc: liang.j.ma@intel.com
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/event/opdl/opdl_ring.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/event/opdl/opdl_ring.c b/drivers/event/opdl/opdl_ring.c
index 69392b56bbec..24e0bbe3222d 100644
--- a/drivers/event/opdl/opdl_ring.c
+++ b/drivers/event/opdl/opdl_ring.c
@@ -31,6 +31,9 @@
 #define OPDL_OPA_MASK    (0xFF)
 #define OPDL_OPA_OFFSET  (0x38)
 
+/* Equivalent to rte_is_power_of_2() but as macro. */
+#define IS_POWER_OF_2(x) (((x) & ((x) - 1)) == 0)
+
 /* Types of dependency between stages */
 enum dep_type {
 	DEP_NONE = 0,  /* no dependency */
@@ -910,7 +913,7 @@ opdl_ring_create(const char *name, uint32_t num_slots, uint32_t slot_size,
 			RTE_CACHE_LINE_MASK) != 0);
 	RTE_BUILD_BUG_ON((offsetof(struct opdl_ring, slots) &
 			RTE_CACHE_LINE_MASK) != 0);
-	RTE_BUILD_BUG_ON(!rte_is_power_of_2(OPDL_DISCLAIMS_PER_LCORE));
+	RTE_BUILD_BUG_ON(!IS_POWER_OF_2(OPDL_DISCLAIMS_PER_LCORE));
 
 	/* Parameter checking */
 	if (name == NULL) {
-- 
2.39.2

[PATCH v2 2/3] net/sfc: fix non-constant expression inr RTE_BUILD_BUG_ON()

[Stephen Hemminger]

The macro RTE_MIN has some hidden assignments to provide type
safety which means the statement can not be fully evaluted in
first pass of compiler. Replace RTE_MIN() with equivalent macro.

This will cause errors from checkpatch about multiple evaluations
of same expression in macro but it is ok in this case.

Fixes: 4f936666d790 ("net/sfc: support TSO for EF100 native datapath")
Cc: ivan.malov@oktetlabs.ru
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/sfc/sfc_ef100_tx.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/sfc/sfc_ef100_tx.c b/drivers/net/sfc/sfc_ef100_tx.c
index 1b6374775f07..f4bcadc1e8e0 100644
--- a/drivers/net/sfc/sfc_ef100_tx.c
+++ b/drivers/net/sfc/sfc_ef100_tx.c
@@ -26,6 +26,10 @@
 #include "sfc_ef100.h"
 #include "sfc_nic_dma_dp.h"
 
+#ifndef MIN
+/* not typesafe but is a constant */
+#define MIN(x, y) ((x) < (y) ? (x) : (y))
+#endif
 
 #define sfc_ef100_tx_err(_txq, ...) \
 	SFC_DP_LOG(SFC_KVARG_DATAPATH_EF100, ERR, &(_txq)->dp.dpq, __VA_ARGS__)
@@ -563,8 +567,7 @@ sfc_ef100_tx_pkt_descs_max(const struct rte_mbuf *m)
 		 * (split into many Tx descriptors).
 		 */
 		RTE_BUILD_BUG_ON(SFC_EF100_TX_SEND_DESC_LEN_MAX <
-				 RTE_MIN((unsigned int)EFX_MAC_PDU_MAX,
-				 SFC_MBUF_SEG_LEN_MAX));
+				 MIN((unsigned int)EFX_MAC_PDU_MAX, SFC_MBUF_SEG_LEN_MAX));
 	}
 
 	if (m->ol_flags & sfc_dp_mport_override) {
-- 
2.39.2

[PATCH v2 3/3] eal: replace out of bounds VLA with static_assert

[Stephen Hemminger]

Both Gcc, clang and MSVC have better way to do compile time
assertions rather than using out of bounds array access.
The old method would fail if -Wvla is enabled because compiler
can't determine size in that code.  Also, the use of new
_Static_assert will catch broken code that is passing non-constant
expression to RTE_BUILD_BUG_ON().

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/eal/include/rte_common.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index c1ba32d00e47..bea7c0e57d5e 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -16,6 +16,7 @@
 extern "C" {
 #endif
 
+#include <assert.h>
 #include <stdint.h>
 #include <limits.h>
 
@@ -495,7 +496,7 @@ rte_is_aligned(const void * const __rte_restrict ptr, const unsigned int align)
 /**
  * Triggers an error at compilation time if the condition is true.
  */
-#define RTE_BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
+#define RTE_BUILD_BUG_ON(condition) static_assert(!(condition), #condition)
 
 /*********** Cache line related macros ********/
 
-- 
2.39.2

Re: [PATCH v2 1/3] event/opdl: fix non-constant compile time assertion

[Bruce Richardson]

On Mon, Nov 13, 2023 at 09:06:03AM -0800, Stephen Hemminger wrote:
> RTE_BUILD_BUG_ON() was being used with a non-constant value.
> The inline function rte_is_power_of_2() is not constant since
> inline expansion happens later in the compile process.
> Replace it with macro which will be constant.
> 
> Fixes: 4236ce9bf5bf ("event/opdl: add OPDL ring infrastructure library")
> Cc: liang.j.ma@intel.com
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  drivers/event/opdl/opdl_ring.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [PATCH v2 3/3] eal: replace out of bounds VLA with static_assert

[Bruce Richardson]

On Mon, Nov 13, 2023 at 09:06:05AM -0800, Stephen Hemminger wrote:
> Both Gcc, clang and MSVC have better way to do compile time
> assertions rather than using out of bounds array access.
> The old method would fail if -Wvla is enabled because compiler
> can't determine size in that code.  Also, the use of new
> _Static_assert will catch broken code that is passing non-constant
> expression to RTE_BUILD_BUG_ON().
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---
>  lib/eal/include/rte_common.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
> index c1ba32d00e47..bea7c0e57d5e 100644
> --- a/lib/eal/include/rte_common.h
> +++ b/lib/eal/include/rte_common.h
> @@ -16,6 +16,7 @@
>  extern "C" {
>  #endif
>  
> +#include <assert.h>
>  #include <stdint.h>
>  #include <limits.h>
>  
> @@ -495,7 +496,7 @@ rte_is_aligned(const void * const __rte_restrict ptr, const unsigned int align)
>  /**
>   * Triggers an error at compilation time if the condition is true.
>   */
> -#define RTE_BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
> +#define RTE_BUILD_BUG_ON(condition) static_assert(!(condition), #condition)
>  
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

And as for the static_assert vs BUILD_BUG_ON debate, I think I'd go with
using static_assert explicitly and only keeping BUILD_BUG_ON for backward
compatbility. Reason being that you can give a reasonable error message
with the assert.

Re: [PATCH v2 1/3] event/opdl: fix non-constant compile time assertion

[Tyler Retzlaff]

On Mon, Nov 13, 2023 at 09:06:03AM -0800, Stephen Hemminger wrote:
> RTE_BUILD_BUG_ON() was being used with a non-constant value.
> The inline function rte_is_power_of_2() is not constant since
> inline expansion happens later in the compile process.
> Replace it with macro which will be constant.
> 
> Fixes: 4236ce9bf5bf ("event/opdl: add OPDL ring infrastructure library")
> Cc: liang.j.ma@intel.com
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---

Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>

Re: [PATCH v2 2/3] net/sfc: fix non-constant expression inr RTE_BUILD_BUG_ON()

[Tyler Retzlaff]

On Mon, Nov 13, 2023 at 09:06:04AM -0800, Stephen Hemminger wrote:
> The macro RTE_MIN has some hidden assignments to provide type
> safety which means the statement can not be fully evaluted in
> first pass of compiler. Replace RTE_MIN() with equivalent macro.
> 
> This will cause errors from checkpatch about multiple evaluations
> of same expression in macro but it is ok in this case.
> 
> Fixes: 4f936666d790 ("net/sfc: support TSO for EF100 native datapath")
> Cc: ivan.malov@oktetlabs.ru
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---

Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>

Re: [PATCH v2 3/3] eal: replace out of bounds VLA with static_assert

[Tyler Retzlaff]

On Mon, Nov 13, 2023 at 05:12:48PM +0000, Bruce Richardson wrote:
> On Mon, Nov 13, 2023 at 09:06:05AM -0800, Stephen Hemminger wrote:
> > Both Gcc, clang and MSVC have better way to do compile time
> > assertions rather than using out of bounds array access.
> > The old method would fail if -Wvla is enabled because compiler
> > can't determine size in that code.  Also, the use of new
> > _Static_assert will catch broken code that is passing non-constant
> > expression to RTE_BUILD_BUG_ON().
> > 
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > ---
> >  lib/eal/include/rte_common.h | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
> > index c1ba32d00e47..bea7c0e57d5e 100644
> > --- a/lib/eal/include/rte_common.h
> > +++ b/lib/eal/include/rte_common.h
> > @@ -16,6 +16,7 @@
> >  extern "C" {
> >  #endif
> >  
> > +#include <assert.h>
> >  #include <stdint.h>
> >  #include <limits.h>
> >  
> > @@ -495,7 +496,7 @@ rte_is_aligned(const void * const __rte_restrict ptr, const unsigned int align)
> >  /**
> >   * Triggers an error at compilation time if the condition is true.
> >   */
> > -#define RTE_BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
> > +#define RTE_BUILD_BUG_ON(condition) static_assert(!(condition), #condition)
> >  
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> 
> And as for the static_assert vs BUILD_BUG_ON debate, I think I'd go with
> using static_assert explicitly and only keeping BUILD_BUG_ON for backward
> compatbility. Reason being that you can give a reasonable error message
> with the assert.

my vote is for the same, i would prefer to just use standard constructs
directly. i do wish the C11 version didn't require a message but
regardless let's use the standard version.

additionally, can we have an addition to checkpatches.pl (if the above
is agreed) that prevents further additions of RTE_BUILD_BUG_ON usage?

Re: [PATCH v2 0/3] use static_assertion for build errors

[Ferruh Yigit]

On 11/13/2023 5:06 PM, Stephen Hemminger wrote:
> This series fixes a couple places where expressions that could not
> be evaluated as constant early in compiler passes were used. And then
> converts RTE_BUILD_BUG_ON() with static_assert.
> 
> Stephen Hemminger (3):
>   event/opdl: fix non-constant compile time assertion
>   net/sfc: fix non-constant expression inr RTE_BUILD_BUG_ON()
>   eal: replace out of bounds VLA with static_assert
> 

Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>


I am getting more build errors [1], [2].



[1] `meson --buildtype=debug build`

In file included from ../lib/eal/include/dev_driver.h:12,
                 from ../lib/ethdev/ethdev_driver.h:23,
                 from ../drivers/net/i40e/i40e_rxtx_vec_sse.c:6:
../drivers/net/i40e/i40e_rxtx_vec_sse.c: In function ‘descs_to_fdir_32b’:
../lib/eal/include/rte_common.h:499:51: error: expression in static
assertion is not constant
  499 | #define RTE_BUILD_BUG_ON(condition) static_assert(!(condition),
#condition)
      |                                                   ^~~~~~~~~~~~
../drivers/net/i40e/i40e_rxtx_vec_sse.c:147:9: note: in expansion of
macro ‘RTE_BUILD_BUG_ON’
  147 |         RTE_BUILD_BUG_ON(RTE_MBUF_F_RX_FDIR_ID != (1 <<
FDIR_ID_BIT_SHIFT));
      |         ^~~~~~~~~~~~~~~~



[2] `CC=clang meson --buildtype=debugoptimized build`

../lib/mempool/rte_mempool.c:749:2: error: static_assert expression is
not an integral constant expression

RTE_BUILD_BUG_ON(CALC_CACHE_FLUSHTHRESH(RTE_MEMPOOL_CACHE_MAX_SIZE) >

^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../lib/eal/include/rte_common.h:499:51: note: expanded from macro
'RTE_BUILD_BUG_ON'
#define RTE_BUILD_BUG_ON(condition) static_assert(!(condition), #condition)
                                                  ^~~~~~~~~~~~
1 error generated.

RE: [PATCH v2 0/3] use static_assertion for build errors

[Morten Brørup]

> From: Ferruh Yigit [mailto:ferruh.yigit@amd.com]
> Sent: Monday, 13 November 2023 19.14
> 
> On 11/13/2023 5:06 PM, Stephen Hemminger wrote:
> > This series fixes a couple places where expressions that could not
> > be evaluated as constant early in compiler passes were used. And then
> > converts RTE_BUILD_BUG_ON() with static_assert.
> >
> 
> Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>
> 
> 
> I am getting more build errors [1], [2].
> 

[...]

> [2] `CC=clang meson --buildtype=debugoptimized build`
> 
> ../lib/mempool/rte_mempool.c:749:2: error: static_assert expression is
> not an integral constant expression
> 
> RTE_BUILD_BUG_ON(CALC_CACHE_FLUSHTHRESH(RTE_MEMPOOL_CACHE_MAX_SIZE) >
> 
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../lib/eal/include/rte_common.h:499:51: note: expanded from macro
> 'RTE_BUILD_BUG_ON'
> #define RTE_BUILD_BUG_ON(condition) static_assert(!(condition),
> #condition)
>                                                   ^~~~~~~~~~~~
> 1 error generated.

Perhaps you can fix it in rte_mempool.c... the CACHE_FLUSHTHRESH_MULTIPLIER constant seems to be only used for the CALC_CACHE_FLUSHTHRESH() macro, so fix it like this:

-#define CACHE_FLUSHTHRESH_MULTIPLIER 1.5
-#define CALC_CACHE_FLUSHTHRESH(c)	\
-	((typeof(c))((c) * CACHE_FLUSHTHRESH_MULTIPLIER))
+/* Cache flush threshold multiplier is 1.5 = 3/2. */
+#define CALC_CACHE_FLUSHTHRESH(c) (c * 3 / 2)

Re: [PATCH v2 0/3] use static_assertion for build errors

[Tyler Retzlaff]

On Mon, Nov 13, 2023 at 06:13:30PM +0000, Ferruh Yigit wrote:
> On 11/13/2023 5:06 PM, Stephen Hemminger wrote:
> > This series fixes a couple places where expressions that could not
> > be evaluated as constant early in compiler passes were used. And then
> > converts RTE_BUILD_BUG_ON() with static_assert.
> > 
> > Stephen Hemminger (3):
> >   event/opdl: fix non-constant compile time assertion
> >   net/sfc: fix non-constant expression inr RTE_BUILD_BUG_ON()
> >   eal: replace out of bounds VLA with static_assert
> > 
> 
> Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>
> 
> 
> I am getting more build errors [1], [2].
> 
> 
> 
> [1] `meson --buildtype=debug build`
> 
> In file included from ../lib/eal/include/dev_driver.h:12,
>                  from ../lib/ethdev/ethdev_driver.h:23,
>                  from ../drivers/net/i40e/i40e_rxtx_vec_sse.c:6:
> ../drivers/net/i40e/i40e_rxtx_vec_sse.c: In function ‘descs_to_fdir_32b’:
> ../lib/eal/include/rte_common.h:499:51: error: expression in static
> assertion is not constant
>   499 | #define RTE_BUILD_BUG_ON(condition) static_assert(!(condition),
> #condition)
>       |                                                   ^~~~~~~~~~~~
> ../drivers/net/i40e/i40e_rxtx_vec_sse.c:147:9: note: in expansion of
> macro ‘RTE_BUILD_BUG_ON’
>   147 |         RTE_BUILD_BUG_ON(RTE_MBUF_F_RX_FDIR_ID != (1 <<
> FDIR_ID_BIT_SHIFT));

this one is fixable by just making it a constant expression instead of a
const variable.

- i40e_rxtx_vec_sse.c:  const uint32_t FDIR_ID_BIT_SHIFT = 13;
+ i40e_rxtx_vec_sse.c:  #define FDIR_ID_BIT_SHIFT (13u)

... whatever ...

+ i40e_rxtx_vec_sse.c:  #undef FDIR_ID_BIT_SHIFT

ty

Re: [PATCH v2 2/3] net/sfc: fix non-constant expression inr RTE_BUILD_BUG_ON()

[Stephen Hemminger]

On Mon, 13 Nov 2023 09:06:04 -0800
Stephen Hemminger <stephen@networkplumber.org> wrote:

> The macro RTE_MIN has some hidden assignments to provide type
> safety which means the statement can not be fully evaluted in
> first pass of compiler. Replace RTE_MIN() with equivalent macro.
> 
> This will cause errors from checkpatch about multiple evaluations
> of same expression in macro but it is ok in this case.
> 
> Fixes: 4f936666d790 ("net/sfc: support TSO for EF100 native datapath")
> Cc: ivan.malov@oktetlabs.ru
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Building with clang finds another issue.
../drivers/net/sfc/sfc_rx.c:158:3: error: expected expression
                RTE_BUILD_BUG_ON(RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN != 0);
yet
lib/mbuf/rte_mbuf_core.h:#define RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN 0

Re: [PATCH v2 2/3] net/sfc: fix non-constant expression inr RTE_BUILD_BUG_ON()

[Tyler Retzlaff]

On Mon, Nov 13, 2023 at 02:13:26PM -0800, Stephen Hemminger wrote:
> On Mon, 13 Nov 2023 09:06:04 -0800
> Stephen Hemminger <stephen@networkplumber.org> wrote:
> 
> > The macro RTE_MIN has some hidden assignments to provide type
> > safety which means the statement can not be fully evaluted in
> > first pass of compiler. Replace RTE_MIN() with equivalent macro.
> > 
> > This will cause errors from checkpatch about multiple evaluations
> > of same expression in macro but it is ok in this case.
> > 
> > Fixes: 4f936666d790 ("net/sfc: support TSO for EF100 native datapath")
> > Cc: ivan.malov@oktetlabs.ru
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> 
> Building with clang finds another issue.
> ../drivers/net/sfc/sfc_rx.c:158:3: error: expected expression
>                 RTE_BUILD_BUG_ON(RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN != 0);
> yet
> lib/mbuf/rte_mbuf_core.h:#define RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN 0

curious. do you have the gcc -E / clang -E preprocessed output for the
expansion? wonder what it looks like.

Re: [PATCH v2 2/3] net/sfc: fix non-constant expression inr RTE_BUILD_BUG_ON()

[Stephen Hemminger]

On Mon, 13 Nov 2023 14:28:55 -0800
Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:

> On Mon, Nov 13, 2023 at 02:13:26PM -0800, Stephen Hemminger wrote:
> > On Mon, 13 Nov 2023 09:06:04 -0800
> > Stephen Hemminger <stephen@networkplumber.org> wrote:
> >   
> > > The macro RTE_MIN has some hidden assignments to provide type
> > > safety which means the statement can not be fully evaluted in
> > > first pass of compiler. Replace RTE_MIN() with equivalent macro.
> > > 
> > > This will cause errors from checkpatch about multiple evaluations
> > > of same expression in macro but it is ok in this case.
> > > 
> > > Fixes: 4f936666d790 ("net/sfc: support TSO for EF100 native datapath")
> > > Cc: ivan.malov@oktetlabs.ru
> > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>  
> > 
> > Building with clang finds another issue.
> > ../drivers/net/sfc/sfc_rx.c:158:3: error: expected expression
> >                 RTE_BUILD_BUG_ON(RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN != 0);
> > yet
> > lib/mbuf/rte_mbuf_core.h:#define RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN 0  
> 
> curious. do you have the gcc -E / clang -E preprocessed output for the
> expansion? wonder what it looks like.

yet another one from clang.
../drivers/net/sfc/sfc_ef10_rx_ev.h:142:4: error: expected expression
                 RTE_BUILD_BUG_ON(ESE_FZ_L4_CLASS_TCP != ESE_DE_L4_CLASS_TCP);

drivers/common/sfc_efx/base/efx_regs_ef10.h:#define     ESE_FZ_L4_CLASS_TCP 1
drivers/common/sfc_efx/base/efx_regs_ef10.h:#define     ESE_DE_L4_CLASS_TCP 1

Re: [PATCH v2 2/3] net/sfc: fix non-constant expression inr RTE_BUILD_BUG_ON()

[Stephen Hemminger]

On Mon, 13 Nov 2023 14:28:55 -0800
Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:

> On Mon, Nov 13, 2023 at 02:13:26PM -0800, Stephen Hemminger wrote:
> > On Mon, 13 Nov 2023 09:06:04 -0800
> > Stephen Hemminger <stephen@networkplumber.org> wrote:
> >   
> > > The macro RTE_MIN has some hidden assignments to provide type
> > > safety which means the statement can not be fully evaluted in
> > > first pass of compiler. Replace RTE_MIN() with equivalent macro.
> > > 
> > > This will cause errors from checkpatch about multiple evaluations
> > > of same expression in macro but it is ok in this case.
> > > 
> > > Fixes: 4f936666d790 ("net/sfc: support TSO for EF100 native datapath")
> > > Cc: ivan.malov@oktetlabs.ru
> > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>  
> > 
> > Building with clang finds another issue.
> > ../drivers/net/sfc/sfc_rx.c:158:3: error: expected expression
> >                 RTE_BUILD_BUG_ON(RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN != 0);
> > yet
> > lib/mbuf/rte_mbuf_core.h:#define RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN 0  
> 
> curious. do you have the gcc -E / clang -E preprocessed output for the
> expansion? wonder what it looks like.

Building with clang and -E.

This code:
	switch (desc_flags & (EFX_PKT_IPV4 | EFX_CKSUM_IPV4)) {
	case (EFX_PKT_IPV4 | EFX_CKSUM_IPV4):
		mbuf_flags |= RTE_MBUF_F_RX_IP_CKSUM_GOOD;
		break;
	case EFX_PKT_IPV4:
		mbuf_flags |= RTE_MBUF_F_RX_IP_CKSUM_BAD;
		break;
	default:
		RTE_BUILD_BUG_ON(RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN != 0);
		SFC_ASSERT((mbuf_flags & RTE_MBUF_F_RX_IP_CKSUM_MASK) ==
			   RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN);
		break;
	}

Becomes:
 switch (desc_flags & (0x0800 | 0x0040)) {
 case (0x0800 | 0x0040):
  mbuf_flags |= (1ULL << 7);
  break;
 case 0x0800:
  mbuf_flags |= (1ULL << 4);
  break;
 default:
  _Static_assert(!(0 != 0), "RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN != 0");
  do {} while (0);

  break;
 }

Doing same with Gcc:
 switch (desc_flags & (0x0800 | 0x0040)) {
 case (0x0800 | 0x0040):
  mbuf_flags |= (1ULL << 7);
  break;
 case 0x0800:
  mbuf_flags |= (1ULL << 4);
  break;
 default:
  
# 158 "./drivers/net/sfc/sfc_rx.c" 3 4
 _Static_assert
# 158 "./drivers/net/sfc/sfc_rx.c"
 (!(0 != 0), "RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN != 0");
  do {} while (0)
                                     ;

Re: [PATCH v2 2/3] net/sfc: fix non-constant expression inr RTE_BUILD_BUG_ON()

[Stephen Hemminger]

On Mon, 13 Nov 2023 16:16:35 -0800
Stephen Hemminger <stephen@networkplumber.org> wrote:

>   _Static_assert(!(0 != 0), "RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN != 0");

Looks like a clang bug, or something about the other compiler flags
because compiling just this part is fine.

RE: [PATCH v2 2/3] net/sfc: fix non-constant expression inr RTE_BUILD_BUG_ON()

[Morten Brørup]

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Tuesday, 14 November 2023 01.22
> 
> On Mon, 13 Nov 2023 16:16:35 -0800
> Stephen Hemminger <stephen@networkplumber.org> wrote:
> 
> >   _Static_assert(!(0 != 0), "RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN != 0");
> 
> Looks like a clang bug, or something about the other compiler flags
> because compiling just this part is fine.

Ideas:

Perhaps this file is not compiled as C11, so _Static_assert is not recognized. Does that part compile in the same file?
Or perhaps the C version or some compiler flags are modified (by a #pragma or similar) in the file.

[PATCH v3 00/10] replace uses of zero length array

[Stephen Hemminger]

Zero length arrays are a GNU extension that has been
superseded by flex arrays.

https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html

These are places found by cocci/zero_length_array.cocci

Series-acked-by: Morten Brørup <mb@smartsharesystems.com>

v3 - fix misuse of flex array in dpaxx found by clang

v2 - rebased on 23.11-rc3
     more places started using zero length array

Stephen Hemminger (10):
  member: replace zero length array with flex array
  cryptodev: replace zero length array with flex array
  security: replace zero length array with flex array
  pipeline: replace zero length array with flex array
  net/nfp: replace zero length array with flex array
  net/enic: replace zero length array with flex array
  net/mlx5: replace zero length array with flex array
  pdcp: replace zero length array with flex array
  net/cpfl: replace zero length array with flex array
  common/dpaxx: remove zero length array

 drivers/common/dpaax/caamflib/desc/ipsec.h | 6 ++----
 drivers/common/mlx5/mlx5_prm.h             | 2 +-
 drivers/net/cpfl/cpfl_flow_engine_fxp.c    | 2 +-
 drivers/net/enic/base/vnic_devcmd.h        | 2 +-
 drivers/net/mlx5/mlx5.h                    | 4 ++--
 drivers/net/mlx5/mlx5_flow.h               | 2 +-
 drivers/net/mlx5/mlx5_tx.h                 | 2 +-
 drivers/net/nfp/flower/nfp_flower_cmsg.h   | 2 +-
 lib/cryptodev/cryptodev_pmd.h              | 2 +-
 lib/member/rte_member_heap.h               | 2 +-
 lib/pdcp/pdcp_entity.h                     | 2 +-
 lib/pipeline/rte_swx_pipeline_internal.h   | 2 +-
 lib/security/rte_security_driver.h         | 2 +-
 13 files changed, 15 insertions(+), 17 deletions(-)

-- 
2.42.0

[PATCH v3 01/10] member: replace zero length array with flex array

[Stephen Hemminger]

Zero length arrays are GNU extension. Replace with
standard flex array.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Reviewed-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/member/rte_member_heap.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/member/rte_member_heap.h b/lib/member/rte_member_heap.h
index 9c4a01aebe95..ab6319bc2de4 100644
--- a/lib/member/rte_member_heap.h
+++ b/lib/member/rte_member_heap.h
@@ -26,7 +26,7 @@ struct hash {
 	uint16_t bkt_cnt;
 	uint16_t num_item;
 	uint32_t seed;
-	struct hash_bkt buckets[0];
+	struct hash_bkt buckets[];
 };
 
 struct node {
-- 
2.42.0

[PATCH v3 02/10] cryptodev: replace zero length array with flex array

[Stephen Hemminger]

Zero length arrays are GNU extension. Replace with
standard flex array.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Reviewed-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/cryptodev/cryptodev_pmd.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/cryptodev/cryptodev_pmd.h b/lib/cryptodev/cryptodev_pmd.h
index 3bb3d95c1338..0732b356883c 100644
--- a/lib/cryptodev/cryptodev_pmd.h
+++ b/lib/cryptodev/cryptodev_pmd.h
@@ -153,7 +153,7 @@ struct rte_cryptodev_sym_session {
 
 	RTE_MARKER cacheline1 __rte_cache_min_aligned;
 	/**< Second cache line - start of the driver session data */
-	uint8_t driver_priv_data[0];
+	uint8_t driver_priv_data[];
 	/**< Driver specific session data, variable size */
 };
 
-- 
2.42.0

[PATCH v3 03/10] security: replace zero length array with flex array

[Stephen Hemminger]

Zero length arrays are GNU extension. Replace with
standard flex array.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Reviewed-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/security/rte_security_driver.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/security/rte_security_driver.h b/lib/security/rte_security_driver.h
index 62664dacdbb4..faa4074f1965 100644
--- a/lib/security/rte_security_driver.h
+++ b/lib/security/rte_security_driver.h
@@ -33,7 +33,7 @@ struct rte_security_session {
 	/**< session private data IOVA address */
 
 	RTE_MARKER cacheline1 __rte_cache_min_aligned;
-	uint8_t driver_priv_data[0];
+	uint8_t driver_priv_data[];
 	/**< Private session material, variable size (depends on driver) */
 };
 
-- 
2.42.0

[PATCH v3 04/10] pipeline: replace zero length array with flex array

[Stephen Hemminger]

Zero length arrays are GNU extension. Replace with
standard flex array.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Reviewed-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/pipeline/rte_swx_pipeline_internal.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/pipeline/rte_swx_pipeline_internal.h b/lib/pipeline/rte_swx_pipeline_internal.h
index a67b6e965de7..8ec12263b989 100644
--- a/lib/pipeline/rte_swx_pipeline_internal.h
+++ b/lib/pipeline/rte_swx_pipeline_internal.h
@@ -213,7 +213,7 @@ TAILQ_HEAD(rss_tailq, rss);
 
 struct rss_runtime {
 	uint32_t key_size; /* key size in bytes. */
-	uint8_t key[0]; /* key. */
+	uint8_t key[]; /* key. */
 };
 
 /*
-- 
2.42.0

[PATCH v3 05/10] net/nfp: replace zero length array with flex array

[Stephen Hemminger]

Zero length arrays are GNU extension. Replace with
standard flex array.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Niklas Söderlund <niklas.soderlund@corigine.com>
Reviewed-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 drivers/net/nfp/flower/nfp_flower_cmsg.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/nfp/flower/nfp_flower_cmsg.h b/drivers/net/nfp/flower/nfp_flower_cmsg.h
index c2938fb6f63c..f00d2e838d8f 100644
--- a/drivers/net/nfp/flower/nfp_flower_cmsg.h
+++ b/drivers/net/nfp/flower/nfp_flower_cmsg.h
@@ -73,7 +73,7 @@ struct nfp_flower_cmsg_mac_repr {
 		uint8_t info;
 		uint8_t nbi_port;
 		uint8_t phys_port;
-	} ports[0];
+	} ports[];
 };
 
 /*
-- 
2.42.0

[PATCH v3 06/10] net/enic: replace zero length array with flex array

[Stephen Hemminger]

Zero length arrays are GNU extension. Replace with
standard flex array.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: John Daley <johndale@cisco.com>
Reviewed-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 drivers/net/enic/base/vnic_devcmd.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/enic/base/vnic_devcmd.h b/drivers/net/enic/base/vnic_devcmd.h
index 253a791c3f65..f91cc3078d63 100644
--- a/drivers/net/enic/base/vnic_devcmd.h
+++ b/drivers/net/enic/base/vnic_devcmd.h
@@ -765,7 +765,7 @@ struct vnic_devcmd_notify {
 struct vnic_devcmd_provinfo {
 	uint8_t oui[3];
 	uint8_t type;
-	uint8_t data[0];
+	uint8_t data[];
 };
 
 /*
-- 
2.42.0

[PATCH v3 07/10] net/mlx5: replace zero length array with flex array

[Stephen Hemminger]

Zero length arrays are GNU extension. Replace with
standard flex array.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Reviewed-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 drivers/common/mlx5/mlx5_prm.h | 2 +-
 drivers/net/mlx5/mlx5.h        | 4 ++--
 drivers/net/mlx5/mlx5_flow.h   | 2 +-
 drivers/net/mlx5/mlx5_tx.h     | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/common/mlx5/mlx5_prm.h b/drivers/common/mlx5/mlx5_prm.h
index 9e22dce6da13..932b89bd79d3 100644
--- a/drivers/common/mlx5/mlx5_prm.h
+++ b/drivers/common/mlx5/mlx5_prm.h
@@ -5181,7 +5181,7 @@ struct mlx5_ifc_flow_context_bits {
 	u8 reserved_at_e0[0x40];
 	u8 encrypt_decrypt_obj_id[0x20];
 	u8 reserved_at_140[0x16c0];
-	union mlx5_ifc_dest_format_flow_counter_list_auto_bits destination[0];
+	union mlx5_ifc_dest_format_flow_counter_list_auto_bits destination[];
 };
 
 struct mlx5_ifc_set_fte_in_bits {
diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
index f0d63a0ba5f5..89d13900fd3c 100644
--- a/drivers/net/mlx5/mlx5.h
+++ b/drivers/net/mlx5/mlx5.h
@@ -1308,7 +1308,7 @@ struct mlx5_aso_ct_pool {
 	};
 	struct mlx5_aso_sq *sq; /* Async ASO SQ. */
 	struct mlx5_aso_sq *shared_sq; /* Shared ASO SQ. */
-	struct mlx5_aso_ct_action actions[0];
+	struct mlx5_aso_ct_action actions[];
 	/* CT action structures bulk. */
 };
 
@@ -1325,7 +1325,7 @@ struct mlx5_aso_ct_pools_mng {
 	rte_spinlock_t ct_sl; /* The ASO CT free list lock. */
 	rte_rwlock_t resize_rwl; /* The ASO CT pool resize lock. */
 	struct aso_ct_list free_cts; /* Free ASO CT objects list. */
-	struct mlx5_aso_sq aso_sqs[0]; /* ASO queue objects. */
+	struct mlx5_aso_sq aso_sqs[]; /* ASO queue objects. */
 };
 
 #ifdef PEDANTIC
diff --git a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flow.h
index 6dde9de688b9..b35079b30a6e 100644
--- a/drivers/net/mlx5/mlx5_flow.h
+++ b/drivers/net/mlx5/mlx5_flow.h
@@ -1257,7 +1257,7 @@ struct rte_flow_hw {
 	cnt_id_t cnt_id;
 	uint32_t mtr_id;
 	uint32_t rule_idx;
-	uint8_t rule[0]; /* HWS layer data struct. */
+	uint8_t rule[]; /* HWS layer data struct. */
 } __rte_packed;
 
 #ifdef PEDANTIC
diff --git a/drivers/net/mlx5/mlx5_tx.h b/drivers/net/mlx5/mlx5_tx.h
index e59ce37667ba..2045e5174e6d 100644
--- a/drivers/net/mlx5/mlx5_tx.h
+++ b/drivers/net/mlx5/mlx5_tx.h
@@ -171,7 +171,7 @@ struct mlx5_txq_data {
 	struct mlx5_txq_stats stats; /* TX queue counters. */
 	struct mlx5_txq_stats stats_reset; /* stats on last reset. */
 	struct mlx5_uar_data uar_data;
-	struct rte_mbuf *elts[0];
+	struct rte_mbuf *elts[];
 	/* Storage for queued packets, must be the last field. */
 } __rte_cache_aligned;
 
-- 
2.42.0

[PATCH v3 08/10] pdcp: replace zero length array with flex array

[Stephen Hemminger]

Zero length arrays are GNU extension. Replace with
standard flex array.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Reviewed-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
Acked-by: Anoob Joseph <anoobj@marvell.com>
---
 lib/pdcp/pdcp_entity.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/pdcp/pdcp_entity.h b/lib/pdcp/pdcp_entity.h
index 4fc6342a5ced..f854192e98dc 100644
--- a/lib/pdcp/pdcp_entity.h
+++ b/lib/pdcp/pdcp_entity.h
@@ -185,7 +185,7 @@ struct entity_priv_dl_part {
 	/** Reorder packet buffer */
 	struct pdcp_reorder reorder;
 	/** Bitmap memory region */
-	uint8_t bitmap_mem[0];
+	uint8_t bitmap_mem[];
 };
 
 struct entity_priv_ul_part {
-- 
2.42.0

[PATCH v3 09/10] net/cpfl: replace zero length array with flex array

[Stephen Hemminger]

Zero length arrays are GNU extension. Replace with
standard flex array.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Reviewed-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 drivers/net/cpfl/cpfl_flow_engine_fxp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/cpfl/cpfl_flow_engine_fxp.c b/drivers/net/cpfl/cpfl_flow_engine_fxp.c
index 8a4e1419b4db..39a281fa61ee 100644
--- a/drivers/net/cpfl/cpfl_flow_engine_fxp.c
+++ b/drivers/net/cpfl/cpfl_flow_engine_fxp.c
@@ -53,7 +53,7 @@ struct cpfl_rule_info_meta {
 	uint32_t pr_num;			/* number of pattern rules */
 	uint32_t mr_num;			/* number of modification rules */
 	uint32_t rule_num;			/* number of all rules */
-	struct cpfl_rule_info rules[0];
+	struct cpfl_rule_info rules[];
 };
 
 static uint32_t cpfl_fxp_mod_idx_alloc(struct cpfl_adapter_ext *ad);
-- 
2.42.0

[PATCH v3 10/10] common/dpaxx: remove zero length array

[Stephen Hemminger]

There is a place holder zero length array in this driver.
But since the structure is embedded in other structures,
it could not have been safely used anyway.
There doesn't appear to be any uses of it in the current code.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/common/dpaax/caamflib/desc/ipsec.h | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/common/dpaax/caamflib/desc/ipsec.h b/drivers/common/dpaax/caamflib/desc/ipsec.h
index 95fc3ea5ba3b..9d59b93292f9 100644
--- a/drivers/common/dpaax/caamflib/desc/ipsec.h
+++ b/drivers/common/dpaax/caamflib/desc/ipsec.h
@@ -336,7 +336,6 @@ struct ipsec_encap_gcm {
  * @ip_hdr_len: optional IP Header length (in bytes)
  *  reserved - 16b
  *  Opt. IP Hdr Len - 16b
- * @ip_hdr: optional IP Header content (only for IPsec legacy mode)
  */
 struct ipsec_encap_pdb {
 	uint32_t options;
@@ -350,7 +349,6 @@ struct ipsec_encap_pdb {
 	};
 	uint32_t spi;
 	uint32_t ip_hdr_len;
-	uint8_t ip_hdr[0];
 };
 
 static inline unsigned int
@@ -776,7 +774,7 @@ cnstr_shdsc_ipsec_encap(uint32_t *descbuf, bool ps, bool swap,
 		PROGRAM_SET_36BIT_ADDR(p);
 	phdr = SHR_HDR(p, share, hdr, 0);
 	__rta_copy_ipsec_encap_pdb(p, pdb, cipherdata->algtype);
-	COPY_DATA(p, pdb->ip_hdr, pdb->ip_hdr_len);
+
 	SET_LABEL(p, hdr);
 	pkeyjmp = JUMP(p, keyjmp, LOCAL_JUMP, ALL_TRUE, BOTH|SHRD);
 	if (authdata->keylen)
@@ -913,7 +911,7 @@ cnstr_shdsc_ipsec_encap_des_aes_xcbc(uint32_t *descbuf,
 	PROGRAM_CNTXT_INIT(p, descbuf, 0);
 	phdr = SHR_HDR(p, share, hdr, 0);
 	__rta_copy_ipsec_encap_pdb(p, pdb, cipherdata->algtype);
-	COPY_DATA(p, pdb->ip_hdr, pdb->ip_hdr_len);
+
 	SET_LABEL(p, hdr);
 	pkeyjump = JUMP(p, keyjump, LOCAL_JUMP, ALL_TRUE, SHRD | SELF);
 	/*
-- 
2.42.0

[PATCH v3 0/5] use static_assert to catch build errors

[Stephen Hemminger]

This series fixes a couple places where expressions that could not
be evaluated as constant early in compiler passes were used. And then
converts RTE_BUILD_BUG_ON() with static_assert.

static_assert() is more picky about the expression has to
be a constant, which also catches some existing undefined
behavior that pre-existed.

Stephen Hemminger (5):
  event/opdl: fix non-constant compile time assertion
  net/sfc: fix non-constant expression in RTE_BUILD_BUG_ON()
  net/i40e: avoid using const variable in assertion
  mempool: avoid floating point expression in static assertion
  eal: replace out of bounds VLA with static_assert

 drivers/event/opdl/opdl_ring.c       |  5 ++++-
 drivers/net/i40e/i40e_ethdev.h       |  1 +
 drivers/net/i40e/i40e_rxtx_vec_sse.c | 10 ++++------
 drivers/net/sfc/sfc_ef100_tx.c       |  7 +++++--
 lib/eal/include/rte_common.h         |  3 ++-
 lib/mempool/rte_mempool.c            |  4 +---
 6 files changed, 17 insertions(+), 13 deletions(-)

-- 
2.43.0

[PATCH v3 1/5] event/opdl: fix non-constant compile time assertion

[Stephen Hemminger]

RTE_BUILD_BUG_ON() was being used with a non-constant value.
The inline function rte_is_power_of_2() is not constant since
inline expansion happens later in the compile process.
Replace it with macro which will be constant.

Fixes: 4236ce9bf5bf ("event/opdl: add OPDL ring infrastructure library")
Cc: liang.j.ma@intel.com
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 drivers/event/opdl/opdl_ring.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/event/opdl/opdl_ring.c b/drivers/event/opdl/opdl_ring.c
index 69392b56bbec..24e0bbe3222d 100644
--- a/drivers/event/opdl/opdl_ring.c
+++ b/drivers/event/opdl/opdl_ring.c
@@ -31,6 +31,9 @@
 #define OPDL_OPA_MASK    (0xFF)
 #define OPDL_OPA_OFFSET  (0x38)
 
+/* Equivalent to rte_is_power_of_2() but as macro. */
+#define IS_POWER_OF_2(x) (((x) & ((x) - 1)) == 0)
+
 /* Types of dependency between stages */
 enum dep_type {
 	DEP_NONE = 0,  /* no dependency */
@@ -910,7 +913,7 @@ opdl_ring_create(const char *name, uint32_t num_slots, uint32_t slot_size,
 			RTE_CACHE_LINE_MASK) != 0);
 	RTE_BUILD_BUG_ON((offsetof(struct opdl_ring, slots) &
 			RTE_CACHE_LINE_MASK) != 0);
-	RTE_BUILD_BUG_ON(!rte_is_power_of_2(OPDL_DISCLAIMS_PER_LCORE));
+	RTE_BUILD_BUG_ON(!IS_POWER_OF_2(OPDL_DISCLAIMS_PER_LCORE));
 
 	/* Parameter checking */
 	if (name == NULL) {
-- 
2.43.0

[PATCH v3 2/5] net/sfc: fix non-constant expression in RTE_BUILD_BUG_ON()

[Stephen Hemminger]

The macro RTE_MIN has some hidden assignments to provide type
safety which means the statement can not be fully evaluated in
first pass of compiler. Replace RTE_MIN() with equivalent macro.

This will cause errors from checkpatch about multiple evaluations
of same expression in macro but it is ok in this case.

Fixes: 4f936666d790 ("net/sfc: support TSO for EF100 native datapath")
Cc: ivan.malov@oktetlabs.ru
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 drivers/net/sfc/sfc_ef100_tx.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/sfc/sfc_ef100_tx.c b/drivers/net/sfc/sfc_ef100_tx.c
index 1b6374775f07..f4bcadc1e8e0 100644
--- a/drivers/net/sfc/sfc_ef100_tx.c
+++ b/drivers/net/sfc/sfc_ef100_tx.c
@@ -26,6 +26,10 @@
 #include "sfc_ef100.h"
 #include "sfc_nic_dma_dp.h"
 
+#ifndef MIN
+/* not typesafe but is a constant */
+#define MIN(x, y) ((x) < (y) ? (x) : (y))
+#endif
 
 #define sfc_ef100_tx_err(_txq, ...) \
 	SFC_DP_LOG(SFC_KVARG_DATAPATH_EF100, ERR, &(_txq)->dp.dpq, __VA_ARGS__)
@@ -563,8 +567,7 @@ sfc_ef100_tx_pkt_descs_max(const struct rte_mbuf *m)
 		 * (split into many Tx descriptors).
 		 */
 		RTE_BUILD_BUG_ON(SFC_EF100_TX_SEND_DESC_LEN_MAX <
-				 RTE_MIN((unsigned int)EFX_MAC_PDU_MAX,
-				 SFC_MBUF_SEG_LEN_MAX));
+				 MIN((unsigned int)EFX_MAC_PDU_MAX, SFC_MBUF_SEG_LEN_MAX));
 	}
 
 	if (m->ol_flags & sfc_dp_mport_override) {
-- 
2.43.0

[PATCH v3 3/5] net/i40e: avoid using const variable in assertion

[Stephen Hemminger]

Clang does not allow const variable in a static_assert
expression.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/i40e/i40e_ethdev.h       |  1 +
 drivers/net/i40e/i40e_rxtx_vec_sse.c | 10 ++++------
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
index 1bbe7ad37600..445e1c0b381f 100644
--- a/drivers/net/i40e/i40e_ethdev.h
+++ b/drivers/net/i40e/i40e_ethdev.h
@@ -278,6 +278,7 @@ enum i40e_flxpld_layer_idx {
 #define I40E_DEFAULT_DCB_APP_PRIO   3
 
 #define I40E_FDIR_PRG_PKT_CNT       128
+#define I40E_FDIR_ID_BIT_SHIFT	    13
 
 /*
  * Struct to store flow created.
diff --git a/drivers/net/i40e/i40e_rxtx_vec_sse.c b/drivers/net/i40e/i40e_rxtx_vec_sse.c
index 9200a23ff662..2d4480a7651b 100644
--- a/drivers/net/i40e/i40e_rxtx_vec_sse.c
+++ b/drivers/net/i40e/i40e_rxtx_vec_sse.c
@@ -143,10 +143,9 @@ descs_to_fdir_32b(volatile union i40e_rx_desc *rxdp, struct rte_mbuf **rx_pkt)
 	/* convert fdir_id_mask into a single bit, then shift as required for
 	 * correct location in the mbuf->olflags
 	 */
-	const uint32_t FDIR_ID_BIT_SHIFT = 13;
-	RTE_BUILD_BUG_ON(RTE_MBUF_F_RX_FDIR_ID != (1 << FDIR_ID_BIT_SHIFT));
+	RTE_BUILD_BUG_ON(RTE_MBUF_F_RX_FDIR_ID != (1 << I40E_FDIR_ID_BIT_SHIFT));
 	v_fd_id_mask = _mm_srli_epi32(v_fd_id_mask, 31);
-	v_fd_id_mask = _mm_slli_epi32(v_fd_id_mask, FDIR_ID_BIT_SHIFT);
+	v_fd_id_mask = _mm_slli_epi32(v_fd_id_mask, I40E_FDIR_ID_BIT_SHIFT);
 
 	/* The returned value must be combined into each mbuf. This is already
 	 * being done for RSS and VLAN mbuf olflags, so return bits to OR in.
@@ -205,10 +204,9 @@ descs_to_fdir_16b(__m128i fltstat, __m128i descs[4], struct rte_mbuf **rx_pkt)
 	descs[0] = _mm_blendv_epi8(descs[0], _mm_setzero_si128(), v_desc0_mask);
 
 	/* Shift to 1 or 0 bit per u32 lane, then to RTE_MBUF_F_RX_FDIR_ID offset */
-	const uint32_t FDIR_ID_BIT_SHIFT = 13;
-	RTE_BUILD_BUG_ON(RTE_MBUF_F_RX_FDIR_ID != (1 << FDIR_ID_BIT_SHIFT));
+	RTE_BUILD_BUG_ON(RTE_MBUF_F_RX_FDIR_ID != (1 << I40E_FDIR_ID_BIT_SHIFT));
 	__m128i v_mask_one_bit = _mm_srli_epi32(v_fdir_id_mask, 31);
-	return _mm_slli_epi32(v_mask_one_bit, FDIR_ID_BIT_SHIFT);
+	return _mm_slli_epi32(v_mask_one_bit, I40E_FDIR_ID_BIT_SHIFT);
 }
 #endif
 
-- 
2.43.0

[PATCH v3 4/5] mempool: avoid floating point expression in static assertion

[Stephen Hemminger]

Clang does not handle casts in static_assert() expressions.
It doesn't like use of floating point to calculate threshold.
Use a different expression with same effect; yes this will cause
checkpatch nag.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/mempool/rte_mempool.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
index b7a19bea7185..ba3a54cfc298 100644
--- a/lib/mempool/rte_mempool.c
+++ b/lib/mempool/rte_mempool.c
@@ -50,9 +50,7 @@ static void
 mempool_event_callback_invoke(enum rte_mempool_event event,
 			      struct rte_mempool *mp);
 
-#define CACHE_FLUSHTHRESH_MULTIPLIER 1.5
-#define CALC_CACHE_FLUSHTHRESH(c)	\
-	((typeof(c))((c) * CACHE_FLUSHTHRESH_MULTIPLIER))
+#define CALC_CACHE_FLUSHTHRESH(c) ((c) + (c) / 2)
 
 #if defined(RTE_ARCH_X86)
 /*
-- 
2.43.0

[PATCH v3 5/5] eal: replace out of bounds VLA with static_assert

[Stephen Hemminger]

Both Gcc, clang and MSVC have better way to do compile time
assertions rather than using out of bounds array access.
The old method would fail if -Wvla is enabled because compiler
can't determine size in that code.  Also, the use of new
_Static_assert will catch broken code that is passing non-constant
expression to RTE_BUILD_BUG_ON().

Need to add brackets {} around the static_assert() to workaround
a bug in clang. Clang was not handling static_assert() in
a switch case properly.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/eal/include/rte_common.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index c1ba32d00e47..413bed23cb73 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -16,6 +16,7 @@
 extern "C" {
 #endif
 
+#include <assert.h>
 #include <stdint.h>
 #include <limits.h>
 
@@ -495,7 +496,7 @@ rte_is_aligned(const void * const __rte_restrict ptr, const unsigned int align)
 /**
  * Triggers an error at compilation time if the condition is true.
  */
-#define RTE_BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
+#define RTE_BUILD_BUG_ON(condition) { static_assert(!(condition), #condition); }
 
 /*********** Cache line related macros ********/
 
-- 
2.43.0

Re: [PATCH v3 5/5] eal: replace out of bounds VLA with static_assert

[Andrew Rybchenko]

On 1/16/24 21:41, Stephen Hemminger wrote:
> Both Gcc, clang and MSVC have better way to do compile time
> assertions rather than using out of bounds array access.
> The old method would fail if -Wvla is enabled because compiler
> can't determine size in that code.  Also, the use of new
> _Static_assert will catch broken code that is passing non-constant
> expression to RTE_BUILD_BUG_ON().
> 
> Need to add brackets {} around the static_assert() to workaround
> a bug in clang. Clang was not handling static_assert() in
> a switch case properly.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>

Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

Does it imply any limitations on Gcc / Clang versions to
be used? Is it documented somewhere in DPDK?

Re: [PATCH v3 5/5] eal: replace out of bounds VLA with static_assert

[Mattias Rönnblom]

On 2024-01-16 19:41, Stephen Hemminger wrote:
> Both Gcc, clang and MSVC have better way to do compile time
> assertions rather than using out of bounds array access.
> The old method would fail if -Wvla is enabled because compiler
> can't determine size in that code.  Also, the use of new
> _Static_assert will catch broken code that is passing non-constant
> expression to RTE_BUILD_BUG_ON().
> 
> Need to add brackets {} around the static_assert() to workaround
> a bug in clang. Clang was not handling static_assert() in
> a switch case properly.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---
>   lib/eal/include/rte_common.h | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
> index c1ba32d00e47..413bed23cb73 100644
> --- a/lib/eal/include/rte_common.h
> +++ b/lib/eal/include/rte_common.h
> @@ -16,6 +16,7 @@
>   extern "C" {
>   #endif
>   
> +#include <assert.h>
>   #include <stdint.h>
>   #include <limits.h>
>   
> @@ -495,7 +496,7 @@ rte_is_aligned(const void * const __rte_restrict ptr, const unsigned int align)
>   /**
>    * Triggers an error at compilation time if the condition is true.
>    */
> -#define RTE_BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
> +#define RTE_BUILD_BUG_ON(condition) { static_assert(!(condition), #condition); }
>   
>   /*********** Cache line related macros ********/
>   

Should one use RTE_BUILD_BUG_ON() or static_assert() in new DPDK code?

If static_assert() occasionally needs a work-around, it sounds like 
keeping using RTE_BUILD_BUG_ON() everywhere is the better choice.

Re: [PATCH v3 2/5] net/sfc: fix non-constant expression in RTE_BUILD_BUG_ON()

[Andrew Rybchenko]

On 1/16/24 21:41, Stephen Hemminger wrote:
> The macro RTE_MIN has some hidden assignments to provide type
> safety which means the statement can not be fully evaluated in
> first pass of compiler. Replace RTE_MIN() with equivalent macro.
> 
> This will cause errors from checkpatch about multiple evaluations
> of same expression in macro but it is ok in this case.
> 
> Fixes: 4f936666d790 ("net/sfc: support TSO for EF100 native datapath")

I'm not sure that it is really a fix.

> Cc: ivan.malov@oktetlabs.ru
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---
>   drivers/net/sfc/sfc_ef100_tx.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/sfc/sfc_ef100_tx.c b/drivers/net/sfc/sfc_ef100_tx.c
> index 1b6374775f07..f4bcadc1e8e0 100644
> --- a/drivers/net/sfc/sfc_ef100_tx.c
> +++ b/drivers/net/sfc/sfc_ef100_tx.c
> @@ -26,6 +26,10 @@
>   #include "sfc_ef100.h"
>   #include "sfc_nic_dma_dp.h"
>   
> +#ifndef MIN
> +/* not typesafe but is a constant */
> +#define MIN(x, y) ((x) < (y) ? (x) : (y))
> +#endif

IMHO adding it in specific driver is a wrong direction. I'm afraid it
will result in duplication of such macros in code base without clear
reason why.

May be it is better to add it with a proper name to EAL?

>   
>   #define sfc_ef100_tx_err(_txq, ...) \
>   	SFC_DP_LOG(SFC_KVARG_DATAPATH_EF100, ERR, &(_txq)->dp.dpq, __VA_ARGS__)
> @@ -563,8 +567,7 @@ sfc_ef100_tx_pkt_descs_max(const struct rte_mbuf *m)
>   		 * (split into many Tx descriptors).
>   		 */
>   		RTE_BUILD_BUG_ON(SFC_EF100_TX_SEND_DESC_LEN_MAX <
> -				 RTE_MIN((unsigned int)EFX_MAC_PDU_MAX,
> -				 SFC_MBUF_SEG_LEN_MAX));
> +				 MIN((unsigned int)EFX_MAC_PDU_MAX, SFC_MBUF_SEG_LEN_MAX));
>   	}
>   
>   	if (m->ol_flags & sfc_dp_mport_override) {

Re: [PATCH v3 1/5] event/opdl: fix non-constant compile time assertion

[Andrew Rybchenko]

On 1/16/24 21:41, Stephen Hemminger wrote:
> RTE_BUILD_BUG_ON() was being used with a non-constant value.
> The inline function rte_is_power_of_2() is not constant since
> inline expansion happens later in the compile process.
> Replace it with macro which will be constant.
> 
> Fixes: 4236ce9bf5bf ("event/opdl: add OPDL ring infrastructure library")
> Cc: liang.j.ma@intel.com
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---
>   drivers/event/opdl/opdl_ring.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/event/opdl/opdl_ring.c b/drivers/event/opdl/opdl_ring.c
> index 69392b56bbec..24e0bbe3222d 100644
> --- a/drivers/event/opdl/opdl_ring.c
> +++ b/drivers/event/opdl/opdl_ring.c
> @@ -31,6 +31,9 @@
>   #define OPDL_OPA_MASK    (0xFF)
>   #define OPDL_OPA_OFFSET  (0x38)
>   
> +/* Equivalent to rte_is_power_of_2() but as macro. */
> +#define IS_POWER_OF_2(x) (((x) & ((x) - 1)) == 0)

IMHO adding it in specific driver is a wrong direction. I'm afraid it
will result in duplication of such macros in code base without clear
reason why.

May be it is better to add it with a proper name to EAL?

> +
>   /* Types of dependency between stages */
>   enum dep_type {
>   	DEP_NONE = 0,  /* no dependency */
> @@ -910,7 +913,7 @@ opdl_ring_create(const char *name, uint32_t num_slots, uint32_t slot_size,
>   			RTE_CACHE_LINE_MASK) != 0);
>   	RTE_BUILD_BUG_ON((offsetof(struct opdl_ring, slots) &
>   			RTE_CACHE_LINE_MASK) != 0);
> -	RTE_BUILD_BUG_ON(!rte_is_power_of_2(OPDL_DISCLAIMS_PER_LCORE));
> +	RTE_BUILD_BUG_ON(!IS_POWER_OF_2(OPDL_DISCLAIMS_PER_LCORE));
>   
>   	/* Parameter checking */
>   	if (name == NULL) {

Re: [PATCH v3 4/5] mempool: avoid floating point expression in static assertion

[Andrew Rybchenko]

On 1/16/24 21:41, Stephen Hemminger wrote:
> Clang does not handle casts in static_assert() expressions.
> It doesn't like use of floating point to calculate threshold.
> Use a different expression with same effect; yes this will cause
> checkpatch nag.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>   lib/mempool/rte_mempool.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
> index b7a19bea7185..ba3a54cfc298 100644
> --- a/lib/mempool/rte_mempool.c
> +++ b/lib/mempool/rte_mempool.c
> @@ -50,9 +50,7 @@ static void
>   mempool_event_callback_invoke(enum rte_mempool_event event,
>   			      struct rte_mempool *mp);
>   
> -#define CACHE_FLUSHTHRESH_MULTIPLIER 1.5

drivers/net/mlx5/mlx5_rxq.c:1447:        * CACHE_FLUSHTHRESH_MULTIPLIER 
is defined in a C file, so using a

> -#define CALC_CACHE_FLUSHTHRESH(c)	\
> -	((typeof(c))((c) * CACHE_FLUSHTHRESH_MULTIPLIER))
> +#define CALC_CACHE_FLUSHTHRESH(c) ((c) + (c) / 2)

Maybe ((c) * 3 / 2) to avoid double usage of arg in macro?

>   
>   #if defined(RTE_ARCH_X86)
>   /*

Re: [PATCH v3 1/5] event/opdl: fix non-constant compile time assertion

[Bruce Richardson]

On Wed, Jan 17, 2024 at 10:58:12AM +0300, Andrew Rybchenko wrote:
> On 1/16/24 21:41, Stephen Hemminger wrote:
> > RTE_BUILD_BUG_ON() was being used with a non-constant value.
> > The inline function rte_is_power_of_2() is not constant since
> > inline expansion happens later in the compile process.
> > Replace it with macro which will be constant.
> > 
> > Fixes: 4236ce9bf5bf ("event/opdl: add OPDL ring infrastructure library")
> > Cc: liang.j.ma@intel.com
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > ---
> >   drivers/event/opdl/opdl_ring.c | 5 ++++-
> >   1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/event/opdl/opdl_ring.c b/drivers/event/opdl/opdl_ring.c
> > index 69392b56bbec..24e0bbe3222d 100644
> > --- a/drivers/event/opdl/opdl_ring.c
> > +++ b/drivers/event/opdl/opdl_ring.c
> > @@ -31,6 +31,9 @@
> >   #define OPDL_OPA_MASK    (0xFF)
> >   #define OPDL_OPA_OFFSET  (0x38)
> > +/* Equivalent to rte_is_power_of_2() but as macro. */
> > +#define IS_POWER_OF_2(x) (((x) & ((x) - 1)) == 0)
> 
> IMHO adding it in specific driver is a wrong direction. I'm afraid it
> will result in duplication of such macros in code base without clear
> reason why.
> 
> May be it is better to add it with a proper name to EAL?
> 
+1
Even if it's a lower-case name, lets make it a macro in EAL so we can use
it everywhere in asserts.

RE: [PATCH v3 1/5] event/opdl: fix non-constant compile time assertion

[Morten Brørup]

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Wednesday, 17 January 2024 10.27
> 
> On Wed, Jan 17, 2024 at 10:58:12AM +0300, Andrew Rybchenko wrote:
> > On 1/16/24 21:41, Stephen Hemminger wrote:
> > > RTE_BUILD_BUG_ON() was being used with a non-constant value.
> > > The inline function rte_is_power_of_2() is not constant since
> > > inline expansion happens later in the compile process.
> > > Replace it with macro which will be constant.
> > >
> > > Fixes: 4236ce9bf5bf ("event/opdl: add OPDL ring infrastructure
> library")
> > > Cc: liang.j.ma@intel.com
> > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > > Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > ---
> > >   drivers/event/opdl/opdl_ring.c | 5 ++++-
> > >   1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/event/opdl/opdl_ring.c
> b/drivers/event/opdl/opdl_ring.c
> > > index 69392b56bbec..24e0bbe3222d 100644
> > > --- a/drivers/event/opdl/opdl_ring.c
> > > +++ b/drivers/event/opdl/opdl_ring.c
> > > @@ -31,6 +31,9 @@
> > >   #define OPDL_OPA_MASK    (0xFF)
> > >   #define OPDL_OPA_OFFSET  (0x38)
> > > +/* Equivalent to rte_is_power_of_2() but as macro. */
> > > +#define IS_POWER_OF_2(x) (((x) & ((x) - 1)) == 0)
> >
> > IMHO adding it in specific driver is a wrong direction. I'm afraid it
> > will result in duplication of such macros in code base without clear
> > reason why.
> >
> > May be it is better to add it with a proper name to EAL?
> >
> +1
> Even if it's a lower-case name, lets make it a macro in EAL so we can
> use
> it everywhere in asserts.

Here's a thought... we could introduce a new code convention for this purpose. E.g. either:

1. Macros in lower case are guaranteed to access each parameter exactly once (and might not be usable for static_assert; they somewhat resemble inline functions), and
macros in upper case may access parameters any number of times (and must be usable for static_assert).

Or:

2. Macros that access any parameter zero or multiple times must have their name postfixed _UNSAFE (or some other word).

Re: [PATCH v3 5/5] eal: replace out of bounds VLA with static_assert

[Stephen Hemminger]

On Wed, 17 Jan 2024 08:53:44 +0100
Mattias Rönnblom <hofors@lysator.liu.se> wrote:

> >    * Triggers an error at compilation time if the condition is true.
> >    */
> > -#define RTE_BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
> > +#define RTE_BUILD_BUG_ON(condition) { static_assert(!(condition), #condition); }
> >   
> >   /*********** Cache line related macros ********/
> >     
> 
> Should one use RTE_BUILD_BUG_ON() or static_assert() in new DPDK code?
> 
> If static_assert() occasionally needs a work-around, it sounds like 
> keeping using RTE_BUILD_BUG_ON() everywhere is the better choice.

Either choice is the same. Keeping the macro instead of directly using
static_assert will reduce the effort to change when the next C standard
introduces something different.

But using static_assert() can allow for a better warning message.

Re: [PATCH v3 5/5] eal: replace out of bounds VLA with static_assert

[Stephen Hemminger]

On Wed, 17 Jan 2024 10:52:40 +0300
Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru> wrote:

> On 1/16/24 21:41, Stephen Hemminger wrote:
> > Both Gcc, clang and MSVC have better way to do compile time
> > assertions rather than using out of bounds array access.
> > The old method would fail if -Wvla is enabled because compiler
> > can't determine size in that code.  Also, the use of new
> > _Static_assert will catch broken code that is passing non-constant
> > expression to RTE_BUILD_BUG_ON().
> > 
> > Need to add brackets {} around the static_assert() to workaround
> > a bug in clang. Clang was not handling static_assert() in
> > a switch case properly.
> > 
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>  
> 
> Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> 
> Does it imply any limitations on Gcc / Clang versions to
> be used? Is it documented somewhere in DPDK?

No new language versions problems.  There are already direct
usages of static_assert() in some drivers.

[PATCH v4 0/6] use static assert to cathc build errors

[Stephen Hemminger]

This series fixes a couple places where expressions that could not
be evaluated as constant early in compiler passes were used.
Then converts RTE_BUILD_BUG_ON() with static_assert.

static_assert() is more picky about the expression has to
be a constant, which also catches some existing undefined
behavior that pre-existed.

v4 - incorporate review feedback
     add RTE_MIN_T and RTE_MAX_T macros

Stephen Hemminger (6):
  eal: introduce RTE_MIN_T() and RTE_MAX_T() macros
  event/opdl: fix non-constant compile time assertion
  net/sfc: fix non-constant expression in RTE_BUILD_BUG_ON()
  net/i40e: avoid using const variable in assertion
  mempool: avoid floating point expression in static assertion
  eal: replace out of bounds VLA with static_assert

 drivers/event/opdl/opdl_ring.c       |  2 +-
 drivers/net/i40e/i40e_ethdev.h       |  1 +
 drivers/net/i40e/i40e_rxtx_vec_sse.c | 10 ++++------
 drivers/net/mlx5/mlx5_rxq.c          |  2 +-
 drivers/net/sfc/sfc_ef100_tx.c       |  4 +---
 lib/eal/include/rte_common.h         | 19 ++++++++++++++++++-
 lib/mempool/rte_mempool.c            |  7 ++++---
 7 files changed, 30 insertions(+), 15 deletions(-)

-- 
2.43.0

[PATCH v4 1/6] eal: introduce RTE_MIN_T() and RTE_MAX_T() macros

[Stephen Hemminger]

These macros work like RTE_MIN and RTE_MAX but take an explicit
type. Necessary when being used in static assertions since
RTE_MIN and RTE_MAX use temporary variables which confuses
compilers constant expression checks. These macros could also
be useful in other scenarios when bounded range is useful.

Naming is chosen to be similar to Linux kernel conventions.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/eal/include/rte_common.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index c1ba32d00e47..33680e818bfb 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -585,6 +585,14 @@ __extension__ typedef uint64_t RTE_MARKER64[0];
 		_a < _b ? _a : _b; \
 	})
 
+/**
+ * Macro to return the minimum of two numbers
+ * does not use temporarys so not safe if a or b is expression
+ * but is guaranteed to be constant for use in static_assert()
+ */
+#define RTE_MIN_T(a, b, t) \
+	((t)(a) < (t)(b) ? (t)(a) : (t)(b))
+
 /**
  * Macro to return the maximum of two numbers
  */
@@ -595,6 +603,14 @@ __extension__ typedef uint64_t RTE_MARKER64[0];
 		_a > _b ? _a : _b; \
 	})
 
+/**
+ * Macro to return the maxiimum of two numbers
+ * does not use temporarys so not safe if a or b is expression
+ * but is guaranteed to be constant for use in static_assert()
+ */
+#define RTE_MAX_T(a, b, t) \
+	((t)(a) > (t)(b) ? (t)(a) : (t)(b))
+
 /*********** Other general functions / macros ********/
 
 #ifndef offsetof
-- 
2.43.0

[PATCH v4 2/6] event/opdl: fix non-constant compile time assertion

[Stephen Hemminger]

RTE_BUILD_BUG_ON() was being used with a non-constant value.
The inline function rte_is_power_of_2() is not constant since
inline expansion happens later in the compile process.
Replace it with the macro which will be constant.

Fixes: 4236ce9bf5bf ("event/opdl: add OPDL ring infrastructure library")
Cc: liang.j.ma@intel.com
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 drivers/event/opdl/opdl_ring.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/event/opdl/opdl_ring.c b/drivers/event/opdl/opdl_ring.c
index 69392b56bbec..da5ea02d1928 100644
--- a/drivers/event/opdl/opdl_ring.c
+++ b/drivers/event/opdl/opdl_ring.c
@@ -910,7 +910,7 @@ opdl_ring_create(const char *name, uint32_t num_slots, uint32_t slot_size,
 			RTE_CACHE_LINE_MASK) != 0);
 	RTE_BUILD_BUG_ON((offsetof(struct opdl_ring, slots) &
 			RTE_CACHE_LINE_MASK) != 0);
-	RTE_BUILD_BUG_ON(!rte_is_power_of_2(OPDL_DISCLAIMS_PER_LCORE));
+	RTE_BUILD_BUG_ON(!RTE_IS_POWER_OF_2(OPDL_DISCLAIMS_PER_LCORE));
 
 	/* Parameter checking */
 	if (name == NULL) {
-- 
2.43.0

[PATCH v4 3/6] net/sfc: fix non-constant expression in RTE_BUILD_BUG_ON()

[Stephen Hemminger]

The macro RTE_MIN has some hidden assignments to provide type
safety which means the statement can not be fully evaluated in
first pass of compiler. Replace RTE_MIN() with equivalent macro.

Fixes: 4f936666d790 ("net/sfc: support TSO for EF100 native datapath")
Cc: ivan.malov@oktetlabs.ru
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 drivers/net/sfc/sfc_ef100_tx.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/sfc/sfc_ef100_tx.c b/drivers/net/sfc/sfc_ef100_tx.c
index 1b6374775f07..c49d004113d3 100644
--- a/drivers/net/sfc/sfc_ef100_tx.c
+++ b/drivers/net/sfc/sfc_ef100_tx.c
@@ -26,7 +26,6 @@
 #include "sfc_ef100.h"
 #include "sfc_nic_dma_dp.h"
 
-
 #define sfc_ef100_tx_err(_txq, ...) \
 	SFC_DP_LOG(SFC_KVARG_DATAPATH_EF100, ERR, &(_txq)->dp.dpq, __VA_ARGS__)
 
@@ -563,8 +562,7 @@ sfc_ef100_tx_pkt_descs_max(const struct rte_mbuf *m)
 		 * (split into many Tx descriptors).
 		 */
 		RTE_BUILD_BUG_ON(SFC_EF100_TX_SEND_DESC_LEN_MAX <
-				 RTE_MIN((unsigned int)EFX_MAC_PDU_MAX,
-				 SFC_MBUF_SEG_LEN_MAX));
+				 RTE_MIN_T(EFX_MAC_PDU_MAX, SFC_MBUF_SEG_LEN_MAX, uint32_t));
 	}
 
 	if (m->ol_flags & sfc_dp_mport_override) {
-- 
2.43.0

[PATCH v4 4/6] net/i40e: avoid using const variable in assertion

[Stephen Hemminger]

Clang does not allow const variable in a static_assert
expression.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/i40e/i40e_ethdev.h       |  1 +
 drivers/net/i40e/i40e_rxtx_vec_sse.c | 10 ++++------
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
index 1bbe7ad37600..445e1c0b381f 100644
--- a/drivers/net/i40e/i40e_ethdev.h
+++ b/drivers/net/i40e/i40e_ethdev.h
@@ -278,6 +278,7 @@ enum i40e_flxpld_layer_idx {
 #define I40E_DEFAULT_DCB_APP_PRIO   3
 
 #define I40E_FDIR_PRG_PKT_CNT       128
+#define I40E_FDIR_ID_BIT_SHIFT	    13
 
 /*
  * Struct to store flow created.
diff --git a/drivers/net/i40e/i40e_rxtx_vec_sse.c b/drivers/net/i40e/i40e_rxtx_vec_sse.c
index 9200a23ff662..2d4480a7651b 100644
--- a/drivers/net/i40e/i40e_rxtx_vec_sse.c
+++ b/drivers/net/i40e/i40e_rxtx_vec_sse.c
@@ -143,10 +143,9 @@ descs_to_fdir_32b(volatile union i40e_rx_desc *rxdp, struct rte_mbuf **rx_pkt)
 	/* convert fdir_id_mask into a single bit, then shift as required for
 	 * correct location in the mbuf->olflags
 	 */
-	const uint32_t FDIR_ID_BIT_SHIFT = 13;
-	RTE_BUILD_BUG_ON(RTE_MBUF_F_RX_FDIR_ID != (1 << FDIR_ID_BIT_SHIFT));
+	RTE_BUILD_BUG_ON(RTE_MBUF_F_RX_FDIR_ID != (1 << I40E_FDIR_ID_BIT_SHIFT));
 	v_fd_id_mask = _mm_srli_epi32(v_fd_id_mask, 31);
-	v_fd_id_mask = _mm_slli_epi32(v_fd_id_mask, FDIR_ID_BIT_SHIFT);
+	v_fd_id_mask = _mm_slli_epi32(v_fd_id_mask, I40E_FDIR_ID_BIT_SHIFT);
 
 	/* The returned value must be combined into each mbuf. This is already
 	 * being done for RSS and VLAN mbuf olflags, so return bits to OR in.
@@ -205,10 +204,9 @@ descs_to_fdir_16b(__m128i fltstat, __m128i descs[4], struct rte_mbuf **rx_pkt)
 	descs[0] = _mm_blendv_epi8(descs[0], _mm_setzero_si128(), v_desc0_mask);
 
 	/* Shift to 1 or 0 bit per u32 lane, then to RTE_MBUF_F_RX_FDIR_ID offset */
-	const uint32_t FDIR_ID_BIT_SHIFT = 13;
-	RTE_BUILD_BUG_ON(RTE_MBUF_F_RX_FDIR_ID != (1 << FDIR_ID_BIT_SHIFT));
+	RTE_BUILD_BUG_ON(RTE_MBUF_F_RX_FDIR_ID != (1 << I40E_FDIR_ID_BIT_SHIFT));
 	__m128i v_mask_one_bit = _mm_srli_epi32(v_fdir_id_mask, 31);
-	return _mm_slli_epi32(v_mask_one_bit, FDIR_ID_BIT_SHIFT);
+	return _mm_slli_epi32(v_mask_one_bit, I40E_FDIR_ID_BIT_SHIFT);
 }
 #endif
 
-- 
2.43.0

[PATCH v4 5/6] mempool: avoid floating point expression in static assertion

[Stephen Hemminger]

Clang does not handle casts in static_assert() expressions.
It doesn't like use of floating point to calculate threshold.
Use a different expression with same effect.

Modify comment in mlx5 so that developers don't go searching
for old value.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/mlx5/mlx5_rxq.c | 2 +-
 lib/mempool/rte_mempool.c   | 7 ++++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
index 1bb036afebb3..7d972b6d927c 100644
--- a/drivers/net/mlx5/mlx5_rxq.c
+++ b/drivers/net/mlx5/mlx5_rxq.c
@@ -1444,7 +1444,7 @@ mlx5_mprq_alloc_mp(struct rte_eth_dev *dev)
 	/*
 	 * rte_mempool_create_empty() has sanity check to refuse large cache
 	 * size compared to the number of elements.
-	 * CACHE_FLUSHTHRESH_MULTIPLIER is defined in a C file, so using a
+	 * CALC_CACHE_FLUSHTHRESH() is defined in a C file, so using a
 	 * constant number 2 instead.
 	 */
 	obj_num = RTE_MAX(obj_num, MLX5_MPRQ_MP_CACHE_SZ * 2);
diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
index b7a19bea7185..12390a2c8155 100644
--- a/lib/mempool/rte_mempool.c
+++ b/lib/mempool/rte_mempool.c
@@ -50,9 +50,10 @@ static void
 mempool_event_callback_invoke(enum rte_mempool_event event,
 			      struct rte_mempool *mp);
 
-#define CACHE_FLUSHTHRESH_MULTIPLIER 1.5
-#define CALC_CACHE_FLUSHTHRESH(c)	\
-	((typeof(c))((c) * CACHE_FLUSHTHRESH_MULTIPLIER))
+/* Note: avoid using floating point since that compiler
+ * may not think that is constant.
+ */
+#define CALC_CACHE_FLUSHTHRESH(c) (((c) * 3) / 2)
 
 #if defined(RTE_ARCH_X86)
 /*
-- 
2.43.0

[PATCH v4 6/6] eal: replace out of bounds VLA with static_assert

[Stephen Hemminger]

Both Gcc, clang and MSVC have better way to do compile time
assertions rather than using out of bounds array access.
The old method would fail if -Wvla is enabled because compiler
can't determine size in that code.  Also, the use of new
_Static_assert will catch broken code that is passing non-constant
expression to RTE_BUILD_BUG_ON().

Need to add brackets {} around the static_assert() to workaround
a bug in clang. Clang was not handling static_assert() in
a switch case properly.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

---
 lib/eal/include/rte_common.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index 33680e818bfb..f63119b444fa 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -16,6 +16,7 @@
 extern "C" {
 #endif
 
+#include <assert.h>
 #include <stdint.h>
 #include <limits.h>
 
@@ -495,7 +496,7 @@ rte_is_aligned(const void * const __rte_restrict ptr, const unsigned int align)
 /**
  * Triggers an error at compilation time if the condition is true.
  */
-#define RTE_BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
+#define RTE_BUILD_BUG_ON(condition) { static_assert(!(condition), #condition); }
 
 /*********** Cache line related macros ********/
 
-- 
2.43.0

RE: [PATCH v4 5/6] mempool: avoid floating point expression in static assertion

[Morten Brørup]

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Wednesday, 17 January 2024 19.20
> 
> Clang does not handle casts in static_assert() expressions.
> It doesn't like use of floating point to calculate threshold.
> Use a different expression with same effect.
> 
> Modify comment in mlx5 so that developers don't go searching
> for old value.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Acked-by: Morten Brørup <mb@smartsharesystems.com>

Re: [PATCH v4 4/6] net/i40e: avoid using const variable in assertion

[Bruce Richardson]

On Wed, Jan 17, 2024 at 10:19:58AM -0800, Stephen Hemminger wrote:
> Clang does not allow const variable in a static_assert
> expression.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

RE: [PATCH v4 5/6] mempool: avoid floating point expression in static assertion

[Konstantin Ananyev]

> Clang does not handle casts in static_assert() expressions.
> It doesn't like use of floating point to calculate threshold.
> Use a different expression with same effect.
> 
> Modify comment in mlx5 so that developers don't go searching
> for old value.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  drivers/net/mlx5/mlx5_rxq.c | 2 +-
>  lib/mempool/rte_mempool.c   | 7 ++++---
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
> index 1bb036afebb3..7d972b6d927c 100644
> --- a/drivers/net/mlx5/mlx5_rxq.c
> +++ b/drivers/net/mlx5/mlx5_rxq.c
> @@ -1444,7 +1444,7 @@ mlx5_mprq_alloc_mp(struct rte_eth_dev *dev)
>  	/*
>  	 * rte_mempool_create_empty() has sanity check to refuse large cache
>  	 * size compared to the number of elements.
> -	 * CACHE_FLUSHTHRESH_MULTIPLIER is defined in a C file, so using a
> +	 * CALC_CACHE_FLUSHTHRESH() is defined in a C file, so using a
>  	 * constant number 2 instead.
>  	 */
>  	obj_num = RTE_MAX(obj_num, MLX5_MPRQ_MP_CACHE_SZ * 2);
> diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
> index b7a19bea7185..12390a2c8155 100644
> --- a/lib/mempool/rte_mempool.c
> +++ b/lib/mempool/rte_mempool.c
> @@ -50,9 +50,10 @@ static void
>  mempool_event_callback_invoke(enum rte_mempool_event event,
>  			      struct rte_mempool *mp);
> 
> -#define CACHE_FLUSHTHRESH_MULTIPLIER 1.5
> -#define CALC_CACHE_FLUSHTHRESH(c)	\
> -	((typeof(c))((c) * CACHE_FLUSHTHRESH_MULTIPLIER))
> +/* Note: avoid using floating point since that compiler
> + * may not think that is constant.
> + */
> +#define CALC_CACHE_FLUSHTHRESH(c) (((c) * 3) / 2)
> 
>  #if defined(RTE_ARCH_X86)
>  /*

Acked-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>

> --
> 2.43.0

RE: [PATCH v4 4/6] net/i40e: avoid using const variable in assertion

[Konstantin Ananyev]

> Clang does not allow const variable in a static_assert
> expression.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  drivers/net/i40e/i40e_ethdev.h       |  1 +
>  drivers/net/i40e/i40e_rxtx_vec_sse.c | 10 ++++------
>  2 files changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
> index 1bbe7ad37600..445e1c0b381f 100644
> --- a/drivers/net/i40e/i40e_ethdev.h
> +++ b/drivers/net/i40e/i40e_ethdev.h
> @@ -278,6 +278,7 @@ enum i40e_flxpld_layer_idx {
>  #define I40E_DEFAULT_DCB_APP_PRIO   3
> 
>  #define I40E_FDIR_PRG_PKT_CNT       128
> +#define I40E_FDIR_ID_BIT_SHIFT	    13
> 
>  /*
>   * Struct to store flow created.
> diff --git a/drivers/net/i40e/i40e_rxtx_vec_sse.c b/drivers/net/i40e/i40e_rxtx_vec_sse.c
> index 9200a23ff662..2d4480a7651b 100644
> --- a/drivers/net/i40e/i40e_rxtx_vec_sse.c
> +++ b/drivers/net/i40e/i40e_rxtx_vec_sse.c
> @@ -143,10 +143,9 @@ descs_to_fdir_32b(volatile union i40e_rx_desc *rxdp, struct rte_mbuf **rx_pkt)
>  	/* convert fdir_id_mask into a single bit, then shift as required for
>  	 * correct location in the mbuf->olflags
>  	 */
> -	const uint32_t FDIR_ID_BIT_SHIFT = 13;
> -	RTE_BUILD_BUG_ON(RTE_MBUF_F_RX_FDIR_ID != (1 << FDIR_ID_BIT_SHIFT));
> +	RTE_BUILD_BUG_ON(RTE_MBUF_F_RX_FDIR_ID != (1 << I40E_FDIR_ID_BIT_SHIFT));
>  	v_fd_id_mask = _mm_srli_epi32(v_fd_id_mask, 31);
> -	v_fd_id_mask = _mm_slli_epi32(v_fd_id_mask, FDIR_ID_BIT_SHIFT);
> +	v_fd_id_mask = _mm_slli_epi32(v_fd_id_mask, I40E_FDIR_ID_BIT_SHIFT);
> 
>  	/* The returned value must be combined into each mbuf. This is already
>  	 * being done for RSS and VLAN mbuf olflags, so return bits to OR in.
> @@ -205,10 +204,9 @@ descs_to_fdir_16b(__m128i fltstat, __m128i descs[4], struct rte_mbuf **rx_pkt)
>  	descs[0] = _mm_blendv_epi8(descs[0], _mm_setzero_si128(), v_desc0_mask);
> 
>  	/* Shift to 1 or 0 bit per u32 lane, then to RTE_MBUF_F_RX_FDIR_ID offset */
> -	const uint32_t FDIR_ID_BIT_SHIFT = 13;
> -	RTE_BUILD_BUG_ON(RTE_MBUF_F_RX_FDIR_ID != (1 << FDIR_ID_BIT_SHIFT));
> +	RTE_BUILD_BUG_ON(RTE_MBUF_F_RX_FDIR_ID != (1 << I40E_FDIR_ID_BIT_SHIFT));
>  	__m128i v_mask_one_bit = _mm_srli_epi32(v_fdir_id_mask, 31);
> -	return _mm_slli_epi32(v_mask_one_bit, FDIR_ID_BIT_SHIFT);
> +	return _mm_slli_epi32(v_mask_one_bit, I40E_FDIR_ID_BIT_SHIFT);
>  }
>  #endif
> 
> --

Acked-by: Konstantin Ananyev <konstantin.ananyev@huawei.com> 

> 2.43.0

RE: [PATCH v4 1/6] eal: introduce RTE_MIN_T() and RTE_MAX_T() macros

[Konstantin Ananyev]

> These macros work like RTE_MIN and RTE_MAX but take an explicit
> type. Necessary when being used in static assertions since
> RTE_MIN and RTE_MAX use temporary variables which confuses
> compilers constant expression checks. These macros could also
> be useful in other scenarios when bounded range is useful.
> 
> Naming is chosen to be similar to Linux kernel conventions.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/eal/include/rte_common.h | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
> index c1ba32d00e47..33680e818bfb 100644
> --- a/lib/eal/include/rte_common.h
> +++ b/lib/eal/include/rte_common.h
> @@ -585,6 +585,14 @@ __extension__ typedef uint64_t RTE_MARKER64[0];
>  		_a < _b ? _a : _b; \
>  	})
> 
> +/**
> + * Macro to return the minimum of two numbers
> + * does not use temporarys so not safe if a or b is expression
> + * but is guaranteed to be constant for use in static_assert()
> + */
> +#define RTE_MIN_T(a, b, t) \
> +	((t)(a) < (t)(b) ? (t)(a) : (t)(b))
> +
>  /**
>   * Macro to return the maximum of two numbers
>   */
> @@ -595,6 +603,14 @@ __extension__ typedef uint64_t RTE_MARKER64[0];
>  		_a > _b ? _a : _b; \
>  	})
> 
> +/**
> + * Macro to return the maxiimum of two numbers
> + * does not use temporarys so not safe if a or b is expression
> + * but is guaranteed to be constant for use in static_assert()
> + */
> +#define RTE_MAX_T(a, b, t) \
> +	((t)(a) > (t)(b) ? (t)(a) : (t)(b))
> +
>  /*********** Other general functions / macros ********/
> 
>  #ifndef offsetof
> --

Acked-by: Konstantin Ananyev <konstantin.ananyev@huawei.com> 

> 2.43.0

Re: [PATCH v4 3/6] net/sfc: fix non-constant expression in RTE_BUILD_BUG_ON()

[Andrew Rybchenko]

On 1/17/24 21:19, Stephen Hemminger wrote:
> The macro RTE_MIN has some hidden assignments to provide type
> safety which means the statement can not be fully evaluated in
> first pass of compiler. Replace RTE_MIN() with equivalent macro.
> 
> Fixes: 4f936666d790 ("net/sfc: support TSO for EF100 native datapath")
> Cc: ivan.malov@oktetlabs.ru
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>

One nit below, anyway:
Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

> ---
>   drivers/net/sfc/sfc_ef100_tx.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/net/sfc/sfc_ef100_tx.c b/drivers/net/sfc/sfc_ef100_tx.c
> index 1b6374775f07..c49d004113d3 100644
> --- a/drivers/net/sfc/sfc_ef100_tx.c
> +++ b/drivers/net/sfc/sfc_ef100_tx.c
> @@ -26,7 +26,6 @@
>   #include "sfc_ef100.h"
>   #include "sfc_nic_dma_dp.h"
>   
> -

unrelated change

>   #define sfc_ef100_tx_err(_txq, ...) \
>   	SFC_DP_LOG(SFC_KVARG_DATAPATH_EF100, ERR, &(_txq)->dp.dpq, __VA_ARGS__)
>   
> @@ -563,8 +562,7 @@ sfc_ef100_tx_pkt_descs_max(const struct rte_mbuf *m)
>   		 * (split into many Tx descriptors).
>   		 */
>   		RTE_BUILD_BUG_ON(SFC_EF100_TX_SEND_DESC_LEN_MAX <
> -				 RTE_MIN((unsigned int)EFX_MAC_PDU_MAX,
> -				 SFC_MBUF_SEG_LEN_MAX));
> +				 RTE_MIN_T(EFX_MAC_PDU_MAX, SFC_MBUF_SEG_LEN_MAX, uint32_t));
>   	}
>   
>   	if (m->ol_flags & sfc_dp_mport_override) {

Re: [PATCH v4 5/6] mempool: avoid floating point expression in static assertion

[Andrew Rybchenko]

On 1/17/24 21:32, Morten Brørup wrote:
>> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
>> Sent: Wednesday, 17 January 2024 19.20
>>
>> Clang does not handle casts in static_assert() expressions.
>> It doesn't like use of floating point to calculate threshold.
>> Use a different expression with same effect.
>>
>> Modify comment in mlx5 so that developers don't go searching
>> for old value.
>>
>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> 
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> 

Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

Re: [PATCH v4 2/6] event/opdl: fix non-constant compile time assertion

[Andrew Rybchenko]

On 1/17/24 21:19, Stephen Hemminger wrote:
> RTE_BUILD_BUG_ON() was being used with a non-constant value.
> The inline function rte_is_power_of_2() is not constant since
> inline expansion happens later in the compile process.
> Replace it with the macro which will be constant.
> 
> Fixes: 4236ce9bf5bf ("event/opdl: add OPDL ring infrastructure library")
> Cc: liang.j.ma@intel.com
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>

Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

Re: [PATCH v4 1/6] eal: introduce RTE_MIN_T() and RTE_MAX_T() macros

[Andrew Rybchenko]

On 1/17/24 21:19, Stephen Hemminger wrote:
> These macros work like RTE_MIN and RTE_MAX but take an explicit
> type. Necessary when being used in static assertions since
> RTE_MIN and RTE_MAX use temporary variables which confuses
> compilers constant expression checks. These macros could also
> be useful in other scenarios when bounded range is useful.
> 
> Naming is chosen to be similar to Linux kernel conventions.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

[PATCH v5 0/6] use static_assert for build error reports

[Stephen Hemminger]

This series fixes a couple places where expressions that could not
be evaluated as constant early in compiler passes were used.
Then converts RTE_BUILD_BUG_ON() with static_assert.

static_assert() is more picky about the expression has to
be a constant, which also catches some existing undefined
behavior that pre-existed.

The series requires a couple of workarounds to deal
with quirks in static_assert() in some toolchains.

v6 - minor cleanups
     handle missing macro in old FreeBSD

Stephen Hemminger (6):
  eal: introduce RTE_MIN_T() and RTE_MAX_T() macros
  event/opdl: fix non-constant compile time assertion
  net/sfc: fix non-constant expression in RTE_BUILD_BUG_ON()
  net/i40e: avoid using const variable in assertion
  mempool: avoid floating point expression in static assertion
  eal: replace out of bounds VLA with static_assert

 drivers/event/opdl/opdl_ring.c       |  2 +-
 drivers/net/i40e/i40e_ethdev.h       |  1 +
 drivers/net/i40e/i40e_rxtx_vec_sse.c | 10 ++++------
 drivers/net/mlx5/mlx5_rxq.c          |  2 +-
 drivers/net/sfc/sfc_ef100_tx.c       |  3 +--
 lib/eal/include/rte_common.h         | 27 ++++++++++++++++++++++++++-
 lib/mempool/rte_mempool.c            |  7 ++++---
 7 files changed, 38 insertions(+), 14 deletions(-)

-- 
2.43.0

[PATCH v5 1/6] eal: introduce RTE_MIN_T() and RTE_MAX_T() macros

[Stephen Hemminger]

These macros work like RTE_MIN and RTE_MAX but take an explicit
type. Necessary when being used in static assertions since
RTE_MIN and RTE_MAX use temporary variables which confuses
compilers constant expression checks. These macros could also
be useful in other scenarios when bounded range is useful.

Naming is chosen to be similar to Linux kernel conventions.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>
Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
---
 lib/eal/include/rte_common.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index c1ba32d00e47..33680e818bfb 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -585,6 +585,14 @@ __extension__ typedef uint64_t RTE_MARKER64[0];
 		_a < _b ? _a : _b; \
 	})
 
+/**
+ * Macro to return the minimum of two numbers
+ * does not use temporarys so not safe if a or b is expression
+ * but is guaranteed to be constant for use in static_assert()
+ */
+#define RTE_MIN_T(a, b, t) \
+	((t)(a) < (t)(b) ? (t)(a) : (t)(b))
+
 /**
  * Macro to return the maximum of two numbers
  */
@@ -595,6 +603,14 @@ __extension__ typedef uint64_t RTE_MARKER64[0];
 		_a > _b ? _a : _b; \
 	})
 
+/**
+ * Macro to return the maxiimum of two numbers
+ * does not use temporarys so not safe if a or b is expression
+ * but is guaranteed to be constant for use in static_assert()
+ */
+#define RTE_MAX_T(a, b, t) \
+	((t)(a) > (t)(b) ? (t)(a) : (t)(b))
+
 /*********** Other general functions / macros ********/
 
 #ifndef offsetof
-- 
2.43.0

[PATCH v5 2/6] event/opdl: fix non-constant compile time assertion

[Stephen Hemminger]

RTE_BUILD_BUG_ON() was being used with a non-constant value.
The inline function rte_is_power_of_2() is not constant since
inline expansion happens later in the compile process.
Replace it with the macro which will be constant.

Fixes: 4236ce9bf5bf ("event/opdl: add OPDL ring infrastructure library")
Cc: liang.j.ma@intel.com
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
---
 drivers/event/opdl/opdl_ring.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/event/opdl/opdl_ring.c b/drivers/event/opdl/opdl_ring.c
index 69392b56bbec..da5ea02d1928 100644
--- a/drivers/event/opdl/opdl_ring.c
+++ b/drivers/event/opdl/opdl_ring.c
@@ -910,7 +910,7 @@ opdl_ring_create(const char *name, uint32_t num_slots, uint32_t slot_size,
 			RTE_CACHE_LINE_MASK) != 0);
 	RTE_BUILD_BUG_ON((offsetof(struct opdl_ring, slots) &
 			RTE_CACHE_LINE_MASK) != 0);
-	RTE_BUILD_BUG_ON(!rte_is_power_of_2(OPDL_DISCLAIMS_PER_LCORE));
+	RTE_BUILD_BUG_ON(!RTE_IS_POWER_OF_2(OPDL_DISCLAIMS_PER_LCORE));
 
 	/* Parameter checking */
 	if (name == NULL) {
-- 
2.43.0

[PATCH v5 3/6] net/sfc: fix non-constant expression in RTE_BUILD_BUG_ON()

[Stephen Hemminger]

The macro RTE_MIN has some hidden assignments to provide type
safety which means the statement can not be fully evaluated in
first pass of compiler. Replace RTE_MIN() with equivalent macro.

Fixes: 4f936666d790 ("net/sfc: support TSO for EF100 native datapath")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
---
 drivers/net/sfc/sfc_ef100_tx.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/sfc/sfc_ef100_tx.c b/drivers/net/sfc/sfc_ef100_tx.c
index 1b6374775f07..c88ab964547e 100644
--- a/drivers/net/sfc/sfc_ef100_tx.c
+++ b/drivers/net/sfc/sfc_ef100_tx.c
@@ -563,8 +563,7 @@ sfc_ef100_tx_pkt_descs_max(const struct rte_mbuf *m)
 		 * (split into many Tx descriptors).
 		 */
 		RTE_BUILD_BUG_ON(SFC_EF100_TX_SEND_DESC_LEN_MAX <
-				 RTE_MIN((unsigned int)EFX_MAC_PDU_MAX,
-				 SFC_MBUF_SEG_LEN_MAX));
+				 RTE_MIN_T(EFX_MAC_PDU_MAX, SFC_MBUF_SEG_LEN_MAX, uint32_t));
 	}
 
 	if (m->ol_flags & sfc_dp_mport_override) {
-- 
2.43.0

[PATCH v5 4/6] net/i40e: avoid using const variable in assertion

[Stephen Hemminger]

Clang does not allow const variable in a static_assert
expression.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>
---
 drivers/net/i40e/i40e_ethdev.h       |  1 +
 drivers/net/i40e/i40e_rxtx_vec_sse.c | 10 ++++------
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
index 1bbe7ad37600..445e1c0b381f 100644
--- a/drivers/net/i40e/i40e_ethdev.h
+++ b/drivers/net/i40e/i40e_ethdev.h
@@ -278,6 +278,7 @@ enum i40e_flxpld_layer_idx {
 #define I40E_DEFAULT_DCB_APP_PRIO   3
 
 #define I40E_FDIR_PRG_PKT_CNT       128
+#define I40E_FDIR_ID_BIT_SHIFT	    13
 
 /*
  * Struct to store flow created.
diff --git a/drivers/net/i40e/i40e_rxtx_vec_sse.c b/drivers/net/i40e/i40e_rxtx_vec_sse.c
index 9200a23ff662..2d4480a7651b 100644
--- a/drivers/net/i40e/i40e_rxtx_vec_sse.c
+++ b/drivers/net/i40e/i40e_rxtx_vec_sse.c
@@ -143,10 +143,9 @@ descs_to_fdir_32b(volatile union i40e_rx_desc *rxdp, struct rte_mbuf **rx_pkt)
 	/* convert fdir_id_mask into a single bit, then shift as required for
 	 * correct location in the mbuf->olflags
 	 */
-	const uint32_t FDIR_ID_BIT_SHIFT = 13;
-	RTE_BUILD_BUG_ON(RTE_MBUF_F_RX_FDIR_ID != (1 << FDIR_ID_BIT_SHIFT));
+	RTE_BUILD_BUG_ON(RTE_MBUF_F_RX_FDIR_ID != (1 << I40E_FDIR_ID_BIT_SHIFT));
 	v_fd_id_mask = _mm_srli_epi32(v_fd_id_mask, 31);
-	v_fd_id_mask = _mm_slli_epi32(v_fd_id_mask, FDIR_ID_BIT_SHIFT);
+	v_fd_id_mask = _mm_slli_epi32(v_fd_id_mask, I40E_FDIR_ID_BIT_SHIFT);
 
 	/* The returned value must be combined into each mbuf. This is already
 	 * being done for RSS and VLAN mbuf olflags, so return bits to OR in.
@@ -205,10 +204,9 @@ descs_to_fdir_16b(__m128i fltstat, __m128i descs[4], struct rte_mbuf **rx_pkt)
 	descs[0] = _mm_blendv_epi8(descs[0], _mm_setzero_si128(), v_desc0_mask);
 
 	/* Shift to 1 or 0 bit per u32 lane, then to RTE_MBUF_F_RX_FDIR_ID offset */
-	const uint32_t FDIR_ID_BIT_SHIFT = 13;
-	RTE_BUILD_BUG_ON(RTE_MBUF_F_RX_FDIR_ID != (1 << FDIR_ID_BIT_SHIFT));
+	RTE_BUILD_BUG_ON(RTE_MBUF_F_RX_FDIR_ID != (1 << I40E_FDIR_ID_BIT_SHIFT));
 	__m128i v_mask_one_bit = _mm_srli_epi32(v_fdir_id_mask, 31);
-	return _mm_slli_epi32(v_mask_one_bit, FDIR_ID_BIT_SHIFT);
+	return _mm_slli_epi32(v_mask_one_bit, I40E_FDIR_ID_BIT_SHIFT);
 }
 #endif
 
-- 
2.43.0

[PATCH v5 5/6] mempool: avoid floating point expression in static assertion

[Stephen Hemminger]

Clang does not handle casts in static_assert() expressions.
It doesn't like use of floating point to calculate threshold.
Use a different expression with same effect.

Modify comment in mlx5 so that developers don't go searching
for old value.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>
Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
---
 drivers/net/mlx5/mlx5_rxq.c | 2 +-
 lib/mempool/rte_mempool.c   | 7 ++++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
index 1bb036afebb3..ca2eeedc9de3 100644
--- a/drivers/net/mlx5/mlx5_rxq.c
+++ b/drivers/net/mlx5/mlx5_rxq.c
@@ -1444,7 +1444,7 @@ mlx5_mprq_alloc_mp(struct rte_eth_dev *dev)
 	/*
 	 * rte_mempool_create_empty() has sanity check to refuse large cache
 	 * size compared to the number of elements.
-	 * CACHE_FLUSHTHRESH_MULTIPLIER is defined in a C file, so using a
+	 * CALC_CACHE_FLUSHTHRESH() is defined in a C file, so using a
 	 * constant number 2 instead.
 	 */
 	obj_num = RTE_MAX(obj_num, MLX5_MPRQ_MP_CACHE_SZ * 2);
diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
index b7a19bea7185..12390a2c8155 100644
--- a/lib/mempool/rte_mempool.c
+++ b/lib/mempool/rte_mempool.c
@@ -50,9 +50,10 @@ static void
 mempool_event_callback_invoke(enum rte_mempool_event event,
 			      struct rte_mempool *mp);
 
-#define CACHE_FLUSHTHRESH_MULTIPLIER 1.5
-#define CALC_CACHE_FLUSHTHRESH(c)	\
-	((typeof(c))((c) * CACHE_FLUSHTHRESH_MULTIPLIER))
+/* Note: avoid using floating point since that compiler
+ * may not think that is constant.
+ */
+#define CALC_CACHE_FLUSHTHRESH(c) (((c) * 3) / 2)
 
 #if defined(RTE_ARCH_X86)
 /*
-- 
2.43.0

[PATCH v5 6/6] eal: replace out of bounds VLA with static_assert

[Stephen Hemminger]

Both Gcc, clang and MSVC have better way to do compile time
assertions rather than using out of bounds array access.
The old method would fail if -Wvla is enabled because compiler
can't determine size in that code.  Also, the use of new
_Static_assert will catch broken code that is passing non-constant
expression to RTE_BUILD_BUG_ON().

Add workaround for clang static_assert in switch,
and missing static_assert in older FreeBSD.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/eal/include/rte_common.h | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index 33680e818bfb..aa066125b6cd 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -16,6 +16,7 @@
 extern "C" {
 #endif
 
+#include <assert.h>
 #include <stdint.h>
 #include <limits.h>
 
@@ -492,10 +493,18 @@ rte_is_aligned(const void * const __rte_restrict ptr, const unsigned int align)
 
 /*********** Macros for compile type checks ********/
 
+/* Workaround for toolchain issues with missing C11 macro in FreeBSD */
+#if !defined(static_assert) && !defined(__cplusplus)
+#define	static_assert	_Static_assert
+#endif
+
 /**
  * Triggers an error at compilation time if the condition is true.
+ *
+ * The do { } while(0) exists to workaround a bug in clang (#55821)
+ * where it would not handle _Static_assert in a switch case.
  */
-#define RTE_BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
+#define RTE_BUILD_BUG_ON(condition) do { static_assert(!(condition), #condition); } while (0)
 
 /*********** Cache line related macros ********/
 
-- 
2.43.0

RE: [PATCH v5 6/6] eal: replace out of bounds VLA with static_assert

[Morten Brørup]

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Thursday, 18 January 2024 17.51
> 
> Both Gcc, clang and MSVC have better way to do compile time
> assertions rather than using out of bounds array access.
> The old method would fail if -Wvla is enabled because compiler
> can't determine size in that code.  Also, the use of new
> _Static_assert will catch broken code that is passing non-constant
> expression to RTE_BUILD_BUG_ON().
> 
> Add workaround for clang static_assert in switch,
> and missing static_assert in older FreeBSD.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---

Reviewed-by: Morten Brørup <mb@smartsharesystems.com>

RE: [PATCH v5 5/6] mempool: avoid floating point expression in static assertion

[Morten Brørup]

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Thursday, 18 January 2024 17.51
> 
> Clang does not handle casts in static_assert() expressions.
> It doesn't like use of floating point to calculate threshold.
> Use a different expression with same effect.
> 
> Modify comment in mlx5 so that developers don't go searching
> for old value.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>
> Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> ---

Reviewed-by: Morten Brørup <mb@smartsharesystems.com>

Re: [PATCH v5 1/6] eal: introduce RTE_MIN_T() and RTE_MAX_T() macros

[fengchengwen]

On 2024/1/19 0:50, Stephen Hemminger wrote:
> These macros work like RTE_MIN and RTE_MAX but take an explicit
> type. Necessary when being used in static assertions since
> RTE_MIN and RTE_MAX use temporary variables which confuses
> compilers constant expression checks. These macros could also
> be useful in other scenarios when bounded range is useful.
> 
> Naming is chosen to be similar to Linux kernel conventions.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>
> Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> ---

Acked-by: Chengwen Feng <fengchengwen@huawei.com>

RE: [PATCH v5 5/6] mempool: avoid floating point expression in static assertion

[Slava Ovsiienko]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Thursday, January 18, 2024 6:51 PM
> To: dev@dpdk.org
> Cc: Stephen Hemminger <stephen@networkplumber.org>; Konstantin Ananyev
> <konstantin.ananyev@huawei.com>; Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>; Dariusz Sosnowski
> <dsosnowski@nvidia.com>; Slava Ovsiienko <viacheslavo@nvidia.com>; Ori
> Kam <orika@nvidia.com>; Suanming Mou <suanmingm@nvidia.com>; Matan
> Azrad <matan@nvidia.com>; Morten Brørup <mb@smartsharesystems.com>
> Subject: [PATCH v5 5/6] mempool: avoid floating point expression in static
> assertion
> 
> Clang does not handle casts in static_assert() expressions.
> It doesn't like use of floating point to calculate threshold.
> Use a different expression with same effect.
> 
> Modify comment in mlx5 so that developers don't go searching for old value.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>
> Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>

Re: [PATCH v5 6/6] eal: replace out of bounds VLA with static_assert

[Ferruh Yigit]

On 1/18/2024 6:42 PM, Morten Brørup wrote:
>> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
>> Sent: Thursday, 18 January 2024 17.51
>>
>> Both Gcc, clang and MSVC have better way to do compile time
>> assertions rather than using out of bounds array access.
>> The old method would fail if -Wvla is enabled because compiler
>> can't determine size in that code.  Also, the use of new
>> _Static_assert will catch broken code that is passing non-constant
>> expression to RTE_BUILD_BUG_ON().
>>
>> Add workaround for clang static_assert in switch,
>> and missing static_assert in older FreeBSD.
>>
>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>> Acked-by: Morten Brørup <mb@smartsharesystems.com>
>> Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
>> ---
> 
> Reviewed-by: Morten Brørup <mb@smartsharesystems.com>
> 

Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>

Re: [PATCH v4 1/6] eal: introduce RTE_MIN_T() and RTE_MAX_T() macros

[Tyler Retzlaff]

On Wed, Jan 17, 2024 at 10:19:55AM -0800, Stephen Hemminger wrote:
> These macros work like RTE_MIN and RTE_MAX but take an explicit
> type. Necessary when being used in static assertions since
> RTE_MIN and RTE_MAX use temporary variables which confuses
> compilers constant expression checks. These macros could also
> be useful in other scenarios when bounded range is useful.
> 
> Naming is chosen to be similar to Linux kernel conventions.

parameter ordering seems weird, also Linux kernel copied? just curious
more than anything. if not i would put 't' as the first parameter.

Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>

> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/eal/include/rte_common.h | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
> index c1ba32d00e47..33680e818bfb 100644
> --- a/lib/eal/include/rte_common.h
> +++ b/lib/eal/include/rte_common.h
> @@ -585,6 +585,14 @@ __extension__ typedef uint64_t RTE_MARKER64[0];
>  		_a < _b ? _a : _b; \
>  	})
>  
> +/**
> + * Macro to return the minimum of two numbers
> + * does not use temporarys so not safe if a or b is expression
> + * but is guaranteed to be constant for use in static_assert()
> + */
> +#define RTE_MIN_T(a, b, t) \
> +	((t)(a) < (t)(b) ? (t)(a) : (t)(b))
> +
>  /**
>   * Macro to return the maximum of two numbers
>   */
> @@ -595,6 +603,14 @@ __extension__ typedef uint64_t RTE_MARKER64[0];
>  		_a > _b ? _a : _b; \
>  	})
>  
> +/**
> + * Macro to return the maxiimum of two numbers
> + * does not use temporarys so not safe if a or b is expression
> + * but is guaranteed to be constant for use in static_assert()
> + */
> +#define RTE_MAX_T(a, b, t) \
> +	((t)(a) > (t)(b) ? (t)(a) : (t)(b))
> +
>  /*********** Other general functions / macros ********/
>  
>  #ifndef offsetof
> -- 
> 2.43.0

Re: [PATCH v4 1/6] eal: introduce RTE_MIN_T() and RTE_MAX_T() macros

[Stephen Hemminger]

On Fri, 19 Jan 2024 12:58:47 -0800
Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:

> On Wed, Jan 17, 2024 at 10:19:55AM -0800, Stephen Hemminger wrote:
> > These macros work like RTE_MIN and RTE_MAX but take an explicit
> > type. Necessary when being used in static assertions since
> > RTE_MIN and RTE_MAX use temporary variables which confuses
> > compilers constant expression checks. These macros could also
> > be useful in other scenarios when bounded range is useful.
> > 
> > Naming is chosen to be similar to Linux kernel conventions.  
> 
> parameter ordering seems weird, also Linux kernel copied? just curious
> more than anything. if not i would put 't' as the first parameter.
> 
> Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>

Hmm, kernel version takes type first will use that.

Re: [PATCH v2 3/3] eal: replace out of bounds VLA with static_assert

[Tyler Retzlaff]

ping

i'd like to see this change go in asap since it is pre-requisite to
turning on -Wvla which explicitly caught use of non-constant expressions
in the RTE_BUILD_BUG_ON() hiding bugs.

thanks!

On Mon, Nov 13, 2023 at 09:06:05AM -0800, Stephen Hemminger wrote:
> Both Gcc, clang and MSVC have better way to do compile time
> assertions rather than using out of bounds array access.
> The old method would fail if -Wvla is enabled because compiler
> can't determine size in that code.  Also, the use of new
> _Static_assert will catch broken code that is passing non-constant
> expression to RTE_BUILD_BUG_ON().
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---
>  lib/eal/include/rte_common.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
> index c1ba32d00e47..bea7c0e57d5e 100644
> --- a/lib/eal/include/rte_common.h
> +++ b/lib/eal/include/rte_common.h
> @@ -16,6 +16,7 @@
>  extern "C" {
>  #endif
>  
> +#include <assert.h>
>  #include <stdint.h>
>  #include <limits.h>
>  
> @@ -495,7 +496,7 @@ rte_is_aligned(const void * const __rte_restrict ptr, const unsigned int align)
>  /**
>   * Triggers an error at compilation time if the condition is true.
>   */
> -#define RTE_BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
> +#define RTE_BUILD_BUG_ON(condition) static_assert(!(condition), #condition)
>  
>  /*********** Cache line related macros ********/
>  
> -- 
> 2.39.2

RE: [PATCH v2 3/3] eal: replace out of bounds VLA with static_assert

[Morten Brørup]

> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> Sent: Friday, 16 February 2024 01.34
> 
> ping
> 
> i'd like to see this change go in asap since it is pre-requisite to
> turning on -Wvla which explicitly caught use of non-constant
> expressions
> in the RTE_BUILD_BUG_ON() hiding bugs.
> 
> thanks!
> 
> On Mon, Nov 13, 2023 at 09:06:05AM -0800, Stephen Hemminger wrote:
> > Both Gcc, clang and MSVC have better way to do compile time
> > assertions rather than using out of bounds array access.
> > The old method would fail if -Wvla is enabled because compiler
> > can't determine size in that code.  Also, the use of new
> > _Static_assert will catch broken code that is passing non-constant
> > expression to RTE_BUILD_BUG_ON().
> >
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > ---
> >  lib/eal/include/rte_common.h | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/eal/include/rte_common.h
> b/lib/eal/include/rte_common.h
> > index c1ba32d00e47..bea7c0e57d5e 100644
> > --- a/lib/eal/include/rte_common.h
> > +++ b/lib/eal/include/rte_common.h
> > @@ -16,6 +16,7 @@
> >  extern "C" {
> >  #endif
> >
> > +#include <assert.h>
> >  #include <stdint.h>
> >  #include <limits.h>
> >
> > @@ -495,7 +496,7 @@ rte_is_aligned(const void * const __rte_restrict
> ptr, const unsigned int align)
> >  /**
> >   * Triggers an error at compilation time if the condition is true.
> >   */
> > -#define RTE_BUILD_BUG_ON(condition) ((void)sizeof(char[1 -
> 2*!!(condition)]))
> > +#define RTE_BUILD_BUG_ON(condition) static_assert(!(condition),
> #condition)
> >
> >  /*********** Cache line related macros ********/
> >
> > --
> > 2.39.2

Reviewed-by: Morten Brørup <mb@smartsharesystems.com>

Re: [PATCH v2 3/3] eal: replace out of bounds VLA with static_assert

[David Marchand]

On Fri, Feb 16, 2024 at 1:33 AM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
>
> ping
>
> i'd like to see this change go in asap since it is pre-requisite to
> turning on -Wvla which explicitly caught use of non-constant expressions
> in the RTE_BUILD_BUG_ON() hiding bugs.

That was the last thing I applied yesterday.
It ran through compile checks during the night.

I am reviewing a few details this morning before pushing.


-- 
David Marchand

Re: [PATCH v5 0/6] use static_assert for build error reports

[David Marchand]

On Thu, Jan 18, 2024 at 5:53 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> This series fixes a couple places where expressions that could not
> be evaluated as constant early in compiler passes were used.
> Then converts RTE_BUILD_BUG_ON() with static_assert.
>
> static_assert() is more picky about the expression has to
> be a constant, which also catches some existing undefined
> behavior that pre-existed.
>
> The series requires a couple of workarounds to deal
> with quirks in static_assert() in some toolchains.
>
> v6 - minor cleanups
>      handle missing macro in old FreeBSD
>
> Stephen Hemminger (6):
>   eal: introduce RTE_MIN_T() and RTE_MAX_T() macros
>   event/opdl: fix non-constant compile time assertion
>   net/sfc: fix non-constant expression in RTE_BUILD_BUG_ON()
>   net/i40e: avoid using const variable in assertion
>   mempool: avoid floating point expression in static assertion
>   eal: replace out of bounds VLA with static_assert
>
>  drivers/event/opdl/opdl_ring.c       |  2 +-
>  drivers/net/i40e/i40e_ethdev.h       |  1 +
>  drivers/net/i40e/i40e_rxtx_vec_sse.c | 10 ++++------
>  drivers/net/mlx5/mlx5_rxq.c          |  2 +-
>  drivers/net/sfc/sfc_ef100_tx.c       |  3 +--
>  lib/eal/include/rte_common.h         | 27 ++++++++++++++++++++++++++-
>  lib/mempool/rte_mempool.c            |  7 ++++---
>  7 files changed, 38 insertions(+), 14 deletions(-)

Added a small RN and applied, thanks Stephen.


-- 
David Marchand

Re: [PATCH v2 3/3] eal: replace out of bounds VLA with static_assert

[Tyler Retzlaff]

On Fri, Feb 16, 2024 at 09:02:29AM +0100, David Marchand wrote:
> On Fri, Feb 16, 2024 at 1:33 AM Tyler Retzlaff
> <roretzla@linux.microsoft.com> wrote:
> >
> > ping
> >
> > i'd like to see this change go in asap since it is pre-requisite to
> > turning on -Wvla which explicitly caught use of non-constant expressions
> > in the RTE_BUILD_BUG_ON() hiding bugs.
> 
> That was the last thing I applied yesterday.
> It ran through compile checks during the night.
> 
> I am reviewing a few details this morning before pushing.

thank you!

> 
> 
> -- 
> David Marchand