[v3,5/5] eal: replace out of bounds VLA with static_assert
Checks
Commit Message
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(-)
Comments
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?
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.
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.
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.
@@ -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 ********/