diff mbox series

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

Message ID 20231113170605.408281-4-stephen@networkplumber.org (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series use static_assertion for build errors |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/github-robot: build fail github build: failed
ci/Intel-compilation fail Compilation issues
ci/intel-Testing success Testing PASS

Commit Message

Stephen Hemminger Nov. 13, 2023, 5:06 p.m. UTC 
  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

Bruce Richardson Nov. 13, 2023, 5:12 p.m. UTC | #1 
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.
Tyler Retzlaff Nov. 13, 2023, 5:57 p.m. UTC | #2 
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?
Tyler Retzlaff Feb. 16, 2024, 12:33 a.m. UTC | #3 
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
Morten Brørup Feb. 16, 2024, 7:48 a.m. UTC | #4 
> 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>
David Marchand Feb. 16, 2024, 8:02 a.m. UTC | #5 
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.
Tyler Retzlaff Feb. 16, 2024, 8:30 p.m. UTC | #6 
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
diff mbox series

Patch

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 ********/