DPDK patches and discussions
 help / color / mirror / Atom feed
* [PATCH] common/sfc: replace out of bounds condition with static_assert
@ 2024-01-18 20:18 Stephen Hemminger
  2024-01-18 23:05 ` Morten Brørup
                   ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: Stephen Hemminger @ 2024-01-18 20:18 UTC (permalink / raw)
  To: dev; +Cc: Stephen Hemminger, Andrew Rybchenko

The sfc base code had its own definition of static assertions
using the out of bound array access hack. Replace it with a
static_assert like rte_common.h.

Fixes: f67e4719147d ("net/sfc/base: fix coding style")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/common/sfc_efx/base/efx.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/common/sfc_efx/base/efx.h b/drivers/common/sfc_efx/base/efx.h
index 3312c2fa8f81..9ce266c43610 100644
--- a/drivers/common/sfc_efx/base/efx.h
+++ b/drivers/common/sfc_efx/base/efx.h
@@ -17,8 +17,8 @@
 extern "C" {
 #endif
 
-#define	EFX_STATIC_ASSERT(_cond)		\
-	((void)sizeof (char[(_cond) ? 1 : -1]))
+#define	EFX_STATIC_ASSERT(_cond) \
+	do { static_assert((_cond), "assert failed" #_cond); } while (0)
 
 #define	EFX_ARRAY_SIZE(_array)			\
 	(sizeof (_array) / sizeof ((_array)[0]))
-- 
2.43.0


^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH] common/sfc: replace out of bounds condition with static_assert
  2024-01-18 20:18 [PATCH] common/sfc: replace out of bounds condition with static_assert Stephen Hemminger
@ 2024-01-18 23:05 ` Morten Brørup
  2024-02-13  7:47   ` Andrew Rybchenko
  2024-01-19 22:13 ` [PATCH v2] " Stephen Hemminger
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 14+ messages in thread
From: Morten Brørup @ 2024-01-18 23:05 UTC (permalink / raw)
  To: Stephen Hemminger, dev; +Cc: Andrew Rybchenko

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Thursday, 18 January 2024 21.18
> 
> The sfc base code had its own definition of static assertions
> using the out of bound array access hack. Replace it with a
> static_assert like rte_common.h.
> 
> Fixes: f67e4719147d ("net/sfc/base: fix coding style")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  drivers/common/sfc_efx/base/efx.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/common/sfc_efx/base/efx.h
> b/drivers/common/sfc_efx/base/efx.h
> index 3312c2fa8f81..9ce266c43610 100644
> --- a/drivers/common/sfc_efx/base/efx.h
> +++ b/drivers/common/sfc_efx/base/efx.h
> @@ -17,8 +17,8 @@
>  extern "C" {
>  #endif
> 
> -#define	EFX_STATIC_ASSERT(_cond)		\
> -	((void)sizeof (char[(_cond) ? 1 : -1]))
> +#define	EFX_STATIC_ASSERT(_cond) \
> +	do { static_assert((_cond), "assert failed" #_cond); } while (0)

This probably works for the DPDK project.

For other projects using the same file, it might also need "#include <assert.h>" (containing the static_assert convenience macro for C), and possibly your workaround for toolchain issues with missing C11 macro in FreeBSD. Maybe not in this file, but somewhere.

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v2] common/sfc: replace out of bounds condition with static_assert
  2024-01-18 20:18 [PATCH] common/sfc: replace out of bounds condition with static_assert Stephen Hemminger
  2024-01-18 23:05 ` Morten Brørup
@ 2024-01-19 22:13 ` Stephen Hemminger
  2024-01-20  7:53   ` Morten Brørup
  2024-02-07 19:10   ` Ferruh Yigit
  2024-02-11 22:24 ` [PATCH] " Stephen Hemminger
  2024-02-12  5:48 ` [PATCH v4] " Stephen Hemminger
  3 siblings, 2 replies; 14+ messages in thread
From: Stephen Hemminger @ 2024-01-19 22:13 UTC (permalink / raw)
  To: dev; +Cc: Stephen Hemminger, Morten Brørup

The sfc base code had its own definition of static assertions
using the out of bound array access hack. Replace it with a
static_assert like rte_common.h.

Fixes: f67e4719147d ("net/sfc/base: fix coding style")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
v2 - add assert.h to make sure it works in other environments

 drivers/common/sfc_efx/base/efx.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/common/sfc_efx/base/efx.h b/drivers/common/sfc_efx/base/efx.h
index 3312c2fa8f81..38f2aed3e336 100644
--- a/drivers/common/sfc_efx/base/efx.h
+++ b/drivers/common/sfc_efx/base/efx.h
@@ -7,6 +7,8 @@
 #ifndef	_SYS_EFX_H
 #define	_SYS_EFX_H
 
+#include <assert.h>
+
 #include "efx_annote.h"
 #include "efsys.h"
 #include "efx_types.h"
@@ -17,8 +19,8 @@
 extern "C" {
 #endif
 
-#define	EFX_STATIC_ASSERT(_cond)		\
-	((void)sizeof (char[(_cond) ? 1 : -1]))
+#define	EFX_STATIC_ASSERT(_cond) \
+	do { static_assert((_cond), "assert failed" #_cond); } while (0)
 
 #define	EFX_ARRAY_SIZE(_array)			\
 	(sizeof (_array) / sizeof ((_array)[0]))
-- 
2.43.0


^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH v2] common/sfc: replace out of bounds condition with static_assert
  2024-01-19 22:13 ` [PATCH v2] " Stephen Hemminger
@ 2024-01-20  7:53   ` Morten Brørup
  2024-02-07 19:10   ` Ferruh Yigit
  1 sibling, 0 replies; 14+ messages in thread
From: Morten Brørup @ 2024-01-20  7:53 UTC (permalink / raw)
  To: Stephen Hemminger, dev

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Friday, 19 January 2024 23.14
> 
> The sfc base code had its own definition of static assertions
> using the out of bound array access hack. Replace it with a
> static_assert like rte_common.h.
> 
> Fixes: f67e4719147d ("net/sfc/base: fix coding style")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> ---

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] common/sfc: replace out of bounds condition with static_assert
  2024-01-19 22:13 ` [PATCH v2] " Stephen Hemminger
  2024-01-20  7:53   ` Morten Brørup
@ 2024-02-07 19:10   ` Ferruh Yigit
  2024-02-07 22:34     ` Stephen Hemminger
  2024-02-07 22:36     ` Stephen Hemminger
  1 sibling, 2 replies; 14+ messages in thread
From: Ferruh Yigit @ 2024-02-07 19:10 UTC (permalink / raw)
  To: Stephen Hemminger, dev; +Cc: Morten Brørup

On 1/19/2024 10:13 PM, Stephen Hemminger wrote:
> The sfc base code had its own definition of static assertions
> using the out of bound array access hack. Replace it with a
> static_assert like rte_common.h.
> 
> Fixes: f67e4719147d ("net/sfc/base: fix coding style")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> ---
> v2 - add assert.h to make sure it works in other environments
> 
>  drivers/common/sfc_efx/base/efx.h | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/common/sfc_efx/base/efx.h b/drivers/common/sfc_efx/base/efx.h
> index 3312c2fa8f81..38f2aed3e336 100644
> --- a/drivers/common/sfc_efx/base/efx.h
> +++ b/drivers/common/sfc_efx/base/efx.h
> @@ -7,6 +7,8 @@
>  #ifndef	_SYS_EFX_H
>  #define	_SYS_EFX_H
>  
> +#include <assert.h>
> +
>  #include "efx_annote.h"
>  #include "efsys.h"
>  #include "efx_types.h"
> @@ -17,8 +19,8 @@
>  extern "C" {
>  #endif
>  
> -#define	EFX_STATIC_ASSERT(_cond)		\
> -	((void)sizeof (char[(_cond) ? 1 : -1]))
> +#define	EFX_STATIC_ASSERT(_cond) \
> +	do { static_assert((_cond), "assert failed" #_cond); } while (0)
>  
>  #define	EFX_ARRAY_SIZE(_array)			\
>  	(sizeof (_array) / sizeof ((_array)[0]))

Getting following build error with clang:

FAILED: drivers/common/sfc_efx/base/libsfc_base.a.p/ef10_filter.c.o

./drivers/common/sfc_efx/base/ef10_filter.c
../drivers/common/sfc_efx/base/ef10_filter.c:503:2: error: static_assert
expression is not an integral constant expression
        EFX_STATIC_ASSERT((EFX_FIELD_OFFSET(efx_filter_spec_t,
efs_outer_vid) %

^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../drivers/common/sfc_efx/base/efx.h:23:21: note: expanded from macro
'EFX_STATIC_ASSERT'
        do { static_assert((_cond), "assert failed" #_cond); } while (0)
                           ^~~~~~~
../drivers/common/sfc_efx/base/ef10_filter.c:503:21: note: cast that
performs the conversions of a reinterpret_cast is not allowed in a
constant expression
        EFX_STATIC_ASSERT((EFX_FIELD_OFFSET(efx_filter_spec_t,
efs_outer_vid) %
                           ^
../drivers/common/sfc_efx/base/efx.h:29:3: note: expanded from macro
'EFX_FIELD_OFFSET'
        ((size_t)&(((_type *)0)->_field))
         ^
../drivers/common/sfc_efx/base/ef10_filter.c:1246:18: error: shift count
>= width of type [-Werror,-Wshift-count-overflow]
        matches_count = MCDI_OUT_DWORD(req,
                        ^~~~~~~~~~~~~~~~~~~
../drivers/common/sfc_efx/base/efx_mcdi.h:493:2: note: expanded from
macro 'MCDI_OUT_DWORD'
        EFX_DWORD_FIELD(*MCDI_OUT2(_emr, efx_dword_t, _ofst),           \
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../drivers/common/sfc_efx/base/efx_types.h:533:30: note: expanded from
macro 'EFX_DWORD_FIELD'
            EFX_HIGH_BIT(_field)) & EFX_MASK32(_field))
                                    ^~~~~~~~~~~~~~~~~~
../drivers/common/sfc_efx/base/efx_types.h:145:23: note: expanded from
macro 'EFX_MASK32'
            (((((uint32_t)1) << EFX_WIDTH(_field))) - 1))
                             ^  ~~~~~~~~~~~~~~~~~
2 errors generated.


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] common/sfc: replace out of bounds condition with static_assert
  2024-02-07 19:10   ` Ferruh Yigit
@ 2024-02-07 22:34     ` Stephen Hemminger
  2024-02-07 22:36     ` Stephen Hemminger
  1 sibling, 0 replies; 14+ messages in thread
From: Stephen Hemminger @ 2024-02-07 22:34 UTC (permalink / raw)
  To: Ferruh Yigit; +Cc: dev, Morten Brørup

On Wed, 7 Feb 2024 19:10:37 +0000
Ferruh Yigit <ferruh.yigit@amd.com> wrote:

> On 1/19/2024 10:13 PM, Stephen Hemminger wrote:
> > The sfc base code had its own definition of static assertions
> > using the out of bound array access hack. Replace it with a
> > static_assert like rte_common.h.
> > 
> > Fixes: f67e4719147d ("net/sfc/base: fix coding style")
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > ---
> > v2 - add assert.h to make sure it works in other environments
> > 
> >  drivers/common/sfc_efx/base/efx.h | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/common/sfc_efx/base/efx.h b/drivers/common/sfc_efx/base/efx.h
> > index 3312c2fa8f81..38f2aed3e336 100644
> > --- a/drivers/common/sfc_efx/base/efx.h
> > +++ b/drivers/common/sfc_efx/base/efx.h
> > @@ -7,6 +7,8 @@
> >  #ifndef	_SYS_EFX_H
> >  #define	_SYS_EFX_H
> >  
> > +#include <assert.h>
> > +
> >  #include "efx_annote.h"
> >  #include "efsys.h"
> >  #include "efx_types.h"
> > @@ -17,8 +19,8 @@
> >  extern "C" {
> >  #endif
> >  
> > -#define	EFX_STATIC_ASSERT(_cond)		\
> > -	((void)sizeof (char[(_cond) ? 1 : -1]))
> > +#define	EFX_STATIC_ASSERT(_cond) \
> > +	do { static_assert((_cond), "assert failed" #_cond); } while (0)
> >  
> >  #define	EFX_ARRAY_SIZE(_array)			\
> >  	(sizeof (_array) / sizeof ((_array)[0]))  
> 
> Getting following build error with clang:

What version of clang?
It works for me with clang 16.0.6

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] common/sfc: replace out of bounds condition with static_assert
  2024-02-07 19:10   ` Ferruh Yigit
  2024-02-07 22:34     ` Stephen Hemminger
@ 2024-02-07 22:36     ` Stephen Hemminger
  2024-02-07 23:30       ` Ferruh Yigit
  1 sibling, 1 reply; 14+ messages in thread
From: Stephen Hemminger @ 2024-02-07 22:36 UTC (permalink / raw)
  To: Ferruh Yigit; +Cc: dev, Morten Brørup

On Wed, 7 Feb 2024 19:10:37 +0000
Ferruh Yigit <ferruh.yigit@amd.com> wrote:

> ../drivers/common/sfc_efx/base/ef10_filter.c:1246:18: error: shift count
> >= width of type [-Werror,-Wshift-count-overflow]  
>         matches_count = MCDI_OUT_DWORD(req,
>                         ^~~~~~~~~~~~~~~~~~~
> ../drivers/common/sfc_efx/base/efx_mcdi.h:493:2: note: expanded from
> macro 'MCDI_OUT_DWORD'
>         EFX_DWORD_FIELD(*MCDI_OUT2(_emr, efx_dword_t, _ofst),           \
>         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../drivers/common/sfc_efx/base/efx_types.h:533:30: note: expanded from
> macro 'EFX_DWORD_FIELD'
>             EFX_HIGH_BIT(_field)) & EFX_MASK32(_field))
>                                     ^~~~~~~~~~~~~~~~~~
> ../drivers/common/sfc_efx/base/efx_types.h:145:23: note: expanded from
> macro 'EFX_MASK32'
>             (((((uint32_t)1) << EFX_WIDTH(_field))) - 1))

None of this got changed by the patch. Looks like it would not compile
even without the patch on your version of clang.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] common/sfc: replace out of bounds condition with static_assert
  2024-02-07 22:36     ` Stephen Hemminger
@ 2024-02-07 23:30       ` Ferruh Yigit
  2024-02-11 17:41         ` Stephen Hemminger
  0 siblings, 1 reply; 14+ messages in thread
From: Ferruh Yigit @ 2024-02-07 23:30 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev, Morten Brørup

On 2/7/2024 10:36 PM, Stephen Hemminger wrote:
> On Wed, 7 Feb 2024 19:10:37 +0000
> Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> 
>> ../drivers/common/sfc_efx/base/ef10_filter.c:1246:18: error: shift count
>>> = width of type [-Werror,-Wshift-count-overflow]  
>>         matches_count = MCDI_OUT_DWORD(req,
>>                         ^~~~~~~~~~~~~~~~~~~
>> ../drivers/common/sfc_efx/base/efx_mcdi.h:493:2: note: expanded from
>> macro 'MCDI_OUT_DWORD'
>>         EFX_DWORD_FIELD(*MCDI_OUT2(_emr, efx_dword_t, _ofst),           \
>>         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> ../drivers/common/sfc_efx/base/efx_types.h:533:30: note: expanded from
>> macro 'EFX_DWORD_FIELD'
>>             EFX_HIGH_BIT(_field)) & EFX_MASK32(_field))
>>                                     ^~~~~~~~~~~~~~~~~~
>> ../drivers/common/sfc_efx/base/efx_types.h:145:23: note: expanded from
>> macro 'EFX_MASK32'
>>             (((((uint32_t)1) << EFX_WIDTH(_field))) - 1))
> 
> None of this got changed by the patch. Looks like it would not compile
> even without the patch on your version of clang.
>

Nope, error only happens with the patch.

And CI seems reporting the errors:
https://mails.dpdk.org/archives/test-report/2024-January/558546.html

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] common/sfc: replace out of bounds condition with static_assert
  2024-02-07 23:30       ` Ferruh Yigit
@ 2024-02-11 17:41         ` Stephen Hemminger
  0 siblings, 0 replies; 14+ messages in thread
From: Stephen Hemminger @ 2024-02-11 17:41 UTC (permalink / raw)
  To: Ferruh Yigit; +Cc: dev, Morten Brørup

On Wed, 7 Feb 2024 23:30:07 +0000
Ferruh Yigit <ferruh.yigit@amd.com> wrote:

> On 2/7/2024 10:36 PM, Stephen Hemminger wrote:
> > On Wed, 7 Feb 2024 19:10:37 +0000
> > Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> >   
> >> ../drivers/common/sfc_efx/base/ef10_filter.c:1246:18: error: shift count  
> >>> = width of type [-Werror,-Wshift-count-overflow]    
> >>         matches_count = MCDI_OUT_DWORD(req,
> >>                         ^~~~~~~~~~~~~~~~~~~
> >> ../drivers/common/sfc_efx/base/efx_mcdi.h:493:2: note: expanded from
> >> macro 'MCDI_OUT_DWORD'
> >>         EFX_DWORD_FIELD(*MCDI_OUT2(_emr, efx_dword_t, _ofst),           \
> >>         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >> ../drivers/common/sfc_efx/base/efx_types.h:533:30: note: expanded from
> >> macro 'EFX_DWORD_FIELD'
> >>             EFX_HIGH_BIT(_field)) & EFX_MASK32(_field))
> >>                                     ^~~~~~~~~~~~~~~~~~
> >> ../drivers/common/sfc_efx/base/efx_types.h:145:23: note: expanded from
> >> macro 'EFX_MASK32'
> >>             (((((uint32_t)1) << EFX_WIDTH(_field))) - 1))  
> > 
> > None of this got changed by the patch. Looks like it would not compile
> > even without the patch on your version of clang.
> >  
> 
> Nope, error only happens with the patch.
> 
> And CI seems reporting the errors:
> https://mails.dpdk.org/archives/test-report/2024-January/558546.html

This patch is allowing compiler to see some things as constant which
were hidden before. The driver is buggy, for example, look at this:

/*
 * Stop lint complaining about some shifts.
 */
#ifdef	__lint
extern int fix_lint;
#define	FIX_LINT(_x)	(_x + fix_lint)
#else
#define	FIX_LINT(_x)	(_x)
#endif

Looks like it is wallpapering over some errors.


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH] common/sfc: replace out of bounds condition with static_assert
  2024-01-18 20:18 [PATCH] common/sfc: replace out of bounds condition with static_assert Stephen Hemminger
  2024-01-18 23:05 ` Morten Brørup
  2024-01-19 22:13 ` [PATCH v2] " Stephen Hemminger
@ 2024-02-11 22:24 ` Stephen Hemminger
  2024-02-12  5:48 ` [PATCH v4] " Stephen Hemminger
  3 siblings, 0 replies; 14+ messages in thread
From: Stephen Hemminger @ 2024-02-11 22:24 UTC (permalink / raw)
  To: dev; +Cc: Stephen Hemminger, Morten Brørup

The sfc base code had its own definition of static assertions
using the out of bound array access hack.  This method does
not force the condition to be const and can have false negatives.
Better to use static_assert() like other places in DPDK.

Fixes: f67e4719147d ("net/sfc/base: fix coding style")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
Reviewed-by: Morten Brørup <mb@smartsharesystems.com>
---
v3 - change the macro to workaround issues with older clang.
     Compiles on Debian testing with Gcc and Clang.

 drivers/common/sfc_efx/base/efx.h | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/common/sfc_efx/base/efx.h b/drivers/common/sfc_efx/base/efx.h
index 3312c2fa8f81..d2fa6bf49396 100644
--- a/drivers/common/sfc_efx/base/efx.h
+++ b/drivers/common/sfc_efx/base/efx.h
@@ -7,6 +7,8 @@
 #ifndef	_SYS_EFX_H
 #define	_SYS_EFX_H
 
+#include <assert.h>
+
 #include "efx_annote.h"
 #include "efsys.h"
 #include "efx_types.h"
@@ -17,8 +19,14 @@
 extern "C" {
 #endif
 
-#define	EFX_STATIC_ASSERT(_cond)		\
-	((void)sizeof (char[(_cond) ? 1 : -1]))
+/*
+ * Triggers an error at compilation time if the condition is false.
+ *
+ * The  { } exists to workaround a bug in clang (#55821)
+ * where it would not handle _Static_assert in a switch case.
+ */
+#define	EFX_STATIC_ASSERT(_cond) \
+	{ static_assert((_cond), #_cond); }
 
 #define	EFX_ARRAY_SIZE(_array)			\
 	(sizeof (_array) / sizeof ((_array)[0]))
-- 
2.43.0


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v4] common/sfc: replace out of bounds condition with static_assert
  2024-01-18 20:18 [PATCH] common/sfc: replace out of bounds condition with static_assert Stephen Hemminger
                   ` (2 preceding siblings ...)
  2024-02-11 22:24 ` [PATCH] " Stephen Hemminger
@ 2024-02-12  5:48 ` Stephen Hemminger
  2024-02-12 12:09   ` Ferruh Yigit
  2024-02-13  9:39   ` Ferruh Yigit
  3 siblings, 2 replies; 14+ messages in thread
From: Stephen Hemminger @ 2024-02-12  5:48 UTC (permalink / raw)
  To: dev; +Cc: Stephen Hemminger, Morten Brørup

The sfc base code had its own definition of static assertions
using the out of bound array access hack. Replace it with a
static_assert like rte_common.h.

The use of null pointer to compute offset is not always a constant
in older versions of clang. Use standard offsetof() instead.

Fixes: f67e4719147d ("net/sfc/base: fix coding style")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
Reviewed-by: Morten Brørup <mb@smartsharesystems.com>
---
 drivers/common/sfc_efx/base/efx.h | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/common/sfc_efx/base/efx.h b/drivers/common/sfc_efx/base/efx.h
index 3312c2fa8f81..5773cb00b3c7 100644
--- a/drivers/common/sfc_efx/base/efx.h
+++ b/drivers/common/sfc_efx/base/efx.h
@@ -7,6 +7,8 @@
 #ifndef	_SYS_EFX_H
 #define	_SYS_EFX_H
 
+#include <assert.h>
+
 #include "efx_annote.h"
 #include "efsys.h"
 #include "efx_types.h"
@@ -17,14 +19,20 @@
 extern "C" {
 #endif
 
-#define	EFX_STATIC_ASSERT(_cond)		\
-	((void)sizeof (char[(_cond) ? 1 : -1]))
+/*
+ * Triggers an error at compilation time if the condition is false.
+ *
+ * The  { } exists to workaround a bug in clang (#55821)
+ * where it would not handle _Static_assert in a switch case.
+ */
+#define	EFX_STATIC_ASSERT(_cond) \
+	{ static_assert((_cond), #_cond); }
 
 #define	EFX_ARRAY_SIZE(_array)			\
 	(sizeof (_array) / sizeof ((_array)[0]))
 
 #define	EFX_FIELD_OFFSET(_type, _field)		\
-	((size_t)&(((_type *)0)->_field))
+	offsetof(_type, _field)
 
 /* The macro expands divider twice */
 #define	EFX_DIV_ROUND_UP(_n, _d)		(((_n) + (_d) - 1) / (_d))
-- 
2.43.0


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v4] common/sfc: replace out of bounds condition with static_assert
  2024-02-12  5:48 ` [PATCH v4] " Stephen Hemminger
@ 2024-02-12 12:09   ` Ferruh Yigit
  2024-02-13  9:39   ` Ferruh Yigit
  1 sibling, 0 replies; 14+ messages in thread
From: Ferruh Yigit @ 2024-02-12 12:09 UTC (permalink / raw)
  To: Stephen Hemminger, dev
  Cc: Morten Brørup, Andrew Rybchenko, Ivan Malov, Andy Moreton

On 2/12/2024 5:48 AM, Stephen Hemminger wrote:
> The sfc base code had its own definition of static assertions
> using the out of bound array access hack. Replace it with a
> static_assert like rte_common.h.
> 
> The use of null pointer to compute offset is not always a constant
> in older versions of clang. Use standard offsetof() instead.
> 
> Fixes: f67e4719147d ("net/sfc/base: fix coding style")> Cc: stable@dpdk.org
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> Reviewed-by: Morten Brørup <mb@smartsharesystems.com>
> 

Looks good to me,
unless there is no objection from driver maintainers I will merge it.


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] common/sfc: replace out of bounds condition with static_assert
  2024-01-18 23:05 ` Morten Brørup
@ 2024-02-13  7:47   ` Andrew Rybchenko
  0 siblings, 0 replies; 14+ messages in thread
From: Andrew Rybchenko @ 2024-02-13  7:47 UTC (permalink / raw)
  To: Morten Brørup, Stephen Hemminger, dev

On 1/19/24 02:05, Morten Brørup wrote:
>> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
>> Sent: Thursday, 18 January 2024 21.18
>>
>> The sfc base code had its own definition of static assertions
>> using the out of bound array access hack. Replace it with a
>> static_assert like rte_common.h.
>>
>> Fixes: f67e4719147d ("net/sfc/base: fix coding style")
>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>> ---
>>   drivers/common/sfc_efx/base/efx.h | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/common/sfc_efx/base/efx.h
>> b/drivers/common/sfc_efx/base/efx.h
>> index 3312c2fa8f81..9ce266c43610 100644
>> --- a/drivers/common/sfc_efx/base/efx.h
>> +++ b/drivers/common/sfc_efx/base/efx.h
>> @@ -17,8 +17,8 @@
>>   extern "C" {
>>   #endif
>>
>> -#define	EFX_STATIC_ASSERT(_cond)		\
>> -	((void)sizeof (char[(_cond) ? 1 : -1]))
>> +#define	EFX_STATIC_ASSERT(_cond) \
>> +	do { static_assert((_cond), "assert failed" #_cond); } while (0)
> 
> This probably works for the DPDK project.
> 
> For other projects using the same file, it might also need "#include <assert.h>" (containing the static_assert convenience macro for C), and possibly your workaround for toolchain issues with missing C11 macro in FreeBSD. Maybe not in this file, but somewhere.
> 
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> 

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v4] common/sfc: replace out of bounds condition with static_assert
  2024-02-12  5:48 ` [PATCH v4] " Stephen Hemminger
  2024-02-12 12:09   ` Ferruh Yigit
@ 2024-02-13  9:39   ` Ferruh Yigit
  1 sibling, 0 replies; 14+ messages in thread
From: Ferruh Yigit @ 2024-02-13  9:39 UTC (permalink / raw)
  To: Stephen Hemminger, dev; +Cc: Morten Brørup

On 2/12/2024 5:48 AM, Stephen Hemminger wrote:
> The sfc base code had its own definition of static assertions
> using the out of bound array access hack. Replace it with a
> static_assert like rte_common.h.
> 
> The use of null pointer to compute offset is not always a constant
> in older versions of clang. Use standard offsetof() instead.
> 
> Fixes: f67e4719147d ("net/sfc/base: fix coding style")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> Reviewed-by: Morten Brørup <mb@smartsharesystems.com>
> 

Moving ack from previous version:
Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

Applied to dpdk-next-net/main, thanks.


^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2024-02-13  9:40 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-18 20:18 [PATCH] common/sfc: replace out of bounds condition with static_assert Stephen Hemminger
2024-01-18 23:05 ` Morten Brørup
2024-02-13  7:47   ` Andrew Rybchenko
2024-01-19 22:13 ` [PATCH v2] " Stephen Hemminger
2024-01-20  7:53   ` Morten Brørup
2024-02-07 19:10   ` Ferruh Yigit
2024-02-07 22:34     ` Stephen Hemminger
2024-02-07 22:36     ` Stephen Hemminger
2024-02-07 23:30       ` Ferruh Yigit
2024-02-11 17:41         ` Stephen Hemminger
2024-02-11 22:24 ` [PATCH] " Stephen Hemminger
2024-02-12  5:48 ` [PATCH v4] " Stephen Hemminger
2024-02-12 12:09   ` Ferruh Yigit
2024-02-13  9:39   ` Ferruh Yigit

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).