[RFC] eal: use _Static_assert() for RTE_BUILD_BUG_ON
Checks
Commit Message
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(-)
Comments
> 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>
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>
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>
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.
> 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)
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
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.
@@ -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 ********/