[v2,3/3] 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().
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 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.
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?
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
> 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>
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.
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
@@ -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 ********/